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

[Jag Raman <jag.raman@oracle.com>]

> 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