Re: [PATCH RFC server v2 06/11] vfio-user: handle PCI config space accesses

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2215 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]