Re: [Xen-devel] [PATCH v6 07/10] use is_iommu_enabled() where appropriate...

2019-08-29 Thread Jan Beulich 
On 16.08.2019 19:19, Paul Durrant wrote:
> ...rather than testing the global iommu_enabled flag and ops pointer.
> 
> Now that there is a per-domain flag indicating whether the domain is
> permitted to use the IOMMU (which determines whether the ops pointer will
> be set), many tests of the global iommu_enabled flag and ops pointer can
> be translated into tests of the per-domain flag. Some of the other tests of
> purely the global iommu_enabled flag can also be translated into tests of
> the per-domain flag.
> 
> NOTE: The comment in iommu_share_p2m_table() is also fixed; need_iommu()
>   disappeared some time ago. Also, whilst the style of the 'if' in
>   flask_iommu_resource_use_perm() is fixed, I have not translated any
>   instances of u32 into uint32_t to keep consistency. IMO such a
>   translation would be better done globally for the source module in
>   a separate patch.
>   The change in the initialization of the 'hd' variable in iommu_map()
>   and iommu_unmap() is done to keep the PV shim build happy. Without
>   this change it will fail to compile with errors of the form:
> 
> iommu.c:361:32: error: unused variable ‘hd’ [-Werror=unused-variable]
>  const struct domain_iommu *hd = dom_iommu(d);
>  ^~

Why would that be? Afaict there's no short-circuiting of
is_iommu_enabled() (albeit there could be, as we don't mean the shim
to have/use an IOMMU). Oh - I guess I see it: Instead of this change
what I think we want is for x86's iommu_call() to evaluate its 1st
argument. Otherwise we're liable to run into the same issue elsewhere
again.

> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -886,7 +886,7 @@ static int flask_map_domain_msi (struct domain *d, int 
> irq, const void *data,
>  #endif
>  }
>  
> -static u32 flask_iommu_resource_use_perm(void)
> +static u32 flask_iommu_resource_use_perm(struct domain *d)

const?

With these adjustments
Reviewed-by: Jan Beulich 

Jan

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v6 07/10] use is_iommu_enabled() where appropriate...

2019-08-29 Thread Paul Durrant 
> -Original Message-
> From: Roger Pau Monne 
> Sent: 23 August 2019 11:56
> To: Paul Durrant 
> Cc: xen-devel@lists.xenproject.org; Stefano Stabellini 
> ; Julien Grall
> ; Volodymyr Babchuk ; Jan 
> Beulich
> ; Andrew Cooper ; Wei Liu 
> ; Jun Nakajima
> ; Kevin Tian ; George Dunlap 
> ;
> Suravee Suthikulpanit ; Brian Woods 
> ; Daniel De
> Graaf 
> Subject: Re: [PATCH v6 07/10] use is_iommu_enabled() where appropriate...
> 
> On Fri, Aug 16, 2019 at 06:19:58PM +0100, Paul Durrant wrote:
> > ...rather than testing the global iommu_enabled flag and ops pointer.
> >
> > Now that there is a per-domain flag indicating whether the domain is
> > permitted to use the IOMMU (which determines whether the ops pointer will
> > be set), many tests of the global iommu_enabled flag and ops pointer can
> > be translated into tests of the per-domain flag. Some of the other tests of
> > purely the global iommu_enabled flag can also be translated into tests of
> > the per-domain flag.
> >
> > NOTE: The comment in iommu_share_p2m_table() is also fixed; need_iommu()
> >   disappeared some time ago. Also, whilst the style of the 'if' in
> >   flask_iommu_resource_use_perm() is fixed, I have not translated any
> >   instances of u32 into uint32_t to keep consistency. IMO such a
> >   translation would be better done globally for the source module in
> >   a separate patch.
> 
> I've usually changed those instances as the line gets modified anyway,
> but I'm not going to ask everyone to do this if they don't feel like
> it.
> 
> The problem with doing wholesale changes of u32 -> uint32_t is that
> you introduce a lot of noise when trying to use git blame afterwards
> for example. Doing it when touching the line for legitimate changes
> avoids the noise.
> 
> >   The change in the initialization of the 'hd' variable in iommu_map()
> >   and iommu_unmap() is done to keep the PV shim build happy. Without
> >   this change it will fail to compile with errors of the form:
> >
> > iommu.c:361:32: error: unused variable ‘hd’ [-Werror=unused-variable]
> >  const struct domain_iommu *hd = dom_iommu(d);
> >  ^~
> >
> > Signed-off-by: Paul Durrant 
> 
> Reviewed-by: Roger Pau Monné 
> 

Thanks.

> I haven't looked however if there are further cases where
> iommu_enabled should be changed into is_iommu_enabled.
> 

Jan had clearly checked on a previous review iteration because he spotted a 
couple of places I had missed. I'm reasonably confident I've found them all now.

  Paul

> > @@ -556,8 +560,8 @@ int iommu_do_domctl(
> >  {
> >  int ret = -ENODEV;
> >
> > -if ( !iommu_enabled )
> > -return -ENOSYS;
> > +if ( !is_iommu_enabled(d) )
> > +return -EOPNOTSUPP;
> 
> I would use ENOENT here, but I don't have a strong opinion. The
> hypercall is supported, it's just that there's no iommu for this
> domain.
> 
> Thanks, Roger.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v6 07/10] use is_iommu_enabled() where appropriate...

2019-08-23 Thread Roger Pau Monné 
On Fri, Aug 16, 2019 at 06:19:58PM +0100, Paul Durrant wrote:
> ...rather than testing the global iommu_enabled flag and ops pointer.
> 
> Now that there is a per-domain flag indicating whether the domain is
> permitted to use the IOMMU (which determines whether the ops pointer will
> be set), many tests of the global iommu_enabled flag and ops pointer can
> be translated into tests of the per-domain flag. Some of the other tests of
> purely the global iommu_enabled flag can also be translated into tests of
> the per-domain flag.
> 
> NOTE: The comment in iommu_share_p2m_table() is also fixed; need_iommu()
>   disappeared some time ago. Also, whilst the style of the 'if' in
>   flask_iommu_resource_use_perm() is fixed, I have not translated any
>   instances of u32 into uint32_t to keep consistency. IMO such a
>   translation would be better done globally for the source module in
>   a separate patch.

I've usually changed those instances as the line gets modified anyway,
but I'm not going to ask everyone to do this if they don't feel like
it.

The problem with doing wholesale changes of u32 -> uint32_t is that
you introduce a lot of noise when trying to use git blame afterwards
for example. Doing it when touching the line for legitimate changes
avoids the noise.

>   The change in the initialization of the 'hd' variable in iommu_map()
>   and iommu_unmap() is done to keep the PV shim build happy. Without
>   this change it will fail to compile with errors of the form:
> 
> iommu.c:361:32: error: unused variable ‘hd’ [-Werror=unused-variable]
>  const struct domain_iommu *hd = dom_iommu(d);
>  ^~
> 
> Signed-off-by: Paul Durrant 

Reviewed-by: Roger Pau Monné 

I haven't looked however if there are further cases where
iommu_enabled should be changed into is_iommu_enabled.

> @@ -556,8 +560,8 @@ int iommu_do_domctl(
>  {
>  int ret = -ENODEV;
>  
> -if ( !iommu_enabled )
> -return -ENOSYS;
> +if ( !is_iommu_enabled(d) )
> +return -EOPNOTSUPP;

I would use ENOENT here, but I don't have a strong opinion. The
hypercall is supported, it's just that there's no iommu for this
domain.

Thanks, Roger.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v6 07/10] use is_iommu_enabled() where appropriate...

2019-08-22 Thread Tian, Kevin 
> From: Paul Durrant [mailto:paul.durr...@citrix.com]
> Sent: Saturday, August 17, 2019 1:20 AM
> 
> ...rather than testing the global iommu_enabled flag and ops pointer.
> 
> Now that there is a per-domain flag indicating whether the domain is
> permitted to use the IOMMU (which determines whether the ops pointer will
> be set), many tests of the global iommu_enabled flag and ops pointer can
> be translated into tests of the per-domain flag. Some of the other tests of
> purely the global iommu_enabled flag can also be translated into tests of
> the per-domain flag.
> 
> NOTE: The comment in iommu_share_p2m_table() is also fixed;
> need_iommu()
>   disappeared some time ago. Also, whilst the style of the 'if' in
>   flask_iommu_resource_use_perm() is fixed, I have not translated any
>   instances of u32 into uint32_t to keep consistency. IMO such a
>   translation would be better done globally for the source module in
>   a separate patch.
>   The change in the initialization of the 'hd' variable in iommu_map()
>   and iommu_unmap() is done to keep the PV shim build happy. Without
>   this change it will fail to compile with errors of the form:
> 
> iommu.c:361:32: error: unused variable ‘hd’ [-Werror=unused-variable]
>  const struct domain_iommu *hd = dom_iommu(d);
>  ^~
> 
> Signed-off-by: Paul Durrant 

Reviewed-by: Kevin Tian 
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v6 07/10] use is_iommu_enabled() where appropriate...

2019-08-19 Thread Daniel De Graaf 

On 8/16/19 1:19 PM, Paul Durrant wrote:

...rather than testing the global iommu_enabled flag and ops pointer.

Now that there is a per-domain flag indicating whether the domain is
permitted to use the IOMMU (which determines whether the ops pointer will
be set), many tests of the global iommu_enabled flag and ops pointer can
be translated into tests of the per-domain flag. Some of the other tests of
purely the global iommu_enabled flag can also be translated into tests of
the per-domain flag.

NOTE: The comment in iommu_share_p2m_table() is also fixed; need_iommu()
   disappeared some time ago. Also, whilst the style of the 'if' in
   flask_iommu_resource_use_perm() is fixed, I have not translated any
   instances of u32 into uint32_t to keep consistency. IMO such a
   translation would be better done globally for the source module in
   a separate patch.
   The change in the initialization of the 'hd' variable in iommu_map()
   and iommu_unmap() is done to keep the PV shim build happy. Without
   this change it will fail to compile with errors of the form:

iommu.c:361:32: error: unused variable ‘hd’ [-Werror=unused-variable]
  const struct domain_iommu *hd = dom_iommu(d);
  ^~

Signed-off-by: Paul Durrant 


Acked-by: Daniel De Graaf 


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v6 07/10] use is_iommu_enabled() where appropriate...

2019-08-16 Thread Paul Durrant 
...rather than testing the global iommu_enabled flag and ops pointer.

Now that there is a per-domain flag indicating whether the domain is
permitted to use the IOMMU (which determines whether the ops pointer will
be set), many tests of the global iommu_enabled flag and ops pointer can
be translated into tests of the per-domain flag. Some of the other tests of
purely the global iommu_enabled flag can also be translated into tests of
the per-domain flag.

NOTE: The comment in iommu_share_p2m_table() is also fixed; need_iommu()
  disappeared some time ago. Also, whilst the style of the 'if' in
  flask_iommu_resource_use_perm() is fixed, I have not translated any
  instances of u32 into uint32_t to keep consistency. IMO such a
  translation would be better done globally for the source module in
  a separate patch.
  The change in the initialization of the 'hd' variable in iommu_map()
  and iommu_unmap() is done to keep the PV shim build happy. Without
  this change it will fail to compile with errors of the form:

iommu.c:361:32: error: unused variable ‘hd’ [-Werror=unused-variable]
 const struct domain_iommu *hd = dom_iommu(d);
 ^~

Signed-off-by: Paul Durrant 
---
Cc: Stefano Stabellini 
Cc: Julien Grall 
Cc: Volodymyr Babchuk 
Cc: Jan Beulich 
Cc: Andrew Cooper 
Cc: Wei Liu 
Cc: "Roger Pau Monné" 
Cc: Jun Nakajima 
Cc: Kevin Tian 
Cc: George Dunlap 
Cc: Suravee Suthikulpanit 
Cc: Brian Woods 
Cc: Daniel De Graaf 

Previously part of series 
https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02267.html

v5:
 - Fix logic in ARM p2m_init()
 - Make iommu_do_domctl() return -EOPNOTSUPP rather than -ENOSYS if the
   IOMMU is not enabled
 - Fix test in pci_enable_acs()
 - Fix test in flask_iommu_resource_use_perm()
---
 xen/arch/arm/p2m.c|  2 +-
 xen/arch/x86/dom0_build.c |  2 +-
 xen/arch/x86/domctl.c |  4 +--
 xen/arch/x86/hvm/hvm.c|  6 ++--
 xen/arch/x86/hvm/vioapic.c|  2 +-
 xen/arch/x86/hvm/vmx/vmcs.c   |  2 +-
 xen/arch/x86/hvm/vmx/vmx.c|  2 +-
 xen/arch/x86/mm/p2m-ept.c |  4 +--
 xen/drivers/passthrough/amd/iommu_guest.c |  2 +-
 xen/drivers/passthrough/device_tree.c |  4 +--
 xen/drivers/passthrough/io.c  |  8 ++---
 xen/drivers/passthrough/iommu.c   | 39 ---
 xen/drivers/passthrough/pci.c | 16 +-
 xen/drivers/passthrough/vtd/iommu.c   |  2 +-
 xen/drivers/passthrough/vtd/x86/hvm.c |  2 +-
 xen/drivers/passthrough/x86/iommu.c   |  2 +-
 xen/xsm/flask/hooks.c | 18 +--
 17 files changed, 59 insertions(+), 58 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index e28ea1c85a..7f1442932a 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1531,7 +1531,7 @@ int p2m_init(struct domain *d)
  * shared with the CPU, Xen has to make sure that the PT changes have
  * reached the memory
  */
-p2m->clean_pte = iommu_enabled &&
+p2m->clean_pte = is_iommu_enabled(d) &&
 !iommu_has_feature(d, IOMMU_FEAT_COHERENT_WALK);
 
 rc = p2m_alloc_table(d);
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index c69570920c..d381784edd 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -356,7 +356,7 @@ unsigned long __init dom0_compute_nr_pages(
 avail -= d->max_vcpus - 1;
 
 /* Reserve memory for iommu_dom0_init() (rough estimate). */
-if ( iommu_enabled )
+if ( is_iommu_enabled(d) )
 {
 unsigned int s;
 
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 2d45e5b8a8..be4b206068 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -715,7 +715,7 @@ long arch_do_domctl(
 break;
 
 ret = -ESRCH;
-if ( iommu_enabled )
+if ( is_iommu_enabled(d) )
 {
 pcidevs_lock();
 ret = pt_irq_create_bind(d, bind);
@@ -744,7 +744,7 @@ long arch_do_domctl(
 if ( ret )
 break;
 
-if ( iommu_enabled )
+if ( is_iommu_enabled(d) )
 {
 pcidevs_lock();
 ret = pt_irq_destroy_bind(d, bind);
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 029eea3b85..172c860acc 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -465,7 +465,7 @@ void hvm_migrate_timers(struct vcpu *v)
 
 void hvm_migrate_pirq(struct hvm_pirq_dpci *pirq_dpci, const struct vcpu *v)
 {
-ASSERT(iommu_enabled &&
+ASSERT(is_iommu_enabled(v->domain) &&
(is_hardware_domain(v->domain) || hvm_domain_irq(v->domain)->dpci));
 
 if ( (pirq_dpci->flags & HVM_IRQ_DPCI_MACH_MSI) &&
@@ -496,7 +496,7 @@ void hvm_migrate_pirqs(struct vcpu *v)
 {
 struct domain *d = v->domain;
 
-if ( !iommu_enabled || !hvm_domain_irq(d)->dpci )
+if