On 23.12.2020 14:50, Julien Grall wrote:
On 23/12/2020 13:27, Jan Beulich wrote:
On 22.12.2020 16:43, Julien Grall wrote:
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -226,7 +226,15 @@ static void iommu_teardown(struct domain *d)
void iommu_domain_destroy(struct domain *d)
{
- if ( !is_iommu_enabled(d) )
+ struct domain_iommu *hd = dom_iommu(d);
+
+ /*
+ * In case of failure during the domain construction, it would be
+ * possible to reach this function with the IOMMU enabled but not
+ * yet initialized. We assume that hd->platforms will be non-NULL as
+ * soon as we start to initialize the IOMMU.
+ */
+ if ( !is_iommu_enabled(d) || !hd->platform_ops )
return;
From your description I'd rather say is_iommu_enabled() is the
wrong predicate to use here. IOW I'd rather see it be replaced.
!hd->platform_ops should be sufficient. So, I think we can drop
!is_iommu_enabled(d). Would that be fine with you?
Well, that's what I was trying to suggest.
A couple of additional nits: "hd" isn't really necessary to
have as a local variable, but if you insist on introducing it
despite being used just once, it should be pointer-to-const.
Plus the comment would better spell correctly the field it
talks about. But I consider this comment excessive anyway, as
the check of ->platform_ops is quite clear by itself.
Well, I added the comment because I think check hd->platform_ops may not
be that obvious (see more below).
And finally "we assume" is in at least latent conflict with
->platform_ops getting set only after arch_iommu_domain_init()
was called. Right now neither x86'es nor Arm's do anything
that would need undoing, but I'd still suggest to replace
"assume" here (if you want to keep the comment in the first
place) and move the assignment up a few lines (right after the
is_iommu_enabled() check there).
My initial implementation of this patch moved the initialization of
hd->platform_ops before arch_iommu_domain_init().
However, this is going to lead to some issues with Paul's series which
add an IOMMU page-table pool ([1]).
The function arch_iommu_domain_init() may now fail. If we initialize
hd->platform_ops earlier on, then the ->teardown() will be called before
->init().
This may be an issue if ->teardown() expects some list pointer to be
initialized by ->init(). I am not aware of any today, but this seems
quite risky to me.
In such a case the obvious thing is to make the teardown handler
check whether its init counterpart has run before. This will then
fit our apparently much wider goal of making cleanup functions
idempotent.
