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

[Matthew Rosato <mjrosato@linux.ibm.com>]

On 11/17/20 10:17 AM, Cornelia Huck wrote:
> 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 :)
> 

OK, I just got the issue reproduced here (no PCI without Phil's patch, 
crash with Phil's patch);  Let me investigate and I will get back.