[PATCH 0/4] x86/EPT: miscellaneous further fixes to EMT determination

[PATCH 0/4] x86/EPT: miscellaneous further fixes to EMT determination

[Jan Beulich]

1: consider page order when checking for APIC MFN
2: refine direct MMIO checking when determining EMT
3: fix pinned cache attribute range checking
4: also force EMT re-evaluation if pinned ranges change

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

[PATCH 1/4] x86/EPT: consider page order when checking for APIC MFN

[Jan Beulich]

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

This was overlooked in 3d90d6e6 ("x86/EPT: split super pages upon
mismatching memory types").

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

--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -816,15 +816,18 @@ int epte_get_entry_emt(struct domain *d,
           !has_arch_pdevs(d)) )
     {
         ASSERT(!direct_mmio ||
-               mfn_x(mfn) == d->arch.hvm_domain.vmx.apic_access_mfn);
+               !((mfn_x(mfn) ^ d->arch.hvm_domain.vmx.apic_access_mfn) >>
+                 order));
         *ipat = 1;
         return MTRR_TYPE_WRBACK;
     }
 
     if ( direct_mmio )
     {
-        if ( mfn_x(mfn) != d->arch.hvm_domain.vmx.apic_access_mfn )
+        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;
     }




[-- Attachment #2: EPT-APIC-access-MFN-order.patch --]
[-- Type: text/plain, Size: 989 bytes --]

x86/EPT: consider page order when checking for APIC MFN

This was overlooked in 3d90d6e6 ("x86/EPT: split super pages upon
mismatching memory types").

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

--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -816,15 +816,18 @@ int epte_get_entry_emt(struct domain *d,
           !has_arch_pdevs(d)) )
     {
         ASSERT(!direct_mmio ||
-               mfn_x(mfn) == d->arch.hvm_domain.vmx.apic_access_mfn);
+               !((mfn_x(mfn) ^ d->arch.hvm_domain.vmx.apic_access_mfn) >>
+                 order));
         *ipat = 1;
         return MTRR_TYPE_WRBACK;
     }
 
     if ( direct_mmio )
     {
-        if ( mfn_x(mfn) != d->arch.hvm_domain.vmx.apic_access_mfn )
+        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;
     }

[-- 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/4] x86/EPT: refine direct MMIO checking when determining EMT

[Jan Beulich]

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

With need_iommu() only ever true when iommu_enabled is also true, and
with the former getting set when a PCI device gets added to a guest,
the checks can be consolidated. The range set check are left in place
just in case raw MMIO or I/O port ranges get passed to a guest.

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

--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -810,10 +810,9 @@ int epte_get_entry_emt(struct domain *d,
         return -1;
     }
 
-    if ( !iommu_enabled ||
-         (rangeset_is_empty(d->iomem_caps) &&
-          rangeset_is_empty(d->arch.ioport_caps) &&
-          !has_arch_pdevs(d)) )
+    if ( !need_iommu(d) &&
+         rangeset_is_empty(d->iomem_caps) &&
+         rangeset_is_empty(d->arch.ioport_caps) )
     {
         ASSERT(!direct_mmio ||
                !((mfn_x(mfn) ^ d->arch.hvm_domain.vmx.apic_access_mfn) >>




[-- Attachment #2: EPT-EMT-IOMMU-dependency.patch --]
[-- Type: text/plain, Size: 957 bytes --]

x86/EPT: refine direct MMIO checking when determining EMT

With need_iommu() only ever true when iommu_enabled is also true, and
with the former getting set when a PCI device gets added to a guest,
the checks can be consolidated. The range set check are left in place
just in case raw MMIO or I/O port ranges get passed to a guest.

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

--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -810,10 +810,9 @@ int epte_get_entry_emt(struct domain *d,
         return -1;
     }
 
-    if ( !iommu_enabled ||
-         (rangeset_is_empty(d->iomem_caps) &&
-          rangeset_is_empty(d->arch.ioport_caps) &&
-          !has_arch_pdevs(d)) )
+    if ( !need_iommu(d) &&
+         rangeset_is_empty(d->iomem_caps) &&
+         rangeset_is_empty(d->arch.ioport_caps) )
     {
         ASSERT(!direct_mmio ||
                !((mfn_x(mfn) ^ d->arch.hvm_domain.vmx.apic_access_mfn) >>

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

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

[PATCH 3/4] x86/EPT: fix pinned cache attribute range checking

[Jan Beulich]

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

This wasn't done properly by 4d66f069 ("x86: fix pinned cache attribute
handling"): The passed in GFN shouldn't be assumed to be order aligned.

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

--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -589,6 +589,7 @@ int hvm_get_mem_pinned_cacheattr(
     uint32_t *type)
 {
     struct hvm_mem_pinned_cacheattr_range *range;
+    uint64_t mask = ~(uint64_t)0 << order;
     int rc = 0;
 
     *type = ~0;
@@ -601,15 +602,15 @@ int hvm_get_mem_pinned_cacheattr(
                               &d->arch.hvm_domain.pinned_cacheattr_ranges,
                               list )
     {
-        if ( (guest_fn >= range->start) &&
-             (guest_fn + (1UL << order) - 1 <= range->end) )
+        if ( ((guest_fn & mask) >= range->start) &&
+             ((guest_fn | ~mask) <= range->end) )
         {
             *type = range->type;
             rc = 1;
             break;
         }
-        if ( (guest_fn <= range->end) &&
-             (range->start <= guest_fn + (1UL << order) - 1) )
+        if ( ((guest_fn & mask) <= range->end) &&
+             (range->start <= (guest_fn | ~mask)) )
         {
             rc = -1;
             break;




[-- Attachment #2: x86-get-pinned-order.patch --]
[-- Type: text/plain, Size: 1298 bytes --]

x86/EPT: fix pinned cache attribute range checking

This wasn't done properly by 4d66f069 ("x86: fix pinned cache attribute
handling"): The passed in GFN shouldn't be assumed to be order aligned.

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

--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -589,6 +589,7 @@ int hvm_get_mem_pinned_cacheattr(
     uint32_t *type)
 {
     struct hvm_mem_pinned_cacheattr_range *range;
+    uint64_t mask = ~(uint64_t)0 << order;
     int rc = 0;
 
     *type = ~0;
@@ -601,15 +602,15 @@ int hvm_get_mem_pinned_cacheattr(
                               &d->arch.hvm_domain.pinned_cacheattr_ranges,
                               list )
     {
-        if ( (guest_fn >= range->start) &&
-             (guest_fn + (1UL << order) - 1 <= range->end) )
+        if ( ((guest_fn & mask) >= range->start) &&
+             ((guest_fn | ~mask) <= range->end) )
         {
             *type = range->type;
             rc = 1;
             break;
         }
-        if ( (guest_fn <= range->end) &&
-             (range->start <= guest_fn + (1UL << order) - 1) )
+        if ( ((guest_fn & mask) <= range->end) &&
+             (range->start <= (guest_fn | ~mask)) )
         {
             rc = -1;
             break;

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

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

[PATCH 4/4] x86/EPT: also force EMT re-evaluation if pinned ranges change

[Jan Beulich]

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

This was inadvertently left out of aa9114ed ("x86/EPT: force
re-evaluation of memory type as necessary"). Note that this
intentionally doesn't use memory_type_changed(): Changes to the pinned
ranges are independent of IOMMU presence, which that function uses to
determine whether to call the underlying p2m_memory_type_changed().

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

--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -650,6 +650,7 @@ int32_t hvm_set_mem_pinned_cacheattr(
                 rcu_read_unlock(&pinned_cacheattr_rcu_lock);
                 list_del_rcu(&range->list);
                 call_rcu(&range->rcu, free_pinned_cacheattr_entry);
+                p2m_memory_type_changed(d);
                 return 0;
             }
         rcu_read_unlock(&pinned_cacheattr_rcu_lock);
@@ -695,6 +696,7 @@ int32_t hvm_set_mem_pinned_cacheattr(
     range->type = type;
 
     list_add_rcu(&range->list, &d->arch.hvm_domain.pinned_cacheattr_ranges);
+    p2m_memory_type_changed(d);
 
     return 0;
 }




[-- Attachment #2: x86-set-pinned-reeval.patch --]
[-- Type: text/plain, Size: 1121 bytes --]

x86/EPT: also force EMT re-evaluation if pinned ranges change

This was inadvertently left out of aa9114ed ("x86/EPT: force
re-evaluation of memory type as necessary"). Note that this
intentionally doesn't use memory_type_changed(): Changes to the pinned
ranges are independent of IOMMU presence, which that function uses to
determine whether to call the underlying p2m_memory_type_changed().

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

--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -650,6 +650,7 @@ int32_t hvm_set_mem_pinned_cacheattr(
                 rcu_read_unlock(&pinned_cacheattr_rcu_lock);
                 list_del_rcu(&range->list);
                 call_rcu(&range->rcu, free_pinned_cacheattr_entry);
+                p2m_memory_type_changed(d);
                 return 0;
             }
         rcu_read_unlock(&pinned_cacheattr_rcu_lock);
@@ -695,6 +696,7 @@ int32_t hvm_set_mem_pinned_cacheattr(
     range->type = type;
 
     list_add_rcu(&range->list, &d->arch.hvm_domain.pinned_cacheattr_ranges);
+    p2m_memory_type_changed(d);
 
     return 0;
 }

[-- 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/4 v2] x86/EPT: refine direct MMIO checking when determining EMT

[Jan Beulich]

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

With need_iommu() only ever true when iommu_enabled is also true, and
with the former getting set when a PCI device gets added to a guest,
the checks can be consolidated. The range set check are left in place
just in case raw MMIO or I/O port ranges get passed to a guest.

At once drop open-coding of cache_flush_permitted().

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Also drop open-coding of cache_flush_permitted().

--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -20,6 +20,7 @@
 #include <public/hvm/e820.h>
 #include <xen/types.h>
 #include <asm/e820.h>
+#include <asm/iocap.h>
 #include <asm/mm.h>
 #include <asm/paging.h>
 #include <asm/p2m.h>
@@ -810,10 +811,7 @@ int epte_get_entry_emt(struct domain *d,
         return -1;
     }
 
-    if ( !iommu_enabled ||
-         (rangeset_is_empty(d->iomem_caps) &&
-          rangeset_is_empty(d->arch.ioport_caps) &&
-          !has_arch_pdevs(d)) )
+    if ( !need_iommu(d) && !cache_flush_permitted(d) )
     {
         ASSERT(!direct_mmio ||
                !((mfn_x(mfn) ^ d->arch.hvm_domain.vmx.apic_access_mfn) >>




[-- Attachment #2: EPT-EMT-IOMMU-dependency.patch --]
[-- Type: text/plain, Size: 1197 bytes --]

x86/EPT: refine direct MMIO checking when determining EMT

With need_iommu() only ever true when iommu_enabled is also true, and
with the former getting set when a PCI device gets added to a guest,
the checks can be consolidated. The range set check are left in place
just in case raw MMIO or I/O port ranges get passed to a guest.

At once drop open-coding of cache_flush_permitted().

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Also drop open-coding of cache_flush_permitted().

--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -20,6 +20,7 @@
 #include <public/hvm/e820.h>
 #include <xen/types.h>
 #include <asm/e820.h>
+#include <asm/iocap.h>
 #include <asm/mm.h>
 #include <asm/paging.h>
 #include <asm/p2m.h>
@@ -810,10 +811,7 @@ int epte_get_entry_emt(struct domain *d,
         return -1;
     }
 
-    if ( !iommu_enabled ||
-         (rangeset_is_empty(d->iomem_caps) &&
-          rangeset_is_empty(d->arch.ioport_caps) &&
-          !has_arch_pdevs(d)) )
+    if ( !need_iommu(d) && !cache_flush_permitted(d) )
     {
         ASSERT(!direct_mmio ||
                !((mfn_x(mfn) ^ d->arch.hvm_domain.vmx.apic_access_mfn) >>

[-- 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/4] x86/EPT: consider page order when checking for APIC MFN

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, April 23, 2014 4:40 PM
> 
> This was overlooked in 3d90d6e6 ("x86/EPT: split super pages upon
> mismatching memory types").
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

> 
> --- a/xen/arch/x86/hvm/mtrr.c
> +++ b/xen/arch/x86/hvm/mtrr.c
> @@ -816,15 +816,18 @@ int epte_get_entry_emt(struct domain *d,
>            !has_arch_pdevs(d)) )
>      {
>          ASSERT(!direct_mmio ||
> -               mfn_x(mfn) ==
> d->arch.hvm_domain.vmx.apic_access_mfn);
> +               !((mfn_x(mfn) ^
> d->arch.hvm_domain.vmx.apic_access_mfn) >>
> +                 order));
>          *ipat = 1;
>          return MTRR_TYPE_WRBACK;
>      }
> 
>      if ( direct_mmio )
>      {
> -        if ( mfn_x(mfn) != d->arch.hvm_domain.vmx.apic_access_mfn )
> +        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;
>      }
> 
> 

Re: [PATCH 2/4] x86/EPT: refine direct MMIO checking when determining EMT

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, April 23, 2014 4:40 PM
> 
> With need_iommu() only ever true when iommu_enabled is also true, and
> with the former getting set when a PCI device gets added to a guest,
> the checks can be consolidated. The range set check are left in place
> just in case raw MMIO or I/O port ranges get passed to a guest.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

> 
> --- a/xen/arch/x86/hvm/mtrr.c
> +++ b/xen/arch/x86/hvm/mtrr.c
> @@ -810,10 +810,9 @@ int epte_get_entry_emt(struct domain *d,
>          return -1;
>      }
> 
> -    if ( !iommu_enabled ||
> -         (rangeset_is_empty(d->iomem_caps) &&
> -          rangeset_is_empty(d->arch.ioport_caps) &&
> -          !has_arch_pdevs(d)) )
> +    if ( !need_iommu(d) &&
> +         rangeset_is_empty(d->iomem_caps) &&
> +         rangeset_is_empty(d->arch.ioport_caps) )
>      {
>          ASSERT(!direct_mmio ||
>                 !((mfn_x(mfn) ^
> d->arch.hvm_domain.vmx.apic_access_mfn) >>
> 
> 

Re: [PATCH 3/4] x86/EPT: fix pinned cache attribute range checking

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, April 23, 2014 4:41 PM
> 
> This wasn't done properly by 4d66f069 ("x86: fix pinned cache attribute
> handling"): The passed in GFN shouldn't be assumed to be order aligned.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

> 
> --- a/xen/arch/x86/hvm/mtrr.c
> +++ b/xen/arch/x86/hvm/mtrr.c
> @@ -589,6 +589,7 @@ int hvm_get_mem_pinned_cacheattr(
>      uint32_t *type)
>  {
>      struct hvm_mem_pinned_cacheattr_range *range;
> +    uint64_t mask = ~(uint64_t)0 << order;
>      int rc = 0;
> 
>      *type = ~0;
> @@ -601,15 +602,15 @@ int hvm_get_mem_pinned_cacheattr(
> 
> &d->arch.hvm_domain.pinned_cacheattr_ranges,
>                                list )
>      {
> -        if ( (guest_fn >= range->start) &&
> -             (guest_fn + (1UL << order) - 1 <= range->end) )
> +        if ( ((guest_fn & mask) >= range->start) &&
> +             ((guest_fn | ~mask) <= range->end) )
>          {
>              *type = range->type;
>              rc = 1;
>              break;
>          }
> -        if ( (guest_fn <= range->end) &&
> -             (range->start <= guest_fn + (1UL << order) - 1) )
> +        if ( ((guest_fn & mask) <= range->end) &&
> +             (range->start <= (guest_fn | ~mask)) )
>          {
>              rc = -1;
>              break;
> 
> 

Re: [PATCH 4/4] x86/EPT: also force EMT re-evaluation if pinned ranges change

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, April 23, 2014 4:42 PM
> 
> This was inadvertently left out of aa9114ed ("x86/EPT: force
> re-evaluation of memory type as necessary"). Note that this
> intentionally doesn't use memory_type_changed(): Changes to the pinned
> ranges are independent of IOMMU presence, which that function uses to
> determine whether to call the underlying p2m_memory_type_changed().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

> 
> --- a/xen/arch/x86/hvm/mtrr.c
> +++ b/xen/arch/x86/hvm/mtrr.c
> @@ -650,6 +650,7 @@ int32_t hvm_set_mem_pinned_cacheattr(
>                  rcu_read_unlock(&pinned_cacheattr_rcu_lock);
>                  list_del_rcu(&range->list);
>                  call_rcu(&range->rcu, free_pinned_cacheattr_entry);
> +                p2m_memory_type_changed(d);
>                  return 0;
>              }
>          rcu_read_unlock(&pinned_cacheattr_rcu_lock);
> @@ -695,6 +696,7 @@ int32_t hvm_set_mem_pinned_cacheattr(
>      range->type = type;
> 
>      list_add_rcu(&range->list,
> &d->arch.hvm_domain.pinned_cacheattr_ranges);
> +    p2m_memory_type_changed(d);
> 
>      return 0;
>  }
> 
> 

Re: [PATCH 2/4 v2] x86/EPT: refine direct MMIO checking when determining EMT

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Friday, April 25, 2014 8:06 PM
> 
> With need_iommu() only ever true when iommu_enabled is also true, and
> with the former getting set when a PCI device gets added to a guest,
> the checks can be consolidated. The range set check are left in place
> just in case raw MMIO or I/O port ranges get passed to a guest.
> 
> At once drop open-coding of cache_flush_permitted().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Also drop open-coding of cache_flush_permitted().
> 
> --- a/xen/arch/x86/hvm/mtrr.c
> +++ b/xen/arch/x86/hvm/mtrr.c
> @@ -20,6 +20,7 @@
>  #include <public/hvm/e820.h>
>  #include <xen/types.h>
>  #include <asm/e820.h>
> +#include <asm/iocap.h>
>  #include <asm/mm.h>
>  #include <asm/paging.h>
>  #include <asm/p2m.h>
> @@ -810,10 +811,7 @@ int epte_get_entry_emt(struct domain *d,
>          return -1;
>      }
> 
> -    if ( !iommu_enabled ||
> -         (rangeset_is_empty(d->iomem_caps) &&
> -          rangeset_is_empty(d->arch.ioport_caps) &&
> -          !has_arch_pdevs(d)) )
> +    if ( !need_iommu(d) && !cache_flush_permitted(d) )

&&->||

>      {
>          ASSERT(!direct_mmio ||
>                 !((mfn_x(mfn) ^
> d->arch.hvm_domain.vmx.apic_access_mfn) >>
> 

Re: [PATCH 2/4 v2] x86/EPT: refine direct MMIO checking when determining EMT

[Jan Beulich]

>>> On 28.04.14 at 09:59, <kevin.tian@intel.com> wrote:
>>  From: Jan Beulich [mailto:JBeulich@suse.com]
>> @@ -810,10 +811,7 @@ int epte_get_entry_emt(struct domain *d,
>>          return -1;
>>      }
>> 
>> -    if ( !iommu_enabled ||
>> -         (rangeset_is_empty(d->iomem_caps) &&
>> -          rangeset_is_empty(d->arch.ioport_caps) &&
>> -          !has_arch_pdevs(d)) )
>> +    if ( !need_iommu(d) && !cache_flush_permitted(d) )
> 
> &&->||

No, specifically not - we shouldn't force WB in either case.

Jan

Re: [PATCH 2/4 v2] x86/EPT: refine direct MMIO checking when determining EMT

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Monday, April 28, 2014 4:25 PM
> 
> >>> On 28.04.14 at 09:59, <kevin.tian@intel.com> wrote:
> >>  From: Jan Beulich [mailto:JBeulich@suse.com]
> >> @@ -810,10 +811,7 @@ int epte_get_entry_emt(struct domain *d,
> >>          return -1;
> >>      }
> >>
> >> -    if ( !iommu_enabled ||
> >> -         (rangeset_is_empty(d->iomem_caps) &&
> >> -          rangeset_is_empty(d->arch.ioport_caps) &&
> >> -          !has_arch_pdevs(d)) )
> >> +    if ( !need_iommu(d) && !cache_flush_permitted(d) )
> >
> > &&->||
> 
> No, specifically not - we shouldn't force WB in either case.
> 

well, I gave original comments with the impression that it's a cleanup patch
instead of fixing anything, e.g. iommu_enabled->need_iommu, and open-coding
to cache_flush_permitted. Based on that thought, && changes original meaning.

so you actually fix the conditions here. I didn't follow up latest advance in device
assignment recently. Could you elaborate in which situation I/O resources are
assigned while iommu is not required?

Thanks
Kevin

Re: [PATCH 2/4 v2] x86/EPT: refine direct MMIO checking when determining EMT

[Jan Beulich]

>>> On 28.04.14 at 11:06, <kevin.tian@intel.com> wrote:
>>  From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Monday, April 28, 2014 4:25 PM
>> 
>> >>> On 28.04.14 at 09:59, <kevin.tian@intel.com> wrote:
>> >>  From: Jan Beulich [mailto:JBeulich@suse.com]
>> >> @@ -810,10 +811,7 @@ int epte_get_entry_emt(struct domain *d,
>> >>          return -1;
>> >>      }
>> >>
>> >> -    if ( !iommu_enabled ||
>> >> -         (rangeset_is_empty(d->iomem_caps) &&
>> >> -          rangeset_is_empty(d->arch.ioport_caps) &&
>> >> -          !has_arch_pdevs(d)) )
>> >> +    if ( !need_iommu(d) && !cache_flush_permitted(d) )
>> >
>> > &&->||
>> 
>> No, specifically not - we shouldn't force WB in either case.
>> 
> 
> well, I gave original comments with the impression that it's a cleanup patch
> instead of fixing anything, e.g. iommu_enabled->need_iommu, and open-coding
> to cache_flush_permitted. Based on that thought, && changes original meaning.
> 
> so you actually fix the conditions here. I didn't follow up latest advance 
> in device
> assignment recently. Could you elaborate in which situation I/O resources 
> are
> assigned while iommu is not required?

It's not so much a question of "required": IOMMU setup only
happens when a PCI device gets assigned to a guest. It specifically
does not happen (and would make no sense, because the IOMMU
concepts are PCI device based) when a raw MMIO or port range gets
assigned. While the latter is insecure, I don't think we currently do
anything to disallow this, and hence we shouldn't break it here.

Jan

Re: [PATCH 2/4 v2] x86/EPT: refine direct MMIO checking when determining EMT

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Monday, April 28, 2014 5:52 PM
> 
> >>> On 28.04.14 at 11:06, <kevin.tian@intel.com> wrote:
> >>  From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Monday, April 28, 2014 4:25 PM
> >>
> >> >>> On 28.04.14 at 09:59, <kevin.tian@intel.com> wrote:
> >> >>  From: Jan Beulich [mailto:JBeulich@suse.com]
> >> >> @@ -810,10 +811,7 @@ int epte_get_entry_emt(struct domain *d,
> >> >>          return -1;
> >> >>      }
> >> >>
> >> >> -    if ( !iommu_enabled ||
> >> >> -         (rangeset_is_empty(d->iomem_caps) &&
> >> >> -          rangeset_is_empty(d->arch.ioport_caps) &&
> >> >> -          !has_arch_pdevs(d)) )
> >> >> +    if ( !need_iommu(d) && !cache_flush_permitted(d) )
> >> >
> >> > &&->||
> >>
> >> No, specifically not - we shouldn't force WB in either case.
> >>
> >
> > well, I gave original comments with the impression that it's a cleanup patch
> > instead of fixing anything, e.g. iommu_enabled->need_iommu, and
> open-coding
> > to cache_flush_permitted. Based on that thought, && changes original
> meaning.
> >
> > so you actually fix the conditions here. I didn't follow up latest advance
> > in device
> > assignment recently. Could you elaborate in which situation I/O resources
> > are
> > assigned while iommu is not required?
> 
> It's not so much a question of "required": IOMMU setup only
> happens when a PCI device gets assigned to a guest. It specifically
> does not happen (and would make no sense, because the IOMMU
> concepts are PCI device based) when a raw MMIO or port range gets
> assigned. While the latter is insecure, I don't think we currently do
> anything to disallow this, and hence we shouldn't break it here.
> 

Got it. Acked-by: Kevin Tian <kevin.tian@intel.com>

Re: [PATCH 0/4] x86/EPT: miscellaneous further fixes to EMT determination

[Tim Deegan]

At 09:21 +0100 on 23 Apr (1398241319), Jan Beulich wrote:
> 1: consider page order when checking for APIC MFN
> 2: refine direct MMIO checking when determining EMT
> 3: fix pinned cache attribute range checking
> 4: also force EMT re-evaluation if pinned ranges change
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Tim Deegan <tim@xen.org>