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

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

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?