| Message ID | 1463874151-6950-3-git-send-email-suravee.suthikulpanit@amd.com |
|---|---|
| State | New |
| Headers | show |
On 5/23/2016 6:54 AM, Jan Beulich wrote: >>>> On 22.05.16 at 01:42, <suravee.suthikulpanit@amd.com> wrote: >> 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 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
On 5/26/2016 10:44 AM, Jan Beulich wrote: >>>> Suravee Suthikulanit <suravee.suthikulpanit@amd.com> 05/25/16 9:01 PM >>> >> On 5/23/2016 6:54 AM, Jan Beulich wrote: >>>>>> On 22.05.16 at 01:42, <suravee.suthikulpanit@amd.com> wrote: >>>> 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 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
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);