[Qemu-devel] [PATCH v3 1/3] loader: rename in_ram/has_mr

[Qemu-devel] [PATCH v3 1/3] loader: rename in_ram/has_mr

[Michael S. Tsirkin]

we put copy of ROMs in MR for migration.
but the name rom_in_ram makes one think we
load it in guest RAM.
Rename has_mr to make intent clearer.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/loader.h | 2 +-
 hw/core/loader.c    | 6 +++---
 hw/i386/pc_piix.c   | 2 +-
 hw/i386/pc_q35.c    | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/hw/loader.h b/include/hw/loader.h
index aaf08c3..3dc5b94 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -49,7 +49,7 @@ void pstrcpy_targphys(const char *name,
                       hwaddr dest, int buf_size,
                       const char *source);
 
-extern bool rom_file_in_ram;
+extern bool rom_file_has_mr;
 
 int rom_add_file(const char *file, const char *fw_dir,
                  hwaddr addr, int32_t bootindex);
diff --git a/hw/core/loader.c b/hw/core/loader.c
index b323c0c..13e98d8 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -54,7 +54,7 @@
 
 #include <zlib.h>
 
-bool rom_file_in_ram = true;
+bool rom_file_has_mr = true;
 
 static int roms_loaded;
 
@@ -694,7 +694,7 @@ int rom_add_file(const char *file, const char *fw_dir,
                  basename);
         snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name);
 
-        if (rom_file_in_ram) {
+        if (rom_file_has_mr) {
             data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
         } else {
             data = rom->data;
@@ -738,7 +738,7 @@ void *rom_add_blob(const char *name, const void *blob, size_t len,
 
         snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name);
 
-        if (rom_file_in_ram) {
+        if (rom_file_has_mr) {
             data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
         } else {
             data = rom->data;
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index ae1699d..fb2d636 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -272,7 +272,7 @@ static void pc_compat_1_6(QEMUMachineInitArgs *args)
 {
     pc_compat_1_7(args);
     has_pci_info = false;
-    rom_file_in_ram = false;
+    rom_file_has_mr = false;
     has_acpi_build = false;
 }
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index a7f6260..eb55ae4 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -250,7 +250,7 @@ static void pc_compat_1_6(QEMUMachineInitArgs *args)
 {
     pc_compat_1_7(args);
     has_pci_info = false;
-    rom_file_in_ram = false;
+    rom_file_has_mr = false;
     has_acpi_build = false;
 }
 
-- 
MST

[Qemu-devel] [PATCH v3 2/3] pc: avoid duplicate names for ROM MRs

[Michael S. Tsirkin]

Since
commit 04920fc0faa4760f9c4fc0e73b992b768099be70
    loader: store FW CFG ROM files in RAM
RAM MRs including ROM files in FW CFGs are created
and named using the file basename.

This becomes problematic if these names are
supplied by user, since the basename might not
be unique.

There are two cases we care about:
- option-rom flag.
- option ROM for devices. This triggers e.g. when
  using rombar=0.

At the moment we get an assert. E.g
qemu -option-rom /usr/share/ipxe/8086100e.rom -option-rom
/usr/share/ipxe.efi/8086100e.rom
RAMBlock "/rom@genroms/8086100e.rom" already registered, abort!

This is a regression from 1.7.

For now let's keep it simple and just avoid creating the
MRs in case of option ROMs.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/loader.h |  6 ++++--
 hw/core/loader.c    | 10 ++++++----
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/include/hw/loader.h b/include/hw/loader.h
index 3dc5b94..796cbf9 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -49,10 +49,12 @@ void pstrcpy_targphys(const char *name,
                       hwaddr dest, int buf_size,
                       const char *source);
 
+extern bool option_rom_has_mr;
 extern bool rom_file_has_mr;
 
 int rom_add_file(const char *file, const char *fw_dir,
-                 hwaddr addr, int32_t bootindex);
+                 hwaddr addr, int32_t bootindex,
+                 bool option_rom);
 void *rom_add_blob(const char *name, const void *blob, size_t len,
                    hwaddr addr, const char *fw_file_name,
                    FWCfgReadCallback fw_callback, void *callback_opaque);
@@ -66,7 +68,7 @@ void *rom_ptr(hwaddr addr);
 void do_info_roms(Monitor *mon, const QDict *qdict);
 
 #define rom_add_file_fixed(_f, _a, _i)          \
-    rom_add_file(_f, NULL, _a, _i)
+    rom_add_file(_f, NULL, _a, _i, false)
 #define rom_add_blob_fixed(_f, _b, _l, _a)      \
     rom_add_blob(_f, _b, _l, _a, NULL, NULL, NULL)
 
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 13e98d8..2bf6b8f 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -54,6 +54,7 @@
 
 #include <zlib.h>
 
+bool option_rom_has_mr = false;
 bool rom_file_has_mr = true;
 
 static int roms_loaded;
@@ -642,7 +643,8 @@ static void *rom_set_mr(Rom *rom, Object *owner, const char *name)
 }
 
 int rom_add_file(const char *file, const char *fw_dir,
-                 hwaddr addr, int32_t bootindex)
+                 hwaddr addr, int32_t bootindex,
+                 bool option_rom)
 {
     Rom *rom;
     int rc, fd = -1;
@@ -694,7 +696,7 @@ int rom_add_file(const char *file, const char *fw_dir,
                  basename);
         snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name);
 
-        if (rom_file_has_mr) {
+        if ((!option_rom || option_rom_has_mr) && rom_file_has_mr) {
             data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
         } else {
             data = rom->data;
@@ -773,12 +775,12 @@ int rom_add_elf_program(const char *name, void *data, size_t datasize,
 
 int rom_add_vga(const char *file)
 {
-    return rom_add_file(file, "vgaroms", 0, -1);
+    return rom_add_file(file, "vgaroms", 0, -1, true);
 }
 
 int rom_add_option(const char *file, int32_t bootindex)
 {
-    return rom_add_file(file, "genroms", 0, bootindex);
+    return rom_add_file(file, "genroms", 0, bootindex, true);
 }
 
 static void rom_reset(void *unused)
-- 
MST

[Qemu-devel] [PATCH v3 3/3] pc: option rom migration compatibility with 1.7

[Michael S. Tsirkin]

when using 1.7 machine types, enable
option ROMs in RAM to match that version.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/i386/pc_piix.c | 1 +
 hw/i386/pc_q35.c  | 1 +
 2 files changed, 2 insertions(+)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index fb2d636..5e1d2d3 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -266,6 +266,7 @@ static void pc_compat_1_7(QEMUMachineInitArgs *args)
 {
     smbios_type1_defaults = false;
     gigabyte_align = false;
+    option_rom_has_mr = true;
 }
 
 static void pc_compat_1_6(QEMUMachineInitArgs *args)
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index eb55ae4..4b0456a 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -244,6 +244,7 @@ static void pc_compat_1_7(QEMUMachineInitArgs *args)
 {
     smbios_type1_defaults = false;
     gigabyte_align = false;
+    option_rom_has_mr = true;
 }
 
 static void pc_compat_1_6(QEMUMachineInitArgs *args)
-- 
MST

Re: [Qemu-devel] [PATCH v3 1/3] loader: rename in_ram/has_mr

[Jeff E. Nelson]

On 03/09/2014 02:06 PM, Michael S. Tsirkin wrote:
> we put copy of ROMs in MR for migration.
> but the name rom_in_ram makes one think we
> load it in guest RAM.
> Rename has_mr to make intent clearer.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>   include/hw/loader.h | 2 +-
>   hw/core/loader.c    | 6 +++---
>   hw/i386/pc_piix.c   | 2 +-
>   hw/i386/pc_q35.c    | 2 +-
>   4 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index aaf08c3..3dc5b94 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -49,7 +49,7 @@ void pstrcpy_targphys(const char *name,
>                         hwaddr dest, int buf_size,
>                         const char *source);
>
> -extern bool rom_file_in_ram;
> +extern bool rom_file_has_mr;

Thank you! It is much easier to understand what is going on.

I don't think I am qualified to offer an official acknowledgment, but 
this looks good to me.

-Jeff

>
>   int rom_add_file(const char *file, const char *fw_dir,
>                    hwaddr addr, int32_t bootindex);
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index b323c0c..13e98d8 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -54,7 +54,7 @@
>
>   #include <zlib.h>
>
> -bool rom_file_in_ram = true;
> +bool rom_file_has_mr = true;
>
>   static int roms_loaded;
>
> @@ -694,7 +694,7 @@ int rom_add_file(const char *file, const char *fw_dir,
>                    basename);
>           snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name);
>
> -        if (rom_file_in_ram) {
> +        if (rom_file_has_mr) {
>               data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
>           } else {
>               data = rom->data;
> @@ -738,7 +738,7 @@ void *rom_add_blob(const char *name, const void *blob, size_t len,
>
>           snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name);
>
> -        if (rom_file_in_ram) {
> +        if (rom_file_has_mr) {
>               data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
>           } else {
>               data = rom->data;
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index ae1699d..fb2d636 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -272,7 +272,7 @@ static void pc_compat_1_6(QEMUMachineInitArgs *args)
>   {
>       pc_compat_1_7(args);
>       has_pci_info = false;
> -    rom_file_in_ram = false;
> +    rom_file_has_mr = false;
>       has_acpi_build = false;
>   }
>
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index a7f6260..eb55ae4 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -250,7 +250,7 @@ static void pc_compat_1_6(QEMUMachineInitArgs *args)
>   {
>       pc_compat_1_7(args);
>       has_pci_info = false;
> -    rom_file_in_ram = false;
> +    rom_file_has_mr = false;
>       has_acpi_build = false;
>   }
>
>


-- 
- Jeff E. Nelson  /  Red Hat  /  Virtualization  /  jen@redhat.com

Re: [Qemu-devel] [PATCH v3 1/3] loader: rename in_ram/has_mr

[Paolo Bonzini]

Il 09/03/2014 19:06, Michael S. Tsirkin ha scritto:
> we put copy of ROMs in MR for migration.
> but the name rom_in_ram makes one think we
> load it in guest RAM.
> Rename has_mr to make intent clearer.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/hw/loader.h | 2 +-
>  hw/core/loader.c    | 6 +++---
>  hw/i386/pc_piix.c   | 2 +-
>  hw/i386/pc_q35.c    | 2 +-
>  4 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index aaf08c3..3dc5b94 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -49,7 +49,7 @@ void pstrcpy_targphys(const char *name,
>                        hwaddr dest, int buf_size,
>                        const char *source);
>
> -extern bool rom_file_in_ram;
> +extern bool rom_file_has_mr;
>
>  int rom_add_file(const char *file, const char *fw_dir,
>                   hwaddr addr, int32_t bootindex);
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index b323c0c..13e98d8 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -54,7 +54,7 @@
>
>  #include <zlib.h>
>
> -bool rom_file_in_ram = true;
> +bool rom_file_has_mr = true;
>
>  static int roms_loaded;
>
> @@ -694,7 +694,7 @@ int rom_add_file(const char *file, const char *fw_dir,
>                   basename);
>          snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name);
>
> -        if (rom_file_in_ram) {
> +        if (rom_file_has_mr) {
>              data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
>          } else {
>              data = rom->data;
> @@ -738,7 +738,7 @@ void *rom_add_blob(const char *name, const void *blob, size_t len,
>
>          snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name);
>
> -        if (rom_file_in_ram) {
> +        if (rom_file_has_mr) {
>              data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
>          } else {
>              data = rom->data;
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index ae1699d..fb2d636 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -272,7 +272,7 @@ static void pc_compat_1_6(QEMUMachineInitArgs *args)
>  {
>      pc_compat_1_7(args);
>      has_pci_info = false;
> -    rom_file_in_ram = false;
> +    rom_file_has_mr = false;
>      has_acpi_build = false;
>  }
>
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index a7f6260..eb55ae4 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -250,7 +250,7 @@ static void pc_compat_1_6(QEMUMachineInitArgs *args)
>  {
>      pc_compat_1_7(args);
>      has_pci_info = false;
> -    rom_file_in_ram = false;
> +    rom_file_has_mr = false;
>      has_acpi_build = false;
>  }
>
>

Series looks good, but please squash patch 3 into patch 2.

Paolo

Re: [Qemu-devel] [PATCH v3 1/3] loader: rename in_ram/has_mr

[Michael S. Tsirkin]

On Mon, Mar 10, 2014 at 09:58:54AM +0100, Paolo Bonzini wrote:
> Il 09/03/2014 19:06, Michael S. Tsirkin ha scritto:
> >we put copy of ROMs in MR for migration.
> >but the name rom_in_ram makes one think we
> >load it in guest RAM.
> >Rename has_mr to make intent clearer.
> >
> >Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >---
> > include/hw/loader.h | 2 +-
> > hw/core/loader.c    | 6 +++---
> > hw/i386/pc_piix.c   | 2 +-
> > hw/i386/pc_q35.c    | 2 +-
> > 4 files changed, 6 insertions(+), 6 deletions(-)
> >
> >diff --git a/include/hw/loader.h b/include/hw/loader.h
> >index aaf08c3..3dc5b94 100644
> >--- a/include/hw/loader.h
> >+++ b/include/hw/loader.h
> >@@ -49,7 +49,7 @@ void pstrcpy_targphys(const char *name,
> >                       hwaddr dest, int buf_size,
> >                       const char *source);
> >
> >-extern bool rom_file_in_ram;
> >+extern bool rom_file_has_mr;
> >
> > int rom_add_file(const char *file, const char *fw_dir,
> >                  hwaddr addr, int32_t bootindex);
> >diff --git a/hw/core/loader.c b/hw/core/loader.c
> >index b323c0c..13e98d8 100644
> >--- a/hw/core/loader.c
> >+++ b/hw/core/loader.c
> >@@ -54,7 +54,7 @@
> >
> > #include <zlib.h>
> >
> >-bool rom_file_in_ram = true;
> >+bool rom_file_has_mr = true;
> >
> > static int roms_loaded;
> >
> >@@ -694,7 +694,7 @@ int rom_add_file(const char *file, const char *fw_dir,
> >                  basename);
> >         snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name);
> >
> >-        if (rom_file_in_ram) {
> >+        if (rom_file_has_mr) {
> >             data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
> >         } else {
> >             data = rom->data;
> >@@ -738,7 +738,7 @@ void *rom_add_blob(const char *name, const void *blob, size_t len,
> >
> >         snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name);
> >
> >-        if (rom_file_in_ram) {
> >+        if (rom_file_has_mr) {
> >             data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
> >         } else {
> >             data = rom->data;
> >diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> >index ae1699d..fb2d636 100644
> >--- a/hw/i386/pc_piix.c
> >+++ b/hw/i386/pc_piix.c
> >@@ -272,7 +272,7 @@ static void pc_compat_1_6(QEMUMachineInitArgs *args)
> > {
> >     pc_compat_1_7(args);
> >     has_pci_info = false;
> >-    rom_file_in_ram = false;
> >+    rom_file_has_mr = false;
> >     has_acpi_build = false;
> > }
> >
> >diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> >index a7f6260..eb55ae4 100644
> >--- a/hw/i386/pc_q35.c
> >+++ b/hw/i386/pc_q35.c
> >@@ -250,7 +250,7 @@ static void pc_compat_1_6(QEMUMachineInitArgs *args)
> > {
> >     pc_compat_1_7(args);
> >     has_pci_info = false;
> >-    rom_file_in_ram = false;
> >+    rom_file_has_mr = false;
> >     has_acpi_build = false;
> > }
> >
> >
> 
> Series looks good, but please squash patch 3 into patch 2.
> 
> Paolo

done. Already merged so won't repost here.

Re: [Qemu-devel] [PATCH v3 1/3] loader: rename in_ram/has_mr

[Marcel Apfelbaum]

On Sun, 2014-03-09 at 20:06 +0200, Michael S. Tsirkin wrote:
> we put copy of ROMs in MR for migration.
> but the name rom_in_ram makes one think we
> load it in guest RAM.
> Rename has_mr to make intent clearer.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/hw/loader.h | 2 +-
>  hw/core/loader.c    | 6 +++---
>  hw/i386/pc_piix.c   | 2 +-
>  hw/i386/pc_q35.c    | 2 +-
>  4 files changed, 6 insertions(+), 6 deletions(-)

Hi, Series
Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com>

Thanks,
Marcel

> 
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index aaf08c3..3dc5b94 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -49,7 +49,7 @@ void pstrcpy_targphys(const char *name,
>                        hwaddr dest, int buf_size,
>                        const char *source);
>  
> -extern bool rom_file_in_ram;
> +extern bool rom_file_has_mr;
>  
>  int rom_add_file(const char *file, const char *fw_dir,
>                   hwaddr addr, int32_t bootindex);
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index b323c0c..13e98d8 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -54,7 +54,7 @@
>  
>  #include <zlib.h>
>  
> -bool rom_file_in_ram = true;
> +bool rom_file_has_mr = true;
>  
>  static int roms_loaded;
>  
> @@ -694,7 +694,7 @@ int rom_add_file(const char *file, const char *fw_dir,
>                   basename);
>          snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name);
>  
> -        if (rom_file_in_ram) {
> +        if (rom_file_has_mr) {
>              data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
>          } else {
>              data = rom->data;
> @@ -738,7 +738,7 @@ void *rom_add_blob(const char *name, const void *blob, size_t len,
>  
>          snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name);
>  
> -        if (rom_file_in_ram) {
> +        if (rom_file_has_mr) {
>              data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
>          } else {
>              data = rom->data;
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index ae1699d..fb2d636 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -272,7 +272,7 @@ static void pc_compat_1_6(QEMUMachineInitArgs *args)
>  {
>      pc_compat_1_7(args);
>      has_pci_info = false;
> -    rom_file_in_ram = false;
> +    rom_file_has_mr = false;
>      has_acpi_build = false;
>  }
>  
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index a7f6260..eb55ae4 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -250,7 +250,7 @@ static void pc_compat_1_6(QEMUMachineInitArgs *args)
>  {
>      pc_compat_1_7(args);
>      has_pci_info = false;
> -    rom_file_in_ram = false;
> +    rom_file_has_mr = false;
>      has_acpi_build = false;
>  }
>