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