[PATCH] Xen/i386 cleanups - AGP bus/phys cleanups

[PATCH] Xen/i386 cleanups - AGP bus/phys cleanups

[Keir Fraser]

[-- Attachment #1: Type: text/plain, Size: 502 bytes --]

This patch cleans up AGP driver treatment of bus/device memory. Every
use of virt_to_phys/phys_to_virt should properly be converting between
virtual and bus addresses: this distinction really matters for the Xen
hypervisor.

Furthermore, when allocating the GATT, it is necessary to use
dma_alloc_coherent rather than get_free_pages.  Again, not a
distinction that matters for i386, but very important for Xen and
possibly for other architectures too.

Signed-off-by: Keir Fraser <keir@xensource.com>


[-- Attachment #2: AGP patch --]
[-- Type: text/plain, Size: 11741 bytes --]

--- linux-2.6-old/drivers/char/agp/ali-agp.c	2005-03-16 10:30:25 +00:00
+++ linux-2.6-new/drivers/char/agp/ali-agp.c	2005-03-16 10:16:30 +00:00
@@ -150,7 +150,7 @@
 	pci_read_config_dword(agp_bridge->dev, ALI_CACHE_FLUSH_CTRL, &temp);
 	pci_write_config_dword(agp_bridge->dev, ALI_CACHE_FLUSH_CTRL,
 			(((temp & ALI_CACHE_FLUSH_ADDR_MASK) |
-			  virt_to_phys(addr)) | ALI_CACHE_FLUSH_EN ));
+			  virt_to_bus(addr)) | ALI_CACHE_FLUSH_EN ));
 	return addr;
 }
 
@@ -174,7 +174,7 @@
 	pci_read_config_dword(agp_bridge->dev, ALI_CACHE_FLUSH_CTRL, &temp);
 	pci_write_config_dword(agp_bridge->dev, ALI_CACHE_FLUSH_CTRL,
 			(((temp & ALI_CACHE_FLUSH_ADDR_MASK) |
-			  virt_to_phys(addr)) | ALI_CACHE_FLUSH_EN));
+			  virt_to_bus(addr)) | ALI_CACHE_FLUSH_EN));
 	agp_generic_destroy_page(addr);
 }
 
--- linux-2.6-old/drivers/char/agp/amd-k7-agp.c	2005-03-16 10:30:25 +00:00
+++ linux-2.6-new/drivers/char/agp/amd-k7-agp.c	2005-03-16 10:17:18 +00:00
@@ -43,7 +43,7 @@
 
 	SetPageReserved(virt_to_page(page_map->real));
 	global_cache_flush();
-	page_map->remapped = ioremap_nocache(virt_to_phys(page_map->real),
+	page_map->remapped = ioremap_nocache(virt_to_bus(page_map->real),
 					    PAGE_SIZE);
 	if (page_map->remapped == NULL) {
 		ClearPageReserved(virt_to_page(page_map->real));
@@ -154,7 +154,7 @@
 
 	agp_bridge->gatt_table_real = (u32 *)page_dir.real;
 	agp_bridge->gatt_table = (u32 __iomem *)page_dir.remapped;
-	agp_bridge->gatt_bus_addr = virt_to_phys(page_dir.real);
+	agp_bridge->gatt_bus_addr = virt_to_bus(page_dir.real);
 
 	/* Get the address for the gart region.
 	 * This is a bus address even on the alpha, b/c its
@@ -167,7 +167,7 @@
 
 	/* Calculate the agp offset */
 	for (i = 0; i < value->num_entries / 1024; i++, addr += 0x00400000) {
-		writel(virt_to_phys(amd_irongate_private.gatt_pages[i]->real) | 1,
+		writel(virt_to_bus(amd_irongate_private.gatt_pages[i]->real) | 1,
 			page_dir.remapped+GET_PAGE_DIR_OFF(addr));
 		readl(page_dir.remapped+GET_PAGE_DIR_OFF(addr));	/* PCI Posting. */
 	}
--- linux-2.6-old/drivers/char/agp/amd64-agp.c	2005-03-16 10:30:25 +00:00
+++ linux-2.6-new/drivers/char/agp/amd64-agp.c	2005-03-16 10:18:00 +00:00
@@ -219,7 +219,7 @@
 
 static int amd_8151_configure(void)
 {
-	unsigned long gatt_bus = virt_to_phys(agp_bridge->gatt_table_real);
+	unsigned long gatt_bus = virt_to_bus(agp_bridge->gatt_table_real);
 
 	/* Configure AGP regs in each x86-64 host bridge. */
 	for_each_nb() {
@@ -591,7 +591,7 @@
 {
 	struct agp_bridge_data *bridge = pci_get_drvdata(pdev);
 
-	release_mem_region(virt_to_phys(bridge->gatt_table_real),
+	release_mem_region(virt_to_bus(bridge->gatt_table_real),
 			   amd64_aperture_sizes[bridge->aperture_size_idx].size);
 	agp_remove_bridge(bridge);
 	agp_put_bridge(bridge);
--- linux-2.6-old/drivers/char/agp/ati-agp.c	2005-03-16 10:30:25 +00:00
+++ linux-2.6-new/drivers/char/agp/ati-agp.c	2005-03-16 10:18:31 +00:00
@@ -61,7 +61,7 @@
 
 	SetPageReserved(virt_to_page(page_map->real));
 	err = map_page_into_agp(virt_to_page(page_map->real));
-	page_map->remapped = ioremap_nocache(virt_to_phys(page_map->real),
+	page_map->remapped = ioremap_nocache(virt_to_bus(page_map->real),
 					    PAGE_SIZE);
 	if (page_map->remapped == NULL || err) {
 		ClearPageReserved(virt_to_page(page_map->real));
--- linux-2.6-old/drivers/char/agp/backend.c	2005-03-16 10:30:25 +00:00
+++ linux-2.6-new/drivers/char/agp/backend.c	2005-03-16 10:23:50 +00:00
@@ -147,7 +147,7 @@
 			return -ENOMEM;
 		}
 
-		bridge->scratch_page_real = virt_to_phys(addr);
+		bridge->scratch_page_real = virt_to_bus(addr);
 		bridge->scratch_page =
 		    bridge->driver->mask_memory(bridge, bridge->scratch_page_real, 0);
 	}
@@ -188,7 +188,7 @@
 err_out:
 	if (bridge->driver->needs_scratch_page)
 		bridge->driver->agp_destroy_page(
-				phys_to_virt(bridge->scratch_page_real));
+				bus_to_virt(bridge->scratch_page_real));
 	if (got_gatt)
 		bridge->driver->free_gatt_table(bridge);
 	if (got_keylist) {
@@ -213,7 +213,7 @@
 	if (bridge->driver->agp_destroy_page &&
 	    bridge->driver->needs_scratch_page)
 		bridge->driver->agp_destroy_page(
-				phys_to_virt(bridge->scratch_page_real));
+				bus_to_virt(bridge->scratch_page_real));
 }
 
 /* When we remove the global variable agp_bridge from all drivers
--- linux-2.6-old/drivers/char/agp/efficeon-agp.c	2005-03-16 10:30:25 +00:00
+++ linux-2.6-new/drivers/char/agp/efficeon-agp.c	2005-03-16 10:19:15 +00:00
@@ -219,7 +219,7 @@
 
 		efficeon_private.l1_table[index] = page;
 
-		value = __pa(page) | pati | present | index;
+		value = virt_to_bus(page) | pati | present | index;
 
 		pci_write_config_dword(agp_bridge->dev,
 			EFFICEON_ATTPAGE, value);
--- linux-2.6-old/drivers/char/agp/generic.c	2005-03-16 10:30:25 +00:00
+++ linux-2.6-new/drivers/char/agp/generic.c	2005-03-16 10:27:34 +00:00
@@ -152,7 +152,7 @@
 	}
 	if (curr->page_count != 0) {
 		for (i = 0; i < curr->page_count; i++) {
-			curr->bridge->driver->agp_destroy_page(phys_to_virt(curr->memory[i]));
+			curr->bridge->driver->agp_destroy_page(bus_to_virt(curr->memory[i]));
 		}
 	}
 	agp_free_key(curr->key);
@@ -208,7 +208,7 @@
 			agp_free_memory(new);
 			return NULL;
 		}
-		new->memory[i] = virt_to_phys(addr);
+		new->memory[i] = virt_to_bus(addr);
 		new->page_count++;
 	}
        new->bridge = bridge;
@@ -767,6 +767,7 @@
 	int i;
 	void *temp;
 	struct page *page;
+	dma_addr_t dma;
 
 	/* The generic routines can't handle 2 level gatt's */
 	if (bridge->driver->size_type == LVL2_APER_SIZE)
@@ -805,8 +806,10 @@
 				break;
 			}
 
-			table = (char *) __get_free_pages(GFP_KERNEL,
-							  page_order);
+			table = dma_alloc_coherent(
+					&agp_bridge->dev->dev,
+					PAGE_SIZE << page_order, &dma,
+					GFP_KERNEL);
 
 			if (table == NULL) {
 				i++;
@@ -837,7 +840,9 @@
 		size = ((struct aper_size_info_fixed *) temp)->size;
 		page_order = ((struct aper_size_info_fixed *) temp)->page_order;
 		num_entries = ((struct aper_size_info_fixed *) temp)->num_entries;
-		table = (char *) __get_free_pages(GFP_KERNEL, page_order);
+		table = dma_alloc_coherent(
+				&agp_bridge->dev->dev,
+				PAGE_SIZE << page_order, &dma, GFP_KERNEL);
 	}
 
 	if (table == NULL)
@@ -852,7 +857,7 @@
 	agp_gatt_table = (void *)table;
 
 	bridge->driver->cache_flush();
-	bridge->gatt_table = ioremap_nocache(virt_to_phys(table),
+	bridge->gatt_table = ioremap_nocache(virt_to_bus(table),
 					(PAGE_SIZE * (1 << page_order)));
 	bridge->driver->cache_flush();
 
@@ -860,11 +865,12 @@
 		for (page = virt_to_page(table); page <= virt_to_page(table_end); page++)
 			ClearPageReserved(page);
 
-		free_pages((unsigned long) table, page_order);
+		dma_free_coherent(&agp_bridge->dev->dev, PAGE_SIZE<<page_order,
+					table, dma);
 
 		return -ENOMEM;
 	}
-	bridge->gatt_bus_addr = virt_to_phys(bridge->gatt_table_real);
+	bridge->gatt_bus_addr = virt_to_bus(bridge->gatt_table_real);
 
 	/* AK: bogus, should encode addresses > 4GB */
 	for (i = 0; i < num_entries; i++) {
@@ -918,7 +924,8 @@
 	for (page = virt_to_page(table); page <= virt_to_page(table_end); page++)
 		ClearPageReserved(page);
 
-	free_pages((unsigned long) bridge->gatt_table_real, page_order);
+	dma_free_coherent(&agp_bridge->dev->dev, PAGE_SIZE<<page_order,
+		agp_bridge->gatt_table_real, agp_bridge->gatt_bus_addr);
 
 	agp_gatt_table = NULL;
 	bridge->gatt_table = NULL;
--- linux-2.6-old/drivers/char/agp/hp-agp.c	2005-03-16 10:30:25 +00:00
+++ linux-2.6-new/drivers/char/agp/hp-agp.c	2005-03-16 10:20:16 +00:00
@@ -110,7 +110,7 @@
 	hp->gart_size = HP_ZX1_GART_SIZE;
 	hp->gatt_entries = hp->gart_size / hp->io_page_size;
 
-	hp->io_pdir = phys_to_virt(readq(hp->ioc_regs+HP_ZX1_PDIR_BASE));
+	hp->io_pdir = bus_to_virt(readq(hp->ioc_regs+HP_ZX1_PDIR_BASE));
 	hp->gatt = &hp->io_pdir[HP_ZX1_IOVA_TO_PDIR(hp->gart_base)];
 
 	if (hp->gatt[0] != HP_ZX1_SBA_IOMMU_COOKIE) {
@@ -248,7 +248,7 @@
 	agp_bridge->mode = readl(hp->lba_regs+hp->lba_cap_offset+PCI_AGP_STATUS);
 
 	if (hp->io_pdir_owner) {
-		writel(virt_to_phys(hp->io_pdir), hp->ioc_regs+HP_ZX1_PDIR_BASE);
+		writel(virt_to_bus(hp->io_pdir), hp->ioc_regs+HP_ZX1_PDIR_BASE);
 		readl(hp->ioc_regs+HP_ZX1_PDIR_BASE);
 		writel(hp->io_tlb_ps, hp->ioc_regs+HP_ZX1_TCNFG);
 		readl(hp->ioc_regs+HP_ZX1_TCNFG);
--- linux-2.6-old/drivers/char/agp/i460-agp.c	2005-03-16 10:30:25 +00:00
+++ linux-2.6-new/drivers/char/agp/i460-agp.c	2005-03-16 10:21:29 +00:00
@@ -372,7 +372,7 @@
 	}
 	memset(lp->alloced_map, 0, map_size);
 
-	lp->paddr = virt_to_phys(lpage);
+	lp->paddr = virt_to_bus(lpage);
 	lp->refcount = 0;
 	atomic_add(I460_KPAGES_PER_IOPAGE, &agp_bridge->current_memory_agp);
 	return 0;
@@ -383,7 +383,7 @@
 	kfree(lp->alloced_map);
 	lp->alloced_map = NULL;
 
-	free_pages((unsigned long) phys_to_virt(lp->paddr), I460_IO_PAGE_SHIFT - PAGE_SHIFT);
+	free_pages((unsigned long) bus_to_virt(lp->paddr), I460_IO_PAGE_SHIFT - PAGE_SHIFT);
 	atomic_sub(I460_KPAGES_PER_IOPAGE, &agp_bridge->current_memory_agp);
 }
 
--- linux-2.6-old/drivers/char/agp/intel-agp.c	2005-03-16 10:30:25 +00:00
+++ linux-2.6-new/drivers/char/agp/intel-agp.c	2005-03-16 10:22:08 +00:00
@@ -286,7 +286,7 @@
 	if (new == NULL)
 		return NULL;
 
-	new->memory[0] = virt_to_phys(addr);
+	new->memory[0] = virt_to_bus(addr);
 	if (pg_count == 4) {
 		/* kludge to get 4 physical pages for ARGB cursor */
 		new->memory[1] = new->memory[0] + PAGE_SIZE;
@@ -329,10 +329,10 @@
 	agp_free_key(curr->key);
 	if(curr->type == AGP_PHYS_MEMORY) {
 		if (curr->page_count == 4)
-			i8xx_destroy_pages(phys_to_virt(curr->memory[0]));
+			i8xx_destroy_pages(bus_to_virt(curr->memory[0]));
 		else
 			agp_bridge->driver->agp_destroy_page(
-				 phys_to_virt(curr->memory[0]));
+				 bus_to_virt(curr->memory[0]));
 		vfree(curr->memory);
 	}
 	kfree(curr);
--- linux-2.6-old/drivers/char/agp/sworks-agp.c	2005-03-16 10:30:25 +00:00
+++ linux-2.6-new/drivers/char/agp/sworks-agp.c	2005-03-16 10:22:49 +00:00
@@ -51,7 +51,7 @@
 	}
 	SetPageReserved(virt_to_page(page_map->real));
 	global_cache_flush();
-	page_map->remapped = ioremap_nocache(virt_to_phys(page_map->real), 
+	page_map->remapped = ioremap_nocache(virt_to_bus(page_map->real), 
 					    PAGE_SIZE);
 	if (page_map->remapped == NULL) {
 		ClearPageReserved(virt_to_page(page_map->real));
@@ -162,7 +162,7 @@
 	/* Create a fake scratch directory */
 	for(i = 0; i < 1024; i++) {
 		writel(agp_bridge->scratch_page, serverworks_private.scratch_dir.remapped+i);
-		writel(virt_to_phys(serverworks_private.scratch_dir.real) | 1, page_dir.remapped+i);
+		writel(virt_to_bus(serverworks_private.scratch_dir.real) | 1, page_dir.remapped+i);
 	}
 
 	retval = serverworks_create_gatt_pages(value->num_entries / 1024);
@@ -174,7 +174,7 @@
 
 	agp_bridge->gatt_table_real = (u32 *)page_dir.real;
 	agp_bridge->gatt_table = (u32 __iomem *)page_dir.remapped;
-	agp_bridge->gatt_bus_addr = virt_to_phys(page_dir.real);
+	agp_bridge->gatt_bus_addr = virt_to_bus(page_dir.real);
 
 	/* Get the address for the gart region.
 	 * This is a bus address even on the alpha, b/c its
@@ -187,7 +187,7 @@
 	/* Calculate the agp offset */	
 
 	for(i = 0; i < value->num_entries / 1024; i++)
-		writel(virt_to_phys(serverworks_private.gatt_pages[i]->real)|1, page_dir.remapped+i);
+		writel(virt_to_bus(serverworks_private.gatt_pages[i]->real)|1, page_dir.remapped+i);
 
 	return 0;
 }
--- linux-2.6-old/drivers/char/agp/uninorth-agp.c	2005-03-16 10:30:25 +00:00
+++ linux-2.6-new/drivers/char/agp/uninorth-agp.c	2005-03-16 10:23:03 +00:00
@@ -378,7 +378,7 @@
 
 	bridge->gatt_table_real = (u32 *) table;
 	bridge->gatt_table = (u32 *)table;
-	bridge->gatt_bus_addr = virt_to_phys(table);
+	bridge->gatt_bus_addr = virt_to_bus(table);
 
 	for (i = 0; i < num_entries; i++)
 		bridge->gatt_table[i] = 0;

Re: [PATCH] Xen/i386 cleanups - AGP bus/phys cleanups

[Christoph Hellwig]

On Wed, Mar 16, 2005 at 11:48:29AM +0000, Keir Fraser wrote:
> This patch cleans up AGP driver treatment of bus/device memory. Every
> use of virt_to_phys/phys_to_virt should properly be converting between
> virtual and bus addresses: this distinction really matters for the Xen
> hypervisor.

It's bogus either way.  You must never use virt_to_phys or virt_to_bus
for bus address.  For systems with an IOMMU there's no 1:1 mapping.

Re: [PATCH] Xen/i386 cleanups - AGP bus/phys cleanups

[Keir Fraser]

> On Wed, Mar 16, 2005 at 11:48:29AM +0000, Keir Fraser wrote:
> > This patch cleans up AGP driver treatment of bus/device memory. Every
> > use of virt_to_phys/phys_to_virt should properly be converting between
> > virtual and bus addresses: this distinction really matters for the Xen
> > hypervisor.
> 
> It's bogus either way.  You must never use virt_to_phys or virt_to_bus
> for bus address.  For systems with an IOMMU there's no 1:1 mapping.

Well, I'd say it's less bogus: it makes the intention clearer, and it
is a 'good enough' improvement for Xen. Indeed, it's good enough for
all the architectures that actually use the AGP drivers (all have no
IOMMU, or an IOMMU only to support legacy 32-bit I/O interfaces). 

 -- Keir

Re: [PATCH] Xen/i386 cleanups - AGP bus/phys cleanups

[Rik van Riel]

On Wed, 16 Mar 2005, Christoph Hellwig wrote:
> On Wed, Mar 16, 2005 at 11:48:29AM +0000, Keir Fraser wrote:
> > This patch cleans up AGP driver treatment of bus/device memory. Every
> > use of virt_to_phys/phys_to_virt should properly be converting between
> > virtual and bus addresses: this distinction really matters for the Xen
> > hypervisor.
> 
> It's bogus either way.  You must never use virt_to_phys or virt_to_bus
> for bus address.  For systems with an IOMMU there's no 1:1 mapping.

In the case of AGP, the AGPGART effectively _is_ the
IOMMU.  Calculating the addresses right for programming
the AGPGART is probably worth fixing.

-- 
"Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it." - Brian W. Kernighan

Re: [PATCH] Xen/i386 cleanups - AGP bus/phys cleanups

[Christoph Hellwig]

On Wed, Mar 16, 2005 at 10:01:07AM -0500, Rik van Riel wrote:
> In the case of AGP, the AGPGART effectively _is_ the
> IOMMU.  Calculating the addresses right for programming
> the AGPGART is probably worth fixing.

Well, it's a half-assed one.  And some systems have a real one.

But the real problem is that virt_to_bus doesn't exist at all
on architectures like ppc64, and this patch touches files like
generic.c and backend.c that aren't PC-specific.   So you
effectively break agp support for them.

Re: [PATCH] Xen/i386 cleanups - AGP bus/phys cleanups

[Keir Fraser]

On 16 Mar 2005, at 18:10, Christoph Hellwig wrote:

> On Wed, Mar 16, 2005 at 10:01:07AM -0500, Rik van Riel wrote:
>> In the case of AGP, the AGPGART effectively _is_ the
>> IOMMU.  Calculating the addresses right for programming
>> the AGPGART is probably worth fixing.
>
> Well, it's a half-assed one.  And some systems have a real one.
>
> But the real problem is that virt_to_bus doesn't exist at all
> on architectures like ppc64, and this patch touches files like
> generic.c and backend.c that aren't PC-specific.   So you
> effectively break agp support for them.

The AGP driver is only configurable for ppc32, alpha, x86, x86_64 and 
ia64, all of which have virt_to_bus.

  -- Keir

Re: [PATCH] Xen/i386 cleanups - AGP bus/phys cleanups

[Jesse Barnes]

On Wednesday, March 16, 2005 10:35 am, Keir Fraser wrote:
> On 16 Mar 2005, at 18:10, Christoph Hellwig wrote:
> > On Wed, Mar 16, 2005 at 10:01:07AM -0500, Rik van Riel wrote:
> >> In the case of AGP, the AGPGART effectively _is_ the
> >> IOMMU.  Calculating the addresses right for programming
> >> the AGPGART is probably worth fixing.
> >
> > Well, it's a half-assed one.  And some systems have a real one.
> >
> > But the real problem is that virt_to_bus doesn't exist at all
> > on architectures like ppc64, and this patch touches files like
> > generic.c and backend.c that aren't PC-specific.   So you
> > effectively break agp support for them.
>
> The AGP driver is only configurable for ppc32, alpha, x86, x86_64 and
> ia64, all of which have virt_to_bus.

Yeah, but that doesn't mean it makes sense on all those platforms.  The 
biggest problem with virt_to_bus (well, depending on who you talk to) is that 
it can't handle systems where the address translation must be done 
differently depending on *which* bus we're getting a bus address for.  Not 
sure what makes sense in this case though... is the DMA mapping interface 
appropriate?

Jesse

Re: [PATCH] Xen/i386 cleanups - AGP bus/phys cleanups

[Keir Fraser]

On 16 Mar 2005, at 18:42, Jesse Barnes wrote:

>> The AGP driver is only configurable for ppc32, alpha, x86, x86_64 and
>> ia64, all of which have virt_to_bus.
>
> Yeah, but that doesn't mean it makes sense on all those platforms.  The
> biggest problem with virt_to_bus (well, depending on who you talk to) 
> is that
> it can't handle systems where the address translation must be done
> differently depending on *which* bus we're getting a bus address for.  
> Not
> sure what makes sense in this case though... is the DMA mapping 
> interface
> appropriate?

I think that makes sense. How much code refactoring is needed to make 
use of it, though?

Certainly it will work for Xen and it sounds better than 
virt_to_bus/bus_to_virt, if someone will cook up the alternative patch. 
Otherwise, virt_to_bus seems better than the status quo. :-)

  -- Keir

Re: [PATCH] Xen/i386 cleanups - AGP bus/phys cleanups

[Christoph Hellwig]

On Wed, Mar 16, 2005 at 06:35:28PM +0000, Keir Fraser wrote:
> 
> On 16 Mar 2005, at 18:10, Christoph Hellwig wrote:
> 
> >On Wed, Mar 16, 2005 at 10:01:07AM -0500, Rik van Riel wrote:
> >>In the case of AGP, the AGPGART effectively _is_ the
> >>IOMMU.  Calculating the addresses right for programming
> >>the AGPGART is probably worth fixing.
> >
> >Well, it's a half-assed one.  And some systems have a real one.
> >
> >But the real problem is that virt_to_bus doesn't exist at all
> >on architectures like ppc64, and this patch touches files like
> >generic.c and backend.c that aren't PC-specific.   So you
> >effectively break agp support for them.
> 
> The AGP driver is only configurable for ppc32, alpha, x86, x86_64 and 
> ia64, all of which have virt_to_bus.

and ppc64 now, which doesn't.

Re: [PATCH] Xen/i386 cleanups - AGP bus/phys cleanups

[Keir Fraser]

On 16 Mar 2005, at 19:06, Christoph Hellwig wrote:

>> The AGP driver is only configurable for ppc32, alpha, x86, x86_64 and
>> ia64, all of which have virt_to_bus.
>
> and ppc64 now, which doesn't.

Sounds like the new DMA-mapping interface is the way to go then.

  -- Keir

Re: [PATCH] Xen/i386 cleanups - AGP bus/phys cleanups

[Alan Cox]

On Mer, 2005-03-16 at 14:31, Christoph Hellwig wrote:
> It's bogus either way.  You must never use virt_to_phys or virt_to_bus
> for bus address.  For systems with an IOMMU there's no 1:1 mapping.

The AGPGART _is_ the IOMMU.

Alan

Re: [PATCH] Xen/i386 cleanups - AGP bus/phys cleanups

[Jesse Barnes]

On Wednesday, March 16, 2005 3:48 am, Keir Fraser wrote:
> This patch cleans up AGP driver treatment of bus/device memory. Every
> use of virt_to_phys/phys_to_virt should properly be converting between
> virtual and bus addresses: this distinction really matters for the Xen
> hypervisor.
>
> Furthermore, when allocating the GATT, it is necessary to use
> dma_alloc_coherent rather than get_free_pages.  Again, not a
> distinction that matters for i386, but very important for Xen and
> possibly for other architectures too.
>
> Signed-off-by: Keir Fraser <keir@xensource.com>

Ugg, if we're messing with these drivers we may as well fix them properly 
rather than adding in more uses of the broken virt_to_bus interface...

Jesse

Re: [PATCH] Xen/i386 cleanups - AGP bus/phys cleanups

[Paul Mackerras]

Keir Fraser writes:

> This patch cleans up AGP driver treatment of bus/device memory. Every
> use of virt_to_phys/phys_to_virt should properly be converting between
> virtual and bus addresses: this distinction really matters for the Xen
> hypervisor.

I think you are misunderstanding the distinction between physical
addresses and bus addresses.  Specifically, it seems wrong to me to be
putting bus addresses into the GATT rather than physical addresses.

For example, on my G5, physical addresses are 36 bits (in the
hardware) but bus addresses are only 32 bits.  Using bus addresses in
the GATT would mean that I couldn't put any memory above the 4GB point
into the GATT.

The distinction is that physical addresses are what are used to access
physical memory, whereas bus addresses are what appears on some
external bus (usually PCI).  The GATT sits between an external (AGP)
bus and memory, so while the GATT is indexed using bus addresses, its
entries contain physical addresses.  So in fact virt_to_phys is the
correct thing to use to calculate values to put in GATT entries.

Paul.

Re: [PATCH] Xen/i386 cleanups - AGP bus/phys cleanups

[Jesse Barnes]

On Wednesday, March 16, 2005 1:24 pm, Paul Mackerras wrote:
> Keir Fraser writes:
> > This patch cleans up AGP driver treatment of bus/device memory. Every
> > use of virt_to_phys/phys_to_virt should properly be converting between
> > virtual and bus addresses: this distinction really matters for the Xen
> > hypervisor.
>
> I think you are misunderstanding the distinction between physical
> addresses and bus addresses.  Specifically, it seems wrong to me to be
> putting bus addresses into the GATT rather than physical addresses.
>
> For example, on my G5, physical addresses are 36 bits (in the
> hardware) but bus addresses are only 32 bits.  Using bus addresses in
> the GATT would mean that I couldn't put any memory above the 4GB point
> into the GATT.
>
> The distinction is that physical addresses are what are used to access
> physical memory, whereas bus addresses are what appears on some
> external bus (usually PCI).  The GATT sits between an external (AGP)
> bus and memory, so while the GATT is indexed using bus addresses, its
> entries contain physical addresses.  So in fact virt_to_phys is the
> correct thing to use to calculate values to put in GATT entries.

Thanks for the explanation Paul, now the code actually makes sense.  
Converting to the DMA mapping API doesn't make sense at all in this context 
then, since we're basically programming the GATT (an IOMMU type table) with 
physical addresses.  Ken, are you sure you need to make these changes at all?  
Does Xen break w/o them?

Jesse

Re: [PATCH] Xen/i386 cleanups - AGP bus/phys cleanups

[Rik van Riel]

On Wed, 16 Mar 2005, Jesse Barnes wrote:

> Thanks for the explanation Paul, now the code actually makes sense.  
> Converting to the DMA mapping API doesn't make sense at all in this context 
> then, since we're basically programming the GATT (an IOMMU type table) with 
> physical addresses.  Ken, are you sure you need to make these changes at all?  
> Does Xen break w/o them?

Thing is, the rest of the kernel uses virt_to_phys for
two different things.  Only one of them has to do with
the real physical address, the other is about getting
the page frame number.

On native x86, x86-64 and others, the page frame number
and physical address are directly related to each other.
Under Xen, however, the two are different - and the
AGPGART really needs to have the physical address ;)

-- 
"Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it." - Brian W. Kernighan

Re: [PATCH] Xen/i386 cleanups - AGP bus/phys cleanups

[Paul Mackerras]

Rik van Riel writes:

> Thing is, the rest of the kernel uses virt_to_phys for
> two different things.  Only one of them has to do with
> the real physical address, the other is about getting
> the page frame number.

So fix the places that are using virt_to_phys to get the page frame
number to use virt_to_pfn instead. :)

> On native x86, x86-64 and others, the page frame number
> and physical address are directly related to each other.
> Under Xen, however, the two are different - and the
> AGPGART really needs to have the physical address ;)

If Xen is letting the kernel program the GART, you just lost any
memory isolation between partitions you might have been trying to
enforce. :)

Paul.

Re: [PATCH] Xen/i386 cleanups - AGP bus/phys cleanups

[Tupshin Harper]

Paul Mackerras wrote:

>Rik van Riel writes:
>
>  
>
>>Thing is, the rest of the kernel uses virt_to_phys for
>>two different things.  Only one of them has to do with
>>the real physical address, the other is about getting
>>the page frame number.
>>    
>>
>
>So fix the places that are using virt_to_phys to get the page frame
>number to use virt_to_pfn instead. :)
>
>  
>
>>On native x86, x86-64 and others, the page frame number
>>and physical address are directly related to each other.
>>Under Xen, however, the two are different - and the
>>AGPGART really needs to have the physical address ;)
>>    
>>
>
>If Xen is letting the kernel program the GART, you just lost any
>memory isolation between partitions you might have been trying to
>enforce. :)
>  
>
Since xen only lets the  dom0 (privileged domain) program the GART, all 
of the domU's  (non-privileged domains) are still isolated from each other.

-Tupshin

Re: [PATCH] Xen/i386 cleanups - AGP bus/phys cleanups

[Rik van Riel]

On Thu, 17 Mar 2005, Paul Mackerras wrote:

> > Under Xen, however, the two are different - and the
> > AGPGART really needs to have the physical address ;)
> 
> If Xen is letting the kernel program the GART, you just lost any
> memory isolation between partitions you might have been trying to
> enforce. :)

All the device drivers live in domain 0, so Xen doesn't
need its own set of device drivers.

-- 
"Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it." - Brian W. Kernighan

Re: [PATCH] Xen/i386 cleanups - AGP bus/phys cleanups

[Christoph Hellwig]

On Wed, Mar 16, 2005 at 06:55:13PM -0500, Rik van Riel wrote:
> Thing is, the rest of the kernel uses virt_to_phys for
> two different things.  Only one of them has to do with
> the real physical address, the other is about getting
> the page frame number.

The latter usage has been converted to page_to_pfn(virt_to_page(v))
in the places I checked.

Re: [PATCH] Xen/i386 cleanups - AGP bus/phys cleanups

[Rik van Riel]

On Thu, 17 Mar 2005, Christoph Hellwig wrote:
> On Wed, Mar 16, 2005 at 06:55:13PM -0500, Rik van Riel wrote:
> > Thing is, the rest of the kernel uses virt_to_phys for
> > two different things.  Only one of them has to do with
> > the real physical address, the other is about getting
> > the page frame number.
> 
> The latter usage has been converted to page_to_pfn(virt_to_page(v))
> in the places I checked.

Nice.  Maybe then virt_to_phys can do the right thing (for AGP)
in Xen without other stuff breaking ?

-- 
"Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it." - Brian W. Kernighan

Re: [PATCH] Xen/i386 cleanups - AGP bus/phys cleanups

[Keir Fraser]

On 16 Mar 2005, at 22:06, Jesse Barnes wrote:

>> The distinction is that physical addresses are what are used to access
>> physical memory, whereas bus addresses are what appears on some
>> external bus (usually PCI).  The GATT sits between an external (AGP)
>> bus and memory, so while the GATT is indexed using bus addresses, its
>> entries contain physical addresses.  So in fact virt_to_phys is the
>> correct thing to use to calculate values to put in GATT entries.
>
> Thanks for the explanation Paul, now the code actually makes sense.
> Converting to the DMA mapping API doesn't make sense at all in this 
> context
> then, since we're basically programming the GATT (an IOMMU type table) 
> with
> physical addresses.  Ken, are you sure you need to make these changes 
> at all?
> Does Xen break w/o them?

Yes, Xen will break w/o them, because physical addresses are an 
illusory trick that the guest OS plays on itself to give itself the 
impression of a contiguous memory map. We use _to_machine and _to_bus 
macros to get 'real' physical addresses.

For allocating the GATT itself, using dma_alloc_coherent() as done in 
my patch certainly seems sane -- the bus base address of that table is 
poked into a chipset register, right?

As for poking entries into the GATT, I guess I'm not sure what ought to 
be used. virt_to_phys() doesn't sound right to me: the GART is a bridge 
between two buses, so some sort of bus mapping would still be in order 
imo. Perhaps Linux should allow mapping requests to be tagged with a 
bridge id, like in *BSD? :-)

So: I would very much like you to take the patches I made to generic.c 
that replace __get_free_pages() calls with dma_alloc_coherent(). For 
now, the patch lines that poke into the GATT I guess stay as they are. 
We can maintain an out-of-tree patch for Xen, or perhaps if 
virt_to_phys() is not used very much we can override its definition.

  -- Keir

Re: [PATCH] Xen/i386 cleanups - AGP bus/phys cleanups

[Paul Mackerras]

Keir Fraser writes:

> Yes, Xen will break w/o them, because physical addresses are an 
> illusory trick that the guest OS plays on itself to give itself the 
> impression of a contiguous memory map. We use _to_machine and _to_bus 
> macros to get 'real' physical addresses.

This code needs real physical addresses, which are not the same things
as bus addresses.  I thought from what Rik said that virt_to_phys
should give you real physical addresses, while virt_to_pfn would give
you the contiguous "physical" page numbers - is that the case?

> For allocating the GATT itself, using dma_alloc_coherent() as done in 
> my patch certainly seems sane -- the bus base address of that table is 
> poked into a chipset register, right?

Well, a northbridge register.  The northbridge is in a privileged
position in that it controls the memory and can access all of it
directly using physical addresses, something which PCI or other
devices can't.  The address you poke into a register to define the
base of the table is a physical address.  How could it be a bus
address, when the whole point of the table is to translate bus
addresses to physical addresses?  If it was a bus address you would
have to know where the table was before you could work out where the
table was.

> As for poking entries into the GATT, I guess I'm not sure what ought to 
> be used. virt_to_phys() doesn't sound right to me: the GART is a bridge 
> between two buses, so some sort of bus mapping would still be in order 

No, the GART is a bridge between a bus and memory, and the GATT
entries are physical addresses.

> So: I would very much like you to take the patches I made to generic.c 
> that replace __get_free_pages() calls with dma_alloc_coherent(). For 

This is also wrong - the base address of the GATT is a physical
address not a bus address.  This change will break agpgart on ppc64
systems and I won't be able to play bzflag on my G5 any more. :)
dma_alloc_coherent allocates iommu entries and returns a bus address,
but the addresses coming out of the GART don't go through a further
translation through the iommu.

> now, the patch lines that poke into the GATT I guess stay as they are. 
> We can maintain an out-of-tree patch for Xen, or perhaps if 
> virt_to_phys() is not used very much we can override its definition.

It sounds like xen is trying to overload the concepts of physical and
bus addresses to represent the mapping from "logical" addresses seen
by the kernel to "absolute" addresses (the "real" physical
addresses).  IMHO that is a mistake and will only lead to trouble.

Regards,
Paul.

Re: [PATCH] Xen/i386 cleanups - AGP bus/phys cleanups

[Keir Fraser]

> > So: I would very much like you to take the patches I made to generic.c 
> > that replace __get_free_pages() calls with dma_alloc_coherent(). For 
> 
> This is also wrong - the base address of the GATT is a physical
> address not a bus address.  This change will break agpgart on ppc64
> systems and I won't be able to play bzflag on my G5 any more. :)
> dma_alloc_coherent allocates iommu entries and returns a bus address,
> but the addresses coming out of the GART don't go through a further
> translation through the iommu.

Okay, thank you for clearing up my confusion over how the GART
actually works. :-) I see you are right: my current patch is indeed
bogus, sorry about that. Unfortunately this means that some other
solution needs to be found for Xen....

> > now, the patch lines that poke into the GATT I guess stay as they are. 
> > We can maintain an out-of-tree patch for Xen, or perhaps if 
> > virt_to_phys() is not used very much we can override its definition.
> 
> It sounds like xen is trying to overload the concepts of physical and
> bus addresses to represent the mapping from "logical" addresses seen
> by the kernel to "absolute" addresses (the "real" physical
> addresses).  IMHO that is a mistake and will only lead to trouble.

Well, actually it has worked well for us so far. Our model of memory
allocation amongst Xen guests is fine-grained (page granularity). The
fact a guest can get random sparse physical pages does not fit well
with Linux's expectation (even with discontig-mem builds) of at least
getting fairly large contiguous physical chunks of RAM.

For this reason, we do rather abuse the notion of 'phys'
addresses. But we get away with it because it really doesn't matter to
the VM system that these addresses bear no relation to reality. In
most cases that it does matter it is because we are programming an I/O
device, in which case we have convenient hook points (bus-address
macros and the DMA-mapping interface). Another slightly tricky one was
block-I/O buffer merging but, again, we found a fairly clean way of
hooking that in an appropriate manner.

Even generic IOMMUs we will be able to handle -- we have control of
the DMA-mapping interface in arch/xen -- so it seems to be the case
that AGPGART is the one significant spanner in the works for us,
because it is an IOMMU in the form of a generic (arch-agnostic)
driver.

So: how about the following solution. It's a bit more distasteful
than I'd like, but I see no alternative apart from us maintaining an
out-of-tree patch to be applied only when building Linux for Xen.
What we need are four hook functions to be defined and used in place
of virt_to_phys, phys_to_virt, __get_free_pages, and
free_pages. Possible names might be:
  To go into asm/io.h:
     virt_to_fsb, fsb_to_virt, alloc_fsb_pages, free_fsb_pages
  Or, to go into asm/agp.h:
     agp_virt_to_phys, agp_phys_to_virt, agp_alloc/free_pages
The former names are an attempt to make the hooks a bit more 'generic'
by defining something applicable wherever you want an address that is
usable directly on the front-side bus. The latter are a bit hackier,
but then, maybe that is appropriate. :-)

I'd be happy to cook up a patch to do this if it isn't too offensive
for anyone.

 -- Keir

Re: [PATCH] Xen/i386 cleanups - AGP bus/phys cleanups

[Eric W. Biederman]

Keir Fraser <Keir.Fraser@cl.cam.ac.uk> writes:

> > > now, the patch lines that poke into the GATT I guess stay as they are. 
> > > We can maintain an out-of-tree patch for Xen, or perhaps if 
> > > virt_to_phys() is not used very much we can override its definition.
> > 
> > It sounds like xen is trying to overload the concepts of physical and
> > bus addresses to represent the mapping from "logical" addresses seen
> > by the kernel to "absolute" addresses (the "real" physical
> > addresses).  IMHO that is a mistake and will only lead to trouble.
> 
> Well, actually it has worked well for us so far. Our model of memory
> allocation amongst Xen guests is fine-grained (page granularity). The
> fact a guest can get random sparse physical pages does not fit well
> with Linux's expectation (even with discontig-mem builds) of at least
> getting fairly large contiguous physical chunks of RAM.

There is data excess data structure but it fits fine.  You simply
allocate one region and set PG_reserved on all of the pages that
the OS does not have access too.  Trivial and you don't have
to hack anything.

Larger than 4K granularity pages are important for performance reasons
in a number of contexts.  The primary reason is the large pages allow
a reduction in tlb misses.  But it is worth noting that DRAM pages
can be as large as 32KiB.  So there are other cases where there are
benefits in dealing with contiguous memory addresses besides reducing
the tlb miss count.

> For this reason, we do rather abuse the notion of 'phys'
> addresses. But we get away with it because it really doesn't matter to
> the VM system that these addresses bear no relation to reality. In
> most cases that it does matter it is because we are programming an I/O
> device, in which case we have convenient hook points (bus-address
> macros and the DMA-mapping interface). Another slightly tricky one was
> block-I/O buffer merging but, again, we found a fairly clean way of
> hooking that in an appropriate manner.

You also have broken kexec.  

And how well does hugetlbfs work under Xen?
 
> I'd be happy to cook up a patch to do this if it isn't too offensive
> for anyone.

For this specific case there may be another resolution but could
you please, please look at marking the missing pages PG_reserved
and not hacking phys_to_virt.

At this point anything short of explicitly introducing an intermediate
step say virt_to_logical() logical_to_virt() will be extremely
confusing and lead to very hard to spot bugs.  Silently changing
the semantics of functions is bad.

Eric

Re: [PATCH] Xen/i386 cleanups - AGP bus/phys cleanups

[Christian Limpach]

On Sat, Mar 19, 2005 at 03:07:18AM -0700, Eric W. Biederman wrote:
> For this specific case there may be another resolution but could
> you please, please look at marking the missing pages PG_reserved
> and not hacking phys_to_virt.
> 
> At this point anything short of explicitly introducing an intermediate
> step say virt_to_logical() logical_to_virt() will be extremely
> confusing and lead to very hard to spot bugs.  Silently changing
> the semantics of functions is bad.

We also use the additional level of indirection to implement suspend/
resume and relocation of virtual machines between physical machines  --
you won't get the same sparse allocation of memory on the target machine.
Also, this will make it much easier to support hot plug memory at the
hypervisor level since it will be able to substitute memory with very
little support from the OS running in the virtual machine.
I agree that adding an additional step would make this much cleaner.

    christian

Re: [PATCH] Xen/i386 cleanups - AGP bus/phys cleanups

[Keir Fraser]

On 19 Mar 2005, at 10:56, Christian Limpach wrote:

>> For this specific case there may be another resolution but could
>> you please, please look at marking the missing pages PG_reserved
>> and not hacking phys_to_virt.
>>
>> At this point anything short of explicitly introducing an intermediate
>> step say virt_to_logical() logical_to_virt() will be extremely
>> confusing and lead to very hard to spot bugs.  Silently changing
>> the semantics of functions is bad.
>
> We also use the additional level of indirection to implement suspend/
> resume and relocation of virtual machines between physical machines  --
> you won't get the same sparse allocation of memory on the target 
> machine.
> Also, this will make it much easier to support hot plug memory at the
> hypervisor level since it will be able to substitute memory with very
> little support from the OS running in the virtual machine.

Also, more generally, I don't believe Linux would deal well with a 
highly fragmented memory map. I wonder how far Linux would boot if you 
PG_reserved every other page? We'd also need to deal with 
virtual<->lowmem not being a 1:1 mapping (at least for kernel code and 
data, as at least that obviously needs to be contiguous in virtual 
space).

  -- Keir

Re: [PATCH] Xen/i386 cleanups - AGP bus/phys cleanups

[Alan Cox]

On Iau, 2005-03-17 at 09:34, Paul Mackerras wrote:
> This code needs real physical addresses, which are not the same things
> as bus addresses.  

Not always. The code needs platform specific goodies. We've only never
been burned so far because there isn't a box with an IOMMU and AGPGART
where one maps through the other.

It's probably simplest to have phys_to_agp/agp_to_phys therefore ?

Alan

Re: [PATCH] Xen/i386 cleanups - AGP bus/phys cleanups

[Paul Mackerras]

Alan Cox writes:

> On Iau, 2005-03-17 at 09:34, Paul Mackerras wrote:
> > This code needs real physical addresses, which are not the same things
> > as bus addresses.  
> 
> Not always. The code needs platform specific goodies. We've only never
> been burned so far because there isn't a box with an IOMMU and AGPGART
> where one maps through the other.

That sounds like a good way to make AGP accesses slower. :)

Seriously, given that AGP is a technology that is being superseded by
PCI Express, I think it's reasonable to look at the range of current
implementations to see what we have to cope with.  So I don't think
it's worth worrying too much about the possibility of GARTs that go
through the IOMMU.  However, the idea of having phys_to_agp/agp_to_phys
(or virt_to_agp/agp_to_virt) sounds like it wouldn't be too much
effort, if it would help Xen.

Paul.

Re: [PATCH] Xen/i386 cleanups - AGP bus/phys cleanups

[Rik van Riel]

On Fri, 18 Mar 2005, Paul Mackerras wrote:

> However, the idea of having phys_to_agp/agp_to_phys (or 
> virt_to_agp/agp_to_virt) sounds like it wouldn't be too much effort, if 
> it would help Xen.

It would be absolutely trivial.  On most architectures you would have:

#define virt_to_agp  virt_to_phys
#define agp_to_virt  phys_to_virt

On Xen you would have:

#define virt_to_agp  virt_to_bus
#define agp_to_virt  bus_to_virt

Or, more likely, defined to arbitrary_machine_to_phys
or whatever it was called ;)

-- 
"Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it." - Brian W. Kernighan

Re: [PATCH] Xen/i386 cleanups - AGP bus/phys cleanups

[Keir Fraser]

On 18 Mar 2005, at 00:16, Paul Mackerras wrote:

> That sounds like a good way to make AGP accesses slower. :)
>
> Seriously, given that AGP is a technology that is being superseded by
> PCI Express, I think it's reasonable to look at the range of current
> implementations to see what we have to cope with.  So I don't think
> it's worth worrying too much about the possibility of GARTs that go
> through the IOMMU.  However, the idea of having phys_to_agp/agp_to_phys
> (or virt_to_agp/agp_to_virt) sounds like it wouldn't be too much
> effort, if it would help Xen.

I'll post a patch for this next week. Thanks for your patience so far!

  -- Keir