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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Mon, Jan 4, 2021 at 11:21 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 04.01.2021 14:28, Tamas K Lengyel wrote:
> > 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.
>
> I suppose we should try to avoid having to change the interface
> again to allow going from "implied 4-level" to "4- or 5-level",
> so I'm with Andrew that this information wants providing even if
> there's going to be only a single value at this time (but you
> wouldn't store a literal number anyway, but instead use the walk
> length associated with the base, so no change to the producer of
> the code would be needed once 5-level walks become an option).

Once 5-level paging is supported a new flag can be added that will
distinguish the tables, for example VM_EVENT_FLAG_NESTED_P2M_5L, if
necessary. So at this time I don't think we really need to do anything
different. If you prefer to change the current flag's name to say _4L,
sure, that's cosmetic.

Tamas