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 X-Spam-Level: X-Spam-Status: No, score=-13.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id CD12AC433ED for ; Thu, 15 Apr 2021 15:59:39 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (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 786786117A for ; Thu, 15 Apr 2021 15:59:39 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 786786117A Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=xen.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from list by lists.xenproject.org with outflank-mailman.111381.213020 (Exim 4.92) (envelope-from ) id 1lX4Oq-0001yo-T2; Thu, 15 Apr 2021 15:59:24 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 111381.213020; Thu, 15 Apr 2021 15:59:24 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1lX4Oq-0001yh-PF; Thu, 15 Apr 2021 15:59:24 +0000 Received: by outflank-mailman (input) for mailman id 111381; Thu, 15 Apr 2021 15:59:23 +0000 Received: from us1-rack-iad1.inumbo.com ([172.99.69.81]) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1lX4Op-0001yc-ME for xen-devel@lists.xenproject.org; Thu, 15 Apr 2021 15:59:23 +0000 Received: from deinos.phlegethon.org (unknown [2001:41d0:8:b1d7::1]) by us1-rack-iad1.inumbo.com (Halon) with ESMTPS id ffbe45f5-159c-409b-9c96-d90ce670d948; Thu, 15 Apr 2021 15:59:22 +0000 (UTC) Received: from tjd by deinos.phlegethon.org with local (Exim 4.92.3 (FreeBSD)) (envelope-from ) id 1lX4Ol-0008Kk-BQ; Thu, 15 Apr 2021 15:59:19 +0000 X-BeenThere: xen-devel@lists.xenproject.org List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list Sender: "Xen-devel" X-Inumbo-ID: ffbe45f5-159c-409b-9c96-d90ce670d948 Date: Thu, 15 Apr 2021 16:59:19 +0100 From: Tim Deegan To: Jan Beulich Cc: "xen-devel@lists.xenproject.org" , George Dunlap , Andrew Cooper , Wei Liu , Roger Pau =?iso-8859-1?Q?Monn=E9?= Subject: Re: [PATCH] x86/shadow: adjust callback arrays Message-ID: References: <621aa6f6-d7f8-25eb-9aeb-f181a9cb3bbc@suse.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <621aa6f6-d7f8-25eb-9aeb-f181a9cb3bbc@suse.com> X-SA-Known-Good: Yes X-SA-Exim-Connect-IP: X-SA-Exim-Mail-From: tim@xen.org X-SA-Exim-Scanned: No (on deinos.phlegethon.org); SAEximRunCond expanded to false At 12:42 +0200 on 12 Apr (1618231332), Jan Beulich wrote: > Some of them have entries with stale comments. Rather than correcting > these comments, re-arrange how these arrays get populated, shrinking > their sizes at the same time (by omitting trailing NULL entries: Use > dedicated element initializers, serving the purpose of what the > comments did so far. This then also makes these arrays independent of > the actual ordering of the individual SH_type_*. > > While tightening respective ASSERT()s in hash_{vcpu,domain}_foreach(), > also tighten related ones in shadow_hash_{insert,delete}(). > > Signed-off-by: Jan Beulich Looks good, but please leave the arrays at full size. With the full array, a bug could lead to an assertion failure or NULL deref; with a short array it could mean following a bogus funtion pointer. With that change, Acked-by: Tim Deegan Cheers, Tim. > --- a/xen/arch/x86/mm/shadow/common.c > +++ b/xen/arch/x86/mm/shadow/common.c > @@ -1565,7 +1565,7 @@ void shadow_hash_insert(struct domain *d > > ASSERT(paging_locked_by_me(d)); > ASSERT(d->arch.paging.shadow.hash_table); > - ASSERT(t); > + ASSERT(t >= SH_type_min_shadow && t <= SH_type_max_shadow); > > sh_hash_audit(d); > > @@ -1590,7 +1590,7 @@ void shadow_hash_delete(struct domain *d > > ASSERT(paging_locked_by_me(d)); > ASSERT(d->arch.paging.shadow.hash_table); > - ASSERT(t); > + ASSERT(t >= SH_type_min_shadow && t <= SH_type_max_shadow); > > sh_hash_audit(d); > > @@ -1668,7 +1668,7 @@ static void hash_vcpu_foreach(struct vcp > { > if ( callback_mask & (1 << x->u.sh.type) ) > { > - ASSERT(x->u.sh.type < SH_type_unused); > + ASSERT(x->u.sh.type <= SH_type_max_shadow); > ASSERT(callbacks[x->u.sh.type] != NULL); > done = callbacks[x->u.sh.type](v, page_to_mfn(x), > callback_mfn); > @@ -1715,7 +1715,7 @@ static void hash_domain_foreach(struct d > { > if ( callback_mask & (1 << x->u.sh.type) ) > { > - ASSERT(x->u.sh.type < SH_type_unused); > + ASSERT(x->u.sh.type <= SH_type_max_shadow); > ASSERT(callbacks[x->u.sh.type] != NULL); > done = callbacks[x->u.sh.type](d, page_to_mfn(x), > callback_mfn); > @@ -1819,26 +1819,16 @@ int sh_remove_write_access(struct domain > unsigned long fault_addr) > { > /* Dispatch table for getting per-type functions */ > - static const hash_domain_callback_t callbacks[SH_type_unused] = { > - NULL, /* none */ > + static const hash_domain_callback_t callbacks[] = { > #ifdef CONFIG_HVM > - SHADOW_INTERNAL_NAME(sh_rm_write_access_from_l1, 2), /* l1_32 */ > - SHADOW_INTERNAL_NAME(sh_rm_write_access_from_l1, 2), /* fl1_32 */ > - NULL, /* l2_32 */ > - SHADOW_INTERNAL_NAME(sh_rm_write_access_from_l1, 3), /* l1_pae */ > - SHADOW_INTERNAL_NAME(sh_rm_write_access_from_l1, 3), /* fl1_pae */ > - NULL, /* l2_pae */ > + [SH_type_l1_32_shadow] = SHADOW_INTERNAL_NAME(sh_rm_write_access_from_l1, 2), > + [SH_type_fl1_32_shadow] = SHADOW_INTERNAL_NAME(sh_rm_write_access_from_l1, 2), > + [SH_type_l1_pae_shadow] = SHADOW_INTERNAL_NAME(sh_rm_write_access_from_l1, 3), > + [SH_type_fl1_pae_shadow] = SHADOW_INTERNAL_NAME(sh_rm_write_access_from_l1, 3), > #endif > - SHADOW_INTERNAL_NAME(sh_rm_write_access_from_l1, 4), /* l1_64 */ > - SHADOW_INTERNAL_NAME(sh_rm_write_access_from_l1, 4), /* fl1_64 */ > - NULL, /* l2_64 */ > - NULL, /* l2h_64 */ > - NULL, /* l3_64 */ > - NULL, /* l4_64 */ > - NULL, /* p2m */ > - NULL /* unused */ > + [SH_type_l1_64_shadow] = SHADOW_INTERNAL_NAME(sh_rm_write_access_from_l1, 4), > + [SH_type_fl1_64_shadow] = SHADOW_INTERNAL_NAME(sh_rm_write_access_from_l1, 4), > }; > - > static const unsigned int callback_mask = SHF_L1_ANY | SHF_FL1_ANY; > struct page_info *pg = mfn_to_page(gmfn); > #if SHADOW_OPTIMIZATIONS & SHOPT_WRITABLE_HEURISTIC > @@ -2044,26 +2034,16 @@ int sh_remove_all_mappings(struct domain > struct page_info *page = mfn_to_page(gmfn); > > /* Dispatch table for getting per-type functions */ > - static const hash_domain_callback_t callbacks[SH_type_unused] = { > - NULL, /* none */ > + static const hash_domain_callback_t callbacks[] = { > #ifdef CONFIG_HVM > - SHADOW_INTERNAL_NAME(sh_rm_mappings_from_l1, 2), /* l1_32 */ > - SHADOW_INTERNAL_NAME(sh_rm_mappings_from_l1, 2), /* fl1_32 */ > - NULL, /* l2_32 */ > - SHADOW_INTERNAL_NAME(sh_rm_mappings_from_l1, 3), /* l1_pae */ > - SHADOW_INTERNAL_NAME(sh_rm_mappings_from_l1, 3), /* fl1_pae */ > - NULL, /* l2_pae */ > + [SH_type_l1_32_shadow] = SHADOW_INTERNAL_NAME(sh_rm_mappings_from_l1, 2), > + [SH_type_fl1_32_shadow] = SHADOW_INTERNAL_NAME(sh_rm_mappings_from_l1, 2), > + [SH_type_l1_pae_shadow] = SHADOW_INTERNAL_NAME(sh_rm_mappings_from_l1, 3), > + [SH_type_fl1_pae_shadow] = SHADOW_INTERNAL_NAME(sh_rm_mappings_from_l1, 3), > #endif > - SHADOW_INTERNAL_NAME(sh_rm_mappings_from_l1, 4), /* l1_64 */ > - SHADOW_INTERNAL_NAME(sh_rm_mappings_from_l1, 4), /* fl1_64 */ > - NULL, /* l2_64 */ > - NULL, /* l2h_64 */ > - NULL, /* l3_64 */ > - NULL, /* l4_64 */ > - NULL, /* p2m */ > - NULL /* unused */ > + [SH_type_l1_64_shadow] = SHADOW_INTERNAL_NAME(sh_rm_mappings_from_l1, 4), > + [SH_type_fl1_64_shadow] = SHADOW_INTERNAL_NAME(sh_rm_mappings_from_l1, 4), > }; > - > static const unsigned int callback_mask = SHF_L1_ANY | SHF_FL1_ANY; > > perfc_incr(shadow_mappings); > @@ -2189,45 +2169,27 @@ void sh_remove_shadows(struct domain *d, > > /* Dispatch table for getting per-type functions: each level must > * be called with the function to remove a lower-level shadow. */ > - static const hash_domain_callback_t callbacks[SH_type_unused] = { > - NULL, /* none */ > + static const hash_domain_callback_t callbacks[] = { > #ifdef CONFIG_HVM > - NULL, /* l1_32 */ > - NULL, /* fl1_32 */ > - SHADOW_INTERNAL_NAME(sh_remove_l1_shadow, 2), /* l2_32 */ > - NULL, /* l1_pae */ > - NULL, /* fl1_pae */ > - SHADOW_INTERNAL_NAME(sh_remove_l1_shadow, 3), /* l2_pae */ > + [SH_type_l2_32_shadow] = SHADOW_INTERNAL_NAME(sh_remove_l1_shadow, 2), > + [SH_type_l2_pae_shadow] = SHADOW_INTERNAL_NAME(sh_remove_l1_shadow, 3), > #endif > - NULL, /* l1_64 */ > - NULL, /* fl1_64 */ > - SHADOW_INTERNAL_NAME(sh_remove_l1_shadow, 4), /* l2_64 */ > - SHADOW_INTERNAL_NAME(sh_remove_l1_shadow, 4), /* l2h_64 */ > - SHADOW_INTERNAL_NAME(sh_remove_l2_shadow, 4), /* l3_64 */ > - SHADOW_INTERNAL_NAME(sh_remove_l3_shadow, 4), /* l4_64 */ > - NULL, /* p2m */ > - NULL /* unused */ > + [SH_type_l2_64_shadow] = SHADOW_INTERNAL_NAME(sh_remove_l1_shadow, 4), > + [SH_type_l2h_64_shadow] = SHADOW_INTERNAL_NAME(sh_remove_l1_shadow, 4), > + [SH_type_l3_64_shadow] = SHADOW_INTERNAL_NAME(sh_remove_l2_shadow, 4), > + [SH_type_l4_64_shadow] = SHADOW_INTERNAL_NAME(sh_remove_l3_shadow, 4), > }; > > /* Another lookup table, for choosing which mask to use */ > - static const unsigned int masks[SH_type_unused] = { > - 0, /* none */ > + static const unsigned int masks[SH_type_max_shadow + 1] = { > #ifdef CONFIG_HVM > - SHF_L2_32, /* l1_32 */ > - 0, /* fl1_32 */ > - 0, /* l2_32 */ > - SHF_L2_PAE, /* l1_pae */ > - 0, /* fl1_pae */ > - 0, /* l2_pae */ > + [SH_type_l1_32_shadow] = SHF_L2_32, > + [SH_type_l1_pae_shadow] = SHF_L2_PAE, > #endif > - SHF_L2H_64 | SHF_L2_64, /* l1_64 */ > - 0, /* fl1_64 */ > - SHF_L3_64, /* l2_64 */ > - SHF_L3_64, /* l2h_64 */ > - SHF_L4_64, /* l3_64 */ > - 0, /* l4_64 */ > - 0, /* p2m */ > - 0 /* unused */ > + [SH_type_l1_64_shadow] = SHF_L2H_64 | SHF_L2_64, > + [SH_type_l2_64_shadow] = SHF_L3_64, > + [SH_type_l2h_64_shadow] = SHF_L3_64, > + [SH_type_l3_64_shadow] = SHF_L4_64, > }; > > ASSERT(!(all && fast)); > @@ -2356,24 +2318,8 @@ static int sh_clear_up_pointer(struct vc > > void sh_reset_l3_up_pointers(struct vcpu *v) > { > - static const hash_vcpu_callback_t callbacks[SH_type_unused] = { > - NULL, /* none */ > -#ifdef CONFIG_HVM > - NULL, /* l1_32 */ > - NULL, /* fl1_32 */ > - NULL, /* l2_32 */ > - NULL, /* l1_pae */ > - NULL, /* fl1_pae */ > - NULL, /* l2_pae */ > -#endif > - NULL, /* l1_64 */ > - NULL, /* fl1_64 */ > - NULL, /* l2_64 */ > - NULL, /* l2h_64 */ > - sh_clear_up_pointer, /* l3_64 */ > - NULL, /* l4_64 */ > - NULL, /* p2m */ > - NULL /* unused */ > + static const hash_vcpu_callback_t callbacks[] = { > + [SH_type_l3_64_shadow] = sh_clear_up_pointer, > }; > static const unsigned int callback_mask = SHF_L3_64; > > @@ -3381,25 +3327,23 @@ int shadow_domctl(struct domain *d, > void shadow_audit_tables(struct vcpu *v) > { > /* Dispatch table for getting per-type functions */ > - static const hash_vcpu_callback_t callbacks[SH_type_unused] = { > - NULL, /* none */ > + static const hash_vcpu_callback_t callbacks[] = { > #if SHADOW_AUDIT & (SHADOW_AUDIT_ENTRIES | SHADOW_AUDIT_ENTRIES_FULL) > # ifdef CONFIG_HVM > - SHADOW_INTERNAL_NAME(sh_audit_l1_table, 2), /* l1_32 */ > - SHADOW_INTERNAL_NAME(sh_audit_fl1_table, 2), /* fl1_32 */ > - SHADOW_INTERNAL_NAME(sh_audit_l2_table, 2), /* l2_32 */ > - SHADOW_INTERNAL_NAME(sh_audit_l1_table, 3), /* l1_pae */ > - SHADOW_INTERNAL_NAME(sh_audit_fl1_table, 3), /* fl1_pae */ > - SHADOW_INTERNAL_NAME(sh_audit_l2_table, 3), /* l2_pae */ > + [SH_type_l1_32_shadow] = SHADOW_INTERNAL_NAME(sh_audit_l1_table, 2), > + [SH_type_fl1_32_shadow] = SHADOW_INTERNAL_NAME(sh_audit_fl1_table, 2), > + [SH_type_l2_32_shadow] = SHADOW_INTERNAL_NAME(sh_audit_l2_table, 2), > + [SH_type_l1_pae_shadow] = SHADOW_INTERNAL_NAME(sh_audit_l1_table, 3), > + [SH_type_fl1_pae_shadow] = SHADOW_INTERNAL_NAME(sh_audit_fl1_table, 3), > + [SH_type_l2_pae_shadow] = SHADOW_INTERNAL_NAME(sh_audit_l2_table, 3), > # endif > - SHADOW_INTERNAL_NAME(sh_audit_l1_table, 4), /* l1_64 */ > - SHADOW_INTERNAL_NAME(sh_audit_fl1_table, 4), /* fl1_64 */ > - SHADOW_INTERNAL_NAME(sh_audit_l2_table, 4), /* l2_64 */ > - SHADOW_INTERNAL_NAME(sh_audit_l2_table, 4), /* l2h_64 */ > - SHADOW_INTERNAL_NAME(sh_audit_l3_table, 4), /* l3_64 */ > - SHADOW_INTERNAL_NAME(sh_audit_l4_table, 4), /* l4_64 */ > + [SH_type_l1_64_shadow] = SHADOW_INTERNAL_NAME(sh_audit_l1_table, 4), > + [SH_type_fl1_64_shadow] = SHADOW_INTERNAL_NAME(sh_audit_fl1_table, 4), > + [SH_type_l2_64_shadow] = SHADOW_INTERNAL_NAME(sh_audit_l2_table, 4), > + [SH_type_l2h_64_shadow] = SHADOW_INTERNAL_NAME(sh_audit_l2_table, 4), > + [SH_type_l3_64_shadow] = SHADOW_INTERNAL_NAME(sh_audit_l3_table, 4), > + [SH_type_l4_64_shadow] = SHADOW_INTERNAL_NAME(sh_audit_l4_table, 4), > #endif > - NULL /* All the rest */ > }; > unsigned int mask; > > @@ -3427,7 +3371,9 @@ void shadow_audit_tables(struct vcpu *v) > } > } > > - HASH_CALLBACKS_CHECK(SHF_page_type_mask); > + HASH_CALLBACKS_CHECK(SHADOW_AUDIT & (SHADOW_AUDIT_ENTRIES | > + SHADOW_AUDIT_ENTRIES_FULL) > + ? SHF_page_type_mask : 0); > hash_vcpu_foreach(v, mask, callbacks, INVALID_MFN); > } >