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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Mon, Jan 4, 2021 at 11:48 AM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>
> On 04/01/2021 16:32, Tamas K Lengyel wrote:
> > 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.
>
> The way this is currently specified will force a new interface version
> just to add the metadata.
>
> It would suffice to explicitly state that the bottom 12 bits are
> reserved for future metadata, and must be masked out for now, and all
> users of this interface may assume 4L by default.
>
> Basically, what we don't want to happen is for libvmi to take the value,
> not mask out the bottom 12 bits, and start using that, because the
> software will break as soon as we try to encode 5L in there.

Sure, if you just want to add that in a comment I don't see an issue.
Do you want me to resend or can you do it at commit?

Tamas