Re: [Xen-devel] [PATCH 1/2] x86/hvm: improve performance of HVMOP_flush_tlbs

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel