Re: [PATCH 1/8] x86/paging: fold HAP and shadow memory alloc related fields

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.12.2022 14:49, Andrew Cooper wrote:
> On 21/12/2022 1:24 pm, Jan Beulich wrote:
>> Especially with struct shadow_domain and struct hap_domain not living in
>> a union inside struct paging_domain, let's avoid the duplication: The
>> fields are named and used in identical ways, and only one of HAP or
>> shadow can be in use for a domain. This then also renders involved
>> expressions slightly more legible.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, with two minor
> suggestions.

Thanks.

>> --- a/xen/arch/x86/include/asm/domain.h
>> +++ b/xen/arch/x86/include/asm/domain.h
>> @@ -179,10 +173,6 @@ struct shadow_vcpu {
>>  /*            hardware assisted paging          */
>>  /************************************************/
>>  struct hap_domain {
>> -    struct page_list_head freelist;
>> -    unsigned int      total_pages;  /* number of pages allocated */
>> -    unsigned int      free_pages;   /* number of pages on freelists */
>> -    unsigned int      p2m_pages;    /* number of pages allocated to p2m */
>>  };
> 
> Do we want to drop hap_domain?  I can't forsee needing to put anything
> back into it.

I would prefer to keep it, even if it's unclear what may want putting
there. It's not obvious to me at all that nothing ever will.

>> --- a/xen/arch/x86/mm/paging.c
>> +++ b/xen/arch/x86/mm/paging.c
>> @@ -979,17 +980,17 @@ int __init paging_set_allocation(struct
>>  
>>  int arch_get_paging_mempool_size(struct domain *d, uint64_t *size)
>>  {
>> -    int rc;
>> +    unsigned long pages;
>>  
>>      if ( is_pv_domain(d) )                 /* TODO: Relax in due course */
>>          return -EOPNOTSUPP;
>>  
>> -    if ( hap_enabled(d) )
>> -        rc = hap_get_allocation_bytes(d, size);
>> -    else
>> -        rc = shadow_get_allocation_bytes(d, size);
>> +    pages = d->arch.paging.total_pages;
>> +    pages += d->arch.paging.p2m_pages;
> 
> Any chance I can talk you into having a second space before the =, so we
> get:
> 
> pages  = d->arch.paging.total_pages;
> pages += d->arch.paging.p2m_pages;
> 
> nicely aligned vertically?

Sure, I can do that. I recall I was actually pondering whether to ...

Jan