diff mbox

[RFC,6/7] Add offset register to fw_cfg DMA interface

Message ID 1437494626-3773-7-git-send-email-markmb@redhat.com
State New
Headers show

Commit Message

Marc Marí July 21, 2015, 4:03 p.m. UTC 
Signed-off-by: Marc Marí <markmb@redhat.com>
---
 hw/nvram/fw_cfg.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

Comments

Laszlo Ersek July 21, 2015, 8:06 p.m. UTC | #1 
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).

Thanks
Laszlo
Kevin O'Connor July 21, 2015, 8:16 p.m. UTC | #2 
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.  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.

Cheers,
-Kevin
Laszlo Ersek July 21, 2015, 8:36 p.m. UTC | #3 
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
Kevin O'Connor July 22, 2015, 4:11 a.m. UTC | #4 
On Tue, Jul 21, 2015 at 10:36:39PM +0200, Laszlo Ersek wrote:
> On 07/21/15 22:16, Kevin O'Connor wrote:
> > 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.

I missed that in my earlier review of the code.  With that in place, I
agree that the offset isn't needed.

-Kevin
Marc Marí July 22, 2015, 9:03 a.m. UTC | #5 
On Tue, 21 Jul 2015 22:36:39 +0200
Laszlo Ersek <lersek@redhat.com> 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í <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>.

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
diff mbox

Patch

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 83205e0..9a39d45 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -47,8 +47,9 @@ 
 #define FW_CFG_DMA_ADDR_LO        0
 #define FW_CFG_DMA_ADDR_HI        4
 #define FW_CFG_DMA_LENGTH         8
-#define FW_CFG_DMA_CONTROL       12
-#define FW_CFG_DMA_SIZE          16
+#define FW_CFG_DMA_OFFSET        12
+#define FW_CFG_DMA_CONTROL       16
+#define FW_CFG_DMA_SIZE          20
 
 /* FW_CFG_DMA_CONTROL bits */
 #define FW_CFG_DMA_CTL_ERROR   0x01
@@ -78,6 +79,7 @@  struct FWCfgState {
     dma_addr_t dma_addr;
     uint32_t   dma_len;
     uint32_t   dma_ctl;
+    uint32_t   dma_off;
 };
 
 struct FWCfgIoState {
@@ -338,6 +340,10 @@  static void fw_cfg_dma_transfer(FWCfgState *s)
             return;
         }
 
+        for (i = 0; i < s->dma_off; ++i) {
+            fw_cfg_read(s);
+        }
+
         for (i = 0; i < len; i++) {
             ptr[i] = fw_cfg_read(s);
         }
@@ -366,6 +372,9 @@  static uint64_t fw_cfg_dma_mem_read(void *opaque, hwaddr addr,
     case FW_CFG_DMA_LENGTH:
         ret = s->dma_len;
         break;
+    case FW_CFG_DMA_OFFSET:
+        ret = s->dma_off;
+        break;
     case FW_CFG_DMA_CONTROL:
         ret = s->dma_ctl;
         break;
@@ -390,6 +399,9 @@  static void fw_cfg_dma_mem_write(void *opaque, hwaddr addr,
     case FW_CFG_DMA_LENGTH:
         s->dma_len = value;
         break;
+    case FW_CFG_DMA_OFFSET:
+        s->dma_off = value;
+        break;
     case FW_CFG_DMA_CONTROL:
         value &= FW_CFG_DMA_CTL_MASK;
         s->dma_ctl = value;
@@ -528,6 +540,7 @@  static VMStateDescription vmstate_fw_cfg_dma = {
     .fields = (VMStateField[]) {
         VMSTATE_UINT64(dma_addr, FWCfgState),
         VMSTATE_UINT32(dma_len, FWCfgState),
+        VMSTATE_UINT32(dma_off, FWCfgState),
         VMSTATE_UINT32(dma_ctl, FWCfgState),
         VMSTATE_END_OF_LIST()
     },
@@ -791,7 +804,7 @@  FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
     if (dma_addr && dma_as) {
         FW_CFG(dev)->dma_as = dma_as;
         FW_CFG(dev)->dma_enabled = true;
-        sysbus_mmio_map(sbd, 1, dma_addr);
+        sysbus_mmio_map(sbd, 2, dma_addr);
     }
 
     return FW_CFG(dev);