On 2021/3/15 11:13, Jason Wang wrote:> > On 2021/3/15 9:14 ??, Jie Deng wrote: >> >> 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. > > > This is not correct. There're already a brunch hardware virtio devices. > > Thanks >Then do you think it is necessary to mark the virtio bufs with ____cacheline_aligned ? I haven't seen any virtio interface being marked yet. If this is a problem, I believe it should? be common for all virtio devices, right ? Thanks.> >> 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 >> >> >
Arnd Bergmann
2021-Mar-15 07:52 UTC
[PATCH v7] i2c: virtio: add a virtio i2c frontend driver
On Mon, Mar 15, 2021 at 6:54 AM Jie Deng <jie.deng at intel.com> wrote:> On 2021/3/15 11:13, Jason Wang wrote: > > On 2021/3/15 9:14 ??, Jie Deng wrote: > >> On 2021/3/12 16:58, Arnd Bergmann wrote: > > > Then do you think it is necessary to mark the virtio bufs with > ____cacheline_aligned ?I think so, yes.> I haven't seen any virtio interface being marked yet. If this is a > problem, I believe it should be common for all virtio devices, right ?Yes, but it's not a problem if the buffers are allocated separately because kmalloc provinces a cachelinen aligned buffer on architectures that need it. It's only a problem here because there is a single allocation for three objects that have different ownership states during the DMA (device owned to-device, cpu-owned, device owned to-cpu). Arnd