Re: [Qemu-devel] [PATCH] pc: acpi: force FADT rev1 for old i440fx machine types

[Igor Mammedov]

On Sat, 22 Jul 2017 02:40:46 +0300
"Michael S. Tsirkin" <address@hidden> wrote:

> On Fri, Jul 21, 2017 at 12:10:48PM +0200, Igor Mammedov wrote:
> > On Fri, 21 Jul 2017 10:49:55 +0100
> > "Daniel P. Berrange" <address@hidden> wrote:
> >   
> > > On Fri, Jul 21, 2017 at 11:32:11AM +0200, Igor Mammedov wrote:  
> > > > w2k used to boot on QEMU until we bumped revision of FADT to rev3
> > > > (commit 77af8a2b hw/i386: Use Rev3 FADT (ACPI 2.0) instead of Rev1 to 
> > > > improve guest OS support.)
> > > > 
> > > > Considering that w2k is ancient and long time EOLed, leave default
> > > > rev3 but make pc-i440fx-2.9 and older machine types to force rev1
> > > > so old setups won't break (w2k could boot).    
> > > 
> > > There needs to be a machine type property added to control this
> > > feature. When provisioning new VMs, management apps need to be
> > > able to set the property explicitly - having them rely on picking
> > > particular machine type name+versions is not viable, because
> > > downstream vendors replace the machine types with their own
> > > names + versions.  
> > having property doesn't really help here and we don't do it for every
> > compat tweak /ex: save_tsc_khz, linuxboot_dma_enabled/.
> > 
> > Management would not benefit much from having property vs machine version
> > as it would have to encode somewhere that for w2k it should set
> > some machine property or pick a particular machine type.  
> 
> I think I'd disagree with that. If
> users might need this for compatibility with some guests,
> then it should be a property not just a machine type.
> 
> But see below - I think we rushed it for the PC anyway.
> 
> > Probably no one would worry about fixing virt-install or something
> > else for the sake of w2k and if they are going to fix it
> > it doesn't matter if they map machine type vs property.
> > 
> > Also with new machine type deprecation policy we would be able
> > easily to phase out rev1 support along with 2.9 machine,
> > but if you expose property then removing it would break
> > CLI not only for 2.9 but possible later machines if it's set there.
> > 
> > So I'm against adding properties/CLI options for unless we have to in this 
> > case,
> > and I'm not convinced that w2k deserves it.  
> 
> If I have to choose, I'd say Mac OSX is way less interesting than old
> windows versions. Lots of people have software that will only run on old
> windows and there's probably good money to be had running it on new
> hardware in VMs. And PC machine is all about compatibility - we have Q35
> for new stuff.  Besides OSX uses q35 anyway I think.
Question is for how long we are going to maintain legacy stuff,
ACPI spec periodically adds new stuff, which someday is going
to break legacy OSes. And maintaining 2 branches of or worse
a mix will cost us in time and future regressions, we need to
have some policy to cut off legacy features that hold us back,
like we are starting to do with machine types.
w2k has been EOLed in 2010 even if we drop its support now,
users still can use it as they have an option to use old QEMU
for that.
(Ladi said that w2k fails to install since 2.7).

> 
> So maybe the right thing to do is to
> - switch default for PC back to rev 1
> - keep default for Q35 at rev 3
> 
> No machinetype hacks.
it's still machine hack pc vs q35 and an extra branch to look
after but it's better than an option, I'll respin patch.

> 
> > > > 
> > > > Signed-off-by: Igor Mammedov <address@hidden>
> > > > ---
> > > > CC: Programmingkid <address@hidden>
> > > > CC: Phil Dennis-Jordan <address@hidden>
> > > > CC: "Michael S. Tsirkin" <address@hidden>
> > > > 
> > > > Only compile test since I don't have w2k to test with
> > > > 
> > > > ---
> > > >  include/hw/i386/pc.h |  1 +
> > > >  hw/i386/acpi-build.c | 26 +++++++++++++++++++-------
> > > >  hw/i386/pc_piix.c    |  2 ++
> > > >  3 files changed, 22 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > > index d80859b..d6f65dd 100644
> > > > --- a/include/hw/i386/pc.h
> > > > +++ b/include/hw/i386/pc.h
> > > > @@ -122,6 +122,7 @@ struct PCMachineClass {
> > > >      bool rsdp_in_ram;
> > > >      int legacy_acpi_table_size;
> > > >      unsigned acpi_data_size;
> > > > +    bool force_rev1_fadt;
> > > >  
> > > >      /* SMBIOS compat: */
> > > >      bool smbios_defaults;
> > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > index 6b7bade..227f9ad 100644
> > > > --- a/hw/i386/acpi-build.c
> > > > +++ b/hw/i386/acpi-build.c
> > > > @@ -272,7 +272,7 @@ build_facs(GArray *table_data, BIOSLinker *linker)
> > > >  }
> > > >  
> > > >  /* Load chipset information in FADT */
> > > > -static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiPmInfo *pm)
> > > > +static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiPmInfo *pm, 
> > > > bool rev1)
> > > >  {
> > > >      fadt->model = 1;
> > > >      fadt->reserved1 = 0;
> > > > @@ -304,6 +304,9 @@ static void fadt_setup(AcpiFadtDescriptorRev3 
> > > > *fadt, AcpiPmInfo *pm)
> > > >          fadt->flags |= cpu_to_le32(1 << 
> > > > ACPI_FADT_F_FORCE_APIC_CLUSTER_MODEL);
> > > >      }
> > > >      fadt->century = RTC_CENTURY;
> > > > +    if (rev1) {
> > > > +        return;
> > > > +    }
> > > >  
> > > >      fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_RESET_REG_SUP);
> > > >      fadt->reset_value = 0xf;
> > > > @@ -335,6 +338,7 @@ static void fadt_setup(AcpiFadtDescriptorRev3 
> > > > *fadt, AcpiPmInfo *pm)
> > > >  /* FADT */
> > > >  static void
> > > >  build_fadt(GArray *table_data, BIOSLinker *linker, AcpiPmInfo *pm,
> > > > +           MachineState *machine,
> > > >             unsigned facs_tbl_offset, unsigned dsdt_tbl_offset,
> > > >             const char *oem_id, const char *oem_table_id)
> > > >  {
> > > > @@ -342,6 +346,9 @@ build_fadt(GArray *table_data, BIOSLinker *linker, 
> > > > AcpiPmInfo *pm,
> > > >      unsigned fw_ctrl_offset = (char *)&fadt->firmware_ctrl - 
> > > > table_data->data;
> > > >      unsigned dsdt_entry_offset = (char *)&fadt->dsdt - 
> > > > table_data->data;
> > > >      unsigned xdsdt_entry_offset = (char *)&fadt->x_dsdt - 
> > > > table_data->data;
> > > > +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
> > > > +    int fadt_size = sizeof(*fadt);
> > > > +    int rev = 3;
> > > >  
> > > >      /* FACS address to be filled by Guest linker */
> > > >      bios_linker_loader_add_pointer(linker,
> > > > @@ -349,16 +356,21 @@ build_fadt(GArray *table_data, BIOSLinker 
> > > > *linker, AcpiPmInfo *pm,
> > > >          ACPI_BUILD_TABLE_FILE, facs_tbl_offset);
> > > >  
> > > >      /* DSDT address to be filled by Guest linker */
> > > > -    fadt_setup(fadt, pm);
> > > > +    fadt_setup(fadt, pm, pcmc->force_rev1_fadt);
> > > >      bios_linker_loader_add_pointer(linker,
> > > >          ACPI_BUILD_TABLE_FILE, dsdt_entry_offset, sizeof(fadt->dsdt),
> > > >          ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);
> > > > -    bios_linker_loader_add_pointer(linker,
> > > > -        ACPI_BUILD_TABLE_FILE, xdsdt_entry_offset, 
> > > > sizeof(fadt->x_dsdt),
> > > > -        ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);
> > > > +    if (pcmc->force_rev1_fadt) {
> > > > +        rev = 1;
> > > > +        fadt_size = offsetof(typeof(*fadt), reset_register);
> > > > +    } else {
> > > > +        bios_linker_loader_add_pointer(linker,
> > > > +            ACPI_BUILD_TABLE_FILE, xdsdt_entry_offset, 
> > > > sizeof(fadt->x_dsdt),
> > > > +            ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);
> > > > +    }
> > > >  
> > > >      build_header(linker, table_data,
> > > > -                 (void *)fadt, "FACP", sizeof(*fadt), 3, oem_id, 
> > > > oem_table_id);
> > > > +                 (void *)fadt, "FACP", fadt_size, rev, oem_id, 
> > > > oem_table_id);
> > > >  }
> > > >  
> > > >  void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
> > > > @@ -2667,7 +2679,7 @@ void acpi_build(AcpiBuildTables *tables, 
> > > > MachineState *machine)
> > > >      /* ACPI tables pointed to by RSDT */
> > > >      fadt = tables_blob->len;
> > > >      acpi_add_table(table_offsets, tables_blob);
> > > > -    build_fadt(tables_blob, tables->linker, &pm, facs, dsdt,
> > > > +    build_fadt(tables_blob, tables->linker, &pm, machine, facs, dsdt,
> > > >                 slic_oem.id, slic_oem.table_id);
> > > >      aml_len += tables_blob->len - fadt;
> > > >  
> > > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > > > index 11b4336..bc61332 100644
> > > > --- a/hw/i386/pc_piix.c
> > > > +++ b/hw/i386/pc_piix.c
> > > > @@ -449,9 +449,11 @@ DEFINE_I440FX_MACHINE(v2_10, "pc-i440fx-2.10", 
> > > > NULL,
> > > >  
> > > >  static void pc_i440fx_2_9_machine_options(MachineClass *m)
> > > >  {
> > > > +    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
> > > >      pc_i440fx_2_10_machine_options(m);
> > > >      m->is_default = 0;
> > > >      m->alias = NULL;
> > > > +    pcmc->force_rev1_fadt = true;
> > > >      SET_MACHINE_COMPAT(m, PC_COMPAT_2_9);  
> 
> whatever switch we use for this option, I think it makes
> sense to add comments to document why we keep this around.
ok

> 
> 
> > > >  }
> > > >  
> > > > -- 
> > > > 2.7.4
> > > > 
> > > >     
> > > 
> > > Regards,
> > > Daniel  
>