Re: readq/writeq on 32bit machines

[ebiederman@lnxi.com (Eric W. Biederman)]

Roland Dreier <roland@topspin.com> writes:

> Thanks for posting this Eric... I sent a less detailed reply yesterday
> (pointing out that atomic writeq is needed sometimes) but it seems to
> have gotten eaten.
> 
>     Eric> This issue came last night on the openib list.  The driver
>     Eric> currently rolls it's own version of writeq and in the case
>     Eric> where there is not an atomic 64bit write it needs to a
>     Eric> spinlock to make certain things don't get out of order.  The
>     Eric> driver fails with the current 2 writel() version.
> 
>     Eric> Here is an SSE version, that should not be to intrusive.
>     Eric> According to intel's docs a 64bit aligned 64bit write is
>     Eric> atomic all of the way back to the Pentium.  If
>     Eric> kernel_fpu_begin/kernel_fpu_end are safe in interrupt
>     Eric> context we can do an atomic/correct version of writeq for
>     Eric> x86 processors that don't support sse as well.  Although I
>     Eric> don't know if we want to.
> 
> 	static inline void __raw_writeq(u64 val, unsigned long dest)
> 	{
> 		unsigned long cr0;
> 		u64 xmmsave __attribute__((aligned(8));
> 		preempt_disable();
> 	        cr0 = read_cr0();
> 		clts();
> 	        asm volatile (
> 			"movlps %%xmm0,(%0); \n\t"
> 			"movlps (%2),%%xmm0; \n\t"
> 			"movlps %%xmm0,(%1); \n\t"
> 			"movlps (%0),%%xmm0; \n\t"
> 			: =m (&xmmsave), "=m" ((void *)dest)
> 			: "m" (&val)
> 			);
> 		write_cr0(cr0);
> 		preempt_enable();
> 	}
> 
> This is pretty much what I wrote in the above-mentioned openib
> driver.  However I'm worried about using 
> 
>         u64 xmmsave __attribute__((aligned(8));
> 
> for a stack variable.  I don't think gcc respects the alignment
> attribute for stack variables (I've had a problem in the past using
> movdqa to a stack variable, even if I do __attribute__((aligned(16))).
> If we're sure gcc aligns xmmsave properly, stick a comment in and
> leave out the __attribute__; if not then I think we have to do

I picked that up out of xor.h where the raid code does something similar,
so if there is a problem it needs to be fixed there as well.

> 
>         u8 xmmsave[8 + 7];
> 
> and then use ~7 & (xmmsave + 7).
> 
>     Eric> Thinking about this a little more we might be able to get
>     Eric> away with.
> 
> 	static inline void __raw_writeq(u64 val, unsigned long dest)
> 	{
> 		unsigned long flags;
> 	        local_irq_save(flags);
> 		writel(val & 0xffffffff, addr);
> 		writel(val >> 32, addr + 4);
> 	        irq_restore(flags);
> 	}
> 
> I don't think this is good enough on SMP.  In the openib case, it's
> entirely possible for one CPU to be ringing a (64-bit) work queue
> doorbell at the same time as another CPU is ringing a (64-bit)
> completion queue doorbell, and if the 32-bit halves of those doorbells
> get interleaved, the hardware gets confused.  Maybe there's some magic
> aspect of the PC hardware that ensures this can't happen but I'd hate
> to count on it without some very good documentation.

Right.  It does make the window incredibly small though.  I am even
nervous that the version with a spinlock might break, if something really
needs an atomic guarantee.  


Eric