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, 14 Nov 2022 16:53:12 -0700
From:   Greg Edwards <gedwards@ddn.com>
To:     Sean Christopherson <seanjc@google.com>
Cc:     kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        Paolo Bonzini <pbonzini@redhat.com>,
        Maxim Levitsky <mlevitsk@redhat.com>
Subject: Re: [PATCH] KVM: x86: Allow APICv APIC ID inhibit to be cleared on
 legacy kernels
Message-ID: <Y3LU6Eg4UaaI6bXu@bobdog.home.arpa>
References: <20221114202037.254176-1-gedwards@ddn.com>
 <Y3KzarZ5xzExFrj9@google.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y3KzarZ5xzExFrj9@google.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?zY0XHb8rPF59e5egcQzSNDZkQjolqxfmTz+NqsLnE7vtQOMKRazEkQNKNS61?=
 =?us-ascii?Q?F07esf12EyEvr+xEIi/cfSXs3eMvoSc/s2mV6rb9PUDp0PifG9bK5jgf6Yt0?=
 =?us-ascii?Q?g2gJPsvV1zN0AKQrlIn3b4WW8mfdhiCD5l1Gog9cbrFzmRLSj7LK45QVrEix?=
 =?us-ascii?Q?ln/k2xkLlcPBdF4PbFxuu0HIPtTzj95PlwK6BPY1EiQgRfqDnJtUXToTN0IR?=
 =?us-ascii?Q?QXR32uxFIj1u7VV4VbIaXNNQ8ZqJ7eWHdas2JQaseCBGMn38RrPpOCZLual9?=
 =?us-ascii?Q?Nef8S6ldB6o+39ukD8yPjEQ61XfsYPH0diJiTL8qUp1vso2pNeHoxliM6VHn?=
 =?us-ascii?Q?5H0I9JHEKmajyVUx54Xs4HutbG0KIGdPpo/63utJhHKzcSf9dtpYRms5fko1?=
 =?us-ascii?Q?Vo1sm9SLAUnJWY7k3BT2E+OLNcGX/FehSZJ05dIhrgoDrNBBZe3q6eoBBbiF?=
 =?us-ascii?Q?2dMLmb8Cdp3FhKmLUTceaRShuxaSfK+hZiJRDW4VfY3OUZE4LlvzlcTrQt6r?=
 =?us-ascii?Q?HDuWxF2OuxamO0bPD65rChoPrraaKNvNz7tkfiIPkMPI8hv/y4FXNiM8QJDv?=
 =?us-ascii?Q?mucSQi1lpU5/nGTRdTSl4Sr4lYj+iTQ9C5RVd/DsepLBEsmmWSXZFH2wKd7G?=
 =?us-ascii?Q?76WwcXHz5H3SBKLtk5ARjr7fZ6AqOFiir3NeYbta8Zt1MrvD0Z7g4wtmCC3r?=
 =?us-ascii?Q?59QftubL/f3+yScWLH4xLAus+OoJPTiNY3AEBqVrujzLRYtz9syxLsrlbbey?=
 =?us-ascii?Q?KESAMiLvhs3Y9niwEllebFTmjeLtTUQIpcuEs86JPolhOuKXK0EmmNv9xVb3?=
 =?us-ascii?Q?zSuTkT9C8k5adpAI/wbdrLtXKRqaqnZZQpqe8ES8SvBZ7x+Z0tA2AhhY18Qc?=
 =?us-ascii?Q?/XM3Kp7QWTc5UYlY9mCVODtxWfE5t+ov1DWEYytOjss3wGVlJprXcNrNvZZy?=
 =?us-ascii?Q?M3H2me193Z7qZxJnLx0Em5molrx3B2+hBsKxDiS8lmQjTBkk8w9UyzG3m8ZQ?=
 =?us-ascii?Q?oE+396FtwBfRIaetMkIPHiezYgNNhTTo4KoThORUBM2CRD3hzv7gQCaqzwDH?=
 =?us-ascii?Q?Yl7z8WJDgQEeXlbnIjoYbmKixueN/Tt3mi7Z5ug3owBrm2HcW2HR5CF0Sred?=
 =?us-ascii?Q?d/qVq6xOezTkBoJGGElpiCHFNlCKqdKLJYjVhwcCb7pUk/HJvllz0nB0vydH?=
 =?us-ascii?Q?NYXVAmAPWUk5WlBo4WV/ZNkFLWhSWmVFLluaHqeW65jCztC4bLptUoBom6VD?=
 =?us-ascii?Q?P93Br4lBZjmgsLGZTYy2adJviAwerNJ0O3Uls7Cmz2p0tAzou/1cwWJmZpgW?=
 =?us-ascii?Q?l9HJgbwieWuu8/cFTE73H0D7EgmEGkE5zDHojeLFnDLk5FBkQt7BNLCk3pXv?=
 =?us-ascii?Q?n6EJlMIhtuG0WlNEVNYIcEV4+Gpet0o2eI8XnppMYXHoter42NhTMX1P7UTu?=
 =?us-ascii?Q?XjMuO3B4FyNVQd5obtkaIlVzsXccoh2GZQhidodrHuHxvsAZ8/mVcXHP07sl?=
 =?us-ascii?Q?PaxhvtG5qkuD2hvN+A7wdpWRfy0BnLQQx5R9Mx8i0B/adUJi31tq1LkPdbII?=
 =?us-ascii?Q?TH142RfsQpqZipP/l8UmCxdRv4CQObipkM/tW2It5xzoZt91izBWEvgB1SY0?=
 =?us-ascii?Q?IQ=3D=3D?=
X-OriginatorOrg: ddn.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6a9a2337-7545-40bc-786f-08dac69b6616
X-MS-Exchange-CrossTenant-AuthSource: MN0PR19MB6407.namprd19.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Nov 2022 23:53:24.4330
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 753b6e26-6fd3-43e6-8248-3f1735d59bb4
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: RlYTMkVhQq2CUKW/UWN/j5yEFnxqoCIxqenNH+sGE85vXD30i8XXZyVwVujj9dFn0hJZrBksnlPzP0JyTqK4rQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY3PR19MB4852
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, Nov 14, 2022 at 09:30:18PM +0000, Sean Christopherson wrote:
> On Mon, Nov 14, 2022, Greg Edwards wrote:
>> Legacy kernels prior to commit 4399c03c6780 ("x86/apic: Remove
>> verify_local_APIC()") write the xAPIC ID of the boot CPU twice to verify
>> a functioning local APIC.  This results in APIC acceleration inhibited
>> on these kernels for reason APICV_INHIBIT_REASON_APIC_ID_MODIFIED.
>>
>> Allow the APICV_INHIBIT_REASON_APIC_ID_MODIFIED inhibit reason to be
>> cleared if/when the xAPIC ID is set back to the expected vcpu_id value.
>> This occurs on the second xAPIC ID write in verify_local_APIC().
>>
>> Fixes: 3743c2f02517 ("KVM: x86: inhibit APICv/AVIC on changes to APIC ID or APIC base")
>> Signed-off-by: Greg Edwards <gedwards@ddn.com>
>> ---
>>  arch/x86/kvm/lapic.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index d7639d126e6c..4064d0af094d 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -2075,8 +2075,13 @@ static void kvm_lapic_xapic_id_updated(struct kvm_lapic *apic)
>>  	if (KVM_BUG_ON(apic_x2apic_mode(apic), kvm))
>>  		return;
>>
>> -	if (kvm_xapic_id(apic) == apic->vcpu->vcpu_id)
>> +	if (kvm_xapic_id(apic) == apic->vcpu->vcpu_id) {
>> +		/* Legacy kernels prior to 4399c03c6780 write APIC ID twice. */
>> +		if (!kvm_apicv_activated(kvm))
>> +			kvm_clear_apicv_inhibit(kvm,
>> +					APICV_INHIBIT_REASON_APIC_ID_MODIFIED);
>
> This sadly doesn't work because the inhibit is per-VM, i.e. will do the wrong
> thing if there are still vCPU's with different APIC IDs.

That is true.  Thanks for pointing that out.

> Does skipping the check if the APIC is disabled help[*]?  At a glance, I can't
> tell if the APIC is enabled/disabled at that point in time.  It's not a true fix,
> but it's a lot easier to backport if it remedies the issue.

It does not.  I did find your patch series when I started investigating
this, but the behavior was the same on that series.

> For a proper fix, this entire path should be moved to kvm_recalculate_apic_map()
> so that can can safely toggle the inhibit, e.g. the recalc helper already deals
> with multiple vCPUs changing their APIC state in parallel.  I don't think the fix
> will be too difficult to craft such that it's backport friendly, but it would need
> to be slotted into the series containing the aforementioned fix.

Thank you for the suggestion.  I will take a look, and start from your
series.