Re: [PATCH v2 00/13] arm gicv3 ITS: Various bug fixes and refactorings

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> I've been working on the ITS to add support for the GICv4 functionality.
> In the course of that I found a handful of bugs in it and also some
> places where the code benefited from refactoring to make it a better
> base to put in the GICv4 parts. This patchset is just the bugfixes
> and cleanups, because there are enough patches here that I figured it
> made sense to send them out now rather than holding on to them.
>
> Most of these patches were in v1 and have been reviewed already.

I've reviewed the patches and they look good to me. kvm-unit-tests is
still failing some tests but the ones it fails hasn't changed from
before this patch:

  ✗  env QEMU=$HOME/lsrc/qemu.git/builds/arm.all/qemu-system-aarch64 
./run_tests.sh -g gic
  PASS gicv2-ipi (3 tests)
  PASS gicv2-mmio (17 tests, 1 skipped)
  FAIL gicv2-mmio-up (17 tests, 2 unexpected failures)
  FAIL gicv2-mmio-3p (17 tests, 3 unexpected failures)
  PASS gicv3-ipi (3 tests)
  PASS gicv2-active (1 tests)
  PASS gicv3-active (1 tests)

That said running with -d unimp,guest_errors there are some things that
should probably be double checked, e.g.:

  /home/alex/lsrc/qemu.git/builds/arm.all/qemu-system-aarch64 -nodefaults 
-machine virt -accel tcg -cpu cortex-a57 -device virtio-serial-device -device 
virtconsole,chardev=
  ctd -chardev testdev,id=ctd -device pci-testdev -display none -serial stdio 
-kernel arm/gic.flat -machine gic-version=2 -smp 1 -append "mmio" -d 
unimp,guest_errors
  PASS: gicv2: mmio: all CPUs have interrupts
  gic_dist_readb: Bad offset 8           
  gic_dist_readb: Bad offset 9                                                  
        
  gic_dist_readb: Bad offset a                                                  
        
  gic_dist_readb: Bad offset b                                                  
        
  INFO: gicv2: mmio: IIDR: 0x00000000                                           
        
  gic_dist_writeb: Bad offset 4                                                 
        
  gic_dist_writeb: Bad offset 5                                                 
        
  gic_dist_writeb: Bad offset 6 
  gic_dist_writeb: Bad offset 7 
  gic_dist_writeb: Bad offset 4 
  gic_dist_writeb: Bad offset 5 
  gic_dist_writeb: Bad offset 6  
  gic_dist_writeb: Bad offset 7  
  gic_dist_writeb: Bad offset 4  
  gic_dist_writeb: Bad offset 5  
  gic_dist_writeb: Bad offset 6 
  gic_dist_writeb: Bad offset 7 
  PASS: gicv2: mmio: GICD_TYPER is read-only
  gic_dist_readb: Bad offset 8  
  gic_dist_readb: Bad offset 9   
  gic_dist_readb: Bad offset a   
  gic_dist_readb: Bad offset b   
  gic_dist_writeb: Bad offset 8  
  gic_dist_writeb: Bad offset 9 
  gic_dist_writeb: Bad offset a 
  gic_dist_writeb: Bad offset b 
  gic_dist_readb: Bad offset 8  
  gic_dist_readb: Bad offset 9                                                  
        
  gic_dist_readb: Bad offset a                                                  
        
  gic_dist_readb: Bad offset b                                                  
        
  gic_dist_writeb: Bad offset 8                                                 
        
  gic_dist_writeb: Bad offset 9                                                 
        
  gic_dist_writeb: Bad offset a                                                 
        
  gic_dist_writeb: Bad offset b                                                 
        
  gic_dist_readb: Bad offset 8                                                  
        
  gic_dist_readb: Bad offset 9                                                  
        
  gic_dist_readb: Bad offset a                                                  
        
  gic_dist_readb: Bad offset b                      
  PASS: gicv2: mmio: GICD_IIDR is read-only                                     
        
  gic_dist_writeb: Bad offset fe8                                               
        
  gic_dist_writeb: Bad offset fe9         
  gic_dist_writeb: Bad offset fea
  gic_dist_writeb: Bad offset feb
  gic_dist_writeb: Bad offset fe8
  gic_dist_writeb: Bad offset fe9
  gic_dist_writeb: Bad offset fea
  gic_dist_writeb: Bad offset feb
  gic_dist_writeb: Bad offset fe8
  gic_dist_writeb: Bad offset fe9
  gic_dist_writeb: Bad offset fea
  gic_dist_writeb: Bad offset feb
  PASS: gicv2: mmio: ICPIDR2 is read-only
  INFO: gicv2: mmio: value of ICPIDR2: 0x0000002b
  PASS: gicv2: mmio: IPRIORITYR: consistent priority masking
  INFO: gicv2: mmio: IPRIORITYR: priority mask is 0xffffffff
  PASS: gicv2: mmio: IPRIORITYR: implements at least 4 priority bits
  INFO: gicv2: mmio: IPRIORITYR: 8 priority bits implemented
  PASS: gicv2: mmio: IPRIORITYR: clearing priorities
  gic_dist_readb: Bad offset 520
  gic_dist_readb: Bad offset 521
  gic_dist_readb: Bad offset 522
  gic_dist_readb: Bad offset 523
  gic_dist_writeb: Bad offset 520
  gic_dist_writeb: Bad offset 521
  gic_dist_writeb: Bad offset 522
  gic_dist_writeb: Bad offset 523
  gic_dist_readb: Bad offset 520
  gic_dist_readb: Bad offset 521
  gic_dist_readb: Bad offset 522
  gic_dist_readb: Bad offset 523
  gic_dist_writeb: Bad offset 520
  gic_dist_writeb: Bad offset 521
  gic_dist_writeb: Bad offset 522
  gic_dist_writeb: Bad offset 523
  gic_dist_readb: Bad offset 520
  gic_dist_readb: Bad offset 521
  gic_dist_readb: Bad offset 522
  gic_dist_readb: Bad offset 523
  PASS: gicv2: mmio: IPRIORITYR: accesses beyond limit RAZ/WI
  PASS: gicv2: mmio: IPRIORITYR: accessing last SPIs
  PASS: gicv2: mmio: IPRIORITYR: priorities are preserved
  PASS: gicv2: mmio: IPRIORITYR: byte reads successful
  PASS: gicv2: mmio: IPRIORITYR: byte writes successful
  PASS: gicv2: mmio: ITARGETSR: bits for non-existent CPUs masked
  INFO: gicv2: mmio: ITARGETSR: 7 non-existent CPUs
  PASS: gicv2: mmio: ITARGETSR: accesses beyond limit RAZ/WI
  FAIL: gicv2: mmio: ITARGETSR: register content preserved
  INFO: gicv2: mmio: ITARGETSR: writing 01010001 reads back as 00000000
  PASS: gicv2: mmio: ITARGETSR: byte reads successful
  FAIL: gicv2: mmio: ITARGETSR: byte writes successful
  INFO: gicv2: mmio: ITARGETSR: writing 0x1f into bytes 2 => 0x00000000
  SUMMARY: 17 tests, 2 unexpected failures


>
> Changes from v1:
>  * first half of the series is now upstream
>  * patch 1 now has the '1ULL' and uint64_t fixes that were
>    partly split across two patches in the old series and partly missing
>  * new patches 12 and 13
>
> NB: I left the returns of -1 in patch 11.
>
> Patches still needing review: 1, 12, 13
>
> thanks
> -- PMM
>
> Peter Maydell (13):
>   hw/intc/arm_gicv3_its: Fix event ID bounds checks
>   hw/intc/arm_gicv3_its: Convert int ID check to num_intids convention
>   hw/intc/arm_gicv3_its: Fix handling of process_its_cmd() return value
>   hw/intc/arm_gicv3_its: Don't use data if reading command failed
>   hw/intc/arm_gicv3_its: Use enum for return value of process_*
>     functions
>   hw/intc/arm_gicv3_its: Fix return codes in process_its_cmd()
>   hw/intc/arm_gicv3_its: Refactor process_its_cmd() to reduce nesting
>   hw/intc/arm_gicv3_its: Fix return codes in process_mapti()
>   hw/intc/arm_gicv3_its: Fix return codes in process_mapc()
>   hw/intc/arm_gicv3_its: Fix return codes in process_mapd()
>   hw/intc/arm_gicv3_its: Factor out "find address of table entry" code
>   hw/intc/arm_gicv3_its: Check indexes before use, not after
>   hw/intc/arm_gicv3_its: Range-check ICID before indexing into
>     collection table
>
>  hw/intc/arm_gicv3_its.c | 492 ++++++++++++++++++----------------------
>  1 file changed, 225 insertions(+), 267 deletions(-)


-- 
Alex Bennée