* [PATCH] IOMMU/x86: avoid pages without GFN in page table creation/updating
@ 2015-05-07 12:59 Jan Beulich
2015-05-07 13:13 ` Andrew Cooper
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Jan Beulich @ 2015-05-07 12:59 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Andrew Cooper, Tim Deegan, Sander Eikelenboom,
Aravind Gopalakrishnan, suravee.suthikulpanit, Yang Z Zhang
[-- Attachment #1: Type: text/plain, Size: 4353 bytes --]
Handing INVALID_GFN to functions like hd->platform_ops->map_page()
just can't do any good, and the ioreq server code results in such pages
being on the list of ones owned by a guest.
While - as suggested by Tim - we should use get_gfn()/put_gfn() there
to eliminate races, we really can't due to holding the domain's page
alloc lock. Ultimately arch_iommu_populate_page_table() may need to be
switched to be GFN based. Here is what Tim said in this regard:
"Ideally this loop would be iterating over all gfns in the p2m rather
than over all owned MFNs. As long as needs_iommu gets set first,
such a loop could safely be paused and restarted without worrying
about concurrent updates. The code sould even stay in this file,
though exposing an iterator from the p2m code would be a lot more
efficient."
Original by Andrew Cooper <andrew.cooper3@citrix.com>, using further
suggestions from Tim Deegan <tim@xen.org>.
Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Tested-by: Sander Eikelenboom <linux@eikelenboom.it>
Acked-by: Tim Deegan <tim@xen.org>
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -557,6 +557,10 @@ static int update_paging_mode(struct dom
unsigned long old_root_mfn;
struct hvm_iommu *hd = domain_hvm_iommu(d);
+ if ( gfn == INVALID_MFN )
+ return -EADDRNOTAVAIL;
+ ASSERT(!(gfn >> DEFAULT_DOMAIN_ADDRESS_WIDTH));
+
level = hd->arch.paging_mode;
old_root = hd->arch.root_table;
offset = gfn >> (PTE_PER_TABLE_SHIFT * (level - 1));
@@ -729,12 +733,15 @@ int amd_iommu_unmap_page(struct domain *
* we might need a deeper page table for lager gfn now */
if ( is_hvm_domain(d) )
{
- if ( update_paging_mode(d, gfn) )
+ int rc = update_paging_mode(d, gfn);
+
+ if ( rc )
{
spin_unlock(&hd->arch.mapping_lock);
AMD_IOMMU_DEBUG("Update page mode failed gfn = %lx\n", gfn);
- domain_crash(d);
- return -EFAULT;
+ if ( rc != -EADDRNOTAVAIL )
+ domain_crash(d);
+ return rc;
}
}
--- a/xen/drivers/passthrough/vtd/iommu.h
+++ b/xen/drivers/passthrough/vtd/iommu.h
@@ -482,7 +482,6 @@ struct qinval_entry {
#define VTD_PAGE_TABLE_LEVEL_3 3
#define VTD_PAGE_TABLE_LEVEL_4 4
-#define DEFAULT_DOMAIN_ADDRESS_WIDTH 48
#define MAX_IOMMU_REGS 0xc0
extern struct list_head acpi_drhd_units;
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -59,10 +59,17 @@ int arch_iommu_populate_page_table(struc
if ( has_hvm_container_domain(d) ||
(page->u.inuse.type_info & PGT_type_mask) == PGT_writable_page )
{
- BUG_ON(SHARED_M2P(mfn_to_gmfn(d, page_to_mfn(page))));
- rc = hd->platform_ops->map_page(
- d, mfn_to_gmfn(d, page_to_mfn(page)), page_to_mfn(page),
- IOMMUF_readable|IOMMUF_writable);
+ unsigned long mfn = page_to_mfn(page);
+ unsigned long gfn = mfn_to_gmfn(d, mfn);
+
+ if ( gfn != INVALID_MFN )
+ {
+ ASSERT(!(gfn >> DEFAULT_DOMAIN_ADDRESS_WIDTH));
+ BUG_ON(SHARED_M2P(gfn));
+ rc = hd->platform_ops->map_page(d, gfn, mfn,
+ IOMMUF_readable |
+ IOMMUF_writable);
+ }
if ( rc )
{
page_list_add(page, &d->page_list);
--- a/xen/include/asm-x86/hvm/iommu.h
+++ b/xen/include/asm-x86/hvm/iommu.h
@@ -46,6 +46,8 @@ struct g2m_ioport {
unsigned int np;
};
+#define DEFAULT_DOMAIN_ADDRESS_WIDTH 48
+
struct arch_hvm_iommu
{
u64 pgd_maddr; /* io page directory machine address */
--- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
+++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
@@ -464,8 +464,6 @@
#define IOMMU_CONTROL_DISABLED 0
#define IOMMU_CONTROL_ENABLED 1
-#define DEFAULT_DOMAIN_ADDRESS_WIDTH 48
-
/* interrupt remapping table */
#define INT_REMAP_ENTRY_REMAPEN_MASK 0x00000001
#define INT_REMAP_ENTRY_REMAPEN_SHIFT 0
[-- Attachment #2: IOMMU-populate-PT-filter-invalid.patch --]
[-- Type: text/plain, Size: 4419 bytes --]
IOMMU/x86: avoid pages without GFN in page table creation/updating
Handing INVALID_GFN to functions like hd->platform_ops->map_page()
just can't do any good, and the ioreq server code results in such pages
being on the list of ones owned by a guest.
While - as suggested by Tim - we should use get_gfn()/put_gfn() there
to eliminate races, we really can't due to holding the domain's page
alloc lock. Ultimately arch_iommu_populate_page_table() may need to be
switched to be GFN based. Here is what Tim said in this regard:
"Ideally this loop would be iterating over all gfns in the p2m rather
than over all owned MFNs. As long as needs_iommu gets set first,
such a loop could safely be paused and restarted without worrying
about concurrent updates. The code sould even stay in this file,
though exposing an iterator from the p2m code would be a lot more
efficient."
Original by Andrew Cooper <andrew.cooper3@citrix.com>, using further
suggestions from Tim Deegan <tim@xen.org>.
Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Tested-by: Sander Eikelenboom <linux@eikelenboom.it>
Acked-by: Tim Deegan <tim@xen.org>
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -557,6 +557,10 @@ static int update_paging_mode(struct dom
unsigned long old_root_mfn;
struct hvm_iommu *hd = domain_hvm_iommu(d);
+ if ( gfn == INVALID_MFN )
+ return -EADDRNOTAVAIL;
+ ASSERT(!(gfn >> DEFAULT_DOMAIN_ADDRESS_WIDTH));
+
level = hd->arch.paging_mode;
old_root = hd->arch.root_table;
offset = gfn >> (PTE_PER_TABLE_SHIFT * (level - 1));
@@ -729,12 +733,15 @@ int amd_iommu_unmap_page(struct domain *
* we might need a deeper page table for lager gfn now */
if ( is_hvm_domain(d) )
{
- if ( update_paging_mode(d, gfn) )
+ int rc = update_paging_mode(d, gfn);
+
+ if ( rc )
{
spin_unlock(&hd->arch.mapping_lock);
AMD_IOMMU_DEBUG("Update page mode failed gfn = %lx\n", gfn);
- domain_crash(d);
- return -EFAULT;
+ if ( rc != -EADDRNOTAVAIL )
+ domain_crash(d);
+ return rc;
}
}
--- a/xen/drivers/passthrough/vtd/iommu.h
+++ b/xen/drivers/passthrough/vtd/iommu.h
@@ -482,7 +482,6 @@ struct qinval_entry {
#define VTD_PAGE_TABLE_LEVEL_3 3
#define VTD_PAGE_TABLE_LEVEL_4 4
-#define DEFAULT_DOMAIN_ADDRESS_WIDTH 48
#define MAX_IOMMU_REGS 0xc0
extern struct list_head acpi_drhd_units;
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -59,10 +59,17 @@ int arch_iommu_populate_page_table(struc
if ( has_hvm_container_domain(d) ||
(page->u.inuse.type_info & PGT_type_mask) == PGT_writable_page )
{
- BUG_ON(SHARED_M2P(mfn_to_gmfn(d, page_to_mfn(page))));
- rc = hd->platform_ops->map_page(
- d, mfn_to_gmfn(d, page_to_mfn(page)), page_to_mfn(page),
- IOMMUF_readable|IOMMUF_writable);
+ unsigned long mfn = page_to_mfn(page);
+ unsigned long gfn = mfn_to_gmfn(d, mfn);
+
+ if ( gfn != INVALID_MFN )
+ {
+ ASSERT(!(gfn >> DEFAULT_DOMAIN_ADDRESS_WIDTH));
+ BUG_ON(SHARED_M2P(gfn));
+ rc = hd->platform_ops->map_page(d, gfn, mfn,
+ IOMMUF_readable |
+ IOMMUF_writable);
+ }
if ( rc )
{
page_list_add(page, &d->page_list);
--- a/xen/include/asm-x86/hvm/iommu.h
+++ b/xen/include/asm-x86/hvm/iommu.h
@@ -46,6 +46,8 @@ struct g2m_ioport {
unsigned int np;
};
+#define DEFAULT_DOMAIN_ADDRESS_WIDTH 48
+
struct arch_hvm_iommu
{
u64 pgd_maddr; /* io page directory machine address */
--- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
+++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
@@ -464,8 +464,6 @@
#define IOMMU_CONTROL_DISABLED 0
#define IOMMU_CONTROL_ENABLED 1
-#define DEFAULT_DOMAIN_ADDRESS_WIDTH 48
-
/* interrupt remapping table */
#define INT_REMAP_ENTRY_REMAPEN_MASK 0x00000001
#define INT_REMAP_ENTRY_REMAPEN_SHIFT 0
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] IOMMU/x86: avoid pages without GFN in page table creation/updating
2015-05-07 12:59 [PATCH] IOMMU/x86: avoid pages without GFN in page table creation/updating Jan Beulich
@ 2015-05-07 13:13 ` Andrew Cooper
2015-05-07 13:50 ` Jan Beulich
2015-05-11 2:53 ` Zhang, Yang Z
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Andrew Cooper @ 2015-05-07 13:13 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Kevin Tian, Tim Deegan, Sander Eikelenboom,
Aravind Gopalakrishnan, suravee.suthikulpanit, Yang Z Zhang
On 07/05/15 13:59, Jan Beulich wrote:
> Handing INVALID_GFN to functions like hd->platform_ops->map_page()
> just can't do any good, and the ioreq server code results in such pages
> being on the list of ones owned by a guest.
>
> While - as suggested by Tim - we should use get_gfn()/put_gfn() there
> to eliminate races, we really can't due to holding the domain's page
> alloc lock. Ultimately arch_iommu_populate_page_table() may need to be
> switched to be GFN based. Here is what Tim said in this regard:
> "Ideally this loop would be iterating over all gfns in the p2m rather
> than over all owned MFNs. As long as needs_iommu gets set first,
> such a loop could safely be paused and restarted without worrying
> about concurrent updates. The code sould even stay in this file,
> though exposing an iterator from the p2m code would be a lot more
> efficient."
>
> Original by Andrew Cooper <andrew.cooper3@citrix.com>, using further
> suggestions from Tim Deegan <tim@xen.org>.
>
> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Tested-by: Sander Eikelenboom <linux@eikelenboom.it>
> Acked-by: Tim Deegan <tim@xen.org>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, albeit with one
further suggestion.
>
> --- a/xen/drivers/passthrough/amd/iommu_map.c
> +++ b/xen/drivers/passthrough/amd/iommu_map.c
> @@ -557,6 +557,10 @@ static int update_paging_mode(struct dom
> unsigned long old_root_mfn;
> struct hvm_iommu *hd = domain_hvm_iommu(d);
>
> + if ( gfn == INVALID_MFN )
> + return -EADDRNOTAVAIL;
> + ASSERT(!(gfn >> DEFAULT_DOMAIN_ADDRESS_WIDTH));
> +
> level = hd->arch.paging_mode;
> old_root = hd->arch.root_table;
> offset = gfn >> (PTE_PER_TABLE_SHIFT * (level - 1));
> @@ -729,12 +733,15 @@ int amd_iommu_unmap_page(struct domain *
> * we might need a deeper page table for lager gfn now */
> if ( is_hvm_domain(d) )
> {
> - if ( update_paging_mode(d, gfn) )
> + int rc = update_paging_mode(d, gfn);
> +
> + if ( rc )
> {
> spin_unlock(&hd->arch.mapping_lock);
> AMD_IOMMU_DEBUG("Update page mode failed gfn = %lx\n", gfn);
Given the domain_crash(), this should probably at error priority rather
than debug.
~Andrew
> - domain_crash(d);
> - return -EFAULT;
> + if ( rc != -EADDRNOTAVAIL )
> + domain_crash(d);
> + return rc;
> }
> }
>
> --- a/xen/drivers/passthrough/vtd/iommu.h
> +++ b/xen/drivers/passthrough/vtd/iommu.h
> @@ -482,7 +482,6 @@ struct qinval_entry {
> #define VTD_PAGE_TABLE_LEVEL_3 3
> #define VTD_PAGE_TABLE_LEVEL_4 4
>
> -#define DEFAULT_DOMAIN_ADDRESS_WIDTH 48
> #define MAX_IOMMU_REGS 0xc0
>
> extern struct list_head acpi_drhd_units;
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -59,10 +59,17 @@ int arch_iommu_populate_page_table(struc
> if ( has_hvm_container_domain(d) ||
> (page->u.inuse.type_info & PGT_type_mask) == PGT_writable_page )
> {
> - BUG_ON(SHARED_M2P(mfn_to_gmfn(d, page_to_mfn(page))));
> - rc = hd->platform_ops->map_page(
> - d, mfn_to_gmfn(d, page_to_mfn(page)), page_to_mfn(page),
> - IOMMUF_readable|IOMMUF_writable);
> + unsigned long mfn = page_to_mfn(page);
> + unsigned long gfn = mfn_to_gmfn(d, mfn);
> +
> + if ( gfn != INVALID_MFN )
> + {
> + ASSERT(!(gfn >> DEFAULT_DOMAIN_ADDRESS_WIDTH));
> + BUG_ON(SHARED_M2P(gfn));
> + rc = hd->platform_ops->map_page(d, gfn, mfn,
> + IOMMUF_readable |
> + IOMMUF_writable);
> + }
> if ( rc )
> {
> page_list_add(page, &d->page_list);
> --- a/xen/include/asm-x86/hvm/iommu.h
> +++ b/xen/include/asm-x86/hvm/iommu.h
> @@ -46,6 +46,8 @@ struct g2m_ioport {
> unsigned int np;
> };
>
> +#define DEFAULT_DOMAIN_ADDRESS_WIDTH 48
> +
> struct arch_hvm_iommu
> {
> u64 pgd_maddr; /* io page directory machine address */
> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> @@ -464,8 +464,6 @@
> #define IOMMU_CONTROL_DISABLED 0
> #define IOMMU_CONTROL_ENABLED 1
>
> -#define DEFAULT_DOMAIN_ADDRESS_WIDTH 48
> -
> /* interrupt remapping table */
> #define INT_REMAP_ENTRY_REMAPEN_MASK 0x00000001
> #define INT_REMAP_ENTRY_REMAPEN_SHIFT 0
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] IOMMU/x86: avoid pages without GFN in page table creation/updating
2015-05-07 13:13 ` Andrew Cooper
@ 2015-05-07 13:50 ` Jan Beulich
0 siblings, 0 replies; 8+ messages in thread
From: Jan Beulich @ 2015-05-07 13:50 UTC (permalink / raw)
To: Andrew Cooper
Cc: Kevin Tian, Tim Deegan, Sander Eikelenboom,
Aravind Gopalakrishnan, suravee.suthikulpanit, Yang Z Zhang,
xen-devel
>>> On 07.05.15 at 15:13, <andrew.cooper3@citrix.com> wrote:
> On 07/05/15 13:59, Jan Beulich wrote:
>> @@ -729,12 +733,15 @@ int amd_iommu_unmap_page(struct domain *
>> * we might need a deeper page table for lager gfn now */
>> if ( is_hvm_domain(d) )
>> {
>> - if ( update_paging_mode(d, gfn) )
>> + int rc = update_paging_mode(d, gfn);
>> +
>> + if ( rc )
>> {
>> spin_unlock(&hd->arch.mapping_lock);
>> AMD_IOMMU_DEBUG("Update page mode failed gfn = %lx\n", gfn);
>
> Given the domain_crash(), this should probably at error priority rather
> than debug.
Not sure (and it's not debug but info level anyway, but behind an
iommu_debug check). Being unrelated to the change here, I think
this ought to be a separate change, perhaps by the AMD IOMMU
maintainers.
Jan
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] IOMMU/x86: avoid pages without GFN in page table creation/updating
2015-05-07 12:59 [PATCH] IOMMU/x86: avoid pages without GFN in page table creation/updating Jan Beulich
2015-05-07 13:13 ` Andrew Cooper
@ 2015-05-11 2:53 ` Zhang, Yang Z
2015-05-11 6:47 ` Jan Beulich
2015-05-13 13:11 ` Ping: " Jan Beulich
2015-05-13 13:42 ` Suravee Suthikulanit
3 siblings, 1 reply; 8+ messages in thread
From: Zhang, Yang Z @ 2015-05-11 2:53 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Tian, Kevin, Andrew Cooper, Tim Deegan, Sander Eikelenboom,
Aravind Gopalakrishnan, suravee.suthikulpanit@amd.com
Jan Beulich wrote on 2015-05-07:
> Handing INVALID_GFN to functions like hd->platform_ops->map_page() just
> can't do any good, and the ioreq server code results in such pages being on the
> list of ones owned by a guest.
>
> While - as suggested by Tim - we should use get_gfn()/put_gfn() there to
> eliminate races, we really can't due to holding the domain's page alloc lock.
> Ultimately arch_iommu_populate_page_table() may need to be switched to be
> GFN based. Here is what Tim said in this regard:
> "Ideally this loop would be iterating over all gfns in the p2m rather than over
> all owned MFNs. As long as needs_iommu gets set first, such a loop could
> safely be paused and restarted without worrying about concurrent updates.
> The code sould even stay in this file, though exposing an iterator from the
> p2m code would be a lot more efficient."
>
> Original by Andrew Cooper <andrew.cooper3@citrix.com>, using further
> suggestions from Tim Deegan <tim@xen.org>.
>
> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Tested-by: Sander Eikelenboom <linux@eikelenboom.it>
> Acked-by: Tim Deegan <tim@xen.org>
>
> --- a/xen/drivers/passthrough/amd/iommu_map.c
> +++ b/xen/drivers/passthrough/amd/iommu_map.c
> @@ -557,6 +557,10 @@ static int update_paging_mode(struct dom
> unsigned long old_root_mfn;
> struct hvm_iommu *hd = domain_hvm_iommu(d);
>
> + if ( gfn == INVALID_MFN )
> + return -EADDRNOTAVAIL;
> + ASSERT(!(gfn >> DEFAULT_DOMAIN_ADDRESS_WIDTH));
> +
> level = hd->arch.paging_mode;
> old_root = hd->arch.root_table;
> offset = gfn >> (PTE_PER_TABLE_SHIFT * (level - 1)); @@ -729,12
> +733,15 @@ int amd_iommu_unmap_page(struct domain *
> * we might need a deeper page table for lager gfn now */
> if ( is_hvm_domain(d) )
> {
> - if ( update_paging_mode(d, gfn) )
> + int rc = update_paging_mode(d, gfn);
> +
> + if ( rc )
> {
> spin_unlock(&hd->arch.mapping_lock);
> AMD_IOMMU_DEBUG("Update page mode failed gfn = %lx\n",
> gfn);
> - domain_crash(d);
> - return -EFAULT;
> + if ( rc != -EADDRNOTAVAIL )
> + domain_crash(d);
> + return rc;
> }
> }
>
> --- a/xen/drivers/passthrough/vtd/iommu.h
> +++ b/xen/drivers/passthrough/vtd/iommu.h
> @@ -482,7 +482,6 @@ struct qinval_entry { #define
> VTD_PAGE_TABLE_LEVEL_3 3 #define VTD_PAGE_TABLE_LEVEL_4 4
>
> -#define DEFAULT_DOMAIN_ADDRESS_WIDTH 48 #define
> MAX_IOMMU_REGS 0xc0
>
> extern struct list_head acpi_drhd_units;
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -59,10 +59,17 @@ int arch_iommu_populate_page_table(struc
> if ( has_hvm_container_domain(d) ||
> (page->u.inuse.type_info & PGT_type_mask) ==
> PGT_writable_page )
> {
> - BUG_ON(SHARED_M2P(mfn_to_gmfn(d,
> page_to_mfn(page))));
> - rc = hd->platform_ops->map_page(
> - d, mfn_to_gmfn(d, page_to_mfn(page)),
> page_to_mfn(page),
> - IOMMUF_readable|IOMMUF_writable);
> + unsigned long mfn = page_to_mfn(page);
> + unsigned long gfn = mfn_to_gmfn(d, mfn);
> +
> + if ( gfn != INVALID_MFN )
> + {
> + ASSERT(!(gfn >> DEFAULT_DOMAIN_ADDRESS_WIDTH));
> + BUG_ON(SHARED_M2P(gfn));
It seems ASSERT() is unnecessary. BUG_ON() is enough to cover it.
> + rc = hd->platform_ops->map_page(d, gfn, mfn,
> +
> IOMMUF_readable |
> +
> IOMMUF_writable);
> + }
> if ( rc )
> {
> page_list_add(page, &d->page_list);
> --- a/xen/include/asm-x86/hvm/iommu.h
> +++ b/xen/include/asm-x86/hvm/iommu.h
> @@ -46,6 +46,8 @@ struct g2m_ioport {
> unsigned int np;
> };
>
> +#define DEFAULT_DOMAIN_ADDRESS_WIDTH 48
> +
> struct arch_hvm_iommu
> {
> u64 pgd_maddr; /* io page directory machine
> address */
> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> @@ -464,8 +464,6 @@
> #define IOMMU_CONTROL_DISABLED 0
> #define IOMMU_CONTROL_ENABLED 1
>
> -#define DEFAULT_DOMAIN_ADDRESS_WIDTH 48
> -
> /* interrupt remapping table */
> #define INT_REMAP_ENTRY_REMAPEN_MASK 0x00000001
> #define INT_REMAP_ENTRY_REMAPEN_SHIFT 0
>
Best regards,
Yang
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] IOMMU/x86: avoid pages without GFN in page table creation/updating
2015-05-11 2:53 ` Zhang, Yang Z
@ 2015-05-11 6:47 ` Jan Beulich
2015-05-11 6:56 ` Zhang, Yang Z
0 siblings, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2015-05-11 6:47 UTC (permalink / raw)
To: Yang Z Zhang
Cc: Kevin Tian, AndrewCooper, Tim Deegan, Sander Eikelenboom,
Aravind Gopalakrishnan, suravee.suthikulpanit@amd.com, xen-devel
>>> On 11.05.15 at 04:53, <yang.z.zhang@intel.com> wrote:
> Jan Beulich wrote on 2015-05-07:
>> --- a/xen/drivers/passthrough/x86/iommu.c
>> +++ b/xen/drivers/passthrough/x86/iommu.c
>> @@ -59,10 +59,17 @@ int arch_iommu_populate_page_table(struc
>> if ( has_hvm_container_domain(d) ||
>> (page->u.inuse.type_info & PGT_type_mask) ==
>> PGT_writable_page )
>> {
>> - BUG_ON(SHARED_M2P(mfn_to_gmfn(d,
>> page_to_mfn(page))));
>> - rc = hd->platform_ops->map_page(
>> - d, mfn_to_gmfn(d, page_to_mfn(page)),
>> page_to_mfn(page),
>> - IOMMUF_readable|IOMMUF_writable);
>> + unsigned long mfn = page_to_mfn(page);
>> + unsigned long gfn = mfn_to_gmfn(d, mfn);
>> +
>> + if ( gfn != INVALID_MFN )
>> + {
>> + ASSERT(!(gfn >> DEFAULT_DOMAIN_ADDRESS_WIDTH));
>> + BUG_ON(SHARED_M2P(gfn));
>
> It seems ASSERT() is unnecessary. BUG_ON() is enough to cover it.
The two check completely different things, so I don't see how
the BUG_ON() would help with out of bounds, yet also not
INVALID_MFN GFNs. Please clarify what you mean here.
Jan
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] IOMMU/x86: avoid pages without GFN in page table creation/updating
2015-05-11 6:47 ` Jan Beulich
@ 2015-05-11 6:56 ` Zhang, Yang Z
0 siblings, 0 replies; 8+ messages in thread
From: Zhang, Yang Z @ 2015-05-11 6:56 UTC (permalink / raw)
To: Jan Beulich
Cc: Tian, Kevin, AndrewCooper, Tim Deegan, Sander Eikelenboom,
Aravind Gopalakrishnan, suravee.suthikulpanit@amd.com, xen-devel
Jan Beulich wrote on 2015-05-11:
>>>> On 11.05.15 at 04:53, <yang.z.zhang@intel.com> wrote:
>> Jan Beulich wrote on 2015-05-07:
>>> --- a/xen/drivers/passthrough/x86/iommu.c
>>> +++ b/xen/drivers/passthrough/x86/iommu.c
>>> @@ -59,10 +59,17 @@ int arch_iommu_populate_page_table(struc
>>> if ( has_hvm_container_domain(d) ||
>>> (page->u.inuse.type_info & PGT_type_mask) ==
>>> PGT_writable_page )
>>> {
>>> - BUG_ON(SHARED_M2P(mfn_to_gmfn(d, page_to_mfn(page)))); -
>>> rc = hd->platform_ops->map_page( - d,
>>> mfn_to_gmfn(d, page_to_mfn(page)), page_to_mfn(page), -
>>> IOMMUF_readable|IOMMUF_writable); + unsigned long mfn =
>>> page_to_mfn(page); + unsigned long gfn = mfn_to_gmfn(d,
>>> mfn); + + if ( gfn != INVALID_MFN ) + { +
>>> ASSERT(!(gfn >> DEFAULT_DOMAIN_ADDRESS_WIDTH)); +
>>> BUG_ON(SHARED_M2P(gfn));
>>
>> It seems ASSERT() is unnecessary. BUG_ON() is enough to cover it.
>
> The two check completely different things, so I don't see how the
> BUG_ON() would help with out of bounds, yet also not INVALID_MFN GFNs.
> Please clarify what you mean here.
You are right. I misread the code as BUG_ON(!SHARED_M2P(gfn)).
For Intel VT-d part:
Acked-by: Yang Zhang <yang.z.zhang@intel.com>
>
> Jan
Best regards,
Yang
^ permalink raw reply [flat|nested] 8+ messages in thread
* Ping: [PATCH] IOMMU/x86: avoid pages without GFN in page table creation/updating
2015-05-07 12:59 [PATCH] IOMMU/x86: avoid pages without GFN in page table creation/updating Jan Beulich
2015-05-07 13:13 ` Andrew Cooper
2015-05-11 2:53 ` Zhang, Yang Z
@ 2015-05-13 13:11 ` Jan Beulich
2015-05-13 13:42 ` Suravee Suthikulanit
3 siblings, 0 replies; 8+ messages in thread
From: Jan Beulich @ 2015-05-13 13:11 UTC (permalink / raw)
To: Aravind Gopalakrishnan, suravee.suthikulpanit
Cc: Kevin Tian, Andrew Cooper, Tim Deegan, Sander Eikelenboom,
Yang Z Zhang, xen-devel
Still waiting for AMD IOMMU maintainer feedback...
Thanks, Jan
>>> On 07.05.15 at 14:59, <JBeulich@suse.com> wrote:
> Handing INVALID_GFN to functions like hd->platform_ops->map_page()
> just can't do any good, and the ioreq server code results in such pages
> being on the list of ones owned by a guest.
>
> While - as suggested by Tim - we should use get_gfn()/put_gfn() there
> to eliminate races, we really can't due to holding the domain's page
> alloc lock. Ultimately arch_iommu_populate_page_table() may need to be
> switched to be GFN based. Here is what Tim said in this regard:
> "Ideally this loop would be iterating over all gfns in the p2m rather
> than over all owned MFNs. As long as needs_iommu gets set first,
> such a loop could safely be paused and restarted without worrying
> about concurrent updates. The code sould even stay in this file,
> though exposing an iterator from the p2m code would be a lot more
> efficient."
>
> Original by Andrew Cooper <andrew.cooper3@citrix.com>, using further
> suggestions from Tim Deegan <tim@xen.org>.
>
> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Tested-by: Sander Eikelenboom <linux@eikelenboom.it>
> Acked-by: Tim Deegan <tim@xen.org>
>
> --- a/xen/drivers/passthrough/amd/iommu_map.c
> +++ b/xen/drivers/passthrough/amd/iommu_map.c
> @@ -557,6 +557,10 @@ static int update_paging_mode(struct dom
> unsigned long old_root_mfn;
> struct hvm_iommu *hd = domain_hvm_iommu(d);
>
> + if ( gfn == INVALID_MFN )
> + return -EADDRNOTAVAIL;
> + ASSERT(!(gfn >> DEFAULT_DOMAIN_ADDRESS_WIDTH));
> +
> level = hd->arch.paging_mode;
> old_root = hd->arch.root_table;
> offset = gfn >> (PTE_PER_TABLE_SHIFT * (level - 1));
> @@ -729,12 +733,15 @@ int amd_iommu_unmap_page(struct domain *
> * we might need a deeper page table for lager gfn now */
> if ( is_hvm_domain(d) )
> {
> - if ( update_paging_mode(d, gfn) )
> + int rc = update_paging_mode(d, gfn);
> +
> + if ( rc )
> {
> spin_unlock(&hd->arch.mapping_lock);
> AMD_IOMMU_DEBUG("Update page mode failed gfn = %lx\n", gfn);
> - domain_crash(d);
> - return -EFAULT;
> + if ( rc != -EADDRNOTAVAIL )
> + domain_crash(d);
> + return rc;
> }
> }
>
> --- a/xen/drivers/passthrough/vtd/iommu.h
> +++ b/xen/drivers/passthrough/vtd/iommu.h
> @@ -482,7 +482,6 @@ struct qinval_entry {
> #define VTD_PAGE_TABLE_LEVEL_3 3
> #define VTD_PAGE_TABLE_LEVEL_4 4
>
> -#define DEFAULT_DOMAIN_ADDRESS_WIDTH 48
> #define MAX_IOMMU_REGS 0xc0
>
> extern struct list_head acpi_drhd_units;
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -59,10 +59,17 @@ int arch_iommu_populate_page_table(struc
> if ( has_hvm_container_domain(d) ||
> (page->u.inuse.type_info & PGT_type_mask) == PGT_writable_page )
> {
> - BUG_ON(SHARED_M2P(mfn_to_gmfn(d, page_to_mfn(page))));
> - rc = hd->platform_ops->map_page(
> - d, mfn_to_gmfn(d, page_to_mfn(page)), page_to_mfn(page),
> - IOMMUF_readable|IOMMUF_writable);
> + unsigned long mfn = page_to_mfn(page);
> + unsigned long gfn = mfn_to_gmfn(d, mfn);
> +
> + if ( gfn != INVALID_MFN )
> + {
> + ASSERT(!(gfn >> DEFAULT_DOMAIN_ADDRESS_WIDTH));
> + BUG_ON(SHARED_M2P(gfn));
> + rc = hd->platform_ops->map_page(d, gfn, mfn,
> + IOMMUF_readable |
> + IOMMUF_writable);
> + }
> if ( rc )
> {
> page_list_add(page, &d->page_list);
> --- a/xen/include/asm-x86/hvm/iommu.h
> +++ b/xen/include/asm-x86/hvm/iommu.h
> @@ -46,6 +46,8 @@ struct g2m_ioport {
> unsigned int np;
> };
>
> +#define DEFAULT_DOMAIN_ADDRESS_WIDTH 48
> +
> struct arch_hvm_iommu
> {
> u64 pgd_maddr; /* io page directory machine address */
> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> @@ -464,8 +464,6 @@
> #define IOMMU_CONTROL_DISABLED 0
> #define IOMMU_CONTROL_ENABLED 1
>
> -#define DEFAULT_DOMAIN_ADDRESS_WIDTH 48
> -
> /* interrupt remapping table */
> #define INT_REMAP_ENTRY_REMAPEN_MASK 0x00000001
> #define INT_REMAP_ENTRY_REMAPEN_SHIFT 0
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] IOMMU/x86: avoid pages without GFN in page table creation/updating
2015-05-07 12:59 [PATCH] IOMMU/x86: avoid pages without GFN in page table creation/updating Jan Beulich
` (2 preceding siblings ...)
2015-05-13 13:11 ` Ping: " Jan Beulich
@ 2015-05-13 13:42 ` Suravee Suthikulanit
3 siblings, 0 replies; 8+ messages in thread
From: Suravee Suthikulanit @ 2015-05-13 13:42 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Kevin Tian, Andrew Cooper, Tim Deegan, Sander Eikelenboom,
Aravind Gopalakrishnan, Yang Z Zhang
Acked-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
Thanks,
Suravee
On 5/7/2015 7:59 AM, Jan Beulich wrote:
> Handing INVALID_GFN to functions like hd->platform_ops->map_page()
> just can't do any good, and the ioreq server code results in such pages
> being on the list of ones owned by a guest.
>
> While - as suggested by Tim - we should use get_gfn()/put_gfn() there
> to eliminate races, we really can't due to holding the domain's page
> alloc lock. Ultimately arch_iommu_populate_page_table() may need to be
> switched to be GFN based. Here is what Tim said in this regard:
> "Ideally this loop would be iterating over all gfns in the p2m rather
> than over all owned MFNs. As long as needs_iommu gets set first,
> such a loop could safely be paused and restarted without worrying
> about concurrent updates. The code sould even stay in this file,
> though exposing an iterator from the p2m code would be a lot more
> efficient."
>
> Original by Andrew Cooper <andrew.cooper3@citrix.com>, using further
> suggestions from Tim Deegan <tim@xen.org>.
>
> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Tested-by: Sander Eikelenboom <linux@eikelenboom.it>
> Acked-by: Tim Deegan <tim@xen.org>
>
> --- a/xen/drivers/passthrough/amd/iommu_map.c
> +++ b/xen/drivers/passthrough/amd/iommu_map.c
> @@ -557,6 +557,10 @@ static int update_paging_mode(struct dom
> unsigned long old_root_mfn;
> struct hvm_iommu *hd = domain_hvm_iommu(d);
>
> + if ( gfn == INVALID_MFN )
> + return -EADDRNOTAVAIL;
> + ASSERT(!(gfn >> DEFAULT_DOMAIN_ADDRESS_WIDTH));
> +
> level = hd->arch.paging_mode;
> old_root = hd->arch.root_table;
> offset = gfn >> (PTE_PER_TABLE_SHIFT * (level - 1));
> @@ -729,12 +733,15 @@ int amd_iommu_unmap_page(struct domain *
> * we might need a deeper page table for lager gfn now */
> if ( is_hvm_domain(d) )
> {
> - if ( update_paging_mode(d, gfn) )
> + int rc = update_paging_mode(d, gfn);
> +
> + if ( rc )
> {
> spin_unlock(&hd->arch.mapping_lock);
> AMD_IOMMU_DEBUG("Update page mode failed gfn = %lx\n", gfn);
> - domain_crash(d);
> - return -EFAULT;
> + if ( rc != -EADDRNOTAVAIL )
> + domain_crash(d);
> + return rc;
> }
> }
>
> --- a/xen/drivers/passthrough/vtd/iommu.h
> +++ b/xen/drivers/passthrough/vtd/iommu.h
> @@ -482,7 +482,6 @@ struct qinval_entry {
> #define VTD_PAGE_TABLE_LEVEL_3 3
> #define VTD_PAGE_TABLE_LEVEL_4 4
>
> -#define DEFAULT_DOMAIN_ADDRESS_WIDTH 48
> #define MAX_IOMMU_REGS 0xc0
>
> extern struct list_head acpi_drhd_units;
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -59,10 +59,17 @@ int arch_iommu_populate_page_table(struc
> if ( has_hvm_container_domain(d) ||
> (page->u.inuse.type_info & PGT_type_mask) == PGT_writable_page )
> {
> - BUG_ON(SHARED_M2P(mfn_to_gmfn(d, page_to_mfn(page))));
> - rc = hd->platform_ops->map_page(
> - d, mfn_to_gmfn(d, page_to_mfn(page)), page_to_mfn(page),
> - IOMMUF_readable|IOMMUF_writable);
> + unsigned long mfn = page_to_mfn(page);
> + unsigned long gfn = mfn_to_gmfn(d, mfn);
> +
> + if ( gfn != INVALID_MFN )
> + {
> + ASSERT(!(gfn >> DEFAULT_DOMAIN_ADDRESS_WIDTH));
> + BUG_ON(SHARED_M2P(gfn));
> + rc = hd->platform_ops->map_page(d, gfn, mfn,
> + IOMMUF_readable |
> + IOMMUF_writable);
> + }
> if ( rc )
> {
> page_list_add(page, &d->page_list);
> --- a/xen/include/asm-x86/hvm/iommu.h
> +++ b/xen/include/asm-x86/hvm/iommu.h
> @@ -46,6 +46,8 @@ struct g2m_ioport {
> unsigned int np;
> };
>
> +#define DEFAULT_DOMAIN_ADDRESS_WIDTH 48
> +
> struct arch_hvm_iommu
> {
> u64 pgd_maddr; /* io page directory machine address */
> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> @@ -464,8 +464,6 @@
> #define IOMMU_CONTROL_DISABLED 0
> #define IOMMU_CONTROL_ENABLED 1
>
> -#define DEFAULT_DOMAIN_ADDRESS_WIDTH 48
> -
> /* interrupt remapping table */
> #define INT_REMAP_ENTRY_REMAPEN_MASK 0x00000001
> #define INT_REMAP_ENTRY_REMAPEN_SHIFT 0
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-05-13 13:42 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-05-07 12:59 [PATCH] IOMMU/x86: avoid pages without GFN in page table creation/updating Jan Beulich
2015-05-07 13:13 ` Andrew Cooper
2015-05-07 13:50 ` Jan Beulich
2015-05-11 2:53 ` Zhang, Yang Z
2015-05-11 6:47 ` Jan Beulich
2015-05-11 6:56 ` Zhang, Yang Z
2015-05-13 13:11 ` Ping: " Jan Beulich
2015-05-13 13:42 ` Suravee Suthikulanit
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.