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

[Michael S. Tsirkin]

On Thu, Jan 14, 2016 at 02:36:57AM +0100, Laszlo Ersek wrote:
> The Microsoft spec about the SLIC and MSDM ACPI tables at
>  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".

But do we really expect people to use this?
Less knobs would be better ...

> Cc: "Michael S. Tsirkin"  (supporter:ACPI/SMBIOS)
> Cc: Igor Mammedov  (supporter:ACPI/SMBIOS)
> Cc: Paolo Bonzini  (maintainer:X86)
> Cc: Richard W.M. Jones 
> Cc: Aleksei Kovura 
> Cc: Michael Tokarev 
> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1248758
> Signed-off-by: Laszlo Ersek 
> ---
>  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

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

[Laszlo Ersek]

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
>>  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).

> 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"  (supporter:ACPI/SMBIOS)
>> Cc: Igor Mammedov  (supporter:ACPI/SMBIOS)
>> Cc: Paolo Bonzini  (maintainer:X86)
>> Cc: Richard W.M. Jones 
>> Cc: Aleksei Kovura 
>> Cc: Michael Tokarev 
>> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1248758
>> Signed-off-by: Laszlo Ersek 
>> ---
>>  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, 
>> AcpiB

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

[Laszlo Ersek]

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
>>>  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"  (supporter:ACPI/SMBIOS)
>>> Cc: Igor Mammedov  (supporter:ACPI/SMBIOS)
>>> Cc: Paolo Bonzini  (maintainer:X86)
>>> Cc: Richard W.M. Jones 
>>> Cc: Aleksei Kovura 
>>> Cc: Michael Tokarev 
>>> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1248758
>>> Signed-off-by: Laszlo Ersek 
>>> ---
>>>  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(t