| Message ID | 20220922084356.878907-1-kraxel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] x86: add etc/phys-bits fw_cfg file | expand |
On Thu, Sep 22, 2022 at 10:43:56AM +0200, Gerd Hoffmann wrote: > In case phys bits are functional and can be used by the guest (aka > host-phys-bits=on) add a fw_cfg file carrying the value. This can > be used by the guest firmware for address space configuration. > > This is only enabled for 7.2+ machine types for live migration > compatibility reasons. > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> I'm curious why you decided to switch from a cpuid flag to fw cfg. I guess firmware reads fw cfg anyway. But would the guest kernel then need to load a fw cfg driver very early to detect this, too? > --- > hw/i386/fw_cfg.h | 1 + > include/hw/i386/pc.h | 1 + > hw/i386/fw_cfg.c | 12 ++++++++++++ > hw/i386/pc.c | 5 +++++ > hw/i386/pc_piix.c | 2 ++ > hw/i386/pc_q35.c | 2 ++ > 6 files changed, 23 insertions(+) > > diff --git a/hw/i386/fw_cfg.h b/hw/i386/fw_cfg.h > index 275f15c1c5e8..6ff198a6cb85 100644 > --- a/hw/i386/fw_cfg.h > +++ b/hw/i386/fw_cfg.h > @@ -26,5 +26,6 @@ FWCfgState *fw_cfg_arch_create(MachineState *ms, > void fw_cfg_build_smbios(MachineState *ms, FWCfgState *fw_cfg); > void fw_cfg_build_feature_control(MachineState *ms, FWCfgState *fw_cfg); > void fw_cfg_add_acpi_dsdt(Aml *scope, FWCfgState *fw_cfg); > +void fw_cfg_phys_bits(FWCfgState *fw_cfg); > > #endif > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > index c95333514ed3..bedef1ee13c1 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 phys_bits_in_fw_cfg; > > /* generate legacy CPU hotplug AML */ > bool legacy_cpu_hotplug; > diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c > index a283785a8de4..6a1f18925725 100644 > --- a/hw/i386/fw_cfg.c > +++ b/hw/i386/fw_cfg.c > @@ -219,3 +219,15 @@ void fw_cfg_add_acpi_dsdt(Aml *scope, FWCfgState *fw_cfg) > aml_append(dev, aml_name_decl("_CRS", crs)); > aml_append(scope, dev); > } > + > +void fw_cfg_phys_bits(FWCfgState *fw_cfg) > +{ > + X86CPU *cpu = X86_CPU(first_cpu); > + uint64_t phys_bits = cpu->phys_bits; > + > + if (cpu->host_phys_bits) { > + fw_cfg_add_file(fw_cfg, "etc/phys-bits", > + g_memdup2(&phys_bits, sizeof(phys_bits)), > + sizeof(phys_bits)); > + } > +} So, this allows a lot of flexibility, any phys_bits value at all can now be used. Do you expect a use-case for such a flexible mechanism? If this ends up merely repeating CPUID at all times then we are just creating confusion. Could you add the motivation to the commit log pls? > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 566accf7e60a..17ecc7fe4331 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -744,6 +744,7 @@ void pc_machine_done(Notifier *notifier, void *data) > { > PCMachineState *pcms = container_of(notifier, > PCMachineState, machine_done); > + PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms); > X86MachineState *x86ms = X86_MACHINE(pcms); > > cxl_hook_up_pxb_registers(pcms->bus, &pcms->cxl_devices_state, > @@ -764,6 +765,9 @@ void pc_machine_done(Notifier *notifier, void *data) > fw_cfg_build_feature_control(MACHINE(pcms), x86ms->fw_cfg); > /* update FW_CFG_NB_CPUS to account for -device added CPUs */ > fw_cfg_modify_i16(x86ms->fw_cfg, FW_CFG_NB_CPUS, x86ms->boot_cpus); > + if (pcmc->phys_bits_in_fw_cfg) { > + fw_cfg_phys_bits(x86ms->fw_cfg); > + } > } > } > > @@ -1907,6 +1911,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data) > pcmc->kvmclock_enabled = true; > pcmc->enforce_aligned_dimm = true; > pcmc->enforce_amd_1tb_hole = true; > + pcmc->phys_bits_in_fw_cfg = 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 8043a250adf3..c6a4dbd5c0b0 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -447,9 +447,11 @@ DEFINE_I440FX_MACHINE(v7_2, "pc-i440fx-7.2", NULL, > > static void pc_i440fx_7_1_machine_options(MachineClass *m) > { > + PCMachineClass *pcmc = PC_MACHINE_CLASS(m); > pc_i440fx_7_2_machine_options(m); > m->alias = NULL; > m->is_default = false; > + pcmc->phys_bits_in_fw_cfg = false; > compat_props_add(m->compat_props, hw_compat_7_1, hw_compat_7_1_len); > compat_props_add(m->compat_props, pc_compat_7_1, pc_compat_7_1_len); > } > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > index 53eda50e818c..c2b56daa1550 100644 > --- a/hw/i386/pc_q35.c > +++ b/hw/i386/pc_q35.c > @@ -384,8 +384,10 @@ DEFINE_Q35_MACHINE(v7_2, "pc-q35-7.2", NULL, > > static void pc_q35_7_1_machine_options(MachineClass *m) > { > + PCMachineClass *pcmc = PC_MACHINE_CLASS(m); > pc_q35_7_2_machine_options(m); > m->alias = NULL; > + pcmc->phys_bits_in_fw_cfg = false; > compat_props_add(m->compat_props, hw_compat_7_1, hw_compat_7_1_len); > compat_props_add(m->compat_props, pc_compat_7_1, pc_compat_7_1_len); > } > -- > 2.37.3
On Thu, Sep 22, 2022 at 04:55:16AM -0400, Michael S. Tsirkin wrote: > On Thu, Sep 22, 2022 at 10:43:56AM +0200, Gerd Hoffmann wrote: > > In case phys bits are functional and can be used by the guest (aka > > host-phys-bits=on) add a fw_cfg file carrying the value. This can > > be used by the guest firmware for address space configuration. > > > > This is only enabled for 7.2+ machine types for live migration > > compatibility reasons. > > > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > > I'm curious why you decided to switch from a cpuid flag to fw cfg. The kernel people didn't like the cpuid approach. > I guess firmware reads fw cfg anyway. Correct. > But would the guest kernel then need to load a fw cfg driver very > early to detect this, too? Nope, the guest kernel can just work with the address space layout created by the firmware. The firmware can for example reserve a larger 64-bit mmio window in case there is enough address space for that. So it programs the bridge windows etc accordingly, qemu generates matching acpi tables and the kernel picks up the changes via _CRS. > > +void fw_cfg_phys_bits(FWCfgState *fw_cfg) > > +{ > > + X86CPU *cpu = X86_CPU(first_cpu); > > + uint64_t phys_bits = cpu->phys_bits; > > + > > + if (cpu->host_phys_bits) { > > + fw_cfg_add_file(fw_cfg, "etc/phys-bits", > > + g_memdup2(&phys_bits, sizeof(phys_bits)), > > + sizeof(phys_bits)); > > + } > > +} > > So, this allows a lot of flexibility, any phys_bits value at all can now > be used. Do you expect a use-case for such a flexible mechanism? If > this ends up merely repeating CPUID at all times then we are just > creating confusion. Yes, it'll just repeat CPUID. Advantage is that the guest gets the information it needs right away. Alternatively I could create a "etc/reliable-phys-bits" bool. The firmware must consult both fw_cfg and cpuid then. take care, Gerd
On Thu, Sep 22, 2022 at 11:37:10AM +0200, Gerd Hoffmann wrote: > On Thu, Sep 22, 2022 at 04:55:16AM -0400, Michael S. Tsirkin wrote: > > On Thu, Sep 22, 2022 at 10:43:56AM +0200, Gerd Hoffmann wrote: > > > In case phys bits are functional and can be used by the guest (aka > > > host-phys-bits=on) add a fw_cfg file carrying the value. This can > > > be used by the guest firmware for address space configuration. > > > > > > This is only enabled for 7.2+ machine types for live migration > > > compatibility reasons. > > > > > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > > > > I'm curious why you decided to switch from a cpuid flag to fw cfg. > > The kernel people didn't like the cpuid approach. > > > I guess firmware reads fw cfg anyway. > > Correct. > > > But would the guest kernel then need to load a fw cfg driver very > > early to detect this, too? > > Nope, the guest kernel can just work with the address space layout > created by the firmware. The firmware can for example reserve a > larger 64-bit mmio window in case there is enough address space for > that. So it programs the bridge windows etc accordingly, qemu > generates matching acpi tables and the kernel picks up the changes > via _CRS. > > > > +void fw_cfg_phys_bits(FWCfgState *fw_cfg) > > > +{ > > > + X86CPU *cpu = X86_CPU(first_cpu); > > > + uint64_t phys_bits = cpu->phys_bits; > > > + > > > + if (cpu->host_phys_bits) { > > > + fw_cfg_add_file(fw_cfg, "etc/phys-bits", > > > + g_memdup2(&phys_bits, sizeof(phys_bits)), > > > + sizeof(phys_bits)); > > > + } > > > +} > > > > So, this allows a lot of flexibility, any phys_bits value at all can now > > be used. Do you expect a use-case for such a flexible mechanism? If > > this ends up merely repeating CPUID at all times then we are just > > creating confusion. > > Yes, it'll just repeat CPUID. Advantage is that the guest gets the > information it needs right away. > > Alternatively I could create a "etc/reliable-phys-bits" bool. > The firmware must consult both fw_cfg and cpuid then. > > take care, > Gerd It might not be too bad if we actually allow these two to be different theoretically (even if unused for now). This is up to you. But my point is, if we do let's document what is the expected behaviour if fw cfg and CPUID differ.
On Thu, Sep 22, 2022 at 10:43:56AM +0200, Gerd Hoffmann wrote: > In case phys bits are functional and can be used by the guest (aka > host-phys-bits=on) add a fw_cfg file carrying the value. This can > be used by the guest firmware for address space configuration. > > This is only enabled for 7.2+ machine types for live migration > compatibility reasons. > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> The patch looks good to me. A comment explaining what is going on as I posted elsewhere in this thread can't hurt. Besides that Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > --- > hw/i386/fw_cfg.h | 1 + > include/hw/i386/pc.h | 1 + > hw/i386/fw_cfg.c | 12 ++++++++++++ > hw/i386/pc.c | 5 +++++ > hw/i386/pc_piix.c | 2 ++ > hw/i386/pc_q35.c | 2 ++ > 6 files changed, 23 insertions(+) > > diff --git a/hw/i386/fw_cfg.h b/hw/i386/fw_cfg.h > index 275f15c1c5e8..6ff198a6cb85 100644 > --- a/hw/i386/fw_cfg.h > +++ b/hw/i386/fw_cfg.h > @@ -26,5 +26,6 @@ FWCfgState *fw_cfg_arch_create(MachineState *ms, > void fw_cfg_build_smbios(MachineState *ms, FWCfgState *fw_cfg); > void fw_cfg_build_feature_control(MachineState *ms, FWCfgState *fw_cfg); > void fw_cfg_add_acpi_dsdt(Aml *scope, FWCfgState *fw_cfg); > +void fw_cfg_phys_bits(FWCfgState *fw_cfg); > > #endif > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > index c95333514ed3..bedef1ee13c1 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 phys_bits_in_fw_cfg; > > /* generate legacy CPU hotplug AML */ > bool legacy_cpu_hotplug; > diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c > index a283785a8de4..6a1f18925725 100644 > --- a/hw/i386/fw_cfg.c > +++ b/hw/i386/fw_cfg.c > @@ -219,3 +219,15 @@ void fw_cfg_add_acpi_dsdt(Aml *scope, FWCfgState *fw_cfg) > aml_append(dev, aml_name_decl("_CRS", crs)); > aml_append(scope, dev); > } > + > +void fw_cfg_phys_bits(FWCfgState *fw_cfg) > +{ > + X86CPU *cpu = X86_CPU(first_cpu); > + uint64_t phys_bits = cpu->phys_bits; > + > + if (cpu->host_phys_bits) { > + fw_cfg_add_file(fw_cfg, "etc/phys-bits", > + g_memdup2(&phys_bits, sizeof(phys_bits)), > + sizeof(phys_bits)); > + } > +} > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 566accf7e60a..17ecc7fe4331 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -744,6 +744,7 @@ void pc_machine_done(Notifier *notifier, void *data) > { > PCMachineState *pcms = container_of(notifier, > PCMachineState, machine_done); > + PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms); > X86MachineState *x86ms = X86_MACHINE(pcms); > > cxl_hook_up_pxb_registers(pcms->bus, &pcms->cxl_devices_state, > @@ -764,6 +765,9 @@ void pc_machine_done(Notifier *notifier, void *data) > fw_cfg_build_feature_control(MACHINE(pcms), x86ms->fw_cfg); > /* update FW_CFG_NB_CPUS to account for -device added CPUs */ > fw_cfg_modify_i16(x86ms->fw_cfg, FW_CFG_NB_CPUS, x86ms->boot_cpus); > + if (pcmc->phys_bits_in_fw_cfg) { > + fw_cfg_phys_bits(x86ms->fw_cfg); > + } > } > } > > @@ -1907,6 +1911,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data) > pcmc->kvmclock_enabled = true; > pcmc->enforce_aligned_dimm = true; > pcmc->enforce_amd_1tb_hole = true; > + pcmc->phys_bits_in_fw_cfg = 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 8043a250adf3..c6a4dbd5c0b0 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -447,9 +447,11 @@ DEFINE_I440FX_MACHINE(v7_2, "pc-i440fx-7.2", NULL, > > static void pc_i440fx_7_1_machine_options(MachineClass *m) > { > + PCMachineClass *pcmc = PC_MACHINE_CLASS(m); > pc_i440fx_7_2_machine_options(m); > m->alias = NULL; > m->is_default = false; > + pcmc->phys_bits_in_fw_cfg = false; > compat_props_add(m->compat_props, hw_compat_7_1, hw_compat_7_1_len); > compat_props_add(m->compat_props, pc_compat_7_1, pc_compat_7_1_len); > } > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > index 53eda50e818c..c2b56daa1550 100644 > --- a/hw/i386/pc_q35.c > +++ b/hw/i386/pc_q35.c > @@ -384,8 +384,10 @@ DEFINE_Q35_MACHINE(v7_2, "pc-q35-7.2", NULL, > > static void pc_q35_7_1_machine_options(MachineClass *m) > { > + PCMachineClass *pcmc = PC_MACHINE_CLASS(m); > pc_q35_7_2_machine_options(m); > m->alias = NULL; > + pcmc->phys_bits_in_fw_cfg = false; > compat_props_add(m->compat_props, hw_compat_7_1, hw_compat_7_1_len); > compat_props_add(m->compat_props, pc_compat_7_1, pc_compat_7_1_len); > } > -- > 2.37.3
diff --git a/hw/i386/fw_cfg.h b/hw/i386/fw_cfg.h index 275f15c1c5e8..6ff198a6cb85 100644 --- a/hw/i386/fw_cfg.h +++ b/hw/i386/fw_cfg.h @@ -26,5 +26,6 @@ FWCfgState *fw_cfg_arch_create(MachineState *ms, void fw_cfg_build_smbios(MachineState *ms, FWCfgState *fw_cfg); void fw_cfg_build_feature_control(MachineState *ms, FWCfgState *fw_cfg); void fw_cfg_add_acpi_dsdt(Aml *scope, FWCfgState *fw_cfg); +void fw_cfg_phys_bits(FWCfgState *fw_cfg); #endif diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index c95333514ed3..bedef1ee13c1 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 phys_bits_in_fw_cfg; /* generate legacy CPU hotplug AML */ bool legacy_cpu_hotplug; diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c index a283785a8de4..6a1f18925725 100644 --- a/hw/i386/fw_cfg.c +++ b/hw/i386/fw_cfg.c @@ -219,3 +219,15 @@ void fw_cfg_add_acpi_dsdt(Aml *scope, FWCfgState *fw_cfg) aml_append(dev, aml_name_decl("_CRS", crs)); aml_append(scope, dev); } + +void fw_cfg_phys_bits(FWCfgState *fw_cfg) +{ + X86CPU *cpu = X86_CPU(first_cpu); + uint64_t phys_bits = cpu->phys_bits; + + if (cpu->host_phys_bits) { + fw_cfg_add_file(fw_cfg, "etc/phys-bits", + g_memdup2(&phys_bits, sizeof(phys_bits)), + sizeof(phys_bits)); + } +} diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 566accf7e60a..17ecc7fe4331 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -744,6 +744,7 @@ void pc_machine_done(Notifier *notifier, void *data) { PCMachineState *pcms = container_of(notifier, PCMachineState, machine_done); + PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms); X86MachineState *x86ms = X86_MACHINE(pcms); cxl_hook_up_pxb_registers(pcms->bus, &pcms->cxl_devices_state, @@ -764,6 +765,9 @@ void pc_machine_done(Notifier *notifier, void *data) fw_cfg_build_feature_control(MACHINE(pcms), x86ms->fw_cfg); /* update FW_CFG_NB_CPUS to account for -device added CPUs */ fw_cfg_modify_i16(x86ms->fw_cfg, FW_CFG_NB_CPUS, x86ms->boot_cpus); + if (pcmc->phys_bits_in_fw_cfg) { + fw_cfg_phys_bits(x86ms->fw_cfg); + } } } @@ -1907,6 +1911,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data) pcmc->kvmclock_enabled = true; pcmc->enforce_aligned_dimm = true; pcmc->enforce_amd_1tb_hole = true; + pcmc->phys_bits_in_fw_cfg = 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 8043a250adf3..c6a4dbd5c0b0 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -447,9 +447,11 @@ DEFINE_I440FX_MACHINE(v7_2, "pc-i440fx-7.2", NULL, static void pc_i440fx_7_1_machine_options(MachineClass *m) { + PCMachineClass *pcmc = PC_MACHINE_CLASS(m); pc_i440fx_7_2_machine_options(m); m->alias = NULL; m->is_default = false; + pcmc->phys_bits_in_fw_cfg = false; compat_props_add(m->compat_props, hw_compat_7_1, hw_compat_7_1_len); compat_props_add(m->compat_props, pc_compat_7_1, pc_compat_7_1_len); } diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index 53eda50e818c..c2b56daa1550 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -384,8 +384,10 @@ DEFINE_Q35_MACHINE(v7_2, "pc-q35-7.2", NULL, static void pc_q35_7_1_machine_options(MachineClass *m) { + PCMachineClass *pcmc = PC_MACHINE_CLASS(m); pc_q35_7_2_machine_options(m); m->alias = NULL; + pcmc->phys_bits_in_fw_cfg = false; compat_props_add(m->compat_props, hw_compat_7_1, hw_compat_7_1_len); compat_props_add(m->compat_props, pc_compat_7_1, pc_compat_7_1_len); }
In case phys bits are functional and can be used by the guest (aka host-phys-bits=on) add a fw_cfg file carrying the value. This can be used by the guest firmware for address space configuration. This is only enabled for 7.2+ machine types for live migration compatibility reasons. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/i386/fw_cfg.h | 1 + include/hw/i386/pc.h | 1 + hw/i386/fw_cfg.c | 12 ++++++++++++ hw/i386/pc.c | 5 +++++ hw/i386/pc_piix.c | 2 ++ hw/i386/pc_q35.c | 2 ++ 6 files changed, 23 insertions(+)