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:   Thu, 10 Aug 2023 07:21:03 +0800
From:   Yan Zhao <yan.y.zhao@intel.com>
To:     Sean Christopherson <seanjc@google.com>
CC:     Like Xu <like.xu.linux@gmail.com>, <kvm@vger.kernel.org>,
        <intel-gfx@lists.freedesktop.org>, <linux-kernel@vger.kernel.org>,
        "Zhenyu Wang" <zhenyuw@linux.intel.com>,
        Ben Gardon <bgardon@google.com>,
        "Paolo Bonzini" <pbonzini@redhat.com>,
        <intel-gvt-dev@lists.freedesktop.org>,
        "Zhi Wang" <zhi.a.wang@intel.com>
Subject: Re: [PATCH 19/27] KVM: x86/mmu: Use page-track notifiers iff there
 are external users
Message-ID: <ZNQfX4JHTJu1Qtl0@yzhao56-desk.sh.intel.com>
Reply-To: Yan Zhao <yan.y.zhao@intel.com>
References: <20221223005739.1295925-1-seanjc@google.com>
 <20221223005739.1295925-20-seanjc@google.com>
 <5581418b-2e1c-6011-f0a4-580df7e00b44@gmail.com>
 <ZNEni2XZuwiPgqaC@google.com>
 <ZNLlseYag5DniUg3@yzhao56-desk.sh.intel.com>
 <ZNOjyf2OHQZYfMEJ@google.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <ZNOjyf2OHQZYfMEJ@google.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?OGkKG0ljQNkJXLnuz3O2vr9U0rswy/xmBd5cgOyiyMo1VUp6l3kTzZpV6N1b?=
 =?us-ascii?Q?Oj90H5Ap15c1h0y9J9e26OxxUtZWDKrgez/HTxz8QBtDDVIInrRka+qC1Jh6?=
 =?us-ascii?Q?AgQX6d/2kg0gXEq+WAQAx+DwCHGPfGuD5qDRdiZxLOZe9GWQCsPdwuXZET/O?=
 =?us-ascii?Q?4KfqdBgJcnTdNLSj/XoGttWIudH3/0sP0gYfcuztHBvKnwCkOVH0eK9HWFg5?=
 =?us-ascii?Q?a0OroxuBjVClFpiOtXtes4ISsqJM+yw4/Ns8kpaDDKREteppmGWOUyZC1hRK?=
 =?us-ascii?Q?zxew2GjDbWpCkT4h43RP6PFzKQKOwBW7jrwEOs8QgVGellVxnV8/ApEUWKkh?=
 =?us-ascii?Q?aQH6nVyxKhvY2YXFR22LTURAyRQhQvQ4ggUOqdShsDs+3cmLjLn3R6LaIB/V?=
 =?us-ascii?Q?8aHsVXDPQHEagd8Cop5WK6X8XADuQw3RrXN+swRezIFM3bdow+Q4yJ7Q0rp2?=
 =?us-ascii?Q?GOfKtBZ2EP0UVrQ+K/Sj9kkiTn6kMuWxI3ACvojQ0Dft6v0FZnApG58WEAmR?=
 =?us-ascii?Q?yIbf4B4Qx5yU/uQ7J9u8kO0uR8RnshZ7xifXyMrYBOXTGHAbnQsnlmTqJFwK?=
 =?us-ascii?Q?b40/y4QhFQnobH5arm+0KKA4oJc6dhMA4jcu20YyJ9RfILco0tpW4fWAhCWc?=
 =?us-ascii?Q?+Qx3HxRZfoDu0StgGIuhEU4Um3OXetQY88bk23WGd8D87VCrSy3Avdt3iiat?=
 =?us-ascii?Q?6l2NuvS4EDQ5nD8Z7JFjSf/4iMjMOmsIfmQrxwsDb98kFlooNyHVBbYgYsBD?=
 =?us-ascii?Q?aP1WTFhn+/sQRiLiBKBJ1ojENfnmbOvFzFIrpcES7I9+qM4Ua6HDgzWV09Hg?=
 =?us-ascii?Q?YaQAVyBroiPkPC3ZELZWWM3QqXt3sZ5Gi24zVKhS7u2yV9b0HXeRUJ03Jjg/?=
 =?us-ascii?Q?s+HGl+B1qkej7rTdr6wNkKydVvvHlUG4slzlFYS0qi0bmob5c3x9iMoATidV?=
 =?us-ascii?Q?IjnBBIsA4cODF3byOlMLIDzBhYQnyRJOL7pcQwjuRf4VDirY/Tw7aztRYG2F?=
 =?us-ascii?Q?AxhwaMyFVS9FkoWfHvUONHseWwJ1U/5UTIvSLkcyMPYqrDw5YDMGrpBaZE9x?=
 =?us-ascii?Q?Yl3+uk4K1vWO1sI66RBXUL6JqANDmwc4eHSuWI1lboYu9yAH+K2IO+QKevTj?=
 =?us-ascii?Q?UCKYryVh4oXHpwn2MfzTW0Rp20DE2Ur034Eyrtsl6Q86coBrnCrm7FwxuK4q?=
 =?us-ascii?Q?u0qiZjpmju/p4jM1MR2FVS/NXheOWquCv0D+RJi5JkxgTSfnHdiAkpSvKwL3?=
 =?us-ascii?Q?GGdPt9t4aqQUvVTUloojFWyBmqUUYclOMCmPx4PCn1bcydahLdn2kb95kSZF?=
 =?us-ascii?Q?hLb1dhw5jBD08X/eu9GHt21bsUV3drThFLjpa4cyEhH1JtHUNe/nTALeKr6v?=
 =?us-ascii?Q?kwce7PCuZ2KCta60UnqYm63eUyAaDnDAl87hrnYKBvtE59ix3Ope3vc6/VUQ?=
 =?us-ascii?Q?rNzQFoAc5uZEit/3qs+haauGTtYQLQb/sptB/+BXMyGCFFR3lwCjrXQsWz9e?=
 =?us-ascii?Q?lfAXLVKuGjt/U/P/AEdC0VVmB68s2O/i62iUpYNyKuSR4eQwKcxW/b1N4Z9d?=
 =?us-ascii?Q?ILhKimuYtcvWOHC+O5iETNqJI16ej5LFtnVVQJGN?=
X-MS-Exchange-CrossTenant-Network-Message-Id: e121b4f4-fcf4-4bfd-8dfb-08db99331070
X-MS-Exchange-CrossTenant-AuthSource: DS7PR11MB5966.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Aug 2023 23:48:00.6107
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: l8bkJDqtkjrZXhxC+5OIy583Nb1krETQtCm7w8tVhnevdu9MmbRgX2XPSfanXxO3njw6EilnFpw0fLZiHHOcOw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB7901
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE,URIBL_BLOCKED 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 Wed, Aug 09, 2023 at 07:33:45AM -0700, Sean Christopherson wrote:
> On Wed, Aug 09, 2023, Yan Zhao wrote:
> > On Mon, Aug 07, 2023 at 10:19:07AM -0700, Sean Christopherson wrote:
> > > On Mon, Aug 07, 2023, Like Xu wrote:
> > > > On 23/12/2022 8:57 am, Sean Christopherson wrote:
> > > > > +static inline void kvm_page_track_write(struct kvm_vcpu *vcpu, gpa_t gpa,
> > > > > +					const u8 *new, int bytes)
> > > > > +{
> > > > > +	__kvm_page_track_write(vcpu, gpa, new, bytes);
> > > > > +
> > > > > +	kvm_mmu_track_write(vcpu, gpa, new, bytes);
> > > > > +}
> > > > 
> > > > The kvm_mmu_track_write() is only used for x86, where the incoming parameter
> > > > "u8 *new" has not been required since 0e0fee5c539b ("kvm: mmu: Fix race in
> > > > emulated page table writes"), please help confirm if it's still needed ? Thanks.
> > > > A minor clean up is proposed.
> > > 
> > > Hmm, unless I'm misreading things, KVMGT ultimately doesn't consume @new either.
> > > So I think we can remove @new from kvm_page_track_write() entirely.
> > Sorry for the late reply.
> > Yes, KVMGT does not consume @new and it reads the guest PTE again in the
> > page track write handler.
> > 
> > But I have a couple of questions related to the memtioned commit as
> > below:
> > 
> > (1) If "re-reading the current value of the guest PTE after the MMU lock has
> > been acquired", then should KVMGT also acquire the MMU lock too?
> 
> No.  If applicable, KVMGT should read the new/current value after acquiring
> whatever lock protects the generation (or update) of the shadow entries.  I
> suspect KVMGT already does this, but I don't have time to confirm that at this
I think the mutex lock and unlock of info->vgpu_lock you added in
kvmgt_page_track_write() is the counterpart :)

> exact memory.
> 
> The race that was fixed in KVM was:
> 
>   vCPU0         vCPU1   
>   write X
>                  write Y
>                  sync SPTE w/ Y
>   sync SPTE w/ X
> 
> Reading the value after acquiring mmu_lock ensures that both vCPUs will see whatever
> value "loses" the race, i.e. whatever written value is processed second ('Y' in the
> above sequence).
I suspect that vCPU0 may still generate a wrong SPTE if vCPU1 wrote 4
bytes while vCPU0 wrote 8 bytes, though the chances are very low.


> 
> > If so, could we move the MMU lock and unlock into kvm_page_track_write()
> > as it's common.
> > 
> > (2) Even if KVMGT consumes @new,
> > will kvm_page_track_write() be called for once or twice if there are two
> > concurent emulated write?
> 
> Twice, kvm_page_track_write() is wired up directly to the emulation of the write,
> i.e. there is no batching.