[v4][PATCH 1/2] xen:x86:mm:p2m: introduce set_rmrr_p2m_entry

[v4][PATCH 1/2] xen:x86:mm:p2m: introduce set_rmrr_p2m_entry

[Tiejun Chen]

Its used conveniently to create RMRR mapping in shared EPT case.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 xen/arch/x86/mm/p2m.c     | 26 ++++++++++++++++++++++++++
 xen/include/asm-x86/p2m.h |  3 +++
 2 files changed, 29 insertions(+)

v4:

* new patch to combine get and set together to create RMRR mapping.

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 642ec28..09aa628 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -858,6 +858,32 @@ int set_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn)
     return set_typed_p2m_entry(d, gfn, mfn, p2m_mmio_direct);
 }
 
+int set_rmrr_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn)
+{
+    p2m_type_t p2mt;
+    p2m_access_t a;
+    mfn_t tmp_mfn;
+    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+    int ret = -EBUSY;
+
+    gfn_lock(p2m, gfn, 0);
+
+    tmp_mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL);
+
+    if ( mfn_valid(tmp_mfn) )
+    {
+        gdprintk(XENLOG_ERR,
+                 "Overlapping RMRRs at %"PRIx64".\n", (paddr_t)gfn);
+        goto out;
+    }
+
+    ret = p2m_set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2m_mmio_direct,
+                        p2m_access_rw);
+ out:
+    gfn_unlock(p2m, gfn, 0);
+    return ret;
+}
+
 /* Returns: 0 for success, -errno for failure */
 int clear_mmio_p2m_entry(struct domain *d, unsigned long gfn)
 {
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 0ddbadb..e616b9b 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -536,6 +536,9 @@ int p2m_is_logdirty_range(struct p2m_domain *, unsigned long start,
 int set_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn);
 int clear_mmio_p2m_entry(struct domain *d, unsigned long gfn);
 
+/* Set rmrr addresses in the p2m table (for pass-through) */
+int set_rmrr_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn);
+
 /* Add foreign mapping to the guest's p2m table. */
 int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
                     unsigned long gpfn, domid_t foreign_domid);
-- 
1.9.1

[v4][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Tiejun Chen]

intel_iommu_map_page() does nothing if VT-d shares EPT page table.
So rmrr_identity_mapping() never create RMRR mapping but in some
cases like some GFX drivers it still need to access RMRR.

Here we will create those RMRR mappings even in shared EPT case.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 xen/drivers/passthrough/vtd/iommu.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

v4:

* After introduce set_rmrr_p2m_entry we can go there directly.

v3:

* Use set_mmio_p2m_entry() to replace p2m_set_entry()
* Use ASSERT to check

v2:

* Fix coding style.
* Still need to abide intel_iommu_map_page(), so we should do nothing
  if dom0 and iommu supports pass thru.

diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 042b882..fe626e0 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1867,8 +1867,14 @@ static int rmrr_identity_mapping(struct domain *d,
 
     while ( base_pfn < end_pfn )
     {
-        if ( intel_iommu_map_page(d, base_pfn, base_pfn,
-                                  IOMMUF_readable|IOMMUF_writable) )
+        if ( iommu_use_hap_pt(d) )
+        {
+            ASSERT(!iommu_passthrough || !is_hardware_domain(d));
+            if ( set_rmrr_p2m_entry(d, base_pfn, _mfn(base_pfn)) )
+                return -1;
+        }
+        else if ( intel_iommu_map_page(d, base_pfn, base_pfn,
+                                       IOMMUF_readable|IOMMUF_writable) )
             return -1;
         base_pfn++;
     }
-- 
1.9.1

Re: [v4][PATCH 1/2] xen:x86:mm:p2m: introduce set_rmrr_p2m_entry

[Jan Beulich]

>>> On 28.07.14 at 13:42, <tiejun.chen@intel.com> wrote:
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -858,6 +858,32 @@ int set_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn)
>      return set_typed_p2m_entry(d, gfn, mfn, p2m_mmio_direct);
>  }
>  
> +int set_rmrr_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn)

Leaving aside all the previously raised points making me hesitate to
accept the pair of patches on their own, let's please try to avoid
having Intel specific function names in non-Intel-specific code unless
that's really the best choice. In the case here, you could
s/rmrr/identity/ and at once drop the redundant mfn argument (the
function can construct this for the call to p2m_set_entry(), and the
function name will make it obvious why this gets derived from gfn).

Jan

Re: [v4][PATCH 1/2] xen:x86:mm:p2m: introduce set_rmrr_p2m_entry

[Chen, Tiejun]

On 2014/7/28 20:07, Jan Beulich wrote:
>>>> On 28.07.14 at 13:42, <tiejun.chen@intel.com> wrote:
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -858,6 +858,32 @@ int set_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn)
>>       return set_typed_p2m_entry(d, gfn, mfn, p2m_mmio_direct);
>>   }
>>
>> +int set_rmrr_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn)
>
> Leaving aside all the previously raised points making me hesitate to
> accept the pair of patches on their own, let's please try to avoid

Do you mean we should fix such RMRR problem completely? Looks we need to 
reserve the RMRR range for each VM. So I'm considering to force 
reserving this range when we go that XENMEM_machine_memory_map for each 
VM. But I'm not sure if this is good. So maybe we can do this after this 
thread since I guess myself need some time to investigate this problem. 
Or you know this is ongoing by some guys, I'd like to refine my patches 
to follow that.

> having Intel specific function names in non-Intel-specific code unless
> that's really the best choice. In the case here, you could
> s/rmrr/identity/ and at once drop the redundant mfn argument (the
> function can construct this for the call to p2m_set_entry(), and the
> function name will make it obvious why this gets derived from gfn).
>

Okay.

Thanks
Tiejun

Re: [v4][PATCH 1/2] xen:x86:mm:p2m: introduce set_rmrr_p2m_entry

[Jan Beulich]

>>> On 29.07.14 at 08:44, <tiejun.chen@intel.com> wrote:
> On 2014/7/28 20:07, Jan Beulich wrote:
>>>>> On 28.07.14 at 13:42, <tiejun.chen@intel.com> wrote:
>>> --- a/xen/arch/x86/mm/p2m.c
>>> +++ b/xen/arch/x86/mm/p2m.c
>>> @@ -858,6 +858,32 @@ int set_mmio_p2m_entry(struct domain *d, unsigned long 
> gfn, mfn_t mfn)
>>>       return set_typed_p2m_entry(d, gfn, mfn, p2m_mmio_direct);
>>>   }
>>>
>>> +int set_rmrr_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn)
>>
>> Leaving aside all the previously raised points making me hesitate to
>> accept the pair of patches on their own, let's please try to avoid
> 
> Do you mean we should fix such RMRR problem completely? Looks we need to 
> reserve the RMRR range for each VM. So I'm considering to force 
> reserving this range when we go that XENMEM_machine_memory_map for each 
> VM. But I'm not sure if this is good. So maybe we can do this after this 
> thread since I guess myself need some time to investigate this problem. 
> Or you know this is ongoing by some guys, I'd like to refine my patches 
> to follow that.

Yes, I do think we either need to fix this in its entirety, or - as
suggested before - disallow passing through devices associated
with an RMRR. And no, I'm not aware of anyone already looking
into this.

Jan