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

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(a)tabit.pro&gt; >>>>>> >>>>>> 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(a)tabit.pro&gt; >>>>>> [added info to docs/formatdomain.html.in] >>>>>> Signed-off-by: Marek Marczykowski-Górecki <marmarek(a)invisiblethingslab.com&gt; >>>>>> --- >>>>>> 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.