Skip to contentSkip to conversation

amd iommu: Do not adjust paging mode for dom0 devices

PATCH In review Wei Wang2 2 participants 9 messages Feb 1, 2011 – Feb 8, 2011

lore
Share & tools
lore

Wei Wang2

author
#0 lore raw

PATCH 1/1 amd iommu: Do not adjust paging mode for dom0 devices

Keir
Here is another small patch to fix consistency issue of paging mode when return passthru devices back to dom0. In this case, paging mode should not be adjusted, since dom0->max_pages = ~0U.
Thanks,
Wei

--
Advanced Micro Devices GmbH
Sitz: Dornach, Gemeinde Aschheim,
Landkreis München Registergericht München,
HRB Nr. 43632
WEEE-Reg-Nr: DE 12919551
Geschäftsführer:
Alberto Bozzo, Andrew Bowd

xen/drivers/passthrough/amd/pci_amd_iommu.c +1 -1
diff -r a9ba2e80765f xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -298,7 +298,7 @@ static int reassign_device( struct domai
     list_move(&pdev->domain_list, &target->arch.pdev_list);
     pdev->domain = target;
 
-    if ( target->max_pages > 0 )
+    if ( target != dom0 && target->max_pages > 0 )
         t->paging_mode = get_paging_mode(target->max_pages);
 
     /* IO page tables might be destroyed after pci-detach the last device

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#1 lore raw
Quoted 4 lines · On 01/02/2011 17:34, "Wei Wang2" <[email protected]> wrote:

Keir
Here is another small patch to fix consistency issue of paging mode when return passthru devices back to dom0. In this case, paging mode should not be adjusted, since dom0->max_pages = ~0U.

The call to get_paging_mode() in amd_iommu_domain_init() looks sensible enough. Why is a call needed in reassign_device() at all? Checking for dom0 in reassign_device just looks like a fragile hack.

-- Keir

Quoted 15 lines

Thanks,
Wei

--
Advanced Micro Devices GmbH
Sitz: Dornach, Gemeinde Aschheim,
Landkreis München Registergericht München,
HRB Nr. 43632
WEEE-Reg-Nr: DE 12919551
Geschäftsführer:
Alberto Bozzo, Andrew Bowd
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

author
#2 lore raw
Quoted 11 lines · On Sunday 06 February 2011 17:58:18 Keir Fraser wrote:
Quoted 4 lines · On 01/02/2011 17:34, "Wei Wang2" <[email protected]> wrote:

Keir
Here is another small patch to fix consistency issue of paging mode when return passthru devices back to dom0. In this case, paging mode should not be adjusted, since dom0->max_pages = ~0U.

The call to get_paging_mode() in amd_iommu_domain_init() looks sensible enough. Why is a call needed in reassign_device() at all? Checking for dom0 in reassign_device just looks like a fragile hack.

-- Keir

Keir,
amd_iommu_domain_init() is called very early, where get_paging_mode() cannot use domU->max_pages to setup proper io page level for domU. Instead, paging mode of domU has to be initialized as 4 for safety. That is why I want to adjust hd->paging_mode in reassign_device() using domU->max_pages. Since most domU use 2-3 level page tables, always using 4 level might be a waste.

But I had a wrong assumption of dom0->max_pages, and consequently when return passthru device back to dom0, following lines in c/s 22825

+ if ( target->max_pages > 0 )
+ t->paging_mode = get_paging_mode(target->max_pages);

will change hd->paging_mode of dom0 from 3 (in most case) to 4 (using get_paging_mode(dom0->max_pages)).

Thanks,
-Wei

#3 lore raw
Quoted 15 lines · On 07/02/2011 09:58, "Wei Wang2" <[email protected]> wrote:
Quoted 1 line · On Sunday 06 February 2011 17:58:18 Keir Fraser wrote:

On 01/02/2011 17:34, "Wei Wang2" <[email protected]> wrote:

amd_iommu_domain_init() is called very early, where get_paging_mode() cannot use domU->max_pages to setup proper io page level for domU. Instead, paging mode of domU has to be initialized as 4 for safety. That is why I want to adjust hd->paging_mode in reassign_device() using domU->max_pages. Since most domU use 2-3 level page tables, always using 4 level might be a waste.

But I had a wrong assumption of dom0->max_pages, and consequently when return passthru device back to dom0, following lines in c/s 22825

(quoted diff) +2 -0
+    if ( target->max_pages > 0 )
+        t->paging_mode = get_paging_mode(target->max_pages);
Quoted 2 lines

will change hd->paging_mode of dom0 from 3 (in most case) to 4 (using get_paging_mode(dom0->max_pages)).

And that's wrong is it? How do you know that dom0 doesn't have a whole load of memory assigned to it?

The correct thing to do would be to adjust the table depth according to the largest page number currently mapped in the table. Or just stick with four levels always if you can't do the optimisation job properly.

-- Keir

Quoted 8 lines

Thanks,
-Wei

author
#4 lore raw
Quoted 20 lines · On Monday 07 February 2011 11:10:12 Keir Fraser wrote:
Quoted 16 lines · On 07/02/2011 09:58, "Wei Wang2" <[email protected]> wrote:
Quoted 1 line · On Sunday 06 February 2011 17:58:18 Keir Fraser wrote:

On 01/02/2011 17:34, "Wei Wang2" <[email protected]> wrote:

amd_iommu_domain_init() is called very early, where get_paging_mode() cannot use domU->max_pages to setup proper io page level for domU. Instead, paging mode of domU has to be initialized as 4 for safety. That is why I want to adjust hd->paging_mode in reassign_device() using domU->max_pages. Since most domU use 2-3 level page tables, always using 4 level might be a waste.

But I had a wrong assumption of dom0->max_pages, and consequently when return passthru device back to dom0, following lines in c/s 22825

(quoted diff) +2 -0
+    if ( target->max_pages > 0 )
+        t->paging_mode = get_paging_mode(target->max_pages);
Quoted 2 lines

will change hd->paging_mode of dom0 from 3 (in most case) to 4 (using get_paging_mode(dom0->max_pages)).

Quoted 8 lines

And that's wrong is it? How do you know that dom0 doesn't have a whole load of memory assigned to it?

The correct thing to do would be to adjust the table depth according to the largest page number currently mapped in the table. Or just stick with four levels always if you can't do the optimisation job properly.

-- Keir

Keir,
I was a little confused, are you suggesting that max_page does not represent the last pfn of dom0? I was assuming max_pdx is the index number... Or are you referring memory hot plug? If so, we might also need 4 level for dom0. Thanks,
Wei

#5 lore raw
Quoted 11 lines · On 07/02/2011 10:33, "Wei Wang2" <[email protected]> wrote:
Quoted 8 lines

And that's wrong is it? How do you know that dom0 doesn't have a whole load of memory assigned to it?

The correct thing to do would be to adjust the table depth according to the largest page number currently mapped in the table. Or just stick with four levels always if you can't do the optimisation job properly.

-- Keir

Keir,
I was a little confused, are you suggesting that max_page does not represent the last pfn of dom0?

The global variable max_page represents the largest machine frame number in the system.

The domain field d->max_pages merely represents an allocation limit for a domain, beyond which further allocation requests will be refused. Note it doesn't guarantee that the domain does not have less memory, or *more* memory (if max_pages got reduced below a domain's current allocation).

Also, for a PV guest like dom0, where the IOMMU table is presumably a 1:1 mapping, d->max_pages is not useful in any case because even if a guest has, say, only 100MB memory allocated to it, that memory can be spread across the entire host memory space from 0 to max_page. And max_page could be big!

Personally I would suggest starting with small 2-level tables and dynamically increase their height as bigger mappings are added to them. Else stick with 4-level tables, or size tables according to global variable max_page. I think basing anything on d->max_pages is not a good idea.

-- Keir

Quoted 2 lines

I was assuming max_pdx is the index number... Or are
you referring memory hot plug? If so, we might also need 4 level for dom0.

author
#6 lore raw
Quoted 8 lines · On Monday 07 February 2011 11:47:32 Keir Fraser wrote:
Quoted 7 lines · On 07/02/2011 10:33, "Wei Wang2" <[email protected]> wrote:
Quoted 7 lines

And that's wrong is it? How do you know that dom0 doesn't have a whole load of memory assigned to it?

The correct thing to do would be to adjust the table depth according to the largest page number currently mapped in the table. Or just stick with four levels always if you can't do the optimisation job properly.

Quoted 8 lines
Quoted 5 lines
Quoted 1 line

-- Keir

Keir,
I was a little confused, are you suggesting that max_page does not represent the last pfn of dom0?

The global variable max_page represents the largest machine frame number in the system.

Yes, that is also my assumption

Quoted 10 lines

The domain field d->max_pages merely represents an allocation limit for a domain, beyond which further allocation requests will be refused. Note it doesn't guarantee that the domain does not have less memory, or *more* memory (if max_pages got reduced below a domain's current allocation).

Also, for a PV guest like dom0, where the IOMMU table is presumably a 1:1 mapping, d->max_pages is not useful in any case because even if a guest has, say, only 100MB memory allocated to it, that memory can be spread across the entire host memory space from 0 to max_page. And max_page could be big!

OK, I misunderstood it. I thought d->max_pages also stands for last gfn for domU like max_page for the whole system.

Quoted 6 lines

Personally I would suggest starting with small 2-level tables and dynamically increase their height as bigger mappings are added to them. Else stick with 4-level tables, or size tables according to global variable max_page. I think basing anything on d->max_pages is not a good idea.

-- Keir

How does the attached patch look like? It uses global variable max_page for pv and dom0 and calculate maxpfn for hvm guest. This should cover gfn holes on hvm guests.

Thanks,
Wei

Quoted 3 lines
Quoted 3 lines

I was assuming max_pdx is the index number... Or are
you referring memory hot plug? If so, we might also need 4 level for dom0.

xen/drivers/passthrough/amd/pci_amd_iommu.c +29 -2
diff -r 77d05af7dc78 xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -19,6 +19,7 @@
  */
 
 #include <xen/sched.h>
+#include <xen/paging.h>
 #include <xen/pci.h>
 #include <xen/pci_regs.h>
 #include <asm/hvm/iommu.h>
@@ -186,6 +187,31 @@ static int allocate_domain_resources(str
     return 0;
 }
 
+static unsigned long get_maxpfn(struct domain *d)
+{
+    struct page_info *page;
+    unsigned long gfn, max_pfn = 0;
+
+    /* For pv & dom0, return maximum machine frame number */
+    if ( !is_hvm_domain(d) )
+        return max_page;
+
+    spin_lock(&d->page_alloc_lock);
+
+    /* Find out maximum gfn */
+    page_list_for_each ( page, &d->page_list )
+    {
+        gfn = mfn_to_gmfn(d, page_to_mfn(page));
+        if ( gfn > max_pfn )
+            max_pfn = gfn;
+    }
+
+    spin_unlock(&d->page_alloc_lock);
+
+    ASSERT(max_pfn > 0);
+    return max_pfn;
+}
+
 static int get_paging_mode(unsigned long entries)
 {
     int level = 1;
@@ -298,8 +324,9 @@ static int reassign_device( struct domai
     list_move(&pdev->domain_list, &target->arch.pdev_list);
     pdev->domain = target;
 
-    if ( target->max_pages > 0 )
-        t->paging_mode = get_paging_mode(target->max_pages);
+    /* Adjust paging mode for hvm guest.
+     * For pv and dom0, stick with get_paging_mode(max_page) */
+    t->paging_mode = get_paging_mode(get_maxpfn(target));
 
     /* IO page tables might be destroyed after pci-detach the last device
      * In this case, we have to re-allocate root table for next pci-attach.*/

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#7 lore raw
Quoted 8 lines · On 07/02/2011 13:30, "Wei Wang2" <[email protected]> wrote:
Quoted 7 lines · On Monday 07 February 2011 11:47:32 Keir Fraser wrote:

On 07/02/2011 10:33, "Wei Wang2" <[email protected]> wrote:

Personally I would suggest starting with small 2-level tables and dynamically increase their height as bigger mappings are added to them. Else stick with 4-level tables, or size tables according to global variable max_page. I think basing anything on d->max_pages is not a good idea.

Quoted 4 lines
Quoted 1 line

-- Keir

How does the attached patch look like? It uses global variable max_page for pv and dom0 and calculate maxpfn for hvm guest. This should cover gfn holes on hvm guests.

The p2m code already tracks the largest gfn for HVM guests. Try using p2m_get_hostp2m(d)->max_mapped_pfn for HVM guests. Note that this could increase after you sample it, however. Hence why you really need to have a statically deep-enough table, or the ability to grow the table depth dynamically.

Your change for PV guests would definitely be correct, however.

-- Keir

Quoted 9 lines

Thanks,
Wei

Quoted 3 lines
Quoted 3 lines

I was assuming max_pdx is the index number... Or are
you referring memory hot plug? If so, we might also need 4 level for dom0.

author
#8 lore raw
Quoted 16 lines · On Monday 07 February 2011 16:00:04 Keir Fraser wrote:
Quoted 9 lines · On 07/02/2011 13:30, "Wei Wang2" <[email protected]> wrote:
Quoted 8 lines · On Monday 07 February 2011 11:47:32 Keir Fraser wrote:

On 07/02/2011 10:33, "Wei Wang2" <[email protected]> wrote:

Personally I would suggest starting with small 2-level tables and dynamically increase their height as bigger mappings are added to them. Else stick with 4-level tables, or size tables according to global variable max_page. I think basing anything on d->max_pages is not a good idea.

Quoted 5 lines
Quoted 1 line

-- Keir

How does the attached patch look like? It uses global variable max_page for pv and dom0 and calculate maxpfn for hvm guest. This should cover gfn holes on hvm guests.

Quoted 5 lines

The p2m code already tracks the largest gfn for HVM guests. Try using p2m_get_hostp2m(d)->max_mapped_pfn for HVM guests. Note that this could increase after you sample it, however. Hence why you really need to have a statically deep-enough table, or the ability to grow the table depth dynamically.

Keir,
Attached patch implements dynamical page table depth adjustment. Please review. IO Page table growth is triggered by amd_iommu_map_page and grows to upper level. I have tested it well for different devices (nic and gfx) and different guests (linux and Win7) with different guest memory sizes (512M, 1G, 4G and above).
Although this looks easier than my first thought, it may not be trivial for the release. If so, I will send you another patch to reverse c/s 22825 tomorrow.
Thanks,
Wei

Quoted 11 lines

Your change for PV guests would definitely be correct, however.

-- Keir

Quoted 7 lines

Thanks,
Wei

Quoted 3 lines
Quoted 3 lines

I was assuming max_pdx is the index number... Or are
you referring memory hot plug? If so, we might also need 4 level for dom0.

xen/drivers/passthrough/amd/iommu_map.c +107 -0
diff -r 77d05af7dc78 xen/drivers/passthrough/amd/iommu_map.c
@@ -472,6 +472,89 @@ static u64 iommu_l2e_from_pfn(struct pag
     return next_table_maddr;
 }
 
+static int update_paging_mode(struct domain *d, unsigned long gfn)
+{
+    u16 bdf;
+    void *device_entry;
+    unsigned int req_id, level, offset;
+    unsigned long flags;
+    struct pci_dev *pdev;
+    struct amd_iommu *iommu = NULL;
+    struct page_info *new_root = NULL;
+    struct page_info *old_root = NULL;
+    void *new_root_vaddr;
+    u64 old_root_maddr;
+    struct hvm_iommu *hd = domain_hvm_iommu(d);
+
+    level = hd->paging_mode;
+    old_root = hd->root_table;
+    offset = gfn >> (PTE_PER_TABLE_SHIFT * (level - 1));
+
+    ASSERT(spin_is_locked(&hd->mapping_lock) && is_hvm_domain(d));
+
+    while ( offset >= PTE_PER_TABLE_SIZE )
+    {
+        /* Allocate and install a new root table.
+         * Only upper I/O page table grows, no need to fix next level bits */
+        new_root = alloc_amd_iommu_pgtable();
+        if ( new_root == NULL )
+        {
+            AMD_IOMMU_DEBUG("%s Cannot allocate I/O page table\n",
+                            __func__);
+            return -ENOMEM;
+        }
+
+        new_root_vaddr = __map_domain_page(new_root);
+        old_root_maddr = page_to_maddr(old_root);
+        amd_iommu_set_page_directory_entry((u32 *)new_root_vaddr,
+                                           old_root_maddr, level);
+        level++;
+        old_root = new_root;
+        offset >>= PTE_PER_TABLE_SHIFT;
+    }
+
+    if ( new_root != NULL )
+    {
+        hd->paging_mode = level;
+        hd->root_table = new_root;
+
+        if ( !spin_is_locked(&pcidevs_lock) )
+            AMD_IOMMU_DEBUG("%s Try to access pdev_list "
+                            "without aquiring pcidevs_lock.\n", __func__);
+
+        /* Update device table entries using new root table and paging mode */
+        for_each_pdev( d, pdev )
+        {
+            bdf = (pdev->bus << 8) | pdev->devfn;
+            req_id = get_dma_requestor_id(bdf);
+            iommu = find_iommu_for_device(bdf);
+            if ( !iommu )
+            {
+                AMD_IOMMU_DEBUG("%s Fail to find iommu.\n", __func__);
+                return -ENODEV;
+            }
+
+            spin_lock_irqsave(&iommu->lock, flags);
+            device_entry = iommu->dev_table.buffer +
+                           (req_id * IOMMU_DEV_TABLE_ENTRY_SIZE);
+
+            /* valid = 0 only works for dom0 passthrough mode */
+            amd_iommu_set_root_page_table((u32 *)device_entry,
+                                          page_to_maddr(hd->root_table),
+                                          hd->domain_id,
+                                          hd->paging_mode, 1);
+
+            invalidate_dev_table_entry(iommu, req_id);
+            flush_command_buffer(iommu);
+            spin_unlock_irqrestore(&iommu->lock, flags);
+        }
+
+        /* For safety, invalidate all entries */
+        invalidate_all_iommu_pages(d);
+    }
+    return 0;
+}
+
 int amd_iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn,
                        unsigned int flags)
 {
@@ -481,6 +564,18 @@ int amd_iommu_map_page(struct domain *d,
     BUG_ON( !hd->root_table );
 
     spin_lock(&hd->mapping_lock);
+
+    /* Since HVM domain is initialized with 2 level IO page table,
+     * we might need a deeper page table for lager gfn now */
+    if ( is_hvm_domain(d) )
+    {
+        if ( update_paging_mode(d, gfn) )
+        {
+            AMD_IOMMU_DEBUG("Update page mode failed gfn = %lx\n", gfn);
+            domain_crash(d);
+            return -EFAULT;
+        }
+    }
 
     iommu_l2e = iommu_l2e_from_pfn(hd->root_table, hd->paging_mode, gfn);
     if ( iommu_l2e == 0 )
@@ -509,6 +604,18 @@ int amd_iommu_unmap_page(struct domain *
     BUG_ON( !hd->root_table );
 
     spin_lock(&hd->mapping_lock);
+
+    /* Since HVM domain is initialized with 2 level IO page table,
+     * we might need a deeper page table for lager gfn now */
+    if ( is_hvm_domain(d) )
+    {
+        if ( update_paging_mode(d, gfn) )
+        {
+            AMD_IOMMU_DEBUG("Update page mode failed gfn = %lx\n", gfn);
+            domain_crash(d);
+            return -EFAULT;
+        }
+    }
 
     iommu_l2e = iommu_l2e_from_pfn(hd->root_table, hd->paging_mode, gfn);
 
xen/drivers/passthrough/amd/pci_amd_iommu.c +4 -4
diff -r 77d05af7dc78 xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -214,8 +214,11 @@ static int amd_iommu_domain_init(struct 
         return -ENOMEM;
     }
 
+    /* For pv and dom0, stick with get_paging_mode(max_page)
+     * For HVM dom0, use 2 level page table at first */
     hd->paging_mode = is_hvm_domain(d) ?
-        IOMMU_PAGE_TABLE_LEVEL_4 : get_paging_mode(max_page);
+                      IOMMU_PAGING_MODE_LEVEL_2 :
+                      get_paging_mode(max_page);
 
     hd->domain_id = d->domain_id;
 
@@ -297,9 +300,6 @@ static int reassign_device( struct domai
 
     list_move(&pdev->domain_list, &target->arch.pdev_list);
     pdev->domain = target;
-
-    if ( target->max_pages > 0 )
-        t->paging_mode = get_paging_mode(target->max_pages);
 
     /* IO page tables might be destroyed after pci-detach the last device
      * In this case, we have to re-allocate root table for next pci-attach.*/
xen/include/asm-x86/hvm/svm/amd-iommu-defs.h +0 -2
diff -r 77d05af7dc78 xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
@@ -386,8 +386,6 @@
 #define IOMMU_PAGES                 (MMIO_PAGES_PER_IOMMU * MAX_AMD_IOMMUS)
 #define DEFAULT_DOMAIN_ADDRESS_WIDTH    48
 #define MAX_AMD_IOMMUS                  32
-#define IOMMU_PAGE_TABLE_LEVEL_3        3
-#define IOMMU_PAGE_TABLE_LEVEL_4        4
 
 /* interrupt remapping table */
 #define INT_REMAP_INDEX_DM_MASK         0x1C00

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel