Re: [Xen-devel] [PATCH v2] x86/mm: also flush TLB when putting writable foreign page reference

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.04.17 at 10:44, <tim@xxxxxxx> wrote:
> At 05:59 -0600 on 25 Apr (1493099950), Jan Beulich wrote:
>> >>> On 25.04.17 at 12:59, <tim@xxxxxxx> wrote:
>> > Hi,
>> > 
>> > At 02:59 -0600 on 25 Apr (1493089158), Jan Beulich wrote:
>> >> Jann's explanation of the problem:
>> >> 
>> >> "start situation:
>> >>  - domain A and domain B are PV domains
>> >>  - domain A and B both have currently scheduled vCPUs, and the vCPUs
>> >>    are not scheduled away
>> >>  - domain A has XSM_TARGET access to domain B
>> >>  - page X is owned by domain B and has no mappings
>> >>  - page X is zeroed
>> >> 
>> >>  steps:
>> >>  - domain A uses do_mmu_update() to map page X in domain A as writable
>> >>  - domain A accesses page X through the new PTE, creating a TLB entry
>> >>  - domain A removes its mapping of page X
>> >>    - type count of page X goes to 0
>> >>    - tlbflush_timestamp of page X is bumped
>> >>  - domain B maps page X as L1 pagetable
>> >>    - type of page X changes to PGT_l1_page_table
>> >>    - TLB flush is forced using domain_dirty_cpumask of domain B
>> >>    - page X is mapped as L1 pagetable in domain B
>> >> 
>> >>  At this point, domain B's vCPUs are guaranteed to have no
>> >>  incorrectly-typed stale TLB entries for page X, but AFAICS domain A's
>> >>  vCPUs can still have stale TLB entries that map page X as writable,
>> >>  permitting domain A to control a live pagetable of domain B."
>> > 
>> > AIUI this patch solves the problem by immediately flushing domain A's
>> > TLB entries at the point where domain A removes its mapping of page X.
>> > 
>> > Could we, instead, bitwise OR domain A's domain_dirty_cpumask into
>> > domain B's domain_dirty_cpumask at the same point?
>> > 
>> > Then when domain B flushes TLBs in the last step (in __get_page_type())
>> > it will catch any stale TLB entries from domain A as well.  But in the
>> > (hopefully common) case where there's a delay between domain A's
>> > __put_page_type() and domain B's __get_page_type(), the usual TLB
>> > timestamp filtering will suppress some of the IPIs/flushes.
>> 
>> Oh, I see. Yes, I think this would be fine. However, we don't have
>> a suitable cpumask accessor allowing us to do this ORing atomically,
>> so we'd have to open code it.
> 
> Probably better to build the accessor than to open code here. :)

Hmm, that would mean building a whole group of accessors (as
I wouldn't want to introduce an atomic OR one without any of
the others which exist as non-atomic ones). Plus there would be
the question of how to name them - the current inconsistency
(single bit operations being atomic unless prefixed by two
underscores, while multi-bit operations are non-atomic despite
their lack of leading underscores) doesn't really help here.

IOW the original question wasn't really whether to introduce
accessors, but whether the approach you suggest is worthwhile
despite the lack of such accessors. Which I think you ...

>> Do you think such a slightly ugly approach would be worth it here?
>> Foreign mappings shouldn't be _that_ performance critical..
> 
> I have no real idea, though there are quite a lot of them in domain
> building/migration.  I can imagine a busy multi-vcpu dom0 could
> generate a lot of IPIs, almost all of which could be merged.

... believe it would be.

>> And then, considering that this will result in time stamp based filtering
>> again, I'm no longer sure I was right to agree with Jann on the flush
>> here needing to be unconditional. Regardless of page table owner
>> matching page owner, the time stamp stored for the page will always
>> be applicable (it's a global property). So we wouldn't even need to
>> OR in the whole dirty mask here, but could already pre-filter (or if we
>> stayed with the flush-on-put approach, then v1 would have been
>> correct).
> 
> I don't think so.  The page's timestamp is set when its typecount
> falls to zero, which hasn't happened yet -- we hold a typecount
> ourselves here.
> 
> In theory we could filter the bits we're adding against a local
> timestamp, but that would have to be tlbflush_current_time()
> because the TLB entries we care about are live right now, and
> filtering against that is (probably) a noop.

Good point. I guess I'll switch to the open coded merging approach
then for v3.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel