[v3,5/5] vfio-user: Fix config space access byte order

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

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.