From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sean Christopherson Date: Thu, 28 Oct 2021 15:55:41 +0000 Subject: [PATCH v2 26/43] KVM: VMX: Read Posted Interrupt "control" exactly once per loop iteration In-Reply-To: References: <20211009021236.4122790-1-seanjc@google.com> <20211009021236.4122790-27-seanjc@google.com> Message-ID: List-Id: To: kvm-riscv@lists.infradead.org MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit On Thu, Oct 28, 2021, Maxim Levitsky wrote: > On Fri, 2021-10-08 at 19:12 -0700, Sean Christopherson wrote: > > Use READ_ONCE() when loading the posted interrupt descriptor control > > field to ensure "old" and "new" have the same base value. If the > > compiler emits separate loads, and loads into "new" before "old", KVM > > could theoretically drop the ON bit if it were set between the loads. > > > > Fixes: 28b835d60fcc ("KVM: Update Posted-Interrupts Descriptor when vCPU is preempted") > > Signed-off-by: Sean Christopherson > > --- > > arch/x86/kvm/vmx/posted_intr.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/arch/x86/kvm/vmx/posted_intr.c b/arch/x86/kvm/vmx/posted_intr.c > > index 414ea6972b5c..fea343dcc011 100644 > > --- a/arch/x86/kvm/vmx/posted_intr.c > > +++ b/arch/x86/kvm/vmx/posted_intr.c > > @@ -53,7 +53,7 @@ void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int cpu) > > > > /* The full case. */ > > do { > > - old.control = new.control = pi_desc->control; > > + old.control = new.control = READ_ONCE(pi_desc->control); > > > > dest = cpu_physical_id(cpu); > > > > @@ -104,7 +104,7 @@ static void __pi_post_block(struct kvm_vcpu *vcpu) > > "Wakeup handler not enabled while the vCPU was blocking"); > > > > do { > > - old.control = new.control = pi_desc->control; > > + old.control = new.control = READ_ONCE(pi_desc->control); > > > > dest = cpu_physical_id(vcpu->cpu); > > > > @@ -160,7 +160,7 @@ int pi_pre_block(struct kvm_vcpu *vcpu) > > "Posted Interrupt Suppress Notification set before blocking"); > > > > do { > > - old.control = new.control = pi_desc->control; > > + old.control = new.control = READ_ONCE(pi_desc->control); > > > > /* set 'NV' to 'wakeup vector' */ > > new.nv = POSTED_INTR_WAKEUP_VECTOR; > > I wish there was a way to mark fields in a struct, as requiring 'READ_ONCE' on them > so that compiler would complain if this isn't done, or automatically use 'READ_ONCE' > logic. Hmm, I think you could make an argument that ON and thus the whole "control" word should be volatile. AFAICT, tagging just "on" as volatile actually works. There's even in a clause in Documentation/process/volatile-considered-harmful.rst that calls this out as a (potentially) legitimate use case. - Pointers to data structures in coherent memory which might be modified by I/O devices can, sometimes, legitimately be volatile. That said, I think I actually prefer forcing the use of READ_ONCE. The descriptor requires more protections than what volatile provides, namely that all writes need to be atomic. So given that volatile alone isn't sufficient, I'd prefer to have the code itself be more self-documenting. E.g. this compiles and does mess up the expected size. diff --git a/arch/x86/kvm/vmx/posted_intr.h b/arch/x86/kvm/vmx/posted_intr.h index 7f7b2326caf5..149df3b18789 100644 --- a/arch/x86/kvm/vmx/posted_intr.h +++ b/arch/x86/kvm/vmx/posted_intr.h @@ -11,9 +11,9 @@ struct pi_desc { union { struct { /* bit 256 - Outstanding Notification */ - u16 on : 1, + volatile u16 on : 1; /* bit 257 - Suppress Notification */ - sn : 1, + u16 sn : 1, /* bit 271:258 - Reserved */ rsvd_1 : 14; /* bit 279:272 - Notification Vector */ @@ -23,7 +23,7 @@ struct pi_desc { /* bit 319:288 - Notification Destination */ u32 ndst; }; - u64 control; + volatile u64 control; }; u32 rsvd[6]; } __aligned(64); From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sean Christopherson Date: Thu, 28 Oct 2021 15:55:41 +0000 Subject: Re: [PATCH v2 26/43] KVM: VMX: Read Posted Interrupt "control" exactly once per loop iteration Message-Id: List-Id: References: <20211009021236.4122790-1-seanjc@google.com> <20211009021236.4122790-27-seanjc@google.com> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Maxim Levitsky Cc: Marc Zyngier , Huacai Chen , Aleksandar Markovic , Paul Mackerras , Anup Patel , Paul Walmsley , Palmer Dabbelt , Albert Ou , Christian Borntraeger , Janosch Frank , Paolo Bonzini , James Morse , Alexandru Elisei , Suzuki K Poulose , Atish Patra , David Hildenbrand , Cornelia Huck , Claudio Imbrenda , Vitaly Kuznetsov , Wanpeng Li , Jim Mattson , Joerg Roedel , linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, linux-mips@vger.kernel.org, kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, kvm-riscv@lists.infradead.org, linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org, David Matlack , Oliver Upton , Jing Zhang On Thu, Oct 28, 2021, Maxim Levitsky wrote: > On Fri, 2021-10-08 at 19:12 -0700, Sean Christopherson wrote: > > Use READ_ONCE() when loading the posted interrupt descriptor control > > field to ensure "old" and "new" have the same base value. If the > > compiler emits separate loads, and loads into "new" before "old", KVM > > could theoretically drop the ON bit if it were set between the loads. > > > > Fixes: 28b835d60fcc ("KVM: Update Posted-Interrupts Descriptor when vCPU is preempted") > > Signed-off-by: Sean Christopherson > > --- > > arch/x86/kvm/vmx/posted_intr.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/arch/x86/kvm/vmx/posted_intr.c b/arch/x86/kvm/vmx/posted_intr.c > > index 414ea6972b5c..fea343dcc011 100644 > > --- a/arch/x86/kvm/vmx/posted_intr.c > > +++ b/arch/x86/kvm/vmx/posted_intr.c > > @@ -53,7 +53,7 @@ void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int cpu) > > > > /* The full case. */ > > do { > > - old.control = new.control = pi_desc->control; > > + old.control = new.control = READ_ONCE(pi_desc->control); > > > > dest = cpu_physical_id(cpu); > > > > @@ -104,7 +104,7 @@ static void __pi_post_block(struct kvm_vcpu *vcpu) > > "Wakeup handler not enabled while the vCPU was blocking"); > > > > do { > > - old.control = new.control = pi_desc->control; > > + old.control = new.control = READ_ONCE(pi_desc->control); > > > > dest = cpu_physical_id(vcpu->cpu); > > > > @@ -160,7 +160,7 @@ int pi_pre_block(struct kvm_vcpu *vcpu) > > "Posted Interrupt Suppress Notification set before blocking"); > > > > do { > > - old.control = new.control = pi_desc->control; > > + old.control = new.control = READ_ONCE(pi_desc->control); > > > > /* set 'NV' to 'wakeup vector' */ > > new.nv = POSTED_INTR_WAKEUP_VECTOR; > > I wish there was a way to mark fields in a struct, as requiring 'READ_ONCE' on them > so that compiler would complain if this isn't done, or automatically use 'READ_ONCE' > logic. Hmm, I think you could make an argument that ON and thus the whole "control" word should be volatile. AFAICT, tagging just "on" as volatile actually works. There's even in a clause in Documentation/process/volatile-considered-harmful.rst that calls this out as a (potentially) legitimate use case. - Pointers to data structures in coherent memory which might be modified by I/O devices can, sometimes, legitimately be volatile. That said, I think I actually prefer forcing the use of READ_ONCE. The descriptor requires more protections than what volatile provides, namely that all writes need to be atomic. So given that volatile alone isn't sufficient, I'd prefer to have the code itself be more self-documenting. E.g. this compiles and does mess up the expected size. diff --git a/arch/x86/kvm/vmx/posted_intr.h b/arch/x86/kvm/vmx/posted_intr.h index 7f7b2326caf5..149df3b18789 100644 --- a/arch/x86/kvm/vmx/posted_intr.h +++ b/arch/x86/kvm/vmx/posted_intr.h @@ -11,9 +11,9 @@ struct pi_desc { union { struct { /* bit 256 - Outstanding Notification */ - u16 on : 1, + volatile u16 on : 1; /* bit 257 - Suppress Notification */ - sn : 1, + u16 sn : 1, /* bit 271:258 - Reserved */ rsvd_1 : 14; /* bit 279:272 - Notification Vector */ @@ -23,7 +23,7 @@ struct pi_desc { /* bit 319:288 - Notification Destination */ u32 ndst; }; - u64 control; + volatile u64 control; }; u32 rsvd[6]; } __aligned(64); From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E78D2C433EF for ; Thu, 28 Oct 2021 15:55:51 +0000 (UTC) Received: from mm01.cs.columbia.edu (mm01.cs.columbia.edu [128.59.11.253]) by mail.kernel.org (Postfix) with ESMTP id 6618A610D2 for ; Thu, 28 Oct 2021 15:55:51 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 6618A610D2 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.cs.columbia.edu Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id E53644B08D; Thu, 28 Oct 2021 11:55:50 -0400 (EDT) X-Virus-Scanned: at lists.cs.columbia.edu Authentication-Results: mm01.cs.columbia.edu (amavisd-new); dkim=softfail (fail, message has been altered) header.i=@google.com Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id ngtuSvCq0f-Q; Thu, 28 Oct 2021 11:55:49 -0400 (EDT) Received: from mm01.cs.columbia.edu (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id BCAA84B099; Thu, 28 Oct 2021 11:55:49 -0400 (EDT) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 920684A524 for ; Thu, 28 Oct 2021 11:55:48 -0400 (EDT) X-Virus-Scanned: at lists.cs.columbia.edu Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 0yudJ93wyBRo for ; Thu, 28 Oct 2021 11:55:47 -0400 (EDT) Received: from mail-pl1-f180.google.com (mail-pl1-f180.google.com [209.85.214.180]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id 401874A49C for ; Thu, 28 Oct 2021 11:55:47 -0400 (EDT) Received: by mail-pl1-f180.google.com with SMTP id t11so4715713plq.11 for ; Thu, 28 Oct 2021 08:55:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=m0tHnHlQJDBdfArgGfCXfVMbH9LdSSnDL35miUYeLjs=; b=QQmaJ8nXN2KbyIG5k1JyBoSquPg6yk3b/FNSx1V8PjEEIwsY4bZfol7aYj6Ws7u8lu AL3QNRVPk8G4o1zbT2wS3T+0q2unqIYIGvpD4X4hbS+GWLCfdCjK1582SOfCPxgbXaj6 rp1/zAb/gxsig9wW4x8Hmz3hVUCDwIHJtvbG8WTJj+OyXJ9Qbywk0hsQ9i5L1juwa1hs n78Jiehn21G0dcgi1vzqMRfnUHdXLWIscCjEk3VHPYnbJSs6c8My2YAhN/ZegigH9hDg LWqS5PZjO6n0Z4WJ0z7BuPmceVWRsCdNKwW5JM9MnEgXi3tzkHrjJO1A1KNXmTOVd/Xz cMxQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=m0tHnHlQJDBdfArgGfCXfVMbH9LdSSnDL35miUYeLjs=; b=YRhgCm+j8WhzTJN0EWFYXmyM/J5VQbfyhfRj98ZKyHYj47WXxQdReyTJ0SzJvKFq6+ S43TFGTKuJndezjNCuEIqAeJgzImUxBX7Nj9R3Z2DNDXWsgmvDPFpVU9vLkxRt4TzSZQ qALQ7zf8wxvJjO7vTnDb9i8PtPSSuAQWRdNQraYg4XiXJfDFSRgG8bGLYJznoqwZCZED Lnr2Oo8z8LVbe+8fXaOGd2b+09nDgWJ5ifFRcRko+5NtTvWyOZxIUUD10oeOQ2oUttnL S6B8G9N0DtomKiJHBwPSztUkH3DyPe4iR4tYgYb0I5yWj3Ac8KM0jnz9GVCau7bXEylm 5KhA== X-Gm-Message-State: AOAM5319NpXGBpB90n80J7001Zyn/BZMGMgxrRSxIav+VLPd84zUn2C8 G0fyuA38N+YeEdTGerS9pK2dTQ== X-Google-Smtp-Source: ABdhPJwGUzCGTG+bFEKCx4HxRTPn7WtM79GFmjO7irPTWNvcrtXMPEuDRtNZ53pGeMNDkz8HGfA6sw== X-Received: by 2002:a17:90b:17d0:: with SMTP id me16mr13884280pjb.152.1635436545938; Thu, 28 Oct 2021 08:55:45 -0700 (PDT) Received: from google.com (157.214.185.35.bc.googleusercontent.com. [35.185.214.157]) by smtp.gmail.com with ESMTPSA id s12sm3788591pfg.70.2021.10.28.08.55.45 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 28 Oct 2021 08:55:45 -0700 (PDT) Date: Thu, 28 Oct 2021 15:55:41 +0000 From: Sean Christopherson To: Maxim Levitsky Subject: Re: [PATCH v2 26/43] KVM: VMX: Read Posted Interrupt "control" exactly once per loop iteration Message-ID: References: <20211009021236.4122790-1-seanjc@google.com> <20211009021236.4122790-27-seanjc@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: Cc: Cornelia Huck , Wanpeng Li , kvm@vger.kernel.org, David Hildenbrand , linux-kernel@vger.kernel.org, Paul Mackerras , Atish Patra , linux-riscv@lists.infradead.org, Claudio Imbrenda , kvmarm@lists.cs.columbia.edu, Janosch Frank , Marc Zyngier , Joerg Roedel , Huacai Chen , Christian Borntraeger , Aleksandar Markovic , Albert Ou , kvm-ppc@vger.kernel.org, Paul Walmsley , David Matlack , linux-arm-kernel@lists.infradead.org, Jim Mattson , Anup Patel , linux-mips@vger.kernel.org, Palmer Dabbelt , kvm-riscv@lists.infradead.org, Paolo Bonzini , Vitaly Kuznetsov X-BeenThere: kvmarm@lists.cs.columbia.edu X-Mailman-Version: 2.1.14 Precedence: list List-Id: Where KVM/ARM decisions are made List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu On Thu, Oct 28, 2021, Maxim Levitsky wrote: > On Fri, 2021-10-08 at 19:12 -0700, Sean Christopherson wrote: > > Use READ_ONCE() when loading the posted interrupt descriptor control > > field to ensure "old" and "new" have the same base value. If the > > compiler emits separate loads, and loads into "new" before "old", KVM > > could theoretically drop the ON bit if it were set between the loads. > > > > Fixes: 28b835d60fcc ("KVM: Update Posted-Interrupts Descriptor when vCPU is preempted") > > Signed-off-by: Sean Christopherson > > --- > > arch/x86/kvm/vmx/posted_intr.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/arch/x86/kvm/vmx/posted_intr.c b/arch/x86/kvm/vmx/posted_intr.c > > index 414ea6972b5c..fea343dcc011 100644 > > --- a/arch/x86/kvm/vmx/posted_intr.c > > +++ b/arch/x86/kvm/vmx/posted_intr.c > > @@ -53,7 +53,7 @@ void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int cpu) > > > > /* The full case. */ > > do { > > - old.control = new.control = pi_desc->control; > > + old.control = new.control = READ_ONCE(pi_desc->control); > > > > dest = cpu_physical_id(cpu); > > > > @@ -104,7 +104,7 @@ static void __pi_post_block(struct kvm_vcpu *vcpu) > > "Wakeup handler not enabled while the vCPU was blocking"); > > > > do { > > - old.control = new.control = pi_desc->control; > > + old.control = new.control = READ_ONCE(pi_desc->control); > > > > dest = cpu_physical_id(vcpu->cpu); > > > > @@ -160,7 +160,7 @@ int pi_pre_block(struct kvm_vcpu *vcpu) > > "Posted Interrupt Suppress Notification set before blocking"); > > > > do { > > - old.control = new.control = pi_desc->control; > > + old.control = new.control = READ_ONCE(pi_desc->control); > > > > /* set 'NV' to 'wakeup vector' */ > > new.nv = POSTED_INTR_WAKEUP_VECTOR; > > I wish there was a way to mark fields in a struct, as requiring 'READ_ONCE' on them > so that compiler would complain if this isn't done, or automatically use 'READ_ONCE' > logic. Hmm, I think you could make an argument that ON and thus the whole "control" word should be volatile. AFAICT, tagging just "on" as volatile actually works. There's even in a clause in Documentation/process/volatile-considered-harmful.rst that calls this out as a (potentially) legitimate use case. - Pointers to data structures in coherent memory which might be modified by I/O devices can, sometimes, legitimately be volatile. That said, I think I actually prefer forcing the use of READ_ONCE. The descriptor requires more protections than what volatile provides, namely that all writes need to be atomic. So given that volatile alone isn't sufficient, I'd prefer to have the code itself be more self-documenting. E.g. this compiles and does mess up the expected size. diff --git a/arch/x86/kvm/vmx/posted_intr.h b/arch/x86/kvm/vmx/posted_intr.h index 7f7b2326caf5..149df3b18789 100644 --- a/arch/x86/kvm/vmx/posted_intr.h +++ b/arch/x86/kvm/vmx/posted_intr.h @@ -11,9 +11,9 @@ struct pi_desc { union { struct { /* bit 256 - Outstanding Notification */ - u16 on : 1, + volatile u16 on : 1; /* bit 257 - Suppress Notification */ - sn : 1, + u16 sn : 1, /* bit 271:258 - Reserved */ rsvd_1 : 14; /* bit 279:272 - Notification Vector */ @@ -23,7 +23,7 @@ struct pi_desc { /* bit 319:288 - Notification Destination */ u32 ndst; }; - u64 control; + volatile u64 control; }; u32 rsvd[6]; } __aligned(64); _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2134BC433F5 for ; Thu, 28 Oct 2021 15:56:02 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id D962060FC1 for ; Thu, 28 Oct 2021 15:56:01 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org D962060FC1 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=oOcXBfCXWEeXDAF8kcPN5TxHWGZWhxvjcrYpcJc18QM=; b=dFwzJG3KYWDm2w llLWYGnzPj67Q+qeobc4brYeEAbKg8l+0MZ7+SMPuJ7a+N4JNp8GnsY1k8SH3n1+fq5FAtG4QNAkP Wzi/+0tdcDeBNFvJHEk1FQZ1ddf1CXCJ+888w3ml1Dl3yE5Z60ZN+IXJ6j0dtvggRZ7+opdFRrf2g y/s/Dk65nRnkG+Hqdn4MCO7eCyXfMD/SzhEerjcVauYR3a57nA736JqBnKJb8MZIebH33Lft9Ds55 TsIHYPOfLVxGXxnk8nFIbebdSVg95Oq8F6Bm2OOFLwwKXnqZibRXySOOfvUqCgwx7ILjZ6kUFRkMx WKRYAXWb8ierhZteXk2A==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mg7ku-008UrN-0H; Thu, 28 Oct 2021 15:55:52 +0000 Received: from mail-pl1-x62e.google.com ([2607:f8b0:4864:20::62e]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mg7kp-008UpJ-6U for linux-riscv@lists.infradead.org; Thu, 28 Oct 2021 15:55:50 +0000 Received: by mail-pl1-x62e.google.com with SMTP id s24so4781228plp.0 for ; Thu, 28 Oct 2021 08:55:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=m0tHnHlQJDBdfArgGfCXfVMbH9LdSSnDL35miUYeLjs=; b=QQmaJ8nXN2KbyIG5k1JyBoSquPg6yk3b/FNSx1V8PjEEIwsY4bZfol7aYj6Ws7u8lu AL3QNRVPk8G4o1zbT2wS3T+0q2unqIYIGvpD4X4hbS+GWLCfdCjK1582SOfCPxgbXaj6 rp1/zAb/gxsig9wW4x8Hmz3hVUCDwIHJtvbG8WTJj+OyXJ9Qbywk0hsQ9i5L1juwa1hs n78Jiehn21G0dcgi1vzqMRfnUHdXLWIscCjEk3VHPYnbJSs6c8My2YAhN/ZegigH9hDg LWqS5PZjO6n0Z4WJ0z7BuPmceVWRsCdNKwW5JM9MnEgXi3tzkHrjJO1A1KNXmTOVd/Xz cMxQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=m0tHnHlQJDBdfArgGfCXfVMbH9LdSSnDL35miUYeLjs=; b=mbZy93sakptZ86TTvwh0DnHqYkSsTofDoKaIWxUjXjyxXgfAXaPSfanIHc3PAYzpx7 CJTMOPymWeNEbL7Wx2bhOA4GS68oSyuRjoRSfDZglwTAODbpw6eh+y+D1RFPW3F5eEy0 ZHN/Iah0tnosnEcQ2qKXFtahXe10Gd201SESMmHMDn6ZZkGygXIH9vVqRUiw+96Mz6On YQOcffAU8mvnverGTwiYNbiDHpR09uqtMg73ByLdS90/UQtDjdg1exJNrGRbsqGJZuOZ PIX2OVXVI1dcsIaxPjxiu731oaD1Qfg3g3XU5kTwSodENQ0iyDNtj2iy9vQ3hnyHddeF 8XuQ== X-Gm-Message-State: AOAM533pWwp+VwYGoR6eKdjqZnRQY/hHEi8vEhcO79WwSuUx5lxF4dAK yCe5T0O0DH0lmdkhcnyq145EIw== X-Google-Smtp-Source: ABdhPJwGUzCGTG+bFEKCx4HxRTPn7WtM79GFmjO7irPTWNvcrtXMPEuDRtNZ53pGeMNDkz8HGfA6sw== X-Received: by 2002:a17:90b:17d0:: with SMTP id me16mr13884280pjb.152.1635436545938; Thu, 28 Oct 2021 08:55:45 -0700 (PDT) Received: from google.com (157.214.185.35.bc.googleusercontent.com. [35.185.214.157]) by smtp.gmail.com with ESMTPSA id s12sm3788591pfg.70.2021.10.28.08.55.45 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 28 Oct 2021 08:55:45 -0700 (PDT) Date: Thu, 28 Oct 2021 15:55:41 +0000 From: Sean Christopherson To: Maxim Levitsky Cc: Marc Zyngier , Huacai Chen , Aleksandar Markovic , Paul Mackerras , Anup Patel , Paul Walmsley , Palmer Dabbelt , Albert Ou , Christian Borntraeger , Janosch Frank , Paolo Bonzini , James Morse , Alexandru Elisei , Suzuki K Poulose , Atish Patra , David Hildenbrand , Cornelia Huck , Claudio Imbrenda , Vitaly Kuznetsov , Wanpeng Li , Jim Mattson , Joerg Roedel , linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, linux-mips@vger.kernel.org, kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, kvm-riscv@lists.infradead.org, linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org, David Matlack , Oliver Upton , Jing Zhang Subject: Re: [PATCH v2 26/43] KVM: VMX: Read Posted Interrupt "control" exactly once per loop iteration Message-ID: References: <20211009021236.4122790-1-seanjc@google.com> <20211009021236.4122790-27-seanjc@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211028_085547_297250_6183B6D0 X-CRM114-Status: GOOD ( 26.49 ) X-BeenThere: linux-riscv@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-riscv" Errors-To: linux-riscv-bounces+linux-riscv=archiver.kernel.org@lists.infradead.org On Thu, Oct 28, 2021, Maxim Levitsky wrote: > On Fri, 2021-10-08 at 19:12 -0700, Sean Christopherson wrote: > > Use READ_ONCE() when loading the posted interrupt descriptor control > > field to ensure "old" and "new" have the same base value. If the > > compiler emits separate loads, and loads into "new" before "old", KVM > > could theoretically drop the ON bit if it were set between the loads. > > > > Fixes: 28b835d60fcc ("KVM: Update Posted-Interrupts Descriptor when vCPU is preempted") > > Signed-off-by: Sean Christopherson > > --- > > arch/x86/kvm/vmx/posted_intr.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/arch/x86/kvm/vmx/posted_intr.c b/arch/x86/kvm/vmx/posted_intr.c > > index 414ea6972b5c..fea343dcc011 100644 > > --- a/arch/x86/kvm/vmx/posted_intr.c > > +++ b/arch/x86/kvm/vmx/posted_intr.c > > @@ -53,7 +53,7 @@ void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int cpu) > > > > /* The full case. */ > > do { > > - old.control = new.control = pi_desc->control; > > + old.control = new.control = READ_ONCE(pi_desc->control); > > > > dest = cpu_physical_id(cpu); > > > > @@ -104,7 +104,7 @@ static void __pi_post_block(struct kvm_vcpu *vcpu) > > "Wakeup handler not enabled while the vCPU was blocking"); > > > > do { > > - old.control = new.control = pi_desc->control; > > + old.control = new.control = READ_ONCE(pi_desc->control); > > > > dest = cpu_physical_id(vcpu->cpu); > > > > @@ -160,7 +160,7 @@ int pi_pre_block(struct kvm_vcpu *vcpu) > > "Posted Interrupt Suppress Notification set before blocking"); > > > > do { > > - old.control = new.control = pi_desc->control; > > + old.control = new.control = READ_ONCE(pi_desc->control); > > > > /* set 'NV' to 'wakeup vector' */ > > new.nv = POSTED_INTR_WAKEUP_VECTOR; > > I wish there was a way to mark fields in a struct, as requiring 'READ_ONCE' on them > so that compiler would complain if this isn't done, or automatically use 'READ_ONCE' > logic. Hmm, I think you could make an argument that ON and thus the whole "control" word should be volatile. AFAICT, tagging just "on" as volatile actually works. There's even in a clause in Documentation/process/volatile-considered-harmful.rst that calls this out as a (potentially) legitimate use case. - Pointers to data structures in coherent memory which might be modified by I/O devices can, sometimes, legitimately be volatile. That said, I think I actually prefer forcing the use of READ_ONCE. The descriptor requires more protections than what volatile provides, namely that all writes need to be atomic. So given that volatile alone isn't sufficient, I'd prefer to have the code itself be more self-documenting. E.g. this compiles and does mess up the expected size. diff --git a/arch/x86/kvm/vmx/posted_intr.h b/arch/x86/kvm/vmx/posted_intr.h index 7f7b2326caf5..149df3b18789 100644 --- a/arch/x86/kvm/vmx/posted_intr.h +++ b/arch/x86/kvm/vmx/posted_intr.h @@ -11,9 +11,9 @@ struct pi_desc { union { struct { /* bit 256 - Outstanding Notification */ - u16 on : 1, + volatile u16 on : 1; /* bit 257 - Suppress Notification */ - sn : 1, + u16 sn : 1, /* bit 271:258 - Reserved */ rsvd_1 : 14; /* bit 279:272 - Notification Vector */ @@ -23,7 +23,7 @@ struct pi_desc { /* bit 319:288 - Notification Destination */ u32 ndst; }; - u64 control; + volatile u64 control; }; u32 rsvd[6]; } __aligned(64); _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9DE7BC433EF for ; Thu, 28 Oct 2021 15:57:07 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 6816360FC1 for ; Thu, 28 Oct 2021 15:57:07 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 6816360FC1 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=OURMBk6lVu0zXOsz7wL2QWDS0U5F+P6eNnUuXIhTIKs=; b=B10VlhMY+J6jJI dGiXnF6dLzL8BYRuDXFlC/F/T4Pw7m8JlJK/iTGlIc+S8BoUxkc4L19wCmiwA3oNMLG3Ht4CwFSAi GWtnVK5x9Nm0hOP2Txbp5zeFcvh+aL3eW6lE42nT9/RrbuqF8kVGU7DwVtV/myr1dfF5iqWifW61i 9Av2GW8uLP6R2VQF67Yp4KzVMgseu9xSdUny6iVd7ZCj3BEjMaUrw11JtBlE5YcbZTLSGHKm+wIP8 6PNRe0MkjTki0fZR9mdopgdrCAcxu1ABg71PkRk3BpwBGkFlYngocGJOxsoT4PH6JQZuJGDA66buL pPhag6h7n2spWMWQ9krA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mg7kx-008Uru-6h; Thu, 28 Oct 2021 15:55:55 +0000 Received: from mail-pj1-x1029.google.com ([2607:f8b0:4864:20::1029]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mg7kp-008UpI-6G for linux-arm-kernel@lists.infradead.org; Thu, 28 Oct 2021 15:55:50 +0000 Received: by mail-pj1-x1029.google.com with SMTP id gn3so5066671pjb.0 for ; Thu, 28 Oct 2021 08:55:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=m0tHnHlQJDBdfArgGfCXfVMbH9LdSSnDL35miUYeLjs=; b=QQmaJ8nXN2KbyIG5k1JyBoSquPg6yk3b/FNSx1V8PjEEIwsY4bZfol7aYj6Ws7u8lu AL3QNRVPk8G4o1zbT2wS3T+0q2unqIYIGvpD4X4hbS+GWLCfdCjK1582SOfCPxgbXaj6 rp1/zAb/gxsig9wW4x8Hmz3hVUCDwIHJtvbG8WTJj+OyXJ9Qbywk0hsQ9i5L1juwa1hs n78Jiehn21G0dcgi1vzqMRfnUHdXLWIscCjEk3VHPYnbJSs6c8My2YAhN/ZegigH9hDg LWqS5PZjO6n0Z4WJ0z7BuPmceVWRsCdNKwW5JM9MnEgXi3tzkHrjJO1A1KNXmTOVd/Xz cMxQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=m0tHnHlQJDBdfArgGfCXfVMbH9LdSSnDL35miUYeLjs=; b=f3crvp2DnP+XF+zwCXZtT9wFnleqHwQyhIez8d/1R5rbw4LTgXc/sbv7w3/Wu8oQ4i 7PCVSrPxCDiDw5jAW/BaZbyWuwn9uq/hslCoJdvA6TsILLjiiFcWnWnIv/zjN14amxtu pqf5iGYVNFRx+GV22IgkmNTw4HDSU2KVaoJFSf549K8/yExt0RjMp1E+FR24MLSRZo+T uRs8gkHNZTXQkpvkshvlsgY4KHuffrHkbJQ53eklOndezHyWZDV3QV4mANljXiYuT5za hsbhkfD539PkE3nXKVOJIe7kItRMyhc50qeXDLZBFasSJT2H4Lxvn3TMMG7CsHZe7zjn 53wA== X-Gm-Message-State: AOAM533aSAXW4bSu4aAC6OAw8cr4nLYF2TsQ+2LkIlZkbeLPQTs4HBJV znfMed6eDoiFufOAKFMM/hxmSg== X-Google-Smtp-Source: ABdhPJwGUzCGTG+bFEKCx4HxRTPn7WtM79GFmjO7irPTWNvcrtXMPEuDRtNZ53pGeMNDkz8HGfA6sw== X-Received: by 2002:a17:90b:17d0:: with SMTP id me16mr13884280pjb.152.1635436545938; Thu, 28 Oct 2021 08:55:45 -0700 (PDT) Received: from google.com (157.214.185.35.bc.googleusercontent.com. [35.185.214.157]) by smtp.gmail.com with ESMTPSA id s12sm3788591pfg.70.2021.10.28.08.55.45 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 28 Oct 2021 08:55:45 -0700 (PDT) Date: Thu, 28 Oct 2021 15:55:41 +0000 From: Sean Christopherson To: Maxim Levitsky Cc: Marc Zyngier , Huacai Chen , Aleksandar Markovic , Paul Mackerras , Anup Patel , Paul Walmsley , Palmer Dabbelt , Albert Ou , Christian Borntraeger , Janosch Frank , Paolo Bonzini , James Morse , Alexandru Elisei , Suzuki K Poulose , Atish Patra , David Hildenbrand , Cornelia Huck , Claudio Imbrenda , Vitaly Kuznetsov , Wanpeng Li , Jim Mattson , Joerg Roedel , linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, linux-mips@vger.kernel.org, kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, kvm-riscv@lists.infradead.org, linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org, David Matlack , Oliver Upton , Jing Zhang Subject: Re: [PATCH v2 26/43] KVM: VMX: Read Posted Interrupt "control" exactly once per loop iteration Message-ID: References: <20211009021236.4122790-1-seanjc@google.com> <20211009021236.4122790-27-seanjc@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211028_085547_283024_878A398A X-CRM114-Status: GOOD ( 28.07 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Oct 28, 2021, Maxim Levitsky wrote: > On Fri, 2021-10-08 at 19:12 -0700, Sean Christopherson wrote: > > Use READ_ONCE() when loading the posted interrupt descriptor control > > field to ensure "old" and "new" have the same base value. If the > > compiler emits separate loads, and loads into "new" before "old", KVM > > could theoretically drop the ON bit if it were set between the loads. > > > > Fixes: 28b835d60fcc ("KVM: Update Posted-Interrupts Descriptor when vCPU is preempted") > > Signed-off-by: Sean Christopherson > > --- > > arch/x86/kvm/vmx/posted_intr.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/arch/x86/kvm/vmx/posted_intr.c b/arch/x86/kvm/vmx/posted_intr.c > > index 414ea6972b5c..fea343dcc011 100644 > > --- a/arch/x86/kvm/vmx/posted_intr.c > > +++ b/arch/x86/kvm/vmx/posted_intr.c > > @@ -53,7 +53,7 @@ void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int cpu) > > > > /* The full case. */ > > do { > > - old.control = new.control = pi_desc->control; > > + old.control = new.control = READ_ONCE(pi_desc->control); > > > > dest = cpu_physical_id(cpu); > > > > @@ -104,7 +104,7 @@ static void __pi_post_block(struct kvm_vcpu *vcpu) > > "Wakeup handler not enabled while the vCPU was blocking"); > > > > do { > > - old.control = new.control = pi_desc->control; > > + old.control = new.control = READ_ONCE(pi_desc->control); > > > > dest = cpu_physical_id(vcpu->cpu); > > > > @@ -160,7 +160,7 @@ int pi_pre_block(struct kvm_vcpu *vcpu) > > "Posted Interrupt Suppress Notification set before blocking"); > > > > do { > > - old.control = new.control = pi_desc->control; > > + old.control = new.control = READ_ONCE(pi_desc->control); > > > > /* set 'NV' to 'wakeup vector' */ > > new.nv = POSTED_INTR_WAKEUP_VECTOR; > > I wish there was a way to mark fields in a struct, as requiring 'READ_ONCE' on them > so that compiler would complain if this isn't done, or automatically use 'READ_ONCE' > logic. Hmm, I think you could make an argument that ON and thus the whole "control" word should be volatile. AFAICT, tagging just "on" as volatile actually works. There's even in a clause in Documentation/process/volatile-considered-harmful.rst that calls this out as a (potentially) legitimate use case. - Pointers to data structures in coherent memory which might be modified by I/O devices can, sometimes, legitimately be volatile. That said, I think I actually prefer forcing the use of READ_ONCE. The descriptor requires more protections than what volatile provides, namely that all writes need to be atomic. So given that volatile alone isn't sufficient, I'd prefer to have the code itself be more self-documenting. E.g. this compiles and does mess up the expected size. diff --git a/arch/x86/kvm/vmx/posted_intr.h b/arch/x86/kvm/vmx/posted_intr.h index 7f7b2326caf5..149df3b18789 100644 --- a/arch/x86/kvm/vmx/posted_intr.h +++ b/arch/x86/kvm/vmx/posted_intr.h @@ -11,9 +11,9 @@ struct pi_desc { union { struct { /* bit 256 - Outstanding Notification */ - u16 on : 1, + volatile u16 on : 1; /* bit 257 - Suppress Notification */ - sn : 1, + u16 sn : 1, /* bit 271:258 - Reserved */ rsvd_1 : 14; /* bit 279:272 - Notification Vector */ @@ -23,7 +23,7 @@ struct pi_desc { /* bit 319:288 - Notification Destination */ u32 ndst; }; - u64 control; + volatile u64 control; }; u32 rsvd[6]; } __aligned(64); _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel