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.