RE: [Xen-devel] Support for AGP aperture as IOMMU in AMD64 mode [2/2]

["Langsdorf, Mark" <mark.langsdorf@xxxxxxx>]

> -----Original Message-----
> From: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx 
> [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxx] On Behalf Of 
> Keir Fraser
> Sent: Thursday, January 19, 2006 1:32 PM
> To: Langsdorf, Mark
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [Xen-devel] Support for AGP aperture as IOMMU in 
> AMD64 mode [2/2]
> 
> 
> 
> On 16 Jan 2006, at 23:50, Langsdorf, Mark wrote:
> 
> > These are the diffs against the pristine versions of 
> > arch/x86_64/kernel/[aperture.c,pci-gart.c] to better show 
> the changes 
> > necessary to adapt those files to Xen.
> >
> > They were included with the patch and should not be
> > applied again.
> 
> Changes to these files will have to be merged upstream into 
> the native x86/64 files. Hence they need cleaning up and
> posting to linux-kernel and Andi Kleen. At the moment they
> don't quite pass muster.

Some of those change are definitely Xen specific, such as the
switch from virt_to_phys() to virt_to_bus around line 416,
and the switch from __get_free_pages to alloc_gatt_pages near
line 740.  Similarly, I had to introduce some hypervisor calls
to get the true size of memory so that the GART is enabled even
if dom0 has less than 4 GB of memory.
 
> A few things I can see are: why disable call to e820_mapped()? 

Couldn't get the implementation of e820_mapped() to work right, 
and missed I had the debug statement in there still.  Any ideas
on what I'm doing wrong in e820_mapped?

> virt_to_gart/gart_to_virt should be moved to our agp.h if we
> want to keep them. Alternatively you only use them a couple
> of times so expanding them at the call site would be okay.

Done.

> You unconditionally allocate a table to the 'gatt' variable, but 
> only set the agp_gatt_table variable if it is NULL. Should 
> you free the table if agp_gatt_table!=NULL? Can that ever happen,
> and if so why not on native?

agp_gatt_table is set in the AGP code on native.  I can't figure
out why Xen isn't setting it right, hence the work-around.

Looking at that code again, the else statement makes no sense and
should probably be removed.

> The big patch you sent out we also need to go through in some detail. 
> It's rather bigger than I would have expected. Hopefully 
> there is some possibility of cleaning up and keeping things closer
> to the native original source files.

Most of the big patch is adding 3 mostly unmodified files
from arch/x86_64/kernel to arch/xen/x86_64/kernel.  The
rest of the code changes are pretty minor.

If you want, I can restructure the patch to reflect -
submit 1 patch to add pci-dma.c, pci-gart.c, and 
aperture.c, and another set of patches to reflect the
incremental changes to those files.  Would that help?

-Mark Langsdorf
AMD, Inc.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel