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

[Cornelia Huck]

On Tue, 17 Nov 2020 09:34:41 -0500
Matthew Rosato <mjrosato@linux.ibm.com> wrote:

> On 11/17/20 9:13 AM, Cornelia Huck wrote:
> > On Tue, 17 Nov 2020 09:02:37 -0500
> > Matthew Rosato <mjrosato@linux.ibm.com> wrote:
> >   
> >> On 11/17/20 8:31 AM, Cornelia Huck wrote:  
> >>> On Tue, 17 Nov 2020 14:23:57 +0100
> >>> Pierre Morel <pmorel@linux.ibm.com> wrote:
> >>>      
> >>>> 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;
> >>>>     
> >>>
> >>> No, same crash... I fear there are more things broken wrt endianness.
> >>>      
> >>
> >> Sorry, just getting online now, looking at the code....  Are the 2
> >> memcpy calls added in 9670ee75 and 28dc86a0 the issue?  Won't they just
> >> present the Q PCI FN / Q PCI FN GRP results in host endianness?
> >>  
> > 
> > I just re-added some st?_p operations in set_pbdev_info and that fixes
> > at least the crash I was seeing with Phil's patch applied. Still, no
> > pci functions get detected, so that's not enough. Those memcpy calls
> > look like a possible culprit.
> >   
> 
> OK, so if everything in set_pbdev_info and s390_pci_init_default_group() 
> is handled with st?_p operations, then the memcpy should be OK...
> 
> Pierre was on to something with his recommendation, as the group id is 
> only 1B of the 'g' field (see CLP_REQ_QPCIG_MASK_PFGID) - the other bits 
> just happen to be unused.
> 
> Did you include his change with your st?_p changes to set_pbdev_info 
> (sorry, I don't have this environment set up but clearly need to do so 
> for future testing)

I tried in conjunction with Phil's patch (otherwise, I don't even get
to the part where it crashes.) Do we need to apply that mask somewhere?
It is hard to guess if you don't know what the structure is supposed to
look like :)