Re: [Xen-devel] [PATCH v8 1/9] x86/xpti: avoid copying L4 page table contents when possible

[Tim Deegan <tim@xxxxxxx>]

At 07:45 +0200 on 23 Apr (1524469545), Juergen Gross wrote:
> On 22/04/18 18:39, Tim Deegan wrote:
> > At 19:11 +0200 on 21 Apr (1524337893), Juergen Gross wrote:
> >> On 21/04/18 15:32, Tim Deegan wrote:
> >>> At 09:44 +0200 on 19 Apr (1524131080), Juergen Gross wrote:
> >>>> Another alternative would be to pass another flag to the callers to
> >>>> signal the need for a flush. This would require quite some modifications
> >>>> to shadow code I'd like to avoid, though. OTOH this way we could combine
> >>>> flushing the tlb and the root page tables. Tim, any preferences?
> >>>
> >>> This sounds a promising direction but it should be doabl without major
> >>> surgery to the shadow code.  The shadow code already leaves old sl4es
> >>> visible (in TLBs) when it's safe to do so, so I think the right place
> >>> to hook this is on the receiving side of the TLB flush IPI.  IOW as
> >>> long as:
> >>>  - you copy the L4 on context switch; and
> >>>  - you copy it on the TLB flush IPI is received
> >>> then you can rely on the existing TLB flush mechanisms to do what you 
> >>> need.
> >>> And shadow doesn't have to behave differently from 'normal' PV MM.
> >>
> >> It is not so easy. The problem is that e.g. a page fault will flush the
> >> TLB entry for the page in question, but it won't lead to the L4 to be
> >> copied.
> > 
> > Oh yes, I see; thanks for the explanation.  It might be worth copying
> > what the hardware does here, and checking/propagating the relevant l4e
> > in the PV pagefault handler.
> 
> While in the long run being an interesting option I'm not sure I want
> to go this route for 4.11. There might be nasty corner cases and I think
> such a lazy approach is much more error prone than doing explicit
> updates of the L4 table on the affected cpus in case of a modified
> entry. I think we should either do the explicit call of flush_mask() in
> shadow_set_l4e() or propagate the need for the flush up to the caller.

FWIW, I disagree -- I think that having the fault handler DTRT and
relying on the existing, tested, TLB-flush logic is more likely to
work than introducing a new mechanism that _also_ has to catch every
possible l4e update.  It should touch less code and be less likely to
break with later changes.  And I think it would be better to do it
'properly' now than to hope there's time to revisit it later.  That
said, if Jan agrees that this way is OK, I'll quit grumbling and
review the shadow parts. :)

I think that setting the bits in shadow_set_l4e() is better than
having this leak out into all the callers.  I'm happy to see that the
hunk in l4e_propagate_from_guest() has gone away too.

Please move the shadow_set_l4e() hunk up so it's just after the write,
and before the general TLB flush logic.

Please move the logic into your code: the new function should take a
domain pointer and do all the filtering itself rather than have shadow
code be aware of what xpti is or why the domain's dirty-cpumask is
relevant.

It doesn't look like there's any check limiting this to PV guests, and
I think there should be, right?

Cheers,

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel