qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 1/2] pc: avoid duplicate names for ROM MRs
@ 2014-03-06 15:48 Michael S. Tsirkin
  2014-03-06 15:48 ` [Qemu-devel] [PATCH 2/2] pc: option rom migration compatibility with 1.7 Michael S. Tsirkin
  2014-03-06 16:14 ` [Qemu-devel] [PATCH 1/2] pc: avoid duplicate names for ROM MRs Marcel Apfelbaum
  0 siblings, 2 replies; 4+ messages in thread
From: Michael S. Tsirkin @ 2014-03-06 15:48 UTC (permalink / raw)
  To: qemu-devel; +Cc: imammedo, pbonzini, kraxel, Marcel Apfelbaum

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 this case.

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 91b0122..edca1ed 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -44,9 +44,11 @@ void pstrcpy_targphys(const char *name,
                       const char *source);
 
 extern bool rom_file_in_ram;
+extern bool option_rom_in_ram;
 
 int rom_add_file(const char *file, const char *fw_dir,
-                 hwaddr addr, int32_t bootindex);
+                 hwaddr addr, int32_t bootindex,
+                 bool option);
 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);
@@ -60,7 +62,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, true)
 #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 0634bee..a19b158 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -54,6 +54,7 @@
 
 #include <zlib.h>
 
+bool option_rom_in_ram = false;
 bool rom_file_in_ram = true;
 
 static int roms_loaded;
@@ -624,7 +625,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;
     int rc, fd = -1;
@@ -676,7 +678,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 ((!option || option_rom_in_ram) && rom_file_in_ram) {
             data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
         } else {
             data = rom->data;
@@ -755,12 +757,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

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [Qemu-devel] [PATCH 2/2] pc: option rom migration compatibility with 1.7
  2014-03-06 15:48 [Qemu-devel] [PATCH 1/2] pc: avoid duplicate names for ROM MRs Michael S. Tsirkin
@ 2014-03-06 15:48 ` Michael S. Tsirkin
  2014-03-06 16:14 ` [Qemu-devel] [PATCH 1/2] pc: avoid duplicate names for ROM MRs Marcel Apfelbaum
  1 sibling, 0 replies; 4+ messages in thread
From: Michael S. Tsirkin @ 2014-03-06 15:48 UTC (permalink / raw)
  To: qemu-devel; +Cc: imammedo, pbonzini, kraxel, Anthony Liguori, Marcel Apfelbaum

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 1acd2b2..99c7a3a 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -260,6 +260,7 @@ static void pc_compat_1_7(QEMUMachineInitArgs *args)
 {
     smbios_type1_defaults = false;
     gigabyte_align = false;
+    option_rom_in_ram = true;
 }
 
 static void pc_compat_1_6(QEMUMachineInitArgs *args)
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index a7f6260..36a1c0d 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_in_ram = true;
 }
 
 static void pc_compat_1_6(QEMUMachineInitArgs *args)
-- 
MST

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] pc: avoid duplicate names for ROM MRs
  2014-03-06 15:48 [Qemu-devel] [PATCH 1/2] pc: avoid duplicate names for ROM MRs Michael S. Tsirkin
  2014-03-06 15:48 ` [Qemu-devel] [PATCH 2/2] pc: option rom migration compatibility with 1.7 Michael S. Tsirkin
@ 2014-03-06 16:14 ` Marcel Apfelbaum
  2014-03-06 16:20   ` Michael S. Tsirkin
  1 sibling, 1 reply; 4+ messages in thread
From: Marcel Apfelbaum @ 2014-03-06 16:14 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: imammedo, pbonzini, qemu-devel, kraxel

On Thu, 2014-03-06 at 17:48 +0200, Michael S. Tsirkin wrote:
> 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 this case.
> 
> 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 91b0122..edca1ed 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -44,9 +44,11 @@ void pstrcpy_targphys(const char *name,
>                        const char *source);
>  
>  extern bool rom_file_in_ram;
> +extern bool option_rom_in_ram;
>  
>  int rom_add_file(const char *file, const char *fw_dir,
> -                 hwaddr addr, int32_t bootindex);
> +                 hwaddr addr, int32_t bootindex,
> +                 bool option);
Hi Michael,
I don't understand the usage of the new "option" parameter.
It is always set to true by the caller

Thanks,
Marcel

>  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);
> @@ -60,7 +62,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, true)
>  #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 0634bee..a19b158 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -54,6 +54,7 @@
>  
>  #include <zlib.h>
>  
> +bool option_rom_in_ram = false;
>  bool rom_file_in_ram = true;
>  
>  static int roms_loaded;
> @@ -624,7 +625,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;
>      int rc, fd = -1;
> @@ -676,7 +678,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 ((!option || option_rom_in_ram) && rom_file_in_ram) {
>              data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
>          } else {
>              data = rom->data;
> @@ -755,12 +757,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)

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] pc: avoid duplicate names for ROM MRs
  2014-03-06 16:14 ` [Qemu-devel] [PATCH 1/2] pc: avoid duplicate names for ROM MRs Marcel Apfelbaum
@ 2014-03-06 16:20   ` Michael S. Tsirkin
  0 siblings, 0 replies; 4+ messages in thread
From: Michael S. Tsirkin @ 2014-03-06 16:20 UTC (permalink / raw)
  To: Marcel Apfelbaum; +Cc: imammedo, kraxel, qemu-devel, pbonzini

On Thu, Mar 06, 2014 at 06:14:11PM +0200, Marcel Apfelbaum wrote:
> On Thu, 2014-03-06 at 17:48 +0200, Michael S. Tsirkin wrote:
> > 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 this case.
> > 
> > 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 91b0122..edca1ed 100644
> > --- a/include/hw/loader.h
> > +++ b/include/hw/loader.h
> > @@ -44,9 +44,11 @@ void pstrcpy_targphys(const char *name,
> >                        const char *source);
> >  
> >  extern bool rom_file_in_ram;
> > +extern bool option_rom_in_ram;
> >  
> >  int rom_add_file(const char *file, const char *fw_dir,
> > -                 hwaddr addr, int32_t bootindex);
> > +                 hwaddr addr, int32_t bootindex,
> > +                 bool option);
> Hi Michael,
> I don't understand the usage of the new "option" parameter.
> It is always set to true by the caller
> 
> Thanks,
> Marcel

Ouch for fixed files this was supposed to be false.
Will send v2.


> >  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);
> > @@ -60,7 +62,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, true)
> >  #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 0634bee..a19b158 100644
> > --- a/hw/core/loader.c
> > +++ b/hw/core/loader.c
> > @@ -54,6 +54,7 @@
> >  
> >  #include <zlib.h>
> >  
> > +bool option_rom_in_ram = false;
> >  bool rom_file_in_ram = true;
> >  
> >  static int roms_loaded;
> > @@ -624,7 +625,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;
> >      int rc, fd = -1;
> > @@ -676,7 +678,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 ((!option || option_rom_in_ram) && rom_file_in_ram) {
> >              data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
> >          } else {
> >              data = rom->data;
> > @@ -755,12 +757,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)
> 
> 
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2014-03-06 16:21 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-03-06 15:48 [Qemu-devel] [PATCH 1/2] pc: avoid duplicate names for ROM MRs Michael S. Tsirkin
2014-03-06 15:48 ` [Qemu-devel] [PATCH 2/2] pc: option rom migration compatibility with 1.7 Michael S. Tsirkin
2014-03-06 16:14 ` [Qemu-devel] [PATCH 1/2] pc: avoid duplicate names for ROM MRs Marcel Apfelbaum
2014-03-06 16:20   ` Michael S. Tsirkin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).