Re: [Xen-devel] [PATCH 7/7] x86/pagewalk: Re-implement the pagetable walker

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 02/03/17 11:52, Jan Beulich wrote:
>>>> On 27.02.17 at 15:03, <andrew.cooper3@xxxxxxxxxx> wrote:
>> @@ -91,12 +91,12 @@ unsigned long hap_p2m_ga_to_gfn(GUEST_PAGING_LEVELS)(
>>  #if GUEST_PAGING_LEVELS == 3
>>      top_map += (cr3 & ~(PAGE_MASK | 31));
>>  #endif
>> -    missing = guest_walk_tables(v, p2m, ga, &gw, pfec[0], top_mfn, top_map);
>> +    walk_ok = guest_walk_tables(v, p2m, ga, &gw, pfec[0], top_mfn, top_map);
> Since afaict pfec doesn't really point to an array, could I talk you into
> adjusting the uses you touch anyway to become *pfec?

I did find the use of an array annoying, but left it as-was.

Any objection if I do a general cleanup of that as a prerequisite patch,
to avoid introducing semi-unrelated changes into this patch?

>
>> --- a/xen/arch/x86/mm/hap/nested_ept.c
>> +++ b/xen/arch/x86/mm/hap/nested_ept.c
>> @@ -208,7 +208,7 @@ nept_walk_tables(struct vcpu *v, unsigned long l2ga, 
>> ept_walk_t *gw)
>>      goto out;
>>  
>>  map_err:
>> -    if ( rc == _PAGE_PAGED )
>> +    if ( rc == PFEC_page_paged )
> While orthogonal to the patch here, is == (instead of &) really the
> right check here?

In practice, if PFEC_page_paged is set, it is the only bit set.

But I agree - conceptually it should &, and will need to be when I fix
nested pagetable walking.

>
>> @@ -3737,18 +3737,9 @@ sh_gva_to_gfn(struct vcpu *v, struct p2m_domain *p2m,
>>          return vtlb_gfn;
>>  #endif /* (SHADOW_OPTIMIZATIONS & SHOPT_VIRTUAL_TLB) */
>>  
>> -    if ( (missing = sh_walk_guest_tables(v, va, &gw, pfec[0])) != 0 )
>> +    if ( !(walk_ok = sh_walk_guest_tables(v, va, &gw, pfec[0])) )
> *pfec again (also further down)?
>
>> --- a/xen/include/asm-x86/guest_pt.h
>> +++ b/xen/include/asm-x86/guest_pt.h
>> @@ -236,19 +236,26 @@ static inline bool guest_supports_nx(const struct vcpu 
>> *v)
>>      return hvm_nx_enabled(v);
>>  }
>>  
>> +static inline bool guest_wp_enabled(const struct vcpu *v)
>> +{
>> +    /* PV guests can't control CR0.WP, and it is unconditionally set by 
>> Xen. */
>> +    return is_pv_vcpu(v) || hvm_wp_enabled(v);
>> +}
>>  
>> -/*
>> - * Some bits are invalid in any pagetable entry.
>> - * Normal flags values get represented in 24-bit values (see
>> - * get_pte_flags() and put_pte_flags()), so set bit 24 in
>> - * addition to be able to flag out of range frame numbers.
>> - */
>> -#if GUEST_PAGING_LEVELS == 3
>> -#define _PAGE_INVALID_BITS \
>> -    (_PAGE_INVALID_BIT | get_pte_flags(((1ull << 63) - 1) & ~(PAGE_SIZE - 
>> 1)))
>> -#else /* 2-level and 4-level */
>> -#define _PAGE_INVALID_BITS _PAGE_INVALID_BIT
>> -#endif
>> +static inline bool guest_smep_enabled(const struct vcpu *v)
>> +{
>> +    return is_pv_vcpu(v) ? 0 : hvm_smep_enabled(v);
>> +}
>> +
>> +static inline bool guest_smap_enabled(const struct vcpu *v)
>> +{
>> +    return is_pv_vcpu(v) ? 0 : hvm_smap_enabled(v);
>> +}
>> +
>> +static inline bool guest_pku_enabled(const struct vcpu *v)
>> +{
>> +    return is_pv_vcpu(v) ? 0 : hvm_pku_enabled(v);
>> +}
> All three "!is_pv_vcpu(v) && hvm_..._enabled(v)" ? Or otherwise
> please use false.

Ok.

>
> It would be nice if you could adjust the one _eflags use to eflags,
> avoiding the need for a follow-up patch.

Will do.

> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks,

~Andrew

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