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

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

On Tue, Oct 19, 2021 at 05:06:27PM +0200, Jan Beulich wrote:
> On 19.10.2021 15:53, Roger Pau Monné wrote:
> > On Tue, Oct 19, 2021 at 02:52:27PM +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().
> >> (Note that strictly speaking the change at the out_of_memory label is
> >> not needed, as the domain gets crashed there anyway. The change is being
> >> made nevertheless to avoid setting up a trap from someone meaning to
> >> deal with that case better than by domain_crash().)
> >>
> >> Similarly for EPT I think ept_set_entry() -> ept_sync_domain() ->
> >> ept_sync_domain_prepare() -> p2m_flush_nestedp2m() is affected. Make its
> >> p2m_flush_nestedp2m() invocation conditional. Note that this then also
> >> alters behavior of p2m_change_type_range() on EPT, deferring the nested
> >> flushes there as well. I think this should have been that way from the
> >> introduction of the flag.
> >>
> >> Reported-by: Elliott Mitchell <ehem+xen@xxxxxxx>
> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> > 
> > Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> Thanks.
> 
> >> --- a/xen/arch/x86/mm/p2m-ept.c
> >> +++ b/xen/arch/x86/mm/p2m-ept.c
> >> @@ -1253,7 +1253,7 @@ static void ept_sync_domain_prepare(stru
> >>      {
> >>          if ( p2m_is_nestedp2m(p2m) )
> >>              ept = &p2m_get_hostp2m(d)->ept;
> >> -        else
> >> +        else if ( !p2m->defer_nested_flush )
> >>              p2m_flush_nestedp2m(d);
> > 
> > I find this model slightly concerning, as we don't actually notify the
> > caller that a nested flush as been deferred, so we must make sure that
> > whoever sets defer_nested_flush also performs a flush unconditionally
> > when clearing the flag.
> 
> Well, this _is_ the model used for now. Until this change there was
> just a single party setting the flag. And like here, any new party
> setting the flag will also need to invoke a flush upon clearing it.
> It's not clear to me what alternative model you may have in mind.

I was mostly thinking of something similar to the
defer_flush/need_flush pair, where the need for a flush is signaled in
need_flush, so setting defer_flush doesn't automatically imply a flush
when clearing it.

Anyway, this is simple enough that it doesn't warrant the use of the
more complicated logic.

Thanks, Roger.