[libvirt] [PATCH] libxl: support <hap> feature

[Jim Fehlig]

On 02/04/2016 05:54 AM, Joao Martins wrote:
>
> On 02/04/2016 01:41 AM, Jim Fehlig wrote:
>> On 02/03/2016 02:20 PM, Joao Martins wrote:
>>> On 02/03/2016 04:12 AM, Jim Fehlig wrote:
>>>> Even though the libxl driver advertises <hap> in capabilities,
>>>> it is ignored when set in domXML. Enable hap in the
>>>> libxl_domain_create_info struct when <hap> is specified in domXML.
>>>>
>>>> The xm/xl <-> domXML parser/formatter already supports hap but
>>>> lacked a test with hap enabled.  Add a hap test.
>> [...]
>>> FWIW looks good and also:
>>>
>>> Tested-by: Joao Martins <joao.m.martins at oracle.com>
>> Thanks for reviewing/testing the patch! But after playing with the patch a bit
>> more, I'm considering dropping it :-/.
>>
>> Before the patch, <hap> was ignored by the libxl driver, which left the 'hap'
>> field of libxl_domain_create_info struct unset (= LIBXL__DEFBOOL_DEFAULT). For
>> HVM guests, libxl will enable hap when libxl_domain_create_info.hap ==
>> LIBXL__DEFBOOL_DEFAULT. And using hap is good, since using shadow page table is
>> less efficient. The downside is there is no way to turn hap off.
>>
>> After the patch it is possible to turn hap on and off via presence or absence of
>> <hap>. However, there is a *lot* of existing config without <hap> that currently
>> reaps the benefits of hap nonetheless. After patch, that config would require
>> adding <hap>, else shadow page table will be used. (Note that in xm and xl
>> config, hap is enabled in the absence of 'hap='.)
>>
>> I'm starting to question whether this feature should even be exposed. I think
>> the only use case is debugging. E.g. turn hap off if suspected hardware bug.
>> Otherwise you would certainly want the hypervisor to use hap if it is able to do
>> so. What do you think?
>>
> Hm, good point. There is another field for VIR_TRISTATE which represents when
> it's absent (VIR_TRISTATE_SWITCH_ABSENT). So perhaps we would leave the default
> (= LIBXL_DEFBOOL_DEFAULT) if it's absent, and only set hap if the user
> explicitly switches on/off?

But IIUC, absent == off. I.e., there is no way to turn hap off except the
absence of <hap>.

One option might be to extend <hap> with an 'enabled=yes|no' attribute. Then the
absence of <hap> means hypervisor default. <hap> without the attribute would be
the same as <hap enabled='yes'/> for backwards compatibility. And of course <hap
enabled='no'/> disables hap.

Note that capabilities currently advertises hap as

  <hap default='off' toggle='yes'/>

If folks agree to this option, the default should be changed to 'on'.

>  I am not sure if the usecase is just debug, but
> given there are certain performance differences compared to shadow paging
> (depending on the HVM guest workload) it might be nice to allow the
> administrator to choose it.

Agreed. I think the above option accomplishes that.

Regards,
Jim