[Qemu-devel] [PATCH v2 0/3] three zpci patches

[Qemu-devel] [PATCH v2 0/3] three zpci patches

[Yi Min Zhao]

This patch set contains three small zpci patches to fixup different issues.
1) remove zpci idx from msix message, instead we could use PCIDevice's id to
   find zpci device in kvm_arch_fixup_msi_route()
2) fixup ind_offset calculation for adapter interrupt routing entry
3) introduce our own iommu_replay callback

Yi Min Zhao (3):
  s390x/pci: remove idx from msix msg data
  s390x/pci: fixup ind_offset of msix routing entry
  s390x/pci: add iommu replay callback

 hw/s390x/s390-pci-bus.c  | 27 ++++++++++++++++-----------
 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       | 11 ++++++-----
 5 files changed, 30 insertions(+), 40 deletions(-)

-- 
Change log:
from v1:
1) Add s390_pci_find_dev_by_target() in s390_pci_stub.c
2) Remove the accepted patch from the series (Thanks for Conny's help).
3) Fixup typo error.
4) Add more comment for s390_pci_iommu_replay().

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

[Yi Min Zhao]

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);
+    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;
-- 
2.11.0 (Apple Git-81)

[Qemu-devel] [PATCH v2 2/3] s390x/pci: fixup ind_offset of msix routing entry

[Yi Min Zhao]

The guest uses the mpcifc instruction to register the aibvo of a zpci
device, which is the starting offset of indicators in the indicator
area and thus remains constant. Each msix vector is an offset from the
aibvo. When we map a msix route to an adapter route, we should not
modify the starting offset, but instead add the vector to the starting
offset to get the absolute offset in the specific route.

Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
---
 target/s390x/kvm.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 3d490c5e4b..21ce06966c 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2545,14 +2545,12 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
         return -ENODEV;
     }
 
-    pbdev->routes.adapter.ind_offset = vec;
-
     route->type = KVM_IRQ_ROUTING_S390_ADAPTER;
     route->flags = 0;
     route->u.adapter.summary_addr = pbdev->routes.adapter.summary_addr;
     route->u.adapter.ind_addr = pbdev->routes.adapter.ind_addr;
     route->u.adapter.summary_offset = pbdev->routes.adapter.summary_offset;
-    route->u.adapter.ind_offset = pbdev->routes.adapter.ind_offset;
+    route->u.adapter.ind_offset = pbdev->routes.adapter.ind_offset + vec;
     route->u.adapter.adapter_id = pbdev->routes.adapter.adapter_id;
     return 0;
 }
-- 
2.11.0 (Apple Git-81)

[Qemu-devel] [PATCH v2 3/3] s390x/pci: add iommu replay callback

[Yi Min Zhao]

Let's introduce iommu replay callback for s390 pci iommu memory region.
Currently we don't need any dma mapping replay. So let it return
directly. This implementation will avoid meaningless loops calling
translation callback.

Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
---
 hw/s390x/s390-pci-bus.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index bd8a3e1e1c..69f45e3715 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -397,6 +397,16 @@ static IOMMUTLBEntry s390_translate_iommu(IOMMUMemoryRegion *mr, hwaddr addr,
     return ret;
 }
 
+static void s390_pci_iommu_replay(IOMMUMemoryRegion *iommu,
+                                  IOMMUNotifier *notifier)
+{
+    /* It's impossible to plug a pci device on s390x that already has iommu
+     * mappings which need to be replayed, that is due to the "one iommu per
+     * zpci device" construct. So we don't need iommu replay currently.
+     */
+    return;
+}
+
 static S390PCIIOMMU *s390_pci_get_iommu(S390pciState *s, PCIBus *bus,
                                         int devfn)
 {
@@ -1045,6 +1055,7 @@ static void s390_iommu_memory_region_class_init(ObjectClass *klass, void *data)
     IOMMUMemoryRegionClass *imrc = IOMMU_MEMORY_REGION_CLASS(klass);
 
     imrc->translate = s390_translate_iommu;
+    imrc->replay = s390_pci_iommu_replay;
 }
 
 static const TypeInfo s390_iommu_memory_region_info = {
-- 
2.11.0 (Apple Git-81)

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

[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.)

> +    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;

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

[Yi Min Zhao]

在 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;
>

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

[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?

> >  
> >> +
> >> +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;  
> >  
> 

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 <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;
>>>   
>

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

[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?

> >>>     
> >>>> +    }
> >>>> +
> >>>> +    pbdev = s390_pci_find_dev_by_target(s390_get_phb(), DEVICE(dev)->id);
> >>>>        if (!pbdev) {
> >>>>            DPRINTF("add_msi_route no dev\n");
> >>>>            return -ENODEV;  
> >>>     
> >  
> 

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 <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;
>>>>>      
>>>   
>

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

[Cornelia Huck]

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;  
> >>>>>        
> >>>     
> >  
> 

Re: [Qemu-devel] [PATCH v2 3/3] s390x/pci: add iommu replay callback

[Cornelia Huck]

On Fri,  1 Sep 2017 06:22:58 +0200
Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:

> Let's introduce iommu replay callback for s390 pci iommu memory region.
> Currently we don't need any dma mapping replay. So let it return
> directly. This implementation will avoid meaningless loops calling
> translation callback.
> 
> Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> ---
>  hw/s390x/s390-pci-bus.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index bd8a3e1e1c..69f45e3715 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -397,6 +397,16 @@ static IOMMUTLBEntry s390_translate_iommu(IOMMUMemoryRegion *mr, hwaddr addr,
>      return ret;
>  }
>  
> +static void s390_pci_iommu_replay(IOMMUMemoryRegion *iommu,
> +                                  IOMMUNotifier *notifier)
> +{
> +    /* It's impossible to plug a pci device on s390x that already has iommu
> +     * mappings which need to be replayed, that is due to the "one iommu per
> +     * zpci device" construct. So we don't need iommu replay currently.

I must say that 'currently' still throws me off. Does this refer to
vfio? If yes, reword to something like 'Should we support migration of
vfio-pci devices in the future, we need to revisit this.'?

> +     */
> +    return;
> +}
> +
>  static S390PCIIOMMU *s390_pci_get_iommu(S390pciState *s, PCIBus *bus,
>                                          int devfn)
>  {
> @@ -1045,6 +1055,7 @@ static void s390_iommu_memory_region_class_init(ObjectClass *klass, void *data)
>      IOMMUMemoryRegionClass *imrc = IOMMU_MEMORY_REGION_CLASS(klass);
>  
>      imrc->translate = s390_translate_iommu;
> +    imrc->replay = s390_pci_iommu_replay;
>  }
>  
>  static const TypeInfo s390_iommu_memory_region_info = {

Re: [Qemu-devel] [PATCH v2 2/3] s390x/pci: fixup ind_offset of msix routing entry

[Cornelia Huck]

On Fri,  1 Sep 2017 06:22:57 +0200
Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:

> The guest uses the mpcifc instruction to register the aibvo of a zpci
> device, which is the starting offset of indicators in the indicator
> area and thus remains constant. Each msix vector is an offset from the
> aibvo. When we map a msix route to an adapter route, we should not
> modify the starting offset, but instead add the vector to the starting
> offset to get the absolute offset in the specific route.
> 
> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> ---
>  target/s390x/kvm.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 3d490c5e4b..21ce06966c 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -2545,14 +2545,12 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
>          return -ENODEV;
>      }
>  
> -    pbdev->routes.adapter.ind_offset = vec;
> -
>      route->type = KVM_IRQ_ROUTING_S390_ADAPTER;
>      route->flags = 0;
>      route->u.adapter.summary_addr = pbdev->routes.adapter.summary_addr;
>      route->u.adapter.ind_addr = pbdev->routes.adapter.ind_addr;
>      route->u.adapter.summary_offset = pbdev->routes.adapter.summary_offset;
> -    route->u.adapter.ind_offset = pbdev->routes.adapter.ind_offset;
> +    route->u.adapter.ind_offset = pbdev->routes.adapter.ind_offset + vec;
>      route->u.adapter.adapter_id = pbdev->routes.adapter.adapter_id;
>      return 0;
>  }

Looks good. I assume you'll send a v3, so I'll hold off on applying for
now.

Re: [Qemu-devel] [PATCH v2 3/3] s390x/pci: add iommu replay callback

[Yi Min Zhao]

在 2017/9/5 下午5:28, Cornelia Huck 写道:
> On Fri,  1 Sep 2017 06:22:58 +0200
> Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:
>
>> Let's introduce iommu replay callback for s390 pci iommu memory region.
>> Currently we don't need any dma mapping replay. So let it return
>> directly. This implementation will avoid meaningless loops calling
>> translation callback.
>>
>> Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>> Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>> ---
>>   hw/s390x/s390-pci-bus.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>> index bd8a3e1e1c..69f45e3715 100644
>> --- a/hw/s390x/s390-pci-bus.c
>> +++ b/hw/s390x/s390-pci-bus.c
>> @@ -397,6 +397,16 @@ static IOMMUTLBEntry s390_translate_iommu(IOMMUMemoryRegion *mr, hwaddr addr,
>>       return ret;
>>   }
>>   
>> +static void s390_pci_iommu_replay(IOMMUMemoryRegion *iommu,
>> +                                  IOMMUNotifier *notifier)
>> +{
>> +    /* It's impossible to plug a pci device on s390x that already has iommu
>> +     * mappings which need to be replayed, that is due to the "one iommu per
>> +     * zpci device" construct. So we don't need iommu replay currently.
> I must say that 'currently' still throws me off. Does this refer to
> vfio? If yes, reword to something like 'Should we support migration of
> vfio-pci devices in the future, we need to revisit this.'?
Yeah, it refers to vfio especially. I update this in next version.
>
>> +     */
>> +    return;
>> +}
>> +
>>   static S390PCIIOMMU *s390_pci_get_iommu(S390pciState *s, PCIBus *bus,
>>                                           int devfn)
>>   {
>> @@ -1045,6 +1055,7 @@ static void s390_iommu_memory_region_class_init(ObjectClass *klass, void *data)
>>       IOMMUMemoryRegionClass *imrc = IOMMU_MEMORY_REGION_CLASS(klass);
>>   
>>       imrc->translate = s390_translate_iommu;
>> +    imrc->replay = s390_pci_iommu_replay;
>>   }
>>   
>>   static const TypeInfo s390_iommu_memory_region_info = {
>