[PATCH for-5.2 v3] s390x/pci: fix endianness issues

[PATCH for-5.2 v3] s390x/pci: fix endianness issues

[Cornelia Huck]

The zPCI group and function structures are big endian. However, we do
not consistently store them as big endian locally, and are missing some
conversions.

Let's just store the structures as host endian instead and convert to
big endian when actually handling the instructions retrieving the data.

Also fix the layout of ClpReqQueryPciGrp: g is actually only 8 bit. This
also fixes accesses on little endian hosts, and makes accesses on big
endian hosts consistent.

Fixes: 28dc86a07299 ("s390x/pci: use a PCI Group structure")
Fixes: 9670ee752727 ("s390x/pci: use a PCI Function structure")
Fixes: 1e7552ff5c34 ("s390x/pci: get zPCI function info from host")
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---

v2->v3: added missing zpci_fn.flags copy, removed forgotten memcopy
v1->v2: switched to keeping the internal structures as host-endian

Again, tested on x86 and s390x (tcg/kvm) with virtio-pci devices;
testing vfio-pci devices would be good.

---
 hw/s390x/s390-pci-bus.c         | 10 +++++-----
 hw/s390x/s390-pci-inst.c        | 16 ++++++++++++++--
 hw/s390x/s390-pci-vfio.c        | 12 ++++++------
 include/hw/s390x/s390-pci-clp.h |  8 ++++----
 4 files changed, 29 insertions(+), 17 deletions(-)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index e0dc20ce4a56..05f7460aec9b 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -777,11 +777,11 @@ static void s390_pci_init_default_group(void)
     group = s390_group_create(ZPCI_DEFAULT_FN_GRP);
     resgrp = &group->zpci_group;
     resgrp->fr = 1;
-    stq_p(&resgrp->dasm, 0);
-    stq_p(&resgrp->msia, ZPCI_MSI_ADDR);
-    stw_p(&resgrp->mui, DEFAULT_MUI);
-    stw_p(&resgrp->i, 128);
-    stw_p(&resgrp->maxstbl, 128);
+    resgrp->dasm = 0;
+    resgrp->msia = ZPCI_MSI_ADDR;
+    resgrp->mui = DEFAULT_MUI;
+    resgrp->i = 128;
+    resgrp->maxstbl = 128;
     resgrp->version = 0;
 }
 
diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index 58cd041d17fb..70bfd91bf70e 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -281,7 +281,13 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra)
             goto out;
         }
 
-        memcpy(resquery, &pbdev->zpci_fn, sizeof(*resquery));
+        stq_p(&resquery->sdma, pbdev->zpci_fn.sdma);
+        stq_p(&resquery->edma, pbdev->zpci_fn.edma);
+        stw_p(&resquery->pchid, pbdev->zpci_fn.pchid);
+        resquery->flags = pbdev->zpci_fn.flags;
+        resquery->pfgid = pbdev->zpci_fn.pfgid;
+        stl_p(&resquery->fid, pbdev->zpci_fn.fid);
+        stl_p(&resquery->uid, pbdev->zpci_fn.uid);
 
         for (i = 0; i < PCI_BAR_COUNT; i++) {
             uint32_t data = pci_get_long(pbdev->pdev->config +
@@ -312,7 +318,13 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra)
             stw_p(&resgrp->hdr.rsp, CLP_RC_QUERYPCIFG_PFGID);
             goto out;
         }
-        memcpy(resgrp, &group->zpci_group, sizeof(ClpRspQueryPciGrp));
+        resgrp->fr = group->zpci_group.fr;
+        stq_p(&resgrp->dasm, group->zpci_group.dasm);
+        stq_p(&resgrp->msia, group->zpci_group.msia);
+        stw_p(&resgrp->mui, group->zpci_group.mui);
+        stw_p(&resgrp->i, group->zpci_group.i);
+        stw_p(&resgrp->maxstbl, group->zpci_group.maxstbl);
+        resgrp->version = group->zpci_group.version;
         stw_p(&resgrp->hdr.rsp, CLP_RC_OK);
         break;
     }
diff --git a/hw/s390x/s390-pci-vfio.c b/hw/s390x/s390-pci-vfio.c
index d5c78063b5bc..9296e1bb6efa 100644
--- a/hw/s390x/s390-pci-vfio.c
+++ b/hw/s390x/s390-pci-vfio.c
@@ -156,12 +156,12 @@ static void s390_pci_read_group(S390PCIBusDevice *pbdev,
         if (cap->flags & VFIO_DEVICE_INFO_ZPCI_FLAG_REFRESH) {
             resgrp->fr = 1;
         }
-        stq_p(&resgrp->dasm, cap->dasm);
-        stq_p(&resgrp->msia, cap->msi_addr);
-        stw_p(&resgrp->mui, cap->mui);
-        stw_p(&resgrp->i, cap->noi);
-        stw_p(&resgrp->maxstbl, cap->maxstbl);
-        stb_p(&resgrp->version, cap->version);
+        resgrp->dasm = cap->dasm;
+        resgrp->msia = cap->msi_addr;
+        resgrp->mui = cap->mui;
+        resgrp->i = cap->noi;
+        resgrp->maxstbl = cap->maxstbl;
+        resgrp->version = cap->version;
     }
 }
 
diff --git a/include/hw/s390x/s390-pci-clp.h b/include/hw/s390x/s390-pci-clp.h
index ea2b1378cd5a..96b8e3f1331b 100644
--- a/include/hw/s390x/s390-pci-clp.h
+++ b/include/hw/s390x/s390-pci-clp.h
@@ -144,10 +144,10 @@ typedef struct ClpReqQueryPciGrp {
     ClpReqHdr hdr;
     uint32_t fmt;
     uint64_t reserved1;
-#define CLP_REQ_QPCIG_MASK_PFGID 0xff
-    uint32_t g;
-    uint32_t reserved2;
-    uint64_t reserved3;
+    uint8_t reserved2[3];
+    uint8_t g;
+    uint32_t reserved3;
+    uint64_t reserved4;
 } QEMU_PACKED ClpReqQueryPciGrp;
 
 /* Query PCI function group response */
-- 
2.26.2

Re: [PATCH for-5.2 v3] s390x/pci: fix endianness issues

[Thomas Huth]

On 18/11/2020 11.42, Cornelia Huck wrote:
> The zPCI group and function structures are big endian. However, we do
> not consistently store them as big endian locally, and are missing some
> conversions.
> 
> Let's just store the structures as host endian instead and convert to
> big endian when actually handling the instructions retrieving the data.
> 
> Also fix the layout of ClpReqQueryPciGrp: g is actually only 8 bit. This
> also fixes accesses on little endian hosts, and makes accesses on big
> endian hosts consistent.
> 
> Fixes: 28dc86a07299 ("s390x/pci: use a PCI Group structure")
> Fixes: 9670ee752727 ("s390x/pci: use a PCI Function structure")
> Fixes: 1e7552ff5c34 ("s390x/pci: get zPCI function info from host")
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
> 
> v2->v3: added missing zpci_fn.flags copy, removed forgotten memcopy
> v1->v2: switched to keeping the internal structures as host-endian
> 
> Again, tested on x86 and s390x (tcg/kvm) with virtio-pci devices;
> testing vfio-pci devices would be good.

Looks good to me now!

Reviewed-by: Thomas Huth <thuth@redhat.com>

... and also my Fedora 28 TCG guest can now use the virtio-net-pci device
again, so also a light:

Tested-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH for-5.2 v3] s390x/pci: fix endianness issues

[Matthew Rosato]

On 11/18/20 5:42 AM, Cornelia Huck wrote:
> The zPCI group and function structures are big endian. However, we do
> not consistently store them as big endian locally, and are missing some
> conversions.
> 
> Let's just store the structures as host endian instead and convert to
> big endian when actually handling the instructions retrieving the data.
> 
> Also fix the layout of ClpReqQueryPciGrp: g is actually only 8 bit. This
> also fixes accesses on little endian hosts, and makes accesses on big
> endian hosts consistent.
> 
> Fixes: 28dc86a07299 ("s390x/pci: use a PCI Group structure")
> Fixes: 9670ee752727 ("s390x/pci: use a PCI Function structure")
> Fixes: 1e7552ff5c34 ("s390x/pci: get zPCI function info from host")
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
> 
> v2->v3: added missing zpci_fn.flags copy, removed forgotten memcopy
> v1->v2: switched to keeping the internal structures as host-endian
> 
> Again, tested on x86 and s390x (tcg/kvm) with virtio-pci devices;
> testing vfio-pci devices would be good.

Thanks Connie, code looks good to me:

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

And I once again took this for a spin with vfio-pci, driving network and 
disk workloads using a fairly recent (5.10-rc3) kernel in host/guest.  I 
also rolled back the host to an older kernel to drive the default clp 
paths with vfio -- Everything works fine.  I also verified that a tcg 
guest on x86 using a virtio pci device can see it as expected (Lesson 
learned: I will make a point of testing against tcg moving forward).  I 
further double-checked the live pfgid / g values going to/from the guest 
in all 3 environments since this structure was changed; everything looks 
good.

So if you'd like:

Tested-by: Matthew Rosato <mjrosato@linux.ibm.com>



> 
> ---
>   hw/s390x/s390-pci-bus.c         | 10 +++++-----
>   hw/s390x/s390-pci-inst.c        | 16 ++++++++++++++--
>   hw/s390x/s390-pci-vfio.c        | 12 ++++++------
>   include/hw/s390x/s390-pci-clp.h |  8 ++++----
>   4 files changed, 29 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index e0dc20ce4a56..05f7460aec9b 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -777,11 +777,11 @@ static void s390_pci_init_default_group(void)
>       group = s390_group_create(ZPCI_DEFAULT_FN_GRP);
>       resgrp = &group->zpci_group;
>       resgrp->fr = 1;
> -    stq_p(&resgrp->dasm, 0);
> -    stq_p(&resgrp->msia, ZPCI_MSI_ADDR);
> -    stw_p(&resgrp->mui, DEFAULT_MUI);
> -    stw_p(&resgrp->i, 128);
> -    stw_p(&resgrp->maxstbl, 128);
> +    resgrp->dasm = 0;
> +    resgrp->msia = ZPCI_MSI_ADDR;
> +    resgrp->mui = DEFAULT_MUI;
> +    resgrp->i = 128;
> +    resgrp->maxstbl = 128;
>       resgrp->version = 0;
>   }
>   
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index 58cd041d17fb..70bfd91bf70e 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -281,7 +281,13 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra)
>               goto out;
>           }
>   
> -        memcpy(resquery, &pbdev->zpci_fn, sizeof(*resquery));
> +        stq_p(&resquery->sdma, pbdev->zpci_fn.sdma);
> +        stq_p(&resquery->edma, pbdev->zpci_fn.edma);
> +        stw_p(&resquery->pchid, pbdev->zpci_fn.pchid);
> +        resquery->flags = pbdev->zpci_fn.flags;
> +        resquery->pfgid = pbdev->zpci_fn.pfgid;
> +        stl_p(&resquery->fid, pbdev->zpci_fn.fid);
> +        stl_p(&resquery->uid, pbdev->zpci_fn.uid);
>   
>           for (i = 0; i < PCI_BAR_COUNT; i++) {
>               uint32_t data = pci_get_long(pbdev->pdev->config +
> @@ -312,7 +318,13 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra)
>               stw_p(&resgrp->hdr.rsp, CLP_RC_QUERYPCIFG_PFGID);
>               goto out;
>           }
> -        memcpy(resgrp, &group->zpci_group, sizeof(ClpRspQueryPciGrp));
> +        resgrp->fr = group->zpci_group.fr;
> +        stq_p(&resgrp->dasm, group->zpci_group.dasm);
> +        stq_p(&resgrp->msia, group->zpci_group.msia);
> +        stw_p(&resgrp->mui, group->zpci_group.mui);
> +        stw_p(&resgrp->i, group->zpci_group.i);
> +        stw_p(&resgrp->maxstbl, group->zpci_group.maxstbl);
> +        resgrp->version = group->zpci_group.version;
>           stw_p(&resgrp->hdr.rsp, CLP_RC_OK);
>           break;
>       }
> diff --git a/hw/s390x/s390-pci-vfio.c b/hw/s390x/s390-pci-vfio.c
> index d5c78063b5bc..9296e1bb6efa 100644
> --- a/hw/s390x/s390-pci-vfio.c
> +++ b/hw/s390x/s390-pci-vfio.c
> @@ -156,12 +156,12 @@ static void s390_pci_read_group(S390PCIBusDevice *pbdev,
>           if (cap->flags & VFIO_DEVICE_INFO_ZPCI_FLAG_REFRESH) {
>               resgrp->fr = 1;
>           }
> -        stq_p(&resgrp->dasm, cap->dasm);
> -        stq_p(&resgrp->msia, cap->msi_addr);
> -        stw_p(&resgrp->mui, cap->mui);
> -        stw_p(&resgrp->i, cap->noi);
> -        stw_p(&resgrp->maxstbl, cap->maxstbl);
> -        stb_p(&resgrp->version, cap->version);
> +        resgrp->dasm = cap->dasm;
> +        resgrp->msia = cap->msi_addr;
> +        resgrp->mui = cap->mui;
> +        resgrp->i = cap->noi;
> +        resgrp->maxstbl = cap->maxstbl;
> +        resgrp->version = cap->version;
>       }
>   }
>   
> diff --git a/include/hw/s390x/s390-pci-clp.h b/include/hw/s390x/s390-pci-clp.h
> index ea2b1378cd5a..96b8e3f1331b 100644
> --- a/include/hw/s390x/s390-pci-clp.h
> +++ b/include/hw/s390x/s390-pci-clp.h
> @@ -144,10 +144,10 @@ typedef struct ClpReqQueryPciGrp {
>       ClpReqHdr hdr;
>       uint32_t fmt;
>       uint64_t reserved1;
> -#define CLP_REQ_QPCIG_MASK_PFGID 0xff
> -    uint32_t g;
> -    uint32_t reserved2;
> -    uint64_t reserved3;
> +    uint8_t reserved2[3];
> +    uint8_t g;
> +    uint32_t reserved3;
> +    uint64_t reserved4;
>   } QEMU_PACKED ClpReqQueryPciGrp;
>   
>   /* Query PCI function group response */
> 

Re: [PATCH for-5.2 v3] s390x/pci: fix endianness issues

[Cornelia Huck]

On Wed, 18 Nov 2020 09:20:39 -0500
Matthew Rosato <mjrosato@linux.ibm.com> wrote:

> On 11/18/20 5:42 AM, Cornelia Huck wrote:
> > The zPCI group and function structures are big endian. However, we do
> > not consistently store them as big endian locally, and are missing some
> > conversions.
> > 
> > Let's just store the structures as host endian instead and convert to
> > big endian when actually handling the instructions retrieving the data.
> > 
> > Also fix the layout of ClpReqQueryPciGrp: g is actually only 8 bit. This
> > also fixes accesses on little endian hosts, and makes accesses on big
> > endian hosts consistent.
> > 
> > Fixes: 28dc86a07299 ("s390x/pci: use a PCI Group structure")
> > Fixes: 9670ee752727 ("s390x/pci: use a PCI Function structure")
> > Fixes: 1e7552ff5c34 ("s390x/pci: get zPCI function info from host")
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> > 
> > v2->v3: added missing zpci_fn.flags copy, removed forgotten memcopy
> > v1->v2: switched to keeping the internal structures as host-endian
> > 
> > Again, tested on x86 and s390x (tcg/kvm) with virtio-pci devices;
> > testing vfio-pci devices would be good.  
> 
> Thanks Connie, code looks good to me:
> 
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
> 
> And I once again took this for a spin with vfio-pci, driving network and 
> disk workloads using a fairly recent (5.10-rc3) kernel in host/guest.  I 
> also rolled back the host to an older kernel to drive the default clp 
> paths with vfio -- Everything works fine.  I also verified that a tcg 
> guest on x86 using a virtio pci device can see it as expected (Lesson 
> learned: I will make a point of testing against tcg moving forward).  I 
> further double-checked the live pfgid / g values going to/from the guest 
> in all 3 environments since this structure was changed; everything looks 
> good.
> 
> So if you'd like:
> 
> Tested-by: Matthew Rosato <mjrosato@linux.ibm.com>

Wonderful, thanks!

> 
> 
> 
> > 
> > ---
> >   hw/s390x/s390-pci-bus.c         | 10 +++++-----
> >   hw/s390x/s390-pci-inst.c        | 16 ++++++++++++++--
> >   hw/s390x/s390-pci-vfio.c        | 12 ++++++------
> >   include/hw/s390x/s390-pci-clp.h |  8 ++++----
> >   4 files changed, 29 insertions(+), 17 deletions(-)

Re: [PATCH for-5.2 v3] s390x/pci: fix endianness issues

[Cornelia Huck]

On Wed, 18 Nov 2020 11:42:02 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> The zPCI group and function structures are big endian. However, we do
> not consistently store them as big endian locally, and are missing some
> conversions.
> 
> Let's just store the structures as host endian instead and convert to
> big endian when actually handling the instructions retrieving the data.
> 
> Also fix the layout of ClpReqQueryPciGrp: g is actually only 8 bit. This
> also fixes accesses on little endian hosts, and makes accesses on big
> endian hosts consistent.
> 
> Fixes: 28dc86a07299 ("s390x/pci: use a PCI Group structure")
> Fixes: 9670ee752727 ("s390x/pci: use a PCI Function structure")
> Fixes: 1e7552ff5c34 ("s390x/pci: get zPCI function info from host")
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
> 
> v2->v3: added missing zpci_fn.flags copy, removed forgotten memcopy
> v1->v2: switched to keeping the internal structures as host-endian
> 
> Again, tested on x86 and s390x (tcg/kvm) with virtio-pci devices;
> testing vfio-pci devices would be good.
> 
> ---
>  hw/s390x/s390-pci-bus.c         | 10 +++++-----
>  hw/s390x/s390-pci-inst.c        | 16 ++++++++++++++--
>  hw/s390x/s390-pci-vfio.c        | 12 ++++++------
>  include/hw/s390x/s390-pci-clp.h |  8 ++++----
>  4 files changed, 29 insertions(+), 17 deletions(-)

Queued to s390-fixes.