Re: [RFC PATCH-for-5.2] hw/s390x/pci: Fix endianness issue

[Pierre Morel]

On 11/17/20 2:00 PM, Peter Maydell wrote:
> On Tue, 17 Nov 2020 at 12:03, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> > Fix an endianness issue reported by Cornelia:
> >
> > > s390x tcg guest on x86, virtio-pci devices are not detected. The
> > > relevant feature bits are visible to the guest. Same breakage with
> > > different guest kernels.
> > > KVM guests and s390x tcg guests on s390x are fine.
> >
> > Fixes: 28dc86a0729 ("s390x/pci: use a PCI Group structure")
> > Reported-by: Cornelia Huck <cohuck@redhat.com>
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > ---
> > RFC because review-only patch, untested
> > ---
> >   hw/s390x/s390-pci-inst.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> > index 58cd041d17f..cfb54b4d8ec 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));
>
> 'g' in the ClpReqQueryPciGrp struct is a uint32_t, so
> adding the ldl_p() will have no effect unless (a) the
> structure is not 4-aligned and (b) the host will fault on
> unaligned accesses, which isn't the case for x86 hosts.
>
> Q: is this struct really in host order, or should we
> be using ldl_le_p() or ldl_be_p() and friends here and
> elsewhere?
>
> thanks
> -- PMM

Hi, I think we better modify the structure here, g should be a byte.

Connie, can you please try this if it resolves the issue?

diff --git a/hw/s390x/s390-pci-inst.h b/hw/s390x/s390-pci-inst.h
index fa3bf8b5aa..641d19c815 100644
--- a/hw/s390x/s390-pci-inst.h
+++ b/hw/s390x/s390-pci-inst.h
@@ -146,7 +146,8 @@ typedef struct ClpReqQueryPciGrp {
     uint32_t fmt;
     uint64_t reserved1;
 #define CLP_REQ_QPCIG_MASK_PFGID 0xff
-    uint32_t g;
+    uint32_t g0 :24;
+    uint32_t g  :8;
     uint32_t reserved2;
     uint64_t reserved3;
 } QEMU_PACKED ClpReqQueryPciGrp;

--
Pierre Morel
IBM Lab Boeblingen