Re: [PATCH 3/8] x86/paging: move update_paging_modes() hook

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.12.2022 18:43, Andrew Cooper wrote:
> On 21/12/2022 1:25 pm, Jan Beulich wrote:
>> The hook isn't mode dependent, hence it's misplaced in struct
>> paging_mode. (Or alternatively I see no reason why the alloc_page() and
>> free_page() hooks don't also live there.) Move it to struct
>> paging_domain.
>>
>> While there rename the hook and HAP's as well as shadow's hook functions
>> to use singular; I never understood why plural was used. (Renaming in
>> particular the wrapper would be touching quite a lot of other code.)
> 
> There are always two modes; Xen's, and the guest's.
> 
> This was probably more visible back in the 32-bit days, but remnants of
> it are still around with the fact that struct vcpu needs to be below the
> 4G boundary for the PDPTRs for when the guest isn't in Long Mode.
> 
> HAP also hides it fairly well given the uniformity of EPT/NPT (and
> always 4 levels in a 64-bit Xen), but I suspect it will become more
> visible again when we start supporting LA57.

So does this boil down to a request to undo the rename? Or undo it just
for shadow code (as the HAP function really does only one thing)? As to
LA57, I'm not convinced it'll become more visible again then, but of
course without actually doing that work it's all hand-waving anyway.

>> --- a/xen/arch/x86/mm/shadow/none.c
>> +++ b/xen/arch/x86/mm/shadow/none.c
>> @@ -27,6 +32,9 @@ int shadow_domain_init(struct domain *d)
>>      };
>>  
>>      paging_log_dirty_init(d, &sh_none_ops);
>> +
>> +    d->arch.paging.update_paging_mode = _update_paging_mode;
>> +
>>      return is_hvm_domain(d) ? -EOPNOTSUPP : 0;
> 
> I know you haven't changed the logic here, but this hook looks broken. 
> Why do we fail it right at the end for HVM domains?

It's been a long time, but I guess my thinking back then was that it's
better to put in place pointers which other code may rely on being non-
NULL.

Jan