diff mbox series

[v2,6/6] hw/i386/pc_sysfw: Alias rather than copy isa-bios region

Message ID 20240430150643.111976-7-shentey@gmail.com (mailing list archive)
State New, archived
Headers show
Series This series changes the "isa-bios" MemoryRegion to be an alias rather than a | expand

Commit Message

Bernhard Beschow April 30, 2024, 3:06 p.m. UTC
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(-)

Comments

Philippe Mathieu-Daudé April 30, 2024, 3:39 p.m. UTC | #1
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()) {
Bernhard Beschow May 8, 2024, 8:17 a.m. UTC | #2
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()) {
>
Philippe Mathieu-Daudé May 8, 2024, 9:58 a.m. UTC | #3
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()) {
>>
Paolo Bonzini May 8, 2024, 3:56 p.m. UTC | #4
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 mbox series

Patch

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()) {