| Message ID | 20230907130410.498935-6-mnissler@rivosinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support message-based DMA in vfio-user server | expand |
On Thu, Sep 07, 2023 at 06:04:10AM -0700, Mattias Nissler wrote: > PCI config space is little-endian, so on a big-endian host we need to > perform byte swaps for values as they are passed to and received from > the generic PCI config space access machinery. Byteswapping only works when registers are accessed with their natural size, even with this patch. If there is something like a PCI Capability structure, then it needs to be read one register at a time to get back valid data. It cannot simply be copied in a single multi-DWORD access. I'm not sure if this fix is sufficient. Maybe pci_host_*() needs to be extended to support little-endian accesses instead? > > Signed-off-by: Mattias Nissler <mnissler@rivosinc.com> > --- > hw/remote/vfio-user-obj.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c > index cee5e615a9..d38b4700f3 100644 > --- a/hw/remote/vfio-user-obj.c > +++ b/hw/remote/vfio-user-obj.c > @@ -281,7 +281,7 @@ static ssize_t vfu_object_cfg_access(vfu_ctx_t *vfu_ctx, char * const buf, > while (bytes > 0) { > len = (bytes > pci_access_width) ? pci_access_width : bytes; > if (is_write) { > - memcpy(&val, ptr, len); > + val = ldn_le_p(ptr, len); > pci_host_config_write_common(o->pci_dev, offset, > pci_config_size(o->pci_dev), > val, len); > @@ -289,7 +289,7 @@ static ssize_t vfu_object_cfg_access(vfu_ctx_t *vfu_ctx, char * const buf, > } else { > val = pci_host_config_read_common(o->pci_dev, offset, > pci_config_size(o->pci_dev), len); > - memcpy(ptr, &val, len); > + stn_le_p(ptr, len, val); > trace_vfu_cfg_read(offset, val); > } > offset += len; > -- > 2.34.1 >
On 7/9/23 15:04, Mattias Nissler wrote: > PCI config space is little-endian, so on a big-endian host we need to > perform byte swaps for values as they are passed to and received from > the generic PCI config space access machinery. > > Signed-off-by: Mattias Nissler <mnissler@rivosinc.com> > --- > hw/remote/vfio-user-obj.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c > index cee5e615a9..d38b4700f3 100644 > --- a/hw/remote/vfio-user-obj.c > +++ b/hw/remote/vfio-user-obj.c > @@ -281,7 +281,7 @@ static ssize_t vfu_object_cfg_access(vfu_ctx_t *vfu_ctx, char * const buf, > while (bytes > 0) { > len = (bytes > pci_access_width) ? pci_access_width : bytes; > if (is_write) { > - memcpy(&val, ptr, len); > + val = ldn_le_p(ptr, len); > pci_host_config_write_common(o->pci_dev, offset, > pci_config_size(o->pci_dev), > val, len); > @@ -289,7 +289,7 @@ static ssize_t vfu_object_cfg_access(vfu_ctx_t *vfu_ctx, char * const buf, > } else { > val = pci_host_config_read_common(o->pci_dev, offset, > pci_config_size(o->pci_dev), len); > - memcpy(ptr, &val, len); > + stn_le_p(ptr, len, val); > trace_vfu_cfg_read(offset, val); > } > offset += len; This makes sense, Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On Thu, Sep 07, 2023 at 06:04:10AM -0700, Mattias Nissler wrote: > PCI config space is little-endian, so on a big-endian host we need to > perform byte swaps for values as they are passed to and received from > the generic PCI config space access machinery. > > Signed-off-by: Mattias Nissler <mnissler@rivosinc.com> > --- > hw/remote/vfio-user-obj.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) After some discussion about PCI Configuration Space endianness on IRC with aw, mcayland, and f4bug I am now happy with this patch: 1. Configuration space can only be accessed in 1-, 2-, or 4-byte accesses. 2. If it's a 2- or 4-byte access then your patch adds the missing little-endian conversion. 3. If it's a 1-byte access then there is (effectively) no byteswap in the code path and the pci_dev->config[] array is already little-endian. Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On Thu, Sep 14, 2023 at 10:32 PM Stefan Hajnoczi <stefanha@redhat.com> wrote: > > On Thu, Sep 07, 2023 at 06:04:10AM -0700, Mattias Nissler wrote: > > PCI config space is little-endian, so on a big-endian host we need to > > perform byte swaps for values as they are passed to and received from > > the generic PCI config space access machinery. > > > > Signed-off-by: Mattias Nissler <mnissler@rivosinc.com> > > --- > > hw/remote/vfio-user-obj.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > After some discussion about PCI Configuration Space endianness on IRC > with aw, mcayland, and f4bug I am now happy with this patch: > > 1. Configuration space can only be accessed in 1-, 2-, or 4-byte > accesses. > 2. If it's a 2- or 4-byte access then your patch adds the missing > little-endian conversion. > 3. If it's a 1-byte access then there is (effectively) no byteswap in > the code path and the pci_dev->config[] array is already > little-endian. Thanks for checking! This indeed relies on the pci_host_config_{read,write}_common to be register-based access paths. I have also experimentally verified that this works as expected using a s390x build.
diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c index cee5e615a9..d38b4700f3 100644 --- a/hw/remote/vfio-user-obj.c +++ b/hw/remote/vfio-user-obj.c @@ -281,7 +281,7 @@ static ssize_t vfu_object_cfg_access(vfu_ctx_t *vfu_ctx, char * const buf, while (bytes > 0) { len = (bytes > pci_access_width) ? pci_access_width : bytes; if (is_write) { - memcpy(&val, ptr, len); + val = ldn_le_p(ptr, len); pci_host_config_write_common(o->pci_dev, offset, pci_config_size(o->pci_dev), val, len); @@ -289,7 +289,7 @@ static ssize_t vfu_object_cfg_access(vfu_ctx_t *vfu_ctx, char * const buf, } else { val = pci_host_config_read_common(o->pci_dev, offset, pci_config_size(o->pci_dev), len); - memcpy(ptr, &val, len); + stn_le_p(ptr, len, val); trace_vfu_cfg_read(offset, val); } offset += len;
PCI config space is little-endian, so on a big-endian host we need to perform byte swaps for values as they are passed to and received from the generic PCI config space access machinery. Signed-off-by: Mattias Nissler <mnissler@rivosinc.com> --- hw/remote/vfio-user-obj.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)