RE: [PATCH 17/31] nVMX: Prepare vmcs02 from vmcs01 and vmcs12
> From: Nadav Har'El
> Sent: Tuesday, May 24, 2011 5:19 PM
>
> On Tue, May 24, 2011, Tian, Kevin wrote about "RE: [PATCH 17/31] nVMX:
> Prepare vmcs02 from vmcs01 and vmcs12":
> > > +static inline unsigned long guest_readable_cr4(struct vmcs12 *fields)
> > > +{
> > > + return (fields->guest_cr4 & ~fields->cr4_guest_host_mask) |
> > > + (fields->cr4_read_shadow &
> fields->cr4_guest_host_mask);
> > > +}
> > > +
> >
> > will guest_ prefix look confusing here? The 'guest' has a broad range which
> makes
> > above two functions look like they can be used in non-nested case. Should we
> stick
> > to nested_prefix for nested specific facilities?
>
> I don't know, I thought it made calls like
>
> vmcs_writel(CR0_READ_SHADOW, guest_readable_cr0(vmcs12));
>
> readable, and the comments (and the parameters) make it obvious it's for
> nested only.
>
> I now renamed these functions nested_read_cr0(), nested_read_cr4() - I hope
> you like these names better.
yes.
>
> > > + if (is_guest_mode(&vmx->vcpu))
> > > + vmx->vcpu.arch.cr4_guest_owned_bits &=
> > > +
> ~get_vmcs12(&vmx->vcpu)->cr4_guest_host_mask;
> >
> > why not is_nested_mode()? :-P
>
> I assume you're wondering why the function is called is_guest_mode(), and
> not is_nested_mode()?
yes
>
> This name was chosen by Avi Kivity in November last year, for the function
> previously introduced by Joerg Roedel. My original code (before Joerg added
> this function to x86.c) indeed used the term "nested_mode", not
> "guest_mode".
>
> In January, I pointed to the possibility of confusion between the new
> is_guest_mode() and other things called "guest mode", and Avi Kivity said
> he will rename it to is_nested_guest() - see
> http://lkml.indiana.edu/hypermail/linux/kernel/1101.1/01418.html
> But as you can see, he never did this renaming.
>
> That being said, after half a year, I got used to the name is_guest_mode(),
> and am no longer convinced it should be changed. It checks whether the vcpu
> (not the underlying CPU) is in Intel-SDM-terminology "guest mode". Just like
> is_long_mode() checks if the vcpu is in long mode. So I'm fine with leaving
> its current name.
well, it's a small issue, and I'm fine with leaving it though I don't like
'guest' here. :-)
>
> > > +static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12
> *vmcs12)
> > > +{
> >...
> > > + if (!vmx->rdtscp_enabled)
> > > + exec_control &= ~SECONDARY_EXEC_RDTSCP;
> > > + /* Take the following fields only from vmcs12 */
> > > + exec_control &=
> ~SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES;
> > > + if (nested_cpu_has(vmcs12,
> > > +
> CPU_BASED_ACTIVATE_SECONDARY_CONTROLS))
> > > + exec_control |=
> vmcs12->secondary_vm_exec_control;
> >
> > should this 2nd exec_control be merged in clear case-by-case flavor?
> >
> > what about L0 sets "virtualize x2APIC" bit while L1 doesn't?
> >
> > Or what about L0 disables EPT while L1 sets it?
> >
> > I think it's better to scrutinize every 2nd exec_control feature with a
> > clear policy:
> > - whether we want to use the stricter policy which is only set when both L0
> and
> > L1 set it
> > - whether we want to use L1 setting absolutely regardless of L0 setting like
> > what you did for virtualize APIC access
>
> Please note that most of the examples you give cannot happen in practice,
> because we tell L1 (via MSR) which features it is allowed to use, and we
> fail entry if it tries to use disallowed features (before ever reaching
> the merge code you're commenting on). So we don't allow L1, for example,
> to use the EPT feature (and when nested-EPT support is added, we won't
> allow L1 to use EPT if L0 didn't). The general thinking was that for most
> fields that we do explicitly allow, "OR" is the right choice.
This really bases on the value of the control bit. To achieve the strictest
setting between L0/L1, sometimes you want to use AND and sometimes you
want to use OR.
>From a design p.o.v, it's better not to have such implicit assumption on other
places. Just make it clean and correct. Also in your example it doesn't cover
the case where L0 sets some bits which are not exposed to L1 via MSR. For
example as I said earlier, what about L0 sets virtualize X2APIC mode while
it's not enabled by or not exposed to L1. With OR, you then also enable this
mode for L2 absolutely, while L1 has no logic to handle it.
I'd like to see a clean policy for the known control bits here, even with a
strict policy to incur most VM-exits which can be optimized in the future.
>
> I'll add this to my bugzilla, and think about it again later.
>
Thanks
Kevin
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 17/31] nVMX: Prepare vmcs02 from vmcs01 and vmcs12
On Tue, May 24, 2011, Tian, Kevin wrote about "RE: [PATCH 17/31] nVMX: Prepare
vmcs02 from vmcs01 and vmcs12":
> > +static inline unsigned long guest_readable_cr4(struct vmcs12 *fields)
> > +{
> > + return (fields->guest_cr4 & ~fields->cr4_guest_host_mask) |
> > + (fields->cr4_read_shadow & fields->cr4_guest_host_mask);
> > +}
> > +
>
> will guest_ prefix look confusing here? The 'guest' has a broad range which
> makes
> above two functions look like they can be used in non-nested case. Should we
> stick
> to nested_prefix for nested specific facilities?
I don't know, I thought it made calls like
vmcs_writel(CR0_READ_SHADOW, guest_readable_cr0(vmcs12));
readable, and the comments (and the parameters) make it obvious it's for
nested only.
I now renamed these functions nested_read_cr0(), nested_read_cr4() - I hope
you like these names better.
> > + if (is_guest_mode(&vmx->vcpu))
> > + vmx->vcpu.arch.cr4_guest_owned_bits &=
> > + ~get_vmcs12(&vmx->vcpu)->cr4_guest_host_mask;
>
> why not is_nested_mode()? :-P
I assume you're wondering why the function is called is_guest_mode(), and
not is_nested_mode()?
This name was chosen by Avi Kivity in November last year, for the function
previously introduced by Joerg Roedel. My original code (before Joerg added
this function to x86.c) indeed used the term "nested_mode", not "guest_mode".
In January, I pointed to the possibility of confusion between the new
is_guest_mode() and other things called "guest mode", and Avi Kivity said
he will rename it to is_nested_guest() - see
http://lkml.indiana.edu/hypermail/linux/kernel/1101.1/01418.html
But as you can see, he never did this renaming.
That being said, after half a year, I got used to the name is_guest_mode(),
and am no longer convinced it should be changed. It checks whether the vcpu
(not the underlying CPU) is in Intel-SDM-terminology "guest mode". Just like
is_long_mode() checks if the vcpu is in long mode. So I'm fine with leaving
its current name.
> > +static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
> > +{
>...
> > + if (!vmx->rdtscp_enabled)
> > + exec_control &= ~SECONDARY_EXEC_RDTSCP;
> > + /* Take the following fields only from vmcs12 */
> > + exec_control &= ~SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES;
> > + if (nested_cpu_has(vmcs12,
> > + CPU_BASED_ACTIVATE_SECONDARY_CONTROLS))
> > + exec_control |= vmcs12->secondary_vm_exec_control;
>
> should this 2nd exec_control be merged in clear case-by-case flavor?
>
> what about L0 sets "virtualize x2APIC" bit while L1 doesn't?
>
> Or what about L0 disables EPT while L1 sets it?
>
> I think it's better to scrutinize every 2nd exec_control feature with a
> clear policy:
> - whether we want to use the stricter policy which is only set when both L0
> and
> L1 set it
> - whether we want to use L1 setting absolutely regardless of L0 setting like
> what you did for virtualize APIC access
Please note that most of the examples you give cannot happen in practice,
because we tell L1 (via MSR) which features it is allowed to use, and we
fail entry if it tries to use disallowed features (before ever reaching
the merge code you're commenting on). So we don't allow L1, for example,
to use the EPT feature (and when nested-EPT support is added, we won't
allow L1 to use EPT if L0 didn't). The general thinking was that for most
fields that we do explicitly allow, "OR" is the right choice.
I'll add this to my bugzilla, and think about it again later.
Thanks,
Nadav
--
Nadav Har'El| Tuesday, May 24 2011, 20 Iyyar 5771
n...@math.technion.ac.il |-
Phone +972-523-790466, ICQ 13349191 |If you choke a Smurf, what color does it
http://nadav.harel.org.il |turn?
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 17/31] nVMX: Prepare vmcs02 from vmcs01 and vmcs12
> From: Nadav Har'El
> Sent: Tuesday, May 17, 2011 3:53 AM
>
> This patch contains code to prepare the VMCS which can be used to actually
> run the L2 guest, vmcs02. prepare_vmcs02 appropriately merges the
> information
> in vmcs12 (the vmcs that L1 built for L2) and in vmcs01 (our desires for our
> own guests).
>
> Signed-off-by: Nadav Har'El
> ---
> arch/x86/kvm/vmx.c | 269
> +++
> 1 file changed, 269 insertions(+)
>
> --- .before/arch/x86/kvm/vmx.c2011-05-16 22:36:48.0 +0300
> +++ .after/arch/x86/kvm/vmx.c 2011-05-16 22:36:48.0 +0300
> @@ -347,6 +347,12 @@ struct nested_vmx {
> /* vmcs02_list cache of VMCSs recently used to run L2 guests */
> struct list_head vmcs02_pool;
> int vmcs02_num;
> + u64 vmcs01_tsc_offset;
> + /*
> + * Guest pages referred to in vmcs02 with host-physical pointers, so
> + * we must keep them pinned while L2 runs.
> + */
> + struct page *apic_access_page;
> };
>
> struct vcpu_vmx {
> @@ -849,6 +855,18 @@ static inline bool report_flexpriority(v
> return flexpriority_enabled;
> }
>
> +static inline bool nested_cpu_has(struct vmcs12 *vmcs12, u32 bit)
> +{
> + return vmcs12->cpu_based_vm_exec_control & bit;
> +}
> +
> +static inline bool nested_cpu_has2(struct vmcs12 *vmcs12, u32 bit)
> +{
> + return (vmcs12->cpu_based_vm_exec_control &
> + CPU_BASED_ACTIVATE_SECONDARY_CONTROLS) &&
> + (vmcs12->secondary_vm_exec_control & bit);
> +}
> +
> static int __find_msr_index(struct vcpu_vmx *vmx, u32 msr)
> {
> int i;
> @@ -1435,6 +1453,22 @@ static void vmx_fpu_activate(struct kvm_
>
> static void vmx_decache_cr0_guest_bits(struct kvm_vcpu *vcpu);
>
> +/*
> + * Return the cr0 value that a nested guest would read. This is a combination
> + * of the real cr0 used to run the guest (guest_cr0), and the bits shadowed
> by
> + * its hypervisor (cr0_read_shadow).
> + */
> +static inline unsigned long guest_readable_cr0(struct vmcs12 *fields)
> +{
> + return (fields->guest_cr0 & ~fields->cr0_guest_host_mask) |
> + (fields->cr0_read_shadow & fields->cr0_guest_host_mask);
> +}
> +static inline unsigned long guest_readable_cr4(struct vmcs12 *fields)
> +{
> + return (fields->guest_cr4 & ~fields->cr4_guest_host_mask) |
> + (fields->cr4_read_shadow & fields->cr4_guest_host_mask);
> +}
> +
will guest_ prefix look confusing here? The 'guest' has a broad range which
makes
above two functions look like they can be used in non-nested case. Should we
stick
to nested_prefix for nested specific facilities?
> static void vmx_fpu_deactivate(struct kvm_vcpu *vcpu)
> {
> vmx_decache_cr0_guest_bits(vcpu);
> @@ -3423,6 +3457,9 @@ static void set_cr4_guest_host_mask(stru
> vmx->vcpu.arch.cr4_guest_owned_bits =
> KVM_CR4_GUEST_OWNED_BITS;
> if (enable_ept)
> vmx->vcpu.arch.cr4_guest_owned_bits |= X86_CR4_PGE;
> + if (is_guest_mode(&vmx->vcpu))
> + vmx->vcpu.arch.cr4_guest_owned_bits &=
> + ~get_vmcs12(&vmx->vcpu)->cr4_guest_host_mask;
why not is_nested_mode()? :-P
> vmcs_writel(CR4_GUEST_HOST_MASK,
> ~vmx->vcpu.arch.cr4_guest_owned_bits);
> }
>
> @@ -4760,6 +4797,11 @@ static void free_nested(struct vcpu_vmx
> vmx->nested.current_vmptr = -1ull;
> vmx->nested.current_vmcs12 = NULL;
> }
> + /* Unpin physical memory we referred to in current vmcs02 */
> + if (vmx->nested.apic_access_page) {
> + nested_release_page(vmx->nested.apic_access_page);
> + vmx->nested.apic_access_page = 0;
> + }
>
> nested_free_all_saved_vmcss(vmx);
> }
> @@ -5829,6 +5871,233 @@ static void vmx_set_supported_cpuid(u32
> }
>
> /*
> + * prepare_vmcs02 is called when the L1 guest hypervisor runs its nested
> + * L2 guest. L1 has a vmcs for L2 (vmcs12), and this function "merges" it
> + * with L0's requirements for its guest (a.k.a. vmsc01), so we can run the L2
> + * guest in a way that will both be appropriate to L1's requests, and our
> + * needs. In addition to modifying the active vmcs (which is vmcs02), this
> + * function also has additional necessary side-effects, like setting various
> + * vcpu->arch fields.
> + */
> +static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
> +{
> + struct vcpu_vmx *vmx = to_vmx(vcpu);
> + u32 exec_control;
> +
> + vmcs_write16(GUEST_ES_SELECTOR, vmcs12->guest_es_selector);
> + vmcs_write16(GUEST_CS_SELECTOR, vmcs12->guest_cs_selector);
> + vmcs_write16(GUEST_SS_SELECTOR, vmcs12->guest_ss_selector);
> + vmcs_write16(GUEST_DS_SELECTOR, vmcs12->guest_ds_selector);
> + vmcs_write16(GUEST_FS_SELECTOR, vmcs12->guest_fs_selector);
> + vmcs_write16(GUEST_GS_SELECTOR, vmcs12->guest_gs_selector);
> + vmcs_write16(GUEST_LDTR_SELECTOR, vmcs12->guest_ldtr_selector);
> + vmcs_write16(GUEST_TR_SELECTOR, vmcs1
