| Message ID | 1504239778-29893-2-git-send-email-zyimin@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | three zpci patches | expand |
On Fri, 1 Sep 2017 06:22:56 +0200 Yi Min Zhao <zyimin@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 <zyimin@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/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;
在 2017/9/5 下午4:29, Cornelia Huck 写道: > On Fri, 1 Sep 2017 06:22:56 +0200 > Yi Min Zhao <zyimin@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 <zyimin@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/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. > >> + 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? > >> + >> +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. > >> + } >> + >> + pbdev = s390_pci_find_dev_by_target(s390_get_phb(), DEVICE(dev)->id); >> if (!pbdev) { >> DPRINTF("add_msi_route no dev\n"); >> return -ENODEV; >
On Tue, 5 Sep 2017 16:44:37 +0800 Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote: > 在 2017/9/5 下午4:29, Cornelia Huck 写道: > > On Fri, 1 Sep 2017 06:22:56 +0200 > > Yi Min Zhao <zyimin@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 <zyimin@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/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? > > > >> + > >> +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? > > > >> + } > >> + > >> + pbdev = s390_pci_find_dev_by_target(s390_get_phb(), DEVICE(dev)->id); > >> if (!pbdev) { > >> DPRINTF("add_msi_route no dev\n"); > >> return -ENODEV; > > >
在 2017/9/5 下午4:50, Cornelia Huck 写道: > On Tue, 5 Sep 2017 16:44:37 +0800 > Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote: > >> 在 2017/9/5 下午4:29, Cornelia Huck 写道: >>> On Fri, 1 Sep 2017 06:22:56 +0200 >>> Yi Min Zhao <zyimin@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 <zyimin@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/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; >>> >
On Tue, 5 Sep 2017 17:08:14 +0800 Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote: > 在 2017/9/5 下午4:50, Cornelia Huck 写道: > > On Tue, 5 Sep 2017 16:44:37 +0800 > > Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote: > > > >> 在 2017/9/5 下午4:29, Cornelia Huck 写道: > >>> On Fri, 1 Sep 2017 06:22:56 +0200 > >>> Yi Min Zhao <zyimin@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 <zyimin@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? > >>> > >>>> + } > >>>> + > >>>> + pbdev = s390_pci_find_dev_by_target(s390_get_phb(), DEVICE(dev)->id); > >>>> if (!pbdev) { > >>>> DPRINTF("add_msi_route no dev\n"); > >>>> return -ENODEV; > >>> > > >
在 2017/9/5 下午5:15, Cornelia Huck 写道: > On Tue, 5 Sep 2017 17:08:14 +0800 > Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote: > >> 在 2017/9/5 下午4:50, Cornelia Huck 写道: >>> On Tue, 5 Sep 2017 16:44:37 +0800 >>> Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote: >>> >>>> 在 2017/9/5 下午4:29, Cornelia Huck 写道: >>>>> On Fri, 1 Sep 2017 06:22:56 +0200 >>>>> Yi Min Zhao <zyimin@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 <zyimin@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"). > >>>>> >>>>>> + } >>>>>> + >>>>>> + pbdev = s390_pci_find_dev_by_target(s390_get_phb(), DEVICE(dev)->id); >>>>>> if (!pbdev) { >>>>>> DPRINTF("add_msi_route no dev\n"); >>>>>> return -ENODEV; >>>>> >>> >
On Tue, 5 Sep 2017 17:21:52 +0800 Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote: > 在 2017/9/5 下午5:15, Cornelia Huck 写道: > > On Tue, 5 Sep 2017 17:08:14 +0800 > > Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote: > > > >> 在 2017/9/5 下午4:50, Cornelia Huck 写道: > >>> On Tue, 5 Sep 2017 16:44:37 +0800 > >>> Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote: > >>> > >>>> 在 2017/9/5 下午4:29, Cornelia Huck 写道: > >>>>> On Fri, 1 Sep 2017 06:22:56 +0200 > >>>>> Yi Min Zhao <zyimin@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 <zyimin@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; > >>>>> > >>> > > >
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); + 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-bus.h b/hw/s390x/s390-pci-bus.h index bd636abc28..560bd82a0f 100644 --- a/hw/s390x/s390-pci-bus.h +++ b/hw/s390x/s390-pci-bus.h @@ -322,6 +322,8 @@ void s390_pci_generate_error_event(uint16_t pec, uint32_t fh, uint32_t fid, S390PCIBusDevice *s390_pci_find_dev_by_idx(S390pciState *s, uint32_t idx); S390PCIBusDevice *s390_pci_find_dev_by_fh(S390pciState *s, uint32_t fh); S390PCIBusDevice *s390_pci_find_dev_by_fid(S390pciState *s, uint32_t fid); +S390PCIBusDevice *s390_pci_find_dev_by_target(S390pciState *s, + const char *target); S390PCIBusDevice *s390_pci_find_next_avail_dev(S390pciState *s, S390PCIBusDevice *pbdev); diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c index eba9ffb5f2..8e088f3dc9 100644 --- a/hw/s390x/s390-pci-inst.c +++ b/hw/s390x/s390-pci-inst.c @@ -413,29 +413,6 @@ int pcilg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2) return 0; } -static void update_msix_table_msg_data(S390PCIBusDevice *pbdev, uint64_t offset, - uint64_t *data, uint8_t len) -{ - uint32_t val; - uint8_t *msg_data; - - if (offset % PCI_MSIX_ENTRY_SIZE != 8) { - return; - } - - if (len != 4) { - DPRINTF("access msix table msg data but len is %d\n", len); - return; - } - - msg_data = (uint8_t *)data - offset % PCI_MSIX_ENTRY_SIZE + - PCI_MSIX_ENTRY_VECTOR_CTRL; - val = pci_get_long(msg_data) | - ((pbdev->fh & FH_MASK_INDEX) << ZPCI_MSI_VEC_BITS); - pci_set_long(msg_data, val); - DPRINTF("update msix msg_data to 0x%" PRIx64 "\n", *data); -} - static int trap_msix(S390PCIBusDevice *pbdev, uint64_t offset, uint8_t pcias) { if (pbdev->msix.available && pbdev->msix.table_bar == pcias && @@ -508,7 +485,6 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2) if (trap_msix(pbdev, offset, pcias)) { offset = offset - pbdev->msix.table_offset; mr = &pbdev->pdev->msix_table_mmio; - update_msix_table_msg_data(pbdev, offset, &data, len); } else { mr = pbdev->pdev->io_regions[pcias].memory; } 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; } + +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; + } + + pbdev = s390_pci_find_dev_by_target(s390_get_phb(), DEVICE(dev)->id); if (!pbdev) { DPRINTF("add_msi_route no dev\n"); return -ENODEV;
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 <zyimin@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(-)