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

[Isaila Alexandru <aisaila@xxxxxxxxxxxxxxx>]

On Thu, 2018-09-13 at 08:17 -0600, Jan Beulich wrote:
> > > > 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.

On this patch I've followed Andrew Cooper's suggestion on how to set
A/D Bits:

"While walking down the levels, set any missing A bits and remember if
we
set any.  If we set A bits, consider ourselves complete and exit back
to
the guest.  If no A bits were set, and the access was a write (which we
know from the EPT violation information), then set the leaf D bit."

If I misunderstood the comment please clarify.

Thanks, 
Alex

> 
> > @@ -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