* [Qemu-devel] [PATCH v3 2/3] pc: avoid duplicate names for ROM MRs
2014-03-09 18:06 [Qemu-devel] [PATCH v3 1/3] loader: rename in_ram/has_mr Michael S. Tsirkin
@ 2014-03-09 18:06 ` Michael S. Tsirkin
2014-03-09 18:06 ` [Qemu-devel] [PATCH v3 3/3] pc: option rom migration compatibility with 1.7 Michael S. Tsirkin
` (3 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Michael S. Tsirkin @ 2014-03-09 18:06 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, jeff.nelson, lersek, dgilbert
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* [Qemu-devel] [PATCH v3 3/3] pc: option rom migration compatibility with 1.7
2014-03-09 18:06 [Qemu-devel] [PATCH v3 1/3] loader: rename in_ram/has_mr Michael S. Tsirkin
2014-03-09 18:06 ` [Qemu-devel] [PATCH v3 2/3] pc: avoid duplicate names for ROM MRs Michael S. Tsirkin
@ 2014-03-09 18:06 ` Michael S. Tsirkin
2014-03-09 21:22 ` [Qemu-devel] [PATCH v3 1/3] loader: rename in_ram/has_mr Jeff E. Nelson
` (2 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Michael S. Tsirkin @ 2014-03-09 18:06 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, jeff.nelson, lersek, dgilbert, Anthony Liguori
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH v3 1/3] loader: rename in_ram/has_mr
2014-03-09 18:06 [Qemu-devel] [PATCH v3 1/3] loader: rename in_ram/has_mr Michael S. Tsirkin
2014-03-09 18:06 ` [Qemu-devel] [PATCH v3 2/3] pc: avoid duplicate names for ROM MRs Michael S. Tsirkin
2014-03-09 18:06 ` [Qemu-devel] [PATCH v3 3/3] pc: option rom migration compatibility with 1.7 Michael S. Tsirkin
@ 2014-03-09 21:22 ` Jeff E. Nelson
2014-03-10 8:58 ` Paolo Bonzini
2014-03-11 11:57 ` Marcel Apfelbaum
4 siblings, 0 replies; 7+ messages in thread
From: Jeff E. Nelson @ 2014-03-09 21:22 UTC (permalink / raw)
To: Michael S. Tsirkin, qemu-devel
Cc: pbonzini, jeff.nelson, lersek, dgilbert, Anthony Liguori
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH v3 1/3] loader: rename in_ram/has_mr
2014-03-09 18:06 [Qemu-devel] [PATCH v3 1/3] loader: rename in_ram/has_mr Michael S. Tsirkin
` (2 preceding siblings ...)
2014-03-09 21:22 ` [Qemu-devel] [PATCH v3 1/3] loader: rename in_ram/has_mr Jeff E. Nelson
@ 2014-03-10 8:58 ` Paolo Bonzini
2014-03-11 11:28 ` Michael S. Tsirkin
2014-03-11 11:57 ` Marcel Apfelbaum
4 siblings, 1 reply; 7+ messages in thread
From: Paolo Bonzini @ 2014-03-10 8:58 UTC (permalink / raw)
To: Michael S. Tsirkin, qemu-devel
Cc: jeff.nelson, lersek, dgilbert, Anthony Liguori
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH v3 1/3] loader: rename in_ram/has_mr
2014-03-10 8:58 ` Paolo Bonzini
@ 2014-03-11 11:28 ` Michael S. Tsirkin
0 siblings, 0 replies; 7+ messages in thread
From: Michael S. Tsirkin @ 2014-03-11 11:28 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: jeff.nelson, lersek, qemu-devel, Anthony Liguori, dgilbert
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/3] loader: rename in_ram/has_mr
2014-03-09 18:06 [Qemu-devel] [PATCH v3 1/3] loader: rename in_ram/has_mr Michael S. Tsirkin
` (3 preceding siblings ...)
2014-03-10 8:58 ` Paolo Bonzini
@ 2014-03-11 11:57 ` Marcel Apfelbaum
4 siblings, 0 replies; 7+ messages in thread
From: Marcel Apfelbaum @ 2014-03-11 11:57 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: jeff.nelson, qemu-devel, dgilbert, Anthony Liguori, pbonzini,
lersek
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;
> }
>
^ permalink raw reply [flat|nested] 7+ messages in thread