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

[Juergen Gross <jgross@suse.com>]

On 24/04/18 12:31, Tim Deegan wrote:
> 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. :)

Okay, thanks.

> 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.

Okay.

> 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.

Okay.

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

In my newest version it already is testing that.


Juergen

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