On 20/03/17 08:45, Jan Beulich wrote:
>>>> On 16.03.17 at 17:31, <andrew.coop...@citrix.com> wrote:
>> Switch them to returning bool, and taking const parameters.
>>
>> Rename guest_supports_superpages() to guest_supports_l2_superpages() to
>> indicate which level of pagetables it is actually referring to, and rename
>> guest_supports_1G_superpages() to guest_supports_l3_superpages() for
>> consistency.
>>
>> guest_supports_l3_superpages() is a static property of the domain, rather
>> than
>> control register settings, so is switched to take a domain pointer.
>> hvm_pse1gb_supported() is inlined into its sole user because it isn't
>> strictly
>> hvm-specific (it is hap-specific) and really should be beside a comment
>> explaining why the cpuid policy is ignored.
>>
>> While cleaning up part of the file, clean up all trailing whilespace, and fix
>> one comment which accidently refered to PG living in CR4 rather than CR0.
>>
>> Requested-by: Jan Beulich <jbeul...@suse.com>
>> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
> Reviewed-by: Jan Beulich <jbeul...@suse.com>
> with two remarks:
>
>> -static inline int
>> -guest_supports_1G_superpages(struct vcpu *v)
>> +static inline bool guest_supports_l3_superpages(const struct domain *d)
>> {
>> - return (GUEST_PAGING_LEVELS >= 4 && hvm_pse1gb_supported(v->domain));
>> + /*
>> + * There are no control register settings for the hardware pagewalk on
>> the
>> + * subject of 1G superpages.
>> + *
>> + * If the guest constructs a 1GB superpage on capable hardware, it will
>> + * function irrespective of whether the feature is advertised. Xen's
>> + * model of performing a pagewalk should match.
>> + */
>> + return GUEST_PAGING_LEVELS >= 4 && paging_mode_hap(d) &&
>> cpu_has_page1gb;
> Is it perhaps worth adding half a sentence stating that shadow
> doesn't support 1Gb pages at all?
Good point.
>
> Also I'm still not really happy with the guest_supports_ prefixes
> for this and its L2 counterpart: The question here isn't whether the
> guest supports it (we can't know whether it does), but whether it
> enabled PSE/PAE/LM. Arguably the L3 case is less clear because
> of the mentioned lack of an explicit enabled bit, so I can live with
> the patch going in unchanged (the L2 side then simply for things
> to remain consistent, albeit there's then already the difference of
> parameter types).
How would you prefer them to be named?
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
