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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.04.2021 09:42, Tim Deegan wrote:
> At 13:25 +0200 on 19 Apr (1618838726), Jan Beulich wrote:
>> On 17.04.2021 21:24, Tim Deegan wrote:
>>> At 12:40 +0200 on 12 Apr (1618231248), Jan Beulich wrote:
>>>> --- 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.
> 
> I think that this is even more of a layering violation: what we
> actually want is to allow un-refcounted mappings of the
> apic_access_mfn, but to do it we're relying on an internal
> implementation detail (that it happens to be un-owned and PGC_extra)
> rather than giving ourselves an API.
> 
> And so we're tangled up talking about how to write comments to warn
> our future selves about the possible side-effects.
> 
>>>  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?
> 
> If you want to go this way there should be a comment here saying that
> we're allowing this for all PGC_extra pages because we need it for
> apic_access_mfn, and a comment at PGC_extra saying that it has this
> effect.

So (along with a comment to this effect) how about I make
page_suppress_refcounting() and page_refcounting_suppressed() helpers?
The former would set PGC_extra on the page and assert the page has no
owner, while the latter would subsume the checks done here. The only
question then is what to do with the ASSERT(type == p2m_mmio_direct):
That's still a property of the APIC access MFN which may or may not
hold for future such pages. (It can't be part of the new helper anyway
as "put" doesn't have the type available.)

Jan