Virtualization - Jul 2020 - [PATCH v4 70/75] x86/head/64: Don't call verify_cpu() on starting APs

On Wed, Jul 15, 2020 at 11:26:38AM +0200, Joerg Roedel
wrote:
> Hi Kees,
> 
> thanks for your reviews!
> 
> On Tue, Jul 14, 2020 at 06:40:30PM -0700, Kees Cook wrote:
> > Eek, no. MSR_IA32_MISC_ENABLE_XD_DISABLE needs to be cleared very
early
> > during CPU startup; this can't just be skipped.
> 
> That MSR is Intel-only, right? The boot-path installed here is only used
> for SEV-ES guests, running on AMD systems, so this MSR is not even
> accessed during boot on those VMs.
Oh, hrm, yes, that's true. If other x86 maintainers are comfortable with
this, then okay. My sense is that changing the early CPU startup paths
will cause trouble down the line.
> The alternative is to set up exception handling prior to calling
> verify_cpu, including segments, stack and IDT. Given that verify_cpu()
> does not add much value to SEV-ES guests, I'd like to avoid adding this
> complexity.
So, going back to the requirements here ... what things in verify_cpu()
can cause exceptions? AFAICT, cpuid is safely handled (i.e. it is
detected and only run in a way to avoid exceptions and the MSR
reads/writes are similarly bound by CPU family/id range checks). I must
be missing something. :)
> 
> > Also, is UNWIND_HINT_EMPTY needed for the new target?
> 
> Yes, I think it is, will add it in the next version.
> 
> Regards,
> 
> 	Joerg
-- 
Kees Cook

Hi Kees,

as a general note: With SEV-ES the guest kernel will get #VC exceptions
for events that, without SEV-ES, would just cause a #VMEXIT to the
hypervisor.

On Wed, Jul 15, 2020 at 08:26:14AM -0700, Kees Cook
wrote:
> On Wed, Jul 15, 2020 at 11:26:38AM +0200, Joerg Roedel wrote:
> > That MSR is Intel-only, right? The boot-path installed here is only
used
> > for SEV-ES guests, running on AMD systems, so this MSR is not even
> > accessed during boot on those VMs.
> 
> Oh, hrm, yes, that's true. If other x86 maintainers are comfortable
with
> this, then okay. My sense is that changing the early CPU startup paths
> will cause trouble down the line.
The AP startup path does not change for non SEV-ES guests. But under
SEV-ES everything that might cause a #VC exception must be avoided until
the kernel is ready to handle them. With the current patches this
happens when the AP runs in 64bit long-mode and loaded TSS and IDT.
Therefore a slightly different AP boot-path is needed for SEV-ES guests.
> So, going back to the requirements here ... what things in verify_cpu()
> can cause exceptions? AFAICT, cpuid is safely handled (i.e. it is
> detected and only run in a way to avoid exceptions and the MSR
> reads/writes are similarly bound by CPU family/id range checks). I must
> be missing something. :)
It is actually the CPUID instructions that cause #VC exceptions. The
MSRs that are accessed on AMD processors are not intercepted in the
hypervisors this code has been tested on, so these will not cause #VC
exceptions.

Regards,

	Joerg

On Wed, Jul 15, 2020 at 05:48:56PM +0200, Joerg Roedel
wrote:
> It is actually the CPUID instructions that cause #VC exceptions. The
> MSRs that are accessed on AMD processors are not intercepted in the
> hypervisors this code has been tested on, so these will not cause #VC
> exceptions.
Aaah. I see. Thanks for the details there. So ... can you add a bunch
more comments about why/when the new entry path is being used? I really
don't want to accidentally discover some unrelated refactoring down
the road (in months, years, unrelated to SEV, etc) starts to also skip
verify_cpu() on Intel systems. There had been a lot of BIOSes that set
this MSR to disable NX, and I don't want to repeat that pain: Linux must
never start an Intel CPU with that MSR set. :P

-- 
Kees Cook