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

[Matthew Rosato <mjrosato@linux.ibm.com>]

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