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=-17.2 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 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 91429C433B4 for ; Tue, 13 Apr 2021 09:24:29 +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 5322F613B7 for ; Tue, 13 Apr 2021 09:24:29 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5322F613B7 Authentication-Results: mail.kernel.org; dmarc=fail (p=quarantine dis=none) header.from=suse.com 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.109604.209228 (Exim 4.92) (envelope-from ) id 1lWFHI-0003Ob-CB; Tue, 13 Apr 2021 09:24:12 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 109604.209228; Tue, 13 Apr 2021 09:24:12 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1lWFHI-0003OU-8x; Tue, 13 Apr 2021 09:24:12 +0000 Received: by outflank-mailman (input) for mailman id 109604; Tue, 13 Apr 2021 09:24:11 +0000 Received: from us1-rack-iad1.inumbo.com ([172.99.69.81]) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1lWFHG-0003OO-UJ for xen-devel@lists.xenproject.org; Tue, 13 Apr 2021 09:24:10 +0000 Received: from mx2.suse.de (unknown [195.135.220.15]) by us1-rack-iad1.inumbo.com (Halon) with ESMTPS id f9911ded-f39a-4bf6-a234-252def82cb15; Tue, 13 Apr 2021 09:24:09 +0000 (UTC) Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id ECAD9AF0D; Tue, 13 Apr 2021 09:24:08 +0000 (UTC) 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: f9911ded-f39a-4bf6-a234-252def82cb15 X-Virus-Scanned: by amavisd-new at test-mx.suse.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=susede1; t=1618305849; h=from:from:reply-to: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=0JyzQT29QISH1n2na2do1XSDb0Bj3BwcVDJrbVg7+zo=; b=j0sTeKWFOS/ctbjv1DJD3oxq5NHFWsQy5Dz4csHgGQMerAyk0wESxHS7QrMFICD54wkktY /edukcNjcIIduBti1iDW2BCZZsan+J7tfzgITIshs5hgnYPYEOyyvsVoQlULlp+0hq8iO/ qgyEb6oTSxvtdfbTmY7ygEZqTZSgWtA= Subject: Re: [PATCH v4] VMX: use a single, global APIC access page To: =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= Cc: "xen-devel@lists.xenproject.org" , Andrew Cooper , Wei Liu , George Dunlap , Kevin Tian , Jun Nakajima , Tim Deegan References: <4731a3a3-906a-98ac-11ba-6a0723903391@suse.com> <1c489e77-6e65-6121-6c28-3c4bd377223c@suse.com> From: Jan Beulich Message-ID: Date: Tue, 13 Apr 2021 11:24:09 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.9.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 12.04.2021 17:31, Roger Pau Monné wrote: > On Mon, Apr 12, 2021 at 12:40:48PM +0200, Jan Beulich wrote: >> The address of this page is used by the CPU only to recognize when to >> access the virtual APIC page instead. No accesses would ever go to this >> page. It only needs to be present in the (CPU) page tables so that >> address translation will produce its address as result for respective >> accesses. >> >> By making this page global, we also eliminate the need to refcount it, >> or to assign it to any domain in the first place. >> >> Signed-off-by: Jan Beulich >> Reviewed-by: Kevin Tian >> --- >> v4: Set PGC_extra on the page. Make shadow mode work. >> v3: Split p2m insertion change to a separate patch. >> v2: Avoid insertion when !has_vlapic(). Split off change to >> p2m_get_iommu_flags(). >> --- >> I did further consider not allocating any real page at all, but just >> using the address of some unpopulated space (which would require >> announcing this page as reserved to Dom0, so it wouldn't put any PCI >> MMIO BARs there). But I thought this would be too controversial, because >> of the possible risks associated with this. > > Really seems more trouble than reward. Also there are systems with > MMIO regions in holes on the memory map, like the issue I had with the > Intel pinctrl stuff that had an MMIO region in a hole on the memory > map [0], so I'm not sure Xen would be in a position to select a > suitable unpopulated page anyway. > > [0] https://lore.kernel.org/xen-devel/YFx80wYt%2FKcHanC7@smile.fi.intel.com/ Yeah, I had seen that. What I'm having trouble to understand is how the OS will know to avoid that range for e.g. placing BARs. >> @@ -411,28 +411,22 @@ static int vmx_domain_initialise(struct >> */ >> d->arch.hvm.vmx.exec_sp = is_hardware_domain(d) || opt_ept_exec_sp; >> >> - if ( !has_vlapic(d) ) >> - return 0; >> - >> - if ( (rc = vmx_alloc_vlapic_mapping(d)) != 0 ) >> - return rc; >> - >> return 0; >> } >> >> -static void vmx_domain_relinquish_resources(struct domain *d) >> +static void domain_creation_finished(struct domain *d) >> { >> - if ( !has_vlapic(d) ) >> + gfn_t gfn = gaddr_to_gfn(APIC_DEFAULT_PHYS_BASE); > > Worth making it const static? The compiler ought to be able to fold this into a suitable constant at the use site. Definitely not static imo, and I see little point in making a local variable const, unless one really wants to document something very special. >> + uint8_t ipat; >> + >> + if ( !has_vlapic(d) || mfn_eq(apic_access_mfn, _mfn(0)) ) > > It would be better to use INVALID_MFN here, and init apic_access_mfn > to that value. Oh, yes, that's easier possible now that the variable is static. >> --- a/xen/arch/x86/mm/shadow/set.c >> +++ b/xen/arch/x86/mm/shadow/set.c >> @@ -94,6 +94,22 @@ shadow_get_page_from_l1e(shadow_l1e_t sl >> ASSERT(!sh_l1e_is_magic(sl1e)); >> ASSERT(shadow_mode_refcounts(d)); >> >> + /* >> + * VMX'es APIC access MFN is just a surrogate page. It doesn't actually >> + * get accessed, and hence there's no need to refcount it (and refcounting >> + * would fail, due to the page having no owner). >> + */ >> + if ( mfn_valid(mfn = shadow_l1e_get_mfn(sl1e)) ) > > I find this assignment inside the parameter list quite ugly, I would > rather split it on it's own line. Well, okay. To be honest I'm not even sure why I did it this way, as I could have expected a respective comment. >> + { >> + const struct page_info *pg = mfn_to_page(mfn); >> + >> + if ( !page_get_owner(pg) && (pg->count_info & PGC_extra) ) >> + { >> + ASSERT(type == p2m_mmio_direct); >> + return 0; > > Are there any other pages that could pass this check? I don't think > so, but wanted to assert. "Normal" extra pages have an owner, so no, there aren't any others. If and when any appear, this may need further customizing, albeit generally I'd hope further pages matching this pattern would also want similar treatment. Jan