[PATCH 0/2] x86: more XSA-154 follow-ups

[PATCH 0/2] x86: more XSA-154 follow-ups

[Jan Beulich]

1: use "unsigned int" for cache attribute values
2: HVM: cache attribute pinning adjustments

Signed-off-by: Jan Beulich <jbeulich@suse.com>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 1/2] x86: use "unsigned int" for cache attribute values

[Jan Beulich]

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

... where suitable. But note that the type of "cacheattr" in
get_page_from_l1e() specifically needs to remain "unsigned long".

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -802,7 +802,7 @@ int is_iomem_page(unsigned long mfn)
     return (page_get_owner(page) == dom_io);
 }
 
-static int update_xen_mappings(unsigned long mfn, unsigned long cacheattr)
+static int update_xen_mappings(unsigned long mfn, unsigned int cacheattr)
 {
     int err = 0;
     bool_t alias = mfn >= PFN_DOWN(xen_phys_start) &&
@@ -2136,7 +2136,7 @@ static int mod_l4_entry(l4_pgentry_t *pl
 
 static int cleanup_page_cacheattr(struct page_info *page)
 {
-    uint32_t cacheattr =
+    unsigned int cacheattr =
         (page->count_info & PGC_cacheattr_mask) >> PGC_cacheattr_base;
 
     if ( likely(cacheattr == 0) )
--- a/xen/include/asm-x86/page.h
+++ b/xen/include/asm-x86/page.h
@@ -348,11 +348,11 @@ void free_xen_pagetable(void *v);
 l1_pgentry_t *virt_to_xen_l1e(unsigned long v);
 
 /* Convert between PAT/PCD/PWT embedded in PTE flags and 3-bit cacheattr. */
-static inline uint32_t pte_flags_to_cacheattr(uint32_t flags)
+static inline unsigned int pte_flags_to_cacheattr(unsigned int flags)
 {
     return ((flags >> 5) & 4) | ((flags >> 3) & 3);
 }
-static inline uint32_t cacheattr_to_pte_flags(uint32_t cacheattr)
+static inline unsigned int cacheattr_to_pte_flags(unsigned int cacheattr)
 {
     return ((cacheattr & 4) << 5) | ((cacheattr & 3) << 3);
 }




[-- Attachment #2: x86-uxm-param-type.patch --]
[-- Type: text/plain, Size: 1599 bytes --]

x86: use "unsigned int" for cache attribute values

... where suitable. But note that the type of "cacheattr" in
get_page_from_l1e() specifically needs to remain "unsigned long".

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -802,7 +802,7 @@ int is_iomem_page(unsigned long mfn)
     return (page_get_owner(page) == dom_io);
 }
 
-static int update_xen_mappings(unsigned long mfn, unsigned long cacheattr)
+static int update_xen_mappings(unsigned long mfn, unsigned int cacheattr)
 {
     int err = 0;
     bool_t alias = mfn >= PFN_DOWN(xen_phys_start) &&
@@ -2136,7 +2136,7 @@ static int mod_l4_entry(l4_pgentry_t *pl
 
 static int cleanup_page_cacheattr(struct page_info *page)
 {
-    uint32_t cacheattr =
+    unsigned int cacheattr =
         (page->count_info & PGC_cacheattr_mask) >> PGC_cacheattr_base;
 
     if ( likely(cacheattr == 0) )
--- a/xen/include/asm-x86/page.h
+++ b/xen/include/asm-x86/page.h
@@ -348,11 +348,11 @@ void free_xen_pagetable(void *v);
 l1_pgentry_t *virt_to_xen_l1e(unsigned long v);
 
 /* Convert between PAT/PCD/PWT embedded in PTE flags and 3-bit cacheattr. */
-static inline uint32_t pte_flags_to_cacheattr(uint32_t flags)
+static inline unsigned int pte_flags_to_cacheattr(unsigned int flags)
 {
     return ((flags >> 5) & 4) | ((flags >> 3) & 3);
 }
-static inline uint32_t cacheattr_to_pte_flags(uint32_t cacheattr)
+static inline unsigned int cacheattr_to_pte_flags(unsigned int cacheattr)
 {
     return ((cacheattr & 4) << 5) | ((cacheattr & 3) << 3);
 }

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 2/2] x86/HVM: cache attribute pinning adjustments

[Jan Beulich]

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

- call hvm_get_mem_pinned_cacheattr() for RAM ranges only (requires
  some re-ordering in epte_get_entry_emt(), to fully handle all MMIO
  aspects first) - it's documented to be intended for RAM only
- remove unnecessary indirection for hvm_get_mem_pinned_cacheattr()'s
  return of the type
- make hvm_set_mem_pinned_cacheattr() return an error on bad domain
  kind or obviously bad GFN range
- also avoid cache flush on EPT when removing a UC- range
- other code structure adjustments without intended functional change

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -521,14 +521,12 @@ struct hvm_mem_pinned_cacheattr_range {
 
 static DEFINE_RCU_READ_LOCK(pinned_cacheattr_rcu_lock);
 
-void hvm_init_cacheattr_region_list(
-    struct domain *d)
+void hvm_init_cacheattr_region_list(struct domain *d)
 {
     INIT_LIST_HEAD(&d->arch.hvm_domain.pinned_cacheattr_ranges);
 }
 
-void hvm_destroy_cacheattr_region_list(
-    struct domain *d)
+void hvm_destroy_cacheattr_region_list(struct domain *d)
 {
     struct list_head *head = &d->arch.hvm_domain.pinned_cacheattr_ranges;
     struct hvm_mem_pinned_cacheattr_range *range;
@@ -543,20 +541,14 @@ void hvm_destroy_cacheattr_region_list(
     }
 }
 
-int hvm_get_mem_pinned_cacheattr(
-    struct domain *d,
-    uint64_t guest_fn,
-    unsigned int order,
-    uint32_t *type)
+int hvm_get_mem_pinned_cacheattr(struct domain *d, uint64_t guest_fn,
+                                 unsigned int order)
 {
     struct hvm_mem_pinned_cacheattr_range *range;
     uint64_t mask = ~(uint64_t)0 << order;
-    int rc = 0;
+    int rc = -ENXIO;
 
-    *type = ~0;
-
-    if ( !is_hvm_domain(d) )
-        return 0;
+    ASSERT(has_hvm_container_domain(d));
 
     rcu_read_lock(&pinned_cacheattr_rcu_lock);
     list_for_each_entry_rcu ( range,
@@ -566,14 +558,13 @@ int hvm_get_mem_pinned_cacheattr(
         if ( ((guest_fn & mask) >= range->start) &&
              ((guest_fn | ~mask) <= range->end) )
         {
-            *type = range->type;
-            rc = 1;
+            rc = range->type;
             break;
         }
         if ( ((guest_fn & mask) <= range->end) &&
              (range->start <= (guest_fn | ~mask)) )
         {
-            rc = -1;
+            rc = -EADDRNOTAVAIL;
             break;
         }
     }
@@ -587,20 +578,21 @@ static void free_pinned_cacheattr_entry(
     xfree(container_of(rcu, struct hvm_mem_pinned_cacheattr_range, rcu));
 }
 
-int32_t hvm_set_mem_pinned_cacheattr(
-    struct domain *d,
-    uint64_t gfn_start,
-    uint64_t gfn_end,
-    uint32_t  type)
+int hvm_set_mem_pinned_cacheattr(struct domain *d, uint64_t gfn_start,
+                                 uint64_t gfn_end, uint32_t type)
 {
     struct hvm_mem_pinned_cacheattr_range *range;
     int rc = 1;
 
-    if ( !is_hvm_domain(d) || gfn_end < gfn_start )
-        return 0;
+    if ( !is_hvm_domain(d) )
+        return -EOPNOTSUPP;
+
+    if ( gfn_end < gfn_start || (gfn_start | gfn_end) >> paddr_bits )
+        return -EINVAL;
 
-    if ( type == XEN_DOMCTL_DELETE_MEM_CACHEATTR )
+    switch ( type )
     {
+    case XEN_DOMCTL_DELETE_MEM_CACHEATTR:
         /* Remove the requested range. */
         rcu_read_lock(&pinned_cacheattr_rcu_lock);
         list_for_each_entry_rcu ( range,
@@ -613,22 +605,37 @@ int32_t hvm_set_mem_pinned_cacheattr(
                 type = range->type;
                 call_rcu(&range->rcu, free_pinned_cacheattr_entry);
                 p2m_memory_type_changed(d);
-                if ( type != PAT_TYPE_UNCACHABLE )
+                switch ( type )
+                {
+                case PAT_TYPE_UC_MINUS:
+                    /*
+                     * For EPT we can also avoid the flush in this case;
+                     * see epte_get_entry_emt().
+                     */
+                    if ( hap_enabled(d) && cpu_has_vmx )
+                case PAT_TYPE_UNCACHABLE:
+                        break;
+                    /* fall through */
+                default:
                     flush_all(FLUSH_CACHE);
+                    break;
+                }
                 return 0;
             }
         rcu_read_unlock(&pinned_cacheattr_rcu_lock);
         return -ENOENT;
-    }
 
-    if ( !((type == PAT_TYPE_UNCACHABLE) ||
-           (type == PAT_TYPE_WRCOMB) ||
-           (type == PAT_TYPE_WRTHROUGH) ||
-           (type == PAT_TYPE_WRPROT) ||
-           (type == PAT_TYPE_WRBACK) ||
-           (type == PAT_TYPE_UC_MINUS)) ||
-         !is_hvm_domain(d) )
+    case PAT_TYPE_UC_MINUS:
+    case PAT_TYPE_UNCACHABLE:
+    case PAT_TYPE_WRBACK:
+    case PAT_TYPE_WRCOMB:
+    case PAT_TYPE_WRPROT:
+    case PAT_TYPE_WRTHROUGH:
+        break;
+
+    default:
         return -EINVAL;
+    }
 
     rcu_read_lock(&pinned_cacheattr_rcu_lock);
     list_for_each_entry_rcu ( range,
@@ -762,7 +769,6 @@ int epte_get_entry_emt(struct domain *d,
                        unsigned int order, uint8_t *ipat, bool_t direct_mmio)
 {
     int gmtrr_mtype, hmtrr_mtype;
-    uint32_t type;
     struct vcpu *v = current;
 
     *ipat = 0;
@@ -782,30 +788,28 @@ int epte_get_entry_emt(struct domain *d,
                                  mfn_x(mfn) + (1UL << order) - 1) )
         return -1;
 
-    switch ( hvm_get_mem_pinned_cacheattr(d, gfn, order, &type) )
+    if ( direct_mmio )
     {
-    case 1:
+        if ( (mfn_x(mfn) ^ d->arch.hvm_domain.vmx.apic_access_mfn) >> order )
+            return MTRR_TYPE_UNCACHABLE;
+        if ( order )
+            return -1;
         *ipat = 1;
-        return type != PAT_TYPE_UC_MINUS ? type : PAT_TYPE_UNCACHABLE;
-    case -1:
-        return -1;
+        return MTRR_TYPE_WRBACK;
     }
 
-    if ( !need_iommu(d) && !cache_flush_permitted(d) )
+    gmtrr_mtype = hvm_get_mem_pinned_cacheattr(d, gfn, order);
+    if ( gmtrr_mtype >= 0 )
     {
-        ASSERT(!direct_mmio ||
-               !((mfn_x(mfn) ^ d->arch.hvm_domain.vmx.apic_access_mfn) >>
-                 order));
         *ipat = 1;
-        return MTRR_TYPE_WRBACK;
+        return gmtrr_mtype != PAT_TYPE_UC_MINUS ? gmtrr_mtype
+                                                : MTRR_TYPE_UNCACHABLE;
     }
+    if ( gmtrr_mtype == -EADDRNOTAVAIL )
+        return -1;
 
-    if ( direct_mmio )
+    if ( !need_iommu(d) && !cache_flush_permitted(d) )
     {
-        if ( (mfn_x(mfn) ^ d->arch.hvm_domain.vmx.apic_access_mfn) >> order )
-            return MTRR_TYPE_UNCACHABLE;
-        if ( order )
-            return -1;
         *ipat = 1;
         return MTRR_TYPE_WRBACK;
     }
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -607,7 +607,7 @@ _sh_propagate(struct vcpu *v,
     if ( (level == 1) && is_hvm_domain(d) &&
          !is_xen_heap_mfn(mfn_x(target_mfn)) )
     {
-        unsigned int type;
+        int type;
 
         ASSERT(!(sflags & (_PAGE_PAT | _PAGE_PCD | _PAGE_PWT)));
 
@@ -618,7 +618,9 @@ _sh_propagate(struct vcpu *v,
          * 3) if disables snoop control, compute the PAT index with
          *    gMTRR and gPAT.
          */
-        if ( hvm_get_mem_pinned_cacheattr(d, gfn_x(target_gfn), 0, &type) )
+        if ( !mmio_mfn &&
+             (type = hvm_get_mem_pinned_cacheattr(d, gfn_x(target_gfn),
+                                                  0)) >= 0 )
             sflags |= pat_type_2_pte_flags(type);
         else if ( d->arch.hvm_domain.is_in_uc_mode )
             sflags |= pat_type_2_pte_flags(PAT_TYPE_UNCACHABLE);
--- a/xen/include/asm-x86/hvm/cacheattr.h
+++ b/xen/include/asm-x86/hvm/cacheattr.h
@@ -1,29 +1,23 @@
 #ifndef __HVM_CACHEATTR_H__
 #define __HVM_CACHEATTR_H__
 
-void hvm_init_cacheattr_region_list(
-    struct domain *d);
-void hvm_destroy_cacheattr_region_list(
-    struct domain *d);
+#include <xen/types.h>
+
+struct domain;
+void hvm_init_cacheattr_region_list(struct domain *d);
+void hvm_destroy_cacheattr_region_list(struct domain *d);
 
 /*
  * To see guest_fn is in the pinned range or not,
- * if yes, return 1, and set type to value in this range
- * if no,  return 0, setting type to ~0
- * if ambiguous, return -1, setting type to ~0 (possible only for order > 0)
+ * if yes, return the (non-negative) type
+ * if no or ambiguous, return a negative error code
  */
-int hvm_get_mem_pinned_cacheattr(
-    struct domain *d,
-    uint64_t guest_fn,
-    unsigned int order,
-    uint32_t *type);
+int hvm_get_mem_pinned_cacheattr(struct domain *d, uint64_t guest_fn,
+                                 unsigned int order);
 
 
 /* Set pinned caching type for a domain. */
-int32_t hvm_set_mem_pinned_cacheattr(
-    struct domain *d,
-    uint64_t gfn_start,
-    uint64_t gfn_end,
-    uint32_t  type);
+int hvm_set_mem_pinned_cacheattr(struct domain *d, uint64_t gfn_start,
+                                 uint64_t gfn_end, uint32_t type);
 
 #endif /* __HVM_CACHEATTR_H__ */



[-- Attachment #2: x86-HVM-mem-pinned-cacheattr.patch --]
[-- Type: text/plain, Size: 9228 bytes --]

x86/HVM: cache attribute pinning adjustments

- call hvm_get_mem_pinned_cacheattr() for RAM ranges only (requires
  some re-ordering in epte_get_entry_emt(), to fully handle all MMIO
  aspects first) - it's documented to be intended for RAM only
- remove unnecessary indirection for hvm_get_mem_pinned_cacheattr()'s
  return of the type
- make hvm_set_mem_pinned_cacheattr() return an error on bad domain
  kind or obviously bad GFN range
- also avoid cache flush on EPT when removing a UC- range
- other code structure adjustments without intended functional change

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -521,14 +521,12 @@ struct hvm_mem_pinned_cacheattr_range {
 
 static DEFINE_RCU_READ_LOCK(pinned_cacheattr_rcu_lock);
 
-void hvm_init_cacheattr_region_list(
-    struct domain *d)
+void hvm_init_cacheattr_region_list(struct domain *d)
 {
     INIT_LIST_HEAD(&d->arch.hvm_domain.pinned_cacheattr_ranges);
 }
 
-void hvm_destroy_cacheattr_region_list(
-    struct domain *d)
+void hvm_destroy_cacheattr_region_list(struct domain *d)
 {
     struct list_head *head = &d->arch.hvm_domain.pinned_cacheattr_ranges;
     struct hvm_mem_pinned_cacheattr_range *range;
@@ -543,20 +541,14 @@ void hvm_destroy_cacheattr_region_list(
     }
 }
 
-int hvm_get_mem_pinned_cacheattr(
-    struct domain *d,
-    uint64_t guest_fn,
-    unsigned int order,
-    uint32_t *type)
+int hvm_get_mem_pinned_cacheattr(struct domain *d, uint64_t guest_fn,
+                                 unsigned int order)
 {
     struct hvm_mem_pinned_cacheattr_range *range;
     uint64_t mask = ~(uint64_t)0 << order;
-    int rc = 0;
+    int rc = -ENXIO;
 
-    *type = ~0;
-
-    if ( !is_hvm_domain(d) )
-        return 0;
+    ASSERT(has_hvm_container_domain(d));
 
     rcu_read_lock(&pinned_cacheattr_rcu_lock);
     list_for_each_entry_rcu ( range,
@@ -566,14 +558,13 @@ int hvm_get_mem_pinned_cacheattr(
         if ( ((guest_fn & mask) >= range->start) &&
              ((guest_fn | ~mask) <= range->end) )
         {
-            *type = range->type;
-            rc = 1;
+            rc = range->type;
             break;
         }
         if ( ((guest_fn & mask) <= range->end) &&
              (range->start <= (guest_fn | ~mask)) )
         {
-            rc = -1;
+            rc = -EADDRNOTAVAIL;
             break;
         }
     }
@@ -587,20 +578,21 @@ static void free_pinned_cacheattr_entry(
     xfree(container_of(rcu, struct hvm_mem_pinned_cacheattr_range, rcu));
 }
 
-int32_t hvm_set_mem_pinned_cacheattr(
-    struct domain *d,
-    uint64_t gfn_start,
-    uint64_t gfn_end,
-    uint32_t  type)
+int hvm_set_mem_pinned_cacheattr(struct domain *d, uint64_t gfn_start,
+                                 uint64_t gfn_end, uint32_t type)
 {
     struct hvm_mem_pinned_cacheattr_range *range;
     int rc = 1;
 
-    if ( !is_hvm_domain(d) || gfn_end < gfn_start )
-        return 0;
+    if ( !is_hvm_domain(d) )
+        return -EOPNOTSUPP;
+
+    if ( gfn_end < gfn_start || (gfn_start | gfn_end) >> paddr_bits )
+        return -EINVAL;
 
-    if ( type == XEN_DOMCTL_DELETE_MEM_CACHEATTR )
+    switch ( type )
     {
+    case XEN_DOMCTL_DELETE_MEM_CACHEATTR:
         /* Remove the requested range. */
         rcu_read_lock(&pinned_cacheattr_rcu_lock);
         list_for_each_entry_rcu ( range,
@@ -613,22 +605,37 @@ int32_t hvm_set_mem_pinned_cacheattr(
                 type = range->type;
                 call_rcu(&range->rcu, free_pinned_cacheattr_entry);
                 p2m_memory_type_changed(d);
-                if ( type != PAT_TYPE_UNCACHABLE )
+                switch ( type )
+                {
+                case PAT_TYPE_UC_MINUS:
+                    /*
+                     * For EPT we can also avoid the flush in this case;
+                     * see epte_get_entry_emt().
+                     */
+                    if ( hap_enabled(d) && cpu_has_vmx )
+                case PAT_TYPE_UNCACHABLE:
+                        break;
+                    /* fall through */
+                default:
                     flush_all(FLUSH_CACHE);
+                    break;
+                }
                 return 0;
             }
         rcu_read_unlock(&pinned_cacheattr_rcu_lock);
         return -ENOENT;
-    }
 
-    if ( !((type == PAT_TYPE_UNCACHABLE) ||
-           (type == PAT_TYPE_WRCOMB) ||
-           (type == PAT_TYPE_WRTHROUGH) ||
-           (type == PAT_TYPE_WRPROT) ||
-           (type == PAT_TYPE_WRBACK) ||
-           (type == PAT_TYPE_UC_MINUS)) ||
-         !is_hvm_domain(d) )
+    case PAT_TYPE_UC_MINUS:
+    case PAT_TYPE_UNCACHABLE:
+    case PAT_TYPE_WRBACK:
+    case PAT_TYPE_WRCOMB:
+    case PAT_TYPE_WRPROT:
+    case PAT_TYPE_WRTHROUGH:
+        break;
+
+    default:
         return -EINVAL;
+    }
 
     rcu_read_lock(&pinned_cacheattr_rcu_lock);
     list_for_each_entry_rcu ( range,
@@ -762,7 +769,6 @@ int epte_get_entry_emt(struct domain *d,
                        unsigned int order, uint8_t *ipat, bool_t direct_mmio)
 {
     int gmtrr_mtype, hmtrr_mtype;
-    uint32_t type;
     struct vcpu *v = current;
 
     *ipat = 0;
@@ -782,30 +788,28 @@ int epte_get_entry_emt(struct domain *d,
                                  mfn_x(mfn) + (1UL << order) - 1) )
         return -1;
 
-    switch ( hvm_get_mem_pinned_cacheattr(d, gfn, order, &type) )
+    if ( direct_mmio )
     {
-    case 1:
+        if ( (mfn_x(mfn) ^ d->arch.hvm_domain.vmx.apic_access_mfn) >> order )
+            return MTRR_TYPE_UNCACHABLE;
+        if ( order )
+            return -1;
         *ipat = 1;
-        return type != PAT_TYPE_UC_MINUS ? type : PAT_TYPE_UNCACHABLE;
-    case -1:
-        return -1;
+        return MTRR_TYPE_WRBACK;
     }
 
-    if ( !need_iommu(d) && !cache_flush_permitted(d) )
+    gmtrr_mtype = hvm_get_mem_pinned_cacheattr(d, gfn, order);
+    if ( gmtrr_mtype >= 0 )
     {
-        ASSERT(!direct_mmio ||
-               !((mfn_x(mfn) ^ d->arch.hvm_domain.vmx.apic_access_mfn) >>
-                 order));
         *ipat = 1;
-        return MTRR_TYPE_WRBACK;
+        return gmtrr_mtype != PAT_TYPE_UC_MINUS ? gmtrr_mtype
+                                                : MTRR_TYPE_UNCACHABLE;
     }
+    if ( gmtrr_mtype == -EADDRNOTAVAIL )
+        return -1;
 
-    if ( direct_mmio )
+    if ( !need_iommu(d) && !cache_flush_permitted(d) )
     {
-        if ( (mfn_x(mfn) ^ d->arch.hvm_domain.vmx.apic_access_mfn) >> order )
-            return MTRR_TYPE_UNCACHABLE;
-        if ( order )
-            return -1;
         *ipat = 1;
         return MTRR_TYPE_WRBACK;
     }
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -607,7 +607,7 @@ _sh_propagate(struct vcpu *v,
     if ( (level == 1) && is_hvm_domain(d) &&
          !is_xen_heap_mfn(mfn_x(target_mfn)) )
     {
-        unsigned int type;
+        int type;
 
         ASSERT(!(sflags & (_PAGE_PAT | _PAGE_PCD | _PAGE_PWT)));
 
@@ -618,7 +618,9 @@ _sh_propagate(struct vcpu *v,
          * 3) if disables snoop control, compute the PAT index with
          *    gMTRR and gPAT.
          */
-        if ( hvm_get_mem_pinned_cacheattr(d, gfn_x(target_gfn), 0, &type) )
+        if ( !mmio_mfn &&
+             (type = hvm_get_mem_pinned_cacheattr(d, gfn_x(target_gfn),
+                                                  0)) >= 0 )
             sflags |= pat_type_2_pte_flags(type);
         else if ( d->arch.hvm_domain.is_in_uc_mode )
             sflags |= pat_type_2_pte_flags(PAT_TYPE_UNCACHABLE);
--- a/xen/include/asm-x86/hvm/cacheattr.h
+++ b/xen/include/asm-x86/hvm/cacheattr.h
@@ -1,29 +1,23 @@
 #ifndef __HVM_CACHEATTR_H__
 #define __HVM_CACHEATTR_H__
 
-void hvm_init_cacheattr_region_list(
-    struct domain *d);
-void hvm_destroy_cacheattr_region_list(
-    struct domain *d);
+#include <xen/types.h>
+
+struct domain;
+void hvm_init_cacheattr_region_list(struct domain *d);
+void hvm_destroy_cacheattr_region_list(struct domain *d);
 
 /*
  * To see guest_fn is in the pinned range or not,
- * if yes, return 1, and set type to value in this range
- * if no,  return 0, setting type to ~0
- * if ambiguous, return -1, setting type to ~0 (possible only for order > 0)
+ * if yes, return the (non-negative) type
+ * if no or ambiguous, return a negative error code
  */
-int hvm_get_mem_pinned_cacheattr(
-    struct domain *d,
-    uint64_t guest_fn,
-    unsigned int order,
-    uint32_t *type);
+int hvm_get_mem_pinned_cacheattr(struct domain *d, uint64_t guest_fn,
+                                 unsigned int order);
 
 
 /* Set pinned caching type for a domain. */
-int32_t hvm_set_mem_pinned_cacheattr(
-    struct domain *d,
-    uint64_t gfn_start,
-    uint64_t gfn_end,
-    uint32_t  type);
+int hvm_set_mem_pinned_cacheattr(struct domain *d, uint64_t gfn_start,
+                                 uint64_t gfn_end, uint32_t type);
 
 #endif /* __HVM_CACHEATTR_H__ */

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 1/2] x86: use "unsigned int" for cache attribute values

[Andrew Cooper]

On 03/03/16 10:31, Jan Beulich wrote:
> ... where suitable. But note that the type of "cacheattr" in
> get_page_from_l1e() specifically needs to remain "unsigned long".
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 2/2] x86/HVM: cache attribute pinning adjustments

[Andrew Cooper]

On 03/03/16 10:31, Jan Beulich wrote:
> - call hvm_get_mem_pinned_cacheattr() for RAM ranges only (requires
>   some re-ordering in epte_get_entry_emt(), to fully handle all MMIO
>   aspects first) - it's documented to be intended for RAM only
> - remove unnecessary indirection for hvm_get_mem_pinned_cacheattr()'s
>   return of the type
> - make hvm_set_mem_pinned_cacheattr() return an error on bad domain
>   kind or obviously bad GFN range
> - also avoid cache flush on EPT when removing a UC- range
> - other code structure adjustments without intended functional change

Reviewing would be far easier if these code structure changes were in a
different patch.  In particular, half the changes to
epte_get_entry_emt() appear to just be reordering the direct_mmio check
in order to drop an ASSERT(), but I am not quite sure, given the
complexity of the change.

> @@ -587,20 +578,21 @@ static void free_pinned_cacheattr_entry(
>      xfree(container_of(rcu, struct hvm_mem_pinned_cacheattr_range, rcu));
>  }
>  
> -int32_t hvm_set_mem_pinned_cacheattr(
> -    struct domain *d,
> -    uint64_t gfn_start,
> -    uint64_t gfn_end,
> -    uint32_t  type)
> +int hvm_set_mem_pinned_cacheattr(struct domain *d, uint64_t gfn_start,
> +                                 uint64_t gfn_end, uint32_t type)
>  {
>      struct hvm_mem_pinned_cacheattr_range *range;
>      int rc = 1;
>  
> -    if ( !is_hvm_domain(d) || gfn_end < gfn_start )
> -        return 0;
> +    if ( !is_hvm_domain(d) )
> +        return -EOPNOTSUPP;

You introduce an asymmetry between set and get here, both in terms of
the checks (hvm vs hvm_container), and assert vs plain failure.  Why is
this?

I would suggest ASSERT(is_hvm_domain(d)) in both cases.

> --- a/xen/include/asm-x86/hvm/cacheattr.h
> +++ b/xen/include/asm-x86/hvm/cacheattr.h
> @@ -1,29 +1,23 @@
>  #ifndef __HVM_CACHEATTR_H__
>  #define __HVM_CACHEATTR_H__
>  
> -void hvm_init_cacheattr_region_list(
> -    struct domain *d);
> -void hvm_destroy_cacheattr_region_list(
> -    struct domain *d);
> +#include <xen/types.h>
> +
> +struct domain;
> +void hvm_init_cacheattr_region_list(struct domain *d);
> +void hvm_destroy_cacheattr_region_list(struct domain *d);
>  
>  /*
>   * To see guest_fn is in the pinned range or not,
> - * if yes, return 1, and set type to value in this range
> - * if no,  return 0, setting type to ~0
> - * if ambiguous, return -1, setting type to ~0 (possible only for order > 0)
> + * if yes, return the (non-negative) type
> + * if no or ambiguous, return a negative error code
>   */
> -int hvm_get_mem_pinned_cacheattr(
> -    struct domain *d,
> -    uint64_t guest_fn,
> -    unsigned int order,
> -    uint32_t *type);
> +int hvm_get_mem_pinned_cacheattr(struct domain *d, uint64_t guest_fn,
> +                                 unsigned int order);

fn, being the usual abbreviation for function, makes this confusing to
read.  As it is already changing, could we change the parameter name to
gfn or guest_frame to be more consistent?  (Perhaps even start using
gfn_t if you are feeing keen).

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 2/2] x86/HVM: cache attribute pinning adjustments

[Wei Liu]

On Thu, Mar 03, 2016 at 11:03:43AM +0000, Andrew Cooper wrote:
[...]
> > @@ -587,20 +578,21 @@ static void free_pinned_cacheattr_entry(
> >      xfree(container_of(rcu, struct hvm_mem_pinned_cacheattr_range, rcu));
> >  }
> >  
> > -int32_t hvm_set_mem_pinned_cacheattr(
> > -    struct domain *d,
> > -    uint64_t gfn_start,
> > -    uint64_t gfn_end,
> > -    uint32_t  type)
> > +int hvm_set_mem_pinned_cacheattr(struct domain *d, uint64_t gfn_start,
> > +                                 uint64_t gfn_end, uint32_t type)
> >  {
> >      struct hvm_mem_pinned_cacheattr_range *range;
> >      int rc = 1;
> >  
> > -    if ( !is_hvm_domain(d) || gfn_end < gfn_start )
> > -        return 0;
> > +    if ( !is_hvm_domain(d) )
> > +        return -EOPNOTSUPP;
> 
> You introduce an asymmetry between set and get here, both in terms of
> the checks (hvm vs hvm_container), and assert vs plain failure.  Why is
> this?
> 
> I would suggest ASSERT(is_hvm_domain(d)) in both cases.
> 

I don't think we can have ASSERT() in the set function because it might
be called by untrusted entity. On the other hand, the get function can
only be used by hypervisor so the ASSERT should be fine.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 2/2] x86/HVM: cache attribute pinning adjustments

[Andrew Cooper]

On 03/03/16 12:10, Wei Liu wrote:
> On Thu, Mar 03, 2016 at 11:03:43AM +0000, Andrew Cooper wrote:
> [...]
>>> @@ -587,20 +578,21 @@ static void free_pinned_cacheattr_entry(
>>>      xfree(container_of(rcu, struct hvm_mem_pinned_cacheattr_range, rcu));
>>>  }
>>>  
>>> -int32_t hvm_set_mem_pinned_cacheattr(
>>> -    struct domain *d,
>>> -    uint64_t gfn_start,
>>> -    uint64_t gfn_end,
>>> -    uint32_t  type)
>>> +int hvm_set_mem_pinned_cacheattr(struct domain *d, uint64_t gfn_start,
>>> +                                 uint64_t gfn_end, uint32_t type)
>>>  {
>>>      struct hvm_mem_pinned_cacheattr_range *range;
>>>      int rc = 1;
>>>  
>>> -    if ( !is_hvm_domain(d) || gfn_end < gfn_start )
>>> -        return 0;
>>> +    if ( !is_hvm_domain(d) )
>>> +        return -EOPNOTSUPP;
>> You introduce an asymmetry between set and get here, both in terms of
>> the checks (hvm vs hvm_container), and assert vs plain failure.  Why is
>> this?
>>
>> I would suggest ASSERT(is_hvm_domain(d)) in both cases.
>>
> I don't think we can have ASSERT() in the set function because it might
> be called by untrusted entity. On the other hand, the get function can
> only be used by hypervisor so the ASSERT should be fine.

The hypercall handler should sanitise the untrusted caller before we get
into this function.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 2/2] x86/HVM: cache attribute pinning adjustments

[Wei Liu]

On Thu, Mar 03, 2016 at 12:12:40PM +0000, Andrew Cooper wrote:
> On 03/03/16 12:10, Wei Liu wrote:
> > On Thu, Mar 03, 2016 at 11:03:43AM +0000, Andrew Cooper wrote:
> > [...]
> >>> @@ -587,20 +578,21 @@ static void free_pinned_cacheattr_entry(
> >>>      xfree(container_of(rcu, struct hvm_mem_pinned_cacheattr_range, rcu));
> >>>  }
> >>>  
> >>> -int32_t hvm_set_mem_pinned_cacheattr(
> >>> -    struct domain *d,
> >>> -    uint64_t gfn_start,
> >>> -    uint64_t gfn_end,
> >>> -    uint32_t  type)
> >>> +int hvm_set_mem_pinned_cacheattr(struct domain *d, uint64_t gfn_start,
> >>> +                                 uint64_t gfn_end, uint32_t type)
> >>>  {
> >>>      struct hvm_mem_pinned_cacheattr_range *range;
> >>>      int rc = 1;
> >>>  
> >>> -    if ( !is_hvm_domain(d) || gfn_end < gfn_start )
> >>> -        return 0;
> >>> +    if ( !is_hvm_domain(d) )
> >>> +        return -EOPNOTSUPP;
> >> You introduce an asymmetry between set and get here, both in terms of
> >> the checks (hvm vs hvm_container), and assert vs plain failure.  Why is
> >> this?
> >>
> >> I would suggest ASSERT(is_hvm_domain(d)) in both cases.
> >>
> > I don't think we can have ASSERT() in the set function because it might
> > be called by untrusted entity. On the other hand, the get function can
> > only be used by hypervisor so the ASSERT should be fine.
> 
> The hypercall handler should sanitise the untrusted caller before we get
> into this function.
> 

I don't disagree. It is just that this seems undone at the moment -- I
don't see xsm hook in the callsite of this function in domctl.

Wei.

> ~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 2/2] x86/HVM: cache attribute pinning adjustments

[Jan Beulich]

>>> On 03.03.16 at 12:03, <andrew.cooper3@citrix.com> wrote:
> On 03/03/16 10:31, Jan Beulich wrote:
>> - call hvm_get_mem_pinned_cacheattr() for RAM ranges only (requires
>>   some re-ordering in epte_get_entry_emt(), to fully handle all MMIO
>>   aspects first) - it's documented to be intended for RAM only
>> - remove unnecessary indirection for hvm_get_mem_pinned_cacheattr()'s
>>   return of the type
>> - make hvm_set_mem_pinned_cacheattr() return an error on bad domain
>>   kind or obviously bad GFN range
>> - also avoid cache flush on EPT when removing a UC- range
>> - other code structure adjustments without intended functional change
> 
> Reviewing would be far easier if these code structure changes were in a
> different patch.  In particular, half the changes to
> epte_get_entry_emt() appear to just be reordering the direct_mmio check
> in order to drop an ASSERT(), but I am not quite sure, given the
> complexity of the change.

I'll see about splitting them, albeit that's not going to help the
hard to read change to epte_get_entry_emt() (in which removal
of the ASSERT() isn't the goal, but a nice side effect).

>> @@ -587,20 +578,21 @@ static void free_pinned_cacheattr_entry(
>>      xfree(container_of(rcu, struct hvm_mem_pinned_cacheattr_range, rcu));
>>  }
>>  
>> -int32_t hvm_set_mem_pinned_cacheattr(
>> -    struct domain *d,
>> -    uint64_t gfn_start,
>> -    uint64_t gfn_end,
>> -    uint32_t  type)
>> +int hvm_set_mem_pinned_cacheattr(struct domain *d, uint64_t gfn_start,
>> +                                 uint64_t gfn_end, uint32_t type)
>>  {
>>      struct hvm_mem_pinned_cacheattr_range *range;
>>      int rc = 1;
>>  
>> -    if ( !is_hvm_domain(d) || gfn_end < gfn_start )
>> -        return 0;
>> +    if ( !is_hvm_domain(d) )
>> +        return -EOPNOTSUPP;
> 
> You introduce an asymmetry between set and get here, both in terms of
> the checks (hvm vs hvm_container), and assert vs plain failure.  Why is
> this?
> 
> I would suggest ASSERT(is_hvm_domain(d)) in both cases.

No, in the "set" case we mustn't assert, or else the caller needs
to change too. And switching to the container variant in "get"
and converting to an assert at the same time is intended too: No
caller calls this for a PV domain, and at least ept_get_entry_emt()
may call this for a PVH one (just that currently "set" won't allow
such ranges to be introduced for them - since I don't know the
reason for this, I didn't want to change the behavior in this regard).

>> --- a/xen/include/asm-x86/hvm/cacheattr.h
>> +++ b/xen/include/asm-x86/hvm/cacheattr.h
>> @@ -1,29 +1,23 @@
>>  #ifndef __HVM_CACHEATTR_H__
>>  #define __HVM_CACHEATTR_H__
>>  
>> -void hvm_init_cacheattr_region_list(
>> -    struct domain *d);
>> -void hvm_destroy_cacheattr_region_list(
>> -    struct domain *d);
>> +#include <xen/types.h>
>> +
>> +struct domain;
>> +void hvm_init_cacheattr_region_list(struct domain *d);
>> +void hvm_destroy_cacheattr_region_list(struct domain *d);
>>  
>>  /*
>>   * To see guest_fn is in the pinned range or not,
>> - * if yes, return 1, and set type to value in this range
>> - * if no,  return 0, setting type to ~0
>> - * if ambiguous, return -1, setting type to ~0 (possible only for order > 0)
>> + * if yes, return the (non-negative) type
>> + * if no or ambiguous, return a negative error code
>>   */
>> -int hvm_get_mem_pinned_cacheattr(
>> -    struct domain *d,
>> -    uint64_t guest_fn,
>> -    unsigned int order,
>> -    uint32_t *type);
>> +int hvm_get_mem_pinned_cacheattr(struct domain *d, uint64_t guest_fn,
>> +                                 unsigned int order);
> 
> fn, being the usual abbreviation for function, makes this confusing to
> read.  As it is already changing, could we change the parameter name to
> gfn or guest_frame to be more consistent?  (Perhaps even start using
> gfn_t if you are feeing keen).

Well, yes, but that will mean yet more difficult to review a patch
(and to be honest with things like this I don't really feel like splitting
things up into too fine grained pieces).

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 2/2] x86/HVM: cache attribute pinning adjustments

[Jan Beulich]

>>> On 03.03.16 at 13:12, <andrew.cooper3@citrix.com> wrote:
> On 03/03/16 12:10, Wei Liu wrote:
>> On Thu, Mar 03, 2016 at 11:03:43AM +0000, Andrew Cooper wrote:
>> [...]
>>>> @@ -587,20 +578,21 @@ static void free_pinned_cacheattr_entry(
>>>>      xfree(container_of(rcu, struct hvm_mem_pinned_cacheattr_range, rcu));
>>>>  }
>>>>  
>>>> -int32_t hvm_set_mem_pinned_cacheattr(
>>>> -    struct domain *d,
>>>> -    uint64_t gfn_start,
>>>> -    uint64_t gfn_end,
>>>> -    uint32_t  type)
>>>> +int hvm_set_mem_pinned_cacheattr(struct domain *d, uint64_t gfn_start,
>>>> +                                 uint64_t gfn_end, uint32_t type)
>>>>  {
>>>>      struct hvm_mem_pinned_cacheattr_range *range;
>>>>      int rc = 1;
>>>>  
>>>> -    if ( !is_hvm_domain(d) || gfn_end < gfn_start )
>>>> -        return 0;
>>>> +    if ( !is_hvm_domain(d) )
>>>> +        return -EOPNOTSUPP;
>>> You introduce an asymmetry between set and get here, both in terms of
>>> the checks (hvm vs hvm_container), and assert vs plain failure.  Why is
>>> this?
>>>
>>> I would suggest ASSERT(is_hvm_domain(d)) in both cases.
>>>
>> I don't think we can have ASSERT() in the set function because it might
>> be called by untrusted entity. On the other hand, the get function can
>> only be used by hypervisor so the ASSERT should be fine.
> 
> The hypercall handler should sanitise the untrusted caller before we get
> into this function.

I don't think this would be a good idea: Once we extend this to
also allow such ranges for PVH, keeping the change in policy to
the source file / function actually carrying this out seems quite
a bit more logical.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel