[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Mon, 4 Jan 2021 16:47:28 +0000
  • Authentication-results: esa3.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none
  • Cc: Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Alexandru Isaila <aisaila@xxxxxxxxxxxxxxx>, Petre Pircalabu <ppircalabu@xxxxxxxxxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>
  • Delivery-date: Mon, 04 Jan 2021 16:48:18 +0000
  • Ironport-sdr: 0uzDxMBKnU9TFKuzhdlc4iajg7Mpi2PfIWHGvAk8Fwa5rogjM4hJJM/uxszrBWrF2Qcc72gjyV dysbyQbR9yZVeCrDlHtp2j+jprA3VyEt9178zUfzlB2n+OK80g6YH8LjqwZ1JBSuTA+b448mnd yCuX/8wnm9cetRXbTMEqwDJ8A6m/xIhVagsQVmw/Md1Cj/dbxex8N2Ev3iT1Elxed72In32xQE H6BPRAwYcJZV6koUhvTPYR0q62H7XvRFzs4RfP+PzfA1YgGJqCkykh8cl9sLcD54eoJlpODfqp mZs=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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.

~Andrew



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.