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

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

[suravee.suthikulpanit]

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

Hi All,

Changes from V2:
  * Use assert instead of sanity check before count increment in
    the hvm_next_io_handler().
  * Post-pone iommu_domain_init() and add proper error handling code
    to destroy hvm in case of failure.
  * Split out sanity check in guest_iommu_init() into a separate patch.

OVERVIEW:
 
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.

Thanks,
Suravee

Suravee Suthikulpanit (3):
  x86/hvm: Add check when register io handler
  svm: iommu: Only call guest_iommu_init() after initialized HVM domain
  AMD IOMMU: Check io_handler before registering mmio handler

 xen/arch/x86/domain.c                     | 9 ++++++---
 xen/arch/x86/hvm/intercept.c              | 2 ++
 xen/drivers/passthrough/amd/iommu_guest.c | 6 ++++++
 3 files changed, 14 insertions(+), 3 deletions(-)

-- 
1.9.1


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

[PATCH v3 1/3] 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 asserted.

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

diff --git a/xen/arch/x86/hvm/intercept.c b/xen/arch/x86/hvm/intercept.c
index fc757d0..2f8d57f 100644
--- a/xen/arch/x86/hvm/intercept.c
+++ b/xen/arch/x86/hvm/intercept.c
@@ -258,6 +258,8 @@ struct hvm_io_handler *hvm_next_io_handler(struct domain *d)
 {
     unsigned int i = d->arch.hvm_domain.io_handler_count++;
 
+    ASSERT( d->arch.hvm_domain.io_handler );
+
     if ( i == NR_IO_HANDLERS )
     {
         domain_crash(d);
-- 
1.9.1


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

[PATCH v3 2/3] 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 iommu_domain_init() to a later point after the
hvm_domain_intialise() instead.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 xen/arch/x86/domain.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 5af2cc5..0260e01 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -642,9 +642,6 @@ 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);
 
@@ -660,6 +657,9 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
         /* 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_1;
+
     /* initialize default tsc behavior in case tools don't */
     tsc_set_info(d, TSC_MODE_DEFAULT, 0UL, 0, 0);
     spin_lock_init(&d->arch.vtsc_lock);
@@ -675,6 +675,9 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
 
     return 0;
 
+ fail_1:
+    if ( has_hvm_container_domain(d) )
+        hvm_domain_destroy(d);
  fail:
     d->is_dying = DOMDYING_dead;
     psr_domain_free(d);
-- 
1.9.1


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

[PATCH v3 3/3] AMD IOMMU: Check io_handler before registering mmio handler

[suravee.suthikulpanit]

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

guest_iommu_init tries to register mmio handler before HVM domain
is initialized. This cause registration to silently failing.
This patch adds a sanitiy check and puts out error message.

Signed-off-by: Suravee Suthikulapanit <suravee.suthikulpanit@amd.com>
---
 xen/drivers/passthrough/amd/iommu_guest.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/xen/drivers/passthrough/amd/iommu_guest.c b/xen/drivers/passthrough/amd/iommu_guest.c
index f96fbf4..49f00de 100644
--- a/xen/drivers/passthrough/amd/iommu_guest.c
+++ b/xen/drivers/passthrough/amd/iommu_guest.c
@@ -890,6 +890,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 )
     {
-- 
1.9.1


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

Re: [PATCH v3 0/3] Fix xen crash when starting HVM guest due to missing io handler

[Suravee Suthikulpanit]

+ Rüdiger

This patch series should help fixing the issue you are seeing.

Thanks,
Suravee

On 05/21/2016 06:42 PM, suravee.suthikulpanit@amd.com wrote:
> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>
> Hi All,
>
> Changes from V2:
>    * Use assert instead of sanity check before count increment in
>      the hvm_next_io_handler().
>    * Post-pone iommu_domain_init() and add proper error handling code
>      to destroy hvm in case of failure.
>    * Split out sanity check in guest_iommu_init() into a separate patch.
>
> OVERVIEW:
>
> 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.
>
> Thanks,
> Suravee
>
> Suravee Suthikulpanit (3):
>    x86/hvm: Add check when register io handler
>    svm: iommu: Only call guest_iommu_init() after initialized HVM domain
>    AMD IOMMU: Check io_handler before registering mmio handler
>
>   xen/arch/x86/domain.c                     | 9 ++++++---
>   xen/arch/x86/hvm/intercept.c              | 2 ++
>   xen/drivers/passthrough/amd/iommu_guest.c | 6 ++++++
>   3 files changed, 14 insertions(+), 3 deletions(-)
>

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

Re: [PATCH v3 1/3] x86/hvm: Add check when register io handler

[Paul Durrant]

> -----Original Message-----
> From: suravee.suthikulpanit@amd.com
> [mailto:suravee.suthikulpanit@amd.com]
> Sent: 22 May 2016 00:42
> To: xen-devel@lists.xen.org; Paul Durrant; jbeulich@suse.com; George
> Dunlap
> Cc: Keir (Xen.org); Suravee Suthikulpanit; Suravee Suthikulpanit
> Subject: [PATCH v3 1/3] 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, which means the hvm_domain.io_handler
> would be NULL. In the hvm_next_io_handler(), this should be asserted.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  xen/arch/x86/hvm/intercept.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/xen/arch/x86/hvm/intercept.c b/xen/arch/x86/hvm/intercept.c
> index fc757d0..2f8d57f 100644
> --- a/xen/arch/x86/hvm/intercept.c
> +++ b/xen/arch/x86/hvm/intercept.c
> @@ -258,6 +258,8 @@ struct hvm_io_handler *hvm_next_io_handler(struct
> domain *d)
>  {
>      unsigned int i = d->arch.hvm_domain.io_handler_count++;
> 
> +    ASSERT( d->arch.hvm_domain.io_handler );

Needs fixing for style, but with that...

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> +
>      if ( i == NR_IO_HANDLERS )
>      {
>          domain_crash(d);
> --
> 1.9.1


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

Re: [PATCH v3 2/3] 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: 22 May 2016 00:43
> To: xen-devel@lists.xen.org; Paul Durrant; jbeulich@suse.com; George
> Dunlap
> Cc: Keir (Xen.org); Suravee Suthikulpanit
> Subject: [PATCH v3 2/3] 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 iommu_domain_init() to a later point after the
> hvm_domain_intialise() instead.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> ---
>  xen/arch/x86/domain.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 5af2cc5..0260e01 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -642,9 +642,6 @@ 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);
> 
> @@ -660,6 +657,9 @@ int arch_domain_create(struct domain *d, unsigned
> int domcr_flags,
>          /* 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_1;
> +
>      /* initialize default tsc behavior in case tools don't */
>      tsc_set_info(d, TSC_MODE_DEFAULT, 0UL, 0, 0);
>      spin_lock_init(&d->arch.vtsc_lock);
> @@ -675,6 +675,9 @@ int arch_domain_create(struct domain *d, unsigned
> int domcr_flags,
> 
>      return 0;
> 
> + fail_1:
> +    if ( has_hvm_container_domain(d) )
> +        hvm_domain_destroy(d);
>   fail:
>      d->is_dying = DOMDYING_dead;
>      psr_domain_free(d);
> --
> 1.9.1


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

Re: [PATCH v3 3/3] AMD IOMMU: Check io_handler before registering mmio handler

[Paul Durrant]

> -----Original Message-----
> From: suravee.suthikulpanit@amd.com
> [mailto:suravee.suthikulpanit@amd.com]
> Sent: 22 May 2016 00:43
> To: xen-devel@lists.xen.org; Paul Durrant; jbeulich@suse.com; George
> Dunlap
> Cc: Keir (Xen.org); Suravee Suthikulpanit; Suravee Suthikulapanit
> Subject: [PATCH v3 3/3] AMD IOMMU: Check io_handler before registering
> mmio handler
> 
> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> 
> guest_iommu_init tries to register mmio handler before HVM domain
> is initialized. This cause registration to silently failing.
> This patch adds a sanitiy check and puts out error message.
> 
> Signed-off-by: Suravee Suthikulapanit <suravee.suthikulpanit@amd.com>

This patch is now defunct isn't it?

  Paul

> ---
>  xen/drivers/passthrough/amd/iommu_guest.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/xen/drivers/passthrough/amd/iommu_guest.c
> b/xen/drivers/passthrough/amd/iommu_guest.c
> index f96fbf4..49f00de 100644
> --- a/xen/drivers/passthrough/amd/iommu_guest.c
> +++ b/xen/drivers/passthrough/amd/iommu_guest.c
> @@ -890,6 +890,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 )
>      {
> --
> 1.9.1


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

Re: [PATCH v3 1/3] x86/hvm: Add check when register io handler

[Jan Beulich]

>>> On 22.05.16 at 01:42, <suravee.suthikulpanit@amd.com> wrote:
> --- a/xen/arch/x86/hvm/intercept.c
> +++ b/xen/arch/x86/hvm/intercept.c
> @@ -258,6 +258,8 @@ struct hvm_io_handler *hvm_next_io_handler(struct domain *d)
>  {
>      unsigned int i = d->arch.hvm_domain.io_handler_count++;
>  
> +    ASSERT( d->arch.hvm_domain.io_handler );

Am I wrong in understanding that without patch 2 this ASSERT() will
actually trigger? In which case the patch order would be wrong (but
that could be taken care of during commit).

Jan


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

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

[Jan Beulich]

>>> On 22.05.16 at 01:42, <suravee.suthikulpanit@amd.com> wrote:
> 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 iommu_domain_init() to a later point after the
> hvm_domain_intialise() instead.

That's one possible approach, which I continue to be not really
happy with. guest_iommu_init() really is HVM-specific, so maybe
no longer calling it from amd_iommu_domain_init() would be the
better solution (instead calling it from hvm_domain_initialise()
would then seem to be the better option). Thoughts?

In any event is the choice of ...

> @@ -675,6 +675,9 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>  
>      return 0;
>  
> + fail_1:
> +    if ( has_hvm_container_domain(d) )
> +        hvm_domain_destroy(d);
>   fail:
>      d->is_dying = DOMDYING_dead;
>      psr_domain_free(d);

... the new label name sub-optimal. Please pick something more
descriptive, e.g. "iommu_fail", if the current approach is to be
retained.

Jan


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

Re: [PATCH v3 3/3] AMD IOMMU: Check io_handler before registering mmio handler

[Suravee Suthikulanit]

On 5/23/2016 3:23 AM, Paul Durrant wrote:
>> -----Original Message-----
>> > From: suravee.suthikulpanit@amd.com
>> > [mailto:suravee.suthikulpanit@amd.com]
>> > Sent: 22 May 2016 00:43
>> > To: xen-devel@lists.xen.org; Paul Durrant; jbeulich@suse.com; George
>> > Dunlap
>> > Cc: Keir (Xen.org); Suravee Suthikulpanit; Suravee Suthikulapanit
>> > Subject: [PATCH v3 3/3] AMD IOMMU: Check io_handler before registering
>> > mmio handler
>> >
>> > From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>> >
>> > guest_iommu_init tries to register mmio handler before HVM domain
>> > is initialized. This cause registration to silently failing.
>> > This patch adds a sanitiy check and puts out error message.
>> >
>> > Signed-off-by: Suravee Suthikulapanit <suravee.suthikulpanit@amd.com>
> This patch is now defunct isn't it?
>
>   Paul
>

It is no longer required, but I think this is a good sanity check in 
case something changes in the future and causing this to be called 
before the HVM I/O handler initialized.

Thanks,
Suravee

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

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

[Suravee Suthikulanit]

On 5/23/2016 6:54 AM, Jan Beulich wrote:
>>>> On 22.05.16 at 01:42, <suravee.suthikulpanit@amd.com> wrote:
>> 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 iommu_domain_init() to a later point after the
>> hvm_domain_intialise() instead.
>
> That's one possible approach, which I continue to be not really
> happy with. guest_iommu_init() really is HVM-specific, so maybe
> no longer calling it from amd_iommu_domain_init() would be the
> better solution (instead calling it from hvm_domain_initialise()
> would then seem to be the better option). Thoughts?

Then, this goes back to the approach I proposed in the v1 of this patch 
series, where I call guest_iommu_init/destroy() in the 
svm_domain_initialise/destroy().

However, I'm still not quite clear in why the iommu_domain_init() is 
needed before hvm_domain_initialise().

>
> In any event is the choice of ...
>
>> @@ -675,6 +675,9 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>>
>>      return 0;
>>
>> + fail_1:
>> +    if ( has_hvm_container_domain(d) )
>> +        hvm_domain_destroy(d);
>>   fail:
>>      d->is_dying = DOMDYING_dead;
>>      psr_domain_free(d);
>
> ... the new label name sub-optimal. Please pick something more
> descriptive, e.g. "iommu_fail", if the current approach is to be
> retained.
>
> Jan
>

In case we are going with this approach, I will make this change.

Thanks,
Suravee

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

Re: [PATCH v3 1/3] x86/hvm: Add check when register io handler

[Suravee Suthikulanit]

On 5/23/2016 6:46 AM, Jan Beulich wrote:
>>>> On 22.05.16 at 01:42, <suravee.suthikulpanit@amd.com> wrote:
>> --- a/xen/arch/x86/hvm/intercept.c
>> +++ b/xen/arch/x86/hvm/intercept.c
>> @@ -258,6 +258,8 @@ struct hvm_io_handler *hvm_next_io_handler(struct domain *d)
>>  {
>>      unsigned int i = d->arch.hvm_domain.io_handler_count++;
>>
>> +    ASSERT( d->arch.hvm_domain.io_handler );
>
> Am I wrong in understanding that without patch 2 this ASSERT() will
> actually trigger? In which case the patch order would be wrong (but
> that could be taken care of during commit).
>
> Jan
>

Right, I can fix this in V4 if we decide to change the 
iommu_domain_initialise() ordering.

Suravee

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

Re: [PATCH v3 3/3] AMD IOMMU: Check io_handler before registering mmio handler

[Paul Durrant]

> -----Original Message-----
> From: Suravee Suthikulanit [mailto:suravee.suthikulpanit@amd.com]
> Sent: 25 May 2016 19:52
> To: Paul Durrant; xen-devel@lists.xen.org; jbeulich@suse.com; George
> Dunlap
> Cc: Keir (Xen.org)
> Subject: Re: [PATCH v3 3/3] AMD IOMMU: Check io_handler before
> registering mmio handler
> 
> On 5/23/2016 3:23 AM, Paul Durrant wrote:
> >> -----Original Message-----
> >> > From: suravee.suthikulpanit@amd.com
> >> > [mailto:suravee.suthikulpanit@amd.com]
> >> > Sent: 22 May 2016 00:43
> >> > To: xen-devel@lists.xen.org; Paul Durrant; jbeulich@suse.com; George
> >> > Dunlap
> >> > Cc: Keir (Xen.org); Suravee Suthikulpanit; Suravee Suthikulapanit
> >> > Subject: [PATCH v3 3/3] AMD IOMMU: Check io_handler before
> registering
> >> > mmio handler
> >> >
> >> > From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> >> >
> >> > guest_iommu_init tries to register mmio handler before HVM domain
> >> > is initialized. This cause registration to silently failing.
> >> > This patch adds a sanitiy check and puts out error message.
> >> >
> >> > Signed-off-by: Suravee Suthikulapanit
> <suravee.suthikulpanit@amd.com>
> > This patch is now defunct isn't it?
> >
> >   Paul
> >
> 
> It is no longer required, but I think this is a good sanity check in
> case something changes in the future and causing this to be called
> before the HVM I/O handler initialized.

Maybe, but shouldn't it result in a domain_crash()?

  Paul

> 
> Thanks,
> Suravee

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

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

[Jan Beulich]

>>> Suravee Suthikulanit <suravee.suthikulpanit@amd.com> 05/25/16 9:01 PM >>>
>On 5/23/2016 6:54 AM, Jan Beulich wrote:
>>>>> On 22.05.16 at 01:42, <suravee.suthikulpanit@amd.com> wrote:
>>> 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 iommu_domain_init() to a later point after the
>>> hvm_domain_intialise() instead.
>>
>> That's one possible approach, which I continue to be not really
>> happy with. guest_iommu_init() really is HVM-specific, so maybe
>> no longer calling it from amd_iommu_domain_init() would be the
>> better solution (instead calling it from hvm_domain_initialise()
>> would then seem to be the better option). Thoughts?
>
>Then, this goes back to the approach I proposed in the v1 of this patch 
>series, where I call guest_iommu_init/destroy() in the 
>svm_domain_initialise/destroy().
>
>However, I'm still not quite clear in why the iommu_domain_init() is 
>needed before hvm_domain_initialise().

I think the two things are only lightly related. Changing the order of calls is
generally fine, but recognizing that guest_iommu_init() really would better be
called elsewhere makes that re-ordering simply unnecessary.

Jan


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

Re: [PATCH v3 3/3] AMD IOMMU: Check io_handler before registering mmio handler

[Jan Beulich]

>>> Suravee Suthikulanit <suravee.suthikulpanit@amd.com> 05/25/16 8:52 PM >>>
>On 5/23/2016 3:23 AM, Paul Durrant wrote:
>> > From: suravee.suthikulpanit@amd.com
>> > Sent: 22 May 2016 00:43
>>> > guest_iommu_init tries to register mmio handler before HVM domain
>>> > is initialized. This cause registration to silently failing.
>>> > This patch adds a sanitiy check and puts out error message.
>>> >
>>> > Signed-off-by: Suravee Suthikulapanit <suravee.suthikulpanit@amd.com>
>> This patch is now defunct isn't it?
>
>It is no longer required, but I think this is a good sanity check in 
>case something changes in the future and causing this to be called 
>before the HVM I/O handler initialized.

To be honest, in this case I'm not convinced, no matter that generally
appreciate ASSERT()s getting added.

Jan


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

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

[Suravee Suthikulanit]

On 5/26/2016 10:44 AM, Jan Beulich wrote:
>>>> Suravee Suthikulanit <suravee.suthikulpanit@amd.com> 05/25/16 9:01 PM >>>
>> On 5/23/2016 6:54 AM, Jan Beulich wrote:
>>>>>> On 22.05.16 at 01:42, <suravee.suthikulpanit@amd.com> wrote:
>>>> 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 iommu_domain_init() to a later point after the
>>>> hvm_domain_intialise() instead.
>>>
>>> That's one possible approach, which I continue to be not really
>>> happy with. guest_iommu_init() really is HVM-specific, so maybe
>>> no longer calling it from amd_iommu_domain_init() would be the
>>> better solution (instead calling it from hvm_domain_initialise()
>>> would then seem to be the better option). Thoughts?
>>
>> Then, this goes back to the approach I proposed in the v1 of this patch
>> series, where I call guest_iommu_init/destroy() in the
>> svm_domain_initialise/destroy().
>>
>> However, I'm still not quite clear in why the iommu_domain_init() is
>> needed before hvm_domain_initialise().
>
> I think the two things are only lightly related. Changing the order of calls is
> generally fine, but recognizing that guest_iommu_init() really would better be
> called elsewhere makes that re-ordering simply unnecessary.
>
> Jan

So, let discussing these two things separately. I would propose to:

1. Let's just remove the guest_iommu_init() for now since it's not 
functioning, and it seems to not being called at a proper place 
according to Jan. We will revisit this when we re-introduce and fully 
test out the feature.

2. As for the ordering of the iommu_domain_init() and hvm_domain_init() 
, let's continue to discuss to find proper ordering if it needs changing.

Let me know what you guys thinks.

Thanks,
Suravee

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

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

[Jan Beulich]

>>> On 31.05.16 at 23:11, <suravee.suthikulpanit@amd.com> wrote:
> On 5/26/2016 10:44 AM, Jan Beulich wrote:
>>>>> Suravee Suthikulanit <suravee.suthikulpanit@amd.com> 05/25/16 9:01 PM >>>
>>> On 5/23/2016 6:54 AM, Jan Beulich wrote:
>>>>>>> On 22.05.16 at 01:42, <suravee.suthikulpanit@amd.com> wrote:
>>>>> 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 iommu_domain_init() to a later point after the
>>>>> hvm_domain_intialise() instead.
>>>>
>>>> That's one possible approach, which I continue to be not really
>>>> happy with. guest_iommu_init() really is HVM-specific, so maybe
>>>> no longer calling it from amd_iommu_domain_init() would be the
>>>> better solution (instead calling it from hvm_domain_initialise()
>>>> would then seem to be the better option). Thoughts?
>>>
>>> Then, this goes back to the approach I proposed in the v1 of this patch
>>> series, where I call guest_iommu_init/destroy() in the
>>> svm_domain_initialise/destroy().
>>>
>>> However, I'm still not quite clear in why the iommu_domain_init() is
>>> needed before hvm_domain_initialise().
>>
>> I think the two things are only lightly related. Changing the order of calls 
> is
>> generally fine, but recognizing that guest_iommu_init() really would better 
> be
>> called elsewhere makes that re-ordering simply unnecessary.
>>
>> Jan
> 
> So, let discussing these two things separately. I would propose to:
> 
> 1. Let's just remove the guest_iommu_init() for now since it's not 
> functioning, and it seems to not being called at a proper place 
> according to Jan. We will revisit this when we re-introduce and fully 
> test out the feature.

Fine with me.

> 2. As for the ordering of the iommu_domain_init() and hvm_domain_init() 
> , let's continue to discuss to find proper ordering if it needs changing.

Sure. The only thing I'd like to avoid is a change for the change's sake.

Jan


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

[PATCH v3 2/3] 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 iommu_domain_init() to a later point after the
hvm_domain_intialise() instead.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 xen/arch/x86/domain.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 5af2cc5..0260e01 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -642,9 +642,6 @@ 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);
 
@@ -660,6 +657,9 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
         /* 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_1;
+
     /* initialize default tsc behavior in case tools don't */
     tsc_set_info(d, TSC_MODE_DEFAULT, 0UL, 0, 0);
     spin_lock_init(&d->arch.vtsc_lock);
@@ -675,6 +675,9 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
 
     return 0;
 
+ fail_1:
+    if ( has_hvm_container_domain(d) )
+        hvm_domain_destroy(d);
  fail:
     d->is_dying = DOMDYING_dead;
     psr_domain_free(d);
-- 
1.9.1


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