Re: [PATCH 7/7] x86/IOMMU: initialize iommu_ops in vendor-independent code

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Kevin Tian <kevin.tian@intel.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	Julien Grall <julien.grall@arm.com>,
	Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	Brian Woods <brian.woods@amd.com>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [PATCH 7/7] x86/IOMMU: initialize iommu_ops in vendor-independent code
Date: Tue, 2 Apr 2019 11:16:43 +0100	[thread overview]
Message-ID: <c5babaad-cee3-f16c-e7df-9bfe4ab5f516@citrix.com> (raw)
In-Reply-To: <5C9DE2250200007800222B61@prv1-mh.provo.novell.com>

On 29/03/2019 09:15, Jan Beulich wrote:
>>>> On 28.03.19 at 18:50, <andrew.cooper3@citrix.com> wrote:
>> On 28/03/2019 14:54, Jan Beulich wrote:
>>> --- a/xen/drivers/passthrough/x86/iommu.c
>>> +++ b/xen/drivers/passthrough/x86/iommu.c
>>> @@ -26,6 +26,19 @@
>>>  const struct iommu_init_ops *__initdata iommu_init_ops;
>>>  struct iommu_ops __read_mostly iommu_ops;
>>>  
>>> +int __init iommu_hardware_setup(void)
>>> +{
>>> +    if ( !iommu_init_ops )
>>> +        return -ENODEV;
>>> +
>>> +    if ( !iommu_ops.init )
>>> +        iommu_ops = *iommu_init_ops->ops;
>>> +    else
>>> +        ASSERT(iommu_ops.init == iommu_init_ops->ops->init);
>> What is this ASSERT() intended to catch?  We pass through this function
>> exactly once, making the else path dead.
> iommu_ops may have got set already during x2APIC IR enabling (see
> patch 6).

In which case a

/* x2apic setup may have previously initialised the IOMMU ops. */

or similar would do nicely, to explain this logic.

With that, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel