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 B1EBFC433F5 for ; Sun, 31 Oct 2021 22:48:43 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 90E8260F22 for ; Sun, 31 Oct 2021 22:48:43 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230476AbhJaWvO (ORCPT ); Sun, 31 Oct 2021 18:51:14 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:34742 "EHLO us-smtp-delivery-124.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230316AbhJaWvN (ORCPT ); Sun, 31 Oct 2021 18:51:13 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1635720520; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=IAXDzBRARqGuWFKHNo+aw3J6+tC4cDZIOgfc+O9TnKM=; b=KXf8KnsELmuZ68+O4xGbHDKP39F/ob3GelkkJxFi755vN7e9wqF+ePCZhqxdPO6sBszYU9 hT+52st9P5rDrHcPtgu3+ehl9/g5cVvarto9lr4UZCE0O0wM6sQaXgUJBO+pU9WCgoxHVU UEUE/6OfNKltvSic9RP5pxmurnoDBog= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-182-DA7Ib6_UPm-Qx5l3Quy1qw-1; Sun, 31 Oct 2021 18:48:35 -0400 X-MC-Unique: DA7Ib6_UPm-Qx5l3Quy1qw-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 95B171006AA2; Sun, 31 Oct 2021 22:48:31 +0000 (UTC) Received: from starship (unknown [10.40.194.243]) by smtp.corp.redhat.com (Postfix) with ESMTP id 03B4019C79; Sun, 31 Oct 2021 22:48:18 +0000 (UTC) Message-ID: <20a17d75855dfb9bd496466fcd9f14baab0b2bda.camel@redhat.com> Subject: Re: [PATCH v2 26/43] KVM: VMX: Read Posted Interrupt "control" exactly once per loop iteration From: Maxim Levitsky To: Sean Christopherson 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 Date: Mon, 01 Nov 2021 00:48:17 +0200 In-Reply-To: References: <20211009021236.4122790-1-seanjc@google.com> <20211009021236.4122790-27-seanjc@google.com> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.36.5 (3.36.5-2.fc32) MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 2021-10-28 at 15:55 +0000, Sean Christopherson wrote: > 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. I took a look at how READ_ONCE/WRITE_ONCE is implemented and indeed they use volatile (the comment above __READ_ONCE is worth gold...), so there is a bit of contradiction: volatile-considered-harmful.rst states not to mark struct members volatile since you usually need more that than (very true often) and yet, I also heard that READ_ONCE/WRITE_ONCE is very encouraged to be used to fields that are used in lockless algorithms, even when not strictly needed, so why not to just mark the field and then use it normally? I guess that explicit READ_ONCE/WRITE_ONCE is much more readable/visible that a volatile in some header file. Anyway this isn't something I am going to argue about or push to be changed, just something I thought about. Best regards, Maxim Levitsky > > 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 8F505C433F5 for ; Sun, 31 Oct 2021 22:49:03 +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 420F260C49 for ; Sun, 31 Oct 2021 22:49:03 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 420F260C49 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.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:MIME-Version:References:In-Reply-To: Date:Cc:To:From:Subject:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=qd+hYc8ZLZvFKvzdfPnhN0+9qulP9ApbakeXbR3NWzo=; b=jXXsY+AtZc3/r6 lK3HqDbts4lUooBso4JXLGLgn7NGaB3ggDgLxZ4Nslf/vIN+SbQQdZ6JSvw9tuI4hgM5BWpFJVofA ghIDZEMn4m0o9dyF8PXObINnQ0jwcFJwqlyY9HHMK5D4jyqV0OW3D1zJYvMFbVxl3K4hlG1Y0hAq4 WqxttXoucX7WOg2Cg+Xo6WNDOg1UVKj+0LBsOOHbFqY8ybgSUadTwo8j4MSCVuJq8b/rCoqB7V1Nx h/g3Jp7ZM/k4UWqCouXWGoM5kuUsm5UUwXEUdK7jC4XisfeLUryKmqFI5jkDnJiGowHgmYVnjtL48 jAu0BJT3F43XEQrRMOJA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mhJdI-00Eqi8-QF; Sun, 31 Oct 2021 22:48:56 +0000 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mhJd3-00Eqej-HY for linux-riscv@lists.infradead.org; Sun, 31 Oct 2021 22:48:43 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1635720520; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=IAXDzBRARqGuWFKHNo+aw3J6+tC4cDZIOgfc+O9TnKM=; b=KXf8KnsELmuZ68+O4xGbHDKP39F/ob3GelkkJxFi755vN7e9wqF+ePCZhqxdPO6sBszYU9 hT+52st9P5rDrHcPtgu3+ehl9/g5cVvarto9lr4UZCE0O0wM6sQaXgUJBO+pU9WCgoxHVU UEUE/6OfNKltvSic9RP5pxmurnoDBog= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-182-DA7Ib6_UPm-Qx5l3Quy1qw-1; Sun, 31 Oct 2021 18:48:35 -0400 X-MC-Unique: DA7Ib6_UPm-Qx5l3Quy1qw-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 95B171006AA2; Sun, 31 Oct 2021 22:48:31 +0000 (UTC) Received: from starship (unknown [10.40.194.243]) by smtp.corp.redhat.com (Postfix) with ESMTP id 03B4019C79; Sun, 31 Oct 2021 22:48:18 +0000 (UTC) Message-ID: <20a17d75855dfb9bd496466fcd9f14baab0b2bda.camel@redhat.com> Subject: Re: [PATCH v2 26/43] KVM: VMX: Read Posted Interrupt "control" exactly once per loop iteration From: Maxim Levitsky To: Sean Christopherson 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 Date: Mon, 01 Nov 2021 00:48:17 +0200 In-Reply-To: References: <20211009021236.4122790-1-seanjc@google.com> <20211009021236.4122790-27-seanjc@google.com> User-Agent: Evolution 3.36.5 (3.36.5-2.fc32) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211031_154841_672879_DEA9D131 X-CRM114-Status: GOOD ( 35.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, 2021-10-28 at 15:55 +0000, Sean Christopherson wrote: > 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. I took a look at how READ_ONCE/WRITE_ONCE is implemented and indeed they use volatile (the comment above __READ_ONCE is worth gold...), so there is a bit of contradiction: volatile-considered-harmful.rst states not to mark struct members volatile since you usually need more that than (very true often) and yet, I also heard that READ_ONCE/WRITE_ONCE is very encouraged to be used to fields that are used in lockless algorithms, even when not strictly needed, so why not to just mark the field and then use it normally? I guess that explicit READ_ONCE/WRITE_ONCE is much more readable/visible that a volatile in some header file. Anyway this isn't something I am going to argue about or push to be changed, just something I thought about. Best regards, Maxim Levitsky > > 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 B4C88C433F5 for ; Sun, 31 Oct 2021 22:50:21 +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 705BE60EFF for ; Sun, 31 Oct 2021 22:50:21 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 705BE60EFF Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.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:MIME-Version:References:In-Reply-To: Date:Cc:To:From:Subject:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=iQI/lfHEyMztlqJLnBnADpLZScfuxWwnZdTQc4JgLFs=; b=ilo6Q1TH4CyQrv ZyKcXOv9spDWTzLfsDJ/kM6u/D8/FYI31GPqkmrIyZY1lY9F8hKHzsd3oMOuDrbJ+Dds11rgdo42X OEjoH0QDMzsvRTzxx1sYNwfY1SbTXqDD+hTLdodTzYJxIOOjXkBeV0Jb0fUOKQH81397zL/pz0/Rf FpUd1xebGtZlGt0ma2cE5CyMhlmdHIwmDTrAqmCRdk6p5WLScVeJD+7Wg/UezyGNKIYGSGurfL9jF MQdHp1xBerva2sj9QbZ5ce6Nzpaw7NoorUihbKvna6Y+K02mDEpUr+mLA16uNSTT1k1c5TqsVajse K3LcfwZiwOFi49pmM68g==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mhJd6-00Eqfa-4N; Sun, 31 Oct 2021 22:48:44 +0000 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mhJd1-00EqeI-Mb for linux-arm-kernel@lists.infradead.org; Sun, 31 Oct 2021 22:48:41 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1635720518; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=IAXDzBRARqGuWFKHNo+aw3J6+tC4cDZIOgfc+O9TnKM=; b=WZrVsXUCnklhCn/rlKfhnZ/5G+v5AYBCQM62cu5MutD1b9HPYF+XBXH5YTAthtACvRpzNP YIo03gHrAlOqbBcBboCHUd6q5DnkUQWlUycBsQL2DCv9Gf49U8zb1d2+VNQZhlBKVHB9sv BhVcHoODp/EHloorSKlyz/5BzuxeLEY= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-182-DA7Ib6_UPm-Qx5l3Quy1qw-1; Sun, 31 Oct 2021 18:48:35 -0400 X-MC-Unique: DA7Ib6_UPm-Qx5l3Quy1qw-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 95B171006AA2; Sun, 31 Oct 2021 22:48:31 +0000 (UTC) Received: from starship (unknown [10.40.194.243]) by smtp.corp.redhat.com (Postfix) with ESMTP id 03B4019C79; Sun, 31 Oct 2021 22:48:18 +0000 (UTC) Message-ID: <20a17d75855dfb9bd496466fcd9f14baab0b2bda.camel@redhat.com> Subject: Re: [PATCH v2 26/43] KVM: VMX: Read Posted Interrupt "control" exactly once per loop iteration From: Maxim Levitsky To: Sean Christopherson 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 Date: Mon, 01 Nov 2021 00:48:17 +0200 In-Reply-To: References: <20211009021236.4122790-1-seanjc@google.com> <20211009021236.4122790-27-seanjc@google.com> User-Agent: Evolution 3.36.5 (3.36.5-2.fc32) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211031_154839_931158_15A667CA X-CRM114-Status: GOOD ( 36.40 ) 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, 2021-10-28 at 15:55 +0000, Sean Christopherson wrote: > 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. I took a look at how READ_ONCE/WRITE_ONCE is implemented and indeed they use volatile (the comment above __READ_ONCE is worth gold...), so there is a bit of contradiction: volatile-considered-harmful.rst states not to mark struct members volatile since you usually need more that than (very true often) and yet, I also heard that READ_ONCE/WRITE_ONCE is very encouraged to be used to fields that are used in lockless algorithms, even when not strictly needed, so why not to just mark the field and then use it normally? I guess that explicit READ_ONCE/WRITE_ONCE is much more readable/visible that a volatile in some header file. Anyway this isn't something I am going to argue about or push to be changed, just something I thought about. Best regards, Maxim Levitsky > > 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 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 3E310C433FE for ; Mon, 1 Nov 2021 21:40:25 +0000 (UTC) Received: from mm01.cs.columbia.edu (mm01.cs.columbia.edu [128.59.11.253]) by mail.kernel.org (Postfix) with ESMTP id E94DF60EE3 for ; Mon, 1 Nov 2021 21:40:24 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org E94DF60EE3 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.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 9BC354B213; Mon, 1 Nov 2021 17:40:24 -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=@redhat.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 cdGOfzxnp6RP; Mon, 1 Nov 2021 17:40:23 -0400 (EDT) Received: from mm01.cs.columbia.edu (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 611E34B267; Mon, 1 Nov 2021 17:40:14 -0400 (EDT) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 270F64B0C2 for ; Sun, 31 Oct 2021 18:48:42 -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 mD96XJrjvgLa for ; Sun, 31 Oct 2021 18:48:40 -0400 (EDT) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mm01.cs.columbia.edu (Postfix) with ESMTP id DDFDF4B0C0 for ; Sun, 31 Oct 2021 18:48:40 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1635720520; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=IAXDzBRARqGuWFKHNo+aw3J6+tC4cDZIOgfc+O9TnKM=; b=KXf8KnsELmuZ68+O4xGbHDKP39F/ob3GelkkJxFi755vN7e9wqF+ePCZhqxdPO6sBszYU9 hT+52st9P5rDrHcPtgu3+ehl9/g5cVvarto9lr4UZCE0O0wM6sQaXgUJBO+pU9WCgoxHVU UEUE/6OfNKltvSic9RP5pxmurnoDBog= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-182-DA7Ib6_UPm-Qx5l3Quy1qw-1; Sun, 31 Oct 2021 18:48:35 -0400 X-MC-Unique: DA7Ib6_UPm-Qx5l3Quy1qw-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 95B171006AA2; Sun, 31 Oct 2021 22:48:31 +0000 (UTC) Received: from starship (unknown [10.40.194.243]) by smtp.corp.redhat.com (Postfix) with ESMTP id 03B4019C79; Sun, 31 Oct 2021 22:48:18 +0000 (UTC) Message-ID: <20a17d75855dfb9bd496466fcd9f14baab0b2bda.camel@redhat.com> Subject: Re: [PATCH v2 26/43] KVM: VMX: Read Posted Interrupt "control" exactly once per loop iteration From: Maxim Levitsky To: Sean Christopherson Date: Mon, 01 Nov 2021 00:48:17 +0200 In-Reply-To: References: <20211009021236.4122790-1-seanjc@google.com> <20211009021236.4122790-27-seanjc@google.com> User-Agent: Evolution 3.36.5 (3.36.5-2.fc32) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Mailman-Approved-At: Mon, 01 Nov 2021 17:40:12 -0400 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, 2021-10-28 at 15:55 +0000, Sean Christopherson wrote: > 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. I took a look at how READ_ONCE/WRITE_ONCE is implemented and indeed they use volatile (the comment above __READ_ONCE is worth gold...), so there is a bit of contradiction: volatile-considered-harmful.rst states not to mark struct members volatile since you usually need more that than (very true often) and yet, I also heard that READ_ONCE/WRITE_ONCE is very encouraged to be used to fields that are used in lockless algorithms, even when not strictly needed, so why not to just mark the field and then use it normally? I guess that explicit READ_ONCE/WRITE_ONCE is much more readable/visible that a volatile in some header file. Anyway this isn't something I am going to argue about or push to be changed, just something I thought about. Best regards, Maxim Levitsky > > 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 From: Maxim Levitsky Date: Sun, 31 Oct 2021 22:48:17 +0000 Subject: Re: [PATCH v2 26/43] KVM: VMX: Read Posted Interrupt "control" exactly once per loop iteration Message-Id: <20a17d75855dfb9bd496466fcd9f14baab0b2bda.camel@redhat.com> 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: Sean Christopherson 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, 2021-10-28 at 15:55 +0000, Sean Christopherson wrote: > 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. I took a look at how READ_ONCE/WRITE_ONCE is implemented and indeed they use volatile (the comment above __READ_ONCE is worth gold...), so there is a bit of contradiction: volatile-considered-harmful.rst states not to mark struct members volatile since you usually need more that than (very true often) and yet, I also heard that READ_ONCE/WRITE_ONCE is very encouraged to be used to fields that are used in lockless algorithms, even when not strictly needed, so why not to just mark the field and then use it normally? I guess that explicit READ_ONCE/WRITE_ONCE is much more readable/visible that a volatile in some header file. Anyway this isn't something I am going to argue about or push to be changed, just something I thought about. Best regards, Maxim Levitsky > > 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); >