hw/s390x/s390-pci-bus.c | 10 +++++----- hw/s390x/s390-pci-inst.c | 14 +++++++++++++- hw/s390x/s390-pci-vfio.c | 12 ++++++------ include/hw/s390x/s390-pci-clp.h | 8 ++++---- 4 files changed, 28 insertions(+), 16 deletions(-)
The zPCI group and function structures are big endian. However, we do
not consistently store them as big endian locally, and are missing some
conversions.
Let's just store the structures as host endian instead and convert to
big endian when actually handling the instructions retrieving the data.
Also fix the layout of ClpReqQueryPciGrp: g is actually only 8 bit. This
also fixes accesses 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>
---
Alternative approach to my patch from yesterday. The change is bigger,
but the end result is arguably nicer.
Again, works for me with virtio-pci devices on x86 and on s390x (tcg/kvm).
Testing with vfio-pci devices would be appreciated.
---
hw/s390x/s390-pci-bus.c | 10 +++++-----
hw/s390x/s390-pci-inst.c | 14 +++++++++++++-
hw/s390x/s390-pci-vfio.c | 12 ++++++------
include/hw/s390x/s390-pci-clp.h | 8 ++++----
4 files changed, 28 insertions(+), 16 deletions(-)
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index e0dc20ce4a56..05f7460aec9b 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -777,11 +777,11 @@ static void s390_pci_init_default_group(void)
group = s390_group_create(ZPCI_DEFAULT_FN_GRP);
resgrp = &group->zpci_group;
resgrp->fr = 1;
- stq_p(&resgrp->dasm, 0);
- stq_p(&resgrp->msia, ZPCI_MSI_ADDR);
- stw_p(&resgrp->mui, DEFAULT_MUI);
- stw_p(&resgrp->i, 128);
- stw_p(&resgrp->maxstbl, 128);
+ resgrp->dasm = 0;
+ resgrp->msia = ZPCI_MSI_ADDR;
+ resgrp->mui = DEFAULT_MUI;
+ resgrp->i = 128;
+ resgrp->maxstbl = 128;
resgrp->version = 0;
}
diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index 58cd041d17fb..6c36201229f3 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -281,7 +281,12 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra)
goto out;
}
- memcpy(resquery, &pbdev->zpci_fn, sizeof(*resquery));
+ stq_p(&resquery->sdma, pbdev->zpci_fn.sdma);
+ stq_p(&resquery->edma, pbdev->zpci_fn.edma);
+ stw_p(&resquery->pchid, pbdev->zpci_fn.pchid);
+ resquery->pfgid = pbdev->zpci_fn.pfgid;
+ stl_p(&resquery->fid, pbdev->zpci_fn.fid);
+ stl_p(&resquery->uid, pbdev->zpci_fn.uid);
for (i = 0; i < PCI_BAR_COUNT; i++) {
uint32_t data = pci_get_long(pbdev->pdev->config +
@@ -313,6 +318,13 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra)
goto out;
}
memcpy(resgrp, &group->zpci_group, sizeof(ClpRspQueryPciGrp));
+ resgrp->fr = group->zpci_group.fr;
+ stq_p(&resgrp->dasm, group->zpci_group.dasm);
+ stq_p(&resgrp->msia, group->zpci_group.msia);
+ stw_p(&resgrp->mui, group->zpci_group.mui);
+ stw_p(&resgrp->i, group->zpci_group.i);
+ stw_p(&resgrp->maxstbl, group->zpci_group.maxstbl);
+ resgrp->version = group->zpci_group.version;
stw_p(&resgrp->hdr.rsp, CLP_RC_OK);
break;
}
diff --git a/hw/s390x/s390-pci-vfio.c b/hw/s390x/s390-pci-vfio.c
index d5c78063b5bc..9296e1bb6efa 100644
--- a/hw/s390x/s390-pci-vfio.c
+++ b/hw/s390x/s390-pci-vfio.c
@@ -156,12 +156,12 @@ static void s390_pci_read_group(S390PCIBusDevice *pbdev,
if (cap->flags & VFIO_DEVICE_INFO_ZPCI_FLAG_REFRESH) {
resgrp->fr = 1;
}
- stq_p(&resgrp->dasm, cap->dasm);
- stq_p(&resgrp->msia, cap->msi_addr);
- stw_p(&resgrp->mui, cap->mui);
- stw_p(&resgrp->i, cap->noi);
- stw_p(&resgrp->maxstbl, cap->maxstbl);
- stb_p(&resgrp->version, cap->version);
+ resgrp->dasm = cap->dasm;
+ resgrp->msia = cap->msi_addr;
+ resgrp->mui = cap->mui;
+ resgrp->i = cap->noi;
+ resgrp->maxstbl = cap->maxstbl;
+ resgrp->version = cap->version;
}
}
diff --git a/include/hw/s390x/s390-pci-clp.h b/include/hw/s390x/s390-pci-clp.h
index ea2b1378cd5a..96b8e3f1331b 100644
--- a/include/hw/s390x/s390-pci-clp.h
+++ b/include/hw/s390x/s390-pci-clp.h
@@ -144,10 +144,10 @@ typedef struct ClpReqQueryPciGrp {
ClpReqHdr hdr;
uint32_t fmt;
uint64_t reserved1;
-#define CLP_REQ_QPCIG_MASK_PFGID 0xff
- uint32_t g;
- uint32_t reserved2;
- uint64_t reserved3;
+ uint8_t reserved2[3];
+ uint8_t g;
+ uint32_t reserved3;
+ uint64_t reserved4;
} QEMU_PACKED ClpReqQueryPciGrp;
/* Query PCI function group response */
--
2.26.2
On Wed, 18 Nov 2020 09:51:54 +0100
Cornelia Huck <cohuck@redhat.com> wrote:
[This is obviously a v2. Should not send patches before the first coffee :/]
> The zPCI group and function structures are big endian. However, we do
> not consistently store them as big endian locally, and are missing some
> conversions.
>
> Let's just store the structures as host endian instead and convert to
> big endian when actually handling the instructions retrieving the data.
>
> Also fix the layout of ClpReqQueryPciGrp: g is actually only 8 bit. This
> also fixes accesses 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>
> ---
>
> Alternative approach to my patch from yesterday. The change is bigger,
> but the end result is arguably nicer.
>
> Again, works for me with virtio-pci devices on x86 and on s390x (tcg/kvm).
>
> Testing with vfio-pci devices would be appreciated.
>
> ---
> hw/s390x/s390-pci-bus.c | 10 +++++-----
> hw/s390x/s390-pci-inst.c | 14 +++++++++++++-
> hw/s390x/s390-pci-vfio.c | 12 ++++++------
> include/hw/s390x/s390-pci-clp.h | 8 ++++----
> 4 files changed, 28 insertions(+), 16 deletions(-)
>
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index e0dc20ce4a56..05f7460aec9b 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -777,11 +777,11 @@ static void s390_pci_init_default_group(void)
> group = s390_group_create(ZPCI_DEFAULT_FN_GRP);
> resgrp = &group->zpci_group;
> resgrp->fr = 1;
> - stq_p(&resgrp->dasm, 0);
> - stq_p(&resgrp->msia, ZPCI_MSI_ADDR);
> - stw_p(&resgrp->mui, DEFAULT_MUI);
> - stw_p(&resgrp->i, 128);
> - stw_p(&resgrp->maxstbl, 128);
> + resgrp->dasm = 0;
> + resgrp->msia = ZPCI_MSI_ADDR;
> + resgrp->mui = DEFAULT_MUI;
> + resgrp->i = 128;
> + resgrp->maxstbl = 128;
> resgrp->version = 0;
> }
>
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index 58cd041d17fb..6c36201229f3 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -281,7 +281,12 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra)
> goto out;
> }
>
> - memcpy(resquery, &pbdev->zpci_fn, sizeof(*resquery));
> + stq_p(&resquery->sdma, pbdev->zpci_fn.sdma);
> + stq_p(&resquery->edma, pbdev->zpci_fn.edma);
> + stw_p(&resquery->pchid, pbdev->zpci_fn.pchid);
> + resquery->pfgid = pbdev->zpci_fn.pfgid;
> + stl_p(&resquery->fid, pbdev->zpci_fn.fid);
> + stl_p(&resquery->uid, pbdev->zpci_fn.uid);
>
> for (i = 0; i < PCI_BAR_COUNT; i++) {
> uint32_t data = pci_get_long(pbdev->pdev->config +
> @@ -313,6 +318,13 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra)
> goto out;
> }
> memcpy(resgrp, &group->zpci_group, sizeof(ClpRspQueryPciGrp));
> + resgrp->fr = group->zpci_group.fr;
> + stq_p(&resgrp->dasm, group->zpci_group.dasm);
> + stq_p(&resgrp->msia, group->zpci_group.msia);
> + stw_p(&resgrp->mui, group->zpci_group.mui);
> + stw_p(&resgrp->i, group->zpci_group.i);
> + stw_p(&resgrp->maxstbl, group->zpci_group.maxstbl);
> + resgrp->version = group->zpci_group.version;
> stw_p(&resgrp->hdr.rsp, CLP_RC_OK);
> break;
> }
> diff --git a/hw/s390x/s390-pci-vfio.c b/hw/s390x/s390-pci-vfio.c
> index d5c78063b5bc..9296e1bb6efa 100644
> --- a/hw/s390x/s390-pci-vfio.c
> +++ b/hw/s390x/s390-pci-vfio.c
> @@ -156,12 +156,12 @@ static void s390_pci_read_group(S390PCIBusDevice *pbdev,
> if (cap->flags & VFIO_DEVICE_INFO_ZPCI_FLAG_REFRESH) {
> resgrp->fr = 1;
> }
> - stq_p(&resgrp->dasm, cap->dasm);
> - stq_p(&resgrp->msia, cap->msi_addr);
> - stw_p(&resgrp->mui, cap->mui);
> - stw_p(&resgrp->i, cap->noi);
> - stw_p(&resgrp->maxstbl, cap->maxstbl);
> - stb_p(&resgrp->version, cap->version);
> + resgrp->dasm = cap->dasm;
> + resgrp->msia = cap->msi_addr;
> + resgrp->mui = cap->mui;
> + resgrp->i = cap->noi;
> + resgrp->maxstbl = cap->maxstbl;
> + resgrp->version = cap->version;
> }
> }
>
> diff --git a/include/hw/s390x/s390-pci-clp.h b/include/hw/s390x/s390-pci-clp.h
> index ea2b1378cd5a..96b8e3f1331b 100644
> --- a/include/hw/s390x/s390-pci-clp.h
> +++ b/include/hw/s390x/s390-pci-clp.h
> @@ -144,10 +144,10 @@ typedef struct ClpReqQueryPciGrp {
> ClpReqHdr hdr;
> uint32_t fmt;
> uint64_t reserved1;
> -#define CLP_REQ_QPCIG_MASK_PFGID 0xff
> - uint32_t g;
> - uint32_t reserved2;
> - uint64_t reserved3;
> + uint8_t reserved2[3];
> + uint8_t g;
> + uint32_t reserved3;
> + uint64_t reserved4;
> } QEMU_PACKED ClpReqQueryPciGrp;
>
> /* Query PCI function group response */
On 18/11/2020 09.51, Cornelia Huck wrote:
> The zPCI group and function structures are big endian. However, we do
> not consistently store them as big endian locally, and are missing some
> conversions.
>
> Let's just store the structures as host endian instead and convert to
> big endian when actually handling the instructions retrieving the data.
>
> Also fix the layout of ClpReqQueryPciGrp: g is actually only 8 bit. This
> also fixes accesses 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>
> ---
>
> Alternative approach to my patch from yesterday. The change is bigger,
> but the end result is arguably nicer.
Looks way better in my eyes, thanks!
[...]
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index 58cd041d17fb..6c36201229f3 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -281,7 +281,12 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra)
> goto out;
> }
>
> - memcpy(resquery, &pbdev->zpci_fn, sizeof(*resquery));
> + stq_p(&resquery->sdma, pbdev->zpci_fn.sdma);
> + stq_p(&resquery->edma, pbdev->zpci_fn.edma);
> + stw_p(&resquery->pchid, pbdev->zpci_fn.pchid);
> + resquery->pfgid = pbdev->zpci_fn.pfgid;
> + stl_p(&resquery->fid, pbdev->zpci_fn.fid);
> + stl_p(&resquery->uid, pbdev->zpci_fn.uid);
Looking at what had been removed in 9670ee7527279, I think you likely miss
this here:
stw_p(&resquery->ug, pbdev->zpci_fn.ug)
?
> for (i = 0; i < PCI_BAR_COUNT; i++) {
> uint32_t data = pci_get_long(pbdev->pdev->config +
> @@ -313,6 +318,13 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra)
> goto out;
> }
> memcpy(resgrp, &group->zpci_group, sizeof(ClpRspQueryPciGrp));
I think you likely could remove the memcpy now, too?
> + resgrp->fr = group->zpci_group.fr;
> + stq_p(&resgrp->dasm, group->zpci_group.dasm);
> + stq_p(&resgrp->msia, group->zpci_group.msia);
> + stw_p(&resgrp->mui, group->zpci_group.mui);
> + stw_p(&resgrp->i, group->zpci_group.i);
> + stw_p(&resgrp->maxstbl, group->zpci_group.maxstbl);
> + resgrp->version = group->zpci_group.version;
> stw_p(&resgrp->hdr.rsp, CLP_RC_OK);
> break;
> }
[...]
> diff --git a/include/hw/s390x/s390-pci-clp.h b/include/hw/s390x/s390-pci-clp.h
> index ea2b1378cd5a..96b8e3f1331b 100644
> --- a/include/hw/s390x/s390-pci-clp.h
> +++ b/include/hw/s390x/s390-pci-clp.h
> @@ -144,10 +144,10 @@ typedef struct ClpReqQueryPciGrp {
> ClpReqHdr hdr;
> uint32_t fmt;
> uint64_t reserved1;
> -#define CLP_REQ_QPCIG_MASK_PFGID 0xff
> - uint32_t g;
> - uint32_t reserved2;
> - uint64_t reserved3;
> + uint8_t reserved2[3];
> + uint8_t g;
> + uint32_t reserved3;
> + uint64_t reserved4;
> } QEMU_PACKED ClpReqQueryPciGrp;
This might even qualify as a separate patch, since it fixes a separate
problem on big endian hosts, too (g should have been masked with 0xff when
read as 32-bit value).
Thomas
On Wed, 18 Nov 2020 10:38:00 +0100
Thomas Huth <thuth@redhat.com> wrote:
> On 18/11/2020 09.51, Cornelia Huck wrote:
> > The zPCI group and function structures are big endian. However, we do
> > not consistently store them as big endian locally, and are missing some
> > conversions.
> >
> > Let's just store the structures as host endian instead and convert to
> > big endian when actually handling the instructions retrieving the data.
> >
> > Also fix the layout of ClpReqQueryPciGrp: g is actually only 8 bit. This
> > also fixes accesses 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>
> > ---
> >
> > Alternative approach to my patch from yesterday. The change is bigger,
> > but the end result is arguably nicer.
>
> Looks way better in my eyes, thanks!
>
> [...]
> > diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> > index 58cd041d17fb..6c36201229f3 100644
> > --- a/hw/s390x/s390-pci-inst.c
> > +++ b/hw/s390x/s390-pci-inst.c
> > @@ -281,7 +281,12 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra)
> > goto out;
> > }
> >
> > - memcpy(resquery, &pbdev->zpci_fn, sizeof(*resquery));
> > + stq_p(&resquery->sdma, pbdev->zpci_fn.sdma);
> > + stq_p(&resquery->edma, pbdev->zpci_fn.edma);
> > + stw_p(&resquery->pchid, pbdev->zpci_fn.pchid);
> > + resquery->pfgid = pbdev->zpci_fn.pfgid;
> > + stl_p(&resquery->fid, pbdev->zpci_fn.fid);
> > + stl_p(&resquery->uid, pbdev->zpci_fn.uid);
>
> Looking at what had been removed in 9670ee7527279, I think you likely miss
> this here:
>
> stw_p(&resquery->ug, pbdev->zpci_fn.ug)
>
> ?
Hm, ug has been split up into pfgid and flags. Only the vfio-pci code
sets flags. It seems I'm only missing copying it during the instruction
emulation, added.
>
> > for (i = 0; i < PCI_BAR_COUNT; i++) {
> > uint32_t data = pci_get_long(pbdev->pdev->config +
> > @@ -313,6 +318,13 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra)
> > goto out;
> > }
> > memcpy(resgrp, &group->zpci_group, sizeof(ClpRspQueryPciGrp));
>
> I think you likely could remove the memcpy now, too?
Yeah, -ENOCOFFEE.
>
> > + resgrp->fr = group->zpci_group.fr;
> > + stq_p(&resgrp->dasm, group->zpci_group.dasm);
> > + stq_p(&resgrp->msia, group->zpci_group.msia);
> > + stw_p(&resgrp->mui, group->zpci_group.mui);
> > + stw_p(&resgrp->i, group->zpci_group.i);
> > + stw_p(&resgrp->maxstbl, group->zpci_group.maxstbl);
> > + resgrp->version = group->zpci_group.version;
> > stw_p(&resgrp->hdr.rsp, CLP_RC_OK);
> > break;
> > }
> [...]
> > diff --git a/include/hw/s390x/s390-pci-clp.h b/include/hw/s390x/s390-pci-clp.h
> > index ea2b1378cd5a..96b8e3f1331b 100644
> > --- a/include/hw/s390x/s390-pci-clp.h
> > +++ b/include/hw/s390x/s390-pci-clp.h
> > @@ -144,10 +144,10 @@ typedef struct ClpReqQueryPciGrp {
> > ClpReqHdr hdr;
> > uint32_t fmt;
> > uint64_t reserved1;
> > -#define CLP_REQ_QPCIG_MASK_PFGID 0xff
> > - uint32_t g;
> > - uint32_t reserved2;
> > - uint64_t reserved3;
> > + uint8_t reserved2[3];
> > + uint8_t g;
> > + uint32_t reserved3;
> > + uint64_t reserved4;
> > } QEMU_PACKED ClpReqQueryPciGrp;
>
> This might even qualify as a separate patch, since it fixes a separate
> problem on big endian hosts, too (g should have been masked with 0xff when
> read as 32-bit value).
It does not really break on be, though. I'd rather not split it out as
an extra patch, but rather tweak the patch description.
© 2016 - 2024 Red Hat, Inc.