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

[Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>]

+ 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