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

[Laszlo Ersek <lersek@redhat.com>]

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í <markmb@redhat.com> wrote:
>>>> Signed-off-by: Marc Marí <markmb@redhat.com>
>>>> ---
>>>>  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>.
>>
>> 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.

Thanks
Laszlo