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;