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

[Yi Min Zhao]

在 2017/9/5 下午4:50, Cornelia Huck 写道:
> On Tue, 5 Sep 2017 16:44:37 +0800
> Yi Min Zhao <address@hidden> wrote:
>
> > 在 2017/9/5 下午4:29, 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.)
> > I think this must not happen. One a pci device is plugged into zPCI bus.
> > We would assign a new memory region with zpci device as opaque
> > for its msix. So if s390_msi_ctrl_write() is called, there must be a write
> > operation to a pci device's msix ctrl memory region which must has zpci
> > device as a opaque. The construct is one-msi-mr-per-pci-device.
> This makes sense.
>
> > >   
> > > > +    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.
> > I'm confused. s390_pci_find_dev_by_idx() can be called in
> > kvm_arch_fixup_msi_route().
> > And kvm_arch_fixup_msi_route() can be called by kvm_irqchip_add_msi_route().
> > As the code, I think s390_pci_find_dev_by_idx() might be called. Could
> > you please
> > explain more?
> But this patch replaces this with s390_pci_find_dev_by_target(), no?
Oh! Sorry, I mixed by_target() and by_idx(). Yes, by_idx() should be 
removed.
> > >   
> > > > +
> > > > +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?
> > I think this cannot happen. But I'm afraid that I miss something.
> > So I added this to avoid NULL pointer. But from the code and
> > my test, there has not been NULL pointer happened.
> I'm wondering if that is in the same category as the instance I
> commented on above. Do you want to log something?
For the case above, I ensure that zpci device must exist. But here, I'm 
not sure.
Because it's called from outside. I'm not sure if the caller might call
kvm_irqchip_add_msi_route() with NULL as pci device argument.

Although msix ctrl mr is accessed from outside. But its initialization
is controled by our code and the pointer to zpci device is saved as
mr's opaque.
> > >   
> > > > +    }
> > > > +
> > > > +    pbdev = s390_pci_find_dev_by_target(s390_get_phb(), DEVICE(dev)->id);
> > > >        if (!pbdev) {
> > > >            DPRINTF("add_msi_route no dev\n");
> > > >            return -ENODEV;
> > >