Emilio G. Cota <c...@braap.org> writes:
> On Mon, Oct 08, 2018 at 14:57:18 +0100, Alex Bennée wrote: >> Emilio G. Cota <c...@braap.org> writes: >> > The readers that do not hold tlb_lock must use atomic reads when >> > reading .addr_write, since this field can be updated by other threads; >> > the conversion to atomic reads is done in the next patch. >> >> We don't enforce this for the TCG code - but rely on the backend ISA's >> to avoid torn reads from updates from cputlb that could invalidate an >> entry. > > We do enforce it though; the TLB reads we emit in TCG backend > code are appropriately sized to guarantee atomic reads. > >> > -/* For atomic correctness when running MTTCG we need to use the right >> > - * primitives when copying entries */ >> > -static inline void copy_tlb_helper(CPUTLBEntry *d, CPUTLBEntry *s, >> > - bool atomic_set) >> > +/* Called with tlb_lock held */ >> > +static inline void copy_tlb_helper_locked(CPUTLBEntry *d, const >> > CPUTLBEntry *s) >> > { >> > -#if TCG_OVERSIZED_GUEST >> > *d = *s; >> >> In general I'm happy with the patch set but what ensures that this >> always DRT with respect to the TCG code reads that race with it? > > copy_tlb_helper is only called by the "owner" CPU, so it cannot > race with TCG code (i.e. the owner thread cannot race with itself). > > I wanted to add an assert_cpu_is_self(cpu) here, but that needs > a CPUState pointer. Maybe I should just get rid of the function? > All the callers have the assert, so that might make the code > clearer. I'm happy keeping the function and just expanding the comment: /* Called with tlb_lock held and only ever from the vCPU context */ Reviewed-by: Alex Bennée <alex.ben...@linaro.org> > > Thanks, > > Emilio -- Alex Bennée