[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH-for-5.2] hw/s390x/pci: Fix endianness issue
From: |
Thomas Huth |
Subject: |
Re: [RFC PATCH-for-5.2] hw/s390x/pci: Fix endianness issue |
Date: |
Tue, 17 Nov 2020 19:28:48 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 |
On 17/11/2020 15.34, Matthew Rosato 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
As Peter also already wrote: Bitfields are not endianess safe either. You'd
need to replace the g0:24 with "uint8_t g0[3]" to get it working that way.
Thomas
- Re: [RFC PATCH-for-5.2] hw/s390x/pci: Fix endianness issue, (continued)
- Re: [RFC PATCH-for-5.2] hw/s390x/pci: Fix endianness issue, Philippe Mathieu-Daudé, 2020/11/17
- Re: [RFC PATCH-for-5.2] hw/s390x/pci: Fix endianness issue, Pierre Morel, 2020/11/17
- Re: [RFC PATCH-for-5.2] hw/s390x/pci: Fix endianness issue, Cornelia Huck, 2020/11/17
- Re: [RFC PATCH-for-5.2] hw/s390x/pci: Fix endianness issue, Matthew Rosato, 2020/11/17
- Re: [RFC PATCH-for-5.2] hw/s390x/pci: Fix endianness issue, Cornelia Huck, 2020/11/17
- Re: [RFC PATCH-for-5.2] hw/s390x/pci: Fix endianness issue, Matthew Rosato, 2020/11/17
- Re: [RFC PATCH-for-5.2] hw/s390x/pci: Fix endianness issue, Cornelia Huck, 2020/11/17
- Re: [RFC PATCH-for-5.2] hw/s390x/pci: Fix endianness issue, Matthew Rosato, 2020/11/17
- Re: [RFC PATCH-for-5.2] hw/s390x/pci: Fix endianness issue, Cornelia Huck, 2020/11/17
- Re: [RFC PATCH-for-5.2] hw/s390x/pci: Fix endianness issue,
Thomas Huth <=
- Re: [RFC PATCH-for-5.2] hw/s390x/pci: Fix endianness issue, Peter Maydell, 2020/11/17