Re: [PATCH 3/7] x86/pagewalk: Helpers for reserved bit handling

["Jan Beulich" <JBeulich@suse.com>]

>>> On 27.02.17 at 15:03, <andrew.cooper3@citrix.com> wrote:
> -static inline int
> -guest_supports_1G_superpages(struct vcpu *v)
> +static inline bool guest_has_pse36(const struct vcpu *v)
> +{
> +     /* No support for 2-level PV guests. */
> +    return is_pv_vcpu(v) ? 0 : paging_mode_hap(v->domain);

Considering the return type ITYM "false" instead of "0" here. But
then why use a conditional expression here at all?

    return !is_pv_vcpu(v) && paging_mode_hap(v->domain);

Furthermore there's no point in passing in a struct vcpu here -
both checked constructs are per-domain, and passing in
struct domain is also a better fit with the guest_ prefix of the
function name.

> +static inline bool guest_supports_1G_superpages(const struct vcpu *v)
>  {
>      return (GUEST_PAGING_LEVELS >= 4 && hvm_pse1gb_supported(v->domain));

Same here for vcpu vs domain.

> -static inline int
> -guest_supports_nx(struct vcpu *v)
> +static inline bool guest_supports_nx(const struct vcpu *v)
>  {
>      if ( GUEST_PAGING_LEVELS == 2 || !cpu_has_nx )
>          return 0;

The situation is different here - hvm_nx_enabled() can vary across
vCPU-s, so here it's rather the name which doesn't really fit the
purpose (should rather be guest_uses_nx() or some such).

> +static inline bool guest_l2e_rsvd_bits(const struct vcpu *v, guest_l2e_t l2e)
> +{
> +    uint64_t rsvd_bits = guest_rsvd_bits(v);
> +
> +    return ((l2e.l2 & (rsvd_bits | GUEST_L2_PAGETABLE_RSVD |
> +                       (guest_supports_superpages(v) ? 0 : _PAGE_PSE))) ||
> +            ((l2e.l2 & _PAGE_PSE) &&
> +             (l2e.l2 & ((GUEST_PAGING_LEVELS == 2 && guest_has_pse36(v))
> +                        ? (fold_pse36(rsvd_bits | (1ULL << 40)))

While one can look it up in the doc, I strongly think this 40 needs a
comment.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel