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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 22/12/2022 8:00 am, Jan Beulich wrote:
> 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.

With LA57, HAP really does become 2 things.  Using a 5-level walk at the
HAP level is a prerequisite for being able to VMEntry with CR4.LA57 set,
on both Intel and AMD hardware IIRC.

Then, for VMs which don't elect to enable LA57, we will be in a 4-on-5
(to borrow the shadow terminology) situation.

The comment by paging_update_paging_modes() is fairly clear about this
operation being strictly related to guest state, which further
demonstrates that hap version playing with the monitor table is wrong.

>
>>> --- 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.

Any chance we could gain a /* set up pointers for safety, then fail */
then ?

~Andrew