Re: [Qemu-devel] [RFC 6/7] Add offset register to fw_cfg DMA interface

[Marc Marí]

On Tue, 21 Jul 2015 22:36:39 +0200
Laszlo Ersek <address@hidden> wrote:

> On 07/21/15 22:16, Kevin O'Connor wrote:
> > On Tue, Jul 21, 2015 at 10:06:51PM +0200, Laszlo Ersek wrote:
> >> On 07/21/15 18:26, Stefan Hajnoczi wrote:
> >>> On Tue, Jul 21, 2015 at 5:03 PM, Marc Marí <address@hidden>
> >>> wrote:
> >>>> Signed-off-by: Marc Marí <address@hidden>
> >>>> ---
> >>>>  hw/nvram/fw_cfg.c | 19 ++++++++++++++++---
> >>>>  1 file changed, 16 insertions(+), 3 deletions(-)
> >>>
> >>> No commit description, no docs/specs/fw_cfg.txt documentation.
> >>
> >> Yes, those would be nice.
> >>
> >> Also, I think this patch should be squashed into the main fw_cfg
> >> patch.
> >>
> >>> I understand how the offset is supposed to work, but why is it
> >>> necessary?  No one needed it before so there must be a reason why
> >>> you decided to add it now.
> >>
> >> I guess because of
> >> <http://thread.gmane.org/gmane.comp.bios.coreboot.seabios/9496/focus=9554>.

Yes, it's for that. Sorry for the short description, I don't know why I
could think it was enough.

> >>
> >> For me chunked transfers would be important (ie. transfering I+J=K
> >> bytes from the same fw_cfg file should be possible as two separate
> >> accesses, with I & J sizes), but I believe the offset register
> >> would not be necessary just for that. So I think it's solely
> >> directed at Kevin's feedback (see link above).
> > 
> > The reason the "offset" field is useful is because several of the
> > fw_cfg files have headers, and it's necessary to read the header
> > into one area of memory and the payload into another area of
> > memory.  So, basically we want to be able to read a chunk of a
> > fw_cfg entry to one memory address and then read another chunk to
> > another memory address.
> > 
> > Not sure what you mean by "I+J=K" but I suspect we have similar
> > requirements - the ability to read different parts of a fw_cfg entry
> > in different calls.
> 
> Yes.
> 
> > Without this ability we'd need to read the entire
> > entry into a big linear area of memory and then memmove that around.
> > If there is a way to accomplish this without an "offset" field then
> > that's fine too.
> 
> What I meant by "I+J=K" is that the qemu-internal offset into the
> selected fw_cfg blob is not (and should not) be reset between calls.
> So, if you want to implement a "scatter read" in the firmware, just
> break up the read into appropriately sized, smaller transfers, and
> pass different target addresses. The source offset should be carried
> forward from one call to the other.
> 
> If you look at patch #2 in this RFC series, the only qemu-internal
> fw-cfg API call that it exercises is fw_cfg_read(). That function
> advances / maintains the "cur_offset" field of FWCfgState. Nothing in
> the patch re-sets "cur_offset" -- that happens only when the firmware
> writes to the selector register, and fw_cfg_select() gets called.
> 
> "docs/specs/fw_cfg.txt" also states,
> 
> > Initially following a write to the selector register, the data
> > offset will be set to zero. Each successful access to the data
> > register will increment the data offset by the appropriate access
> > width.
> 
> This is a very nice property of fw_cfg (allowing for chunked and
> scatter reads, if you like), and I think patch #2 preserves that
> property. If we'd like a readv()-like pattern in the firmware, we can
> decompose that into read()-like calls; a pread()-like interface is
> not necessary.
> 
> So, I think patch #6 is not actually needed.

It's true that I missed that point in the guest side (SeaBIOS support
for these patches). This means, the actual guest code can be improved a
lot.

But I still think this is useful. With the DMA interface, every read
operation copies the data into the memory region provided. If you want
to discard a big chunk of data, you need a big chunk of memory to place
it, unless you want to corrupt the memory. In the IO interface, you
could just read and not save until you are where you want to read.

This doesn't mean to leave the offset. Probably there's another
solution. I can think of using address NULL to discard the data.
Probably there are other good or better options.

Of course, you can always read the size of the region you have
allocated as many times as necessary to reach the desired offset, but
it's better to do it in just one operation.

Thanks
Marc