Re: Fwd: [PATCH 0/18] Nested Virtualization: Overview

[Keir Fraser <keir.fraser@eu.citrix.com>]

On 15/04/2010 16:17, "Christoph Egger" <Christoph.Egger@amd.com> wrote:

> (XEN)    [<ffff82c4801a5257>] nestedsvm_vcpu_stgi+0xa0/0xaf
> (XEN)    [<ffff82c4801a5583>] nestedsvm_vcpu_destroy+0x48/0x6d

Well, I haven't found which of the 19 patches actually implements the above
functions. But it seems odd to me that a function on the vcpu_destroy path
-- which is concerned with final teardown and freeing of vcpu structures
after a domain's death -- would call a function relating to STGI. The latter
being the kind of operation that doesn't seem to make sense outside the
context of a currently-running or at least runnable-in-future guest? You
haven't convinced me yet. ;-) It's not a barrier to the other patches though
-- worst case the rest go in and we argue the merits of this patch versus
other possible approaches as a second step.

>> What if we are not running a nestedhvm guest, or otherwise on a system not
>> supporting paged real mode?
> 
> The hvmloader itself switches to real mode right before invoking the BIOS
> via a trampoline. In virtualization, the cpu always uses paged real mode or
> it can't fetch the instructions, otherwise.

Well, no, hvmloader thinks it is setting CR0.PE *and* CR0.PG both to zero.
The fact that is either emulated or turned into paged real mode under the
hood is an implementation concern. Fact is afaik it is not valid in general
to set CR0.PE=0,CR0.PG=1 via an ordinary MOV-to-CR0 instruction, for
example. In my version of the AMD docs, Vol.2 has a section called Paged
Real Mode which only discusses this mode in the context of the VMRUN and RSM
instructions.

>> Is it wise to remove the check in that case?
> 
> No. The mov-to-cr0 instruction fails with a #GP, otherwise. The guest
> doesn't expect the #GP to happen. The guest just retries the same instruction
> summing up to a double-fault and triple-fault.

It is a guest-visible change in behaviour however. I don't want regressions
in our emulation of x86 semantics as a simple matter of principle, even in
cases like this where indeed it may well not really matter all that much.

In this case, my main objection is that hvm_set_cr0() looks like it emulates
ordinary CR0 semantics, and it will now look like a bug that this check is
missing. You even risk it getting added back in down the road by someone
ignorant of the requirement of nexted SVM.

>> Even where we *do* support nestedhvm, should all guest writes to CR0 be
>> allowed to bypass that check (Isn't paged real mode architecturally only
>> allowed to be entered via VMRUN)?
> 
> Are you talking about the VMRUN instruction or the VMRUN emulation ?
> Note, this is a difference. With nestedhvm, the guest believes to be able
> to run VMRUN instruction. In reality, VMRUN is intercepted and emulated in
> the host. The papers (pdf documents) describe how VMRUN emulation works.

Yeah, right, and the emulated VMRUN needs to be able to set
CR0.PG=1,CR0.PE=0. I can see that. In short, I suggest adding a flags
parameter to hvm_set_cr0() and defining a flag to specify that this specific
check be bypassed. That's all nice and explicit and it's clear-as-day then
that we are emulating CR0 semantics correctly as possible in all cases. The
downside that we've made hvm_set_cr0() a bit more clunky is worth it imo.

 -- Keir