diff mbox series

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

Message ID 20230920080622.3600226-6-mnissler@rivosinc.com (mailing list archive)
State New, archived
Headers show
Series Support message-based DMA in vfio-user server | expand

Commit Message

Mattias Nissler Sept. 20, 2023, 8:06 a.m. UTC 
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(-)

Comments

Jag Raman Oct. 5, 2023, 4:30 p.m. UTC | #1 
> On Sep 20, 2023, at 4:06 AM, Mattias Nissler <mnissler@rivosinc.com> 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 6a561f7969..6043a91b11 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
> 

Hey,

When you tested on s390x, could you see the correct values for the config space in the Kernel? For example, were any known device's vendor and device IDs valid?

I'm asking because flatview_read_continue() / flatview_write_continue() does endianness adjustment. So, I want to confirm that the endianness adjustment in your code also makes sense from Kernel's perspective.

I'm trying to access a big-endian system, but no luck.

#0  0x0000555555b97a30 in vfio_user_io_region_read (vbasedev=0x555557802c80, index=7 '\a', off=4, size=2, data=0x7fff6cfb945c) at ../hw/vfio/user.c:1985
#1  0x0000555555b8dcfb in vfio_pci_read_config (pdev=0x555557802250, addr=4, len=2) at ../hw/vfio/pci.c:1202
#2  0x000055555599d3f9 in pci_host_config_read_common (pci_dev=0x555557802250, addr=addr@entry=4, limit=<optimized out>, limit@entry=256, len=len@entry=2) at ../hw/pci/pci_host.c:107
#3  0x000055555599d74a in pci_data_read (s=<optimized out>, addr=2147493892, len=2) at ../hw/pci/pci_host.c:143
#4  0x000055555599d84f in pci_host_data_read (opaque=<optimized out>, addr=<optimized out>, len=<optimized out>) at ../hw/pci/pci_host.c:188
#5  0x0000555555bc3c4d in memory_region_read_accessor (mr=mr@entry=0x5555569de370, addr=0, value=value@entry=0x7fff6cfb9710, size=size@entry=2, shift=0, mask=mask@entry=65535, attrs=...) at ../softmmu/memory.c:441
#6  0x0000555555bc3fce in access_with_adjusted_size (addr=addr@entry=0, value=value@entry=0x7fff6cfb9710, size=size@entry=2, access_size_min=<optimized out>, access_size_max=<optimized out>, access_fn=0x555555bc3c10 <memory_region_read_accessor>, mr=0x5555569de370, attrs=...) at ../softmmu/memory.c:569
#7  0x0000555555bc41a1 in memory_region_dispatch_read1 (attrs=..., size=2, pval=0x7fff6cfb9710, addr=<optimized out>, mr=<optimized out>) at ../softmmu/memory.c:1443
#8  0x0000555555bc41a1 in memory_region_dispatch_read (mr=mr@entry=0x5555569de370, addr=<optimized out>, pval=pval@entry=0x7fff6cfb9710, op=MO_16, attrs=attrs@entry=...) at ../softmmu/memory.c:1476
#9  0x0000555555bce13b in flatview_read_continue (fv=fv@entry=0x7fff6861e120, addr=addr@entry=3324, attrs=..., ptr=ptr@entry=0x7ffff7fdf000, len=len@entry=2, addr1=<optimized out>, l=<optimized out>, mr=0x5555569de370) at /home/github/oracle/qemu/include/qemu/host-utils.h:219
#10 0x0000555555bce2f7 in flatview_read (fv=0x7fff6861e120, addr=addr@entry=3324, attrs=attrs@entry=..., buf=buf@entry=0x7ffff7fdf000, len=len@entry=2) at ../softmmu/physmem.c:2762
#11 0x0000555555bce448 in address_space_read_full (as=0x555556671620 <address_space_io>, addr=3324, attrs=..., buf=0x7ffff7fdf000, len=2) at ../softmmu/physmem.c:2775
#12 0x0000555555bce595 in address_space_rw (as=as@entry=0x555556671620 <address_space_io>, addr=addr@entry=3324, attrs=..., attrs@entry=..., buf=<optimized out>, len=len@entry=2, is_write=is_write@entry=false) at ../softmmu/physmem.c:2803
#13 0x0000555555c1973f in kvm_handle_io (count=1, size=2, direction=<optimized out>, data=<optimized out>, attrs=..., port=3324) at ../accel/kvm/kvm-all.c:2778
#14 0x0000555555c1973f in kvm_cpu_exec (cpu=cpu@entry=0x5555569ab390) at ../accel/kvm/kvm-all.c:3029
#15 0x0000555555c1a8dd in kvm_vcpu_thread_fn (arg=arg@entry=0x5555569ab390) at ../accel/kvm/kvm-accel-ops.c:51
#16 0x0000555555d8f4fb in qemu_thread_start (args=<optimized out>) at ../util/qemu-thread-posix.c:541
#17 0x00007ffff577dea5 in start_thread () at /lib64/libpthread.so.0
#18 0x00007ffff54a6b0d in clone () at /lib64/libc.so.6

FYI, the above is the stack trace from the client. vfio_user_io_region_read() in the above sends a message to the server, and the server ends up calling either vfu_object_cfg_access() or vfu_object_bar_rw()  (which also does the endianness correction) depending on the region.

--
Jag
Mattias Nissler Oct. 6, 2023, 6:44 a.m. UTC | #2 
On Thu, Oct 5, 2023 at 6:30 PM Jag Raman <jag.raman@oracle.com> wrote:

>
>
> > On Sep 20, 2023, at 4:06 AM, Mattias Nissler <mnissler@rivosinc.com>
> 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 6a561f7969..6043a91b11 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
> >
>
> Hey,
>
> When you tested on s390x, could you see the correct values for the config
> space in the Kernel? For example, were any known device's vendor and device
> IDs valid?
>

I don't exactly remember whether I checked vendor and device IDs, but I've
done something more comprehensive: I set up a qemu vfio-user server
exposing an nvme device and then connected a qemu client (with the
vfio-user client patches from the oracle qemu github repo). Linux running
in the client probes the nvme device successfully and I've mounted a file
system on it. Both qemu binaries are s390x.


>
> I'm asking because flatview_read_continue() / flatview_write_continue()
> does endianness adjustment. So, I want to confirm that the endianness
> adjustment in your code also makes sense from Kernel's perspective.
>

The conversion in the flatview access path is adjusting from the endianness
of the memory region to what the emulated CPU needs. Since the PCI config
space memory region is little-endian (see pci_host_data_le_ops), we're
doing a swap there. The code I'm changing is backing the memory region, so
incoming/outgoing data for writes/reads must be in little-endian to adhere
to the endianness declared by the memory region.


>
> I'm trying to access a big-endian system, but no luck.
>

Btw. I don't have access to big-endian hardware either, but it was
surprisingly straightforward to make my x86 ubuntu machine run s390x
binaries via multiarch + qemu user mode (qemu turtles all the way down :-D)


>
> #0  0x0000555555b97a30 in vfio_user_io_region_read
> (vbasedev=0x555557802c80, index=7 '\a', off=4, size=2, data=0x7fff6cfb945c)
> at ../hw/vfio/user.c:1985
> #1  0x0000555555b8dcfb in vfio_pci_read_config (pdev=0x555557802250,
> addr=4, len=2) at ../hw/vfio/pci.c:1202
> #2  0x000055555599d3f9 in pci_host_config_read_common
> (pci_dev=0x555557802250, addr=addr@entry=4, limit=<optimized out>,
> limit@entry=256, len=len@entry=2) at ../hw/pci/pci_host.c:107
> #3  0x000055555599d74a in pci_data_read (s=<optimized out>,
> addr=2147493892, len=2) at ../hw/pci/pci_host.c:143
> #4  0x000055555599d84f in pci_host_data_read (opaque=<optimized out>,
> addr=<optimized out>, len=<optimized out>) at ../hw/pci/pci_host.c:188
> #5  0x0000555555bc3c4d in memory_region_read_accessor (mr=mr@entry=0x5555569de370,
> addr=0, value=value@entry=0x7fff6cfb9710, size=size@entry=2, shift=0,
> mask=mask@entry=65535, attrs=...) at ../softmmu/memory.c:441
> #6  0x0000555555bc3fce in access_with_adjusted_size (addr=addr@entry=0,
> value=value@entry=0x7fff6cfb9710, size=size@entry=2,
> access_size_min=<optimized out>, access_size_max=<optimized out>,
> access_fn=0x555555bc3c10 <memory_region_read_accessor>, mr=0x5555569de370,
> attrs=...) at ../softmmu/memory.c:569
> #7  0x0000555555bc41a1 in memory_region_dispatch_read1 (attrs=..., size=2,
> pval=0x7fff6cfb9710, addr=<optimized out>, mr=<optimized out>) at
> ../softmmu/memory.c:1443
> #8  0x0000555555bc41a1 in memory_region_dispatch_read (mr=mr@entry=0x5555569de370,
> addr=<optimized out>, pval=pval@entry=0x7fff6cfb9710, op=MO_16,
> attrs=attrs@entry=...) at ../softmmu/memory.c:1476
> #9  0x0000555555bce13b in flatview_read_continue (fv=fv@entry=0x7fff6861e120,
> addr=addr@entry=3324, attrs=..., ptr=ptr@entry=0x7ffff7fdf000,
> len=len@entry=2, addr1=<optimized out>, l=<optimized out>,
> mr=0x5555569de370) at /home/github/oracle/qemu/include/qemu/host-utils.h:219
> #10 0x0000555555bce2f7 in flatview_read (fv=0x7fff6861e120, addr=addr@entry=3324,
> attrs=attrs@entry=..., buf=buf@entry=0x7ffff7fdf000, len=len@entry=2) at
> ../softmmu/physmem.c:2762
> #11 0x0000555555bce448 in address_space_read_full (as=0x555556671620
> <address_space_io>, addr=3324, attrs=..., buf=0x7ffff7fdf000, len=2) at
> ../softmmu/physmem.c:2775
> #12 0x0000555555bce595 in address_space_rw (as=as@entry=0x555556671620
> <address_space_io>, addr=addr@entry=3324, attrs=..., attrs@entry=...,
> buf=<optimized out>, len=len@entry=2, is_write=is_write@entry=false) at
> ../softmmu/physmem.c:2803
> #13 0x0000555555c1973f in kvm_handle_io (count=1, size=2,
> direction=<optimized out>, data=<optimized out>, attrs=..., port=3324) at
> ../accel/kvm/kvm-all.c:2778
> #14 0x0000555555c1973f in kvm_cpu_exec (cpu=cpu@entry=0x5555569ab390) at
> ../accel/kvm/kvm-all.c:3029
> #15 0x0000555555c1a8dd in kvm_vcpu_thread_fn (arg=arg@entry=0x5555569ab390)
> at ../accel/kvm/kvm-accel-ops.c:51
> #16 0x0000555555d8f4fb in qemu_thread_start (args=<optimized out>) at
> ../util/qemu-thread-posix.c:541
> #17 0x00007ffff577dea5 in start_thread () at /lib64/libpthread.so.0
> #18 0x00007ffff54a6b0d in clone () at /lib64/libc.so.6
>
> FYI, the above is the stack trace from the client.
> vfio_user_io_region_read() in the above sends a message to the server, and
> the server ends up calling either vfu_object_cfg_access() or
> vfu_object_bar_rw()  (which also does the endianness correction) depending
> on the region.
>
> --
> Jag
>
>
diff mbox series

Patch

diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
index 6a561f7969..6043a91b11 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;