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:   Fri, 16 Jun 2023 15:37:29 +0800
From:   Yan Zhao <yan.y.zhao@intel.com>
To:     Yuan Yao <yuan.yao@linux.intel.com>
CC:     <kvm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <pbonzini@redhat.com>, <seanjc@google.com>, <chao.gao@intel.com>,
        <kai.huang@intel.com>, <robert.hoo.linux@gmail.com>
Subject: Re: [PATCH v3 09/11] KVM: x86/mmu: serialize vCPUs to zap gfn when
 guest MTRRs are honored
Message-ID: <ZIwROWti5d0sCFwT@yzhao56-desk.sh.intel.com>
Reply-To: Yan Zhao <yan.y.zhao@intel.com>
References: <20230616023101.7019-1-yan.y.zhao@intel.com>
 <20230616023945.7570-1-yan.y.zhao@intel.com>
 <20230616074550.g2ikzbni2rjy7dfw@yy-desk-7060>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230616074550.g2ikzbni2rjy7dfw@yy-desk-7060>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?5E6G7mJozrkNT1fJfohaexcUWCmKCI5NUrDBcU4y2Vd7EtI60jPR0D86k5bH?=
 =?us-ascii?Q?T7b+E5tMu9lsA8UUEt7dG1EoKxnNhVzc0z0dwCOdpl2kkQyxIfRSyli0u6kl?=
 =?us-ascii?Q?r+cZffndBAk6yHRBhc2TgzHyZ9Hw7mu2JhOGDm+k1mWmxQro6j44JvgmaH/N?=
 =?us-ascii?Q?s/f0aoMKyNFtuLMJRvHe6yMZjqq+2qFTJBk7/T3QpY/W2NXPVxIWug6GQ17l?=
 =?us-ascii?Q?DM9RuizBmU1pgY7Z/Yr373y3biUnqfoBWEPeDNkIb7bKYgpV/6B0/yqoF1EW?=
 =?us-ascii?Q?bYjIQ5dr/GzKqS/oZLQbk324qlsRUPBIl3Qkb6Aj6FcQ5H3bPpuqapsYYPks?=
 =?us-ascii?Q?fAILY5JdUWNRYY/aQADVlRctAWdxtLNGfX76uEpcxtBdqUCmNuftwLF1hvfj?=
 =?us-ascii?Q?y/zlwhJtLa3iq6pnzbp8K0TA3nBSpzA8jdFmBYcuPSj7Nyxmk9wij8OqGHYk?=
 =?us-ascii?Q?zqF1ownvtBRmr31a0YCBdRNHszHs+2eBY3288P65Ctt9pS+lLh/Au9+IyVym?=
 =?us-ascii?Q?YnLLpvvkrI3OFTt82UTE2FwXjB28nkX1CYnnI+nLjqWzzjFbHFDwhUCcgIUy?=
 =?us-ascii?Q?R0+HcxyoBGmmq7zHM6vDXrVVOLjSg8xs2ShE0nVxqij/D82CF9DHiAMvGJU9?=
 =?us-ascii?Q?4OyBJ4U8u7X1nnN1xF9fMXEfLIdJTffQrTAxcO4RnzWsxQj3T5pt4RCFKxRT?=
 =?us-ascii?Q?N9Qr7u5VG+mKsObgqt6fWyBzZo1vOwi32KpTqIt6GmKdJqSRUKQzpojjI7Ad?=
 =?us-ascii?Q?bzjo67bqOqR1gJvm4aSoNJY1m2raxDKqjbAgguGpxhGWmgc4q5u3iTxZZgzq?=
 =?us-ascii?Q?maZZXlmzzTciXKnPW37D8OLh1RFzCigtIiuApAwiNmvgUW0rWhXnnRb91uwb?=
 =?us-ascii?Q?czN7qi/cIpXTD9KlS+RwW3mwwpAJVDSN9LgElsyAYhPaTXWvUyxB4JSPT3SM?=
 =?us-ascii?Q?f6BuM/1e/VJffLNbkeheXRejlD4q0Pj6/RcI+mJ8dOufRRgM/LucnIH5nFCa?=
 =?us-ascii?Q?rkwP20+yHC8dWF0jdiIKKfwAutw4OY/daJNfXXUTHKHOk35+XsEm4ADYQkcr?=
 =?us-ascii?Q?ZFBKeISH1B03rtzz0m0cDvCeYSlRMuLGiEjNnO2zQiYExsDAVK6rXTO1vOWB?=
 =?us-ascii?Q?+53IpnkBdjrhPadOHtSKYyiPWzn9Fwr00HQc6/0/45d0FGtIPYiVmwv+bqz2?=
 =?us-ascii?Q?5s8JlfVqk1xelTov5XZdxYPMeajsVuJqrq6bLWomKC+VqDtEI0osmuy49qQX?=
 =?us-ascii?Q?khLji5TdTLNzYEkA7IP1wbrFLC8EPp5mJS60DRRn6DkBYYeULLYKGcuw+qCT?=
 =?us-ascii?Q?cNHHhmeDrB5bvTFQjqcgJ3rMBrI6v8Qwj/fFCC2Csu0e9bT4eSnIKDDI9eeA?=
 =?us-ascii?Q?2ja0tv11gxOao5ZEBLB5JPJBcChoFdPJ45I6CO0HBMlIXPOxCMUZs6YK5nuQ?=
 =?us-ascii?Q?ZGAUH9wTodmPI/xIcnp6OM4APwgojyQw7u/aqliBm2zaZLWKcf1NsyH/8rJR?=
 =?us-ascii?Q?Q6w2dAVg+JZOhcnlh05Gi6/R35bH4dMGHPc37Tg5NuxczUPeI6G9NIWE9nyW?=
 =?us-ascii?Q?whK//YQsLe31O8Z+l04VC4Q9CRV0Jh0p3LfZEW2w?=
X-MS-Exchange-CrossTenant-Network-Message-Id: bee7f926-bc03-4e34-ad0b-08db6e4011ae
X-MS-Exchange-CrossTenant-AuthSource: DS7PR11MB5966.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Jun 2023 08:02:46.5979
 (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: hNf6Gjtn4GNmeoK+Wh3tV95nXm3yH1NvCWGQR6rad3dHALONMtX9Czd9oreEEp6m8mil9UhYaESYBBQEBhKQSg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR11MB4881
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_MSPIKE_H2,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE,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 Fri, Jun 16, 2023 at 03:45:50PM +0800, Yuan Yao wrote:
> > +/*
> > + * Add @range into kvm->arch.mtrr_zap_list and sort the list in
> > + * "length" ascending + "start" descending order, so that
> > + * ranges consuming more zap cycles can be dequeued later and their
> > + * chances of being found duplicated are increased.
> > + */
> > +static void kvm_add_mtrr_zap_list(struct kvm *kvm, struct mtrr_zap_range *range)
> > +{
> > +	struct list_head *head = &kvm->arch.mtrr_zap_list;
> > +	u64 len = range->end - range->start;
> > +	struct mtrr_zap_range *cur, *n;
> > +	bool added = false;
> > +
> > +	spin_lock(&kvm->arch.mtrr_zap_list_lock);
> > +
> > +	if (list_empty(head)) {
> > +		list_add(&range->node, head);
> > +		spin_unlock(&kvm->arch.mtrr_zap_list_lock);
> > +		return;
> > +	}
> > +
> > +	list_for_each_entry_safe(cur, n, head, node) {
> > +		u64 cur_len = cur->end - cur->start;
> > +
> > +		if (len < cur_len)
> > +			break;
> > +
> > +		if (len > cur_len)
> > +			continue;
> > +
> > +		if (range->start > cur->start)
> > +			break;
> > +
> > +		if (range->start < cur->start)
> > +			continue;
> > +
> > +		/* equal len & start, no need to add */
> > +		added = true;
> 
> Possible/worth to ignore the range already covered
> by queued range ?

I may not get you correctly, but
the "added" here means an queued range with exactly same start + len
found, so free and drop adding the new range here.

> 
> > +		kfree(range);
> > +		break;
> > +	}
> > +
> > +	if (!added)
> > +		list_add_tail(&range->node, &cur->node);
> > +
> > +	spin_unlock(&kvm->arch.mtrr_zap_list_lock);
> > +}
> > +
> > +static void kvm_zap_mtrr_zap_list(struct kvm *kvm)
> > +{
> > +	struct list_head *head = &kvm->arch.mtrr_zap_list;
> > +	struct mtrr_zap_range *cur = NULL;
> > +
> > +	spin_lock(&kvm->arch.mtrr_zap_list_lock);
> > +
> > +	while (!list_empty(head)) {
> > +		u64 start, end;
> > +
> > +		cur = list_first_entry(head, typeof(*cur), node);
> > +		start = cur->start;
> > +		end = cur->end;
> > +		list_del(&cur->node);
> > +		kfree(cur);
> > +		spin_unlock(&kvm->arch.mtrr_zap_list_lock);
> > +
> > +		kvm_zap_gfn_range(kvm, start, end);
> > +
> > +		spin_lock(&kvm->arch.mtrr_zap_list_lock);
> > +	}
> > +
> > +	spin_unlock(&kvm->arch.mtrr_zap_list_lock);
> > +}
> > +
> > +static void kvm_zap_or_wait_mtrr_zap_list(struct kvm *kvm)
> > +{
> > +	if (atomic_cmpxchg_acquire(&kvm->arch.mtrr_zapping, 0, 1) == 0) {
> > +		kvm_zap_mtrr_zap_list(kvm);
> > +		atomic_set_release(&kvm->arch.mtrr_zapping, 0);
> > +		return;
> > +	}
> > +
> > +	while (atomic_read(&kvm->arch.mtrr_zapping))
> > +		cpu_relax();
> > +}
> > +
> > +static void kvm_mtrr_zap_gfn_range(struct kvm_vcpu *vcpu,
> > +				   gfn_t gfn_start, gfn_t gfn_end)
> > +{
> > +	struct mtrr_zap_range *range;
> > +
> > +	range = kmalloc(sizeof(*range), GFP_KERNEL_ACCOUNT);
> > +	if (!range)
> > +		goto fail;
> > +
> > +	range->start = gfn_start;
> > +	range->end = gfn_end;
> > +
> > +	kvm_add_mtrr_zap_list(vcpu->kvm, range);
> > +
> > +	kvm_zap_or_wait_mtrr_zap_list(vcpu->kvm);
> > +	return;
> > +
> > +fail:
> > +	kvm_clear_mtrr_zap_list(vcpu->kvm);
> A very small chance race condition that incorrectly
> clear the queued ranges which have not been zapped by another thread ?
> Like below:
> 
> Thread A                         |  Thread B
> kvm_add_mtrr_zap_list()          |
>                                  |  kvm_clear_mtrr_zap_list()
> kvm_zap_or_wait_mtrr_zap_list()  |
> 
> Call kvm_clear_mtrr_zap_list() here looks unnecessary, other
> threads(B here) who put thing in the queue will take care them well.

> > +   kvm_zap_gfn_range(vcpu->kvm, gfn_start, gfn_end); 

Yes, if gfn_start and gfn_end here are not 0 and ~0ULL, the
kvm_clear_mtrr_zap_list() is not necessary.
Though in reality, they are always 0-~0ULL, I agree dropping the
kvm_clear_mtrr_zap_list() here is better.

Thanks!