Virtualization - Mar 2021 - [PATCH v7] i2c: virtio: add a virtio i2c frontend driver

On Fri, Mar 12, 2021 at 2:33 PM Jie Deng <jie.deng at intel.com> wrote:
> +
> +/**
> + * struct virtio_i2c_req - the virtio I2C request structure
> + * @out_hdr: the OUT header of the virtio I2C message
> + * @buf: the buffer into which data is read, or from which it's
written
> + * @in_hdr: the IN header of the virtio I2C message
> + */
> +struct virtio_i2c_req {
> +       struct virtio_i2c_out_hdr out_hdr;
> +       uint8_t *buf;
> +       struct virtio_i2c_in_hdr in_hdr;
> +};
The simpler request structure clearly looks better than the previous version,
but I think I found another problem here, at least a theoretical one:

When you map the headers into the DMA address space, they should
be in separate cache lines, to allow the DMA mapping interfaces to
perform cache management on each one without accidentally clobbering
another member.

So far I think there is an assumption that virtio buffers are always
on cache-coherent devices, but if you ever have a virtio-i2c device
backend on a physical interconnect that is not cache coherent (e.g. a
microcontroller that shares the memory bus), this breaks down.

You could avoid this by either allocating arrays of each type separately,
or by marking each member that you pass to the device as
____cacheline_aligned.

      Arnd

On 2021/3/12 16:58, Arnd Bergmann wrote:
> On Fri, Mar 12, 2021 at 2:33 PM Jie Deng <jie.deng at intel.com>
wrote:
>
>> +
>> +/**
>> + * struct virtio_i2c_req - the virtio I2C request structure
>> + * @out_hdr: the OUT header of the virtio I2C message
>> + * @buf: the buffer into which data is read, or from which it's
written
>> + * @in_hdr: the IN header of the virtio I2C message
>> + */
>> +struct virtio_i2c_req {
>> +       struct virtio_i2c_out_hdr out_hdr;
>> +       uint8_t *buf;
>> +       struct virtio_i2c_in_hdr in_hdr;
>> +};
> The simpler request structure clearly looks better than the previous
version,
> but I think I found another problem here, at least a theoretical one:
>
> When you map the headers into the DMA address space, they should
> be in separate cache lines, to allow the DMA mapping interfaces to
> perform cache management on each one without accidentally clobbering
> another member.
>
> So far I think there is an assumption that virtio buffers are always
> on cache-coherent devices, but if you ever have a virtio-i2c device
> backend on a physical interconnect that is not cache coherent (e.g. a
> microcontroller that shares the memory bus), this breaks down.
>
> You could avoid this by either allocating arrays of each type separately,
> or by marking each member that you pass to the device as
> ____cacheline_aligned.
>
>        Arnd
The virtio devices are software emulated. The backend software may need to
consider this since it may exchange data with physical devices. But I 
don't think
we need it for this interface, because no DMA operation is involved 
between the
frontend driver and backend driver.

Regards,

Jie