| Message ID | 20240430150643.111976-7-shentey@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | This series changes the "isa-bios" MemoryRegion to be an alias rather than a | expand |
On 30/4/24 17:06, Bernhard Beschow wrote: > In the -bios case the "isa-bios" memory region is an alias to the BIOS mapped > to the top of the 4G memory boundary. Do the same in the -pflash case, but only > for new machine versions for migration compatibility. This establishes common > behavior and makes pflash commands work in the "isa-bios" region which some > real-world legacy bioses rely on. Can you amend a diff of 'info mtree' here to see how the layout changes? > Note that in the sev_enabled() case, the "isa-bios" memory region in the -pflash > case will now also point to encrypted memory, just like it already does in the > -bios case. > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > --- > include/hw/i386/pc.h | 1 + > hw/i386/pc.c | 1 + > hw/i386/pc_piix.c | 3 +++ > hw/i386/pc_q35.c | 2 ++ > hw/i386/pc_sysfw.c | 8 +++++++- > 5 files changed, 14 insertions(+), 1 deletion(-) I'm still not convinced we need a migration back compat for this... > diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c > index 82d37cb376..ac88ad4eb9 100644 > --- a/hw/i386/pc_sysfw.c > +++ b/hw/i386/pc_sysfw.c > @@ -135,6 +135,7 @@ static void pc_system_flash_map(PCMachineState *pcms, > MemoryRegion *rom_memory) > { > X86MachineState *x86ms = X86_MACHINE(pcms); > + PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms); > hwaddr total_size = 0; > int i; > BlockBackend *blk; > @@ -184,7 +185,12 @@ static void pc_system_flash_map(PCMachineState *pcms, > > if (i == 0) { > flash_mem = pflash_cfi01_get_memory(system_flash); > - pc_isa_bios_init(&x86ms->isa_bios, rom_memory, flash_mem); > + if (pcmc->isa_bios_alias) { > + x86_isa_bios_init(&x86ms->isa_bios, rom_memory, flash_mem, > + true); > + } else { > + pc_isa_bios_init(&x86ms->isa_bios, rom_memory, flash_mem); > + } > > /* Encrypt the pflash boot ROM */ > if (sev_enabled()) {
Am 30. April 2024 15:39:21 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >On 30/4/24 17:06, Bernhard Beschow wrote: >> In the -bios case the "isa-bios" memory region is an alias to the BIOS mapped >> to the top of the 4G memory boundary. Do the same in the -pflash case, but only >> for new machine versions for migration compatibility. This establishes common >> behavior and makes pflash commands work in the "isa-bios" region which some >> real-world legacy bioses rely on. > >Can you amend a diff of 'info mtree' here to see how the layout changes? Will do. Right now I have to manually sort the output to get a minimal diff. Is there a way to get a stable ordering of the memory regions? How would one fix that if this is currently impossible? With stable orderings we could have automated memory map tests which had been handy in the past. > >> Note that in the sev_enabled() case, the "isa-bios" memory region in the -pflash >> case will now also point to encrypted memory, just like it already does in the >> -bios case. >> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> --- >> include/hw/i386/pc.h | 1 + >> hw/i386/pc.c | 1 + >> hw/i386/pc_piix.c | 3 +++ >> hw/i386/pc_q35.c | 2 ++ >> hw/i386/pc_sysfw.c | 8 +++++++- >> 5 files changed, 14 insertions(+), 1 deletion(-) > >I'm still not convinced we need a migration back compat for this... A copy behaves different than an alias, thus there is a behavioral change. Whether it really matters in practice for the kind of guests we care about I can't tell. Therefore I'd keep the compat machinery. Best regards, Bernhard > >> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c >> index 82d37cb376..ac88ad4eb9 100644 >> --- a/hw/i386/pc_sysfw.c >> +++ b/hw/i386/pc_sysfw.c >> @@ -135,6 +135,7 @@ static void pc_system_flash_map(PCMachineState *pcms, >> MemoryRegion *rom_memory) >> { >> X86MachineState *x86ms = X86_MACHINE(pcms); >> + PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms); >> hwaddr total_size = 0; >> int i; >> BlockBackend *blk; >> @@ -184,7 +185,12 @@ static void pc_system_flash_map(PCMachineState *pcms, >> if (i == 0) { >> flash_mem = pflash_cfi01_get_memory(system_flash); >> - pc_isa_bios_init(&x86ms->isa_bios, rom_memory, flash_mem); >> + if (pcmc->isa_bios_alias) { >> + x86_isa_bios_init(&x86ms->isa_bios, rom_memory, flash_mem, >> + true); >> + } else { >> + pc_isa_bios_init(&x86ms->isa_bios, rom_memory, flash_mem); >> + } >> /* Encrypt the pflash boot ROM */ >> if (sev_enabled()) { >
On 8/5/24 10:17, Bernhard Beschow wrote: > > > Am 30. April 2024 15:39:21 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >> On 30/4/24 17:06, Bernhard Beschow wrote: >>> In the -bios case the "isa-bios" memory region is an alias to the BIOS mapped >>> to the top of the 4G memory boundary. Do the same in the -pflash case, but only >>> for new machine versions for migration compatibility. This establishes common >>> behavior and makes pflash commands work in the "isa-bios" region which some >>> real-world legacy bioses rely on. >> >> Can you amend a diff of 'info mtree' here to see how the layout changes? > > Will do. > > Right now I have to manually sort the output to get a minimal diff. Is there a way to get a stable ordering of the memory regions? How would one fix that if this is currently impossible? With stable orderings we could have automated memory map tests which had been handy in the past. It is stable until the guest plays with it, so it depends at which point in guest execution time you stop your VM to dump the mem tree. > >> >>> Note that in the sev_enabled() case, the "isa-bios" memory region in the -pflash >>> case will now also point to encrypted memory, just like it already does in the >>> -bios case. >>> >>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>> --- >>> include/hw/i386/pc.h | 1 + >>> hw/i386/pc.c | 1 + >>> hw/i386/pc_piix.c | 3 +++ >>> hw/i386/pc_q35.c | 2 ++ >>> hw/i386/pc_sysfw.c | 8 +++++++- >>> 5 files changed, 14 insertions(+), 1 deletion(-) >> >> I'm still not convinced we need a migration back compat for this... > > A copy behaves different than an alias, thus there is a behavioral change. Whether it really matters in practice for the kind of guests we care about I can't tell. Therefore I'd keep the compat machinery. Yeah I know MST asked that, I'm not against, I'm just not convinced (in particular because we'll need to maintain these few lines for 6 years). > > Best regards, > Bernhard > >> >>> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c >>> index 82d37cb376..ac88ad4eb9 100644 >>> --- a/hw/i386/pc_sysfw.c >>> +++ b/hw/i386/pc_sysfw.c >>> @@ -135,6 +135,7 @@ static void pc_system_flash_map(PCMachineState *pcms, >>> MemoryRegion *rom_memory) >>> { >>> X86MachineState *x86ms = X86_MACHINE(pcms); >>> + PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms); >>> hwaddr total_size = 0; >>> int i; >>> BlockBackend *blk; >>> @@ -184,7 +185,12 @@ static void pc_system_flash_map(PCMachineState *pcms, >>> if (i == 0) { >>> flash_mem = pflash_cfi01_get_memory(system_flash); >>> - pc_isa_bios_init(&x86ms->isa_bios, rom_memory, flash_mem); >>> + if (pcmc->isa_bios_alias) { >>> + x86_isa_bios_init(&x86ms->isa_bios, rom_memory, flash_mem, >>> + true); >>> + } else { >>> + pc_isa_bios_init(&x86ms->isa_bios, rom_memory, flash_mem); >>> + } >>> /* Encrypt the pflash boot ROM */ >>> if (sev_enabled()) { >>
On Tue, Apr 30, 2024 at 5:39 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > I'm still not convinced we need a migration back compat for this... It's absolutely needed, memory_region_init_ram(isa_bios, NULL, "isa-bios", isa_bios_size, &error_fatal); will register a RAM region for migration, and when the destination receives data from an older source, it will not find it it will fail. On the other hand, if migrating backwards isa-bios will not be populated and the guest may fail after reboot. Paolo > > diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c > > index 82d37cb376..ac88ad4eb9 100644 > > --- a/hw/i386/pc_sysfw.c > > +++ b/hw/i386/pc_sysfw.c > > @@ -135,6 +135,7 @@ static void pc_system_flash_map(PCMachineState *pcms, > > MemoryRegion *rom_memory) > > { > > X86MachineState *x86ms = X86_MACHINE(pcms); > > + PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms); > > hwaddr total_size = 0; > > int i; > > BlockBackend *blk; > > @@ -184,7 +185,12 @@ static void pc_system_flash_map(PCMachineState *pcms, > > > > if (i == 0) { > > flash_mem = pflash_cfi01_get_memory(system_flash); > > - pc_isa_bios_init(&x86ms->isa_bios, rom_memory, flash_mem); > > + if (pcmc->isa_bios_alias) { > > + x86_isa_bios_init(&x86ms->isa_bios, rom_memory, flash_mem, > > + true); > > + } else { > > + pc_isa_bios_init(&x86ms->isa_bios, rom_memory, flash_mem); > > + } > > > > /* Encrypt the pflash boot ROM */ > > if (sev_enabled()) { >
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index e52290916c..ad9c3d9ba8 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -119,6 +119,7 @@ struct PCMachineClass { bool enforce_aligned_dimm; bool broken_reserved_end; bool enforce_amd_1tb_hole; + bool isa_bios_alias; /* generate legacy CPU hotplug AML */ bool legacy_cpu_hotplug; diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 08c7de416f..ce61bb7fc1 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1811,6 +1811,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data) pcmc->has_reserved_memory = true; pcmc->enforce_aligned_dimm = true; pcmc->enforce_amd_1tb_hole = true; + pcmc->isa_bios_alias = true; /* BIOS ACPI tables: 128K. Other BIOS datastructures: less than 4K reported * to be used at the moment, 32K should be enough for a while. */ pcmc->acpi_data_size = 0x20000 + 0x8000; diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index 8850c49c66..d4e9deb509 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -525,12 +525,15 @@ DEFINE_I440FX_MACHINE(v9_1, "pc-i440fx-9.1", NULL, static void pc_i440fx_9_0_machine_options(MachineClass *m) { + PCMachineClass *pcmc = PC_MACHINE_CLASS(m); + pc_i440fx_9_1_machine_options(m); m->alias = NULL; m->is_default = false; compat_props_add(m->compat_props, hw_compat_9_0, hw_compat_9_0_len); compat_props_add(m->compat_props, pc_compat_9_0, pc_compat_9_0_len); + pcmc->isa_bios_alias = false; } DEFINE_I440FX_MACHINE(v9_0, "pc-i440fx-9.0", NULL, diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index bb53a51ac1..bd7db4abac 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -378,10 +378,12 @@ DEFINE_Q35_MACHINE(v9_1, "pc-q35-9.1", NULL, static void pc_q35_9_0_machine_options(MachineClass *m) { + PCMachineClass *pcmc = PC_MACHINE_CLASS(m); pc_q35_9_1_machine_options(m); m->alias = NULL; compat_props_add(m->compat_props, hw_compat_9_0, hw_compat_9_0_len); compat_props_add(m->compat_props, pc_compat_9_0, pc_compat_9_0_len); + pcmc->isa_bios_alias = false; } DEFINE_Q35_MACHINE(v9_0, "pc-q35-9.0", NULL, diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c index 82d37cb376..ac88ad4eb9 100644 --- a/hw/i386/pc_sysfw.c +++ b/hw/i386/pc_sysfw.c @@ -135,6 +135,7 @@ static void pc_system_flash_map(PCMachineState *pcms, MemoryRegion *rom_memory) { X86MachineState *x86ms = X86_MACHINE(pcms); + PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms); hwaddr total_size = 0; int i; BlockBackend *blk; @@ -184,7 +185,12 @@ static void pc_system_flash_map(PCMachineState *pcms, if (i == 0) { flash_mem = pflash_cfi01_get_memory(system_flash); - pc_isa_bios_init(&x86ms->isa_bios, rom_memory, flash_mem); + if (pcmc->isa_bios_alias) { + x86_isa_bios_init(&x86ms->isa_bios, rom_memory, flash_mem, + true); + } else { + pc_isa_bios_init(&x86ms->isa_bios, rom_memory, flash_mem); + } /* Encrypt the pflash boot ROM */ if (sev_enabled()) {
In the -bios case the "isa-bios" memory region is an alias to the BIOS mapped to the top of the 4G memory boundary. Do the same in the -pflash case, but only for new machine versions for migration compatibility. This establishes common behavior and makes pflash commands work in the "isa-bios" region which some real-world legacy bioses rely on. Note that in the sev_enabled() case, the "isa-bios" memory region in the -pflash case will now also point to encrypted memory, just like it already does in the -bios case. Signed-off-by: Bernhard Beschow <shentey@gmail.com> --- include/hw/i386/pc.h | 1 + hw/i386/pc.c | 1 + hw/i386/pc_piix.c | 3 +++ hw/i386/pc_q35.c | 2 ++ hw/i386/pc_sysfw.c | 8 +++++++- 5 files changed, 14 insertions(+), 1 deletion(-)