diff mbox series

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

Message ID 5C9CE0270200007800222881@prv1-mh.provo.novell.com (mailing list archive)
State New, archived
Headers show
Series x86: eliminate Intel-isms from x2APIC setup | expand

Commit Message

Jan Beulich March 28, 2019, 2:54 p.m. UTC 
Move this into iommu_hardware_setup() and make that function non-
inline. Move its declaration into common code.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

Comments

Andrew Cooper March 28, 2019, 5:50 p.m. UTC | #1 
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.

Do you have some plans in future series which make this a non-init function?

~Andrew
Jan Beulich March 29, 2019, 9:15 a.m. UTC | #2 
>>> 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).

> Do you have some plans in future series which make this a non-init function?

No.

Jan
Tian, Kevin April 2, 2019, 3:26 a.m. UTC | #3 
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Thursday, March 28, 2019 10:55 PM
> 
> Move this into iommu_hardware_setup() and make that function non-
> inline. Move its declaration into common code.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Andrew Cooper April 2, 2019, 10:16 a.m. UTC | #4 
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
Woods, Brian April 5, 2019, 2:30 p.m. UTC | #5 
On 3/28/19 9:54 AM, Jan Beulich wrote:
> Move this into iommu_hardware_setup() and make that function non-
> inline. Move its declaration into common code.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 

Acked-by: Brian Woods <brian.woods@amd.com>
diff mbox series

Patch

--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -31,7 +31,6 @@ 
 static bool_t __read_mostly init_done;
 
 static const struct iommu_init_ops _iommu_init_ops;
-static const struct iommu_ops amd_iommu_ops;
 
 struct amd_iommu *find_iommu_for_device(int seg, int bdf)
 {
@@ -196,8 +195,6 @@  static int __init iov_detect(void)
     if ( !iommu_enable && !iommu_intremap )
         return 0;
 
-    iommu_ops = amd_iommu_ops;
-
     if ( amd_iommu_init() != 0 )
     {
         printk("AMD-Vi: Error initialization\n");
@@ -582,7 +579,7 @@  static void amd_dump_p2m_table(struct do
     amd_dump_p2m_table_level(hd->arch.root_table, hd->arch.paging_mode, 0, 0);
 }
 
-static const struct iommu_ops __initconstrel amd_iommu_ops = {
+static const struct iommu_ops __initconstrel _iommu_ops = {
     .init = amd_iommu_domain_init,
     .hwdom_init = amd_iommu_hwdom_init,
     .add_device = amd_iommu_add_device,
@@ -609,5 +606,6 @@  static const struct iommu_ops __initcons
 };
 
 static const struct iommu_init_ops __initconstrel _iommu_init_ops = {
+    .ops = &_iommu_ops,
     .setup = iov_detect,
 };
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2305,8 +2305,6 @@  static int __init vtd_setup(void)
         goto error;
     }
 
-    iommu_ops = intel_iommu_ops;
-
     /* We enable the following features only if they are supported by all VT-d
      * engines: Snoop Control, DMA passthrough, Queued Invalidation, Interrupt
      * Remapping, and Posted Interrupt
--- 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);
+
+    return iommu_init_ops->setup();
+}
+
 int iommu_enable_x2apic_IR(void)
 {
     if ( system_state < SYS_STATE_active )
--- a/xen/include/asm-arm/iommu.h
+++ b/xen/include/asm-arm/iommu.h
@@ -26,8 +26,6 @@  struct arch_iommu
 const struct iommu_ops *iommu_get_ops(void);
 void iommu_set_ops(const struct iommu_ops *ops);
 
-int iommu_hardware_setup(void);
-
 #endif /* __ARCH_ARM_IOMMU_H__ */
 
 /*
--- a/xen/include/asm-x86/iommu.h
+++ b/xen/include/asm-x86/iommu.h
@@ -73,11 +73,6 @@  struct iommu_init_ops {
 
 extern const struct iommu_init_ops *iommu_init_ops;
 
-static inline int iommu_hardware_setup(void)
-{
-    return iommu_init_ops ? iommu_init_ops->setup() : -ENODEV;
-}
-
 /* Are we using the domain P2M table as its IOMMU pagetable? */
 #define iommu_use_hap_pt(d) \
     (hap_enabled(d) && has_iommu_pt(d) && iommu_hap_pt_share)
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -65,6 +65,7 @@  extern int8_t iommu_hwdom_reserved;
 extern unsigned int iommu_dev_iotlb_timeout;
 
 int iommu_setup(void);
+int iommu_hardware_setup(void);
 
 int iommu_domain_init(struct domain *d);
 void iommu_hwdom_init(struct domain *d);