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

[Laszlo Ersek]

On 10/28/15 02:12, Gabriel L. Somlo wrote:
> On Tue, Oct 27, 2015 at 01:43:39PM +0100, Laszlo Ersek wrote:
>> 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.
> 
> OK, but none of "etc/smbios/*" blobs actually have a callback at all.
> 
> After some grepping, the only places inserting callback-equipped blobs
> are:
> 
>       - hw/i386/acpi-build.c (via rom_add_blob or directly by calling
>                               fw_cfg_add_file_callback)
> 
>               - files added are
>                       "/etc/acpi/rsdp",
>                       "/etc/acpi/tables", and
>                       "/etc/table-loader".
> 
>               - all using the same callback: acpi_build_update()
> 
>       - hw/arm/virt-acpi-build.c (via rom_add_blob only)
> 
>               - same three files as on i386
> 
>               - all using the same callback: virt_acpi_build_update()
> 
> Both of these callbacks are a one-shot deal, i.e. they both
> contain something along these lines:
> 
>     /* No state to update or already patched? Nothing to do. */
>     if (!build_state || build_state->patched) {
>         return;
>     }
>     build_state->patched = 1;
> 
> So, they do something *once* before the first byte is ever read, and
> never again after that.
> 
>> ... 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.
> 
> OK, so I was proposing to amend the specs (now, while externally
> visible behavior won't be affected), and *THEN* stick to them :)
> 
> We're already giving up on the letter of the specs (right now, they
> say once per byte read, but DMA is only doing once per chunk transfered,
> which in practice amounts to once each time a whole blob is read).

Good point.

> Of course, if you (or anyone else with much more clue than me) expect
> a future scenario where we'd need the opportunity to run the callback
> more than once *before* reading anything from the blob, or (as is the
> case with smbios) wish to select (but not read from) blobs, and the
> blobs will be callback-enabled, but running the callback will be a bad
> thing when no read follows, then by all means, let's stick with
> hooking into each individual read operation.
> 
> As it is right now, the ammended spec I'm proposing (if set, callback
> runs on select, whether a read follows or not) is a NOP w.r.t. currently
> visible behavior. It allows simplifying things, at the price of removing
> theoretical future flexibility (but also unnecessary slowness as well).

Okay. Please go ahead with the change, as far as I'm concerned.

Thanks
Laszlo

> Thanks for helping me think this through !
> 
> --Gabriel
>