[PATCH V2 0/2] Fix xen crash when starting HVM guest due to missing io handler

[PATCH V2 0/2] Fix xen crash when starting HVM guest due to missing io handler

[suravee.suthikulpanit]

From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>

Hi All,

On systems with iommu v2 enabled, the hypervisor crashes when trying
to start up an HVM guest. 

Investigating shows that the guest_iommu_init() is called before the
HVM domain is initialized. It then tries to register_mmio_handler()
causing the hvm_next_io_handler() to increment the io_handler_count.
However, the registration fails silently and left the I/O handler
uninitialized.

At later time, hvm_find_io_handler() is called and iterate through
the registered handlered, but then resulting in referencing NULL
pointers.

This patch series proposes fix for this issue.

NOTE: For patch 2, since guest IOMMU emulation is still incompleted,
this change is tentative and will be verified in the future. Alterantively,
I can just simply remove the guest_iommu_init()/destroy() for now.
I will be also looking at re-enabling this feature in Xen.

Changes from V1:
  * Fix upper bound NR_IO_HANDLERS check for in patch 1.

Thanks,
Suravee

Suravee Suthikulpanit (2):
  x86/hvm: Add check when register io handler
  svm: iommu: Only call guest_iommu_init() after initialized HVM domain

 xen/arch/x86/hvm/intercept.c                |  6 +++++-
 xen/arch/x86/hvm/svm/svm.c                  | 10 ++++++++++
 xen/drivers/passthrough/amd/iommu_guest.c   |  6 ++++++
 xen/drivers/passthrough/amd/pci_amd_iommu.c |  4 ----
 4 files changed, 21 insertions(+), 5 deletions(-)

-- 
1.9.1


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

[PATCH V2 1/2] x86/hvm: Add check when register io handler

[suravee.suthikulpanit]

From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>

At the time of registering HVM I/O handler, the HVM domain might
not have been initialized, which means the hvm_domain.io_handler
would be NULL. In the hvm_next_io_handler(), this should be checked
before returning and referencing the array. Also, the io_handler_count
should only be incremented on success.

So, this patch adds error handling in hvm_next_io_handler.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 xen/arch/x86/hvm/intercept.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/intercept.c b/xen/arch/x86/hvm/intercept.c
index 7096d74..b837f5f 100644
--- a/xen/arch/x86/hvm/intercept.c
+++ b/xen/arch/x86/hvm/intercept.c
@@ -248,7 +248,10 @@ int hvm_io_intercept(ioreq_t *p)
 
 struct hvm_io_handler *hvm_next_io_handler(struct domain *d)
 {
-    unsigned int i = d->arch.hvm_domain.io_handler_count++;
+    unsigned int i = d->arch.hvm_domain.io_handler_count;
+
+    if ( !d->arch.hvm_domain.io_handler )
+        return NULL;
 
     if ( i == NR_IO_HANDLERS )
     {
@@ -256,6 +259,7 @@ struct hvm_io_handler *hvm_next_io_handler(struct domain *d)
         return NULL;
     }
 
+    d->arch.hvm_domain.io_handler_count++;
     return &d->arch.hvm_domain.io_handler[i];
 }
 
-- 
1.9.1


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

[PATCH V2 2/2] svm: iommu: Only call guest_iommu_init() after initialized HVM domain

[suravee.suthikulpanit]

From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>

The guest_iommu_init() is currently called by the following code path:

    arch/x86/domain.c: arch_domain_create()
      ]- drivers/passthrough/iommu.c: iommu_domain_init()
        |- drivers/passthrough/amd/pci_amd_iommu.c: amd_iommu_domain_init();
          |- drivers/passthrough/amd/iommu_guest.c: guest_iommu_init()

At this point, the hvm_domain_initialised() has not been
called. So register_mmio_handler(), in guest_iommu_init(), silently fails.
This patch moves the call to guest_iommu_init/destroy() into
the svm_domain_intialise/_destroy() instead.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 xen/arch/x86/hvm/svm/svm.c                  | 10 ++++++++++
 xen/drivers/passthrough/amd/iommu_guest.c   |  6 ++++++
 xen/drivers/passthrough/amd/pci_amd_iommu.c |  4 ----
 3 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index e62dfa1..0c4affc 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -45,6 +45,7 @@
 #include <asm/hvm/support.h>
 #include <asm/hvm/io.h>
 #include <asm/hvm/emulate.h>
+#include <asm/hvm/svm/amd-iommu-proto.h>
 #include <asm/hvm/svm/asid.h>
 #include <asm/hvm/svm/svm.h>
 #include <asm/hvm/svm/vmcb.h>
@@ -1176,11 +1177,20 @@ void svm_host_osvw_init()
 
 static int svm_domain_initialise(struct domain *d)
 {
+    if ( is_hvm_domain(d) )
+        /*
+         * This requires the hvm domain to be
+         * initialized first.
+         */
+        return guest_iommu_init(d);
+
     return 0;
 }
 
 static void svm_domain_destroy(struct domain *d)
 {
+    if ( is_hvm_domain(d) )
+        guest_iommu_destroy(d);
 }
 
 static int svm_vcpu_initialise(struct vcpu *v)
diff --git a/xen/drivers/passthrough/amd/iommu_guest.c b/xen/drivers/passthrough/amd/iommu_guest.c
index b4e75ac..9f26765 100644
--- a/xen/drivers/passthrough/amd/iommu_guest.c
+++ b/xen/drivers/passthrough/amd/iommu_guest.c
@@ -891,6 +891,12 @@ int guest_iommu_init(struct domain* d)
          !has_viommu(d) )
         return 0;
 
+    if ( d->arch.hvm_domain.io_handler == NULL )
+    {
+        AMD_IOMMU_DEBUG("Error: uninitalized hvm io handler\n");
+        return 1;
+    }
+
     iommu = xzalloc(struct guest_iommu);
     if ( !iommu )
     {
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index c1c0b6b..f791618 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -274,9 +274,6 @@ static int amd_iommu_domain_init(struct domain *d)
     hd->arch.paging_mode = is_hvm_domain(d) ?
                       IOMMU_PAGING_MODE_LEVEL_2 :
                       get_paging_mode(max_page);
-
-    guest_iommu_init(d);
-
     return 0;
 }
 
@@ -476,7 +473,6 @@ static void deallocate_iommu_page_tables(struct domain *d)
 
 static void amd_iommu_domain_destroy(struct domain *d)
 {
-    guest_iommu_destroy(d);
     deallocate_iommu_page_tables(d);
     amd_iommu_flush_all_pages(d);
 }
-- 
1.9.1


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

Re: [PATCH V2 1/2] x86/hvm: Add check when register io handler

[Paul Durrant]

> -----Original Message-----
> From: suravee.suthikulpanit@amd.com
> [mailto:suravee.suthikulpanit@amd.com]
> Sent: 13 May 2016 20:37
> To: xen-devel@lists.xen.org; George Dunlap; jbeulich@suse.com
> Cc: Paul Durrant; Suravee Suthikulpanit; Suravee Suthikulpanit
> Subject: [PATCH V2 1/2] x86/hvm: Add check when register io handler
> 
> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> 
> At the time of registering HVM I/O handler, the HVM domain might
> not have been initialized,

I/O handler registration is internal to Xen so any caller that attempt to register before domain initialization should be removed and replaced with one that does it at the right time.

> which means the hvm_domain.io_handler
> would be NULL. In the hvm_next_io_handler(), this should be checked
> before returning and referencing the array. Also, the io_handler_count
> should only be incremented on success.
> 
> So, this patch adds error handling in hvm_next_io_handler.
> 

This isn't necessary. An ASSERT would be preferable so that buggy callers can be easily caught.

  Paul

> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  xen/arch/x86/hvm/intercept.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hvm/intercept.c b/xen/arch/x86/hvm/intercept.c
> index 7096d74..b837f5f 100644
> --- a/xen/arch/x86/hvm/intercept.c
> +++ b/xen/arch/x86/hvm/intercept.c
> @@ -248,7 +248,10 @@ int hvm_io_intercept(ioreq_t *p)
> 
>  struct hvm_io_handler *hvm_next_io_handler(struct domain *d)
>  {
> -    unsigned int i = d->arch.hvm_domain.io_handler_count++;
> +    unsigned int i = d->arch.hvm_domain.io_handler_count;
> +
> +    if ( !d->arch.hvm_domain.io_handler )
> +        return NULL;
> 
>      if ( i == NR_IO_HANDLERS )
>      {
> @@ -256,6 +259,7 @@ struct hvm_io_handler *hvm_next_io_handler(struct
> domain *d)
>          return NULL;
>      }
> 
> +    d->arch.hvm_domain.io_handler_count++;
>      return &d->arch.hvm_domain.io_handler[i];
>  }
> 
> --
> 1.9.1


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

Re: [PATCH V2 2/2] svm: iommu: Only call guest_iommu_init() after initialized HVM domain

[Paul Durrant]

> -----Original Message-----
> From: suravee.suthikulpanit@amd.com
> [mailto:suravee.suthikulpanit@amd.com]
> Sent: 13 May 2016 20:37
> To: xen-devel@lists.xen.org; George Dunlap; jbeulich@suse.com
> Cc: Paul Durrant; Suravee Suthikulpanit; Suravee Suthikulpanit
> Subject: [PATCH V2 2/2] svm: iommu: Only call guest_iommu_init() after
> initialized HVM domain
> 
> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> 
> The guest_iommu_init() is currently called by the following code path:
> 
>     arch/x86/domain.c: arch_domain_create()
>       ]- drivers/passthrough/iommu.c: iommu_domain_init()
>         |- drivers/passthrough/amd/pci_amd_iommu.c:
> amd_iommu_domain_init();
>           |- drivers/passthrough/amd/iommu_guest.c: guest_iommu_init()
> 
> At this point, the hvm_domain_initialised() has not been
> called. So register_mmio_handler(), in guest_iommu_init(), silently fails.
> This patch moves the call to guest_iommu_init/destroy() into
> the svm_domain_intialise/_destroy() instead.
> 

That seems wrong. You're taking a call that currently comes via a jump table, i.e. an abstraction layer, and calling it directly. Is it possible, instead, to move the call to iommu_domain_init() later in arch_domain_create()? It seems odd, to me at least, that it's done before hvm_domain_initialise() anyway.

  Paul

> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  xen/arch/x86/hvm/svm/svm.c                  | 10 ++++++++++
>  xen/drivers/passthrough/amd/iommu_guest.c   |  6 ++++++
>  xen/drivers/passthrough/amd/pci_amd_iommu.c |  4 ----
>  3 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index e62dfa1..0c4affc 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -45,6 +45,7 @@
>  #include <asm/hvm/support.h>
>  #include <asm/hvm/io.h>
>  #include <asm/hvm/emulate.h>
> +#include <asm/hvm/svm/amd-iommu-proto.h>
>  #include <asm/hvm/svm/asid.h>
>  #include <asm/hvm/svm/svm.h>
>  #include <asm/hvm/svm/vmcb.h>
> @@ -1176,11 +1177,20 @@ void svm_host_osvw_init()
> 
>  static int svm_domain_initialise(struct domain *d)
>  {
> +    if ( is_hvm_domain(d) )
> +        /*
> +         * This requires the hvm domain to be
> +         * initialized first.
> +         */
> +        return guest_iommu_init(d);
> +
>      return 0;
>  }
> 
>  static void svm_domain_destroy(struct domain *d)
>  {
> +    if ( is_hvm_domain(d) )
> +        guest_iommu_destroy(d);
>  }
> 
>  static int svm_vcpu_initialise(struct vcpu *v)
> diff --git a/xen/drivers/passthrough/amd/iommu_guest.c
> b/xen/drivers/passthrough/amd/iommu_guest.c
> index b4e75ac..9f26765 100644
> --- a/xen/drivers/passthrough/amd/iommu_guest.c
> +++ b/xen/drivers/passthrough/amd/iommu_guest.c
> @@ -891,6 +891,12 @@ int guest_iommu_init(struct domain* d)
>           !has_viommu(d) )
>          return 0;
> 
> +    if ( d->arch.hvm_domain.io_handler == NULL )
> +    {
> +        AMD_IOMMU_DEBUG("Error: uninitalized hvm io handler\n");
> +        return 1;
> +    }
> +
>      iommu = xzalloc(struct guest_iommu);
>      if ( !iommu )
>      {
> diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> index c1c0b6b..f791618 100644
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -274,9 +274,6 @@ static int amd_iommu_domain_init(struct domain *d)
>      hd->arch.paging_mode = is_hvm_domain(d) ?
>                        IOMMU_PAGING_MODE_LEVEL_2 :
>                        get_paging_mode(max_page);
> -
> -    guest_iommu_init(d);
> -
>      return 0;
>  }
> 
> @@ -476,7 +473,6 @@ static void deallocate_iommu_page_tables(struct
> domain *d)
> 
>  static void amd_iommu_domain_destroy(struct domain *d)
>  {
> -    guest_iommu_destroy(d);
>      deallocate_iommu_page_tables(d);
>      amd_iommu_flush_all_pages(d);
>  }
> --
> 1.9.1


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

Re: [PATCH V2 1/2] x86/hvm: Add check when register io handler

[Suravee Suthikulpanit]

Hi Paul,

On 05/16/2016 03:01 AM, Paul Durrant wrote:
>> -----Original Message-----
>> >From:suravee.suthikulpanit@amd.com
>> >[mailto:suravee.suthikulpanit@amd.com]
>> >Sent: 13 May 2016 20:37
>> >To:xen-devel@lists.xen.org; George Dunlap;jbeulich@suse.com
>> >Cc: Paul Durrant; Suravee Suthikulpanit; Suravee Suthikulpanit
>> >Subject: [PATCH V2 1/2] x86/hvm: Add check when register io handler
>> >
>> >From: Suravee Suthikulpanit<Suravee.Suthikulpanit@amd.com>
>> >
>> >At the time of registering HVM I/O handler, the HVM domain might
>> >not have been initialized,
> I/O handler registration is internal to Xen so any caller that attempt to register before domain initialization should be removed and replaced with one that does it at the right time.

Ok. I'll just remove that call for now.

>> >which means the hvm_domain.io_handler
>> >would be NULL. In the hvm_next_io_handler(), this should be checked
>> >before returning and referencing the array. Also, the io_handler_count
>> >should only be incremented on success.
>> >
>> >So, this patch adds error handling in hvm_next_io_handler.
>> >
> This isn't necessary. An ASSERT would be preferable so that buggy callers can be easily caught.

Ok, I'll update the patch to ASSERT() and send it out. Although, just 
want to make sure that you think it should really be doing assert and 
not warning + handling error? It seems quite aggressive to crash the 
hypervisor simply because some io handler are not properly call.

Thanks,
Suravee

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

Re: [PATCH V2 1/2] x86/hvm: Add check when register io handler

[Paul Durrant]

> -----Original Message-----
> From: Suravee Suthikulpanit [mailto:Suravee.Suthikulpanit@amd.com]
> Sent: 16 May 2016 17:03
> To: Paul Durrant; xen-devel@lists.xen.org; George Dunlap;
> jbeulich@suse.com
> Subject: Re: [PATCH V2 1/2] x86/hvm: Add check when register io handler
> 
> Hi Paul,
> 
> On 05/16/2016 03:01 AM, Paul Durrant wrote:
> >> -----Original Message-----
> >> >From:suravee.suthikulpanit@amd.com
> >> >[mailto:suravee.suthikulpanit@amd.com]
> >> >Sent: 13 May 2016 20:37
> >> >To:xen-devel@lists.xen.org; George Dunlap;jbeulich@suse.com
> >> >Cc: Paul Durrant; Suravee Suthikulpanit; Suravee Suthikulpanit
> >> >Subject: [PATCH V2 1/2] x86/hvm: Add check when register io handler
> >> >
> >> >From: Suravee Suthikulpanit<Suravee.Suthikulpanit@amd.com>
> >> >
> >> >At the time of registering HVM I/O handler, the HVM domain might
> >> >not have been initialized,
> > I/O handler registration is internal to Xen so any caller that attempt to
> register before domain initialization should be removed and replaced with
> one that does it at the right time.
> 
> Ok. I'll just remove that call for now.
> 
> >> >which means the hvm_domain.io_handler
> >> >would be NULL. In the hvm_next_io_handler(), this should be checked
> >> >before returning and referencing the array. Also, the io_handler_count
> >> >should only be incremented on success.
> >> >
> >> >So, this patch adds error handling in hvm_next_io_handler.
> >> >
> > This isn't necessary. An ASSERT would be preferable so that buggy callers
> can be easily caught.
> 
> Ok, I'll update the patch to ASSERT() and send it out. Although, just
> want to make sure that you think it should really be doing assert and
> not warning + handling error? It seems quite aggressive to crash the
> hypervisor simply because some io handler are not properly call.
> 

That's a reasonable argument, but IMO since all the callers are in the hypervisor and ASSERTs only affect debug builds I think it's reasonable.

  Paul

> Thanks,
> Suravee

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

Re: [PATCH V2 2/2] svm: iommu: Only call guest_iommu_init() after initialized HVM domain

[Suravee Suthikulpanit]

+ Keir (since he added this code originally).

On 05/16/2016 03:19 AM, Paul Durrant wrote:
>> -----Original Message-----
>> >From:suravee.suthikulpanit@amd.com
>> >[mailto:suravee.suthikulpanit@amd.com]
>> >Sent: 13 May 2016 20:37
>> >To:xen-devel@lists.xen.org; George Dunlap;jbeulich@suse.com
>> >Cc: Paul Durrant; Suravee Suthikulpanit; Suravee Suthikulpanit
>> >Subject: [PATCH V2 2/2] svm: iommu: Only call guest_iommu_init() after
>> >initialized HVM domain
>> >
>> >From: Suravee Suthikulpanit<Suravee.Suthikulpanit@amd.com>
>> >
>> >The guest_iommu_init() is currently called by the following code path:
>> >
>> >     arch/x86/domain.c: arch_domain_create()
>> >       ]- drivers/passthrough/iommu.c: iommu_domain_init()
>> >         |- drivers/passthrough/amd/pci_amd_iommu.c:
>> >amd_iommu_domain_init();
>> >           |- drivers/passthrough/amd/iommu_guest.c: guest_iommu_init()
>> >
>> >At this point, the hvm_domain_initialised() has not been
>> >called. So register_mmio_handler(), in guest_iommu_init(), silently fails.
>> >This patch moves the call to guest_iommu_init/destroy() into
>> >the svm_domain_intialise/_destroy() instead.
>> >
> That seems wrong. You're taking a call that currently comes via a jump table, i.e. an abstraction layer, and calling it directly. Is it possible, instead, to move the call to iommu_domain_init() later in arch_domain_create()? It seems odd, to me at least, that it's done before hvm_domain_initialise() anyway.
>
>    Paul
>

Good point. I think I should be able to move iommu_domain_init() call to 
go after hvm_domain_initialise() as the following.

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 9d43f7b..ac289b6 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -625,24 +625,21 @@ int arch_domain_create(struct domain *d, unsigned 
int domcr_flags,

          if ( (rc = init_domain_irq_mapping(d)) != 0 )
              goto fail;
-
-        if ( (rc = iommu_domain_init(d)) != 0 )
-            goto fail;
      }
      spin_lock_init(&d->arch.e820_lock);

      if ( has_hvm_container_domain(d) )
      {
          if ( (rc = hvm_domain_initialise(d)) != 0 )
-        {
-            iommu_domain_destroy(d);
              goto fail;
-        }
      }
      else
          /* 64-bit PV guest by default. */
          d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;

+    if ( !is_idle_domain(d) && (rc = iommu_domain_init(d)) != 0 )
+        goto fail;
+
      if ( (rc = psr_domain_init(d)) != 0 )
          goto fail;

-----

This was added in the commit 66a882392272346ce1d0bc5a26568894f450a7c0,
and only says initialization cleanup and bugfix. I am not sure what bug 
was reported at the time. Anyone has an idea?

Suravee

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

Re: [PATCH V2 2/2] svm: iommu: Only call guest_iommu_init() after initialized HVM domain

[Jan Beulich]

>>> Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> 05/19/16 7:22 AM >>>
>On 05/16/2016 03:19 AM, Paul Durrant wrote:
>> >From:suravee.suthikulpanit@amd.com
>> >Sent: 13 May 2016 20:37
>>> >The guest_iommu_init() is currently called by the following code path:
>>> >
>>> >     arch/x86/domain.c: arch_domain_create()
>>> >       ]- drivers/passthrough/iommu.c: iommu_domain_init()
>>> >         |- drivers/passthrough/amd/pci_amd_iommu.c:
>>> >amd_iommu_domain_init();
>>> >           |- drivers/passthrough/amd/iommu_guest.c: guest_iommu_init()
>>> >
>>> >At this point, the hvm_domain_initialised() has not been
>>> >called. So register_mmio_handler(), in guest_iommu_init(), silently fails.
>>> >This patch moves the call to guest_iommu_init/destroy() into
>>> >the svm_domain_intialise/_destroy() instead.
>>> >
>> That seems wrong. You're taking a call that currently comes via a jump table, i.e. an abstraction layer, and calling it directly. Is it possible, instead, to move the call to iommu_domain_init() later in arch_domain_create()? It seems odd, to me at least, that it's done before hvm_domain_initialise() anyway.
>
>Good point. I think I should be able to move iommu_domain_init() call to 
>go after hvm_domain_initialise() as the following.
>
>--- a/xen/arch/x86/domain.c
>+++ b/xen/arch/x86/domain.c
>@@ -625,24 +625,21 @@ int arch_domain_create(struct domain *d, unsigned 
>int domcr_flags,
>
>if ( (rc = init_domain_irq_mapping(d)) != 0 )
>goto fail;
>-
>-        if ( (rc = iommu_domain_init(d)) != 0 )
>-            goto fail;
>}
>spin_lock_init(&d->arch.e820_lock);
>
>if ( has_hvm_container_domain(d) )
>{
>if ( (rc = hvm_domain_initialise(d)) != 0 )
>-        {
>-            iommu_domain_destroy(d);
>goto fail;
>-        }
>}
      >else
>/* 64-bit PV guest by default. */
>d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
>
>+    if ( !is_idle_domain(d) && (rc = iommu_domain_init(d)) != 0 )
>+        goto fail;

This would in the error case fail to undo what hvm_domain_initialise() did.
There was a fix very recently dealing with a similar issue. There really
shouldn't be anything that can fail after hvm_domain_initialise(). And I also
can't see why both of you think iommu_domain_init() has to come later -
that's a function affecting not just HVM guests.

Jan


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

Re: [PATCH V2 2/2] svm: iommu: Only call guest_iommu_init() after initialized HVM domain

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:jbeulich@suse.com]
> Sent: 19 May 2016 07:04
> To: Suravee.Suthikulpanit@amd.com; Paul Durrant
> Cc: George Dunlap; xen-devel@lists.xen.org; Keir (Xen.org)
> Subject: Re: [PATCH V2 2/2] svm: iommu: Only call guest_iommu_init() after
> initialized HVM domain
> 
> >>> Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> 05/19/16 7:22
> AM >>>
> >On 05/16/2016 03:19 AM, Paul Durrant wrote:
> >> >From:suravee.suthikulpanit@amd.com
> >> >Sent: 13 May 2016 20:37
> >>> >The guest_iommu_init() is currently called by the following code path:
> >>> >
> >>> >     arch/x86/domain.c: arch_domain_create()
> >>> >       ]- drivers/passthrough/iommu.c: iommu_domain_init()
> >>> >         |- drivers/passthrough/amd/pci_amd_iommu.c:
> >>> >amd_iommu_domain_init();
> >>> >           |- drivers/passthrough/amd/iommu_guest.c: guest_iommu_init()
> >>> >
> >>> >At this point, the hvm_domain_initialised() has not been
> >>> >called. So register_mmio_handler(), in guest_iommu_init(), silently
> fails.
> >>> >This patch moves the call to guest_iommu_init/destroy() into
> >>> >the svm_domain_intialise/_destroy() instead.
> >>> >
> >> That seems wrong. You're taking a call that currently comes via a jump
> table, i.e. an abstraction layer, and calling it directly. Is it possible, instead, to
> move the call to iommu_domain_init() later in arch_domain_create()? It
> seems odd, to me at least, that it's done before hvm_domain_initialise()
> anyway.
> >
> >Good point. I think I should be able to move iommu_domain_init() call to
> >go after hvm_domain_initialise() as the following.
> >
> >--- a/xen/arch/x86/domain.c
> >+++ b/xen/arch/x86/domain.c
> >@@ -625,24 +625,21 @@ int arch_domain_create(struct domain *d,
> unsigned
> >int domcr_flags,
> >
> >if ( (rc = init_domain_irq_mapping(d)) != 0 )
> >goto fail;
> >-
> >-        if ( (rc = iommu_domain_init(d)) != 0 )
> >-            goto fail;
> >}
> >spin_lock_init(&d->arch.e820_lock);
> >
> >if ( has_hvm_container_domain(d) )
> >{
> >if ( (rc = hvm_domain_initialise(d)) != 0 )
> >-        {
> >-            iommu_domain_destroy(d);
> >goto fail;
> >-        }
> >}
>       >else
> >/* 64-bit PV guest by default. */
> >d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
> >
> >+    if ( !is_idle_domain(d) && (rc = iommu_domain_init(d)) != 0 )
> >+        goto fail;
> 
> This would in the error case fail to undo what hvm_domain_initialise() did.
> There was a fix very recently dealing with a similar issue. There really
> shouldn't be anything that can fail after hvm_domain_initialise().

Why is that? There are many failure paths within hvm_domain_initialise(). What's wrong with calling hvm_domain_destroy() to undo the whole thing? (Although I do notice that the io_bitmap would appear to leak in that case... but that looks like a bug to me).

> And I also
> can't see why both of you think iommu_domain_init() has to come later -
> that's a function affecting not just HVM guests.
> 

Yes, I realise that. But the problem is that, in the HVM case, it calls functions that make use of infrastructure that's initialized by hvm_domain_initialise().

  Paul

> Jan


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

Re: [PATCH V2 2/2] svm: iommu: Only call guest_iommu_init() after initialized HVM domain

[Jan Beulich]

>>> On 19.05.16 at 09:52, <Paul.Durrant@citrix.com> wrote:
>> >--- a/xen/arch/x86/domain.c
>> >+++ b/xen/arch/x86/domain.c
>> >@@ -625,24 +625,21 @@ int arch_domain_create(struct domain *d,
>> unsigned
>> >int domcr_flags,
>> >
>> >if ( (rc = init_domain_irq_mapping(d)) != 0 )
>> >goto fail;
>> >-
>> >-        if ( (rc = iommu_domain_init(d)) != 0 )
>> >-            goto fail;
>> >}
>> >spin_lock_init(&d->arch.e820_lock);
>> >
>> >if ( has_hvm_container_domain(d) )
>> >{
>> >if ( (rc = hvm_domain_initialise(d)) != 0 )
>> >-        {
>> >-            iommu_domain_destroy(d);
>> >goto fail;
>> >-        }
>> >}
>>       >else
>> >/* 64-bit PV guest by default. */
>> >d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
>> >
>> >+    if ( !is_idle_domain(d) && (rc = iommu_domain_init(d)) != 0 )
>> >+        goto fail;
>> 
>> This would in the error case fail to undo what hvm_domain_initialise() did.
>> There was a fix very recently dealing with a similar issue. There really
>> shouldn't be anything that can fail after hvm_domain_initialise().
> 
> Why is that? There are many failure paths within hvm_domain_initialise(). 
> What's wrong with calling hvm_domain_destroy() to undo the whole thing? 

Well, I simply didn't check whether hvm_domain_destroy() would be
suitable to be called in that case, but yes, it looks like it would be.

> (Although I do notice that the io_bitmap would appear to leak in that case... 
> but that looks like a bug to me).

That's a hardware domain only aspect, and failure to construct
the hardware domain would be fatal to the system anyway.

Jan


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

Re: [PATCH V2 2/2] svm: iommu: Only call guest_iommu_init() after initialized HVM domain

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 19 May 2016 10:00
> To: Paul Durrant
> Cc: Suravee.Suthikulpanit@amd.com; George Dunlap; xen-
> devel@lists.xen.org; Keir (Xen.org)
> Subject: RE: [PATCH V2 2/2] svm: iommu: Only call guest_iommu_init() after
> initialized HVM domain
> 
> >>> On 19.05.16 at 09:52, <Paul.Durrant@citrix.com> wrote:
> >> >--- a/xen/arch/x86/domain.c
> >> >+++ b/xen/arch/x86/domain.c
> >> >@@ -625,24 +625,21 @@ int arch_domain_create(struct domain *d,
> >> unsigned
> >> >int domcr_flags,
> >> >
> >> >if ( (rc = init_domain_irq_mapping(d)) != 0 )
> >> >goto fail;
> >> >-
> >> >-        if ( (rc = iommu_domain_init(d)) != 0 )
> >> >-            goto fail;
> >> >}
> >> >spin_lock_init(&d->arch.e820_lock);
> >> >
> >> >if ( has_hvm_container_domain(d) )
> >> >{
> >> >if ( (rc = hvm_domain_initialise(d)) != 0 )
> >> >-        {
> >> >-            iommu_domain_destroy(d);
> >> >goto fail;
> >> >-        }
> >> >}
> >>       >else
> >> >/* 64-bit PV guest by default. */
> >> >d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
> >> >
> >> >+    if ( !is_idle_domain(d) && (rc = iommu_domain_init(d)) != 0 )
> >> >+        goto fail;
> >>
> >> This would in the error case fail to undo what hvm_domain_initialise() did.
> >> There was a fix very recently dealing with a similar issue. There really
> >> shouldn't be anything that can fail after hvm_domain_initialise().
> >
> > Why is that? There are many failure paths within hvm_domain_initialise().
> > What's wrong with calling hvm_domain_destroy() to undo the whole
> thing?
> 
> Well, I simply didn't check whether hvm_domain_destroy() would be
> suitable to be called in that case, but yes, it looks like it would be.
> 
> > (Although I do notice that the io_bitmap would appear to leak in that
> case...
> > but that looks like a bug to me).
> 
> That's a hardware domain only aspect, and failure to construct
> the hardware domain would be fatal to the system anyway.
>

Ok. No problem then.

  Paul

 > Jan


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