Re: [Qemu-devel] [PATCH 4/4] pc: set the OEM fields in the RSDT and the FADT from the SLIC

[Laszlo Ersek <lersek@redhat.com>]

On 01/14/16 17:44, Laszlo Ersek wrote:
> On 01/14/16 11:24, Michael S. Tsirkin wrote:
>> On Thu, Jan 14, 2016 at 02:36:57AM +0100, Laszlo Ersek wrote:
>>> The Microsoft spec about the SLIC and MSDM ACPI tables at
>>> <http://go.microsoft.com/fwlink/p/?LinkId=234834> requires the OEM ID and
>>> OEM Table ID fields to be consistent between the SLIC and the RSDT/XSDT.
>>> That further affects the FADT, because a similar match between the FADT
>>> and the RSDT/XSDT is required by the ACPI spec in general.
>>>
>>> The stashed SLIC OEM identifiers can be ignored with the new
>>>
>>>   -machine heed-slic-oem=no
>>>
>>> option.
>>
>> I'd prefer "use-slic-oem".
> 
> That is, an option with the opposite meaning. Sounds doable.
> 
> Would you like me to give that option a default value that had the
> equivalent effect? (That is, use-slic-oem=on by default?) If not, that
> might cause further trouble for users (they would have to figure out one
> more option).

Ugh, nevermind. You suggested another option name, but with the *same*
meaning. Okay, that's really easy to change.

But the more foundational question below remains open.

Thanks
Laszlo

>> But do we really expect people to use this?
>> Less knobs would be better ...
> 
> I don't disagree; I only did this because you had suggested it in the
> earlier discussion:
> 
> http://thread.gmane.org/gmane.comp.emulators.qemu/358854/focus=359239
> 
> Perhaps I misunderstood. FWIW, the SLIC spec from Microsoft is
> unambiguous about the OEM identity between SLIC and RSDT/XSDT (and
> consequently the FADT): that requirement is not optional. Turning off
> the OEM matching is therefore arguably useless, if the user provides a SLIC.
> 
> I can certainly drop this knob if you think that's best.
> 
> Thanks!
> Laszlo
> 
>>
>>> Cc: "Michael S. Tsirkin" <mst@redhat.com> (supporter:ACPI/SMBIOS)
>>> Cc: Igor Mammedov <imammedo@redhat.com> (supporter:ACPI/SMBIOS)
>>> Cc: Paolo Bonzini <pbonzini@redhat.com> (maintainer:X86)
>>> Cc: Richard W.M. Jones <rjones@redhat.com>
>>> Cc: Aleksei Kovura <alex3kov@zoho.com>
>>> Cc: Michael Tokarev <mjt@tls.msk.ru>
>>> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1248758
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>> ---
>>>  include/hw/i386/pc.h |  2 ++
>>>  hw/i386/acpi-build.c | 22 ++++++++++++++++++----
>>>  hw/i386/pc.c         | 19 +++++++++++++++++++
>>>  qemu-options.hx      | 10 +++++++++-
>>>  4 files changed, 48 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>>> index 588a33c..a762c29 100644
>>> --- a/include/hw/i386/pc.h
>>> +++ b/include/hw/i386/pc.h
>>> @@ -56,6 +56,7 @@ struct PCMachineState {
>>>      OnOffAuto vmport;
>>>      OnOffAuto smm;
>>>      bool nvdimm;
>>> +    bool heed_slic_oem;
>>>  
>>>      /* RAM information (sizes, addresses, configuration): */
>>>      ram_addr_t below_4g_mem_size, above_4g_mem_size;
>>> @@ -67,6 +68,7 @@ struct PCMachineState {
>>>  #define PC_MACHINE_VMPORT           "vmport"
>>>  #define PC_MACHINE_SMM              "smm"
>>>  #define PC_MACHINE_NVDIMM           "nvdimm"
>>> +#define PC_MACHINE_HEED_SLIC_OEM    "heed-slic-oem"
>>>  
>>>  /**
>>>   * PCMachineClass:
>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>>> index 6408362..cf2aafc 100644
>>> --- a/hw/i386/acpi-build.c
>>> +++ b/hw/i386/acpi-build.c
>>> @@ -337,7 +337,8 @@ static void fadt_setup(AcpiFadtDescriptorRev1 *fadt, AcpiPmInfo *pm)
>>>  /* FADT */
>>>  static void
>>>  build_fadt(GArray *table_data, GArray *linker, AcpiPmInfo *pm,
>>> -           unsigned facs, unsigned dsdt)
>>> +           unsigned facs, unsigned dsdt,
>>> +           const char *oem_id, const char *oem_table_id)
>>>  {
>>>      AcpiFadtDescriptorRev1 *fadt = acpi_data_push(table_data, sizeof(*fadt));
>>>  
>>> @@ -358,7 +359,7 @@ build_fadt(GArray *table_data, GArray *linker, AcpiPmInfo *pm,
>>>      fadt_setup(fadt, pm);
>>>  
>>>      build_header(linker, table_data,
>>> -                 (void *)fadt, "FACP", sizeof(*fadt), 1, NULL, NULL);
>>> +                 (void *)fadt, "FACP", sizeof(*fadt), 1, oem_id, oem_table_id);
>>>  }
>>>  
>>>  static void
>>> @@ -2621,6 +2622,17 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
>>>      uint8_t *u;
>>>      size_t aml_len = 0;
>>>      GArray *tables_blob = tables->table_data;
>>> +    char *slic_oem_id = NULL;
>>> +    char *slic_oem_table_id = NULL;
>>> +    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
>>> +    bool heed_slic_oem = object_property_get_bool(OBJECT(pcms),
>>> +                                                  PC_MACHINE_HEED_SLIC_OEM,
>>> +                                                  &error_abort);
>>> +
>>> +    if (heed_slic_oem) {
>>> +        slic_oem_id = acpi_slic_oem_id;
>>> +        slic_oem_table_id = acpi_slic_oem_table_id;
>>> +    }
>>>  
>>>      acpi_get_cpu_info(&cpu);
>>>      acpi_get_pm_info(&pm);
>>> @@ -2654,7 +2666,8 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
>>>  
>>>      /* ACPI tables pointed to by RSDT */
>>>      acpi_add_table(table_offsets, tables_blob);
>>> -    build_fadt(tables_blob, tables->linker, &pm, facs, dsdt);
>>> +    build_fadt(tables_blob, tables->linker, &pm, facs, dsdt,
>>> +               slic_oem_id, slic_oem_table_id);
>>>  
>>>      ssdt = tables_blob->len;
>>>      acpi_add_table(table_offsets, tables_blob);
>>> @@ -2705,7 +2718,8 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
>>>  
>>>      /* RSDT is pointed to by RSDP */
>>>      rsdt = tables_blob->len;
>>> -    build_rsdt(tables_blob, tables->linker, table_offsets, NULL, NULL);
>>> +    build_rsdt(tables_blob, tables->linker, table_offsets,
>>> +               slic_oem_id, slic_oem_table_id);
>>>  
>>>      /* RSDP is in FSEG memory, so allocate it separately */
>>>      build_rsdp(tables->rsdp, tables->linker, rsdt);
>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>> index c36b8cf..3e7a72a 100644
>>> --- a/hw/i386/pc.c
>>> +++ b/hw/i386/pc.c
>>> @@ -1887,6 +1887,20 @@ static void pc_machine_set_nvdimm(Object *obj, bool value, Error **errp)
>>>      pcms->nvdimm = value;
>>>  }
>>>  
>>> +static bool pc_machine_get_heed_slic_oem(Object *obj, Error **errp)
>>> +{
>>> +    PCMachineState *pcms = PC_MACHINE(obj);
>>> +
>>> +    return pcms->heed_slic_oem;
>>> +}
>>> +
>>> +static void pc_machine_set_heed_slic_oem(Object *obj, bool value, Error **errp)
>>> +{
>>> +    PCMachineState *pcms = PC_MACHINE(obj);
>>> +
>>> +    pcms->heed_slic_oem = value;
>>> +}
>>> +
>>>  static void pc_machine_initfn(Object *obj)
>>>  {
>>>      PCMachineState *pcms = PC_MACHINE(obj);
>>> @@ -1926,6 +1940,11 @@ static void pc_machine_initfn(Object *obj)
>>>      pcms->nvdimm = false;
>>>      object_property_add_bool(obj, PC_MACHINE_NVDIMM, pc_machine_get_nvdimm,
>>>                               pc_machine_set_nvdimm, &error_abort);
>>> +
>>> +    pcms->heed_slic_oem = true;
>>> +    object_property_add_bool(obj, PC_MACHINE_HEED_SLIC_OEM,
>>> +                             pc_machine_get_heed_slic_oem,
>>> +                             pc_machine_set_heed_slic_oem, &error_abort);
>>>  }
>>>  
>>>  static void pc_machine_reset(void)
>>> diff --git a/qemu-options.hx b/qemu-options.hx
>>> index 215d00d..e49964c 100644
>>> --- a/qemu-options.hx
>>> +++ b/qemu-options.hx
>>> @@ -43,7 +43,8 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
>>>      "                aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n"
>>>      "                dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n"
>>>      "                suppress-vmdesc=on|off disables self-describing migration (default=off)\n"
>>> -    "                nvdimm=on|off controls NVDIMM support (default=off)\n",
>>> +    "                nvdimm=on|off controls NVDIMM support (default=off)\n"
>>> +    "                heed_slic_oem=on|off adapts RSDT and FADT OEM identifiers to external SLIC (default=on)\n",
>>>      QEMU_ARCH_ALL)
>>>  STEXI
>>>  @item -machine [type=]@var{name}[,prop=@var{value}[,...]]
>>> @@ -84,6 +85,13 @@ controls whether DEA wrapping keys will be created to allow
>>>  execution of DEA cryptographic functions.  The default is on.
>>>  @item nvdimm=on|off
>>>  Enables or disables NVDIMM support. The default is off.
>>> +@item heed_slic_oem=on|off
>>> +If the user provides an external SLIC ACPI table with the -acpitable option,
>>> +then heed_slic_oem=on will adapt the OEM ID and OEM Table ID fields of the
>>> +auto-generated RSDT and FADT tables to the same fields in the external SLIC.
>>> +When heed_slic_oem is turned off, the RSDT and FADT tables will have general,
>>> +QEMU-branded OEM ID and OEM Table ID values. The default is on. heed_slic_oem
>>> +makes no difference if no SLIC table is provided by the user.
>>>  @end table
>>>  ETEXI
>>>  
>>> -- 
>>> 1.8.3.1
> 
>