On Mon, Jul 24, 2017 at 08:48:48AM +0200, Igor Mammedov wrote:
> On Sat, 22 Jul 2017 02:40:46 +0300
> "Michael S. Tsirkin" <m...@redhat.com> 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" <berra...@redhat.com> 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.
Users can't normally get old QEMU, even if they could it is not
a good idea because of security.
> (Ladi said that w2k fails to install since 2.7).
Bisect will likely find what's wrong.
> >
> > 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 <imamm...@redhat.com>
> > > > > ---
> > > > > CC: Programmingkid <programmingk...@gmail.com>
> > > > > CC: Phil Dennis-Jordan <li...@philjordan.eu>
> > > > > CC: "Michael S. Tsirkin" <m...@redhat.com>
> > > > >
> > > > > 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
> >
