|
From: | Yi Min Zhao |
Subject: | Re: [Qemu-devel] [PATCH v2 1/3] s390x/pci: remove idx from msix msg data |
Date: | Tue, 5 Sep 2017 16:44:37 +0800 |
User-agent: | Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 |
在 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.
I'm confused. s390_pci_find_dev_by_idx() can be called in kvm_arch_fixup_msi_route().+ 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.
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;
[Prev in Thread] | Current Thread | [Next in Thread] |