On Tue, 5 Sep 2017 17:21:52 +0800
Yi Min Zhao <zyi...@linux.vnet.ibm.com> wrote:
> 在 2017/9/5 下午5:15, Cornelia Huck 写道:
> > On Tue, 5 Sep 2017 17:08:14 +0800
> > Yi Min Zhao <zyi...@linux.vnet.ibm.com> wrote:
> >
> >> 在 2017/9/5 下午4:50, Cornelia Huck 写道:
> >>> On Tue, 5 Sep 2017 16:44:37 +0800
> >>> Yi Min Zhao <zyi...@linux.vnet.ibm.com> wrote:
> >>>
> >>>> 在 2017/9/5 下午4:29, Cornelia Huck 写道:
> >>>>> On Fri, 1 Sep 2017 06:22:56 +0200
> >>>>> Yi Min Zhao <zyi...@linux.vnet.ibm.com> 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 <zyi...@linux.vnet.ibm.com>
> >>>>>> ---
> >>>>>> 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").
works for me
> >
> >>>>>
> >>>>>> + }
> >>>>>> +
> >>>>>> + pbdev = s390_pci_find_dev_by_target(s390_get_phb(),
> >>>>>> DEVICE(dev)->id);
> >>>>>> if (!pbdev) {
> >>>>>> DPRINTF("add_msi_route no dev\n");
> >>>>>> return -ENODEV;
> >>>>>
> >>>
> >
>
