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

[Yi Min Zhao]

在 2017/9/5 下午5:15, Cornelia Huck 写道:
> On Tue, 5 Sep 2017 17:08:14 +0800
> Yi Min Zhao <address@hidden> wrote:
>
> > 在 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/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.
> OK. Maybe add a DPRINTF as for the condition below?
OK. How about DPRINTF("add_msi_route no pci device\n")?
And change the DPRINTF for the below condition to
DPRINTF("add_msi_route no zpci device\n").
> > > > >      
> > > > > > +    }
> > > > > > +
> > > > > > +    pbdev = s390_pci_find_dev_by_target(s390_get_phb(), DEVICE(dev)->id);
> > > > > >         if (!pbdev) {
> > > > > >             DPRINTF("add_msi_route no dev\n");
> > > > > >             return -ENODEV;
> > > > >      
> > >