Re: [Qemu-devel] [PATCH v3 4/5] Enable fw_cfg DMA interface for ARM

[Laszlo Ersek <lersek@redhat.com>]

On 10/27/15 12:11, Gerd Hoffmann wrote:
>   Hi,
> 
>>> My hypothesis (which I guess I'm volunteering to verify, unless we
>>> end up rejecting this immediately as a bad idea, for some reason that
>>> I have missed), is that current functionality wouldn't change, given
>>> the way existing callbacks work right now, and that we could run the
>>> callback each time a blob is *selected*, rather than hooking into the
>>> (dma/mmio/pio) read methods.
>>
>> Callback executed on first read only sounds okay to me, callback
>> executed on selection... hm... don't like it. :)
> 
> Care to explain why?
> 
> I think callback on selection would be better.  Interface is more clear
> then, I don't like read having different behavior depending on hidden
> state (current offset).

> And in practice selection and read will always
> be called together,

This is what I think you cannot guarantee on the host side, without
auditing all guest code. The behavior of callbacks has been specified
under fw_cfg_add_file_callback(), in docs/specs/fw_cfg.txt, and guest
code is allowed to work off that.

> so there shouldn't be a difference in practice ...

I guess I have no choice but to audit all QemuFwCfgSelectItem calls in
edk2...

Right, here's what I've had in the back of my mind: see the
DetectSmbiosVersion() function in
"OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c". It selects
the key that belongs to the "etc/smbios/smbios-anchor" fw_cfg file, but
the switch statement right after it can jump to the "default" label, and
under that label *nothing* is read from fw_cfg.

This is valid guest code according to the current specs. Its behavior
would change (however obscurely) if there was a callback on the
"etc/smbios/smbios-anchor" file, and the callback was executed on
selection, not read.

... This one instance wouldn't be particularly hard to patch in edk2,
but in general our specs are useless if we don't stick to them.

Thanks
Laszlo