Re: [PATCH 17/31] nVMX: Prepare vmcs02 from vmcs01 and vmcs12

From: "Nadav Har'El" <nyh@math.technion.ac.il>
To: "Tian, Kevin" <kevin.tian@intel.com>
Cc: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"gleb@redhat.com" <gleb@redhat.com>,
	"avi@redhat.com" <avi@redhat.com>
Subject: Re: [PATCH 17/31] nVMX: Prepare vmcs02 from vmcs01 and vmcs12
Date: Tue, 24 May 2011 12:19:18 +0300	[thread overview]
Message-ID: <20110524091918.GA136@fermat.math.technion.ac.il> (raw)
In-Reply-To: <625BA99ED14B2D499DC4E29D8138F1505C9BFA33C3@shsmsx502.ccr.corp.intel.com>

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
nyh@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?