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

[Gabriel L. Somlo]

On Mon, Oct 26, 2015 at 02:38:11PM +0100, Laszlo Ersek wrote:
> On 10/26/15 13:49, Gabriel L. Somlo wrote:
> > On Mon, Oct 26, 2015 at 10:48:08AM +0000, Stefan Hajnoczi wrote:
> >> On Thu, Oct 22, 2015 at 05:22:16PM -0400, Gabriel L. Somlo wrote:
> >>> I was re-reading the documentation for fw_cfg_add_file_callback(),
> >>> and noticed that non-dma read operations check for the presence
> >>> of a callback (and call it if present) for *every* *single* *byte*,
> >>> even on 64-bit MMIO reads. That's also what the documentation says
> >>> (in docs/specs/fw_cfg.txt, being moved into fw_cfg.h as per
> >>>  http://lists.nongnu.org/archive/html/qemu-devel/2015-10/msg05315.html).
> >>>
> >>> During DMA reads, however, the callback is only checked once before
> >>> each chunk, effectively once per DMA read operation.
> >>>
> >>> Now, typical callbacks I found throughout the qemu source tend to return
> >>> immediately except for the first time they're invoked, but I wonder if
> >>> skipping over all those extra "do I have a callback, if so call it,
> >>> mostly so it can return without doing anything" per-byte operations
> >>> account in some significant part for the dramatically faster transfers?
> >>>
> >>> Not sure how I'd test for that -- besides my not having anything
> >>> resembling a viable ARM setup, I'm not sure if limiting the callbacks
> >>> to only be invoked if (s->cur_offset == 0) would make sense, just as a
> >>> test ?
> >>
> >> I think Marc came to the conclusion that it's safe and therefore made
> >> that optimization for DMA.
> >>
> >> The same can be done for PIO.
> > 
> > OK, so at the risk of over-reaching here, would it make sense to
> > rewrite the fw_cfg spec to say "If present, a callback will be
> > executed *once* before each time a blob is read" ?
> > 
> > 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. :)

I figured there's different code paths for the different read methods,
so instead of checking for (and calling) the callback in each of them,
(and additionally looking at whether the current read offset is 0 if
we're to only call it on first read only), I could maybe factor it out
a bit further. Since the only reason you'd want select something is to
then read from it, that sort-of made sense to me, at the time... :)

I don't have strong feelings about it, though... :)

Thanks,
--Gabriel