Re: [PATCH v4] VMX: use a single, global APIC access page

From: Jan Beulich <jbeulich@suse.com>
To: Tim Deegan <tim@xen.org>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Wei Liu" <wl@xen.org>, "Roger Pau Monné" <roger.pau@citrix.com>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Kevin Tian" <kevin.tian@intel.com>,
	"Jun Nakajima" <jun.nakajima@intel.com>
Subject: Re: [PATCH v4] VMX: use a single, global APIC access page
Date: Mon, 19 Apr 2021 13:25:26 +0200	[thread overview]
Message-ID: <d698a29e-fb4e-a650-9e48-ad209e64c800@suse.com> (raw)
In-Reply-To: <YHs2AJ6pqrAw7uew@deinos.phlegethon.org>

On 17.04.2021 21:24, Tim Deegan wrote:
> At 12:40 +0200 on 12 Apr (1618231248), 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.
> 
> What is the aim here?  To save 4k per domain?  It seems to come out
> about even for adding and removing code. 

True, but still it looks wrong to me to use a page per guest when one
her host suffices. Think about many tiny, short-lived VMs (as in
Tamas'es VM forking).

>> --- 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)) )
> 
> Would it be better to check specifically for mfn == apic_access_mfn
> (and apic_access_mfn != 0, I guess)?

Roger did ask about the same - I neither want to expose apic_access_mfn
outside its CU, nor do I want to introduce an accessor function. Both
feel like layering violations to me.

>  If we want this behaviour for
> for all un-owned PGC_extra MFNs it would be good to explain that in the
> comments.

This is hard to tell without knowing which (or even if) further such
PGC_extra pages will appear. Hence any comment to that effect would be
guesswork at best. Of course I can add e.g. "Other pages with the same
properties would be treated the same", if that's what you're after?

Jan