[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: |
Cornelia Huck |
Subject: |
Re: [RFC PATCH-for-5.2] hw/s390x/pci: Fix endianness issue |
Date: |
Tue, 17 Nov 2020 17:43:15 +0100 |
On Tue, 17 Nov 2020 11:01:31 -0500
Matthew Rosato <mjrosato@linux.ibm.com> wrote:
> 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.
I think I may have something (there were some more fields that needed
care). Let me check whether it works on s390x-on-s390x as well, then
I'll polish it and post.
(Still hoping to get that included; I have two other fixes queued.)
- 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, Peter Maydell, 2020/11/17
- 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 <=
- Re: [RFC PATCH-for-5.2] hw/s390x/pci: Fix endianness issue, Thomas Huth, 2020/11/17
- Re: [RFC PATCH-for-5.2] hw/s390x/pci: Fix endianness issue, Peter Maydell, 2020/11/17