Re: [virtio-comment] virtio-blk vs. caching on FVP Base

[Laszlo Ersek <lersek@redhat.com>]

On 01/23/14 18:24, Paolo Bonzini wrote:
> Il 23/01/2014 18:09, Laszlo Ersek ha scritto:
>>
>> They both worked in the sense that each got me over the immediate FVP
>> Base model warning, and *something* was loaded from the virtio-blk disk.
>> However that "something" (which should have been a grub2 binary) could
>> not be executed, implying that further corruption was happening.
>>
>> The straightforward idea is that it's not enough to make the ring area
>> itself uncacheable: the scatter-gather memory areas *referenced* by the
>> descriptors located inside the ring have to be uncacheable too.
> 
> I think making any of these uncacheable is wrong, for two reasons.
> 
> First, the solution to this would be to force a dcache writeback after
> writing, before kicking the vring; and flushing the dcache and icache
> before reading.  This is different from memory barriers, and is true for
> all DMA-capable devices.
> 
> How to do this is probably somewhere in the ARM docs but UEFI ought to
> provide generic primitives already.

Turns out such primitives already exist, in
"MdePkg/Include/Library/CacheMaintenanceLib.h".

> Uncacheable memory should only be
> used if the cost of the dcache flushes exceeds the cost of sacrificing
> caching for all accesses.  As you guessed this is not the case.
> 
> Second, because virtio should not require any dcache flushing.  Icache,
> yes (just like any other case of self-modifying code in a guest; in this
> case the "modification" is done by reading from disk),

Ah OK. This wasn't clear to me immediately. You're saying this because
the stuff we load (ie. the grub2 image) we're also going to execute.

> but ISTR that the
> device reads and writes should be considered cache-coherent.

So this is the thing that doesn't seem to happen in the FVP Base
platform. The virtio host implementation probably writes to main memory
only, but when reading the buffers and the ring, those may be served
from the data cache.

> So an icache flush might be missing in TianoCore, but not in the virtio
> drivers (rather the loader should do it just after loading executable
> code into memory).

Good point (we're again talking about the grub2 case I think) -- I just
checked, and these flushes are already there, thankfully, in the
LoadImage() and LoadImageEx() implementations
[MdeModulePkg/Core/Dxe/Image/Image.c].

CoreLoadImage()
  CoreLoadImageCommon()
    CoreLoadPeImage()
      PeCoffLoaderRelocateImage()
      InvalidateInstructionCacheRange()

So in this case the image could not have been executed likely because
the transfer was garbled and the PE+ sanity check code caught it.

> And any requirement on the dcache is likely an
> emulator bug.

It crossed my mind but I wasn't sure of it :)

Thanks!
Laszlo