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

[Suravee Suthikulpanit <Suravee.Suthikulpanit@xxxxxxx>]

+ Keir (since he added this code originally).

On 05/16/2016 03:19 AM, Paul Durrant wrote:
> > -----Original Message-----
> > >From:suravee.suthikulpanit@xxxxxxx
> > >[mailto:suravee.suthikulpanit@xxxxxxx]
> > >Sent: 13 May 2016 20:37
> > >To:xen-devel@xxxxxxxxxxxxx; George Dunlap;jbeulich@xxxxxxxx
> > >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@xxxxxxx>
> > >
> > >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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel