Re: [Xen-devel] [PATCH 1/3] xen/vt-d: need barriers to workaround CLFLUSH

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Chen, Tiejun
> Sent: Monday, May 04, 2015 7:26 PM
> 
> On 2015/5/4 18:52, Tian, Kevin wrote:
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: Monday, May 04, 2015 6:44 PM
> >>
> >>>>> On 04.05.15 at 12:39, <tiejun.chen@xxxxxxxxx> wrote:
> >>> On 2015/5/4 16:52, Jan Beulich wrote:
> >>>>>>> On 04.05.15 at 04:16, <tiejun.chen@xxxxxxxxx> wrote:
> >>>>> --- a/xen/drivers/passthrough/vtd/x86/vtd.c
> >>>>> +++ b/xen/drivers/passthrough/vtd/x86/vtd.c
> >>>>> @@ -56,7 +56,9 @@ unsigned int get_cache_line_size(void)
> >>>>>
> >>>>>    void cacheline_flush(char * addr)
> >>>>>    {
> >>>>> +    mb();
> >>>>>        clflush(addr);
> >>>>> +    mb();
> >>>>>    }
> >>>>
> >>>> I think the purpose of the flush is to force write back, not to evict
> >>>> the cache line, and if so wmb() would appear to be sufficient. As
> >>>> the SDM says that's not the case, a comment explaining why wmb()
> >>>> is not sufficient would seem necessary. Plus in the description I
> >>>
> >>> Seems wmb() is not sufficient here.
> >>>
> >>> "CLFLUSH is only ordered by the MFENCE instruction. It is not guaranteed
> >>> to be ordered by any other fencing, serializing or other CLFLUSH
> >>> instruction."
> >>
> >> Right - that's what I said in the second sentence.
> >>
> >
> > btw why do we need two fences here? Suppose we just care about
> > writes before the flush point...
> >
> 
> The first MFENCE guarantees all load/store visible before flush cache
> line. But the second MFENCE just makes sure CLFLUSH is not ordered by
> that ensuing load/store, right?
> 

It's not an usual case to have another store to same destination address
right after CLFLUSH. Usually there's some protocol to do another update
until device completes access. Store to different address is not a concern.
But yes if just looking at this interface alone, we'd better not make
assumption about the caller so two mb() look necessary...

btw as Jan commented in another thread, if there's no other caller of
this function, it might be more reasonable to expand it into the bigger
loop in __iommu_flush_cache so you don't need to fence for each
cache line flush.

Thanks
Kevin

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