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

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Suravee Suthikulpanit <Suravee.Suthikulpanit@xxxxxxx> 05/19/16 7:22 AM >>>
>On 05/16/2016 03:19 AM, Paul Durrant wrote:
>> >From:suravee.suthikulpanit@xxxxxxx
>> >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(). 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.

Jan


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