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

[Gabriel L. Somlo]

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).

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).

Thanks for helping me think this through !

--Gabriel