xen/arch/x86/hvm/monitor.c | 32 ++++++++++++++++++++++++++++++-- xen/include/public/vm_event.h | 7 ++++++- 2 files changed, 36 insertions(+), 3 deletions(-)
Required to introspect events originating from nested VMs.
Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
---
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)
+{
+ if ( nestedhvm_enabled(curr->domain) && nestedhvm_vcpu_in_guestmode(curr) )
+ {
+ req->flags |= VM_EVENT_FLAG_NESTED_P2M;
+ req->data.regs.x86.npt_base = nhvm_vcpu_p2m_base(curr);
+ }
+}
+
bool hvm_monitor_cr(unsigned int index, unsigned long value, unsigned long old)
{
struct vcpu *curr = current;
@@ -53,6 +63,8 @@ bool hvm_monitor_cr(unsigned int index, unsigned long value, unsigned long old)
.u.write_ctrlreg.old_value = old
};
+ set_npt_base(curr, &req);
+
return monitor_traps(curr, sync, &req) >= 0 &&
curr->domain->arch.monitor.control_register_values;
}
@@ -73,6 +85,8 @@ bool hvm_monitor_emul_unimplemented(void)
.vcpu_id = curr->vcpu_id,
};
+ set_npt_base(curr, &req);
+
return curr->domain->arch.monitor.emul_unimplemented_enabled &&
monitor_traps(curr, true, &req) == 1;
}
@@ -92,6 +106,8 @@ bool hvm_monitor_msr(unsigned int msr, uint64_t new_value, uint64_t old_value)
.u.mov_to_msr.old_value = old_value
};
+ set_npt_base(curr, &req);
+
return monitor_traps(curr, 1, &req) >= 0 &&
curr->domain->arch.monitor.control_register_values;
}
@@ -103,6 +119,7 @@ void hvm_monitor_descriptor_access(uint64_t exit_info,
uint64_t vmx_exit_qualification,
uint8_t descriptor, bool is_write)
{
+ struct vcpu *curr = current;
vm_event_request_t req = {
.reason = VM_EVENT_REASON_DESCRIPTOR_ACCESS,
.u.desc_access.descriptor = descriptor,
@@ -115,7 +132,9 @@ void hvm_monitor_descriptor_access(uint64_t exit_info,
req.u.desc_access.arch.vmx.exit_qualification = vmx_exit_qualification;
}
- monitor_traps(current, true, &req);
+ set_npt_base(curr, &req);
+
+ monitor_traps(curr, true, &req);
}
static inline unsigned long gfn_of_rip(unsigned long rip)
@@ -189,6 +208,8 @@ int hvm_monitor_debug(unsigned long rip, enum hvm_monitor_debug_type type,
return -EOPNOTSUPP;
}
+ set_npt_base(curr, &req);
+
return monitor_traps(curr, sync, &req);
}
@@ -207,12 +228,15 @@ int hvm_monitor_cpuid(unsigned long insn_length, unsigned int leaf,
req.u.cpuid.leaf = leaf;
req.u.cpuid.subleaf = subleaf;
+ set_npt_base(curr, &req);
+
return monitor_traps(curr, 1, &req);
}
void hvm_monitor_interrupt(unsigned int vector, unsigned int type,
unsigned int err, uint64_t cr2)
{
+ struct vcpu *curr = current;
vm_event_request_t req = {
.reason = VM_EVENT_REASON_INTERRUPT,
.u.interrupt.x86.vector = vector,
@@ -221,7 +245,9 @@ void hvm_monitor_interrupt(unsigned int vector, unsigned int type,
.u.interrupt.x86.cr2 = cr2,
};
- monitor_traps(current, 1, &req);
+ set_npt_base(curr, &req);
+
+ monitor_traps(curr, 1, &req);
}
/*
@@ -297,6 +323,8 @@ bool hvm_monitor_check_p2m(unsigned long gla, gfn_t gfn, uint32_t pfec,
req.u.mem_access.gla = gla;
req.u.mem_access.offset = gpa & ~PAGE_MASK;
+ set_npt_base(curr, &req);
+
return monitor_traps(curr, true, &req) >= 0;
}
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
@@ -29,7 +29,7 @@
#include "xen.h"
-#define VM_EVENT_INTERFACE_VERSION 0x00000006
+#define VM_EVENT_INTERFACE_VERSION 0x00000007
#if defined(__XEN__) || defined(__XEN_TOOLS__)
@@ -119,6 +119,10 @@
* which singlestep gets automatically disabled.
*/
#define VM_EVENT_FLAG_FAST_SINGLESTEP (1 << 11)
+/*
+ * Set if the event comes from a nested VM and thus npt_base is valid.
+ */
+#define VM_EVENT_FLAG_NESTED_P2M (1 << 12)
/*
* Reasons for the vm event request
@@ -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;
uint32_t cs_base;
uint32_t ss_base;
uint32_t ds_base;
--
2.25.1
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@tklengyel.com>
> ---
> 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).
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.
~Andrew
On Mon, Jan 4, 2021 at 6:57 AM Andrew Cooper <andrew.cooper3@citrix.com> 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@tklengyel.com>
> > ---
> > 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
On 04.01.2021 14:28, Tamas K Lengyel wrote:
> On Mon, Jan 4, 2021 at 6:57 AM Andrew Cooper <andrew.cooper3@citrix.com> 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@tklengyel.com>
>>> ---
>>> 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
On Mon, Jan 4, 2021 at 11:21 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 04.01.2021 14:28, Tamas K Lengyel wrote:
> > On Mon, Jan 4, 2021 at 6:57 AM Andrew Cooper <andrew.cooper3@citrix.com> 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@tklengyel.com>
> >>> ---
> >>> 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
On 04/01/2021 16:32, Tamas K Lengyel wrote:
> On Mon, Jan 4, 2021 at 11:21 AM Jan Beulich <jbeulich@suse.com> wrote:
>> On 04.01.2021 14:28, Tamas K Lengyel wrote:
>>> On Mon, Jan 4, 2021 at 6:57 AM Andrew Cooper <andrew.cooper3@citrix.com> 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@tklengyel.com>
>>>>> ---
>>>>> 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
On Mon, Jan 4, 2021 at 11:48 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 04/01/2021 16:32, Tamas K Lengyel wrote:
> > On Mon, Jan 4, 2021 at 11:21 AM Jan Beulich <jbeulich@suse.com> wrote:
> >> On 04.01.2021 14:28, Tamas K Lengyel wrote:
> >>> On Mon, Jan 4, 2021 at 6:57 AM Andrew Cooper <andrew.cooper3@citrix.com> 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@tklengyel.com>
> >>>>> ---
> >>>>> 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
© 2016 - 2024 Red Hat, Inc.