[libvirt] [PATCH 1/2] libxl: add acpi slic table support

[Jim Fehlig]

On 9/11/19 10:33 AM, Marek Marczykowski-Górecki  wrote:
> On Wed, Sep 11, 2019 at 01:31:34PM +0000, Jim Fehlig wrote:
>> On 9/11/19 5:43 AM, Marek Marczykowski-Górecki  wrote:
>>> On Wed, Sep 11, 2019 at 02:34:57AM +0000, Jim Fehlig wrote:
>>>> On 9/10/19 5:24 PM, Marek Marczykowski-Górecki  wrote:
>>>>> On Tue, Sep 10, 2019 at 10:54:15PM +0000, Jim Fehlig wrote:
>>>>>> On 9/6/19 8:31 PM, Marek Marczykowski-Górecki  wrote:
>>>>>>> From: Ivan Kardykov <kardykov at tabit.pro>
>>>>>>>
>>>>>>> Libxl driver did not support setup additional acpi firmware to xen
>>>>>>> guest. It is necessary to activate OEM Windows installs. This patch
>>>>>>> allow to define in OS section acpi table param (which supported domain
>>>>>>> common schema).
>>>>>>>
>>>>>>> Signed-off-by: Ivan Kardykov <kardykov at tabit.pro>
>>>>>>> [added info to docs/formatdomain.html.in]
>>>>>>> Signed-off-by: Marek Marczykowski-Górecki <marmarek at invisiblethingslab.com>
>>>>>>> ---
>>>>>>>      docs/formatdomain.html.in | 3 ++-
>>>>>>>      src/libxl/libxl_conf.c    | 5 +++++
>>>>>>>      2 files changed, 7 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>>>>>>> index fcb7c59c00..de612ae870 100644
>>>>>>> --- a/docs/formatdomain.html.in
>>>>>>> +++ b/docs/formatdomain.html.in
>>>>>>> @@ -363,7 +363,8 @@
>>>>>>>            <dd>The <code>table</code> element contains a fully-qualified path
>>>>>>>              to the ACPI table. The <code>type</code> attribute contains the
>>>>>>>              ACPI table type (currently only <code>slic</code> is supported)
>>>>>>> -        <span class="since">Since 1.3.5 (QEMU only)</span></dd>
>>>>>>> +        <span class="since">Since 1.3.5 (QEM)</span>
>>>>>>
>>>>>> You removed one too many characters :-). s/QEM/QEMU/
>>>>>>
>>>>>>> +        <span class="since">Since 5.8.0 (Xen)</span></dd>
>>>>>>>          </dl>
>>>>>>>      
>>>>>>>          <h4><a id="elementsOSContainer">Container boot</a></h4>
>>>>>>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>>>>>>> index 766a726ebc..c1e248d98c 100644
>>>>>>> --- a/src/libxl/libxl_conf.c
>>>>>>> +++ b/src/libxl/libxl_conf.c
>>>>>>> @@ -506,6 +506,11 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
>>>>>>>                                def->features[VIR_DOMAIN_FEATURE_ACPI] ==
>>>>>>>                                VIR_TRISTATE_SWITCH_ON);
>>>>>>>      
>>>>>>> +        /* copy SLIC table path to acpi_firmware */
>>>>>>> +        if (def->os.slic_table &&
>>>>>>> +                VIR_STRDUP(b_info->u.hvm.acpi_firmware, def->os.slic_table) < 0)
>>>>>>> +            return -1;
>>>>>>> +
>>>>>>
>>>>>> Is 'acpi_firmware=' the xl.cfg equivalent setting? If so we'll want it added to
>>>>>> the domXML<->xl.cfg converter (which now lives in the src/libxl/ directory).
>>>>>
>>>>> Functionally yes. But acpi_firmware= is about generic ACPI table, not
>>>>> only SLIC. This means xl.cfg acpi_firmware= converted to domXML may be
>>>>> misleading. Is it a problem?
>>>>
>>>> I don't think it's a problem. But let me ask another way: How would you specify
>>>> the SLIC in xl.cfg? I.e., what would a comparable xl.cfg snippet look like?
>>>
>>> acpi_firmware="/sys/firmware/acpi/tables/SLIC"
>>>
>>> (for those brave enough ;) )
>>
>> :-)
>>
>> Ok, so there is no setting to specify the type of ACPI firmware.
>>
>>> My concern (maybe not important), is that
>>> acpi_firmware="/path/to/non-SLIC/table" will be converted to SLIC entry
>>> in libvirt xml.
>>
>> Are there other tables in use? Or was this added to libxl primarily for the
>> license table? We are probably fine to imply SLIC if that is all there is ATM.
>> We can add support for other tables as those become prevalent.
> 
> I'm not sure what is the primary purpose of this option, I've never
> needed it for anything else. But it can point a file with multiple ACPI
> tables concatenated. From man page:
> 
>      acpi_firmware="STRING"
>          Specify a path to a file that contains extra ACPI firmware tables to
>          pass in to a guest. The file can contain several tables in their binary
>          AML form concatenated together. Each table self describes its length so
>          no additional information is needed. These tables will be added to the
>          ACPI table set in the guest. Note that existing tables cannot be
>          overridden by this feature. For example this cannot be used to override
>          tables like DSDT, FADT, etc.

I suspect SLIC was the motivation for this option, and I think we are safe to 
only support it in the converter since the libvirt documentation already states 
that SLIC is the only supported type.

Regards,
Jim