| Message ID | 5f371fa15eb347317ce9ce56a329a24c713129e0.1630084211.git.jag.raman@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio-user server in QEMU | expand |
On Fri, Aug 27, 2021 at 01:53:25PM -0400, Jagannathan Raman wrote: > +static ssize_t vfu_object_cfg_access(vfu_ctx_t *vfu_ctx, char * const buf, > + size_t count, loff_t offset, > + const bool is_write) > +{ > + VfuObject *o = vfu_get_private(vfu_ctx); > + uint32_t pci_access_width = sizeof(uint32_t); > + size_t bytes = count; > + uint32_t val = 0; > + char *ptr = buf; > + int len; > + > + while (bytes > 0) { > + len = (bytes > pci_access_width) ? pci_access_width : bytes; > + if (is_write) { > + memcpy(&val, ptr, len); > + pci_default_write_config(PCI_DEVICE(o->pci_dev), > + offset, val, len); > + trace_vfu_cfg_write(offset, val); > + } else { > + val = pci_default_read_config(PCI_DEVICE(o->pci_dev), > + offset, len); > + memcpy(ptr, &val, len); pci_default_read_config() returns a host-endian 32-bit value. This code looks wrong because it copies different bytes on big- and little-endian hosts. > + trace_vfu_cfg_read(offset, val); > + } Why call pci_default_read/write_config() directly instead of pci_dev->config_read/write()?
> On Sep 9, 2021, at 3:27 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote: > > On Fri, Aug 27, 2021 at 01:53:25PM -0400, Jagannathan Raman wrote: >> +static ssize_t vfu_object_cfg_access(vfu_ctx_t *vfu_ctx, char * const buf, >> + size_t count, loff_t offset, >> + const bool is_write) >> +{ >> + VfuObject *o = vfu_get_private(vfu_ctx); >> + uint32_t pci_access_width = sizeof(uint32_t); >> + size_t bytes = count; >> + uint32_t val = 0; >> + char *ptr = buf; >> + int len; >> + >> + while (bytes > 0) { >> + len = (bytes > pci_access_width) ? pci_access_width : bytes; >> + if (is_write) { >> + memcpy(&val, ptr, len); >> + pci_default_write_config(PCI_DEVICE(o->pci_dev), >> + offset, val, len); >> + trace_vfu_cfg_write(offset, val); >> + } else { >> + val = pci_default_read_config(PCI_DEVICE(o->pci_dev), >> + offset, len); >> + memcpy(ptr, &val, len); > > pci_default_read_config() returns a host-endian 32-bit value. This code > looks wrong because it copies different bytes on big- and little-endian > hosts. I’ll collect more details on this one, trying to wrap my head around it. Concerning config space writes, it doesn’t look like we need to perform any conversion as the store operation automatically happens in the CPU’s native format when we do something like the following: PCIDevice->config[addr] = val; Concerning config read, I observed that pci_default_read_config() performs le32_to_cpu() conversion. May be we should not rely on it doing the conversion. > >> + trace_vfu_cfg_read(offset, val); >> + } > > Why call pci_default_read/write_config() directly instead of > pci_dev->config_read/write()? That makes sense - we should be calling pci_dev->config_read/write(). After performing pci_dev->config_read(), I’ll convert it to the CPU’s endianness format using le32_to_cpu(). On big-endian systems, it would re-order the bytes, and on little-endian systems it would be a no-op. -- Jag
On Fri, Sep 10, 2021 at 04:22:56PM +0000, Jag Raman wrote: > > > > On Sep 9, 2021, at 3:27 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > > On Fri, Aug 27, 2021 at 01:53:25PM -0400, Jagannathan Raman wrote: > >> +static ssize_t vfu_object_cfg_access(vfu_ctx_t *vfu_ctx, char * const buf, > >> + size_t count, loff_t offset, > >> + const bool is_write) > >> +{ > >> + VfuObject *o = vfu_get_private(vfu_ctx); > >> + uint32_t pci_access_width = sizeof(uint32_t); > >> + size_t bytes = count; > >> + uint32_t val = 0; > >> + char *ptr = buf; > >> + int len; > >> + > >> + while (bytes > 0) { > >> + len = (bytes > pci_access_width) ? pci_access_width : bytes; > >> + if (is_write) { > >> + memcpy(&val, ptr, len); > >> + pci_default_write_config(PCI_DEVICE(o->pci_dev), > >> + offset, val, len); > >> + trace_vfu_cfg_write(offset, val); > >> + } else { > >> + val = pci_default_read_config(PCI_DEVICE(o->pci_dev), > >> + offset, len); > >> + memcpy(ptr, &val, len); > > > > pci_default_read_config() returns a host-endian 32-bit value. This code > > looks wrong because it copies different bytes on big- and little-endian > > hosts. > > I’ll collect more details on this one, trying to wrap my head around it. > > Concerning config space writes, it doesn’t look like we need to > perform any conversion as the store operation automatically happens > in the CPU’s native format when we do something like the following: > PCIDevice->config[addr] = val; PCIDevice->config is uint8_t*. Endianness isn't an issue with single byte accesses. > > Concerning config read, I observed that pci_default_read_config() > performs le32_to_cpu() conversion. May be we should not rely on > it doing the conversion. Yes, ->config_read() returns a host-endian 32-bit value and ->config_write() receives a host-endian 32-bit value (it has a bit-shifting loop that implicitly handles endianness conversion). Stefan
diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c index 0726eb9..13011ce 100644 --- a/hw/remote/vfio-user-obj.c +++ b/hw/remote/vfio-user-obj.c @@ -36,6 +36,7 @@ #include "sysemu/runstate.h" #include "qemu/notify.h" #include "qemu/thread.h" +#include "qemu/main-loop.h" #include "qapi/error.h" #include "sysemu/sysemu.h" #include "libvfio-user.h" @@ -144,6 +145,38 @@ retry_attach: return NULL; } +static ssize_t vfu_object_cfg_access(vfu_ctx_t *vfu_ctx, char * const buf, + size_t count, loff_t offset, + const bool is_write) +{ + VfuObject *o = vfu_get_private(vfu_ctx); + uint32_t pci_access_width = sizeof(uint32_t); + size_t bytes = count; + uint32_t val = 0; + char *ptr = buf; + int len; + + while (bytes > 0) { + len = (bytes > pci_access_width) ? pci_access_width : bytes; + if (is_write) { + memcpy(&val, ptr, len); + pci_default_write_config(PCI_DEVICE(o->pci_dev), + offset, val, len); + trace_vfu_cfg_write(offset, val); + } else { + val = pci_default_read_config(PCI_DEVICE(o->pci_dev), + offset, len); + memcpy(ptr, &val, len); + trace_vfu_cfg_read(offset, val); + } + offset += len; + ptr += len; + bytes -= len; + } + + return count; +} + static void vfu_object_machine_done(Notifier *notifier, void *data) { VfuObject *o = container_of(notifier, VfuObject, machine_done); @@ -182,6 +215,17 @@ static void vfu_object_machine_done(Notifier *notifier, void *data) return; } + ret = vfu_setup_region(o->vfu_ctx, VFU_PCI_DEV_CFG_REGION_IDX, + pci_config_size(o->pci_dev), &vfu_object_cfg_access, + VFU_REGION_FLAG_RW | VFU_REGION_FLAG_ALWAYS_CB, + NULL, 0, -1, 0); + if (ret < 0) { + error_setg(&error_abort, + "vfu: Failed to setup config space handlers for %s- %s", + o->devid, strerror(errno)); + return; + } + ret = vfu_realize_ctx(o->vfu_ctx); if (ret < 0) { error_setg(&error_abort, "vfu: Failed to realize device %s- %s", diff --git a/hw/remote/trace-events b/hw/remote/trace-events index 7da12f0..2ef7884 100644 --- a/hw/remote/trace-events +++ b/hw/remote/trace-events @@ -5,3 +5,5 @@ mpqemu_recv_io_error(int cmd, int size, int nfds) "failed to receive %d size %d, # vfio-user-obj.c vfu_prop(const char *prop, const char *val) "vfu: setting %s as %s" +vfu_cfg_read(uint32_t offset, uint32_t val) "vfu: cfg: 0x%u -> 0x%x" +vfu_cfg_write(uint32_t offset, uint32_t val) "vfu: cfg: 0x%u <- 0x%x"