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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 27/02/17 14:03, Andrew Cooper wrote:
> The existing pagetable walker has complicated return semantics, which squeeze
> multiple pieces of information into single integer.  This would be fine if the
> information didn't overlap, but it does.
> 
> Specifically, _PAGE_INVALID_BITS for 3-level guests alias _PAGE_PAGED and
> _PAGE_SHARED.  A guest which constructs a PTE with bits 52 or 53 set (the
> start of the upper software-available range) will create a virtual address
> which, when walked by Xen, tricks Xen into believing the frame is paged or
> shared.  This behaviour was introduced by XSA-173 (c/s 8b17648).
> 
> It is also complicated to turn rc back into a normal pagefault error code.
> Instead, change the calling semantics to return a boolean indicating success,
> and have the function accumulate a real pagefault error code as it goes
> (including synthetic error codes, which do not alias hardware ones).  This
> requires an equivalent adjustment to map_domain_gfn().
> 
> Issues fixed:
>  * 2-level PSE36 superpages now return the correct translation.
>  * 2-level L2 superpages without CR0.PSE now return the correct translation.
>  * SMEP now inhibits a user instruction fetch even if NX isn't active.
>  * Supervisor writes without CR0.WP now set the leaf dirty bit.
>  * L4e._PAGE_GLOBAL is strictly reserved on AMD.
>  * 3-level l3 entries have all reserved bits checked.
>  * 3-level entries can no longer alias Xen's idea of paged or shared.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Looks good -- thanks for doing this.

One tiny comment...

> @@ -372,15 +387,16 @@ static inline unsigned int 
> guest_walk_to_page_order(const walk_t *gw)
>   * we go.  For the purposes of reading pagetables we treat all non-RAM
>   * memory as contining zeroes.
>   * 
> - * Returns 0 for success, or the set of permission bits that we failed on 
> - * if the walk did not complete. */
> + * Returns a boolean indicating success or failure.  walk_t.pfec contains
> + * the accumulated error code on failure.
> + */

Nit: You add the "wing" to the bottom of the comment here, but not to
the top.

Other than that:

Reviewed-by: George Dunlap <george.dunlap@xxxxxxxxxx>


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