[Top][All Lists]

[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: Matthew Rosato
Subject: Re: [RFC PATCH-for-5.2] hw/s390x/pci: Fix endianness issue
Date: Tue, 17 Nov 2020 09:34:41 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.3.1

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

-- 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)

reply via email to

[Prev in Thread] Current Thread [Next in Thread]