Re: [PATCH] x86/vm_event: transfer nested p2m base info

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Mon, Jan 4, 2021 at 6:57 AM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>
> On 03/01/2021 18:41, Tamas K Lengyel wrote:
> > Required to introspect events originating from nested VMs.
> >
> > Signed-off-by: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
> > ---
> >  xen/arch/x86/hvm/monitor.c    | 32 ++++++++++++++++++++++++++++++--
> >  xen/include/public/vm_event.h |  7 ++++++-
> >  2 files changed, 36 insertions(+), 3 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
> > index e4a09964a0..eb4afe81b3 100644
> > --- a/xen/arch/x86/hvm/monitor.c
> > +++ b/xen/arch/x86/hvm/monitor.c
> > @@ -26,6 +26,7 @@
> >  #include <xen/mem_access.h>
> >  #include <xen/monitor.h>
> >  #include <asm/hvm/monitor.h>
> > +#include <asm/hvm/nestedhvm.h>
> >  #include <asm/altp2m.h>
> >  #include <asm/monitor.h>
> >  #include <asm/p2m.h>
> > @@ -33,6 +34,15 @@
> >  #include <asm/vm_event.h>
> >  #include <public/vm_event.h>
> >
> > +static inline void set_npt_base(struct vcpu *curr, vm_event_request_t *req)
>
> No need for inline here.  Can fix on commit.
>
> > diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
> > index fdd3ad8a30..8415bc7618 100644
> > --- a/xen/include/public/vm_event.h
> > +++ b/xen/include/public/vm_event.h
> > @@ -208,6 +212,7 @@ struct vm_event_regs_x86 {
> >      uint64_t msr_star;
> >      uint64_t msr_lstar;
> >      uint64_t gdtr_base;
> > +    uint64_t npt_base;
>
> This needs enough description to actually use it correctly.
>
> /* Guest physical address.  On Intel hardware, this is the EPT_POINTER
> field from the L1 hypervisors VMCS, including all architecturally
> defined metadata. */
>
> Except, its not.  nvmx_vcpu_eptp_base() masks out the lower metadata, so
> the walk length is missing, and the introspection agent can't
> distinguish between 4 and 5 level EPT.  Same on the AMD side (except it
> could be any paging mode, including 2 and 3 level).

AMD is AFAIK not supported for vm_events. Also, only 4L EPT is
available at this time, so that information is irrelevant anyway.

>
> Furthermore, (and more in reference to your pagewalk patch), it might be
> necessary to know whether EPT A/D is enabled for the agent to do the
> correct thing when getting a gla-not-valid fault.

Not for a translation. For use-cases where they need to know whether
the page got modified (and want to use A/D for that instead of
tracking such modifications with mem_access), sure, but I'm not aware
of such a use-case. So I would leave that up as a TODO for the time
when it's actually needed.

Tamas