Re: [Xen-devel] [PATCH v3 1/3] x86/p2m-pt: simplify p2m_next_level()

[George Dunlap]

On 08/11/2017 02:19 PM, Jan Beulich wrote:
> Calculate entry PFN and flags just once. Convert the two successive
> main if()-s to and if/else-if chain. Restrict variable scope where
> reasonable. Take the opportunity and also make the induction variable
> unsigned.
> 
> This at once fixes excessive permissions granted in the 2M PTEs
> resulting from splitting a 1G one - original permissions should be
> inherited instead. This is not a security issue only because all of
> this takes no effect anyway, as iommu_hap_pt_share is always false on
> AMD systems for all supported branches.
> 
> Signed-off-by: Jan Beulich 

Acked-by: George Dunlap 

Sorry for the delay.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH v3 1/3] x86/p2m-pt: simplify p2m_next_level()

[Jan Beulich]

Calculate entry PFN and flags just once. Convert the two successive
main if()-s to and if/else-if chain. Restrict variable scope where
reasonable. Take the opportunity and also make the induction variable
unsigned.

This at once fixes excessive permissions granted in the 2M PTEs
resulting from splitting a 1G one - original permissions should be
inherited instead. This is not a security issue only because all of
this takes no effect anyway, as iommu_hap_pt_share is always false on
AMD systems for all supported branches.

Signed-off-by: Jan Beulich 
---
v3: Fix IOMMU permission handling for shattered PTEs.
v2: Re-do mostly from scratch following review feedback.

--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -191,18 +191,18 @@ p2m_next_level(struct p2m_domain *p2m, v
unsigned long *gfn_remainder, unsigned long gfn, u32 shift,
u32 max, unsigned long type, bool_t unmap)
 {
-l1_pgentry_t *l1_entry;
-l1_pgentry_t *p2m_entry;
-l1_pgentry_t new_entry;
+l1_pgentry_t *p2m_entry, new_entry;
 void *next;
-int i;
+unsigned int flags;
 
 if ( !(p2m_entry = p2m_find_entry(*table, gfn_remainder, gfn,
   shift, max)) )
 return -ENOENT;
 
+flags = l1e_get_flags(*p2m_entry);
+
 /* PoD/paging: Not present doesn't imply empty. */
-if ( !l1e_get_flags(*p2m_entry) )
+if ( !flags )
 {
 struct page_info *pg;
 
@@ -231,70 +231,67 @@ p2m_next_level(struct p2m_domain *p2m, v
 break;
 }
 }
-
-ASSERT(l1e_get_flags(*p2m_entry) & (_PAGE_PRESENT|_PAGE_PSE));
-
-/* split 1GB pages into 2MB pages */
-if ( type == PGT_l2_page_table && (l1e_get_flags(*p2m_entry) & _PAGE_PSE) )
+else if ( flags & _PAGE_PSE )
 {
-unsigned long flags, pfn;
+/* Split superpages pages into smaller ones. */
+unsigned long pfn = l1e_get_pfn(*p2m_entry);
 struct page_info *pg;
+l1_pgentry_t *l1_entry;
+unsigned int i, level;
 
-pg = p2m_alloc_ptp(p2m, PGT_l2_page_table);
-if ( pg == NULL )
-return -ENOMEM;
-
-flags = l1e_get_flags(*p2m_entry);
-pfn = l1e_get_pfn(*p2m_entry);
-
-l1_entry = __map_domain_page(pg);
-for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++ )
+switch ( type )
 {
-new_entry = l1e_from_pfn(pfn | (i * L1_PAGETABLE_ENTRIES), flags);
-p2m_add_iommu_flags(_entry, 1, 
IOMMUF_readable|IOMMUF_writable);
-p2m->write_p2m_entry(p2m, gfn, l1_entry + i, new_entry, 2);
-}
-unmap_domain_page(l1_entry);
-new_entry = l1e_from_pfn(mfn_x(page_to_mfn(pg)),
- P2M_BASE_FLAGS | _PAGE_RW); /* disable PSE */
-p2m_add_iommu_flags(_entry, 2, IOMMUF_readable|IOMMUF_writable);
-p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry, 3);
-}
+case PGT_l2_page_table:
+level = 2;
+break;
 
+case PGT_l1_page_table:
+/*
+ * New splintered mappings inherit the flags of the old superpage,
+ * with a little reorganisation for the _PAGE_PSE_PAT bit.
+ */
+if ( pfn & 1 )   /* ==> _PAGE_PSE_PAT was set */
+pfn -= 1;/* Clear it; _PAGE_PSE becomes _PAGE_PAT 
*/
+else
+flags &= ~_PAGE_PSE; /* Clear _PAGE_PSE (== _PAGE_PAT) */
 
-/* split single 2MB large page into 4KB page in P2M table */
-if ( type == PGT_l1_page_table && (l1e_get_flags(*p2m_entry) & _PAGE_PSE) )
-{
-unsigned long flags, pfn;
-struct page_info *pg;
+level = 1;
+break;
+
+default:
+ASSERT_UNREACHABLE();
+return -EINVAL;
+}
 
-pg = p2m_alloc_ptp(p2m, PGT_l1_page_table);
+pg = p2m_alloc_ptp(p2m, type);
 if ( pg == NULL )
 return -ENOMEM;
 
-/* New splintered mappings inherit the flags of the old superpage, 
- * with a little reorganisation for the _PAGE_PSE_PAT bit. */
-flags = l1e_get_flags(*p2m_entry);
-pfn = l1e_get_pfn(*p2m_entry);
-if ( pfn & 1 )   /* ==> _PAGE_PSE_PAT was set */
-pfn -= 1;/* Clear it; _PAGE_PSE becomes _PAGE_PAT */
-else
-flags &= ~_PAGE_PSE; /* Clear _PAGE_PSE (== _PAGE_PAT) */
-
 l1_entry = __map_domain_page(pg);
-for ( i = 0; i < L1_PAGETABLE_ENTRIES; i++ )
+
+/* Inherit original IOMMU permissions, but update Next Level. */
+if ( iommu_hap_pt_share )
 {
-new_entry = l1e_from_pfn(pfn | i, flags);
-p2m_add_iommu_flags(_entry, 0, 0);
-p2m->write_p2m_entry(p2m, gfn, l1_entry + i, new_entry, 1);
+flags &= ~iommu_nlevel_to_flags(~0, 0);
+flags |= iommu_nlevel_to_flags(level - 1, 0);