[PATCH v2] xen/iommu: fix usage of shared EPT/IOMMU page tables on PVH guests

[PATCH v2] xen/iommu: fix usage of shared EPT/IOMMU page tables on PVH guests

[Roger Pau Monne]

iommu_share_p2m_table should not prevent PVH guests from using a shared page
table between the IOMMU and EPT. Clean the code by removing the asserts in
the vendor specific implementations (amd_iommu_share_p2m, iommu_set_pgd),
and moving the hap_enabled assert to the caller (iommu_share_p2m_table).

Also fix another incorrect usage of is_hvm_domain usage in
arch_iommu_populate_page_table. This has not given problems so far because
all the pages in PVH guests are of type PGT_writable_page.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Yang Zhang <yang.z.zhang@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
---
Changes since v1:
 - Reword commit message.
 - Move the hap_enabled assert from vendor implementation to the generic
   code.
 - Remove the has_hvm_container_domain condition from iommu_share_p2m_table,
   hap_enabled is more restrictive.
---
 xen/drivers/passthrough/amd/iommu_map.c | 2 --
 xen/drivers/passthrough/iommu.c         | 3 ++-
 xen/drivers/passthrough/vtd/iommu.c     | 2 --
 xen/drivers/passthrough/x86/iommu.c     | 2 +-
 4 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/xen/drivers/passthrough/amd/iommu_map.c b/xen/drivers/passthrough/amd/iommu_map.c
index a8c60ec..31dc05d 100644
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -785,8 +785,6 @@ void amd_iommu_share_p2m(struct domain *d)
     struct page_info *p2m_table;
     mfn_t pgd_mfn;
 
-    ASSERT( is_hvm_domain(d) && d->arch.hvm_domain.hap_enabled );
-
     if ( !iommu_use_hap_pt(d) )
         return;
 
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index cc12735..7fcbbb1 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -332,7 +332,8 @@ void iommu_share_p2m_table(struct domain* d)
 {
     const struct iommu_ops *ops = iommu_get_ops();
 
-    if ( iommu_enabled && is_hvm_domain(d) )
+    ASSERT( hap_enabled(d) );
+    if ( iommu_enabled )
         ops->share_p2m(d);
 }
 
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 2e113d7..ff542cb 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1788,8 +1788,6 @@ static void iommu_set_pgd(struct domain *d)
     struct hvm_iommu *hd  = domain_hvm_iommu(d);
     mfn_t pgd_mfn;
 
-    ASSERT( is_hvm_domain(d) && d->arch.hvm_domain.hap_enabled );
-
     if ( !iommu_use_hap_pt(d) )
         return;
 
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index 52d8948..9eb8d33 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -56,7 +56,7 @@ int arch_iommu_populate_page_table(struct domain *d)
 
     while ( !rc && (page = page_list_remove_head(&d->page_list)) )
     {
-        if ( is_hvm_domain(d) ||
+        if ( has_hvm_container_domain(d) ||
             (page->u.inuse.type_info & PGT_type_mask) == PGT_writable_page )
         {
             BUG_ON(SHARED_M2P(mfn_to_gmfn(d, page_to_mfn(page))));
-- 
1.9.3 (Apple Git-50)


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

Re: [PATCH v2] xen/iommu: fix usage of shared EPT/IOMMU page tables on PVH guests

[David Vrabel]

On 27/02/15 11:33, Roger Pau Monne wrote:
> iommu_share_p2m_table should not prevent PVH guests from using a shared page
> table between the IOMMU and EPT. Clean the code by removing the asserts in
> the vendor specific implementations (amd_iommu_share_p2m, iommu_set_pgd),
> and moving the hap_enabled assert to the caller (iommu_share_p2m_table).
> 
> Also fix another incorrect usage of is_hvm_domain usage in
> arch_iommu_populate_page_table. This has not given problems so far because
> all the pages in PVH guests are of type PGT_writable_page.

dom0 PVH failed to boot on my test system without this patch so:

Tested-by: David Vrabel <david.vrabel@citrix.com>

Thanks.

David

Re: [PATCH v2] xen/iommu: fix usage of shared EPT/IOMMU page tables on PVH guests

[Tim Deegan]

At 12:33 +0100 on 27 Feb (1425036812), Roger Pau Monne wrote:
> iommu_share_p2m_table should not prevent PVH guests from using a shared page
> table between the IOMMU and EPT. Clean the code by removing the asserts in
> the vendor specific implementations (amd_iommu_share_p2m, iommu_set_pgd),
> and moving the hap_enabled assert to the caller (iommu_share_p2m_table).
> 
> Also fix another incorrect usage of is_hvm_domain usage in
> arch_iommu_populate_page_table. This has not given problems so far because
> all the pages in PVH guests are of type PGT_writable_page.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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

Re: [PATCH v2] xen/iommu: fix usage of shared EPT/IOMMU page tables on PVH guests

[Tian, Kevin]

> From: Roger Pau Monne [mailto:roger.pau@citrix.com]
> Sent: Friday, February 27, 2015 7:34 PM
> 
> iommu_share_p2m_table should not prevent PVH guests from using a shared
> page
> table between the IOMMU and EPT. Clean the code by removing the asserts in
> the vendor specific implementations (amd_iommu_share_p2m,
> iommu_set_pgd),
> and moving the hap_enabled assert to the caller (iommu_share_p2m_table).
> 
> Also fix another incorrect usage of is_hvm_domain usage in
> arch_iommu_populate_page_table. This has not given problems so far
> because
> all the pages in PVH guests are of type PGT_writable_page.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Cc: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Yang Zhang <yang.z.zhang@intel.com>
> Cc: Kevin Tian <kevin.tian@intel.com>

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

btw, does this patch fix your earlier iommu faults with PVH Dom0?

Thanks
Kevin

> ---
> Changes since v1:
>  - Reword commit message.
>  - Move the hap_enabled assert from vendor implementation to the generic
>    code.
>  - Remove the has_hvm_container_domain condition from
> iommu_share_p2m_table,
>    hap_enabled is more restrictive.
> ---
>  xen/drivers/passthrough/amd/iommu_map.c | 2 --
>  xen/drivers/passthrough/iommu.c         | 3 ++-
>  xen/drivers/passthrough/vtd/iommu.c     | 2 --
>  xen/drivers/passthrough/x86/iommu.c     | 2 +-
>  4 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/amd/iommu_map.c
> b/xen/drivers/passthrough/amd/iommu_map.c
> index a8c60ec..31dc05d 100644
> --- a/xen/drivers/passthrough/amd/iommu_map.c
> +++ b/xen/drivers/passthrough/amd/iommu_map.c
> @@ -785,8 +785,6 @@ void amd_iommu_share_p2m(struct domain *d)
>      struct page_info *p2m_table;
>      mfn_t pgd_mfn;
> 
> -    ASSERT( is_hvm_domain(d) && d->arch.hvm_domain.hap_enabled );
> -
>      if ( !iommu_use_hap_pt(d) )
>          return;
> 
> diff --git a/xen/drivers/passthrough/iommu.c
> b/xen/drivers/passthrough/iommu.c
> index cc12735..7fcbbb1 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -332,7 +332,8 @@ void iommu_share_p2m_table(struct domain* d)
>  {
>      const struct iommu_ops *ops = iommu_get_ops();
> 
> -    if ( iommu_enabled && is_hvm_domain(d) )
> +    ASSERT( hap_enabled(d) );
> +    if ( iommu_enabled )
>          ops->share_p2m(d);
>  }
> 
> diff --git a/xen/drivers/passthrough/vtd/iommu.c
> b/xen/drivers/passthrough/vtd/iommu.c
> index 2e113d7..ff542cb 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1788,8 +1788,6 @@ static void iommu_set_pgd(struct domain *d)
>      struct hvm_iommu *hd  = domain_hvm_iommu(d);
>      mfn_t pgd_mfn;
> 
> -    ASSERT( is_hvm_domain(d) && d->arch.hvm_domain.hap_enabled );
> -
>      if ( !iommu_use_hap_pt(d) )
>          return;
> 
> diff --git a/xen/drivers/passthrough/x86/iommu.c
> b/xen/drivers/passthrough/x86/iommu.c
> index 52d8948..9eb8d33 100644
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -56,7 +56,7 @@ int arch_iommu_populate_page_table(struct domain *d)
> 
>      while ( !rc && (page = page_list_remove_head(&d->page_list)) )
>      {
> -        if ( is_hvm_domain(d) ||
> +        if ( has_hvm_container_domain(d) ||
>              (page->u.inuse.type_info & PGT_type_mask) ==
> PGT_writable_page )
>          {
>              BUG_ON(SHARED_M2P(mfn_to_gmfn(d,
> page_to_mfn(page))));
> --
> 1.9.3 (Apple Git-50)

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

Re: [PATCH v2] xen/iommu: fix usage of shared EPT/IOMMU page tables on PVH guests

[Roger Pau Monné]

El 09/03/15 a les 4.30, Tian, Kevin ha escrit:
>> From: Roger Pau Monne [mailto:roger.pau@citrix.com]
>> Sent: Friday, February 27, 2015 7:34 PM
>>
>> iommu_share_p2m_table should not prevent PVH guests from using a shared
>> page
>> table between the IOMMU and EPT. Clean the code by removing the asserts in
>> the vendor specific implementations (amd_iommu_share_p2m,
>> iommu_set_pgd),
>> and moving the hap_enabled assert to the caller (iommu_share_p2m_table).
>>
>> Also fix another incorrect usage of is_hvm_domain usage in
>> arch_iommu_populate_page_table. This has not given problems so far
>> because
>> all the pages in PVH guests are of type PGT_writable_page.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> Cc: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Yang Zhang <yang.z.zhang@intel.com>
>> Cc: Kevin Tian <kevin.tian@intel.com>
> 
> Acked-by: Kevin Tian <kevin.tian@intel.com>
> 
> btw, does this patch fix your earlier iommu faults with PVH Dom0?

Yes, it fixes booting a PVH Dom0 on hosts that share the page tables
between EPT and the IOMMU(s).

Roger.

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

Re: [PATCH v2] xen/iommu: fix usage of shared EPT/IOMMU page tables on PVH guests

[Julien Grall]

Hi,

On 27/02/15 11:33, Roger Pau Monne wrote:
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index cc12735..7fcbbb1 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -332,7 +332,8 @@ void iommu_share_p2m_table(struct domain* d)
>  {
>      const struct iommu_ops *ops = iommu_get_ops();
>  
> -    if ( iommu_enabled && is_hvm_domain(d) )
> +    ASSERT( hap_enabled(d) );

This line is breaking compilation on ARM.

Shouldn't it be replaced by iommu_use_hap_pt?

> +    if ( iommu_enabled )


Regards,

-- 
Julien Grall

Re: [PATCH v2] xen/iommu: fix usage of shared EPT/IOMMU page tables on PVH guests

[Jan Beulich]

>>> On 10.03.15 at 13:51, <julien.grall@linaro.org> wrote:
> Hi,
> 
> On 27/02/15 11:33, Roger Pau Monne wrote:
>> diff --git a/xen/drivers/passthrough/iommu.c 
> b/xen/drivers/passthrough/iommu.c
>> index cc12735..7fcbbb1 100644
>> --- a/xen/drivers/passthrough/iommu.c
>> +++ b/xen/drivers/passthrough/iommu.c
>> @@ -332,7 +332,8 @@ void iommu_share_p2m_table(struct domain* d)
>>  {
>>      const struct iommu_ops *ops = iommu_get_ops();
>>  
>> -    if ( iommu_enabled && is_hvm_domain(d) )
>> +    ASSERT( hap_enabled(d) );
> 
> This line is breaking compilation on ARM.
> 
> Shouldn't it be replaced by iommu_use_hap_pt?

No, that's a different thing. But shouldn't ARM have a stub
hap_enabled() evaluating to constant true?

Jan

Re: [PATCH v2] xen/iommu: fix usage of shared EPT/IOMMU page tables on PVH guests

[Julien Grall]

On 10/03/15 13:06, Jan Beulich wrote:
>>>> On 10.03.15 at 13:51, <julien.grall@linaro.org> wrote:
>> Hi,
>>
>> On 27/02/15 11:33, Roger Pau Monne wrote:
>>> diff --git a/xen/drivers/passthrough/iommu.c 
>> b/xen/drivers/passthrough/iommu.c
>>> index cc12735..7fcbbb1 100644
>>> --- a/xen/drivers/passthrough/iommu.c
>>> +++ b/xen/drivers/passthrough/iommu.c
>>> @@ -332,7 +332,8 @@ void iommu_share_p2m_table(struct domain* d)
>>>  {
>>>      const struct iommu_ops *ops = iommu_get_ops();
>>>  
>>> -    if ( iommu_enabled && is_hvm_domain(d) )
>>> +    ASSERT( hap_enabled(d) );
>>
>> This line is breaking compilation on ARM.
>>
>> Shouldn't it be replaced by iommu_use_hap_pt?
> 
> No, that's a different thing. But shouldn't ARM have a stub
> hap_enabled() evaluating to constant true?

I'm not sure if we should introduce hap_enabled. It's not something that
we should used in general.

What are we trying to catch with this ASSERT? I guess wrong caller?

If so, every share_p2m callbacks have a check "if iommu_use_hap_pt()"
which contains a check to hap_enabled on x86.

So I don't think this check is worthwhile in the common iommu code.

Regards,

-- 
Julien Grall

Re: [PATCH v2] xen/iommu: fix usage of shared EPT/IOMMU page tables on PVH guests

[Jan Beulich]

>>> On 10.03.15 at 14:18, <julien.grall@linaro.org> wrote:
> On 10/03/15 13:06, Jan Beulich wrote:
>>>>> On 10.03.15 at 13:51, <julien.grall@linaro.org> wrote:
>>> Hi,
>>>
>>> On 27/02/15 11:33, Roger Pau Monne wrote:
>>>> diff --git a/xen/drivers/passthrough/iommu.c 
>>> b/xen/drivers/passthrough/iommu.c
>>>> index cc12735..7fcbbb1 100644
>>>> --- a/xen/drivers/passthrough/iommu.c
>>>> +++ b/xen/drivers/passthrough/iommu.c
>>>> @@ -332,7 +332,8 @@ void iommu_share_p2m_table(struct domain* d)
>>>>  {
>>>>      const struct iommu_ops *ops = iommu_get_ops();
>>>>  
>>>> -    if ( iommu_enabled && is_hvm_domain(d) )
>>>> +    ASSERT( hap_enabled(d) );
>>>
>>> This line is breaking compilation on ARM.
>>>
>>> Shouldn't it be replaced by iommu_use_hap_pt?
>> 
>> No, that's a different thing. But shouldn't ARM have a stub
>> hap_enabled() evaluating to constant true?
> 
> I'm not sure if we should introduce hap_enabled. It's not something that
> we should used in general.
> 
> What are we trying to catch with this ASSERT? I guess wrong caller?
> 
> If so, every share_p2m callbacks have a check "if iommu_use_hap_pt()"
> which contains a check to hap_enabled on x86.

Ah, right, I mixed this up with iommu_hap_pt_share. Roger - looks
like this could indeed be replaced by

     if ( iommu_enabled && iommu_use_hap_pt(d) )
         ops->share_p2m(d);

and the corresponding check in VT-d and AMD Vi code could then
also be dropped.

Jan

Re: [PATCH v2] xen/iommu: fix usage of shared EPT/IOMMU page tables on PVH guests

[Julien Grall]

Hi Jan,

On 10/03/2015 13:35, Jan Beulich wrote:
> Ah, right, I mixed this up with iommu_hap_pt_share. Roger - looks
> like this could indeed be replaced by
>
>       if ( iommu_enabled && iommu_use_hap_pt(d) )
>           ops->share_p2m(d);
>
> and the corresponding check in VT-d and AMD Vi code could then
> also be dropped.

Royger seems to be away. As it break ARM support I will send a patch today.

Regards,

-- 
Julien Grall