LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Zeng Guang <guang.zeng@intel.com>
To: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	Vitaly Kuznetsov <vkuznets@redhat.com>,
	Wanpeng Li <wanpengli@tencent.com>,
	Jim Mattson <jmattson@google.com>, Joerg Roedel <joro@8bytes.org>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	"Luck, Tony" <tony.luck@intel.com>,
	Kan Liang <kan.liang@linux.intel.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	"H. Peter Anvin" <hpa@zytor.com>,
	Kim Phillips <kim.phillips@amd.com>,
	Jarkko Sakkinen <jarkko@kernel.org>,
	Jethro Beekman <jethro@fortanix.com>,
	"Huang, Kai" <kai.huang@intel.com>,
	"x86@kernel.org" <x86@kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Hu, Robert" <robert.hu@intel.com>,
	"Gao, Chao" <chao.gao@intel.com>
Subject: Re: [PATCH v4 5/6] KVM: x86: Support interrupt dispatch in x2APIC mode with APIC-write VM exit
Date: Sat, 18 Sep 2021 00:00:15 +0800	[thread overview]
Message-ID: <1d40cec2-d3ea-f5f8-d889-52026aea85de@intel.com> (raw)
In-Reply-To: <YTvcJZSd1KQvNmaz@google.com>

On 9/11/2021 6:28 AM, Sean Christopherson wrote:
> On Mon, Aug 09, 2021, Zeng Guang wrote:
>> Since IA x86 platform introduce features of IPI virtualization and
>> User Interrupts, new behavior applies to the execution of WRMSR ICR
> What do User Interrupts have to do with anything?

User interrupt will also use APIC-Write VM exit to emulate the MSR write 
process on vICR register.
May be better here to give a general description rather than list 
detailed feature samples.  Actually
this new behavior start to be common implementation in Intel platform now.

>> register that causes APIC-write VM exit instead of MSR-write VM exit
>> in x2APIC mode.
> Please lead with what support is actually being added, and more directly state
> what the new behavior actually is, e.g. when should KVM expect these types of
> traps.  The shortlog helps a bit, but APIC-write is somewhat ambiguous without
> the context that it refers to the trap-like exits, not exception-like exits on
> the WRMSR itself.

IIUC, APIC-write is always trap-like exits. I would give more clear and 
accurate description here as you suggested.

> Peeking ahead, this probably should be squashed with the next patch that adds
> IPI virtualizatio support.  Without that patch's code that disables ICR MSR
> intercepts for IPIv, this patch makes zero sense.
>
> I'm not totally opposed to splitting IPIv support into two patches, I just don't
> like splitting out this tiny subset that makes zero sense without the IPIv
> code/context.  I assume you took this approach so that the shortlog could be
> "KVM: VMX:" for the IPIv code.  IMO it's perfectly ok to keep that shortlog even
> though there are minor changes outside of vmx/.  VMX is the only user of
> kvm_apic_write_nodecode(), so it's not wrong to say it affects only VMX.

IMO, this patch targets to solve the mishandling in APIC-write VM exit 
if it presents 64-bit value writing to vICR in x2APIC mode.
Since Intel hardware with some new features will support such function, 
KVM have to enhance its solution and also be backward-compatible with
previous platform. I think it could be an independent fix patch.

>> This requires KVM to emulate writing 64-bit value to offset 300H on
>> the virtual-APIC page(VICR) for guest running in x2APIC mode when
> Maybe stylize that as vICR to make it stand out as virtual ICR?
OK.
>> APIC-wrtie VM exit occurs. Prevoisely KVM doesn't consider this
>         ^^^^^                 ^^^^^^^^^^
>         write                 Previously
Thanks for correction.
>> situation as CPU never produce APIC-write VM exit in x2APIC mode before.
>>
>> Signed-off-by: Zeng Guang <guang.zeng@intel.com>
>> ---
>>   arch/x86/kvm/lapic.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index ba5a27879f1d..0b0f0ce96679 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -2188,7 +2188,14 @@ void kvm_apic_write_nodecode(struct kvm_vcpu *vcpu, u32 offset)
>>   	/* hw has done the conditional check and inst decode */
>>   	offset &= 0xff0;
>>   
>> -	kvm_lapic_reg_read(vcpu->arch.apic, offset, 4, &val);
> Probably worth snapshotting vcpu->arch.apic.
>
>> +	if (apic_x2apic_mode(vcpu->arch.apic) && (offset == APIC_ICR)) {
>
> A comment here would be _extremely_ helpful.  IIUC, this path is reached when IPIv
> is enabled for all ICR writes that can't be virtualized, e.g. broadcast IPIs.
>
> And I'm tempted to say this should WARN and do nothing if KVM gets an exit on
> anything except ICR writes.
>
>> +		u64 icr_val = *((u64 *)(vcpu->arch.apic->regs + offset));
> Maybe just bump "val" to a u64?
>
> Rather than open code this, can't this be:
>
> 		kvm_lapic_reg_read(apic, offset, 8, &val);
>> +
>> +		kvm_lapic_reg_write(vcpu->arch.apic, APIC_ICR2, (u32)(icr_val>>32));
>> +		val = (u32)icr_val;
> Hmm, this is the third path that open codes the ICR2:ICR split.  I think it's
> probably worth adding a helper (patch below), and this can become:
>
> void kvm_apic_write_nodecode(struct kvm_vcpu *vcpu, u32 offset)
> {
> 	struct kvm_lapic *apic = vcpu->arch.apic;
> 	u64 val = 0;
>
> 	/* hw has done the conditional check and inst decode */
> 	offset &= 0xff0;
>
> 	/* TODO: optimize to just emulate side effect w/o one more write */
> 	if (apic_x2apic_mode(apic)) {
> 		if (WARN_ON_ONCE(offset != APIC_ICR))
> 			return 1;
>
> 		kvm_lapic_reg_read(apic, offset, 8, &val);
> 		kvm_lapic_reg_write64(apic, offset, val);
> 	} else {
> 		kvm_lapic_reg_read(apic, offset, 4, &val);
> 		kvm_lapic_reg_write(apic, offset, val);
> 	}
> }
>
> There is some risk my idea will backfire if the CPU traps other WRMSRs, but even
> then the pedant in me thinks the code for that should be:
>
>
> 	if (apic_x2apic_mode(apic)) {
> 		int size = offset == APIC_ICR ? 8 : 4;
>
> 		kvm_lapic_reg_read(apic, offset, size, &val);
> 		kvm_lapic_reg_write64(apic, offset, val);
> 	} else {
> 		...
> 	}
>
> or worst case scenario, move the APIC_ICR check back so that the non-ICR path
> back to "if (apic_x2apic_mode(vcpu->arch.apic) && (offset == APIC_ICR))" so that
> it naturally falls into the 4-byte read+write.
Only vICR in x2APIC mode is an exception to process 64-bit data 
instead.  That could be expensive to use
kvm_lapic_reg_read which will do many check unnecessarily specific for 
vICR, especially in case there are
intensive interrupts arising. Besides, kvm_lapic_reg_write64() requires 
caller to ensure 64-bit data containing
right ICR2 and ICR value along with respective apic mode. Otherwise it 
will corrupt APIC_ICR2 setting.

Probably it's better to optimize code as below. How do you think?

void kvm_apic_write_nodecode(struct kvm_vcpu *vcpu, u32 offset)
{
	struct kvm_lapic *apic = vcpu->arch.apic;
	u64 val = 0;

	/* hw has done the conditional check and inst decode */
	offset &= 0xff0;

	/* TODO: optimize to just emulate side effect w/o one more write */
	if ((offset == APIC_ICR) && apic_x2apic_mode(apic)) {
		val = *((u64 *)(apic->regs + offset));
		kvm_lapic_reg_write(apic, APIC_ICR2, (u32)(val>>32));
	 } else {
		kvm_lapic_reg_read(apic, offset, 4, &val);
	}
		
	kvm_lapic_reg_write(apic, offset, (u32)val);
  }

+ } else {
>> +		kvm_lapic_reg_read(vcpu->arch.apic, offset, 4, &val);
>> +	}
>>   
>>   	/* TODO: optimize to just emulate side effect w/o one more write */
>>   	kvm_lapic_reg_write(vcpu->arch.apic, offset, val);
>> -- 
>> 2.25.1
>
>  From c7641cf0c2ea2a1c5e6dda4007f8d285595ff82d Mon Sep 17 00:00:00 2001
> From: Sean Christopherson <seanjc@google.com>
> Date: Fri, 10 Sep 2021 15:07:57 -0700
> Subject: [PATCH] KVM: x86: Add a helper to handle 64-bit APIC writes to ICR
>
> Add a helper to handle 64-bit APIC writes, e.g. for x2APIC WRMSR, to
> deduplicate the handling of ICR writes, which KVM needs to emulate as
> back-to-back writes to ICR2 and then ICR.  Future support for IPI
> virtualization will add yet another path where KVM must handle a 64-bit
> APIC write.
>
> Opportunistically fix the comment; ICR2 holds the destination (if there's
> no shorthand), not the vector.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/kvm/lapic.c | 18 ++++++++++--------
>   1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 76fb00921203..5f526ee10301 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -2183,6 +2183,14 @@ void kvm_lapic_set_eoi(struct kvm_vcpu *vcpu)
>   }
>   EXPORT_SYMBOL_GPL(kvm_lapic_set_eoi);
>
> +static int kvm_lapic_reg_write64(struct kvm_lapic *apic, u32 reg, u64 data)
> +{
> +	/* For 64-bit ICR writes, set ICR2 (dest) before ICR (command). */
> +	if (reg == APIC_ICR)
> +		kvm_lapic_reg_write(apic, APIC_ICR2, (u32)(data >> 32));
> +	return kvm_lapic_reg_write(apic, reg, (u32)data);
> +}
> +
>   /* emulate APIC access in a trap manner */
>   void kvm_apic_write_nodecode(struct kvm_vcpu *vcpu, u32 offset)
>   {
> @@ -2794,10 +2802,7 @@ int kvm_x2apic_msr_write(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>   	if (reg == APIC_ICR2)
>   		return 1;
>
> -	/* if this is ICR write vector before command */
> -	if (reg == APIC_ICR)
> -		kvm_lapic_reg_write(apic, APIC_ICR2, (u32)(data >> 32));
> -	return kvm_lapic_reg_write(apic, reg, (u32)data);
> +	return kvm_lapic_reg_write64(apic, reg, data);
>   }
>
>   int kvm_x2apic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data)
> @@ -2828,10 +2833,7 @@ int kvm_hv_vapic_msr_write(struct kvm_vcpu *vcpu, u32 reg, u64 data)
>   	if (!lapic_in_kernel(vcpu))
>   		return 1;
>
> -	/* if this is ICR write vector before command */
> -	if (reg == APIC_ICR)
> -		kvm_lapic_reg_write(apic, APIC_ICR2, (u32)(data >> 32));
> -	return kvm_lapic_reg_write(apic, reg, (u32)data);
> +	return kvm_lapic_reg_write64(apic, reg, data);
>   }
>
>   int kvm_hv_vapic_msr_read(struct kvm_vcpu *vcpu, u32 reg, u64 *data)
> --
>

  reply	other threads:[~2021-09-17 16:00 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-09  3:29 [PATCH v4 0/6] IPI virtualization support for VM Zeng Guang
2021-08-09  3:29 ` [PATCH v4 1/6] x86/feat_ctl: Add new VMX feature, Tertiary VM-Execution control Zeng Guang
2021-09-10 21:25   ` Sean Christopherson
2021-09-17 16:10     ` Zeng Guang
2021-08-09  3:29 ` [PATCH v4 2/6] KVM: VMX: Extend BUILD_CONTROLS_SHADOW macro to support 64-bit variation Zeng Guang
2021-09-10 21:28   ` Sean Christopherson
2021-09-17 16:13     ` Zeng Guang
2021-08-09  3:29 ` [PATCH v4 3/6] KVM: VMX: Detect Tertiary VM-Execution control when setup VMCS config Zeng Guang
2021-09-10 21:35   ` Sean Christopherson
2021-09-17 16:15     ` Zeng Guang
2021-08-09  3:29 ` [PATCH v4 4/6] KVM: VMX: dump_vmcs() reports tertiary_exec_control field as well Zeng Guang
2021-08-09  3:29 ` [PATCH v4 5/6] KVM: x86: Support interrupt dispatch in x2APIC mode with APIC-write VM exit Zeng Guang
2021-09-10 22:28   ` Sean Christopherson
2021-09-17 16:00     ` Zeng Guang [this message]
2021-08-09  3:29 ` [PATCH v4 6/6] KVM: VMX: enable IPI virtualization Zeng Guang
2021-09-10 23:43   ` Sean Christopherson
2021-09-10 23:55     ` Sean Christopherson
2021-09-17 16:10       ` Zeng Guang
2021-10-14 18:56         ` Sean Christopherson
2021-09-17 16:00     ` Zeng Guang
2021-08-20 13:19 ` [PATCH v4 0/6] IPI virtualization support for VM Zeng Guang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1d40cec2-d3ea-f5f8-d889-52026aea85de@intel.com \
    --to=guang.zeng@intel.com \
    --cc=bp@alien8.de \
    --cc=chao.gao@intel.com \
    --cc=dave.hansen@linux.intel.com \
    --cc=hpa@zytor.com \
    --cc=jarkko@kernel.org \
    --cc=jethro@fortanix.com \
    --cc=jmattson@google.com \
    --cc=joro@8bytes.org \
    --cc=kai.huang@intel.com \
    --cc=kan.liang@linux.intel.com \
    --cc=kim.phillips@amd.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=robert.hu@intel.com \
    --cc=seanjc@google.com \
    --cc=tglx@linutronix.de \
    --cc=tony.luck@intel.com \
    --cc=vkuznets@redhat.com \
    --cc=wanpengli@tencent.com \
    --cc=x86@kernel.org \
    --subject='Re: [PATCH v4 5/6] KVM: x86: Support interrupt dispatch in x2APIC mode with APIC-write VM exit' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).