Re: [Xen-devel] [PATCH] x86/shim: Fixes to replace_linear_mapping()

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jan 24, 2018 at 11:00:05AM +0000, Andrew Cooper wrote:
> The function replace_va_mapping() has multiple issues:
>  * It uses linear addresses, not virtual addresses.  Fix its name.
>  * Guest pagetables are allocated from the domheap not the xenheap, so need
>    map_domain_page() to safely access.

This should also need changing in mark_pv_pt_pages_rdonly (which is
where I got the idea from).

>  * put_page_and_type() should only apply to present mappings.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

LGTM, just a couple of comments/questions.

> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Sergey Dyasli <sergey.dyasli@xxxxxxxxxx>
> ---
>  xen/arch/x86/pv/shim.c | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/x86/pv/shim.c b/xen/arch/x86/pv/shim.c
> index d5383dc..fae7818 100644
> --- a/xen/arch/x86/pv/shim.c
> +++ b/xen/arch/x86/pv/shim.c
> @@ -119,19 +119,23 @@ uint64_t pv_shim_mem(uint64_t avail)
>                   _PAGE_GUEST_KERNEL)
>  #define COMPAT_L1_PROT (_PAGE_PRESENT|_PAGE_RW|_PAGE_ACCESSED)
>  
> -static void __init replace_va_mapping(struct domain *d, l4_pgentry_t 
> *l4start,
> -                                      unsigned long va, mfn_t mfn)
> +static void __init replace_linear_mapping(
> +    struct domain *d, l4_pgentry_t *l4t, unsigned long linear, mfn_t mfn)

I would prefer to keep the previous style here (which is also used in
the rest of the file).

>  {
> -    l4_pgentry_t *pl4e = l4start + l4_table_offset(va);
> -    l3_pgentry_t *pl3e = l4e_to_l3e(*pl4e) + l3_table_offset(va);
> -    l2_pgentry_t *pl2e = l3e_to_l2e(*pl3e) + l2_table_offset(va);
> -    l1_pgentry_t *pl1e = l2e_to_l1e(*pl2e) + l1_table_offset(va);
> -    struct page_info *page = mfn_to_page(l1e_get_mfn(*pl1e));
> +    l4_pgentry_t *l4e = l4t                    + l4_table_offset(linear);
> +    l3_pgentry_t *l3e = map_l3t_from_l4e(*l4e) + l3_table_offset(linear);
> +    l2_pgentry_t *l2e = map_l2t_from_l3e(*l3e) + l2_table_offset(linear);
> +    l1_pgentry_t *l1e = map_l1t_from_l2e(*l2e) + l1_table_offset(linear);
> +    unsigned int flags = is_pv_32bit_domain(d) ? COMPAT_L1_PROT : L1_PROT;
>  
> -    put_page_and_type(page);
> +    if ( l1e_get_flags(*l1e) & _PAGE_PRESENT )

Given the limited usage of the function ATM, isn't this always
guaranteed?

Or are you maybe planning on using it for a different purpose?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel