[Qemu-devel] [PATCH] [RfC] fw_cfg file sort

[Qemu-devel] [PATCH] [RfC] fw_cfg file sort

[Gerd Hoffmann]

This is what it takes to have a sorted fw_cfg file directory.
Entries are inserted at the correct place instead of being
appended to the end in case sorting is enabled.

Compatibility fluff (enable sorting for new machine types only)
isn't there yet.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/nvram/fw_cfg.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 0f35931..83205e0 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -639,7 +639,7 @@ void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
                               FWCfgReadCallback callback, void *callback_opaque,
                               void *data, size_t len)
 {
-    int i, index;
+    int i, index, count;
     size_t dsize;
 
     if (!s->files) {
@@ -648,13 +648,31 @@ void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
         fw_cfg_add_bytes(s, FW_CFG_FILE_DIR, s->files, dsize);
     }
 
-    index = be32_to_cpu(s->files->count);
-    assert(index < FW_CFG_FILE_SLOTS);
+    count = be32_to_cpu(s->files->count);
+    assert(count < FW_CFG_FILE_SLOTS);
+
+    index = count;
+    if (1 /* sort entries */) {
+        while (index > 0 && strcmp(filename, s->files->f[index-1].name) < 0) {
+            s->files->f[index] =
+                s->files->f[index - 1];
+            s->files->f[index].select =
+                cpu_to_be16(FW_CFG_FILE_FIRST + index);
+            s->entries[0][FW_CFG_FILE_FIRST + index] =
+                s->entries[0][FW_CFG_FILE_FIRST + index - 1];
+            index--;
+        }
+        memset(&s->files->f[index],
+               0, sizeof(FWCfgFile));
+        memset(&s->entries[0][FW_CFG_FILE_FIRST + index],
+               0, sizeof(FWCfgEntry));
+    }
 
     pstrcpy(s->files->f[index].name, sizeof(s->files->f[index].name),
             filename);
-    for (i = 0; i < index; i++) {
-        if (strcmp(s->files->f[index].name, s->files->f[i].name) == 0) {
+    for (i = 0; i <= count; i++) {
+        if (i != index &&
+            strcmp(s->files->f[index].name, s->files->f[i].name) == 0) {
             error_report("duplicate fw_cfg file name: %s",
                          s->files->f[index].name);
             exit(1);
@@ -668,7 +686,7 @@ void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
     s->files->f[index].select = cpu_to_be16(FW_CFG_FILE_FIRST + index);
     trace_fw_cfg_add_file(s, index, s->files->f[index].name, len);
 
-    s->files->count = cpu_to_be32(index+1);
+    s->files->count = cpu_to_be32(count+1);
 }
 
 void fw_cfg_add_file(FWCfgState *s,  const char *filename,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] [RfC] fw_cfg file sort

[Gabriel L. Somlo]

On Fri, Jun 19, 2015 at 12:13:36PM +0200, Gerd Hoffmann wrote:
> This is what it takes to have a sorted fw_cfg file directory.
> Entries are inserted at the correct place instead of being
> appended to the end in case sorting is enabled.

On Mon, Jun 08, 2015 at 05:53:48PM +0200, Michael S. Tsirkin wrote:
> Unfortunately we have a bunch of code adding fw cfg entries
> after machine ready.

I'm not really clear about the relationship between "after machine
ready" and "after guest vcpu begins executing instructions", but as
long as we never insert new fw_cfg files after the *latter*, this
should work fine.

Otherwise, there's a chance that guest code might grab a selector
value which would become stale if the fw_cfg structures are re-sorted
due to a new blob being inserted.

Thanks,
--Gabriel

> Compatibility fluff (enable sorting for new machine types only)
> isn't there yet.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/nvram/fw_cfg.c | 30 ++++++++++++++++++++++++------
>  1 file changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 0f35931..83205e0 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -639,7 +639,7 @@ void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
>                                FWCfgReadCallback callback, void *callback_opaque,
>                                void *data, size_t len)
>  {
> -    int i, index;
> +    int i, index, count;
>      size_t dsize;
>  
>      if (!s->files) {
> @@ -648,13 +648,31 @@ void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
>          fw_cfg_add_bytes(s, FW_CFG_FILE_DIR, s->files, dsize);
>      }
>  
> -    index = be32_to_cpu(s->files->count);
> -    assert(index < FW_CFG_FILE_SLOTS);
> +    count = be32_to_cpu(s->files->count);
> +    assert(count < FW_CFG_FILE_SLOTS);
> +
> +    index = count;
> +    if (1 /* sort entries */) {
> +        while (index > 0 && strcmp(filename, s->files->f[index-1].name) < 0) {
> +            s->files->f[index] =
> +                s->files->f[index - 1];
> +            s->files->f[index].select =
> +                cpu_to_be16(FW_CFG_FILE_FIRST + index);
> +            s->entries[0][FW_CFG_FILE_FIRST + index] =
> +                s->entries[0][FW_CFG_FILE_FIRST + index - 1];
> +            index--;
> +        }
> +        memset(&s->files->f[index],
> +               0, sizeof(FWCfgFile));
> +        memset(&s->entries[0][FW_CFG_FILE_FIRST + index],
> +               0, sizeof(FWCfgEntry));
> +    }
>  
>      pstrcpy(s->files->f[index].name, sizeof(s->files->f[index].name),
>              filename);
> -    for (i = 0; i < index; i++) {
> -        if (strcmp(s->files->f[index].name, s->files->f[i].name) == 0) {
> +    for (i = 0; i <= count; i++) {
> +        if (i != index &&
> +            strcmp(s->files->f[index].name, s->files->f[i].name) == 0) {
>              error_report("duplicate fw_cfg file name: %s",
>                           s->files->f[index].name);
>              exit(1);
> @@ -668,7 +686,7 @@ void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
>      s->files->f[index].select = cpu_to_be16(FW_CFG_FILE_FIRST + index);
>      trace_fw_cfg_add_file(s, index, s->files->f[index].name, len);
>  
> -    s->files->count = cpu_to_be32(index+1);
> +    s->files->count = cpu_to_be32(count+1);
>  }
>  
>  void fw_cfg_add_file(FWCfgState *s,  const char *filename,
> -- 
> 1.8.3.1
>