Re: [Xen-devel] [PATCH v2] x86/mm: Suppresses vm_events caused by page-walks

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.09.18 at 11:47, <aisaila@xxxxxxxxxxxxxxx> wrote:
> The original version of the patch emulated the current instruction
> (which, as a side-effect, emulated the page-walk as well), however we
> need finer-grained control. We want to emulate the page-walk, but still
> get an EPT violation event if the current instruction would trigger one.
> This patch performs just the page-walk emulation.

Rather than making this basically a revision log, could you please focus
on what you actually want to achieve?

As to the title: "Suppress ..." please.

> @@ -149,6 +151,10 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m,
>      ar_and &= gflags;
>      ar_or  |= gflags;
>  
> +    if ( set_ad && set_ad_bits(&l4p[guest_l4_table_offset(va)].l4,
> +                               &gw->l4e.l4, false) )
> +        accessed = true;

It is in particular this seemingly odd (and redundant with what's done
later in the function) which needs thorough explanation.

> @@ -362,6 +376,13 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m,
>       */
>      ar = (ar_and & AR_ACCUM_AND) | (ar_or & AR_ACCUM_OR);
>  
> +    if ( set_ad )
> +    {
> +        set_ad_bits(&l1p[guest_l1_table_offset(va)].l1, &gw->l1e.l1,
> +                    (ar & _PAGE_RW) && !accessed && !guest_wp_enabled(v));
> +        goto out;
> +    }

Why does A bits being set (or not) in non-leaf (possibly, but even that's
inconsistent in your handling) tables have any meaning for whether the
D bit wants setting in the leaf page table? Similarly, why does it matter
whether the accumulated permissions allow writing, irrespective of
whether the operation was actually a write? Same for CR0.WP.

> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -768,7 +768,8 @@ static const struct paging_mode hap_paging_real_mode = {
>      .update_cr3             = hap_update_cr3,
>      .update_paging_modes    = hap_update_paging_modes,
>      .write_p2m_entry        = hap_write_p2m_entry,
> -    .guest_levels           = 1
> +    .guest_levels           = 1,
> +    .page_walk_set_ad_bits  = hap_page_walk_set_ad_bits_2_levels
>  };

Here and elsewhere, please add new hooks next to other hooks,
not at the end.

> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -214,7 +214,10 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>           d->arch.monitor.inguest_pagefault_disabled &&
>           npfec.kind != npfec_kind_with_gla ) /* don't send a mem_event */
>      {
> -        hvm_emulate_one_vm_event(EMUL_KIND_NORMAL, TRAP_invalid_op, 
> X86_EVENT_NO_EC);
> +        struct hvm_hw_cpu ctxt;
> +
> +        hvm_funcs.save_cpu_ctxt(v, &ctxt);

Pretty expensive an operation when all you're after is
v->arch.hvm.guest_cr[3].

Jan



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