Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 24 Aug 2022 21:57:09 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Maxim Levitsky <mlevitsk@redhat.com>
Cc:     kvm@vger.kernel.org, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        linux-kernel@vger.kernel.org, Wanpeng Li <wanpengli@tencent.com>,
        Ingo Molnar <mingo@redhat.com>, x86@kernel.org,
        Jim Mattson <jmattson@google.com>,
        Kees Cook <keescook@chromium.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        "H. Peter Anvin" <hpa@zytor.com>, Joerg Roedel <joro@8bytes.org>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH v3 05/13] KVM: x86: emulator: update the emulation mode
 after CR0 write
Message-ID: <YwaetYqmL4Wfr21I@google.com>
References: <20220803155011.43721-1-mlevitsk@redhat.com>
 <20220803155011.43721-6-mlevitsk@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220803155011.43721-6-mlevitsk@redhat.com>
Precedence: bulk

On Wed, Aug 03, 2022, Maxim Levitsky wrote:
> CR0.PE toggles real/protected mode, thus its update

Uber nit, I like using title case for Real Mode, Protected Mode, etc... so that
it's more obvious that a changelog/comment is referring to the architectural modes.

> should update the emulation mode.
> 
> This is likely a benign bug because there is no writeback
> of state, other than the RIP increment, and when toggling
> CR0.PE, the CPU has to execute code from a very low memory address.
> 
> Also CR0.PG toggle when EFER.LMA is set, toggles the long mode.

This last sentence is jumbled, and it probably fits better with the opening
sentence.  And it's technically EFER.LME; EFER.LMA=1 indicates the Long Mode is
fully active.  E.g. something like

  Update the emulation mode when handling writes to CR0, toggling CR0.PE switches
  between Real and Protected Mode, and toggling CR0.PG when EFER.LME=1 switches
  between Long and Protected Mode.

> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  arch/x86/kvm/emulate.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 5e91b26cc1d8aa..765ec65b2861ba 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -3658,11 +3658,23 @@ static int em_movbe(struct x86_emulate_ctxt *ctxt)
>  
>  static int em_cr_write(struct x86_emulate_ctxt *ctxt)
>  {
> -	if (ctxt->ops->set_cr(ctxt, ctxt->modrm_reg, ctxt->src.val))
> +	int cr_num = ctxt->modrm_reg;
> +	int r;
> +
> +	if (ctxt->ops->set_cr(ctxt, cr_num, ctxt->src.val))
>  		return emulate_gp(ctxt, 0);
>  
>  	/* Disable writeback. */
>  	ctxt->dst.type = OP_NONE;
> +
> +	if (cr_num == 0) {
> +		/* CR0 write might have updated CR0.PE and/or CR0.PG
> +		 * which can affect the cpu execution mode */

		/*
		 * Multi-line comment format should look like this.  I need more
		 * words to make this multiple lines.
		 */

> +		r = emulator_recalc_and_set_mode(ctxt);
> +		if (r != X86EMUL_CONTINUE)
> +			return r;
> +	}
> +
>  	return X86EMUL_CONTINUE;
>  }
>  
> -- 
> 2.26.3
>