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;
Message-ID: <3b7194b7-4616-0585-9a4a-91d42eebea9a@redhat.com>
Date:   Mon, 5 Jun 2023 20:27:30 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH] KVM: x86/mmu: Remove KVM MMU write lock when accessing
 indirect_shadow_pages
Content-Language: en-US
To:     Ben Gardon <bgardon@google.com>, Jim Mattson <jmattson@google.com>
Cc:     Mingwei Zhang <mizhang@google.com>,
        Sean Christopherson <seanjc@google.com>,
        "H. Peter Anvin" <hpa@zytor.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20230605004334.1930091-1-mizhang@google.com>
 <CALMp9eSQgcKd=SN4q2QRYbveKoayKzuYEQPM0Xu+FgQ_Mja8-g@mail.gmail.com>
 <CANgfPd9kKxq1146F3mX_u7KCC0HrWfgYrxZd6c9Dh7s19E4Eog@mail.gmail.com>
From:   Paolo Bonzini <pbonzini@redhat.com>
In-Reply-To: <CANgfPd9kKxq1146F3mX_u7KCC0HrWfgYrxZd6c9Dh7s19E4Eog@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 6/5/23 19:17, Ben Gardon wrote:
>>> -               if (indirect_shadow_pages)
>>> +               if (READ_ONCE(vcpu->kvm->arch.indirect_shadow_pages))
>> I don't understand the need for READ_ONCE() here. That implies that
>> there is something tricky going on, and I don't think that's the case.
> Agree this doesn't need a READ_ONCE. Just a read is fine.
> kvm_mmu_unprotect_page starts by acquiring the MMU lock, so there's
> not much room to reorder anything anyway.

I agree that "there's not much room to reorder anything", but it's not a 
particularly formal/reassuring statement :) and READ_ONCE/WRITE_ONCE 
have documentation value too.

Or maybe it's because I've become used to the C11 memory model.  Either 
way, I like the idea to use READ_ONCE/WRITE_ONCE more explicitly 
whenever there can be concurrent accesses (which would be data races in 
the C11 model), and I think Linux is moving in that direction too.

Paolo