Re: [Qemu-devel] [PATCH v2 for-2.1 2/2] pc: hack for migration compatibility from QEMU 2.0

[Paolo Bonzini <pbonzini@redhat.com>]

Il 28/07/2014 13:45, Michael S. Tsirkin ha scritto:
>> +/* These are used to size the ACPI tables for -M pc-i440fx-1.7 and
>> + * -M pc-i440fx-2.0.
> 
> Let's just say 2.0 and earlier?

This would give the idea that 1.6 is broken, but it isn't.

>>  Even if the actual amount of AML generated grows
>> + * a little bit, there should be plenty of free space since the DSDT
>> + * shrunk by ~1.5k between QEMU 2.0 and QEMU 2.1.
>> + */
>> +#define ACPI_BUILD_CPU_AML_SIZE    97
>> +#define ACPI_BUILD_BRIDGE_AML_SIZE 1875
> 
> Let's put _LEGACY_ somewhere here?

Ok.

>> +
>> +#define ACPI_BUILD_TABLE_SIZE      0x10000
>> +
>>  typedef struct AcpiCpuInfo {
>>      DECLARE_BITMAP(found_cpus, ACPI_CPU_HOTPLUG_ID_LIMIT);
>>  } AcpiCpuInfo;
>> @@ -87,6 +99,8 @@ typedef struct AcpiBuildPciBusHotplugState {
>>      struct AcpiBuildPciBusHotplugState *parent;
>>  } AcpiBuildPciBusHotplugState;
>>  
>> +unsigned bsel_alloc;
>> +
> 
> Patch will be better contained if instead of using a global
> bsel_alloc, we actually go and count the devices that
> have ACPI_PCIHP_PROP_BSEL.
> You can just scan all devices, or all pci devices, it
> should not matter.
> This way, this code will be local to the legacy path.

Ok.

> 
>>  static void acpi_get_dsdt(AcpiMiscInfo *info)
>>  {
>>      uint16_t *applesmc_sta;
>> @@ -759,8 +773,8 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
>>  static void acpi_set_pci_info(void)
>>  {
>>      PCIBus *bus = find_i440fx(); /* TODO: Q35 support */
>> -    unsigned bsel_alloc = 0;
>>  
>> +    assert(bsel_alloc == 0);
>>      if (bus) {
>>          /* Scan all PCI buses. Set property to enable acpi based hotplug. */
>>          pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &bsel_alloc);
>> @@ -1440,13 +1454,14 @@ static
>>  void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
>>  {
>>      GArray *table_offsets;
>> -    unsigned facs, dsdt, rsdt;
>> +    unsigned facs, ssdt, dsdt, rsdt;
>>      AcpiCpuInfo cpu;
>>      AcpiPmInfo pm;
>>      AcpiMiscInfo misc;
>>      AcpiMcfgInfo mcfg;
>>      PcPciInfo pci;
>>      uint8_t *u;
>> +    size_t aml_len = 0;
>>  
>>      acpi_get_cpu_info(&cpu);
>>      acpi_get_pm_info(&pm);
>> @@ -1474,13 +1489,20 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
>>      dsdt = tables->table_data->len;
>>      build_dsdt(tables->table_data, tables->linker, &misc);
>>  
>> +    /* Count the size of the DSDT and SSDT, we will need it for legacy
>> +     * sizing of ACPI tables.
>> +     */
>> +    aml_len += tables->table_data->len - dsdt;
>> +
>>      /* ACPI tables pointed to by RSDT */
>>      acpi_add_table(table_offsets, tables->table_data);
>>      build_fadt(tables->table_data, tables->linker, &pm, facs, dsdt);
>>  
>> +    ssdt = tables->table_data->len;
>>      acpi_add_table(table_offsets, tables->table_data);
>>      build_ssdt(tables->table_data, tables->linker, &cpu, &pm, &misc, &pci,
>>                 guest_info);
>> +    aml_len += tables->table_data->len - ssdt;
>>  
>>      acpi_add_table(table_offsets, tables->table_data);
>>      build_madt(tables->table_data, tables->linker, &cpu, guest_info);
>> @@ -1513,12 +1535,53 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
>>      /* RSDP is in FSEG memory, so allocate it separately */
>>      build_rsdp(tables->rsdp, tables->linker, rsdt);
>>  
>> -    /* We'll expose it all to Guest so align size to reduce
>> +    /* We'll expose it all to Guest so we want to reduce
>>       * chance of size changes.
>>       * RSDP is small so it's easy to keep it immutable, no need to
>>       * bother with alignment.
>> +     *
>> +     * We used to align the tables to 4k, but of course this would
>> +     * too simple to be enough.  4k turned out to be too small an
>> +     * alignment very soon, and in fact it is almost impossible to
>> +     * keep the table size stable for all (max_cpus, max_memory_slots)
>> +     * combinations.  So the table size is always 64k for pc-i440fx-2.1
>> +     * and we give an error if the table grows beyond that limit.
>> +     *
>> +     * We still have the problem of migrating from "-M pc-i440fx-2.0".  For
>> +     * that, we exploit the fact that QEMU 2.1 generates _smaller_ tables
>> +     * than 2.0 and we can always pad the smaller tables with zeros.  We can
>> +     * then use the exact size of the 2.0 tables.
>> +     *
>> +     * All this is for PIIX4, since QEMU 2.0 didn't support Q35 migration.
>>       */
>> -    acpi_align_size(tables->table_data, 0x1000);
>> +    if (guest_info->legacy_acpi_table_size) {
>> +        /* Subtracting aml_len gives the size of fixed tables.  Then add the
>> +         * size of the PIIX4 DSDT/SSDT in QEMU 2.0.
>> +         */
>> +        int legacy_aml_len =
>> +            guest_info->legacy_acpi_table_size +
>> +            ACPI_BUILD_CPU_AML_SIZE * max_cpus +
>> +            ACPI_BUILD_BRIDGE_AML_SIZE * (MAX(bsel_alloc, 1) - 1);
>> +        int legacy_table_size =
>> +            ROUND_UP(tables->table_data->len - aml_len + legacy_aml_len, 0x1000);
>> +        if (tables->table_data->len > legacy_table_size) {
>> +            /* -M pc-i440fx-2.0 doesn't support memory hotplug, so this should
>> +             * never happen.
>> +             */
>> +            error_report("This configuration is not supported with -M pc-i440fx-2.0.");
>> +            error_report("Please report this to qemu-devel@nongnu.org.");
> 
> Downstreams would need to patch this line out? Let's just drop the
> second line.

This should never happen really, so...

>> +            exit(1);
> 
> 
> so what happens here is 2.x -> 2.0 migration becomes broken, but
> 2.0 -> 2.x could still work.

Only with Igor's patches.

> I think I would prefer a warning instead.
> 
>> +        }
>> +        g_array_set_size(tables->table_data, legacy_table_size);
>> +    } else {
>> +        if (tables->table_data->len > ACPI_BUILD_TABLE_SIZE) {
>> +            /* As of QEMU 2.1, this fires with 160 VCPUs and 255 memory slots.  */
>> +            error_report("ACPI tables are larger than 64k.  Please remove");
>> +            error_report("CPUs, NUMA nodes, memory slots or PCI bridges.");
>> +            exit(1);
>> +        }
>> +        g_array_set_size(tables->table_data, ACPI_BUILD_TABLE_SIZE);
> 
> 
> Let's split the idea to use 64K always out, to a separate patch, ok?

Ok.

Paolo