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
