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

[Paul Durrant <Paul.Durrant@citrix.com>]

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