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. All of that said, I have debated the utility of this hook. It mixes up various concepts including the singleton construction of monitor tables, and TLB flushing (which should not happen for a mov cr3 with NOFLUSH set), and with those concepts abstracted properly, the only remaining action is caching the right ops pointer. I'm not convinced it will survive a concerned attempt to fix the HVM p2m vs guest tlb flushing confusion. > --- 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? ~Andrew
