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

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

My concern (maybe not important), is that acpi_firmware="/path/to/non-SLIC/table" will be converted to SLIC entry in libvirt xml.