On Fri, Jan 03, 2020 at 04:17:14PM +0100, Jan Beulich wrote: > On 24.12.2019 14:26, Roger Pau Monne wrote: > > There's no need to call paging_update_cr3 unless CR3 trapping is > > enabled, and that's only the case when using shadow paging or when > > requested for introspection purposes, otherwise there's no need to > > pause all the vCPUs of the domain in order to perform the flush. > > > > Check whether CR3 trapping is currently in use in order to decide > > whether the vCPUs should be paused, otherwise just perform the flush. > > First of all - with the commit introducing the pausing not saying > anything on the "why", you must have gained some understanding > there. Could you share this?
hap_update_cr3 does a "v->arch.hvm.hw_cr[3] = v->arch.hvm.guest_cr[3]" unconditionally, and such access would be racy if the vCPU is running and also modifying cr3 at the same time AFAICT. Just pausing each vCPU before calling paging_update_cr3 should be fine and would have a smaller performance penalty. > I can't see why this was needed, and > sh_update_cr3() also doesn't look to have any respective ASSERT() > or alike. I'm having even more trouble seeing why in HAP mode the > pausing would be needed. > > As a result I wonder whether, rather than determining whether > pausing is needed inside the function, this shouldn't be a flag > in struct paging_mode. > > Next I seriously doubt introspection hooks should be called here. > Introspection should be about guest actions, and us calling > paging_update_cr3() is an implementation detail of Xen, not > something the guest controls. Even more, there not being any CR3 > change here I wonder whether the call by the hooks to > hvm_update_guest_cr3() couldn't be suppressed altogether in this > case. Quite possibly in the shadow case there could be more > steps that aren't really needed, so perhaps a separate hook might > be on order. Right, I guess just having a hook that does a flush would be enough. Let me try to propose something slightly better. Thanks, Roger. _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
