Re: [Qemu-devel] [PATCH for-2.1 2/2] acpi: mark ACPI tables ROM blob as extend-able on migration
On Tue, Nov 04, 2014 at 06:35:50PM +0100, Paolo Bonzini wrote: > > > On 04/11/2014 18:25, Michael S. Tsirkin wrote: > >> (Regarding recompilation with a different iasl version, SSDT blocks are > >> simple enough that I think we can just build them in C code. We're > >> already doing it for the much more complicated PCI bridge hotplug > >> interface. BTW, can you pick up at least the patch to move the memory > >> hotplug device from SSDT to DSDT?). > >> > >> Paolo > > > > I'm not against that - does this have value by itself? > > It matches more closely what we do for processors, and is more faithful > to the idea that the SSDTs express supplementary functionality. > > Paolo OK - I'll review that separately. Thanks! -- MST
Re: [Qemu-devel] [PATCH for-2.1 2/2] acpi: mark ACPI tables ROM blob as extend-able on migration
On 04/11/2014 18:25, Michael S. Tsirkin wrote: >> (Regarding recompilation with a different iasl version, SSDT blocks are >> simple enough that I think we can just build them in C code. We're >> already doing it for the much more complicated PCI bridge hotplug >> interface. BTW, can you pick up at least the patch to move the memory >> hotplug device from SSDT to DSDT?). >> >> Paolo > > I'm not against that - does this have value by itself? It matches more closely what we do for processors, and is more faithful to the idea that the SSDTs express supplementary functionality. Paolo
Re: [Qemu-devel] [PATCH for-2.1 2/2] acpi: mark ACPI tables ROM blob as extend-able on migration
On Tue, Nov 04, 2014 at 05:54:26PM +0100, Paolo Bonzini wrote: > On 04/11/2014 17:46, Michael S. Tsirkin wrote: > > On Tue, Nov 04, 2014 at 05:30:14PM +0100, Paolo Bonzini wrote: > >> On 03/11/2014 18:36, Dr. David Alan Gilbert wrote: > >>> 1) It's a block of data that's never mapped into the guests address > >>> space > >>> 2) It can change, but only at guest reset > >>> 3) Worst case is it can get upto about 2MB in size > >>> > >>> it's pretty marginal whether this thing should be a RAMBlock, > >>> it doesn't feel like normal RAM or ROM in most ways; but there > >>> again 2MB is getting a bit large for the device state; hmm. > >> > >> And also I think changing migration format gratuitously is bad. We > >> decided to make these RAMs, which has some advantages and turned out to > >> have some possible disadvantages, but it's not a big deal. They are > >> some kind of EPROM if you wish. > >> > >> The important point is that we can (and arguably _should_ since it keeps > >> us honest!) make these ACPI tables RAMBlocks fixed-size per machine > >> type. See the patches I posted around late September/early October. > >> There is no need to support auto-fixing of the RAMBlock's sizes. > > > > I'm not sure I buy that we should. ACPI bytecode can express > > identical interfaces in different ways. Even just recompiling > > ACPI from source can give you a different binary, > > same is true for a minor change in ACPI code. > > Migrating between two almost identical builds from qemu seems a > > very reasonable thing to do. > > Yes, identical ACPI blocks are just a sufficient condition, not a > necessary one. But it's very easy to enforce it, and it's what the > acpi-tables-test already checks. It makes sense to me to stick with it. > > (Regarding recompilation with a different iasl version, SSDT blocks are > simple enough that I think we can just build them in C code. We're > already doing it for the much more complicated PCI bridge hotplug > interface. BTW, can you pick up at least the patch to move the memory > hotplug device from SSDT to DSDT?). > > Paolo I'm not against that - does this have value by itself? -- MST
Re: [Qemu-devel] [PATCH for-2.1 2/2] acpi: mark ACPI tables ROM blob as extend-able on migration
On 04/11/2014 17:46, Michael S. Tsirkin wrote: > On Tue, Nov 04, 2014 at 05:30:14PM +0100, Paolo Bonzini wrote: >> On 03/11/2014 18:36, Dr. David Alan Gilbert wrote: >>> 1) It's a block of data that's never mapped into the guests address >>> space >>> 2) It can change, but only at guest reset >>> 3) Worst case is it can get upto about 2MB in size >>> >>> it's pretty marginal whether this thing should be a RAMBlock, >>> it doesn't feel like normal RAM or ROM in most ways; but there >>> again 2MB is getting a bit large for the device state; hmm. >> >> And also I think changing migration format gratuitously is bad. We >> decided to make these RAMs, which has some advantages and turned out to >> have some possible disadvantages, but it's not a big deal. They are >> some kind of EPROM if you wish. >> >> The important point is that we can (and arguably _should_ since it keeps >> us honest!) make these ACPI tables RAMBlocks fixed-size per machine >> type. See the patches I posted around late September/early October. >> There is no need to support auto-fixing of the RAMBlock's sizes. > > I'm not sure I buy that we should. ACPI bytecode can express > identical interfaces in different ways. Even just recompiling > ACPI from source can give you a different binary, > same is true for a minor change in ACPI code. > Migrating between two almost identical builds from qemu seems a > very reasonable thing to do. Yes, identical ACPI blocks are just a sufficient condition, not a necessary one. But it's very easy to enforce it, and it's what the acpi-tables-test already checks. It makes sense to me to stick with it. (Regarding recompilation with a different iasl version, SSDT blocks are simple enough that I think we can just build them in C code. We're already doing it for the much more complicated PCI bridge hotplug interface. BTW, can you pick up at least the patch to move the memory hotplug device from SSDT to DSDT?). Paolo
Re: [Qemu-devel] [PATCH for-2.1 2/2] acpi: mark ACPI tables ROM blob as extend-able on migration
On Tue, Nov 04, 2014 at 05:30:14PM +0100, Paolo Bonzini wrote: > On 03/11/2014 18:36, Dr. David Alan Gilbert wrote: > > 1) It's a block of data that's never mapped into the guests address > > space > > 2) It can change, but only at guest reset > > 3) Worst case is it can get upto about 2MB in size > > > > it's pretty marginal whether this thing should be a RAMBlock, > > it doesn't feel like normal RAM or ROM in most ways; but there > > again 2MB is getting a bit large for the device state; hmm. > > And also I think changing migration format gratuitously is bad. We > decided to make these RAMs, which has some advantages and turned out to > have some possible disadvantages, but it's not a big deal. They are > some kind of EPROM if you wish. > > The important point is that we can (and arguably _should_ since it keeps > us honest!) make these ACPI tables RAMBlocks fixed-size per machine > type. See the patches I posted around late September/early October. > There is no need to support auto-fixing of the RAMBlock's sizes. > > Paolo I'm not sure I buy that we should. ACPI bytecode can express identical interfaces in different ways. Even just recompiling ACPI from source can give you a different binary, same is true for a minor change in ACPI code. Migrating between two almost identical builds from qemu seems a very reasonable thing to do. -- MST
Re: [Qemu-devel] [PATCH for-2.1 2/2] acpi: mark ACPI tables ROM blob as extend-able on migration
On 03/11/2014 18:36, Dr. David Alan Gilbert wrote: > 1) It's a block of data that's never mapped into the guests address > space > 2) It can change, but only at guest reset > 3) Worst case is it can get upto about 2MB in size > > it's pretty marginal whether this thing should be a RAMBlock, > it doesn't feel like normal RAM or ROM in most ways; but there > again 2MB is getting a bit large for the device state; hmm. And also I think changing migration format gratuitously is bad. We decided to make these RAMs, which has some advantages and turned out to have some possible disadvantages, but it's not a big deal. They are some kind of EPROM if you wish. The important point is that we can (and arguably _should_ since it keeps us honest!) make these ACPI tables RAMBlocks fixed-size per machine type. See the patches I posted around late September/early October. There is no need to support auto-fixing of the RAMBlock's sizes. Paolo
Re: [Qemu-devel] [PATCH for-2.1 2/2] acpi: mark ACPI tables ROM blob as extend-able on migration
* Igor Mammedov (imamm...@redhat.com) wrote:
> It fixes migration failure for machine type pc-i440fx-1.7 from
> QEMU 1.7/2.0 to QEMU 2.1
>
> Migration fails due to ACPI tables size grows across 1.7-2.1
> versions. That causes ACPI tables ROM blob to change its size
> differently for the same configurations on different QEMU versions.
> As result migration code bails out when attempting to load
> smaller ROM blob into a bigger one on a newer QEMU.
> To trigger failure it's enough to add pci-bridge device to QEMU CLI
>
> Marking ACPI tables ROM blob as extend-able on migration allows
> QEMU to load smaller ACPI tables from QEMU 1.7/2.0, fixing
> forward migration failure introduced since 2.0 which affects
> only configurations that cause ACPI ROM blob size change.
(To follow up from an irc thread earlier today)
I only vaguely understand this case but my understanding is:
1) It's a block of data that's never mapped into the guests address
space
2) It can change, but only at guest reset
3) Worst case is it can get upto about 2MB in size
it's pretty marginal whether this thing should be a RAMBlock,
it doesn't feel like normal RAM or ROM in most ways; but there
again 2MB is getting a bit large for the device state; hmm.
Dave
>
> Signed-off-by: Igor Mammedov
> ---
> hw/core/loader.c | 6 +-
> hw/i386/acpi-build.c | 2 +-
> include/hw/loader.h | 5 +++--
> 3 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 2bf6b8f..d09b894 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -722,7 +722,8 @@ err:
>
> 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)
> + FWCfgReadCallback fw_callback, void *callback_opaque,
> + bool extendable)
> {
> Rom *rom;
> void *data = NULL;
> @@ -742,6 +743,9 @@ void *rom_add_blob(const char *name, const void *blob,
> size_t len,
>
> if (rom_file_has_mr) {
> data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
> +if (extendable) {
> +memory_region_permit_extendable_migration(rom->mr);
> +}
> } else {
> data = rom->data;
> }
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index ebc5f03..651c06b 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1558,7 +1558,7 @@ static void *acpi_add_rom_blob(AcpiBuildState
> *build_state, GArray *blob,
> const char *name)
> {
> return rom_add_blob(name, blob->data, acpi_data_len(blob), -1, name,
> -acpi_build_update, build_state);
> +acpi_build_update, build_state, true);
> }
>
> static const VMStateDescription vmstate_acpi_build = {
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 796cbf9..e5a24ac 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -57,7 +57,8 @@ int rom_add_file(const char *file, const char *fw_dir,
> 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);
> + FWCfgReadCallback fw_callback, void *callback_opaque,
> + bool extendable);
> int rom_add_elf_program(const char *name, void *data, size_t datasize,
> size_t romsize, hwaddr addr);
> int rom_load_all(void);
> @@ -70,7 +71,7 @@ void do_info_roms(Monitor *mon, const QDict *qdict);
> #define rom_add_file_fixed(_f, _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)
> +rom_add_blob(_f, _b, _l, _a, NULL, NULL, NULL, false)
>
> #define PC_ROM_MIN_VGA 0xc
> #define PC_ROM_MIN_OPTION 0xc8000
> --
> 1.8.3.1
>
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH for-2.1 2/2] acpi: mark ACPI tables ROM blob as extend-able on migration
Il 28/07/2014 09:56, Igor Mammedov ha scritto: >> > It doesn't handle the case of ACPI tables that shrink, which can happen >> > as well. I guess if this ever happens we can just hard-code the table >> > size of the "old" versions to something big enough (64K?) and keep using >> > fine-grained sizing. > That was intentionally omitted in here so far size only goes up from 1.7 to > 2.1. My though was that can enforce minimum size later during 2.2 cycle > Anyway, I'll think more about it, and maybe post additional patch > on top of this to set minimum size if I find a reason for it to be in 2.1. No, there's no need for it in 2.1. Paolo
Re: [Qemu-devel] [PATCH for-2.1 2/2] acpi: mark ACPI tables ROM blob as extend-able on migration
On Fri, 25 Jul 2014 19:37:41 +0200 Paolo Bonzini wrote: > Il 25/07/2014 17:48, Igor Mammedov ha scritto: > > It fixes migration failure for machine type pc-i440fx-1.7 from > > QEMU 1.7/2.0 to QEMU 2.1 > > > > Migration fails due to ACPI tables size grows across 1.7-2.1 > > versions. That causes ACPI tables ROM blob to change its size > > differently for the same configurations on different QEMU versions. > > As result migration code bails out when attempting to load > > smaller ROM blob into a bigger one on a newer QEMU. > > To trigger failure it's enough to add pci-bridge device to QEMU CLI > > > > Marking ACPI tables ROM blob as extend-able on migration allows > > QEMU to load smaller ACPI tables from QEMU 1.7/2.0, fixing > > forward migration failure introduced since 2.0 which affects > > only configurations that cause ACPI ROM blob size change. > > This works in this case, and it is more friendly to downstreams indeed. > It also is simpler, at least on the surface. I think the ramifications > could be a bit larger than with my own patches, but still I guess it's > more appropriate at this point of the release cycle. > > It doesn't handle the case of ACPI tables that shrink, which can happen > as well. I guess if this ever happens we can just hard-code the table > size of the "old" versions to something big enough (64K?) and keep using > fine-grained sizing. That was intentionally omitted in here so far size only goes up from 1.7 to 2.1. My though was that can enforce minimum size later during 2.2 cycle Anyway, I'll think more about it, and maybe post additional patch on top of this to set minimum size if I find a reason for it to be in 2.1. > > I'd like a day or two to mull about it, but I have it even if the > patches are applied. Peter, feel free to go ahead with Igor's patches. > > Paolo > >
Re: [Qemu-devel] [PATCH for-2.1 2/2] acpi: mark ACPI tables ROM blob as extend-able on migration
On 07/25/14 17:48, Igor Mammedov wrote:
> It fixes migration failure for machine type pc-i440fx-1.7 from
> QEMU 1.7/2.0 to QEMU 2.1
>
> Migration fails due to ACPI tables size grows across 1.7-2.1
> versions. That causes ACPI tables ROM blob to change its size
> differently for the same configurations on different QEMU versions.
> As result migration code bails out when attempting to load
> smaller ROM blob into a bigger one on a newer QEMU.
> To trigger failure it's enough to add pci-bridge device to QEMU CLI
>
> Marking ACPI tables ROM blob as extend-able on migration allows
> QEMU to load smaller ACPI tables from QEMU 1.7/2.0, fixing
> forward migration failure introduced since 2.0 which affects
> only configurations that cause ACPI ROM blob size change.
>
> Signed-off-by: Igor Mammedov
> ---
> hw/core/loader.c | 6 +-
> hw/i386/acpi-build.c | 2 +-
> include/hw/loader.h | 5 +++--
> 3 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 2bf6b8f..d09b894 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -722,7 +722,8 @@ err:
>
> 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)
> + FWCfgReadCallback fw_callback, void *callback_opaque,
> + bool extendable)
> {
> Rom *rom;
> void *data = NULL;
> @@ -742,6 +743,9 @@ void *rom_add_blob(const char *name, const void *blob,
> size_t len,
>
> if (rom_file_has_mr) {
> data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
> +if (extendable) {
> +memory_region_permit_extendable_migration(rom->mr);
> +}
> } else {
> data = rom->data;
> }
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index ebc5f03..651c06b 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1558,7 +1558,7 @@ static void *acpi_add_rom_blob(AcpiBuildState
> *build_state, GArray *blob,
> const char *name)
> {
> return rom_add_blob(name, blob->data, acpi_data_len(blob), -1, name,
> -acpi_build_update, build_state);
> +acpi_build_update, build_state, true);
> }
>
> static const VMStateDescription vmstate_acpi_build = {
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 796cbf9..e5a24ac 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -57,7 +57,8 @@ int rom_add_file(const char *file, const char *fw_dir,
> 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);
> + FWCfgReadCallback fw_callback, void *callback_opaque,
> + bool extendable);
> int rom_add_elf_program(const char *name, void *data, size_t datasize,
> size_t romsize, hwaddr addr);
> int rom_load_all(void);
> @@ -70,7 +71,7 @@ void do_info_roms(Monitor *mon, const QDict *qdict);
> #define rom_add_file_fixed(_f, _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)
> +rom_add_blob(_f, _b, _l, _a, NULL, NULL, NULL, false)
>
> #define PC_ROM_MIN_VGA 0xc
> #define PC_ROM_MIN_OPTION 0xc8000
>
Reviewed-by: Laszlo Ersek
Re: [Qemu-devel] [PATCH for-2.1 2/2] acpi: mark ACPI tables ROM blob as extend-able on migration
Il 25/07/2014 17:48, Igor Mammedov ha scritto: > It fixes migration failure for machine type pc-i440fx-1.7 from > QEMU 1.7/2.0 to QEMU 2.1 > > Migration fails due to ACPI tables size grows across 1.7-2.1 > versions. That causes ACPI tables ROM blob to change its size > differently for the same configurations on different QEMU versions. > As result migration code bails out when attempting to load > smaller ROM blob into a bigger one on a newer QEMU. > To trigger failure it's enough to add pci-bridge device to QEMU CLI > > Marking ACPI tables ROM blob as extend-able on migration allows > QEMU to load smaller ACPI tables from QEMU 1.7/2.0, fixing > forward migration failure introduced since 2.0 which affects > only configurations that cause ACPI ROM blob size change. This works in this case, and it is more friendly to downstreams indeed. It also is simpler, at least on the surface. I think the ramifications could be a bit larger than with my own patches, but still I guess it's more appropriate at this point of the release cycle. It doesn't handle the case of ACPI tables that shrink, which can happen as well. I guess if this ever happens we can just hard-code the table size of the "old" versions to something big enough (64K?) and keep using fine-grained sizing. I'd like a day or two to mull about it, but I have it even if the patches are applied. Peter, feel free to go ahead with Igor's patches. Paolo
[Qemu-devel] [PATCH for-2.1 2/2] acpi: mark ACPI tables ROM blob as extend-able on migration
It fixes migration failure for machine type pc-i440fx-1.7 from
QEMU 1.7/2.0 to QEMU 2.1
Migration fails due to ACPI tables size grows across 1.7-2.1
versions. That causes ACPI tables ROM blob to change its size
differently for the same configurations on different QEMU versions.
As result migration code bails out when attempting to load
smaller ROM blob into a bigger one on a newer QEMU.
To trigger failure it's enough to add pci-bridge device to QEMU CLI
Marking ACPI tables ROM blob as extend-able on migration allows
QEMU to load smaller ACPI tables from QEMU 1.7/2.0, fixing
forward migration failure introduced since 2.0 which affects
only configurations that cause ACPI ROM blob size change.
Signed-off-by: Igor Mammedov
---
hw/core/loader.c | 6 +-
hw/i386/acpi-build.c | 2 +-
include/hw/loader.h | 5 +++--
3 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 2bf6b8f..d09b894 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -722,7 +722,8 @@ err:
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)
+ FWCfgReadCallback fw_callback, void *callback_opaque,
+ bool extendable)
{
Rom *rom;
void *data = NULL;
@@ -742,6 +743,9 @@ void *rom_add_blob(const char *name, const void *blob,
size_t len,
if (rom_file_has_mr) {
data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
+if (extendable) {
+memory_region_permit_extendable_migration(rom->mr);
+}
} else {
data = rom->data;
}
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index ebc5f03..651c06b 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1558,7 +1558,7 @@ static void *acpi_add_rom_blob(AcpiBuildState
*build_state, GArray *blob,
const char *name)
{
return rom_add_blob(name, blob->data, acpi_data_len(blob), -1, name,
-acpi_build_update, build_state);
+acpi_build_update, build_state, true);
}
static const VMStateDescription vmstate_acpi_build = {
diff --git a/include/hw/loader.h b/include/hw/loader.h
index 796cbf9..e5a24ac 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -57,7 +57,8 @@ int rom_add_file(const char *file, const char *fw_dir,
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);
+ FWCfgReadCallback fw_callback, void *callback_opaque,
+ bool extendable);
int rom_add_elf_program(const char *name, void *data, size_t datasize,
size_t romsize, hwaddr addr);
int rom_load_all(void);
@@ -70,7 +71,7 @@ void do_info_roms(Monitor *mon, const QDict *qdict);
#define rom_add_file_fixed(_f, _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)
+rom_add_blob(_f, _b, _l, _a, NULL, NULL, NULL, false)
#define PC_ROM_MIN_VGA 0xc
#define PC_ROM_MIN_OPTION 0xc8000
--
1.8.3.1
