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

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.