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

[PATCH for-5.2] 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.

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

Alternative approach to my patch from yesterday. The change is bigger,
but the end result is arguably nicer.

Again, works for me with virtio-pci devices on x86 and on s390x (tcg/kvm).

Testing with vfio-pci devices would be appreciated.

---
 hw/s390x/s390-pci-bus.c         | 10 +++++-----
 hw/s390x/s390-pci-inst.c        | 14 +++++++++++++-
 hw/s390x/s390-pci-vfio.c        | 12 ++++++------
 include/hw/s390x/s390-pci-clp.h |  8 ++++----
 4 files changed, 28 insertions(+), 16 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..6c36201229f3 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -281,7 +281,12 @@ 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->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 +
@@ -313,6 +318,13 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra)
             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] s390x/pci: fix endianness issues

[Cornelia Huck]

On Wed, 18 Nov 2020 09:51:54 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

[This is obviously a v2. Should not send patches before the first coffee :/]

> 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.
> 
> 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>
> ---
> 
> Alternative approach to my patch from yesterday. The change is bigger,
> but the end result is arguably nicer.
> 
> Again, works for me with virtio-pci devices on x86 and on s390x (tcg/kvm).
> 
> Testing with vfio-pci devices would be appreciated.
> 
> ---
>  hw/s390x/s390-pci-bus.c         | 10 +++++-----
>  hw/s390x/s390-pci-inst.c        | 14 +++++++++++++-
>  hw/s390x/s390-pci-vfio.c        | 12 ++++++------
>  include/hw/s390x/s390-pci-clp.h |  8 ++++----
>  4 files changed, 28 insertions(+), 16 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..6c36201229f3 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -281,7 +281,12 @@ 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->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 +
> @@ -313,6 +318,13 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra)
>              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] s390x/pci: fix endianness issues

[Thomas Huth]

On 18/11/2020 09.51, 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.
> 
> 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>
> ---
> 
> Alternative approach to my patch from yesterday. The change is bigger,
> but the end result is arguably nicer.

Looks way better in my eyes, thanks!

[...]
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index 58cd041d17fb..6c36201229f3 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -281,7 +281,12 @@ 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->pfgid = pbdev->zpci_fn.pfgid;
> +        stl_p(&resquery->fid, pbdev->zpci_fn.fid);
> +        stl_p(&resquery->uid, pbdev->zpci_fn.uid);

Looking at what had been removed in 9670ee7527279, I think you likely miss
this here:

        stw_p(&resquery->ug, pbdev->zpci_fn.ug)

?

>          for (i = 0; i < PCI_BAR_COUNT; i++) {
>              uint32_t data = pci_get_long(pbdev->pdev->config +
> @@ -313,6 +318,13 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra)
>              goto out;
>          }
>          memcpy(resgrp, &group->zpci_group, sizeof(ClpRspQueryPciGrp));

I think you likely could remove the memcpy now, too?

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

This might even qualify as a separate patch, since it fixes a separate
problem on big endian hosts, too (g should have been masked with 0xff when
read as 32-bit value).

 Thomas

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

[Cornelia Huck]

On Wed, 18 Nov 2020 10:38:00 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 18/11/2020 09.51, 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.
> > 
> > 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>
> > ---
> > 
> > Alternative approach to my patch from yesterday. The change is bigger,
> > but the end result is arguably nicer.  
> 
> Looks way better in my eyes, thanks!
> 
> [...]
> > diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> > index 58cd041d17fb..6c36201229f3 100644
> > --- a/hw/s390x/s390-pci-inst.c
> > +++ b/hw/s390x/s390-pci-inst.c
> > @@ -281,7 +281,12 @@ 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->pfgid = pbdev->zpci_fn.pfgid;
> > +        stl_p(&resquery->fid, pbdev->zpci_fn.fid);
> > +        stl_p(&resquery->uid, pbdev->zpci_fn.uid);  
> 
> Looking at what had been removed in 9670ee7527279, I think you likely miss
> this here:
> 
>         stw_p(&resquery->ug, pbdev->zpci_fn.ug)
> 
> ?

Hm, ug has been split up into pfgid and flags. Only the vfio-pci code
sets flags. It seems I'm only missing copying it during the instruction
emulation, added.

> 
> >          for (i = 0; i < PCI_BAR_COUNT; i++) {
> >              uint32_t data = pci_get_long(pbdev->pdev->config +
> > @@ -313,6 +318,13 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra)
> >              goto out;
> >          }
> >          memcpy(resgrp, &group->zpci_group, sizeof(ClpRspQueryPciGrp));  
> 
> I think you likely could remove the memcpy now, too?

Yeah, -ENOCOFFEE.

> 
> > +        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/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;  
> 
> This might even qualify as a separate patch, since it fixes a separate
> problem on big endian hosts, too (g should have been masked with 0xff when
> read as 32-bit value).

It does not really break on be, though. I'd rather not split it out as
an extra patch, but rather tweak the patch description.

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

[Cornelia Huck]

zPCI control blocks are big endian, we need to take care that we
do proper accesses in order not to break tcg guests on little endian
hosts.

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

Works for me with virtio-pci devices for tcg on x86 and s390x, and for kvm.
The vfio changes are not strictly needed; did not test them due to lack of
hardware -- testing appreciated.

As this fixes a regression, I want this in 5.2.

---
 hw/s390x/s390-pci-bus.c  | 12 ++++++------
 hw/s390x/s390-pci-inst.c |  4 ++--
 hw/s390x/s390-pci-vfio.c |  8 ++++----
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index e0dc20ce4a56..17e64e0b1200 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -787,12 +787,12 @@ static void s390_pci_init_default_group(void)
 
 static void set_pbdev_info(S390PCIBusDevice *pbdev)
 {
-    pbdev->zpci_fn.sdma = ZPCI_SDMA_ADDR;
-    pbdev->zpci_fn.edma = ZPCI_EDMA_ADDR;
-    pbdev->zpci_fn.pchid = 0;
+    stq_p(&pbdev->zpci_fn.sdma, ZPCI_SDMA_ADDR);
+    stq_p(&pbdev->zpci_fn.edma, ZPCI_EDMA_ADDR);
+    stw_p(&pbdev->zpci_fn.pchid, 0);
     pbdev->zpci_fn.pfgid = ZPCI_DEFAULT_FN_GRP;
-    pbdev->zpci_fn.fid = pbdev->fid;
-    pbdev->zpci_fn.uid = pbdev->uid;
+    stl_p(&pbdev->zpci_fn.fid, pbdev->fid);
+    stl_p(&pbdev->zpci_fn.uid, pbdev->uid);
     pbdev->pci_group = s390_group_find(ZPCI_DEFAULT_FN_GRP);
 }
 
@@ -871,7 +871,7 @@ static int s390_pci_msix_init(S390PCIBusDevice *pbdev)
     memory_region_init_io(&pbdev->msix_notify_mr, OBJECT(pbdev),
                           &s390_msi_ctrl_ops, pbdev, name, PAGE_SIZE);
     memory_region_add_subregion(&pbdev->iommu->mr,
-                                pbdev->pci_group->zpci_group.msia,
+                                ldq_p(&pbdev->pci_group->zpci_group.msia),
                                 &pbdev->msix_notify_mr);
     g_free(name);
 
diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index 58cd041d17fb..7bc6b79f10ce 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -305,7 +305,7 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra)
         ClpReqQueryPciGrp *reqgrp = (ClpReqQueryPciGrp *)reqh;
         S390PCIGroup *group;
 
-        group = s390_group_find(reqgrp->g);
+        group = s390_group_find(ldl_p(&reqgrp->g));
         if (!group) {
             /* We do not allow access to unknown groups */
             /* The group must have been obtained with a vfio device */
@@ -788,7 +788,7 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
     /* Length must be greater than 8, a multiple of 8 */
     /* and not greater than maxstbl */
     if ((len <= 8) || (len % 8) ||
-        (len > pbdev->pci_group->zpci_group.maxstbl)) {
+        (len > lduw_p(&pbdev->pci_group->zpci_group.maxstbl))) {
         goto specification_error;
     }
     /* Do not cross a 4K-byte boundary */
diff --git a/hw/s390x/s390-pci-vfio.c b/hw/s390x/s390-pci-vfio.c
index d5c78063b5bc..f455c6f20a1a 100644
--- a/hw/s390x/s390-pci-vfio.c
+++ b/hw/s390x/s390-pci-vfio.c
@@ -116,10 +116,10 @@ static void s390_pci_read_base(S390PCIBusDevice *pbdev,
     }
     cap = (void *) hdr;
 
-    pbdev->zpci_fn.sdma = cap->start_dma;
-    pbdev->zpci_fn.edma = cap->end_dma;
-    pbdev->zpci_fn.pchid = cap->pchid;
-    pbdev->zpci_fn.vfn = cap->vfn;
+    stq_p(&pbdev->zpci_fn.sdma, cap->start_dma);
+    stq_p(&pbdev->zpci_fn.edma, cap->end_dma);
+    stw_p(&pbdev->zpci_fn.pchid, cap->pchid);
+    stw_p(&pbdev->zpci_fn.vfn, cap->vfn);
     pbdev->zpci_fn.pfgid = cap->gid;
     /* The following values remain 0 until we support other FMB formats */
     pbdev->zpci_fn.fmbl = 0;
-- 
2.26.2

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

[Philippe Mathieu-Daudé]

On 11/17/20 6:13 PM, Cornelia Huck wrote:
> zPCI control blocks are big endian, we need to take care that we
> do proper accesses in order not to break tcg guests on little endian
> hosts.
> 
> 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>
> ---
> 
> Works for me with virtio-pci devices for tcg on x86 and s390x, and for kvm.
> The vfio changes are not strictly needed; did not test them due to lack of
> hardware -- testing appreciated.
> 
> As this fixes a regression, I want this in 5.2.
> 
> ---
>  hw/s390x/s390-pci-bus.c  | 12 ++++++------
>  hw/s390x/s390-pci-inst.c |  4 ++--
>  hw/s390x/s390-pci-vfio.c |  8 ++++----
>  3 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index e0dc20ce4a56..17e64e0b1200 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -787,12 +787,12 @@ static void s390_pci_init_default_group(void)
>  
>  static void set_pbdev_info(S390PCIBusDevice *pbdev)
>  {
> -    pbdev->zpci_fn.sdma = ZPCI_SDMA_ADDR;
> -    pbdev->zpci_fn.edma = ZPCI_EDMA_ADDR;
> -    pbdev->zpci_fn.pchid = 0;
> +    stq_p(&pbdev->zpci_fn.sdma, ZPCI_SDMA_ADDR);

"zPCI control blocks are big endian" so don't we
need the _be_ accessors? stq_be_p() etc...

> +    stq_p(&pbdev->zpci_fn.edma, ZPCI_EDMA_ADDR);
> +    stw_p(&pbdev->zpci_fn.pchid, 0);
>      pbdev->zpci_fn.pfgid = ZPCI_DEFAULT_FN_GRP;
> -    pbdev->zpci_fn.fid = pbdev->fid;
> -    pbdev->zpci_fn.uid = pbdev->uid;
> +    stl_p(&pbdev->zpci_fn.fid, pbdev->fid);
> +    stl_p(&pbdev->zpci_fn.uid, pbdev->uid);
>      pbdev->pci_group = s390_group_find(ZPCI_DEFAULT_FN_GRP);
>  }
>  
> @@ -871,7 +871,7 @@ static int s390_pci_msix_init(S390PCIBusDevice *pbdev)
>      memory_region_init_io(&pbdev->msix_notify_mr, OBJECT(pbdev),
>                            &s390_msi_ctrl_ops, pbdev, name, PAGE_SIZE);
>      memory_region_add_subregion(&pbdev->iommu->mr,
> -                                pbdev->pci_group->zpci_group.msia,
> +                                ldq_p(&pbdev->pci_group->zpci_group.msia),
>                                  &pbdev->msix_notify_mr);
>      g_free(name);
>  
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index 58cd041d17fb..7bc6b79f10ce 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -305,7 +305,7 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra)
>          ClpReqQueryPciGrp *reqgrp = (ClpReqQueryPciGrp *)reqh;
>          S390PCIGroup *group;
>  
> -        group = s390_group_find(reqgrp->g);
> +        group = s390_group_find(ldl_p(&reqgrp->g));
>          if (!group) {
>              /* We do not allow access to unknown groups */
>              /* The group must have been obtained with a vfio device */
> @@ -788,7 +788,7 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>      /* Length must be greater than 8, a multiple of 8 */
>      /* and not greater than maxstbl */
>      if ((len <= 8) || (len % 8) ||
> -        (len > pbdev->pci_group->zpci_group.maxstbl)) {
> +        (len > lduw_p(&pbdev->pci_group->zpci_group.maxstbl))) {
>          goto specification_error;
>      }
>      /* Do not cross a 4K-byte boundary */
> diff --git a/hw/s390x/s390-pci-vfio.c b/hw/s390x/s390-pci-vfio.c
> index d5c78063b5bc..f455c6f20a1a 100644
> --- a/hw/s390x/s390-pci-vfio.c
> +++ b/hw/s390x/s390-pci-vfio.c
> @@ -116,10 +116,10 @@ static void s390_pci_read_base(S390PCIBusDevice *pbdev,
>      }
>      cap = (void *) hdr;
>  
> -    pbdev->zpci_fn.sdma = cap->start_dma;
> -    pbdev->zpci_fn.edma = cap->end_dma;
> -    pbdev->zpci_fn.pchid = cap->pchid;
> -    pbdev->zpci_fn.vfn = cap->vfn;
> +    stq_p(&pbdev->zpci_fn.sdma, cap->start_dma);
> +    stq_p(&pbdev->zpci_fn.edma, cap->end_dma);
> +    stw_p(&pbdev->zpci_fn.pchid, cap->pchid);
> +    stw_p(&pbdev->zpci_fn.vfn, cap->vfn);
>      pbdev->zpci_fn.pfgid = cap->gid;
>      /* The following values remain 0 until we support other FMB formats */
>      pbdev->zpci_fn.fmbl = 0;
> 

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

[Matthew Rosato]

On 11/17/20 12:59 PM, Philippe Mathieu-Daudé wrote:
> On 11/17/20 6:13 PM, Cornelia Huck wrote:
>> zPCI control blocks are big endian, we need to take care that we
>> do proper accesses in order not to break tcg guests on little endian
>> hosts.
>>
>> 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>
>> ---
>>
>> Works for me with virtio-pci devices for tcg on x86 and s390x, and for kvm.
>> The vfio changes are not strictly needed; did not test them due to lack of
>> hardware -- testing appreciated. >>
>> As this fixes a regression, I want this in 5.2.
>>
>> ---
>>   hw/s390x/s390-pci-bus.c  | 12 ++++++------
>>   hw/s390x/s390-pci-inst.c |  4 ++--
>>   hw/s390x/s390-pci-vfio.c |  8 ++++----
>>   3 files changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>> index e0dc20ce4a56..17e64e0b1200 100644
>> --- a/hw/s390x/s390-pci-bus.c
>> +++ b/hw/s390x/s390-pci-bus.c
>> @@ -787,12 +787,12 @@ static void s390_pci_init_default_group(void)
>>   
>>   static void set_pbdev_info(S390PCIBusDevice *pbdev)
>>   {
>> -    pbdev->zpci_fn.sdma = ZPCI_SDMA_ADDR;
>> -    pbdev->zpci_fn.edma = ZPCI_EDMA_ADDR;
>> -    pbdev->zpci_fn.pchid = 0;
>> +    stq_p(&pbdev->zpci_fn.sdma, ZPCI_SDMA_ADDR);
> 
> "zPCI control blocks are big endian" so don't we
> need the _be_ accessors? stq_be_p() etc...
> 

I don't think this is necessary.  This is only available for target 
s390x, which is always big endian...  cpu-all.h should define stq_p as 
stq_be_p for example inside the #if defined(TARGET_WORDS_BIGENDIAN).

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

[Philippe Mathieu-Daudé]

On 11/17/20 7:19 PM, Matthew Rosato wrote:
> On 11/17/20 12:59 PM, Philippe Mathieu-Daudé wrote:
>> On 11/17/20 6:13 PM, Cornelia Huck wrote:
>>> zPCI control blocks are big endian, we need to take care that we
>>> do proper accesses in order not to break tcg guests on little endian
>>> hosts.
>>>
>>> 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>
>>> ---
>>>
>>> Works for me with virtio-pci devices for tcg on x86 and s390x, and
>>> for kvm.
>>> The vfio changes are not strictly needed; did not test them due to
>>> lack of
>>> hardware -- testing appreciated. >>
>>> As this fixes a regression, I want this in 5.2.
>>>
>>> ---
>>>   hw/s390x/s390-pci-bus.c  | 12 ++++++------
>>>   hw/s390x/s390-pci-inst.c |  4 ++--
>>>   hw/s390x/s390-pci-vfio.c |  8 ++++----
>>>   3 files changed, 12 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>>> index e0dc20ce4a56..17e64e0b1200 100644
>>> --- a/hw/s390x/s390-pci-bus.c
>>> +++ b/hw/s390x/s390-pci-bus.c
>>> @@ -787,12 +787,12 @@ static void s390_pci_init_default_group(void)
>>>     static void set_pbdev_info(S390PCIBusDevice *pbdev)
>>>   {
>>> -    pbdev->zpci_fn.sdma = ZPCI_SDMA_ADDR;
>>> -    pbdev->zpci_fn.edma = ZPCI_EDMA_ADDR;
>>> -    pbdev->zpci_fn.pchid = 0;
>>> +    stq_p(&pbdev->zpci_fn.sdma, ZPCI_SDMA_ADDR);
>>
>> "zPCI control blocks are big endian" so don't we
>> need the _be_ accessors? stq_be_p() etc...
>>
> 
> I don't think this is necessary.  This is only available for target
> s390x, which is always big endian...  cpu-all.h should define stq_p as
> stq_be_p for example inside the #if defined(TARGET_WORDS_BIGENDIAN).

But if you run on little-endian host, you need to byte-swap that,
isn't it?

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

[Thomas Huth]

On 17/11/2020 19.30, Philippe Mathieu-Daudé wrote:
> On 11/17/20 7:19 PM, Matthew Rosato wrote:
>> On 11/17/20 12:59 PM, Philippe Mathieu-Daudé wrote:
>>> On 11/17/20 6:13 PM, Cornelia Huck wrote:
>>>> zPCI control blocks are big endian, we need to take care that we
>>>> do proper accesses in order not to break tcg guests on little endian
>>>> hosts.
>>>>
>>>> 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>
>>>> ---
>>>>
>>>> Works for me with virtio-pci devices for tcg on x86 and s390x, and
>>>> for kvm.
>>>> The vfio changes are not strictly needed; did not test them due to
>>>> lack of
>>>> hardware -- testing appreciated. >>
>>>> As this fixes a regression, I want this in 5.2.
>>>>
>>>> ---
>>>>   hw/s390x/s390-pci-bus.c  | 12 ++++++------
>>>>   hw/s390x/s390-pci-inst.c |  4 ++--
>>>>   hw/s390x/s390-pci-vfio.c |  8 ++++----
>>>>   3 files changed, 12 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>>>> index e0dc20ce4a56..17e64e0b1200 100644
>>>> --- a/hw/s390x/s390-pci-bus.c
>>>> +++ b/hw/s390x/s390-pci-bus.c
>>>> @@ -787,12 +787,12 @@ static void s390_pci_init_default_group(void)
>>>>     static void set_pbdev_info(S390PCIBusDevice *pbdev)
>>>>   {
>>>> -    pbdev->zpci_fn.sdma = ZPCI_SDMA_ADDR;
>>>> -    pbdev->zpci_fn.edma = ZPCI_EDMA_ADDR;
>>>> -    pbdev->zpci_fn.pchid = 0;
>>>> +    stq_p(&pbdev->zpci_fn.sdma, ZPCI_SDMA_ADDR);
>>>
>>> "zPCI control blocks are big endian" so don't we
>>> need the _be_ accessors? stq_be_p() etc...
>>>
>>
>> I don't think this is necessary.  This is only available for target
>> s390x, which is always big endian...  cpu-all.h should define stq_p as
>> stq_be_p for example inside the #if defined(TARGET_WORDS_BIGENDIAN).
> 
> But if you run on little-endian host, you need to byte-swap that,
> isn't it?

It's done by the macros. They depend on the target endianess. See cpu-all.h.

 Thomas

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

[Philippe Mathieu-Daudé]

On 11/17/20 7:39 PM, Thomas Huth wrote:
> On 17/11/2020 19.30, Philippe Mathieu-Daudé wrote:
>> On 11/17/20 7:19 PM, Matthew Rosato wrote:
>>> On 11/17/20 12:59 PM, Philippe Mathieu-Daudé wrote:
>>>> On 11/17/20 6:13 PM, Cornelia Huck wrote:
>>>>> zPCI control blocks are big endian, we need to take care that we
>>>>> do proper accesses in order not to break tcg guests on little endian
>>>>> hosts.
>>>>>
>>>>> 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>
>>>>> ---
>>>>>
>>>>> Works for me with virtio-pci devices for tcg on x86 and s390x, and
>>>>> for kvm.
>>>>> The vfio changes are not strictly needed; did not test them due to
>>>>> lack of
>>>>> hardware -- testing appreciated. >>
>>>>> As this fixes a regression, I want this in 5.2.
>>>>>
>>>>> ---
>>>>>   hw/s390x/s390-pci-bus.c  | 12 ++++++------
>>>>>   hw/s390x/s390-pci-inst.c |  4 ++--
>>>>>   hw/s390x/s390-pci-vfio.c |  8 ++++----
>>>>>   3 files changed, 12 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>>>>> index e0dc20ce4a56..17e64e0b1200 100644
>>>>> --- a/hw/s390x/s390-pci-bus.c
>>>>> +++ b/hw/s390x/s390-pci-bus.c
>>>>> @@ -787,12 +787,12 @@ static void s390_pci_init_default_group(void)
>>>>>     static void set_pbdev_info(S390PCIBusDevice *pbdev)
>>>>>   {
>>>>> -    pbdev->zpci_fn.sdma = ZPCI_SDMA_ADDR;
>>>>> -    pbdev->zpci_fn.edma = ZPCI_EDMA_ADDR;
>>>>> -    pbdev->zpci_fn.pchid = 0;
>>>>> +    stq_p(&pbdev->zpci_fn.sdma, ZPCI_SDMA_ADDR);
>>>>
>>>> "zPCI control blocks are big endian" so don't we
>>>> need the _be_ accessors? stq_be_p() etc...
>>>>
>>>
>>> I don't think this is necessary.  This is only available for target
>>> s390x, which is always big endian...  cpu-all.h should define stq_p as
>>> stq_be_p for example inside the #if defined(TARGET_WORDS_BIGENDIAN).
>>
>> But if you run on little-endian host, you need to byte-swap that,
>> isn't it?
> 
> It's done by the macros. They depend on the target endianess. See cpu-all.h.

I'm confused because the description is about target endianness,
but stq_p() is about host alignment.

If there is no alignment problem, doesn't using stq_p() make code
harder to review?

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

[Thomas Huth]

On 17/11/2020 19.49, Philippe Mathieu-Daudé wrote:
> On 11/17/20 7:39 PM, Thomas Huth wrote:
>> On 17/11/2020 19.30, Philippe Mathieu-Daudé wrote:
>>> On 11/17/20 7:19 PM, Matthew Rosato wrote:
>>>> On 11/17/20 12:59 PM, Philippe Mathieu-Daudé wrote:
>>>>> On 11/17/20 6:13 PM, Cornelia Huck wrote:
>>>>>> zPCI control blocks are big endian, we need to take care that we
>>>>>> do proper accesses in order not to break tcg guests on little endian
>>>>>> hosts.
>>>>>>
>>>>>> 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>
>>>>>> ---
>>>>>>
>>>>>> Works for me with virtio-pci devices for tcg on x86 and s390x, and
>>>>>> for kvm.
>>>>>> The vfio changes are not strictly needed; did not test them due to
>>>>>> lack of
>>>>>> hardware -- testing appreciated. >>
>>>>>> As this fixes a regression, I want this in 5.2.
>>>>>>
>>>>>> ---
>>>>>>   hw/s390x/s390-pci-bus.c  | 12 ++++++------
>>>>>>   hw/s390x/s390-pci-inst.c |  4 ++--
>>>>>>   hw/s390x/s390-pci-vfio.c |  8 ++++----
>>>>>>   3 files changed, 12 insertions(+), 12 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>>>>>> index e0dc20ce4a56..17e64e0b1200 100644
>>>>>> --- a/hw/s390x/s390-pci-bus.c
>>>>>> +++ b/hw/s390x/s390-pci-bus.c
>>>>>> @@ -787,12 +787,12 @@ static void s390_pci_init_default_group(void)
>>>>>>     static void set_pbdev_info(S390PCIBusDevice *pbdev)
>>>>>>   {
>>>>>> -    pbdev->zpci_fn.sdma = ZPCI_SDMA_ADDR;
>>>>>> -    pbdev->zpci_fn.edma = ZPCI_EDMA_ADDR;
>>>>>> -    pbdev->zpci_fn.pchid = 0;
>>>>>> +    stq_p(&pbdev->zpci_fn.sdma, ZPCI_SDMA_ADDR);
>>>>>
>>>>> "zPCI control blocks are big endian" so don't we
>>>>> need the _be_ accessors? stq_be_p() etc...
>>>>>
>>>>
>>>> I don't think this is necessary.  This is only available for target
>>>> s390x, which is always big endian...  cpu-all.h should define stq_p as
>>>> stq_be_p for example inside the #if defined(TARGET_WORDS_BIGENDIAN).
>>>
>>> But if you run on little-endian host, you need to byte-swap that,
>>> isn't it?
>>
>> It's done by the macros. They depend on the target endianess. See cpu-all.h.
> 
> I'm confused because the description is about target endianness,
> but stq_p() is about host alignment.

stq_p() is apparently also about endianess. Why would it depend on
TARGET_WORDS_BIGENDIAN otherwise?

 Thomas

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

[Matthew Rosato]

On 11/17/20 12:13 PM, Cornelia Huck wrote:
> zPCI control blocks are big endian, we need to take care that we
> do proper accesses in order not to break tcg guests on little endian
> hosts.
> 
> 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>
> ---
> 
> Works for me with virtio-pci devices for tcg on x86 and s390x, and for kvm.
> The vfio changes are not strictly needed; did not test them due to lack of
> hardware -- testing appreciated.
> 
> As this fixes a regression, I want this in 5.2.
> 
> ---
>   hw/s390x/s390-pci-bus.c  | 12 ++++++------
>   hw/s390x/s390-pci-inst.c |  4 ++--
>   hw/s390x/s390-pci-vfio.c |  8 ++++----
>   3 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index e0dc20ce4a56..17e64e0b1200 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -787,12 +787,12 @@ static void s390_pci_init_default_group(void)
>   
>   static void set_pbdev_info(S390PCIBusDevice *pbdev)
>   {
> -    pbdev->zpci_fn.sdma = ZPCI_SDMA_ADDR;
> -    pbdev->zpci_fn.edma = ZPCI_EDMA_ADDR;
> -    pbdev->zpci_fn.pchid = 0;
> +    stq_p(&pbdev->zpci_fn.sdma, ZPCI_SDMA_ADDR);
> +    stq_p(&pbdev->zpci_fn.edma, ZPCI_EDMA_ADDR);
> +    stw_p(&pbdev->zpci_fn.pchid, 0);
>       pbdev->zpci_fn.pfgid = ZPCI_DEFAULT_FN_GRP;
> -    pbdev->zpci_fn.fid = pbdev->fid;
> -    pbdev->zpci_fn.uid = pbdev->uid;
> +    stl_p(&pbdev->zpci_fn.fid, pbdev->fid);
> +    stl_p(&pbdev->zpci_fn.uid, pbdev->uid);
>       pbdev->pci_group = s390_group_find(ZPCI_DEFAULT_FN_GRP);
>   }
>   
> @@ -871,7 +871,7 @@ static int s390_pci_msix_init(S390PCIBusDevice *pbdev)
>       memory_region_init_io(&pbdev->msix_notify_mr, OBJECT(pbdev),
>                             &s390_msi_ctrl_ops, pbdev, name, PAGE_SIZE);
>       memory_region_add_subregion(&pbdev->iommu->mr,
> -                                pbdev->pci_group->zpci_group.msia,
> +                                ldq_p(&pbdev->pci_group->zpci_group.msia),
>                                   &pbdev->msix_notify_mr);
>       g_free(name);
>   
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index 58cd041d17fb..7bc6b79f10ce 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -305,7 +305,7 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra)
>           ClpReqQueryPciGrp *reqgrp = (ClpReqQueryPciGrp *)reqh;
>           S390PCIGroup *group;
>   
> -        group = s390_group_find(reqgrp->g);
> +        group = s390_group_find(ldl_p(&reqgrp->g));

So as I alluded to in my other email, this should really be:
+        group = s390_group_find(ldl_p(&reqgrp->g) & 
CLP_REQ_QPCIG_MASK_PFGID);

To ensure that only the 8b pfgid is used from the 32b 'g' - doing it 
this way ensure we've already accounted for endianness.  The other 24b 
are reserved 0s so that's why things are working anyway without this 
mask.  If you'd prefer to split this change off, we can do that too.

I took this for a spin (with AND without that 1-liner change) 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 (so, no hardware capabilities from vfio) to drive the 
default clp paths against vfio -- Everything works fine there.  I also 
tested (with AND without that 1-liner change) against a tcg guest on x86 
using a virtio-blk-pci device.

So either way, thank you and:

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


>           if (!group) {
>               /* We do not allow access to unknown groups */
>               /* The group must have been obtained with a vfio device */
> @@ -788,7 +788,7 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>       /* Length must be greater than 8, a multiple of 8 */
>       /* and not greater than maxstbl */
>       if ((len <= 8) || (len % 8) ||
> -        (len > pbdev->pci_group->zpci_group.maxstbl)) {
> +        (len > lduw_p(&pbdev->pci_group->zpci_group.maxstbl))) {
>           goto specification_error;
>       }
>       /* Do not cross a 4K-byte boundary */
> diff --git a/hw/s390x/s390-pci-vfio.c b/hw/s390x/s390-pci-vfio.c
> index d5c78063b5bc..f455c6f20a1a 100644
> --- a/hw/s390x/s390-pci-vfio.c
> +++ b/hw/s390x/s390-pci-vfio.c
> @@ -116,10 +116,10 @@ static void s390_pci_read_base(S390PCIBusDevice *pbdev,
>       }
>       cap = (void *) hdr;
>   
> -    pbdev->zpci_fn.sdma = cap->start_dma;
> -    pbdev->zpci_fn.edma = cap->end_dma;
> -    pbdev->zpci_fn.pchid = cap->pchid;
> -    pbdev->zpci_fn.vfn = cap->vfn;
> +    stq_p(&pbdev->zpci_fn.sdma, cap->start_dma);
> +    stq_p(&pbdev->zpci_fn.edma, cap->end_dma);
> +    stw_p(&pbdev->zpci_fn.pchid, cap->pchid);
> +    stw_p(&pbdev->zpci_fn.vfn, cap->vfn);
>       pbdev->zpci_fn.pfgid = cap->gid;
>       /* The following values remain 0 until we support other FMB formats */
>       pbdev->zpci_fn.fmbl = 0;
> 

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

[Thomas Huth]

On 17/11/2020 18.13, Cornelia Huck wrote:
> zPCI control blocks are big endian, we need to take care that we
> do proper accesses in order not to break tcg guests on little endian
> hosts.
> 
> 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")

This fixes the problem with my old Fedora 28 under TCG, too, but... do we
really want to store this information in big endian on the QEMU side (e.g.
in the QTAILQ lists)? ... that smells like trouble again in the future...

I think it would be better if we rather replace all those memcpy() functions
in the patches that you cited in the "Fixes:" lines above with code that
changes the endianess when we copy the date from QEMU space to guest space
and vice versa. What do you think?

 Thomas

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

[Matthew Rosato]

On 11/17/20 2:21 PM, Thomas Huth wrote:
> On 17/11/2020 18.13, Cornelia Huck wrote:
>> zPCI control blocks are big endian, we need to take care that we
>> do proper accesses in order not to break tcg guests on little endian
>> hosts.
>>
>> 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")
> 
> This fixes the problem with my old Fedora 28 under TCG, too, but... do we
> really want to store this information in big endian on the QEMU side (e.g.
> in the QTAILQ lists)? ... that smells like trouble again in the future...
> 
> I think it would be better if we rather replace all those memcpy() functions
> in the patches that you cited in the "Fixes:" lines above with code that
> changes the endianess when we copy the date from QEMU space to guest space
> and vice versa. What do you think?

Hmm, that's actually a fair point...  Such an approach would have the 
advantage of avoiding weird lines like the following:

      memory_region_add_subregion(&pbdev->iommu->mr,
-                                pbdev->pci_group->zpci_group.msia,
+                                ldq_p(&pbdev->pci_group->zpci_group.msia),
                                  &pbdev->msix_notify_mr);


And would keep messing with endianness largely contained to the code 
that handles CLPs.  It does take away the niceness of being able to 
gather the CLP response in one fell memcpy but...  It's not like these 
are done very often (device init).

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

[Cornelia Huck]

On Tue, 17 Nov 2020 14:49:53 -0500
Matthew Rosato <mjrosato@linux.ibm.com> wrote:

> On 11/17/20 2:21 PM, Thomas Huth wrote:
> > On 17/11/2020 18.13, Cornelia Huck wrote:  
> >> zPCI control blocks are big endian, we need to take care that we
> >> do proper accesses in order not to break tcg guests on little endian
> >> hosts.
> >>
> >> 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")  
> > 
> > This fixes the problem with my old Fedora 28 under TCG, too, but... do we
> > really want to store this information in big endian on the QEMU side (e.g.
> > in the QTAILQ lists)? ... that smells like trouble again in the future...
> > 
> > I think it would be better if we rather replace all those memcpy() functions
> > in the patches that you cited in the "Fixes:" lines above with code that
> > changes the endianess when we copy the date from QEMU space to guest space
> > and vice versa. What do you think?  
> 
> Hmm, that's actually a fair point...  Such an approach would have the 
> advantage of avoiding weird lines like the following:
> 
>       memory_region_add_subregion(&pbdev->iommu->mr,
> -                                pbdev->pci_group->zpci_group.msia,
> +                                ldq_p(&pbdev->pci_group->zpci_group.msia),
>                                   &pbdev->msix_notify_mr);
> 
> 
> And would keep messing with endianness largely contained to the code 
> that handles CLPs.  It does take away the niceness of being able to 
> gather the CLP response in one fell memcpy but...  It's not like these 
> are done very often (device init).
> 

Not opposed to it, can try to put a patch together and see what it
looks like. As long as we get this into 5.2 :)