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:   Mon, 9 Jan 2023 16:32:57 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Maxim Levitsky <mlevitsk@redhat.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, Marc Orr <marcorr@google.com>,
        Ben Gardon <bgardon@google.com>,
        Venkatesh Srinivas <venkateshs@chromium.org>
Subject: Re: [PATCH 5/6] KVM: VMX: Always intercept accesses to unsupported
 "extended" x2APIC regs
Message-ID: <Y7xBuYBYCfEgS9sI@google.com>
References: <20230107011025.565472-1-seanjc@google.com>
 <20230107011025.565472-6-seanjc@google.com>
 <5e50b87a4c7d19f9386bac1aa7061675018a2caa.camel@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <5e50b87a4c7d19f9386bac1aa7061675018a2caa.camel@redhat.com>
Precedence: bulk

On Sun, Jan 08, 2023, Maxim Levitsky wrote:
> On Sat, 2023-01-07 at 01:10 +0000, Sean Christopherson wrote:
> >  static void vmx_update_msr_bitmap_x2apic(struct kvm_vcpu *vcpu)
> >  {
> > +	/*
> > +	 * x2APIC indices for 64-bit accesses into the RDMSR and WRMSR halves
> > +	 * of the MSR bitmap.  KVM emulates APIC registers up through 0x3f0,
> > +	 * i.e. MSR 0x83f, and so only needs to dynamically manipulate 64 bits.
> > +	 */
> The above comment is better to be placed down below, near the actual write,
> otherwise it is confusing.

Can you elaborate on why it's confusing?  The intent of this specific comment is
to capture why the index calculations use BITS_PER_LONG_LONG and sizeof(u64).

> > +	const int read_idx = APIC_BASE_MSR / BITS_PER_LONG_LONG;
> > +	const int write_idx = read_idx + (0x800 / sizeof(u64));
> >  	struct vcpu_vmx *vmx = to_vmx(vcpu);
> > +	u64 *msr_bitmap = (u64 *)vmx->vmcs01.msr_bitmap;
> >  	u8 mode;
> >  
> >  	if (!cpu_has_vmx_msr_bitmap())
> > @@ -4058,7 +4049,18 @@ static void vmx_update_msr_bitmap_x2apic(struct kvm_vcpu *vcpu)
> >  
> >  	vmx->x2apic_msr_bitmap_mode = mode;
> >  
> > -	vmx_reset_x2apic_msrs(vcpu, mode);
> > +	/*
> > +	 * Reset the bitmap for MSRs 0x800 - 0x83f.  Leave AMD's uber-extended
> > +	 * registers (0x840 and above) intercepted, KVM doesn't support them.
> 
> I don't think AMD calls them uber-extended. Just extended.

Yeah, I took some creative liberaties.  I want to avoid confusion with the more
common use of Extended APIC (x2APIC).