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

[Jan Beulich <jbeulich@xxxxxxxx>]

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).

Jan