[Qemu-devel] fw_cfg specification ?

[Qemu-devel] fw_cfg specification ?

[Gabriel L. Somlo]

Hi,

I'm looking for the closest thing to an official spec for qemu's
fw_cfg device, and so far I have found this:

http://lists.gnu.org/archive/html/qemu-devel/2011-04/msg00238.html

but it apparently never got committed to qemu (any idea why not?).

Googling around didn't get me much further than that.

Is there anything better or more up to date floating around out
there somewhere ?

Thanks much,
--Gabriel

Re: [Qemu-devel] fw_cfg specification ?

[Laszlo Ersek]

On 03/11/15 16:27, Gabriel L. Somlo wrote:
> Hi,
> 
> I'm looking for the closest thing to an official spec for qemu's
> fw_cfg device, and so far I have found this:
> 
> http://lists.gnu.org/archive/html/qemu-devel/2011-04/msg00238.html
> 
> but it apparently never got committed to qemu (any idea why not?).

Must have fallen through the cracks. (Just speculating; in April 2011 I
had been with RH for less than half a year, and learning Xen. :))

> Googling around didn't get me much further than that.
> 
> Is there anything better or more up to date floating around out
> there somewhere ?

I won't say "better", but it is "committed": check
"Documentation/devicetree/bindings/arm/fw-cfg.txt" in the kernel tree.
It is intentionally vague on the set of keys and fw_cfg files that qemu
provides, because that's a moving target. You can only rely on the qemu
source for those.

Also, I updated Gleb's email address in the CC: header.

If you have a ton of time, you could try documenting fw_cfg yourself,
but as I said, it's a moving target, so the description would either
become stale quickly, or require people to keep it in sync with the
source all the time. Updating documentation sucks *hard*.

(At one point I had also started a text file like Jordan's, but I didn't
even get as far as posting it anywhere; I just gave up.)

Thanks
Laszlo

Re: [Qemu-devel] fw_cfg specification ?

[Gabriel L. Somlo]

On Wed, Mar 11, 2015 at 07:50:40PM +0100, Laszlo Ersek wrote:
> On 03/11/15 16:27, Gabriel L. Somlo wrote:
> > Hi,
> > 
> > I'm looking for the closest thing to an official spec for qemu's
> > fw_cfg device, and so far I have found this:
> > 
> > http://lists.gnu.org/archive/html/qemu-devel/2011-04/msg00238.html
> > 
> > but it apparently never got committed to qemu (any idea why not?).
> 
> Must have fallen through the cracks. (Just speculating; in April 2011 I
> had been with RH for less than half a year, and learning Xen. :))
> 
> > Googling around didn't get me much further than that.
> > 
> > Is there anything better or more up to date floating around out
> > there somewhere ?
> 
> I won't say "better", but it is "committed": check
> "Documentation/devicetree/bindings/arm/fw-cfg.txt" in the kernel tree.
> It is intentionally vague on the set of keys and fw_cfg files that qemu
> provides, because that's a moving target. You can only rely on the qemu
> source for those.

The fw_cfg interface from the guest's perspective is pretty
straightforward: select something on the control port, read a blob
from the data port.

I was more interested in the "hardware" implementation details,
and the reasoning behind them.

For instance, I get there's a total of 0x30 entries
(FWCfgEntry entries), the last 0x10 of which have 
"file names" and are referenced from the "FWCfgFiles *files"
structure.

I don't quite get the part where

  struct FWCfgState {
    ...
    FWCfgEntry entries[2][FW_CFG_MAX_ENTRY];
    ...
  }

is accessed via 

  int arch = !!(key & FW_CFG_ARCH_LOCAL);
  ...
  s->entries[arch][key].foo = ...

I.e., what's the significance of arch==0 vs. arch==1 ?

Also, what's the significance of handling guest-initiated writes to
the data port ? Could the guest write larger chunks of data than the
current size of the selected entry ? Would the selected entry's size
grow accordingly ? Would it shrink if the guest (over)wrote less than
the current size ?

I know it's all going to start making sense eventually, if I stare at
the source long enough :) I'm just trying to speed up that process as
much as possible.

Right now, at a quick glance, qemu writes a bunch of stuff into the
fw_cfg data structure before starting the guest, and the guest (well,
mostly the guest's BIOS) does a bunch of things based on the
"breadcrumbs" it reads from the fw_cfg device.

But that doesn't seem to be the whole story...

> If you have a ton of time, you could try documenting fw_cfg yourself,
> but as I said, it's a moving target, so the description would either
> become stale quickly, or require people to keep it in sync with the
> source all the time. Updating documentation sucks *hard*.

Once I start getting what's going on (e.g., w.r.t. the questions above)
I wouldn't mind just adding *comments* to the source, for the next guy
who, like me, is trying to get the lay of the land, but I'm not there
yet...

Thanks much,
--Gabriel

Re: [Qemu-devel] fw_cfg specification ?

[Laszlo Ersek]

On 03/11/15 21:45, Gabriel L. Somlo wrote:
> On Wed, Mar 11, 2015 at 07:50:40PM +0100, Laszlo Ersek wrote:
>> On 03/11/15 16:27, Gabriel L. Somlo wrote:
>>> Hi,
>>>
>>> I'm looking for the closest thing to an official spec for qemu's
>>> fw_cfg device, and so far I have found this:
>>>
>>> http://lists.gnu.org/archive/html/qemu-devel/2011-04/msg00238.html
>>>
>>> but it apparently never got committed to qemu (any idea why not?).
>>
>> Must have fallen through the cracks. (Just speculating; in April 2011 I
>> had been with RH for less than half a year, and learning Xen. :))
>>
>>> Googling around didn't get me much further than that.
>>>
>>> Is there anything better or more up to date floating around out
>>> there somewhere ?
>>
>> I won't say "better", but it is "committed": check
>> "Documentation/devicetree/bindings/arm/fw-cfg.txt" in the kernel tree.
>> It is intentionally vague on the set of keys and fw_cfg files that qemu
>> provides, because that's a moving target. You can only rely on the qemu
>> source for those.
> 
> The fw_cfg interface from the guest's perspective is pretty
> straightforward: select something on the control port, read a blob
> from the data port.
> 
> I was more interested in the "hardware" implementation details,
> and the reasoning behind them.
> 
> For instance, I get there's a total of 0x30 entries
> (FWCfgEntry entries), the last 0x10 of which have 
> "file names" and are referenced from the "FWCfgFiles *files"
> structure.
> 
> I don't quite get the part where
> 
>   struct FWCfgState {
>     ...
>     FWCfgEntry entries[2][FW_CFG_MAX_ENTRY];
>     ...
>   }
> 
> is accessed via 
> 
>   int arch = !!(key & FW_CFG_ARCH_LOCAL);
>   ...
>   s->entries[arch][key].foo = ...
> 
> I.e., what's the significance of arch==0 vs. arch==1 ?

Well, from that aborted :) text file of mine:

> The client writes a 16-bit wide selector to the control port. The most
> significant bit (FW_CFG_ARCH_LOCAL) selects the namespace: 0
> corresponds to "generic" namespace, while 1 corresponds to
> "architecture specific" namespace.

Jordan had also dedicated a paragraph to Bit15 in his patch that you
linked above.

> Also, what's the significance of handling guest-initiated writes to
> the data port ? Could the guest write larger chunks of data than the
> current size of the selected entry ?

No.

Again, from that aborted text file:

> The client code writes a selector (a key) to the control port, and
> then can read the corresponding data (produced by qemu-kvm) via the
> data port, or, if the selected entry is writable, rewrite it. If
> qemu-kvm has associated a callback with the entry: when the entry is
> completely rewritten, qemu-kvm runs the callback. This way the client
> code can exchange data with and even invoke functions in qemu-kvm.

> Would the selected entry's size
> grow accordingly ? Would it shrink if the guest (over)wrote less than
> the current size ?

No. Please see the fw_cfg_write() function. Only the same size can be
written, and the write callback runs when that's completed.

In any case, I'm unaware of *any* instance when an fw_cfg blob is
rewritten by the guest.

In fact, such *write* callbacks are registered in qemu with
fw_cfg_add_callback(), and I can't see any calls to that function. It's
dead code, apparently.

(fw_cfg_add_file_callback() is different; it is much more recent, and it
is for read callbacks.)

> I know it's all going to start making sense eventually, if I stare at
> the source long enough :) I'm just trying to speed up that process as
> much as possible.
> 
> Right now, at a quick glance, qemu writes a bunch of stuff into the
> fw_cfg data structure before starting the guest, and the guest (well,
> mostly the guest's BIOS) does a bunch of things based on the
> "breadcrumbs" it reads from the fw_cfg device.
> 
> But that doesn't seem to be the whole story...

It is pretty close; it's almost the whole story, as far as I'm aware.
What more should an interface called "firmware config" do than, well,
configure the firmware? :)

(Even -kernel / -initrd / -append abuse the original intent of fw_cfg,
because those blobs are huge, and not configuration-like at all. But
they got popular and now we're stuck with them; higher level tools
depend on them heavily.)

There is though a bit more to the story: the (recent) read callbacks.
Qemu sometimes decides to re-generate a "bunch of stuff" in the fw_cfg
blobs. See the commit message here:

https://github.com/tianocore/edk2/commit/66b280df

(The edk2 patch visible in that commit has been completely reimplemented
since, but the qemu description in the commit *message* remains valid.)

>> If you have a ton of time, you could try documenting fw_cfg yourself,
>> but as I said, it's a moving target, so the description would either
>> become stale quickly, or require people to keep it in sync with the
>> source all the time. Updating documentation sucks *hard*.
> 
> Once I start getting what's going on (e.g., w.r.t. the questions above)
> I wouldn't mind just adding *comments* to the source, for the next guy
> who, like me, is trying to get the lay of the land, but I'm not there
> yet...

I believe that would be useful, yes.

Thanks
Laszlo

Re: [Qemu-devel] fw_cfg specification ?

[Gerd Hoffmann]

  Hi,

> >> I won't say "better", but it is "committed": check
> >> "Documentation/devicetree/bindings/arm/fw-cfg.txt" in the kernel tree.
> >> It is intentionally vague on the set of keys and fw_cfg files that qemu
> >> provides, because that's a moving target. You can only rely on the qemu
> >> source for those.
> > 
> > The fw_cfg interface from the guest's perspective is pretty
> > straightforward: select something on the control port, read a blob
> > from the data port.

I think just commiting Jordan's patch as-is would be a good start.
Then we can go from there adding missing bits.

It's also easier to refuse patches without doc updates if there actually
is documentation in the first place ;)

> > For instance, I get there's a total of 0x30 entries
> > (FWCfgEntry entries), the last 0x10 of which have 
> > "file names" and are referenced from the "FWCfgFiles *files"
> > structure.

That's basically referencing stuff by name rather than magic numbers.
It's used for all new stuff.  There is a simliar interface named 'cbfs'
between coreboot and seabios.

> No. Please see the fw_cfg_write() function. Only the same size can be
> written, and the write callback runs when that's completed.
> 
> In any case, I'm unaware of *any* instance when an fw_cfg blob is
> rewritten by the guest.

> In fact, such *write* callbacks are registered in qemu with
> fw_cfg_add_callback(), and I can't see any calls to that function. It's
> dead code, apparently.

Guess we should just drop the code then.

> There is though a bit more to the story: the (recent) read callbacks.
> Qemu sometimes decides to re-generate a "bunch of stuff" in the fw_cfg
> blobs. See the commit message here:

That is used for acpi.  acpi initialization goes like this:

  (1) firmware initializes the hardware as it pleases.
  (2) firmware fetches acpi tables from qemu, and the read callback
      is used to update the addresses in the acpi tables according
      to the initialization done by the firmware (acpi pci device,
      mmconfig xbar, ...)

Main reason to do it this way is we don't need paravirtual interfaces to
pass around those addresses.

> > Once I start getting what's going on (e.g., w.r.t. the questions above)
> > I wouldn't mind just adding *comments* to the source, for the next guy
> > who, like me, is trying to get the lay of the land, but I'm not there
> > yet...

I'd suggest to update the doc file created by the old patch instead.

cheers,
  Gerd

Re: [Qemu-devel] fw_cfg specification ?

[Paolo Bonzini]

On 11/03/2015 22:30, Laszlo Ersek wrote:
> In any case, I'm unaware of *any* instance when an fw_cfg blob is
> rewritten by the guest.
> 
> In fact, such *write* callbacks are registered in qemu with
> fw_cfg_add_callback(), and I can't see any calls to that function. It's
> dead code, apparently.

Let's nuke it. :)

Paolo

Re: [Qemu-devel] fw_cfg specification ?

[Laszlo Ersek]

On 03/12/15 15:17, Paolo Bonzini wrote:
> 
> 
> On 11/03/2015 22:30, Laszlo Ersek wrote:
>> In any case, I'm unaware of *any* instance when an fw_cfg blob is
>> rewritten by the guest.
>>
>> In fact, such *write* callbacks are registered in qemu with
>> fw_cfg_add_callback(), and I can't see any calls to that function. It's
>> dead code, apparently.
> 
> Let's nuke it. :)

Okay... Gabriel, do you want to include such a patch in your upcoming
series, or should I do the nuking sooner?

Thanks
Laszlo

Re: [Qemu-devel] fw_cfg specification ?

[Gabriel L. Somlo]

On Thu, Mar 12, 2015 at 04:42:25PM +0100, Laszlo Ersek wrote:
> On 03/12/15 15:17, Paolo Bonzini wrote:
> > Let's nuke it. :)
> 
> Okay... Gabriel, do you want to include such a patch in your upcoming
> series, or should I do the nuking sooner?

I was working on it (good for practice :) and so far I have the patch
below.

Still trying to understand how to update .valid.accepts in the various
mem_ops structures, but I think I'm on the right track. Will send a
proper patch once I wrap my head around that :)

Thanks,
--Gabriel


diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 78a37be..0a9eaf6 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -46,7 +46,6 @@ typedef struct FWCfgEntry {
     uint32_t len;
     uint8_t *data;
     void *callback_opaque;
-    FWCfgCallback callback;
     FWCfgReadCallback read_callback;
 } FWCfgEntry;
 
@@ -230,23 +229,6 @@ static void fw_cfg_reboot(FWCfgState *s)
     fw_cfg_add_file(s, "etc/boot-fail-wait", g_memdup(&reboot_timeout, 4), 4);
 }
 
-static void fw_cfg_write(FWCfgState *s, uint8_t value)
-{
-    int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
-    FWCfgEntry *e = &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
-
-    trace_fw_cfg_write(s, value);
-
-    if (s->cur_entry & FW_CFG_WRITE_CHANNEL && e->callback &&
-        s->cur_offset < e->len) {
-        e->data[s->cur_offset++] = value;
-        if (s->cur_offset == e->len) {
-            e->callback(e->callback_opaque, e->data);
-            s->cur_offset = 0;
-        }
-    }
-}
-
 static int fw_cfg_select(FWCfgState *s, uint16_t key)
 {
     int ret;
@@ -299,17 +281,13 @@ static uint64_t fw_cfg_data_mem_read(void *opaque, hwaddr addr,
 static void fw_cfg_data_mem_write(void *opaque, hwaddr addr,
                                   uint64_t value, unsigned size)
 {
-    FWCfgState *s = opaque;
-    unsigned i = size;
-
-    do {
-        fw_cfg_write(s, value >> (8 * --i));
-    } while (i);
+//FIXME: unused, remove from fw_cfg_data_mem_ops
 }
 
 static bool fw_cfg_data_mem_valid(void *opaque, hwaddr addr,
                                   unsigned size, bool is_write)
 {
+//FIXME: additional checks before we nuke fw_cfg_data_mem_write() ?
     return addr == 0;
 }
 
@@ -336,7 +314,7 @@ static void fw_cfg_comb_write(void *opaque, hwaddr addr,
 {
     switch (size) {
     case 1:
-        fw_cfg_write(opaque, (uint8_t)value);
+        //FIXME: unused, fix fw_cfg_comb_mem_ops then remove case
         break;
     case 2:
         fw_cfg_select(opaque, (uint16_t)value);
@@ -347,6 +325,7 @@ static void fw_cfg_comb_write(void *opaque, hwaddr addr,
 static bool fw_cfg_comb_valid(void *opaque, hwaddr addr,
                                   unsigned size, bool is_write)
 {
+//FIXME: update checks before removing size==1 fw_cfg_comb_write() case
     return (size == 1) || (is_write && size == 2);
 }
 
@@ -358,6 +337,7 @@ static const MemoryRegionOps fw_cfg_ctl_mem_ops = {
 
 static const MemoryRegionOps fw_cfg_data_mem_ops = {
     .read = fw_cfg_data_mem_read,
+//FIXME: nuke this after updating fw_cfg_data_mem_valid()
     .write = fw_cfg_data_mem_write,
     .endianness = DEVICE_BIG_ENDIAN,
     .valid = {
@@ -458,7 +438,6 @@ static void *fw_cfg_modify_bytes_read(FWCfgState *s, uint16_t key,
     s->entries[arch][key].data = data;
     s->entries[arch][key].len = len;
     s->entries[arch][key].callback_opaque = NULL;
-    s->entries[arch][key].callback = NULL;
 
     return ptr;
 }
@@ -502,23 +481,6 @@ void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value)
     fw_cfg_add_bytes(s, key, copy, sizeof(value));
 }
 
-void fw_cfg_add_callback(FWCfgState *s, uint16_t key, FWCfgCallback callback,
-                         void *callback_opaque, void *data, size_t len)
-{
-    int arch = !!(key & FW_CFG_ARCH_LOCAL);
-
-    assert(key & FW_CFG_WRITE_CHANNEL);
-
-    key &= FW_CFG_ENTRY_MASK;
-
-    assert(key < FW_CFG_MAX_ENTRY && len <= UINT32_MAX);
-
-    s->entries[arch][key].data = data;
-    s->entries[arch][key].len = (uint32_t)len;
-    s->entries[arch][key].callback_opaque = callback_opaque;
-    s->entries[arch][key].callback = callback;
-}
-
 void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
                               FWCfgReadCallback callback, void *callback_opaque,
                               void *data, size_t len)
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index 6d8a8ac..b2e10c2 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -69,8 +69,6 @@ void fw_cfg_add_string(FWCfgState *s, uint16_t key, const char *value);
 void fw_cfg_add_i16(FWCfgState *s, uint16_t key, uint16_t value);
 void fw_cfg_add_i32(FWCfgState *s, uint16_t key, uint32_t value);
 void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value);
-void fw_cfg_add_callback(FWCfgState *s, uint16_t key, FWCfgCallback callback,
-                         void *callback_opaque, void *data, size_t len);
 void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data,
                      size_t len);
 void fw_cfg_add_file_callback(FWCfgState *s, const char *filename,

Re: [Qemu-devel] fw_cfg specification ?

[Paolo Bonzini]

On 12/03/2015 17:16, Gabriel L. Somlo wrote:

>  static bool fw_cfg_data_mem_valid(void *opaque, hwaddr addr,
>                                    unsigned size, bool is_write)
>  {
> +//FIXME: additional checks before we nuke fw_cfg_data_mem_write() ?

yes, "&& !is_write".  Or just leave an empty fw_cfg_data_mem_write.

>      return addr == 0;
>  }
>  
> @@ -336,7 +314,7 @@ static void fw_cfg_comb_write(void *opaque, hwaddr addr,
>  {
>      switch (size) {
>      case 1:
> -        fw_cfg_write(opaque, (uint8_t)value);
> +        //FIXME: unused, fix fw_cfg_comb_mem_ops then remove case
>          break;
>      case 2:
>          fw_cfg_select(opaque, (uint16_t)value);
> @@ -347,6 +325,7 @@ static void fw_cfg_comb_write(void *opaque, hwaddr addr,
>  static bool fw_cfg_comb_valid(void *opaque, hwaddr addr,
>                                    unsigned size, bool is_write)
>  {
> +//FIXME: update checks before removing size==1 fw_cfg_comb_write() case
>      return (size == 1) || (is_write && size == 2);

Same here: "size == 1 && !is_write", or just leave an empty "case" after
removing the call to fw_cfg_write.

>  }
>  
> @@ -358,6 +337,7 @@ static const MemoryRegionOps fw_cfg_ctl_mem_ops = {
>  
>  static const MemoryRegionOps fw_cfg_data_mem_ops = {
>      .read = fw_cfg_data_mem_read,
> +//FIXME: nuke this after updating fw_cfg_data_mem_valid()

If you want, but it's not necessary.

Paolo

>      .write = fw_cfg_data_mem_write,
>      .endianness = DEVICE_BIG_ENDIAN,
>      .valid = {
> @@ -458,7 +438,6 @@ static void *fw_cfg_modify_bytes_read(FWCfgState *s, uint16_t key,
>      s->entries[arch][key].data = data;
>      s->entries[arch][key].len = len;
>      s->entries[arch][key].callback_opaque = NULL;
> -    s->entries[arch][key].callback = NULL;
>  
>      return ptr;
>  }
> @@ -502,23 +481,6 @@ void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value)
>      fw_cfg_add_bytes(s, key, copy, sizeof(value));
>  }
>  
> -void fw_cfg_add_callback(FWCfgState *s, uint16_t key, FWCfgCallback callback,
> -                         void *callback_opaque, void *data, size_t len)
> -{
> -    int arch = !!(key & FW_CFG_ARCH_LOCAL);
> -
> -    assert(key & FW_CFG_WRITE_CHANNEL);
> -
> -    key &= FW_CFG_ENTRY_MASK;
> -
> -    assert(key < FW_CFG_MAX_ENTRY && len <= UINT32_MAX);
> -
> -    s->entries[arch][key].data = data;
> -    s->entries[arch][key].len = (uint32_t)len;
> -    s->entries[arch][key].callback_opaque = callback_opaque;
> -    s->entries[arch][key].callback = callback;
> -}
> -
>  void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
>                                FWCfgReadCallback callback, void *callback_opaque,
>                                void *data, size_t len)
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index 6d8a8ac..b2e10c2 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -69,8 +69,6 @@ void fw_cfg_add_string(FWCfgState *s, uint16_t key, const char *value);
>  void fw_cfg_add_i16(FWCfgState *s, uint16_t key, uint16_t value);
>  void fw_cfg_add_i32(FWCfgState *s, uint16_t key, uint32_t value);
>  void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value);
> -void fw_cfg_add_callback(FWCfgState *s, uint16_t key, FWCfgCallback callback,
> -                         void *callback_opaque, void *data, size_t len);
>  void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data,
>                       size_t len);
>  void fw_cfg_add_file_callback(FWCfgState *s, const char *filename,
> 
>