Re: [Xen-devel] [PATCH v2 4/9] x86/pagewalk: Clean up guest_supports_* predicates
>>> On 23.03.17 at 18:32, wrote: > On 20/03/17 13:59, Jan Beulich wrote: > On 20.03.17 at 14:36, wrote: >>> On 20/03/17 08:45, Jan Beulich wrote: 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? >> I think I did (or at least had meant to) suggest guest_uses_...() or >> something similar. > > Grammatically, that is still somewhat awkward. > > How about guest_can_use_...() ? That logically covers both that the > feature might be missing, or the control register might not be suitably > configured. Fine with me. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 4/9] x86/pagewalk: Clean up guest_supports_* predicates
On 20/03/17 13:59, Jan Beulich wrote: On 20.03.17 at 14:36, wrote: >> On 20/03/17 08:45, Jan Beulich wrote: >>> 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? > I think I did (or at least had meant to) suggest guest_uses_...() or > something similar. Grammatically, that is still somewhat awkward. How about guest_can_use_...() ? That logically covers both that the feature might be missing, or the control register might not be suitably configured. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 4/9] x86/pagewalk: Clean up guest_supports_* predicates
At 16:31 + on 16 Mar (1489681898), Andrew Cooper 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 > Signed-off-by: Andrew Cooper Acked-by: Tim Deegan (with or without the renaming Jan asked for). ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 4/9] x86/pagewalk: Clean up guest_supports_* predicates
>>> On 20.03.17 at 14:36, wrote: > On 20/03/17 08:45, Jan Beulich wrote: >> 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? I think I did (or at least had meant to) suggest guest_uses_...() or something similar. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 4/9] x86/pagewalk: Clean up guest_supports_* predicates
On 20/03/17 08:45, Jan Beulich wrote:
On 16.03.17 at 17:31, 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
>> Signed-off-by: Andrew Cooper
> Reviewed-by: Jan Beulich
> 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
Re: [Xen-devel] [PATCH v2 4/9] x86/pagewalk: Clean up guest_supports_* predicates
>>> On 16.03.17 at 17:31, 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
> Signed-off-by: Andrew Cooper
Reviewed-by: Jan Beulich
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?
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).
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
