Re: x86 swiotlb questions

[Muli Ben-Yehuda <muli@il.ibm.com>]

On Fri, Dec 22, 2006 at 02:49:53PM +0000, Jan Beulich wrote:

> Patch update, fixing a bug on x86/PAE, and making
> include/xen/swiotlb.h look a lot nicer (but still not really
> nice). My plan is to submit the non-Xen ones to lkml right after New
> Year, unless I hear negative feedback.

> >Patch order is
> >swiotlb-bugs.patch
> >swiotlb-bus.patch
> >swiotlb-cleanup.patch
> >swiotlb-split.patch
> >xen-swiotlb.patch

Comments inline.

swiotlb-bugs.patch:

[snip]

>  /*
> @@ -758,8 +739,10 @@ swiotlb_sync_sg(struct device *hwdev, st
>  
>  	for (i = 0; i < nelems; i++, sg++)
>  		if (sg->dma_address != SG_ENT_PHYS_ADDRESS(sg))
> -			sync_single(hwdev, (void *) sg->dma_address,
> +			sync_single(hwdev, phys_to_virt(sg->dma_address),
>  				    sg->dma_length, dir, target);

Fix looks correct and bug looks painful. I think you should send this
one to mainline immediately.

swiotlb-bus.patch:

> Convert all phys_to_virt/virt_to_phys uses to
> bus_to_virt/virt_to_bus.

"... because Xen needs it", otherwise someone is bound to ask why, as
all other archs define _bus as _phys.

> Signed-off-by: Jan Beulich <jbeulich@novell.com>

Acked-by: Muli Ben-Yehuda <muli@il.ibm.com>

swiotlb-cleanup:

> This patch
> - adds proper __init decoration to swiotlb's init code (and the code calling
>   it, where not already the case)
> - replaces uses of 'unsigned long' with dma_addr_t where appropriate
> - does miscellaneous simplicfication and cleanup
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>

Looks good in general, not acking yet because I want to give it a spin
first.

swiotlb-split.patch:

> This patch adds abstraction so that the file can be used by environments other
> than IA64 and EM64T, namely for Xen.

I'm sorry, but this patch is horrible. swiotlb.c is now pretty much
unreadable. I'd be surprised if mainline accepted it - I would
certainly NAK it with my mainline hat on, especially for an unmerged
architecture.

If Xen needs so many "abstractions", I have to ask whether it isn't
better off just using its own swiotlb.c as we are now.

I'll take another look at this later and try to come up with a
different way of merging them that isn't quite this horrible. Maybe
using function pointers for the "low level" operations?

Cheers,
Muli