| Message ID | 20201117171340.1289659-1-cohuck@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [for-5.2] s390x/pci: fix endianness issues | expand |
On 11/17/20 6:13 PM, Cornelia Huck wrote: > zPCI control blocks are big endian, we need to take care that we > do proper accesses in order not to break tcg guests on little endian > hosts. > > Fixes: 28dc86a07299 ("s390x/pci: use a PCI Group structure") > Fixes: 9670ee752727 ("s390x/pci: use a PCI Function structure") > Fixes: 1e7552ff5c34 ("s390x/pci: get zPCI function info from host") > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > --- > > Works for me with virtio-pci devices for tcg on x86 and s390x, and for kvm. > The vfio changes are not strictly needed; did not test them due to lack of > hardware -- testing appreciated. > > As this fixes a regression, I want this in 5.2. > > --- > hw/s390x/s390-pci-bus.c | 12 ++++++------ > hw/s390x/s390-pci-inst.c | 4 ++-- > hw/s390x/s390-pci-vfio.c | 8 ++++---- > 3 files changed, 12 insertions(+), 12 deletions(-) > > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c > index e0dc20ce4a56..17e64e0b1200 100644 > --- a/hw/s390x/s390-pci-bus.c > +++ b/hw/s390x/s390-pci-bus.c > @@ -787,12 +787,12 @@ static void s390_pci_init_default_group(void) > > static void set_pbdev_info(S390PCIBusDevice *pbdev) > { > - pbdev->zpci_fn.sdma = ZPCI_SDMA_ADDR; > - pbdev->zpci_fn.edma = ZPCI_EDMA_ADDR; > - pbdev->zpci_fn.pchid = 0; > + stq_p(&pbdev->zpci_fn.sdma, ZPCI_SDMA_ADDR); "zPCI control blocks are big endian" so don't we need the _be_ accessors? stq_be_p() etc... > + stq_p(&pbdev->zpci_fn.edma, ZPCI_EDMA_ADDR); > + stw_p(&pbdev->zpci_fn.pchid, 0); > pbdev->zpci_fn.pfgid = ZPCI_DEFAULT_FN_GRP; > - pbdev->zpci_fn.fid = pbdev->fid; > - pbdev->zpci_fn.uid = pbdev->uid; > + stl_p(&pbdev->zpci_fn.fid, pbdev->fid); > + stl_p(&pbdev->zpci_fn.uid, pbdev->uid); > pbdev->pci_group = s390_group_find(ZPCI_DEFAULT_FN_GRP); > } > > @@ -871,7 +871,7 @@ static int s390_pci_msix_init(S390PCIBusDevice *pbdev) > memory_region_init_io(&pbdev->msix_notify_mr, OBJECT(pbdev), > &s390_msi_ctrl_ops, pbdev, name, PAGE_SIZE); > memory_region_add_subregion(&pbdev->iommu->mr, > - pbdev->pci_group->zpci_group.msia, > + ldq_p(&pbdev->pci_group->zpci_group.msia), > &pbdev->msix_notify_mr); > g_free(name); > > diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c > index 58cd041d17fb..7bc6b79f10ce 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)); > if (!group) { > /* We do not allow access to unknown groups */ > /* The group must have been obtained with a vfio device */ > @@ -788,7 +788,7 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr, > /* Length must be greater than 8, a multiple of 8 */ > /* and not greater than maxstbl */ > if ((len <= 8) || (len % 8) || > - (len > pbdev->pci_group->zpci_group.maxstbl)) { > + (len > lduw_p(&pbdev->pci_group->zpci_group.maxstbl))) { > goto specification_error; > } > /* Do not cross a 4K-byte boundary */ > diff --git a/hw/s390x/s390-pci-vfio.c b/hw/s390x/s390-pci-vfio.c > index d5c78063b5bc..f455c6f20a1a 100644 > --- a/hw/s390x/s390-pci-vfio.c > +++ b/hw/s390x/s390-pci-vfio.c > @@ -116,10 +116,10 @@ static void s390_pci_read_base(S390PCIBusDevice *pbdev, > } > cap = (void *) hdr; > > - pbdev->zpci_fn.sdma = cap->start_dma; > - pbdev->zpci_fn.edma = cap->end_dma; > - pbdev->zpci_fn.pchid = cap->pchid; > - pbdev->zpci_fn.vfn = cap->vfn; > + stq_p(&pbdev->zpci_fn.sdma, cap->start_dma); > + stq_p(&pbdev->zpci_fn.edma, cap->end_dma); > + stw_p(&pbdev->zpci_fn.pchid, cap->pchid); > + stw_p(&pbdev->zpci_fn.vfn, cap->vfn); > pbdev->zpci_fn.pfgid = cap->gid; > /* The following values remain 0 until we support other FMB formats */ > pbdev->zpci_fn.fmbl = 0; >
On 11/17/20 12:59 PM, Philippe Mathieu-Daudé wrote: > On 11/17/20 6:13 PM, Cornelia Huck wrote: >> zPCI control blocks are big endian, we need to take care that we >> do proper accesses in order not to break tcg guests on little endian >> hosts. >> >> Fixes: 28dc86a07299 ("s390x/pci: use a PCI Group structure") >> Fixes: 9670ee752727 ("s390x/pci: use a PCI Function structure") >> Fixes: 1e7552ff5c34 ("s390x/pci: get zPCI function info from host") >> Signed-off-by: Cornelia Huck <cohuck@redhat.com> >> --- >> >> Works for me with virtio-pci devices for tcg on x86 and s390x, and for kvm. >> The vfio changes are not strictly needed; did not test them due to lack of >> hardware -- testing appreciated. >> >> As this fixes a regression, I want this in 5.2. >> >> --- >> hw/s390x/s390-pci-bus.c | 12 ++++++------ >> hw/s390x/s390-pci-inst.c | 4 ++-- >> hw/s390x/s390-pci-vfio.c | 8 ++++---- >> 3 files changed, 12 insertions(+), 12 deletions(-) >> >> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c >> index e0dc20ce4a56..17e64e0b1200 100644 >> --- a/hw/s390x/s390-pci-bus.c >> +++ b/hw/s390x/s390-pci-bus.c >> @@ -787,12 +787,12 @@ static void s390_pci_init_default_group(void) >> >> static void set_pbdev_info(S390PCIBusDevice *pbdev) >> { >> - pbdev->zpci_fn.sdma = ZPCI_SDMA_ADDR; >> - pbdev->zpci_fn.edma = ZPCI_EDMA_ADDR; >> - pbdev->zpci_fn.pchid = 0; >> + stq_p(&pbdev->zpci_fn.sdma, ZPCI_SDMA_ADDR); > > "zPCI control blocks are big endian" so don't we > need the _be_ accessors? stq_be_p() etc... > I don't think this is necessary. This is only available for target s390x, which is always big endian... cpu-all.h should define stq_p as stq_be_p for example inside the #if defined(TARGET_WORDS_BIGENDIAN).
On 11/17/20 7:19 PM, Matthew Rosato wrote: > On 11/17/20 12:59 PM, Philippe Mathieu-Daudé wrote: >> On 11/17/20 6:13 PM, Cornelia Huck wrote: >>> zPCI control blocks are big endian, we need to take care that we >>> do proper accesses in order not to break tcg guests on little endian >>> hosts. >>> >>> Fixes: 28dc86a07299 ("s390x/pci: use a PCI Group structure") >>> Fixes: 9670ee752727 ("s390x/pci: use a PCI Function structure") >>> Fixes: 1e7552ff5c34 ("s390x/pci: get zPCI function info from host") >>> Signed-off-by: Cornelia Huck <cohuck@redhat.com> >>> --- >>> >>> Works for me with virtio-pci devices for tcg on x86 and s390x, and >>> for kvm. >>> The vfio changes are not strictly needed; did not test them due to >>> lack of >>> hardware -- testing appreciated. >> >>> As this fixes a regression, I want this in 5.2. >>> >>> --- >>> hw/s390x/s390-pci-bus.c | 12 ++++++------ >>> hw/s390x/s390-pci-inst.c | 4 ++-- >>> hw/s390x/s390-pci-vfio.c | 8 ++++---- >>> 3 files changed, 12 insertions(+), 12 deletions(-) >>> >>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c >>> index e0dc20ce4a56..17e64e0b1200 100644 >>> --- a/hw/s390x/s390-pci-bus.c >>> +++ b/hw/s390x/s390-pci-bus.c >>> @@ -787,12 +787,12 @@ static void s390_pci_init_default_group(void) >>> static void set_pbdev_info(S390PCIBusDevice *pbdev) >>> { >>> - pbdev->zpci_fn.sdma = ZPCI_SDMA_ADDR; >>> - pbdev->zpci_fn.edma = ZPCI_EDMA_ADDR; >>> - pbdev->zpci_fn.pchid = 0; >>> + stq_p(&pbdev->zpci_fn.sdma, ZPCI_SDMA_ADDR); >> >> "zPCI control blocks are big endian" so don't we >> need the _be_ accessors? stq_be_p() etc... >> > > I don't think this is necessary. This is only available for target > s390x, which is always big endian... cpu-all.h should define stq_p as > stq_be_p for example inside the #if defined(TARGET_WORDS_BIGENDIAN). But if you run on little-endian host, you need to byte-swap that, isn't it?
On 17/11/2020 19.30, Philippe Mathieu-Daudé wrote: > On 11/17/20 7:19 PM, Matthew Rosato wrote: >> On 11/17/20 12:59 PM, Philippe Mathieu-Daudé wrote: >>> On 11/17/20 6:13 PM, Cornelia Huck wrote: >>>> zPCI control blocks are big endian, we need to take care that we >>>> do proper accesses in order not to break tcg guests on little endian >>>> hosts. >>>> >>>> Fixes: 28dc86a07299 ("s390x/pci: use a PCI Group structure") >>>> Fixes: 9670ee752727 ("s390x/pci: use a PCI Function structure") >>>> Fixes: 1e7552ff5c34 ("s390x/pci: get zPCI function info from host") >>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com> >>>> --- >>>> >>>> Works for me with virtio-pci devices for tcg on x86 and s390x, and >>>> for kvm. >>>> The vfio changes are not strictly needed; did not test them due to >>>> lack of >>>> hardware -- testing appreciated. >> >>>> As this fixes a regression, I want this in 5.2. >>>> >>>> --- >>>> hw/s390x/s390-pci-bus.c | 12 ++++++------ >>>> hw/s390x/s390-pci-inst.c | 4 ++-- >>>> hw/s390x/s390-pci-vfio.c | 8 ++++---- >>>> 3 files changed, 12 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c >>>> index e0dc20ce4a56..17e64e0b1200 100644 >>>> --- a/hw/s390x/s390-pci-bus.c >>>> +++ b/hw/s390x/s390-pci-bus.c >>>> @@ -787,12 +787,12 @@ static void s390_pci_init_default_group(void) >>>> static void set_pbdev_info(S390PCIBusDevice *pbdev) >>>> { >>>> - pbdev->zpci_fn.sdma = ZPCI_SDMA_ADDR; >>>> - pbdev->zpci_fn.edma = ZPCI_EDMA_ADDR; >>>> - pbdev->zpci_fn.pchid = 0; >>>> + stq_p(&pbdev->zpci_fn.sdma, ZPCI_SDMA_ADDR); >>> >>> "zPCI control blocks are big endian" so don't we >>> need the _be_ accessors? stq_be_p() etc... >>> >> >> I don't think this is necessary. This is only available for target >> s390x, which is always big endian... cpu-all.h should define stq_p as >> stq_be_p for example inside the #if defined(TARGET_WORDS_BIGENDIAN). > > But if you run on little-endian host, you need to byte-swap that, > isn't it? It's done by the macros. They depend on the target endianess. See cpu-all.h. Thomas
On 11/17/20 7:39 PM, Thomas Huth wrote: > On 17/11/2020 19.30, Philippe Mathieu-Daudé wrote: >> On 11/17/20 7:19 PM, Matthew Rosato wrote: >>> On 11/17/20 12:59 PM, Philippe Mathieu-Daudé wrote: >>>> On 11/17/20 6:13 PM, Cornelia Huck wrote: >>>>> zPCI control blocks are big endian, we need to take care that we >>>>> do proper accesses in order not to break tcg guests on little endian >>>>> hosts. >>>>> >>>>> Fixes: 28dc86a07299 ("s390x/pci: use a PCI Group structure") >>>>> Fixes: 9670ee752727 ("s390x/pci: use a PCI Function structure") >>>>> Fixes: 1e7552ff5c34 ("s390x/pci: get zPCI function info from host") >>>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com> >>>>> --- >>>>> >>>>> Works for me with virtio-pci devices for tcg on x86 and s390x, and >>>>> for kvm. >>>>> The vfio changes are not strictly needed; did not test them due to >>>>> lack of >>>>> hardware -- testing appreciated. >> >>>>> As this fixes a regression, I want this in 5.2. >>>>> >>>>> --- >>>>> hw/s390x/s390-pci-bus.c | 12 ++++++------ >>>>> hw/s390x/s390-pci-inst.c | 4 ++-- >>>>> hw/s390x/s390-pci-vfio.c | 8 ++++---- >>>>> 3 files changed, 12 insertions(+), 12 deletions(-) >>>>> >>>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c >>>>> index e0dc20ce4a56..17e64e0b1200 100644 >>>>> --- a/hw/s390x/s390-pci-bus.c >>>>> +++ b/hw/s390x/s390-pci-bus.c >>>>> @@ -787,12 +787,12 @@ static void s390_pci_init_default_group(void) >>>>> static void set_pbdev_info(S390PCIBusDevice *pbdev) >>>>> { >>>>> - pbdev->zpci_fn.sdma = ZPCI_SDMA_ADDR; >>>>> - pbdev->zpci_fn.edma = ZPCI_EDMA_ADDR; >>>>> - pbdev->zpci_fn.pchid = 0; >>>>> + stq_p(&pbdev->zpci_fn.sdma, ZPCI_SDMA_ADDR); >>>> >>>> "zPCI control blocks are big endian" so don't we >>>> need the _be_ accessors? stq_be_p() etc... >>>> >>> >>> I don't think this is necessary. This is only available for target >>> s390x, which is always big endian... cpu-all.h should define stq_p as >>> stq_be_p for example inside the #if defined(TARGET_WORDS_BIGENDIAN). >> >> But if you run on little-endian host, you need to byte-swap that, >> isn't it? > > It's done by the macros. They depend on the target endianess. See cpu-all.h. I'm confused because the description is about target endianness, but stq_p() is about host alignment. If there is no alignment problem, doesn't using stq_p() make code harder to review?
On 11/17/20 12:13 PM, Cornelia Huck wrote: > zPCI control blocks are big endian, we need to take care that we > do proper accesses in order not to break tcg guests on little endian > hosts. > > Fixes: 28dc86a07299 ("s390x/pci: use a PCI Group structure") > Fixes: 9670ee752727 ("s390x/pci: use a PCI Function structure") > Fixes: 1e7552ff5c34 ("s390x/pci: get zPCI function info from host") > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > --- > > Works for me with virtio-pci devices for tcg on x86 and s390x, and for kvm. > The vfio changes are not strictly needed; did not test them due to lack of > hardware -- testing appreciated. > > As this fixes a regression, I want this in 5.2. > > --- > hw/s390x/s390-pci-bus.c | 12 ++++++------ > hw/s390x/s390-pci-inst.c | 4 ++-- > hw/s390x/s390-pci-vfio.c | 8 ++++---- > 3 files changed, 12 insertions(+), 12 deletions(-) > > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c > index e0dc20ce4a56..17e64e0b1200 100644 > --- a/hw/s390x/s390-pci-bus.c > +++ b/hw/s390x/s390-pci-bus.c > @@ -787,12 +787,12 @@ static void s390_pci_init_default_group(void) > > static void set_pbdev_info(S390PCIBusDevice *pbdev) > { > - pbdev->zpci_fn.sdma = ZPCI_SDMA_ADDR; > - pbdev->zpci_fn.edma = ZPCI_EDMA_ADDR; > - pbdev->zpci_fn.pchid = 0; > + stq_p(&pbdev->zpci_fn.sdma, ZPCI_SDMA_ADDR); > + stq_p(&pbdev->zpci_fn.edma, ZPCI_EDMA_ADDR); > + stw_p(&pbdev->zpci_fn.pchid, 0); > pbdev->zpci_fn.pfgid = ZPCI_DEFAULT_FN_GRP; > - pbdev->zpci_fn.fid = pbdev->fid; > - pbdev->zpci_fn.uid = pbdev->uid; > + stl_p(&pbdev->zpci_fn.fid, pbdev->fid); > + stl_p(&pbdev->zpci_fn.uid, pbdev->uid); > pbdev->pci_group = s390_group_find(ZPCI_DEFAULT_FN_GRP); > } > > @@ -871,7 +871,7 @@ static int s390_pci_msix_init(S390PCIBusDevice *pbdev) > memory_region_init_io(&pbdev->msix_notify_mr, OBJECT(pbdev), > &s390_msi_ctrl_ops, pbdev, name, PAGE_SIZE); > memory_region_add_subregion(&pbdev->iommu->mr, > - pbdev->pci_group->zpci_group.msia, > + ldq_p(&pbdev->pci_group->zpci_group.msia), > &pbdev->msix_notify_mr); > g_free(name); > > diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c > index 58cd041d17fb..7bc6b79f10ce 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)); So as I alluded to in my other email, this should really be: + group = s390_group_find(ldl_p(&reqgrp->g) & CLP_REQ_QPCIG_MASK_PFGID); To ensure that only the 8b pfgid is used from the 32b 'g' - doing it this way ensure we've already accounted for endianness. The other 24b are reserved 0s so that's why things are working anyway without this mask. If you'd prefer to split this change off, we can do that too. I took this for a spin (with AND without that 1-liner change) with vfio-pci, driving network and disk workloads using a fairly recent (5.10-rc3) kernel in host/guest. I also rolled back the host to an older kernel (so, no hardware capabilities from vfio) to drive the default clp paths against vfio -- Everything works fine there. I also tested (with AND without that 1-liner change) against a tcg guest on x86 using a virtio-blk-pci device. So either way, thank you and: Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com> > if (!group) { > /* We do not allow access to unknown groups */ > /* The group must have been obtained with a vfio device */ > @@ -788,7 +788,7 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr, > /* Length must be greater than 8, a multiple of 8 */ > /* and not greater than maxstbl */ > if ((len <= 8) || (len % 8) || > - (len > pbdev->pci_group->zpci_group.maxstbl)) { > + (len > lduw_p(&pbdev->pci_group->zpci_group.maxstbl))) { > goto specification_error; > } > /* Do not cross a 4K-byte boundary */ > diff --git a/hw/s390x/s390-pci-vfio.c b/hw/s390x/s390-pci-vfio.c > index d5c78063b5bc..f455c6f20a1a 100644 > --- a/hw/s390x/s390-pci-vfio.c > +++ b/hw/s390x/s390-pci-vfio.c > @@ -116,10 +116,10 @@ static void s390_pci_read_base(S390PCIBusDevice *pbdev, > } > cap = (void *) hdr; > > - pbdev->zpci_fn.sdma = cap->start_dma; > - pbdev->zpci_fn.edma = cap->end_dma; > - pbdev->zpci_fn.pchid = cap->pchid; > - pbdev->zpci_fn.vfn = cap->vfn; > + stq_p(&pbdev->zpci_fn.sdma, cap->start_dma); > + stq_p(&pbdev->zpci_fn.edma, cap->end_dma); > + stw_p(&pbdev->zpci_fn.pchid, cap->pchid); > + stw_p(&pbdev->zpci_fn.vfn, cap->vfn); > pbdev->zpci_fn.pfgid = cap->gid; > /* The following values remain 0 until we support other FMB formats */ > pbdev->zpci_fn.fmbl = 0; >
On 17/11/2020 18.13, Cornelia Huck wrote: > zPCI control blocks are big endian, we need to take care that we > do proper accesses in order not to break tcg guests on little endian > hosts. > > Fixes: 28dc86a07299 ("s390x/pci: use a PCI Group structure") > Fixes: 9670ee752727 ("s390x/pci: use a PCI Function structure") > Fixes: 1e7552ff5c34 ("s390x/pci: get zPCI function info from host") This fixes the problem with my old Fedora 28 under TCG, too, but... do we really want to store this information in big endian on the QEMU side (e.g. in the QTAILQ lists)? ... that smells like trouble again in the future... I think it would be better if we rather replace all those memcpy() functions in the patches that you cited in the "Fixes:" lines above with code that changes the endianess when we copy the date from QEMU space to guest space and vice versa. What do you think? Thomas
On 17/11/2020 19.49, Philippe Mathieu-Daudé wrote: > On 11/17/20 7:39 PM, Thomas Huth wrote: >> On 17/11/2020 19.30, Philippe Mathieu-Daudé wrote: >>> On 11/17/20 7:19 PM, Matthew Rosato wrote: >>>> On 11/17/20 12:59 PM, Philippe Mathieu-Daudé wrote: >>>>> On 11/17/20 6:13 PM, Cornelia Huck wrote: >>>>>> zPCI control blocks are big endian, we need to take care that we >>>>>> do proper accesses in order not to break tcg guests on little endian >>>>>> hosts. >>>>>> >>>>>> Fixes: 28dc86a07299 ("s390x/pci: use a PCI Group structure") >>>>>> Fixes: 9670ee752727 ("s390x/pci: use a PCI Function structure") >>>>>> Fixes: 1e7552ff5c34 ("s390x/pci: get zPCI function info from host") >>>>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com> >>>>>> --- >>>>>> >>>>>> Works for me with virtio-pci devices for tcg on x86 and s390x, and >>>>>> for kvm. >>>>>> The vfio changes are not strictly needed; did not test them due to >>>>>> lack of >>>>>> hardware -- testing appreciated. >> >>>>>> As this fixes a regression, I want this in 5.2. >>>>>> >>>>>> --- >>>>>> hw/s390x/s390-pci-bus.c | 12 ++++++------ >>>>>> hw/s390x/s390-pci-inst.c | 4 ++-- >>>>>> hw/s390x/s390-pci-vfio.c | 8 ++++---- >>>>>> 3 files changed, 12 insertions(+), 12 deletions(-) >>>>>> >>>>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c >>>>>> index e0dc20ce4a56..17e64e0b1200 100644 >>>>>> --- a/hw/s390x/s390-pci-bus.c >>>>>> +++ b/hw/s390x/s390-pci-bus.c >>>>>> @@ -787,12 +787,12 @@ static void s390_pci_init_default_group(void) >>>>>> static void set_pbdev_info(S390PCIBusDevice *pbdev) >>>>>> { >>>>>> - pbdev->zpci_fn.sdma = ZPCI_SDMA_ADDR; >>>>>> - pbdev->zpci_fn.edma = ZPCI_EDMA_ADDR; >>>>>> - pbdev->zpci_fn.pchid = 0; >>>>>> + stq_p(&pbdev->zpci_fn.sdma, ZPCI_SDMA_ADDR); >>>>> >>>>> "zPCI control blocks are big endian" so don't we >>>>> need the _be_ accessors? stq_be_p() etc... >>>>> >>>> >>>> I don't think this is necessary. This is only available for target >>>> s390x, which is always big endian... cpu-all.h should define stq_p as >>>> stq_be_p for example inside the #if defined(TARGET_WORDS_BIGENDIAN). >>> >>> But if you run on little-endian host, you need to byte-swap that, >>> isn't it? >> >> It's done by the macros. They depend on the target endianess. See cpu-all.h. > > I'm confused because the description is about target endianness, > but stq_p() is about host alignment. stq_p() is apparently also about endianess. Why would it depend on TARGET_WORDS_BIGENDIAN otherwise? Thomas
On 11/17/20 2:21 PM, Thomas Huth wrote: > On 17/11/2020 18.13, Cornelia Huck wrote: >> zPCI control blocks are big endian, we need to take care that we >> do proper accesses in order not to break tcg guests on little endian >> hosts. >> >> Fixes: 28dc86a07299 ("s390x/pci: use a PCI Group structure") >> Fixes: 9670ee752727 ("s390x/pci: use a PCI Function structure") >> Fixes: 1e7552ff5c34 ("s390x/pci: get zPCI function info from host") > > This fixes the problem with my old Fedora 28 under TCG, too, but... do we > really want to store this information in big endian on the QEMU side (e.g. > in the QTAILQ lists)? ... that smells like trouble again in the future... > > I think it would be better if we rather replace all those memcpy() functions > in the patches that you cited in the "Fixes:" lines above with code that > changes the endianess when we copy the date from QEMU space to guest space > and vice versa. What do you think? Hmm, that's actually a fair point... Such an approach would have the advantage of avoiding weird lines like the following: memory_region_add_subregion(&pbdev->iommu->mr, - pbdev->pci_group->zpci_group.msia, + ldq_p(&pbdev->pci_group->zpci_group.msia), &pbdev->msix_notify_mr); And would keep messing with endianness largely contained to the code that handles CLPs. It does take away the niceness of being able to gather the CLP response in one fell memcpy but... It's not like these are done very often (device init).
On Tue, 17 Nov 2020 14:49:53 -0500 Matthew Rosato <mjrosato@linux.ibm.com> wrote: > On 11/17/20 2:21 PM, Thomas Huth wrote: > > On 17/11/2020 18.13, Cornelia Huck wrote: > >> zPCI control blocks are big endian, we need to take care that we > >> do proper accesses in order not to break tcg guests on little endian > >> hosts. > >> > >> Fixes: 28dc86a07299 ("s390x/pci: use a PCI Group structure") > >> Fixes: 9670ee752727 ("s390x/pci: use a PCI Function structure") > >> Fixes: 1e7552ff5c34 ("s390x/pci: get zPCI function info from host") > > > > This fixes the problem with my old Fedora 28 under TCG, too, but... do we > > really want to store this information in big endian on the QEMU side (e.g. > > in the QTAILQ lists)? ... that smells like trouble again in the future... > > > > I think it would be better if we rather replace all those memcpy() functions > > in the patches that you cited in the "Fixes:" lines above with code that > > changes the endianess when we copy the date from QEMU space to guest space > > and vice versa. What do you think? > > Hmm, that's actually a fair point... Such an approach would have the > advantage of avoiding weird lines like the following: > > memory_region_add_subregion(&pbdev->iommu->mr, > - pbdev->pci_group->zpci_group.msia, > + ldq_p(&pbdev->pci_group->zpci_group.msia), > &pbdev->msix_notify_mr); > > > And would keep messing with endianness largely contained to the code > that handles CLPs. It does take away the niceness of being able to > gather the CLP response in one fell memcpy but... It's not like these > are done very often (device init). > Not opposed to it, can try to put a patch together and see what it looks like. As long as we get this into 5.2 :)
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c index e0dc20ce4a56..17e64e0b1200 100644 --- a/hw/s390x/s390-pci-bus.c +++ b/hw/s390x/s390-pci-bus.c @@ -787,12 +787,12 @@ static void s390_pci_init_default_group(void) static void set_pbdev_info(S390PCIBusDevice *pbdev) { - pbdev->zpci_fn.sdma = ZPCI_SDMA_ADDR; - pbdev->zpci_fn.edma = ZPCI_EDMA_ADDR; - pbdev->zpci_fn.pchid = 0; + stq_p(&pbdev->zpci_fn.sdma, ZPCI_SDMA_ADDR); + stq_p(&pbdev->zpci_fn.edma, ZPCI_EDMA_ADDR); + stw_p(&pbdev->zpci_fn.pchid, 0); pbdev->zpci_fn.pfgid = ZPCI_DEFAULT_FN_GRP; - pbdev->zpci_fn.fid = pbdev->fid; - pbdev->zpci_fn.uid = pbdev->uid; + stl_p(&pbdev->zpci_fn.fid, pbdev->fid); + stl_p(&pbdev->zpci_fn.uid, pbdev->uid); pbdev->pci_group = s390_group_find(ZPCI_DEFAULT_FN_GRP); } @@ -871,7 +871,7 @@ static int s390_pci_msix_init(S390PCIBusDevice *pbdev) memory_region_init_io(&pbdev->msix_notify_mr, OBJECT(pbdev), &s390_msi_ctrl_ops, pbdev, name, PAGE_SIZE); memory_region_add_subregion(&pbdev->iommu->mr, - pbdev->pci_group->zpci_group.msia, + ldq_p(&pbdev->pci_group->zpci_group.msia), &pbdev->msix_notify_mr); g_free(name); diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c index 58cd041d17fb..7bc6b79f10ce 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)); if (!group) { /* We do not allow access to unknown groups */ /* The group must have been obtained with a vfio device */ @@ -788,7 +788,7 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr, /* Length must be greater than 8, a multiple of 8 */ /* and not greater than maxstbl */ if ((len <= 8) || (len % 8) || - (len > pbdev->pci_group->zpci_group.maxstbl)) { + (len > lduw_p(&pbdev->pci_group->zpci_group.maxstbl))) { goto specification_error; } /* Do not cross a 4K-byte boundary */ diff --git a/hw/s390x/s390-pci-vfio.c b/hw/s390x/s390-pci-vfio.c index d5c78063b5bc..f455c6f20a1a 100644 --- a/hw/s390x/s390-pci-vfio.c +++ b/hw/s390x/s390-pci-vfio.c @@ -116,10 +116,10 @@ static void s390_pci_read_base(S390PCIBusDevice *pbdev, } cap = (void *) hdr; - pbdev->zpci_fn.sdma = cap->start_dma; - pbdev->zpci_fn.edma = cap->end_dma; - pbdev->zpci_fn.pchid = cap->pchid; - pbdev->zpci_fn.vfn = cap->vfn; + stq_p(&pbdev->zpci_fn.sdma, cap->start_dma); + stq_p(&pbdev->zpci_fn.edma, cap->end_dma); + stw_p(&pbdev->zpci_fn.pchid, cap->pchid); + stw_p(&pbdev->zpci_fn.vfn, cap->vfn); pbdev->zpci_fn.pfgid = cap->gid; /* The following values remain 0 until we support other FMB formats */ pbdev->zpci_fn.fmbl = 0;
zPCI control blocks are big endian, we need to take care that we do proper accesses in order not to break tcg guests on little endian hosts. Fixes: 28dc86a07299 ("s390x/pci: use a PCI Group structure") Fixes: 9670ee752727 ("s390x/pci: use a PCI Function structure") Fixes: 1e7552ff5c34 ("s390x/pci: get zPCI function info from host") Signed-off-by: Cornelia Huck <cohuck@redhat.com> --- Works for me with virtio-pci devices for tcg on x86 and s390x, and for kvm. The vfio changes are not strictly needed; did not test them due to lack of hardware -- testing appreciated. As this fixes a regression, I want this in 5.2. --- hw/s390x/s390-pci-bus.c | 12 ++++++------ hw/s390x/s390-pci-inst.c | 4 ++-- hw/s390x/s390-pci-vfio.c | 8 ++++---- 3 files changed, 12 insertions(+), 12 deletions(-)