On 02/04/2016 06:22 PM, Jim Fehlig wrote: > 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@xxxxxxxxxx> >>> 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>. > Ah, right. I was double checking and there's really no way to have it. > 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. > And it sounds that it could be applicable for other features too: e.g. pae (instead of having both pae and nonpae?) > 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. Yeap, I wonder if the absence of a "default" in the capabilities could turn into "hypervisor/driver default" since you can explicitly choose "on" or "off" already. Or perhaps it doesn't make a lot of sense to do that. Joao > > Regards, > Jim > > -- > libvir-list mailing list > libvir-list@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/libvir-list > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list