Re: [Qemu-devel] [PATCH v2 1/3] s390x/pci: remove idx from msix msg data

[Cornelia Huck]

On Fri,  1 Sep 2017 06:22:56 +0200
Yi Min Zhao <address@hidden> wrote:

> PCIDevice pointer has been a parameter of kvm_arch_fixup_msi_route().
> So we don't need to store zpci idx in msix message data to find out the
> specific zpci device. Instead, we could use pci device id to find its
> corresponding zpci device.
> 
> Signed-off-by: Yi Min Zhao <address@hidden>
> ---
>  hw/s390x/s390-pci-bus.c  | 16 +++++-----------
>  hw/s390x/s390-pci-bus.h  |  2 ++
>  hw/s390x/s390-pci-inst.c | 24 ------------------------
>  hw/s390x/s390-pci-stub.c |  6 ++++++
>  target/s390x/kvm.c       |  7 +++++--
>  5 files changed, 18 insertions(+), 37 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 0a31a4ae88..bd8a3e1e1c 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -199,8 +199,8 @@ static S390PCIBusDevice 
> *s390_pci_find_dev_by_uid(S390pciState *s, uint16_t uid)
>      return NULL;
>  }
>  
> -static S390PCIBusDevice *s390_pci_find_dev_by_target(S390pciState *s,
> -                                                     const char *target)
> +S390PCIBusDevice *s390_pci_find_dev_by_target(S390pciState *s,
> +                                              const char *target)
>  {
>      S390PCIBusDevice *pbdev;
>  
> @@ -465,19 +465,13 @@ static void s390_msi_ctrl_write(void *opaque, hwaddr 
> addr, uint64_t data,
>                                  unsigned int size)
>  {
>      S390PCIBusDevice *pbdev = opaque;
> -    uint32_t idx = data >> ZPCI_MSI_VEC_BITS;
>      uint32_t vec = data & ZPCI_MSI_VEC_MASK;
>      uint64_t ind_bit;
>      uint32_t sum_bit;
> -    uint32_t e = 0;
>  
> -    DPRINTF("write_msix data 0x%" PRIx64 " idx %d vec 0x%x\n", data, idx, 
> vec);
> -
> -    if (!pbdev) {
> -        e |= (vec << ERR_EVENT_MVN_OFFSET);
> -        s390_pci_generate_error_event(ERR_EVENT_NOMSI, idx, 0, addr, e);
> -        return;
> -    }
> +    assert(pbdev);

I'm wondering whether you could/should generate an error event here.
The one above probably won't work (as it seems to take idx as a
parameter), but is this really 'this must not happen, we messed up in
our code'? (Probably yes, but I want to be sure.)

> +    DPRINTF("write_msix data 0x%" PRIx64 " idx %d vec 0x%x\n", data,
> +            pbdev->idx, vec);
>  
>      if (pbdev->state != ZPCI_FS_ENABLED) {
>          return;

> diff --git a/hw/s390x/s390-pci-stub.c b/hw/s390x/s390-pci-stub.c
> index 7a642d376c..e501e1b9ea 100644
> --- a/hw/s390x/s390-pci-stub.c
> +++ b/hw/s390x/s390-pci-stub.c
> @@ -74,3 +74,9 @@ S390PCIBusDevice *s390_pci_find_dev_by_idx(S390pciState *s, 
> uint32_t idx)
>  {
>      return NULL;
>  }

Please remove s390_pci_find_dev_by_idx() from the stubs file, as it is
not used outside of the conditionally-built pci code anymore.

> +
> +S390PCIBusDevice *s390_pci_find_dev_by_target(S390pciState *s,
> +                                              const char *target)
> +{
> +    return NULL;
> +}
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 1338c29528..3d490c5e4b 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -2533,10 +2533,13 @@ int kvm_arch_fixup_msi_route(struct 
> kvm_irq_routing_entry *route,
>                               uint64_t address, uint32_t data, PCIDevice *dev)
>  {
>      S390PCIBusDevice *pbdev;
> -    uint32_t idx = data >> ZPCI_MSI_VEC_BITS;
>      uint32_t vec = data & ZPCI_MSI_VEC_MASK;
>  
> -    pbdev = s390_pci_find_dev_by_idx(s390_get_phb(), idx);
> +    if (!dev) {
> +        return -ENODEV;

Can this actually happen?

> +    }
> +
> +    pbdev = s390_pci_find_dev_by_target(s390_get_phb(), DEVICE(dev)->id);
>      if (!pbdev) {
>          DPRINTF("add_msi_route no dev\n");
>          return -ENODEV;