Fixes for paging/sharing with AMD processors
PATCH 1/1 Fixes for paging/sharing with AMD processors
These are patches previously posted as RFC towards making paging and sharing work for AMD NPT.
They are now ready for inclusion in the tree. They do not yet completely fix paging in AMD, but each fixes an existing bug:
- Add "no-sharept" cli to allow disabling of sharing of iommu and p2m tables.
- Mask high order bits when installing INVALID_MFN on a pte, so those bits don't trample over flags such as _PAGE_NX. I've used a suggestion from Tim Deegan in this patch, so I took the liberty of adding his SoB.
- Teach paging to the page table-based p2m (p2m-pt). This involves fixing a few if's and ASSERT's.
They've been tested to not break existing modes (shadow, AMD hap, dom0 as test for pv domains)
xen/drivers/passthrough/iommu.c | 2 ++ xen/include/asm-x86/page.h | 12 ++++++++---- xen/arch/x86/mm/p2m-pt.c | 30 +++++++++++++++++++----------- 3 files changed, 29 insertions(+), 15 deletions(-)
PATCH 1/1 IOMMU: Add command line param to disable sharing of IOMMU and hap tables
xen/drivers/passthrough/iommu.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
The default is 1, and the command line parameter sets to 1. Disabling may be desired if the host will contain VMs that do paging, sharing or mem access, and won't be doing passthrough. These two features are mutually exclusive for AMD processors.
xen/drivers/passthrough/iommu.c +2 -0
@@ -87,6 +87,8 @@ static void __init parse_iommu_param(cha iommu_dom0_strict =1 ; else if ( !strcmp(s, "sharept") ) iommu_hap_pt_share =1 ; + else if ( !strcmp(s, "no-sharept") ) + iommu_hap_pt_share =0 ; s = ss +1 ; }while ( ss );
PATCH 1/1 Clip mfn to allowable width when building a PTE
In reply to Andres Lagar-Cavillaxen/include/asm-x86/page.h | 12 ++++++++---- 1 files changed, 8 insertions(+), 4 deletions(-)
Otherwise, INVALID_MFN tramples over high order bits used for additional flags.
xen/include/asm-x86/page.h +8 -4
@@ -107,13 +107,17 @@/* Construct a pte from a pfn and access flags. */ #define l1e_from_pfn (pfn, flags) \ - ((l1_pgentry_t) { ((intpte_t)(pfn) << PAGE_SHIFT) | put_pte_flags(flags) }) + ((l1_pgentry_t) { (((intpte_t)(pfn) << PAGE_SHIFT) & PADDR_MASK) \ + | put_pte_flags(flags) })#define l2e_from_pfn (pfn, flags) \ - ((l2_pgentry_t) { ((intpte_t)(pfn) << PAGE_SHIFT) | put_pte_flags(flags) }) + ((l2_pgentry_t) { (((intpte_t)(pfn) << PAGE_SHIFT) & PADDR_MASK) \ + | put_pte_flags(flags) })#define l3e_from_pfn (pfn, flags) \ - ((l3_pgentry_t) { ((intpte_t)(pfn) << PAGE_SHIFT) | put_pte_flags(flags) }) + ((l3_pgentry_t) { (((intpte_t)(pfn) << PAGE_SHIFT) & PADDR_MASK) \ + | put_pte_flags(flags) })#define l4e_from_pfn (pfn, flags) \ - ((l4_pgentry_t) { ((intpte_t)(pfn) << PAGE_SHIFT) | put_pte_flags(flags) }) + ((l4_pgentry_t) { (((intpte_t)(pfn) << PAGE_SHIFT) & PADDR_MASK) \ + | put_pte_flags(flags) })/* Construct a pte from a physical address and access flags. */ #ifndef __ASSEMBLY__
PATCH 1/1 x86/mm: Teach paging to page table-based p2m
In reply to Andres Lagar-Cavillaxen/arch/x86/mm/p2m-pt.c | 30 +++++++++++++++++++----------- 1 files changed, 19 insertions(+), 11 deletions(-)
The p2m-pt.c code, used by both shadow and AMD NPT modes, was not aware of paging types, and the implications those types have on p2m entries. Add support to the page table-based p2m to understand the paging types. This is a necessary step towards enabling memory paging on AMD NPT mode, but not yet the full solution.
Tested not to break neither shadow mode nor "normal" (i.e. no paging) AMD NPT mode.
xen/arch/x86/mm/p2m-pt.c +19 -11
@@ -77,6 +77,9 @@ static unsigned long p2m_type_to_flags(pcase p2m_invalid:case p2m_mmio_dm:case p2m_populate_on_demand: +case p2m_ram_paging_out: +case p2m_ram_paged: +case p2m_ram_paging_in:default :return flags;case p2m_ram_ro:
@@ -168,7 +171,7 @@ p2m_next_level(struct p2m_domain *p2m, m shift, max)) )return 0 ; -/* PoD: Not present doesn't imply empty. */ +/* PoD/paging: Not present doesn't imply empty. */ if ( !l1e_get_flags (*p2m_entry) ) {struct page_info *pg;
@@ -384,7 +387,7 @@ p2m_set_entry(struct p2m_domain *p2m, un 0, L1_PAGETABLE_ENTRIES);ASSERT (p2m_entry); -if (mfn_valid (mfn) || (p2mt == p2m_mmio_direct) ) +if (mfn_valid (mfn) || (p2mt == p2m_mmio_direct) ||p2m_is_paging (p2mt) ) entry_content =l1e_from_pfn (mfn_x (mfn),p2m_type_to_flags (p2mt, mfn));else
@@ -615,11 +618,12 @@ pod_retry_l1: sizeof(l1e));if ( ret ==0 ) { +unsigned long l1e_mfn =l1e_get_pfn (l1e); p2mt =p2m_flags_to_type (l1e_get_flags (l1e)); -ASSERT (l1e_get_pfn (l1e) !=INVALID_MFN || !p2m_is_ram (p2mt)); +ASSERT ( (l1e_mfn !=INVALID_MFN || !p2m_is_ram (p2mt)) || + (l1e_mfn ==INVALID_MFN &&p2m_is_paging (p2mt)) ); -if (p2m_flags_to_type (l1e_get_flags (l1e)) - == p2m_populate_on_demand ) +if ( p2mt == p2m_populate_on_demand ) {/* The read has succeeded, so we know that the mapping * exits at this point. */
@@ -641,7 +645,7 @@ pod_retry_l1: }if (p2m_is_valid (p2mt) ||p2m_is_grant (p2mt) ) - mfn =_mfn (l1e_get_pfn (l1e)); + mfn =_mfn (l1e_mfn);else /* XXX see above */ p2mt = p2m_mmio_dm;
@@ -663,6 +667,8 @@ p2m_gfn_to_mfn(struct p2m_domain *p2m, upaddr_t addr = ((paddr_t )gfn) <<PAGE_SHIFT ;l2_pgentry_t *l2e;l1_pgentry_t *l1e; +unsigned long l1e_flags; +p2m_type_t l1t;ASSERT (paging_mode_translate (p2m->domain));
@@ -781,10 +787,12 @@ pod_retry_l2: l1e =map_domain_page (mfn_x (mfn)); l1e +=l1_table_offset (addr); pod_retry_l1: -if ( (l1e_get_flags (*l1e) & _PAGE_PRESENT) ==0 ) + l1e_flags =l1e_get_flags (*l1e); + l1t =p2m_flags_to_type (l1e_flags); +if ( ((l1e_flags & _PAGE_PRESENT) ==0 ) && (!p2m_is_paging (l1t)) ) {/* PoD: Try to populate */ -if (p2m_flags_to_type (l1e_get_flags (*l1e)) == p2m_populate_on_demand ) +if ( l1t == p2m_populate_on_demand ) {if ( q &P2M_ALLOC ) {if ( !p2m_pod_demand_populate (p2m, gfn,PAGE_ORDER_4K , q) )
@@ -792,15 +800,15 @@ pod_retry_l1: }else *t = p2m_populate_on_demand; } - +unmap_domain_page (l1e);return _mfn (INVALID_MFN ); } mfn =_mfn (l1e_get_pfn (*l1e)); - *t =p2m_flags_to_type (l1e_get_flags (*l1e)); + *t = l1t;unmap_domain_page (l1e); -ASSERT (mfn_valid (mfn) || !p2m_is_ram (*t)); +ASSERT (mfn_valid (mfn) || !p2m_is_ram (*t) ||p2m_is_paging (*t));if ( page_order ) *page_order =PAGE_ORDER_4K ;return (p2m_is_valid (*t) ||p2m_is_grant (*t)) ? mfn :_mfn (INVALID_MFN );
Re: [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables
In reply to Andres Lagar-CavillaQuoted 18 lines
Quoted 1 line
Quoted 1 line · Andres Lagar-Cavilla
xen/drivers/passthrough/iommu.c| 2 ++1 files changed, 2 insertions(+), 0 deletions(-)The default is 1, and the command line parameter sets to 1. Disabling may be desired if the host will contain VMs that do paging, sharing or mem access, and
won't be doing passthrough. These two features are mutually exclusive for AMD
processors.Signed-off-by Andres Lagar-Cavilla <[email protected]>diff -r
d7e417afcbe4-r642c0e6a01c2xen/drivers/passthrough/iommu.c
xen/drivers/passthrough/iommu.c static void __init parse_iommu_param(cha · line 91
Quoted 5 lines
xen/drivers/passthrough/iommu.c +2
-0@@ -87,6 +87,8 @@ static void __init parse_iommu_param(cha iommu_dom0_strict =1 ; else if ( !strcmp(s, "sharept") ) iommu_hap_pt_share =1 ; + else if ( !strcmp(s, "no-sharept") ) + iommu_hap_pt_share =0 ;
Taking the description above into consideration - why not simply replace the pointless enabling option with the disabling one?
Jan
Quoted 8 lines
xen/drivers/passthrough/iommu.c +0
-0s = ss +1 ; }while ( ss );_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel
Re: [PATCH 2 of 3] Clip mfn to allowable width when building a PTE
In reply to Andres Lagar-CavillaQuoted 7 lines
Quoted 1 line
Quoted 1 line · Andres Lagar-Cavilla
xen/include/asm-x86/page.h| 12 ++++++++----1 files changed, 8 insertions(+), 4 deletions(-)Otherwise,
INVALID_MFNtramples over high order bits used for additional flags.
But is passing INVALID_PFN into these macros valid/sensible in the first place?
Jan
Quoted 37 lines
Signed-off-by Andres Lagar-Cavilla <[email protected]>Signed-off-by Tim Deegan <[email protected]>diff -r
642c0e6a01c2-re325da663345xen/include/asm-x86/page.hxen/include/asm-x86/page.h +6
-4@@ -107,13 +107,17 @@/* Construct a pte from a pfn and access flags. */ #define l1e_from_pfn (pfn, flags) \ - ((l1_pgentry_t) { ((intpte_t)(pfn) << PAGE_SHIFT) | put_pte_flags(flags) }) + ((l1_pgentry_t ) { (((intpte_t)(pfn) <<PAGE_SHIFT ) &PADDR_MASK ) \ + | put_pte_flags(flags) })#define l2e_from_pfn (pfn, flags) \ - ((l2_pgentry_t) { ((intpte_t)(pfn) << PAGE_SHIFT) | put_pte_flags(flags) }) + ((l2_pgentry_t ) { (((intpte_t)(pfn) <<PAGE_SHIFT ) &PADDR_MASK ) \ + | put_pte_flags(flags) })#define l3e_from_pfn (pfn, flags) \ - ((l3_pgentry_t) { ((intpte_t)(pfn) << PAGE_SHIFT) | put_pte_flags(flags) }) + ((l3_pgentry_t ) { (((intpte_t)(pfn) <<PAGE_SHIFT ) &PADDR_MASK ) \ + | put_pte_flags(flags) })#define l4e_from_pfn (pfn, flags) \ - ((l4_pgentry_t) { ((intpte_t)(pfn) << PAGE_SHIFT) | put_pte_flags(flags)})
xen/include/asm-x86/page.h +2
-0+ ((l4_pgentry_t ) { (((intpte_t)(pfn) <<PAGE_SHIFT ) &PADDR_MASK ) \ + |put_pte_flags (flags) }) /* Construct a pte from a physical address and access flags. */#ifndef __ASSEMBLY_________________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel
Re: [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables
In reply to Jan BeulichQuoted 10 lines · Jan Beulich
Quoted 10 lines
Quoted 1 line
Quoted 1 line · Andres Lagar-Cavilla
The default is 1, and the command line parameter sets to 1. Disabling may be desired if the host will contain VMs that do paging, sharing or mem access, and
won't be doing passthrough. These two features are mutually exclusive for AMD
processors.Signed-off-by Andres Lagar-Cavilla <[email protected]>
Quoted 12 lines
xen/drivers/passthrough/iommu.c static void __init parse_iommu_param(cha · line 91
Quoted 9 lines
diff -r
d7e417afcbe4-r642c0e6a01c2xen/drivers/passthrough/iommu.cxen/drivers/passthrough/iommu.c +2
-0@@ -87,6 +87,8 @@ static void __init parse_iommu_param(cha iommu_dom0_strict =1 ; else if ( !strcmp(s, "sharept") ) iommu_hap_pt_share =1 ; + else if ( !strcmp(s, "no-sharept") ) + iommu_hap_pt_share =0 ;Taking the description above into consideration - why not simply replace the pointless enabling option with the disabling one?
Let's just fix the whole function to handle 'no-' the way the main parser does:
-----
IOMMU: clean up handling of 'foo' and 'no-foo' command-line options
xen/drivers/passthrough/iommu.c +19 -14
@@ -57,36 +57,41 @@ DEFINE_PER_CPU(bool_t, iommu_dont_flush_static void __init parse_iommu_param (char *s) {char *ss; +int val;do { + val = !!strncmp (s,"no-" ,3 ); +if ( !val ) + s +=3 ; + ss =strchr (s,',' );if ( ss ) *ss ='\0' ;if ( !parse_bool (s) ) - iommu_enabled =0 ; + iommu_enabled = val;else if ( !strcmp (s,"force" ) || !strcmp (s,"required" ) ) - force_iommu =1 ; + force_iommu = val;else if ( !strcmp (s,"workaround_bios_bug" ) ) iommu_workaround_bios_bug =1 ;else if ( !strcmp (s,"verbose" ) ) - iommu_verbose =1 ; -else if ( !strcmp (s,"no-snoop" ) ) - iommu_snoop =0 ; -else if ( !strcmp (s,"no-qinval" ) ) - iommu_qinval =0 ; -else if ( !strcmp (s,"no-intremap" ) ) - iommu_intremap =0 ; + iommu_verbose = val; +else if ( !strcmp (s,"snoop" ) ) + iommu_snoop = val; +else if ( !strcmp (s,"qinval" ) ) + iommu_qinval = val; +else if ( !strcmp (s,"intremap" ) ) + iommu_intremap = val;else if ( !strcmp (s,"debug" ) ) - iommu_debug =1 ; + iommu_debug = val;else if ( !strcmp (s,"amd-iommu-perdev-intremap" ) ) - amd_iommu_perdev_intremap =1 ; + amd_iommu_perdev_intremap = val;else if ( !strcmp (s,"dom0-passthrough" ) ) - iommu_passthrough =1 ; + iommu_passthrough = val;else if ( !strcmp (s,"dom0-strict" ) ) - iommu_dom0_strict =1 ; + iommu_dom0_strict = val;else if ( !strcmp (s,"sharept" ) ) - iommu_hap_pt_share =1 ; + iommu_hap_pt_share = val; s = ss +1 ; }while ( ss );
Re: [PATCH 2 of 3] Clip mfn to allowable width when building a PTE
In reply to Jan BeulichQuoted 10 lines · Jan Beulich
Quoted 7 lines
Quoted 1 line
Quoted 1 line · Andres Lagar-Cavilla
xen/include/asm-x86/page.h| 12 ++++++++----1 files changed, 8 insertions(+), 4 deletions(-)Otherwise,
INVALID_MFNtramples over high order bits used for additional flags.But is passing
INVALID_PFNinto these macros valid/sensible in the first place?
The p2m code uses pte layout even for entries that don't have the _PAGE_PRESENT bit set. We can:
- mask out in these macros, making everything safe;
- make new macros just for p2m code;
- rewrite p2m callers not to use INVALID_MFN; or
- have the p2m code explicitly replace INVALID_MFN with some other value when callers specify it.
I'm happy to do it this way but I guess maybe it slows down paths that don't need the mask.
Tim.
Quoted 45 lines
Quoted 33 lines
Signed-off-by Andres Lagar-Cavilla <[email protected]>Signed-off-by Tim Deegan <[email protected]>diff -r
642c0e6a01c2-re325da663345xen/include/asm-x86/page.hxen/include/asm-x86/page.h +6
-4@@ -107,13 +107,17 @@/* Construct a pte from a pfn and access flags. */ #define l1e_from_pfn (pfn, flags) \ - ((l1_pgentry_t) { ((intpte_t)(pfn) << PAGE_SHIFT) | put_pte_flags(flags) }) + ((l1_pgentry_t ) { (((intpte_t)(pfn) <<PAGE_SHIFT ) &PADDR_MASK ) \ + | put_pte_flags(flags) })#define l2e_from_pfn (pfn, flags) \ - ((l2_pgentry_t) { ((intpte_t)(pfn) << PAGE_SHIFT) | put_pte_flags(flags) }) + ((l2_pgentry_t ) { (((intpte_t)(pfn) <<PAGE_SHIFT ) &PADDR_MASK ) \ + | put_pte_flags(flags) })#define l3e_from_pfn (pfn, flags) \ - ((l3_pgentry_t) { ((intpte_t)(pfn) << PAGE_SHIFT) | put_pte_flags(flags) }) + ((l3_pgentry_t ) { (((intpte_t)(pfn) <<PAGE_SHIFT ) &PADDR_MASK ) \ + | put_pte_flags(flags) })#define l4e_from_pfn (pfn, flags) \ - ((l4_pgentry_t) { ((intpte_t)(pfn) << PAGE_SHIFT) | put_pte_flags(flags)})
xen/include/asm-x86/page.h +2
-0+ ((l4_pgentry_t ) { (((intpte_t)(pfn) <<PAGE_SHIFT ) &PADDR_MASK ) \ + |put_pte_flags (flags) }) /* Construct a pte from a physical address and access flags. */#ifndef __ASSEMBLY__Quoted 4 lines
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel
Re: [PATCH 3 of 3] x86/mm: Teach paging to page table-based p2m
In reply to Andres Lagar-CavillaHi,
The last version of this patch had the beginnings of an interlock to avoid iommu-pt-sharing and p2m-fu happening at the same time. I suggested taht it wasn't complete enough, but it seems to have gone away entirely!
Also:
xen/arch/x86/mm/p2m-pt.c pod_retry_l1: · line 624
Quoted 9 lines · Andres Lagar-Cavilla
xen/arch/x86/mm/p2m-pt.c +3
-1@@ -615,11 +618,12 @@ pod_retry_l1: sizeof(l1e));if ( ret ==0 ) { +unsigned long l1e_mfn =l1e_get_pfn (l1e); p2mt =p2m_flags_to_type (l1e_get_flags (l1e)); -ASSERT (l1e_get_pfn (l1e) !=INVALID_MFN || !p2m_is_ram (p2mt)); +ASSERT ( (l1e_mfn !=INVALID_MFN || !p2m_is_ram (p2mt)) || + (l1e_mfn ==INVALID_MFN &&p2m_is_paging (p2mt)) );
I guess, given the discussion in the other subthread, that this ASSERT always passes, and should be using mfn_valid() instead?
Cheers,
Tim.
Re: [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables
In reply to Tim DeeganQuoted 27 lines
Quoted 1 line
Quoted 1 line · Tim Deegan
Quoted 2 lines · Jan Beulich
Quoted 2 lines
Quoted 1 line
Quoted 1 line · Andres Lagar-Cavilla
The default is 1, and the command line parameter sets to 1. Disabling may
be
Quoted 9 lines
Quoted 9 lines
desired if the host will contain VMs that do paging, sharing or mem access,
and
won't be doing passthrough. These two features are mutually exclusive for AMD
processors.Signed-off-by Andres Lagar-Cavilla <[email protected]>Quoted 12 lines
xen/drivers/passthrough/iommu.c static void __init parse_iommu_param(cha · line 91
Quoted 9 lines
diff -r
d7e417afcbe4-r642c0e6a01c2xen/drivers/passthrough/iommu.cxen/drivers/passthrough/iommu.c +2
-0@@ -87,6 +87,8 @@ static void __init parse_iommu_param(cha iommu_dom0_strict =1 ; else if ( !strcmp(s, "sharept") ) iommu_hap_pt_share =1 ; + else if ( !strcmp(s, "no-sharept") ) + iommu_hap_pt_share =0 ;Taking the description above into consideration - why not simply replace the pointless enabling option with the disabling one?
Quoted 2 lines
Let's just fix the whole function to handle 'no-' the way the main parser does:
Good idea, but ...
Quoted 17 lines
-----
IOMMU: clean up handling of 'foo' and 'no-foo' command-line options
Signed-off-by Tim Deegan <[email protected]>diff -r
8180cb3895afxen/drivers/passthrough/iommu.cxen/drivers/passthrough/iommu.c +2
-0@@ -57,36 +57,41 @@ DEFINE_PER_CPU(bool_t, iommu_dont_flush_static void __init parse_iommu_param (char *s) {char *ss; +int val; do { + val = !!strncmp(s, "no-", 3);
xen/drivers/passthrough/iommu.c DEFINE_PER_CPU(bool_t, iommu_dont_flush_ · line 72
Quoted 10 lines
xen/drivers/passthrough/iommu.c +4
-1+if ( !val ) + s +=3 ; + ss =strchr (s,',' );if ( ss ) *ss ='\0' ;if ( !parse_bool (s) ) - iommu_enabled =0 ; + iommu_enabled = val;
... this one must remain unchanged, and the handling of the "no-" prefix should come afterwards.
Jan
Quoted 38 lines
xen/drivers/passthrough/iommu.c +13
-13else if ( !strcmp (s,"force" ) || !strcmp (s,"required" ) ) - force_iommu =1 ; + force_iommu = val;else if ( !strcmp (s,"workaround_bios_bug" ) ) iommu_workaround_bios_bug =1 ;else if ( !strcmp (s,"verbose" ) ) - iommu_verbose =1 ; -else if ( !strcmp (s,"no-snoop" ) ) - iommu_snoop =0 ; -else if ( !strcmp (s,"no-qinval" ) ) - iommu_qinval =0 ; -else if ( !strcmp (s,"no-intremap" ) ) - iommu_intremap =0 ; + iommu_verbose = val; +else if ( !strcmp (s,"snoop" ) ) + iommu_snoop = val; +else if ( !strcmp (s,"qinval" ) ) + iommu_qinval = val; +else if ( !strcmp (s,"intremap" ) ) + iommu_intremap = val;else if ( !strcmp (s,"debug" ) ) - iommu_debug =1 ; + iommu_debug = val;else if ( !strcmp (s,"amd-iommu-perdev-intremap" ) ) - amd_iommu_perdev_intremap =1 ; + amd_iommu_perdev_intremap = val;else if ( !strcmp (s,"dom0-passthrough" ) ) - iommu_passthrough =1 ; + iommu_passthrough = val;else if ( !strcmp (s,"dom0-strict" ) ) - iommu_dom0_strict =1 ; + iommu_dom0_strict = val;else if ( !strcmp (s,"sharept" ) ) - iommu_hap_pt_share =1 ; + iommu_hap_pt_share = val; s = ss +1 ; }while ( ss );
Re: [PATCH 2 of 3] Clip mfn to allowable width when building a PTE
In reply to Tim DeeganQuoted 13 lines
Quoted 1 line
Quoted 1 line · Tim Deegan
Quoted 10 lines · Jan Beulich
Quoted 7 lines
Quoted 1 line
Quoted 1 line · Andres Lagar-Cavilla
xen/include/asm-x86/page.h| 12 ++++++++----1 files changed, 8 insertions(+), 4 deletions(-)Otherwise,
INVALID_MFNtramples over high order bits used for additional flags.But is passing
INVALID_PFNinto these macros valid/sensible in the first place?
Quoted 7 lines
The p2m code uses pte layout even for entries that don't have the
_PAGE_PRESENTbit set. We can:
- mask out in these macros, making everything safe;
- make new macros just for p2m code;
- rewrite p2m callers not to useINVALID_MFN; or
- have the p2m code explicitly replaceINVALID_MFNwith some other value when callers specify it.
As the transformation backwards doesn't yield INVALID_MFN anyway, I'd prefer one of those options.
Quoted 2 lines
I'm happy to do it this way but I guess maybe it slows down paths that don't need the mask.
Yes, that was one of the concerns - less because of the slowdown, but more because the generated code then becomes even worse to look at (which at least I happen to be required to do now and then, albeit admittedly less frequently over time, i.e. as the general code quality improves).
Jan
Re: [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables
In reply to Jan BeulichQuoted 6 lines · Jan Beulich
xen/drivers/passthrough/iommu.c DEFINE_PER_CPU(bool_t, iommu_dont_flush_ · line 72
Quoted 3 lines
if ( !parse_bool(s) )xen/drivers/passthrough/iommu.c +1
-1- iommu_enabled =0 ; + iommu_enabled = val;... this one must remain unchanged, and the handling of the "no-" prefix should come afterwards.
Meh. I reserve the right to set 'iommu=no-yes'. :) That is to say, I don't think it makes any real difference.
Tim.
Quoted 46 lines
Quoted 28 lines
else if ( !strcmp(s, "force") || !strcmp(s, "required") )xen/drivers/passthrough/iommu.c +10
-11- force_iommu =1 ; + force_iommu = val; else if ( !strcmp(s, "workaround_bios_bug") ) iommu_workaround_bios_bug =1 ; else if ( !strcmp(s, "verbose") ) - iommu_verbose =1 ; - else if ( !strcmp(s, "no-snoop") ) - iommu_snoop =0 ; - else if ( !strcmp(s, "no-qinval") ) - iommu_qinval =0 ; - else if ( !strcmp(s, "no-intremap") ) - iommu_intremap =0 ; + iommu_verbose = val; + else if ( !strcmp(s, "snoop") ) + iommu_snoop = val; + else if ( !strcmp(s, "qinval") ) + iommu_qinval = val; + else if ( !strcmp(s, "intremap") ) + iommu_intremap = val; else if ( !strcmp(s, "debug") ) - iommu_debug =1 ; + iommu_debug = val; else if ( !strcmp(s, "amd-iommu-perdev-intremap") ) - amd_iommu_perdev_intremap =1 ; + amd_iommu_perdev_intremap = val; else if ( !strcmp(s, "dom0-passthrough") ) - iommu_passthrough =1 ;xen/drivers/passthrough/iommu.c DEFINE_PER_CPU(bool_t, iommu_dont_flush_ · line 97
Quoted 10 lines
xen/drivers/passthrough/iommu.c +3
-2+ iommu_passthrough = val; else if ( !strcmp(s, "dom0-strict") ) - iommu_dom0_strict =1 ; + iommu_dom0_strict = val; else if ( !strcmp(s, "sharept") ) - iommu_hap_pt_share =1 ; + iommu_hap_pt_share = val; s = ss +1 ; }while ( ss );_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel
Re: [PATCH 3 of 3] x86/mm: Teach paging to page table-based p2m
In reply to Tim DeeganQuoted 6 lines
Hi,
The last version of this patch had the beginnings of an interlock to avoid iommu-pt-sharing and p2m-fu happening at the same time. I suggested taht it wasn't complete enough, but it seems to have gone away entirely!
Tim,
a follow up patch would allow enabling paging on AMD, with the interlock in place. It would be of the form:
xen/arch/x86/mm/mem_event.c +5 -2
@@ -563,8 +563,11 @@ int mem_event_domctl(struct domain *d, xif ( !hap_enabled (d) )break ; -/* Currently only EPT is supported */ -if ( boot_cpu_data.x86_vendor !=X86_VENDOR_INTEL ) +/* Currently EPT or AMD with no iommu/hap page table sharing are +* supported */ +if ( !((boot_cpu_data.x86_vendor ==X86_VENDOR_INTEL ) || + ((boot_cpu_data.x86_vendor ==X86_VENDOR_AMD ) && + !need_iommu (d))) )break ; rc = -EXDEV ;
Right now I haven't posted that as paging on AMD doesn't fully work. The current patch moves us closer into that direction.
The interlock itself would need more fleshing out methinks. AMD needs hap_pt_share to be disabled in order to enable paging (and in the fullness of time, mem access). But more generally, wouldn't both Intel and AMD require !need_iommu()? Also, is it enough to just check during enable-p2m-foo time? What about a "p2m-foo on" flag to prevent passthrough from happening at a later time, as well?
Quoted 7 lines
Also:
Quoted 3 lines · Andres Lagar-Cavilla
(quoted diff) +0
-0@@ -615,11 +618,12 @@ pod_retry_l1: sizeof(l1e));
Quoted 9 lines
Review pod_retry_l1: · line 624
Quoted 6 lines
(quoted diff) +3
-1if ( ret == 0 ) { + unsigned long l1e_mfn = l1e_get_pfn(l1e); p2mt = p2m_flags_to_type(l1e_get_flags(l1e)); - ASSERT(l1e_get_pfn(l1e) != INVALID_MFN || !p2m_is_ram(p2mt)); + ASSERT( (l1e_mfn != INVALID_MFN || !p2m_is_ram(p2mt)) || + (l1e_mfn == INVALID_MFN && p2m_is_paging(p2mt)) );I guess, given the discussion in the other subthread, that this ASSERT always passes, and should be using
mfn_valid()instead?
Correct right now. Depends on where we end up wrt to patch #2 of this series.
Thanks
Andres
Quoted 5 lines
Cheers,
Tim.
Re: [PATCH 2 of 3] Clip mfn to allowable width when building a PTE
In reply to Jan BeulichQuoted 24 lines
Quoted 16 lines
Quoted 1 line
Quoted 1 line · Tim Deegan
Quoted 6 lines · Jan Beulich
Quoted 1 line
Quoted 1 line
Quoted 1 line
On 21.03.12 at 20:22, Andres Lagar-Cavilla
Quoted 4 lines · <[email protected]> wrote:
xen/include/asm-x86/page.h| 12 ++++++++----1 files changed, 8 insertions(+), 4 deletions(-)Quoted 7 lines
Quoted 1 line
Otherwise,
INVALID_MFNtramples over high order bits used foradditional
Quoted 1 line
flags.
But is passing
INVALID_PFNinto these macros valid/sensible in the first
place?Quoted 7 lines
The p2m code uses pte layout even for entries that don't have the
_PAGE_PRESENTbit set. We can:
- mask out in these macros, making everything safe;
- make new macros just for p2m code;
- rewrite p2m callers not to useINVALID_MFN; or
- have the p2m code explicitly replaceINVALID_MFNwith some other value when callers specify it.
Quoted 2 lines
As the transformation backwards doesn't yield
INVALID_MFNanyway, I'd prefer one of those options.
The whole issue originates in p2m-pt, which uses l1e_from_pfn with INVALID_PFN -- sometimes. My favourite option would be to replace that with p2m_l1e_from_pfn, which does the masking of the pfn and then calls l1e_from_pfn.
Limits the masking to the only places where it is relevant. If that's ok with you I'll prepare and resend.
Andres
Quoted 13 lines
Quoted 3 lines
I'm happy to do it this way but I guess maybe it slows down paths that don't need the mask.
Yes, that was one of the concerns - less because of the slowdown, but more because the generated code then becomes even worse to look at (which at least I happen to be required to do now and then, albeit admittedly less frequently over time, i.e. as the general code quality improves).
Jan
Re: [PATCH 2 of 3] Clip mfn to allowable width when building a PTE
Quoted 28 lines
Quoted 25 lines
Quoted 17 lines · Andres Lagar-Cavilla
Quoted 1 line
Quoted 1 line · Tim Deegan
Quoted 6 lines · Jan Beulich
Quoted 1 line
Quoted 1 line
Quoted 1 line
On 21.03.12 at 20:22, Andres Lagar-Cavilla
Quoted 4 lines · <[email protected]> wrote:
xen/include/asm-x86/page.h| 12 ++++++++----1 files changed, 8 insertions(+), 4 deletions(-)Quoted 7 lines
Quoted 1 line
Otherwise,
INVALID_MFNtramples over high order bits used foradditional
Quoted 1 line
flags.
But is passing
INVALID_PFNinto these macros valid/sensible in the first
place?Quoted 7 lines
The p2m code uses pte layout even for entries that don't have the
_PAGE_PRESENTbit set. We can:
- mask out in these macros, making everything safe;
- make new macros just for p2m code;
- rewrite p2m callers not to useINVALID_MFN; or
- have the p2m code explicitly replaceINVALID_MFNwith some other value when callers specify it.Quoted 2 lines
As the transformation backwards doesn't yield
INVALID_MFNanyway, I'd prefer one of those options.
Quoted 7 lines
The whole issue originates in p2m-pt, which uses
l1e_from_pfnwithINVALID_PFN-- sometimes. My favourite option would be to replace that withp2m_l1e_from_pfn, which does the masking of the pfn and then callsl1e_from_pfn.Limits the masking to the only places where it is relevant. If that's ok with you I'll prepare and resend.
Yes, that sounds good to me.
Jan
Re: [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables
In reply to Tim DeeganQuoted 10 lines
Quoted 6 lines · Jan Beulich
xen/drivers/passthrough/iommu.c DEFINE_PER_CPU(bool_t, iommu_dont_flush_ · line 72
Quoted 3 lines
if ( !parse_bool(s) )xen/drivers/passthrough/iommu.c +1
-1- iommu_enabled =0 ; + iommu_enabled = val;... this one must remain unchanged, and the handling of the "no-" prefix should come afterwards.
Meh. I reserve the right to set 'iommu=no-yes'. :) That is to say, I don't think it makes any real difference.
Well, it'd be great if one version or the other goes in before 4.2 closes :) Thanks
Andres
Quoted 50 lines
Tim.
Quoted 46 lines
Quoted 28 lines
else if ( !strcmp(s, "force") || !strcmp(s, "required") )xen/drivers/passthrough/iommu.c +10
-11- force_iommu =1 ; + force_iommu = val; else if ( !strcmp(s, "workaround_bios_bug") ) iommu_workaround_bios_bug =1 ; else if ( !strcmp(s, "verbose") ) - iommu_verbose =1 ; - else if ( !strcmp(s, "no-snoop") ) - iommu_snoop =0 ; - else if ( !strcmp(s, "no-qinval") ) - iommu_qinval =0 ; - else if ( !strcmp(s, "no-intremap") ) - iommu_intremap =0 ; + iommu_verbose = val; + else if ( !strcmp(s, "snoop") ) + iommu_snoop = val; + else if ( !strcmp(s, "qinval") ) + iommu_qinval = val; + else if ( !strcmp(s, "intremap") ) + iommu_intremap = val; else if ( !strcmp(s, "debug") ) - iommu_debug =1 ; + iommu_debug = val; else if ( !strcmp(s, "amd-iommu-perdev-intremap") ) - amd_iommu_perdev_intremap =1 ; + amd_iommu_perdev_intremap = val; else if ( !strcmp(s, "dom0-passthrough") ) - iommu_passthrough =1 ;xen/drivers/passthrough/iommu.c DEFINE_PER_CPU(bool_t, iommu_dont_flush_ · line 97
Quoted 10 lines
xen/drivers/passthrough/iommu.c +3
-2+ iommu_passthrough = val; else if ( !strcmp(s, "dom0-strict") ) - iommu_dom0_strict =1 ; + iommu_dom0_strict = val; else if ( !strcmp(s, "sharept") ) - iommu_hap_pt_share =1 ; + iommu_hap_pt_share = val; s = ss +1 ; }while ( ss );_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel
Re: [PATCH 2 of 3] Clip mfn to allowable width when building a PTE
In reply to Jan BeulichQuoted 34 lines
Quoted 34 lines
Quoted 26 lines
Quoted 18 lines
Quoted 1 line · Andres Lagar-Cavilla
Quoted 1 line · Tim Deegan
Quoted 6 lines · Jan Beulich
Quoted 1 line
Quoted 1 line
Quoted 1 line
On 21.03.12 at 20:22, Andres Lagar-Cavilla
Quoted 4 lines · <[email protected]> wrote:
xen/include/asm-x86/page.h| 12 ++++++++----1 files changed, 8 insertions(+), 4 deletions(-)Quoted 7 lines
Quoted 1 line
Otherwise,
INVALID_MFNtramples over high order bits used foradditional
Quoted 1 line
flags.
But is passing
INVALID_PFNinto these macros valid/sensible in the first
place?Quoted 7 lines
The p2m code uses pte layout even for entries that don't have the
_PAGE_PRESENTbit set. We can:
- mask out in these macros, making everything safe;
- make new macros just for p2m code;
- rewrite p2m callers not to useINVALID_MFN; or
- have the p2m code explicitly replaceINVALID_MFNwith some other value when callers specify it.Quoted 2 lines
As the transformation backwards doesn't yield
INVALID_MFNanyway, I'd prefer one of those options.The whole issue originates in p2m-pt, which uses
l1e_from_pfnwithINVALID_PFN-- sometimes. My favourite option would be to replace that withp2m_l1e_from_pfn, which does the masking of the pfn and then callsl1e_from_pfn.
Quoted 4 lines
Quoted 2 lines
Limits the masking to the only places where it is relevant. If that's ok with you I'll prepare and resend.
Yes, that sounds good to me.
Posted here
http://lists.xen.org/archives/html/xen-devel/2012-03/msg01982.html
(just making sure it doesn't slip through the cracks) Andres
Quoted 4 lines
Jan
Re: [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables
In reply to Andres Lagar-CavillaQuoted 5 lines · Andres Lagar-Cavilla
Quoted 5 lines
Quoted 4 lines · Jan Beulich
Quoted 3 lines
if ( !parse_bool(s) )(quoted diff) +1
-1- iommu_enabled = 0; + iommu_enabled = val;
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
... this one must remain unchanged, and the handling of the "no-" prefix should come afterwards.
Meh. I reserve the right to set 'iommu=no-yes'. :) That is to say, I don't think it makes any real difference.
Well, it'd be great if one version or the other goes in before 4.2 closes :)
OK, here's a version that keeps the 'iommu_enabled = 0' (so we never turn it _on_ in this function). Jan, does that address your concern?
Any IOMMU maintainers have an opinion about this patch?
Keir, I think you're maintainer for this file - can I get an Ack?
Cheers,
Tim.
# HG changeset patch
# User Tim Deegan <[email protected]>
# Date 1333016609 -3600
# Node ID 67418ef0095b93b0084c2fae5fd87bdae714a69e
# Parent 74d2af0b56ea7e6072bdfd6e493be5f108808bb7
IOMMU: clean up handling of 'foo' and 'no-foo' command-line options
xen/drivers/passthrough/iommu.c +18 -13
@@ -57,8 +57,13 @@ DEFINE_PER_CPU(bool_t, iommu_dont_flush_static void __init parse_iommu_param (char *s) {char *ss; +int val; do { + val = !!strncmp(s, "no-", 3); + if ( !val ) + s += 3; + ss = strchr(s, ','); if ( ss ) *ss = '\0';
@@ -66,27 +71,27 @@ static void __init parse_iommu_param(chaif ( !parse_bool (s) ) iommu_enabled =0 ;else if ( !strcmp (s,"force" ) || !strcmp (s,"required" ) ) - force_iommu =1 ; + force_iommu = val;else if ( !strcmp (s,"workaround_bios_bug" ) ) iommu_workaround_bios_bug =1 ;else if ( !strcmp (s,"verbose" ) ) - iommu_verbose =1 ; -else if ( !strcmp (s,"no-snoop" ) ) - iommu_snoop =0 ; -else if ( !strcmp (s,"no-qinval" ) ) - iommu_qinval =0 ; -else if ( !strcmp (s,"no-intremap" ) ) - iommu_intremap =0 ; + iommu_verbose = val; +else if ( !strcmp (s,"snoop" ) ) + iommu_snoop = val; +else if ( !strcmp (s,"qinval" ) ) + iommu_qinval = val; +else if ( !strcmp (s,"intremap" ) ) + iommu_intremap = val;else if ( !strcmp (s,"debug" ) ) - iommu_debug =1 ; + iommu_debug = val;else if ( !strcmp (s,"amd-iommu-perdev-intremap" ) ) - amd_iommu_perdev_intremap =1 ; + amd_iommu_perdev_intremap = val;else if ( !strcmp (s,"dom0-passthrough" ) ) - iommu_passthrough =1 ; + iommu_passthrough = val;else if ( !strcmp (s,"dom0-strict" ) ) - iommu_dom0_strict =1 ; + iommu_dom0_strict = val;else if ( !strcmp (s,"sharept" ) ) - iommu_hap_pt_share =1 ; + iommu_hap_pt_share = val; s = ss +1 ; }while ( ss );
Keir Fraser
Acked-byRe: [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables
Quoted 12 lines · Tim Deegan
Quoted 11 lines · Andres Lagar-Cavilla
Quoted 10 lines
Quoted 6 lines · Jan Beulich
Review
Quoted 3 lines
if ( !parse_bool(s) )(quoted diff) +1
-1- iommu_enabled = 0; + iommu_enabled = val;... this one must remain unchanged, and the handling of the "no-" prefix should come afterwards.
Meh. I reserve the right to set 'iommu=no-yes'. :) That is to say, I don't think it makes any real difference.
Quoted 8 lines
Quoted 1 line
Well, it'd be great if one version or the other goes in before 4.2 closes :)
OK, here's a version that keeps the '
iommu_enabled= 0' (so we never turn it _on_ in this function). Jan, does that address your concern?Any IOMMU maintainers have an opinion about this patch?
Keir, I think you're maintainer for this file - can I get an Ack?
Why does workaround_bios_bug not respect val like all the other options? Apart from that:
Quoted 73 lines
Cheers,
Tim.
# HG changeset patch
# User Tim Deegan <[email protected]>
# Date 1333016609 -3600
# Node ID67418ef0095b93b0084c2fae5fd87bdae714a69e
# Parent74d2af0b56ea7e6072bdfd6e493be5f108808bb7
IOMMU: clean up handling of 'foo' and 'no-foo' command-line optionsSigned-off-by Tim Deegan <[email protected]>diff -r
74d2af0b56ea-r67418ef0095bxen/drivers/passthrough/iommu.cxen/drivers/passthrough/iommu.c Thu Mar 29 11:23:29 2012 +0100 renamed from xen/drivers/passthrough/iommu.c Wed Mar 28 16:59:02 2012 +0200 +18
-13@@ -57,8 +57,13 @@ DEFINE_PER_CPU(bool_t, iommu_dont_flush_ static void __init parse_iommu_param(char *s) { char *ss; + int val; do { + val = !!strncmp(s, "no-", 3); + if ( !val ) + s += 3; + ss = strchr(s, ','); if ( ss ) *ss = '\0';@@ -66,27 +71,27 @@ static void __init parse_iommu_param(cha if ( !parse_bool(s) ) iommu_enabled = 0; else if ( !strcmp(s, "force") || !strcmp(s, "required") ) - force_iommu = 1; + force_iommu = val; else if ( !strcmp(s, "workaround_bios_bug") ) iommu_workaround_bios_bug = 1; else if ( !strcmp(s, "verbose") ) - iommu_verbose = 1; - else if ( !strcmp(s, "no-snoop") ) - iommu_snoop = 0; - else if ( !strcmp(s, "no-qinval") ) - iommu_qinval = 0; - else if ( !strcmp(s, "no-intremap") ) - iommu_intremap = 0; + iommu_verbose = val; + else if ( !strcmp(s, "snoop") ) + iommu_snoop = val; + else if ( !strcmp(s, "qinval") ) + iommu_qinval = val; + else if ( !strcmp(s, "intremap") ) + iommu_intremap = val; else if ( !strcmp(s, "debug") ) - iommu_debug = 1; + iommu_debug = val; else if ( !strcmp(s, "amd-iommu-perdev-intremap") ) - amd_iommu_perdev_intremap = 1; + amd_iommu_perdev_intremap = val; else if ( !strcmp(s, "dom0-passthrough") ) - iommu_passthrough = 1; + iommu_passthrough = val; else if ( !strcmp(s, "dom0-strict") ) - iommu_dom0_strict = 1; + iommu_dom0_strict = val; else if ( !strcmp(s, "sharept") ) - iommu_hap_pt_share = 1; + iommu_hap_pt_share = val; s = ss + 1; } while ( ss );
Re: [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables
Quoted 16 lines · Keir Fraser
Quoted 14 lines · Tim Deegan
Quoted 12 lines · Andres Lagar-Cavilla
Quoted 10 lines
Quoted 6 lines · Jan Beulich
Review
Quoted 3 lines
if ( !parse_bool(s) )(quoted diff) +1
-1- iommu_enabled = 0; + iommu_enabled = val;... this one must remain unchanged, and the handling of the "no-" prefix should come afterwards.
Meh. I reserve the right to set 'iommu=no-yes'. :) That is to say, I don't think it makes any real difference.
Well, it'd be great if one version or the other goes in before 4.2 closes :)
Quoted 8 lines
Quoted 6 lines
OK, here's a version that keeps the '
iommu_enabled= 0' (so we never turn it _on_ in this function). Jan, does that address your concern?Any IOMMU maintainers have an opinion about this patch?
Keir, I think you're maintainer for this file - can I get an Ack?
Why does
workaround_bios_bugnot respect val like all the other options?
Oversight. I've changed it to '= val' too.
Quoted 2 lines
Apart from that:
Acked-by Keir Fraser <[email protected]>
Ta. Applied, with that one change.
Tim.
Re: [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables
In reply to Tim DeeganQuoted 3 lines
Quoted 1 line
Quoted 1 line
Tim Deegan <[email protected]> 03/29/12 12:27 PM >>>
OK, here's a version that keeps the '
iommu_enabled= 0' (so we never turn it _on_ in this function). Jan, does that address your concern?
Yes, thanks.
Jan