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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.04.2021 16:17, Roger Pau Monné wrote:
> On Fri, Apr 23, 2021 at 12:52:57PM +0200, Jan Beulich wrote:
>> --- a/xen/arch/x86/mm/shadow/set.c
>> +++ b/xen/arch/x86/mm/shadow/set.c
>> @@ -94,6 +94,15 @@ shadow_get_page_from_l1e(shadow_l1e_t sl
>>      ASSERT(!sh_l1e_is_magic(sl1e));
>>      ASSERT(shadow_mode_refcounts(d));
>>  
>> +    /*
>> +     * Check whether refcounting is suppressed on this page. For example,
>> +     * 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.
>> +     */
>> +    mfn = shadow_l1e_get_mfn(sl1e);
>> +    if ( mfn_valid(mfn) && page_refcounting_suppressed(mfn_to_page(mfn)) )
>> +        return 0;
>> +
>>      res = get_page_from_l1e(sl1e, d, d);
>>  
>>      /*
>> --- a/xen/arch/x86/mm/shadow/types.h
>> +++ b/xen/arch/x86/mm/shadow/types.h
>> @@ -276,9 +276,16 @@ int shadow_set_l4e(struct domain *d, sha
>>  static void inline
>>  shadow_put_page_from_l1e(shadow_l1e_t sl1e, struct domain *d)
>>  {
>> +    mfn_t mfn;
>> +
>>      if ( !shadow_mode_refcounts(d) )
>>          return;
>>  
>> +    if ( mfn_valid(mfn = shadow_l1e_get_mfn(sl1e)) &&
> 
> Nit: I would prefer if assigned mfn outside of the condition, like
> it's done in the chunk added to shadow_get_page_from_l1e.

Well, I did it differently here because the variable really is
only needed inside the if(), whereas in "get" the subsequent
patches use it elsewhere as well. I'll wait what Tim says.

> The rest LGTM, so:
> 
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks.

Jan