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

Commit Message

Suravee Suthikulpanit May 21, 2016, 11:42 p.m. UTC

From: Suravee Suthikulpanit <[e-mail]>

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 <[e-mail]>
---
 xen/arch/x86/domain.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Paul Durrant May 23, 2016, 8:21 a.m. UTC | #1

> -----Original Message-----
> From: [e-mail]
> [mailto:[e-mail]]
> Sent: 22 May 2016 00:43
> To: [e-mail]; Paul Durrant; [e-mail]; 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 <[e-mail]>
> 
> 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 <[e-mail]>

Reviewed-by: Paul Durrant <[e-mail]>

> ---
>  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

Jan Beulich May 23, 2016, 11:54 a.m. UTC | #2

>>> On 22.05.16 at 01:42, <[e-mail]> wrote:
> From: Suravee Suthikulpanit <[e-mail]>
> 
> 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

Suravee Suthikulpanit May 25, 2016, 7 p.m. UTC | #3

On 5/23/2016 6:54 AM, Jan Beulich wrote:
>>>> On 22.05.16 at 01:42, <[e-mail]> wrote:
>> From: Suravee Suthikulpanit <[e-mail]>
>>
>> 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

Jan Beulich May 26, 2016, 3:44 p.m. UTC | #4

>>> Suravee Suthikulanit <[e-mail]> 05/25/16 9:01 PM >>>
>On 5/23/2016 6:54 AM, Jan Beulich wrote:
>>>>> On 22.05.16 at 01:42, <[e-mail]> wrote:
>>> From: Suravee Suthikulpanit <[e-mail]>
>>>
>>> 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

Suravee Suthikulpanit May 31, 2016, 9:11 p.m. UTC | #5

On 5/26/2016 10:44 AM, Jan Beulich wrote:
>>>> Suravee Suthikulanit <[e-mail]> 05/25/16 9:01 PM >>>
>> On 5/23/2016 6:54 AM, Jan Beulich wrote:
>>>>>> On 22.05.16 at 01:42, <[e-mail]> wrote:
>>>> From: Suravee Suthikulpanit <[e-mail]>
>>>>
>>>> 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

Jan Beulich June 1, 2016, 7:55 a.m. UTC | #6

>>> On 31.05.16 at 23:11, <[e-mail]> wrote:
> On 5/26/2016 10:44 AM, Jan Beulich wrote:
>>>>> Suravee Suthikulanit <[e-mail]> 05/25/16 9:01 PM >>>
>>> On 5/23/2016 6:54 AM, Jan Beulich wrote:
>>>>>>> On 22.05.16 at 01:42, <[e-mail]> wrote:
>>>>> From: Suravee Suthikulpanit <[e-mail]>
>>>>>
>>>>> 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

Patch

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);