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

[PATCH 1/1] 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 | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 042b882..be9384d 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -41,6 +41,7 @@
 #include "extern.h"
 #include "vtd.h"
 #include "../ats.h"
+#include "../../../arch/x86/mm/mm-locks.h"
 
 struct mapped_rmrr {
     struct list_head list;
@@ -1842,6 +1843,7 @@ static int rmrr_identity_mapping(struct domain *d,
     unsigned long base_pfn, end_pfn;
     struct mapped_rmrr *mrmrr;
     struct hvm_iommu *hd = domain_hvm_iommu(d);
+    struct p2m_domain *p2m = p2m_get_hostp2m(d);
 
     ASSERT(spin_is_locked(&pcidevs_lock));
     ASSERT(rmrr->base_address < rmrr->end_address);
@@ -1867,7 +1869,19 @@ static int rmrr_identity_mapping(struct domain *d,
 
     while ( base_pfn < end_pfn )
     {
-        if ( intel_iommu_map_page(d, base_pfn, base_pfn,
+        if ( iommu_use_hap_pt(d) ) {
+            dprintk(XENLOG_DEBUG VTDPREFIX,
+                    "Set RMRR mapping: pfn:0x%lx mfn:0x%lx.\n",
+                    base_pfn, mfn_x(_mfn(base_pfn)));
+            p2m_lock(p2m);
+            if ( p2m_set_entry(p2m, base_pfn, _mfn(base_pfn), PAGE_ORDER_4K,
+                    p2m_mmio_direct, p2m_access_rw) ) {
+                p2m_unlock(p2m);
+                return -1;
+            }
+            p2m_unlock(p2m);
+        }
+        else if ( intel_iommu_map_page(d, base_pfn, base_pfn,
                                   IOMMUF_readable|IOMMUF_writable) )
             return -1;
         base_pfn++;
-- 
1.9.1

Re: [PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 23.07.14 at 11:35, <tiejun.chen@intel.com> wrote:
> 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.

Technically this is fine, but there are mechanical issues:

> @@ -1867,7 +1869,19 @@ static int rmrr_identity_mapping(struct domain *d,
>  
>      while ( base_pfn < end_pfn )
>      {
> -        if ( intel_iommu_map_page(d, base_pfn, base_pfn,
> +        if ( iommu_use_hap_pt(d) ) {
> +            dprintk(XENLOG_DEBUG VTDPREFIX,
> +                    "Set RMRR mapping: pfn:0x%lx mfn:0x%lx.\n",
> +                    base_pfn, mfn_x(_mfn(base_pfn)));

Do we really need this message, even more so not at guest level?
And if you're convinced we need it, please use %#lx instead of
0x%lx.

> +            p2m_lock(p2m);
> +            if ( p2m_set_entry(p2m, base_pfn, _mfn(base_pfn), PAGE_ORDER_4K,
> +                    p2m_mmio_direct, p2m_access_rw) ) {
> +                p2m_unlock(p2m);
> +                return -1;
> +            }
> +            p2m_unlock(p2m);
> +        }
> +        else if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>                                    IOMMUF_readable|IOMMUF_writable) )
>              return -1;
>          base_pfn++;

Apart from the above there are several coding style issues here.

Jan

Re: [PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/7/23 23:42, Jan Beulich wrote:
>>>> On 23.07.14 at 11:35, <tiejun.chen@intel.com> wrote:
>> 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.
>
> Technically this is fine, but there are mechanical issues:
>
>> @@ -1867,7 +1869,19 @@ static int rmrr_identity_mapping(struct domain *d,
>>
>>       while ( base_pfn < end_pfn )
>>       {
>> -        if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>> +        if ( iommu_use_hap_pt(d) ) {
>> +            dprintk(XENLOG_DEBUG VTDPREFIX,
>> +                    "Set RMRR mapping: pfn:0x%lx mfn:0x%lx.\n",
>> +                    base_pfn, mfn_x(_mfn(base_pfn)));
>
> Do we really need this message, even more so not at guest level?

Its useful to debug as I think, but if you insist on this point, I'm 
fine to remove this as well.

> And if you're convinced we need it, please use %#lx instead of
> 0x%lx.

Okay.

>
>> +            p2m_lock(p2m);
>> +            if ( p2m_set_entry(p2m, base_pfn, _mfn(base_pfn), PAGE_ORDER_4K,
>> +                    p2m_mmio_direct, p2m_access_rw) ) {
>> +                p2m_unlock(p2m);
>> +                return -1;
>> +            }
>> +            p2m_unlock(p2m);
>> +        }
>> +        else if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>>                                     IOMMUF_readable|IOMMUF_writable) )
>>               return -1;
>>           base_pfn++;
>
> Apart from the above there are several coding style issues here.

Are you saying this thing?

if ()
{
}

So what about this?


@@ -1867,7 +1869,21 @@ static int rmrr_identity_mapping(struct domain *d,

      while ( base_pfn < end_pfn )
      {
-        if ( intel_iommu_map_page(d, base_pfn, base_pfn,
+        if ( iommu_use_hap_pt(d) )
+        {
+            dprintk(XENLOG_DEBUG VTDPREFIX,
+                    "Set RMRR mapping: pfn:%#lx mfn:%#lx.\n",
+                    base_pfn, mfn_x(_mfn(base_pfn)));
+            p2m_lock(p2m);
+            if ( p2m_set_entry(p2m, base_pfn, _mfn(base_pfn), 
PAGE_ORDER_4K,
+                    p2m_mmio_direct, p2m_access_rw) )
+            {
+                p2m_unlock(p2m);
+                return -1;
+            }
+            p2m_unlock(p2m);
+        }
+        else if ( intel_iommu_map_page(d, base_pfn, base_pfn,
                                    IOMMUF_readable|IOMMUF_writable) )
              return -1;
          base_pfn++;

Thanks
Tiejun

Re: [PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 24.07.14 at 03:23, <tiejun.chen@intel.com> wrote:
> On 2014/7/23 23:42, Jan Beulich wrote:
>>>>> On 23.07.14 at 11:35, <tiejun.chen@intel.com> wrote:
>>> @@ -1867,7 +1869,19 @@ static int rmrr_identity_mapping(struct domain *d,
>>>
>>>       while ( base_pfn < end_pfn )
>>>       {
>>> -        if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>>> +        if ( iommu_use_hap_pt(d) ) {
>>> +            dprintk(XENLOG_DEBUG VTDPREFIX,
>>> +                    "Set RMRR mapping: pfn:0x%lx mfn:0x%lx.\n",
>>> +                    base_pfn, mfn_x(_mfn(base_pfn)));
>>
>> Do we really need this message, even more so not at guest level?
> 
> Its useful to debug as I think, but if you insist on this point, I'm 
> fine to remove this as well.

The main question is how frequently this may get printed vs how
useful the message is.

>> Apart from the above there are several coding style issues here.
> 
> Are you saying this thing?
> 
> if ()
> {
> }

Yes, among other things.

> So what about this?

Almost:

> @@ -1867,7 +1869,21 @@ static int rmrr_identity_mapping(struct domain *d,
> 
>       while ( base_pfn < end_pfn )
>       {
> -        if ( intel_iommu_map_page(d, base_pfn, base_pfn,
> +        if ( iommu_use_hap_pt(d) )

Don't you, btw, need to extend this condition by
&& (!iommu_passthrough || !is_hardware_domain(d))?

> +        {
> +            dprintk(XENLOG_DEBUG VTDPREFIX,

This still (if you absolutely want to retain the message) needs
changing to XENLOG_G_DEBUG, and you want to include the domain
ID in what gets printed for the message to be of any practical use.

> +                    "Set RMRR mapping: pfn:%#lx mfn:%#lx.\n",

Additionally please omit the stop at the end. Also, with VTDPREFIX
not ending with a space, you want the message to be starting
with one.

> +                    base_pfn, mfn_x(_mfn(base_pfn)));
> +            p2m_lock(p2m);
> +            if ( p2m_set_entry(p2m, base_pfn, _mfn(base_pfn), PAGE_ORDER_4K,
> +                    p2m_mmio_direct, p2m_access_rw) )

Indentation.

> +            {
> +                p2m_unlock(p2m);
> +                return -1;
> +            }
> +            p2m_unlock(p2m);
> +        }
> +        else if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>                                     IOMMUF_readable|IOMMUF_writable) )

Again (here you need you also adjust the second line for indentation
to match up again).

Jan

Re: [PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/7/24 14:14, Jan Beulich wrote:
>>>> On 24.07.14 at 03:23, <tiejun.chen@intel.com> wrote:
>> On 2014/7/23 23:42, Jan Beulich wrote:
>>>>>> On 23.07.14 at 11:35, <tiejun.chen@intel.com> wrote:
>>>> @@ -1867,7 +1869,19 @@ static int rmrr_identity_mapping(struct domain *d,
>>>>
>>>>        while ( base_pfn < end_pfn )
>>>>        {
>>>> -        if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>>>> +        if ( iommu_use_hap_pt(d) ) {
>>>> +            dprintk(XENLOG_DEBUG VTDPREFIX,
>>>> +                    "Set RMRR mapping: pfn:0x%lx mfn:0x%lx.\n",
>>>> +                    base_pfn, mfn_x(_mfn(base_pfn)));
>>>
>>> Do we really need this message, even more so not at guest level?
>>
>> Its useful to debug as I think, but if you insist on this point, I'm
>> fine to remove this as well.
>
> The main question is how frequently this may get printed vs how
> useful the message is.
>
>>> Apart from the above there are several Indentation issues here.
>>
>> Are you saying this thing?
>>
>> if ()
>> {
>> }
>
> Yes, among other things.
>
>> So what about this?
>
> Almost:
>
>> @@ -1867,7 +1869,21 @@ static int rmrr_identity_mapping(struct domain *d,
>>
>>        while ( base_pfn < end_pfn )
>>        {
>> -        if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>> +        if ( iommu_use_hap_pt(d) )
>
> Don't you, btw, need to extend this condition by
> && (!iommu_passthrough || !is_hardware_domain(d))?

Why do we need these checks here?

Current problem I met is issued when do GFX passthrough for Windows Guest.

>
>> +        {
>> +            dprintk(XENLOG_DEBUG VTDPREFIX,
>
> This still (if you absolutely want to retain the message) needs

I will remove this simply since this is not a big deal :)

> changing to XENLOG_G_DEBUG, and you want to include the domain
> ID in what gets printed for the message to be of any practical use.
>
>> +                    "Set RMRR mapping: pfn:%#lx mfn:%#lx.\n",
>
> Additionally please omit the stop at the end. Also, with VTDPREFIX
> not ending with a space, you want the message to be starting
> with one.
>
>> +                    base_pfn, mfn_x(_mfn(base_pfn)));
>> +            p2m_lock(p2m);
>> +            if ( p2m_set_entry(p2m, base_pfn, _mfn(base_pfn), PAGE_ORDER_4K,
>> +                    p2m_mmio_direct, p2m_access_rw) )
>
> Indentation.
>
>> +            {
>> +                p2m_unlock(p2m);
>> +                return -1;
>> +            }
>> +            p2m_unlock(p2m);
>> +        }
>> +        else if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>>                                      IOMMUF_readable|IOMMUF_writable) )
>
> Again (here you need you also adjust the second line for indentation
> to match up again).

I'm not familiar with our xen coding style so I'm wondering if we have 
such a similar .pl like checkpatch.pl.

Thanks
Tiejun

Re: [PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/7/24 15:00, Chen, Tiejun wrote:
> On 2014/7/24 14:14, Jan Beulich wrote:
>>>>> On 24.07.14 at 03:23, <tiejun.chen@intel.com> wrote:
>>> On 2014/7/23 23:42, Jan Beulich wrote:
>>>>>>> On 23.07.14 at 11:35, <tiejun.chen@intel.com> wrote:
>>>>> @@ -1867,7 +1869,19 @@ static int rmrr_identity_mapping(struct
>>>>> domain *d,
>>>>>
>>>>>        while ( base_pfn < end_pfn )
>>>>>        {
>>>>> -        if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>>>>> +        if ( iommu_use_hap_pt(d) ) {
>>>>> +            dprintk(XENLOG_DEBUG VTDPREFIX,
>>>>> +                    "Set RMRR mapping: pfn:0x%lx mfn:0x%lx.\n",
>>>>> +                    base_pfn, mfn_x(_mfn(base_pfn)));
>>>>
>>>> Do we really need this message, even more so not at guest level?
>>>
>>> Its useful to debug as I think, but if you insist on this point, I'm
>>> fine to remove this as well.
>>
>> The main question is how frequently this may get printed vs how
>> useful the message is.
>>
>>>> Apart from the above there are several Indentation issues here.
>>>
>>> Are you saying this thing?
>>>
>>> if ()
>>> {
>>> }
>>
>> Yes, among other things.
>>
>>> So what about this?
>>
>> Almost:
>>
>>> @@ -1867,7 +1869,21 @@ static int rmrr_identity_mapping(struct domain
>>> *d,
>>>
>>>        while ( base_pfn < end_pfn )
>>>        {
>>> -        if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>>> +        if ( iommu_use_hap_pt(d) )
>>
>> Don't you, btw, need to extend this condition by
>> && (!iommu_passthrough || !is_hardware_domain(d))?
>
> Why do we need these checks here?
>
> Current problem I met is issued when do GFX passthrough for Windows Guest.
>
>>
>>> +        {
>>> +            dprintk(XENLOG_DEBUG VTDPREFIX,
>>
>> This still (if you absolutely want to retain the message) needs
>
> I will remove this simply since this is not a big deal :)
>
>> changing to XENLOG_G_DEBUG, and you want to include the domain
>> ID in what gets printed for the message to be of any practical use.
>>
>>> +                    "Set RMRR mapping: pfn:%#lx mfn:%#lx.\n",
>>
>> Additionally please omit the stop at the end. Also, with VTDPREFIX
>> not ending with a space, you want the message to be starting
>> with one.
>>
>>> +                    base_pfn, mfn_x(_mfn(base_pfn)));
>>> +            p2m_lock(p2m);
>>> +            if ( p2m_set_entry(p2m, base_pfn, _mfn(base_pfn),
>>> PAGE_ORDER_4K,
>>> +                    p2m_mmio_direct, p2m_access_rw) )
>>
>> Indentation.
>>
>>> +            {
>>> +                p2m_unlock(p2m);
>>> +                return -1;
>>> +            }
>>> +            p2m_unlock(p2m);
>>> +        }
>>> +        else if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>>>                                      IOMMUF_readable|IOMMUF_writable) )
>>
>> Again (here you need you also adjust the second line for indentation
>> to match up again).
>
> I'm not familiar with our xen coding style so I'm wondering if we have
> such a similar .pl like checkpatch.pl.
>

Anyway, what about this?

@@ -1867,8 +1869,20 @@ 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) && (!iommu_passthrough ||
+                                     !is_hardware_domain(d)) )
+        {
+            p2m_lock(p2m);
+            if ( p2m_set_entry(p2m, base_pfn, _mfn(base_pfn), 
PAGE_ORDER_4K,
+                               p2m_mmio_direct, p2m_access_rw) )
+            {
+                p2m_unlock(p2m);
+                return -1;
+            }
+            p2m_unlock(p2m);
+        }
+        else if ( intel_iommu_map_page(d, base_pfn, base_pfn,
+                                       IOMMUF_readable|IOMMUF_writable) )
              return -1;
          base_pfn++;
      }

Thanks
Tiejun

Re: [PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 24.07.14 at 09:15, <tiejun.chen@intel.com> wrote:
> Anyway, what about this?

Looks okay (apart from the question about the if() vs ASSERT()
mentioned in the previous reply).

Jan

Re: [PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 24.07.14 at 09:00, <tiejun.chen@intel.com> wrote:
> On 2014/7/24 14:14, Jan Beulich wrote:
>>>>> On 24.07.14 at 03:23, <tiejun.chen@intel.com> wrote:
>>> @@ -1867,7 +1869,21 @@ static int rmrr_identity_mapping(struct domain *d,
>>>
>>>        while ( base_pfn < end_pfn )
>>>        {
>>> -        if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>>> +        if ( iommu_use_hap_pt(d) )
>>
>> Don't you, btw, need to extend this condition by
>> && (!iommu_passthrough || !is_hardware_domain(d))?
> 
> Why do we need these checks here?

At least for documentation purposes: It would be wrong to try to
establish these mappings. I reckon iommu_use_hap_pt() implies the
combined other condition, so an ASSERT() would presumably be fine
as well (and get even closer to the intended documentation purpose).

Jan

Re: [PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/7/24 15:45, Jan Beulich wrote:
>>>> On 24.07.14 at 09:00, <tiejun.chen@intel.com> wrote:
>> On 2014/7/24 14:14, Jan Beulich wrote:
>>>>>> On 24.07.14 at 03:23, <tiejun.chen@intel.com> wrote:
>>>> @@ -1867,7 +1869,21 @@ static int rmrr_identity_mapping(struct domain *d,
>>>>
>>>>         while ( base_pfn < end_pfn )
>>>>         {
>>>> -        if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>>>> +        if ( iommu_use_hap_pt(d) )
>>>
>>> Don't you, btw, need to extend this condition by
>>> && (!iommu_passthrough || !is_hardware_domain(d))?
>>
>> Why do we need these checks here?
>
> At least for documentation purposes: It would be wrong to try to
> establish these mappings. I reckon iommu_use_hap_pt() implies the
> combined other condition, so an ASSERT() would presumably be fine
> as well (and get even closer to the intended documentation purpose).
>

I think if() should be reasonable here. Because

intel_iommu_map_page()
{
	...
     /* do nothing if dom0 and iommu supports pass thru */
     if ( iommu_passthrough && is_hardware_domain(d) )
         return 0;

We just do nothing to return simply. But if ASSERT will cause abort.

Thanks
Tiejun

Re: [PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 24.07.14 at 10:28, <tiejun.chen@intel.com> wrote:
> On 2014/7/24 15:45, Jan Beulich wrote:
>>>>> On 24.07.14 at 09:00, <tiejun.chen@intel.com> wrote:
>>> On 2014/7/24 14:14, Jan Beulich wrote:
>>>>>>> On 24.07.14 at 03:23, <tiejun.chen@intel.com> wrote:
>>>>> @@ -1867,7 +1869,21 @@ static int rmrr_identity_mapping(struct domain *d,
>>>>>
>>>>>         while ( base_pfn < end_pfn )
>>>>>         {
>>>>> -        if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>>>>> +        if ( iommu_use_hap_pt(d) )
>>>>
>>>> Don't you, btw, need to extend this condition by
>>>> && (!iommu_passthrough || !is_hardware_domain(d))?
>>>
>>> Why do we need these checks here?
>>
>> At least for documentation purposes: It would be wrong to try to
>> establish these mappings. I reckon iommu_use_hap_pt() implies the
>> combined other condition, so an ASSERT() would presumably be fine
>> as well (and get even closer to the intended documentation purpose).
>>
> 
> I think if() should be reasonable here. Because
> 
> intel_iommu_map_page()
> {
> 	...
>      /* do nothing if dom0 and iommu supports pass thru */
>      if ( iommu_passthrough && is_hardware_domain(d) )
>          return 0;
> 
> We just do nothing to return simply. But if ASSERT will cause abort.

Then tell me the scenario where iommu_use_hap_pt(d) is true and
both iommu_passthrough and is_hardware_domain(d) are true too.
Remember that for Dom0 iommu_use_hap_pt() can be true only for
PVH, and PVH implies !iommu_passthrough.

Jan

Re: [PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/7/24 17:41, Jan Beulich wrote:
>>>> On 24.07.14 at 10:28, <tiejun.chen@intel.com> wrote:
>> On 2014/7/24 15:45, Jan Beulich wrote:
>>>>>> On 24.07.14 at 09:00, <tiejun.chen@intel.com> wrote:
>>>> On 2014/7/24 14:14, Jan Beulich wrote:
>>>>>>>> On 24.07.14 at 03:23, <tiejun.chen@intel.com> wrote:
>>>>>> @@ -1867,7 +1869,21 @@ static int rmrr_identity_mapping(struct domain *d,
>>>>>>
>>>>>>          while ( base_pfn < end_pfn )
>>>>>>          {
>>>>>> -        if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>>>>>> +        if ( iommu_use_hap_pt(d) )
>>>>>
>>>>> Don't you, btw, need to extend this condition by
>>>>> && (!iommu_passthrough || !is_hardware_domain(d))?
>>>>
>>>> Why do we need these checks here?
>>>
>>> At least for documentation purposes: It would be wrong to try to
>>> establish these mappings. I reckon iommu_use_hap_pt() implies the
>>> combined other condition, so an ASSERT() would presumably be fine
>>> as well (and get even closer to the intended documentation purpose).
>>>
>>
>> I think if() should be reasonable here. Because
>>
>> intel_iommu_map_page()
>> {
>> 	...
>>       /* do nothing if dom0 and iommu supports pass thru */
>>       if ( iommu_passthrough && is_hardware_domain(d) )
>>           return 0;
>>
>> We just do nothing to return simply. But if ASSERT will cause abort.
>
> Then tell me the scenario where iommu_use_hap_pt(d) is true and
> both iommu_passthrough and is_hardware_domain(d) are true too.

Then HVM?

Anyway I did a test like this,

	if (iommu_use_hap_pt(d))
	{
		ASSERT (iommu_passthrough && is_hardware_domain(d));

Then Xen really reboot.

Thanks
Tiejun

> Remember that for Dom0 iommu_use_hap_pt() can be true only for
> PVH, and PVH implies !iommu_passthrough.
>
> Jan
>
>
>

Re: [PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 24.07.14 at 11:56, <tiejun.chen@intel.com> wrote:
> On 2014/7/24 17:41, Jan Beulich wrote:
>>>>> On 24.07.14 at 10:28, <tiejun.chen@intel.com> wrote:
>>> On 2014/7/24 15:45, Jan Beulich wrote:
>>>>>>> On 24.07.14 at 09:00, <tiejun.chen@intel.com> wrote:
>>>>> On 2014/7/24 14:14, Jan Beulich wrote:
>>>>>>>>> On 24.07.14 at 03:23, <tiejun.chen@intel.com> wrote:
>>>>>>> @@ -1867,7 +1869,21 @@ static int rmrr_identity_mapping(struct domain *d,
>>>>>>>
>>>>>>>          while ( base_pfn < end_pfn )
>>>>>>>          {
>>>>>>> -        if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>>>>>>> +        if ( iommu_use_hap_pt(d) )
>>>>>>
>>>>>> Don't you, btw, need to extend this condition by
>>>>>> && (!iommu_passthrough || !is_hardware_domain(d))?
>>>>>
>>>>> Why do we need these checks here?
>>>>
>>>> At least for documentation purposes: It would be wrong to try to
>>>> establish these mappings. I reckon iommu_use_hap_pt() implies the
>>>> combined other condition, so an ASSERT() would presumably be fine
>>>> as well (and get even closer to the intended documentation purpose).
>>>>
>>>
>>> I think if() should be reasonable here. Because
>>>
>>> intel_iommu_map_page()
>>> {
>>> 	...
>>>       /* do nothing if dom0 and iommu supports pass thru */
>>>       if ( iommu_passthrough && is_hardware_domain(d) )
>>>           return 0;
>>>
>>> We just do nothing to return simply. But if ASSERT will cause abort.
>>
>> Then tell me the scenario where iommu_use_hap_pt(d) is true and
>> both iommu_passthrough and is_hardware_domain(d) are true too.
> 
> Then HVM?

If we will ever see a HVM Dom0 it'll clearly also require
!iommu_passthrough, just like PVH does - this mode is solely for PV.

> Anyway I did a test like this,
> 
> 	if (iommu_use_hap_pt(d))
> 	{
> 		ASSERT (iommu_passthrough && is_hardware_domain(d));
> 
> Then Xen really reboot.

Of course, because you inverted the condition. You want

	ASSERT(!iommu_passthrough || !is_hardware_domain(d));

i.e. what I had previously suggested to add with && to the original
condition.

Jan

Re: [PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/7/24 18:11, Jan Beulich wrote:
>>>> On 24.07.14 at 11:56, <tiejun.chen@intel.com> wrote:
>> On 2014/7/24 17:41, Jan Beulich wrote:
>>>>>> On 24.07.14 at 10:28, <tiejun.chen@intel.com> wrote:
>>>> On 2014/7/24 15:45, Jan Beulich wrote:
>>>>>>>> On 24.07.14 at 09:00, <tiejun.chen@intel.com> wrote:
>>>>>> On 2014/7/24 14:14, Jan Beulich wrote:
>>>>>>>>>> On 24.07.14 at 03:23, <tiejun.chen@intel.com> wrote:
>>>>>>>> @@ -1867,7 +1869,21 @@ static int rmrr_identity_mapping(struct domain *d,
>>>>>>>>
>>>>>>>>           while ( base_pfn < end_pfn )
>>>>>>>>           {
>>>>>>>> -        if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>>>>>>>> +        if ( iommu_use_hap_pt(d) )
>>>>>>>
>>>>>>> Don't you, btw, need to extend this condition by
>>>>>>> && (!iommu_passthrough || !is_hardware_domain(d))?
>>>>>>
>>>>>> Why do we need these checks here?
>>>>>
>>>>> At least for documentation purposes: It would be wrong to try to
>>>>> establish these mappings. I reckon iommu_use_hap_pt() implies the
>>>>> combined other condition, so an ASSERT() would presumably be fine
>>>>> as well (and get even closer to the intended documentation purpose).
>>>>>
>>>>
>>>> I think if() should be reasonable here. Because
>>>>
>>>> intel_iommu_map_page()
>>>> {
>>>> 	...
>>>>        /* do nothing if dom0 and iommu supports pass thru */
>>>>        if ( iommu_passthrough && is_hardware_domain(d) )
>>>>            return 0;
>>>>
>>>> We just do nothing to return simply. But if ASSERT will cause abort.
>>>
>>> Then tell me the scenario where iommu_use_hap_pt(d) is true and
>>> both iommu_passthrough and is_hardware_domain(d) are true too.
>>
>> Then HVM?
>
> If we will ever see a HVM Dom0 it'll clearly also require
> !iommu_passthrough, just like PVH does - this mode is solely for PV.
>
>> Anyway I did a test like this,
>>
>> 	if (iommu_use_hap_pt(d))
>> 	{
>> 		ASSERT (iommu_passthrough && is_hardware_domain(d));
>>
>> Then Xen really reboot.
>
> Of course, because you inverted the condition. You want
>
> 	ASSERT(!iommu_passthrough || !is_hardware_domain(d));

You're right. Sorry I'm misunderstanding this point.

Tiejun

>
> i.e. what I had previously suggested to add with && to the original
> condition.
>
> Jan
>
>