Re: [PATCH] x86/PoD: defer nested P2M flushes

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

On Tue, Oct 19, 2021 at 03:14:25PM +0200, Jan Beulich wrote:
> On 19.10.2021 14:59, Roger Pau Monné wrote:
> > On Tue, Oct 19, 2021 at 01:58:38PM +0200, Jan Beulich wrote:
> >> On 19.10.2021 12:41, Roger Pau Monné wrote:
> >>> On Mon, Oct 11, 2021 at 10:17:08AM +0200, Jan Beulich wrote:
> >>>> With NPT or shadow in use, the p2m_set_entry() -> p2m_pt_set_entry() ->
> >>>> write_p2m_entry() -> p2m_flush_nestedp2m() call sequence triggers a lock
> >>>> order violation when the PoD lock is held around it. Hence such flushing
> >>>> needs to be deferred. Steal the approach from p2m_change_type_range().
> >>>>
> >>>> Similarly for EPT I think ept_set_entry() -> ept_sync_domain() ->
> >>>> ept_sync_domain_prepare() -> p2m_flush_nestedp2m() is affected.
> >>>
> >>> I'm slightly worried by this path because it doesn't seem to
> >>> acknowledge defer_nested_flush.
> >>
> >> Oh, yes. Iirc I did look at that logic, write down the remark, and
> >> then forgot to add the conditional to ept_sync_domain_prepare().
> >> The interactions with the real (host) flush are kind of ugly there,
> >> though - we then further depend on the ->defer_flush / ->need_flush
> >> logic, which is EPT-only. But I think I've convinced myself that
> >> this ought to work out.
> >>
> >>> Maybe the flag should be checked by
> >>> p2m_flush_nestedp2m instead of leaving it to the callers?
> >>
> >> I'm not sure this would be a good idea, as there are more callers.
> > 
> > We should maybe add an ASSERT to p2m_flush_nestedp2m to make sure it's
> > not called with defer_nested_flush being set then, or else it's
> > possible for new callers of p2m_flush_nestedp2m to forget checking
> > defer_nested_flush.
> 
> I'm afraid we can't do that, or at least not this easily: The flag
> assumes the p2m lock to be held when it gets set. Hence callers not
> holding the lock (hvm_set_efer(), nvmx_handle_invept()) may trigger
> such an assert just because another CPU set the flag.

Hm, indeed. Forcing those to take the p2m lock might be too much
overhead for the sake of correctness.

Thanks, Roger.