[PATCH for 4.6] VT-d: Create IOMMU mappings for RMRR regions if shared EPT is not being used

[PATCH for 4.6] VT-d: Create IOMMU mappings for RMRR regions if shared EPT is not being used

[Malcolm Crossley]

Add RMRR 1:1 IOMMU mappings to IOMMU page tables if EPT page table are not being
shared with the IOMMU.

This is a regression in behaviour versus Xen 4.5.

Signed-off-by: Malcolm Crossley <malcolm.crossley@citrix.com>
---
 xen/drivers/passthrough/vtd/iommu.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 836aed5..89de741 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1839,8 +1839,16 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map,
 
             while ( base_pfn < end_pfn )
             {
-                if ( clear_identity_p2m_entry(d, base_pfn) )
-                    ret = -ENXIO;
+                if ( iommu_use_hap_pt(d) )
+                {
+                    if ( clear_identity_p2m_entry(d, base_pfn) )
+                            ret = -ENXIO;
+                }
+                else
+                {
+                    if ( intel_iommu_unmap_page(d, base_pfn) )
+                        ret = -ENXIO;
+                }
                 base_pfn++;
             }
 
@@ -1855,7 +1863,16 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map,
 
     while ( base_pfn < end_pfn )
     {
-        int err = set_identity_p2m_entry(d, base_pfn, p2m_access_rw, flag);
+        int err;
+        if ( iommu_use_hap_pt(d) )
+        {
+            err = set_identity_p2m_entry(d, base_pfn, p2m_access_rw, flag);
+        }
+        else
+        {
+            err = intel_iommu_map_page(d, base_pfn, base_pfn,
+                                       IOMMUF_readable|IOMMUF_writable);
+        }
 
         if ( err )
             return err;
-- 
1.7.12.4

Re: [PATCH for 4.6] VT-d: Create IOMMU mappings for RMRR regions if shared EPT is not being used

[Chen, Tiejun]

This kind of issue is already gone.

https://www.mail-archive.com/xen-devel@lists.xen.org/msg32464.html

Thanks
Tiejun

On 8/26/2015 11:49 PM, Malcolm Crossley wrote:
> Add RMRR 1:1 IOMMU mappings to IOMMU page tables if EPT page table are not being
> shared with the IOMMU.
>
> This is a regression in behaviour versus Xen 4.5.
>
> Signed-off-by: Malcolm Crossley <malcolm.crossley@citrix.com>
> ---
>   xen/drivers/passthrough/vtd/iommu.c | 23 ++++++++++++++++++++---
>   1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
> index 836aed5..89de741 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1839,8 +1839,16 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map,
>
>               while ( base_pfn < end_pfn )
>               {
> -                if ( clear_identity_p2m_entry(d, base_pfn) )
> -                    ret = -ENXIO;
> +                if ( iommu_use_hap_pt(d) )
> +                {
> +                    if ( clear_identity_p2m_entry(d, base_pfn) )
> +                            ret = -ENXIO;
> +                }
> +                else
> +                {
> +                    if ( intel_iommu_unmap_page(d, base_pfn) )
> +                        ret = -ENXIO;
> +                }
>                   base_pfn++;
>               }
>
> @@ -1855,7 +1863,16 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map,
>
>       while ( base_pfn < end_pfn )
>       {
> -        int err = set_identity_p2m_entry(d, base_pfn, p2m_access_rw, flag);
> +        int err;
> +        if ( iommu_use_hap_pt(d) )
> +        {
> +            err = set_identity_p2m_entry(d, base_pfn, p2m_access_rw, flag);
> +        }
> +        else
> +        {
> +            err = intel_iommu_map_page(d, base_pfn, base_pfn,
> +                                       IOMMUF_readable|IOMMUF_writable);
> +        }
>
>           if ( err )
>               return err;
>

Re: [PATCH for 4.6] VT-d: Create IOMMU mappings for RMRR regions if shared EPT is not being used

[Malcolm Crossley]

On 27/08/15 03:59, Chen, Tiejun wrote:
> This kind of issue is already gone.
> 
> https://www.mail-archive.com/xen-devel@lists.xen.org/msg32464.html

There is a bug in the code you refer to above which results in no IOMMU page table
mappings being created if the guest domain is not sharing it's EPT page tables with
the IOMMU.

set_identity_p2m_entry only configures the EPT page tables and does not configure
the IOMMU page tables.

We had a real world regression (with xen 4.6-rc1) on a Intel Haswell system with
integrated graphics.

The patch below resolves the regression.

Malcolm

> 
> Thanks
> Tiejun
> 
> On 8/26/2015 11:49 PM, Malcolm Crossley wrote:
>> Add RMRR 1:1 IOMMU mappings to IOMMU page tables if EPT page table are not being
>> shared with the IOMMU.
>>
>> This is a regression in behaviour versus Xen 4.5.
>>
>> Signed-off-by: Malcolm Crossley <malcolm.crossley@citrix.com>
>> ---
>>   xen/drivers/passthrough/vtd/iommu.c | 23 ++++++++++++++++++++---
>>   1 file changed, 20 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
>> index 836aed5..89de741 100644
>> --- a/xen/drivers/passthrough/vtd/iommu.c
>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>> @@ -1839,8 +1839,16 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map,
>>
>>               while ( base_pfn < end_pfn )
>>               {
>> -                if ( clear_identity_p2m_entry(d, base_pfn) )
>> -                    ret = -ENXIO;
>> +                if ( iommu_use_hap_pt(d) )
>> +                {
>> +                    if ( clear_identity_p2m_entry(d, base_pfn) )
>> +                            ret = -ENXIO;
>> +                }
>> +                else
>> +                {
>> +                    if ( intel_iommu_unmap_page(d, base_pfn) )
>> +                        ret = -ENXIO;
>> +                }
>>                   base_pfn++;
>>               }
>>
>> @@ -1855,7 +1863,16 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map,
>>
>>       while ( base_pfn < end_pfn )
>>       {
>> -        int err = set_identity_p2m_entry(d, base_pfn, p2m_access_rw, flag);
>> +        int err;
>> +        if ( iommu_use_hap_pt(d) )
>> +        {
>> +            err = set_identity_p2m_entry(d, base_pfn, p2m_access_rw, flag);
>> +        }
>> +        else
>> +        {
>> +            err = intel_iommu_map_page(d, base_pfn, base_pfn,
>> +                                       IOMMUF_readable|IOMMUF_writable);
>> +        }
>>
>>           if ( err )
>>               return err;
>>

Re: [PATCH for 4.6] VT-d: Create IOMMU mappings for RMRR regions if shared EPT is not being used

[Chen, Tiejun]

On 8/27/2015 4:40 PM, Malcolm Crossley wrote:
> On 27/08/15 03:59, Chen, Tiejun wrote:
>> This kind of issue is already gone.
>>
>> https://www.mail-archive.com/xen-devel@lists.xen.org/msg32464.html
>
> There is a bug in the code you refer to above which results in no IOMMU page table
> mappings being created if the guest domain is not sharing it's EPT page tables with
> the IOMMU.
>
> set_identity_p2m_entry only configures the EPT page tables and does not configure
> the IOMMU page tables.

Okay, I got what you mean.

Instead, could you insert iommu_{map,unmap_page() into 
{set,clear}_identity_p2m_entry()? I think this can make 
{set,clear}_identity_p2m_entry approachable in all circumstances.

Kevin and Jan,

Is this fine?

Thanks
Tiejun

>
> We had a real world regression (with xen 4.6-rc1) on a Intel Haswell system with
> integrated graphics.
>
> The patch below resolves the regression.
>
> Malcolm
>
>>
>> Thanks
>> Tiejun
>>
>> On 8/26/2015 11:49 PM, Malcolm Crossley wrote:
>>> Add RMRR 1:1 IOMMU mappings to IOMMU page tables if EPT page table are not being
>>> shared with the IOMMU.
>>>
>>> This is a regression in behaviour versus Xen 4.5.
>>>
>>> Signed-off-by: Malcolm Crossley <malcolm.crossley@citrix.com>
>>> ---
>>>   xen/drivers/passthrough/vtd/iommu.c | 23 ++++++++++++++++++++---
>>>   1 file changed, 20 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
>>> index 836aed5..89de741 100644
>>> --- a/xen/drivers/passthrough/vtd/iommu.c
>>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>>> @@ -1839,8 +1839,16 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map,
>>>
>>>               while ( base_pfn < end_pfn )
>>>               {
>>> -                if ( 	(d, base_pfn) )
>>> -                    ret = -ENXIO;
>>> +                if ( iommu_use_hap_pt(d) )
>>> +                {
>>> +                    if ( clear_identity_p2m_entry(d, base_pfn) )
>>> +                            ret = -ENXIO;
>>> +                }
>>> +                else
>>> +                {
>>> +                    if ( intel_iommu_unmap_page(d, base_pfn) )
>>> +                        ret = -ENXIO;
>>> +                }
>>>                   base_pfn++;
>>>               }
>>>
>>> @@ -1855,7 +1863,16 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map,
>>>
>>>       while ( base_pfn < end_pfn )
>>>       {
>>> -        int err = set_identity_p2m_entry(d, base_pfn, p2m_access_rw, flag);
>>> +        int err;
>>> +        if ( iommu_use_hap_pt(d) )
>>> +        {
>>> +            err = set_identity_p2m_entry(d, base_pfn, p2m_access_rw, flag);
>>> +        }
>>> +        else
>>> +        {
>>> +            err = intel_iommu_map_page(d, base_pfn, base_pfn,
>>> +                                       IOMMUF_readable|IOMMUF_writable);
>>> +        }
>>>
>>>           if ( err )
>>>               return err;
>>>
>
>

Re: [PATCH for 4.6] VT-d: Create IOMMU mappings for RMRR regions if shared EPT is not being used

[Jan Beulich]

>>> On 27.08.15 at 11:05, <tiejun.chen@intel.com> wrote:
> On 8/27/2015 4:40 PM, Malcolm Crossley wrote:
>> On 27/08/15 03:59, Chen, Tiejun wrote:
>>> This kind of issue is already gone.
>>>
>>> https://www.mail-archive.com/xen-devel@lists.xen.org/msg32464.html 
>>
>> There is a bug in the code you refer to above which results in no IOMMU page 
> table
>> mappings being created if the guest domain is not sharing it's EPT page 
> tables with
>> the IOMMU.
>>
>> set_identity_p2m_entry only configures the EPT page tables and does not 
> configure
>> the IOMMU page tables.
> 
> Okay, I got what you mean.
> 
> Instead, could you insert iommu_{map,unmap_page() into 
> {set,clear}_identity_p2m_entry()? I think this can make 
> {set,clear}_identity_p2m_entry approachable in all circumstances.
> 
> Kevin and Jan,
> 
> Is this fine?

This matches what I've also just suggested in another reply.

Jan

Re: [PATCH for 4.6] VT-d: Create IOMMU mappings for RMRR regions if shared EPT is not being used

[Tian, Kevin]

> From: Chen, Tiejun
> Sent: Thursday, August 27, 2015 5:05 PM
> 
> On 8/27/2015 4:40 PM, Malcolm Crossley wrote:
> > On 27/08/15 03:59, Chen, Tiejun wrote:
> >> This kind of issue is already gone.
> >>
> >> https://www.mail-archive.com/xen-devel@lists.xen.org/msg32464.html
> >
> > There is a bug in the code you refer to above which results in no IOMMU page table
> > mappings being created if the guest domain is not sharing it's EPT page tables with
> > the IOMMU.
> >
> > set_identity_p2m_entry only configures the EPT page tables and does not configure
> > the IOMMU page tables.
> 
> Okay, I got what you mean.
> 
> Instead, could you insert iommu_{map,unmap_page() into
> {set,clear}_identity_p2m_entry()? I think this can make
> {set,clear}_identity_p2m_entry approachable in all circumstances.
> 
> Kevin and Jan,
> 
> Is this fine?
> 

Yes that is the right thing to do. It's a bit surprise to me that it's
not there yet. :-)

Thanks
Kevin

Re: [PATCH for 4.6] VT-d: Create IOMMU mappings for RMRR regions if shared EPT is not being used

[Jan Beulich]

>>> On 26.08.15 at 17:49, <malcolm.crossley@citrix.com> wrote:
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1839,8 +1839,16 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map,
>  
>              while ( base_pfn < end_pfn )
>              {
> -                if ( clear_identity_p2m_entry(d, base_pfn) )
> -                    ret = -ENXIO;
> +                if ( iommu_use_hap_pt(d) )
> +                {
> +                    if ( clear_identity_p2m_entry(d, base_pfn) )
> +                            ret = -ENXIO;
> +                }
> +                else
> +                {
> +                    if ( intel_iommu_unmap_page(d, base_pfn) )
> +                        ret = -ENXIO;
> +                }
>                  base_pfn++;
>              }
>  
> @@ -1855,7 +1863,16 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map,
>  
>      while ( base_pfn < end_pfn )
>      {
> -        int err = set_identity_p2m_entry(d, base_pfn, p2m_access_rw, flag);
> +        int err;
> +        if ( iommu_use_hap_pt(d) )
> +        {
> +            err = set_identity_p2m_entry(d, base_pfn, p2m_access_rw, flag);
> +        }
> +        else
> +        {
> +            err = intel_iommu_map_page(d, base_pfn, base_pfn,
> +                                       IOMMUF_readable|IOMMUF_writable);
> +        }
>  
>          if ( err )
>              return err;

Considering that {set,clear}_identity_p2m_entry are really clones of
guest_physmap_{add,remove}_page(), I think it would be more
logical to follow their model of invoking iommu_{,un}map_page()
instead of having the current (and possible future) callers take care
of this for themselves. Which of course then raises the question of
the right predicate: The guest_physmap_* functions check
!paging_mode_translate(), and the !shared-ept case for HVM/PVH
is being covered by {p2m_pt,ept}_set_entry(). Surely asymmetry
here would be at least suspicious.

And which raises a second question (to the VT-d maintainers): Why
is it that intel_iommu_unmap_page() doesn't use
iommu_use_hap_pt() just like intel_iommu_map_page() as well as
amd_iommu_unmap_page() do?

Jan