[PATCH 0/7] x86: eliminate Intel-isms from x2APIC setup

[PATCH 0/7] x86: eliminate Intel-isms from x2APIC setup

[Jan Beulich]

This is a first preparatory step for enabling x2APIC support also
for AMD (plus some misc cleanup).

1: entry: drop unused header inclusions
2: APIC: suppress redundant "Switched to ..." messages
3: ACPI: also parse AMD IOMMU tables early
4: IOMMU: introduce init-ops structure
5: IOMMU: abstract Intel-specific iommu_supports_eim()
6: IOMMU: abstract Intel-specific iommu_{en,dis}able_x2apic_IR()
7: IOMMU: initialize iommu_ops in vendor-independent code

Jan



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

[PATCH 1/7] x86/entry: drop unused header inclusions

[Jan Beulich]

I'm in particular after getting rid of asm/apicdef.h, but there are more
no longer (or perhaps never having been) used ones.

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

--- a/xen/arch/x86/hvm/svm/entry.S
+++ b/xen/arch/x86/hvm/svm/entry.S
@@ -19,13 +19,8 @@
 
         .file "svm/entry.S"
 
-#include <xen/errno.h>
-#include <xen/softirq.h>
-#include <asm/types.h>
 #include <asm/asm_defns.h>
-#include <asm/apicdef.h>
 #include <asm/page.h>
-#include <public/xen.h>
 
 #define VMRUN  .byte 0x0F,0x01,0xD8
 #define STGI   .byte 0x0F,0x01,0xDC
--- a/xen/arch/x86/hvm/vmx/entry.S
+++ b/xen/arch/x86/hvm/vmx/entry.S
@@ -18,13 +18,8 @@
 
         .file "vmx/entry.S"
 
-#include <xen/errno.h>
-#include <xen/softirq.h>
-#include <asm/types.h>
 #include <asm/asm_defns.h>
-#include <asm/apicdef.h>
 #include <asm/page.h>
-#include <public/xen.h>
 
 #define VMRESUME     .byte 0x0f,0x01,0xc3
 #define VMLAUNCH     .byte 0x0f,0x01,0xc2
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -4,10 +4,7 @@
 
         .file "x86_64/compat/entry.S"
 
-#include <xen/errno.h>
-#include <xen/softirq.h>
 #include <asm/asm_defns.h>
-#include <asm/apicdef.h>
 #include <asm/page.h>
 #include <asm/processor.h>
 #include <asm/desc.h>
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -6,10 +6,7 @@
 
         .file "x86_64/entry.S"
 
-#include <xen/errno.h>
-#include <xen/softirq.h>
 #include <asm/asm_defns.h>
-#include <asm/apicdef.h>
 #include <asm/page.h>
 #include <asm/processor.h>
 #include <public/xen.h>




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

Re: [PATCH 1/7] x86/entry: drop unused header inclusions

[Andrew Cooper]

On 28/03/2019 14:48, Jan Beulich wrote:
> I'm in particular after getting rid of asm/apicdef.h, but there are more
> no longer (or perhaps never having been) used ones.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

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

Re: [PATCH 1/7] x86/entry: drop unused header inclusions

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Thursday, March 28, 2019 10:49 PM
> 
> I'm in particular after getting rid of asm/apicdef.h, but there are more
> no longer (or perhaps never having been) used ones.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

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

Re: [PATCH 1/7] x86/entry: drop unused header inclusions

[Boris Ostrovsky]

On 3/28/19 10:48 AM, Jan Beulich wrote:
> I'm in particular after getting rid of asm/apicdef.h, but there are more
> no longer (or perhaps never having been) used ones.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>




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

Re: [Xen-devel] [PATCH 1/7] x86/entry: drop unused header inclusions

[Boris Ostrovsky]

On 3/28/19 10:48 AM, Jan Beulich wrote:
> I'm in particular after getting rid of asm/apicdef.h, but there are more
> no longer (or perhaps never having been) used ones.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>




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

[PATCH 2/7] x86/APIC: suppress redundant "Switched to ..." messages

[Jan Beulich]

There's no need to log anything when what we "switch to" is what is in
use already.

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

--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -884,6 +884,7 @@ void x2apic_ap_setup(void)
 void __init x2apic_bsp_setup(void)
 {
     struct IO_APIC_route_entry **ioapic_entries = NULL;
+    const char *orig_name;
 
     if ( !cpu_has_x2apic )
         return;
@@ -946,8 +947,10 @@ void __init x2apic_bsp_setup(void)
 
     force_iommu = 1;
 
+    orig_name = genapic.name;
     genapic = *apic_x2apic_probe();
-    printk("Switched to APIC driver %s.\n", genapic.name);
+    if ( genapic.name != orig_name )
+        printk("Switched to APIC driver %s\n", genapic.name);
 
     if ( !x2apic_enabled )
     {
--- a/xen/arch/x86/genapic/probe.c
+++ b/xen/arch/x86/genapic/probe.c
@@ -85,7 +85,8 @@ int __init mps_oem_check(struct mp_confi
 	int i;
 	for (i = 0; apic_probe[i]; ++i) { 
 		if (apic_probe[i]->mps_oem_check(mpc,oem,productid)) { 
-			if (!cmdline_apic) {
+			if (!cmdline_apic &&
+			     genapic.name != apic_probe[i]->name) {
 				genapic = *apic_probe[i];
 				printk(KERN_INFO "Switched to APIC driver `%s'.\n", 
 				       genapic.name);
@@ -101,7 +102,8 @@ int __init acpi_madt_oem_check(char *oem
 	int i;
 	for (i = 0; apic_probe[i]; ++i) { 
 		if (apic_probe[i]->acpi_madt_oem_check(oem_id, oem_table_id)) { 
-			if (!cmdline_apic) {
+			if (!cmdline_apic &&
+			     genapic.name != apic_probe[i]->name) {
 				genapic = *apic_probe[i];
 				printk(KERN_INFO "Switched to APIC driver `%s'.\n", 
 				       genapic.name);





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

Re: [PATCH 2/7] x86/APIC: suppress redundant "Switched to ..." messages

[Andrew Cooper]

On 28/03/2019 14:49, Jan Beulich wrote:
> There's no need to log anything when what we "switch to" is what is in
> use already.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

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

[PATCH 3/7] x86/ACPI: also parse AMD IOMMU tables early

[Jan Beulich]

In order to be able to initialize x2APIC mode we need to parse
respective ACPI tables early. Split amd_iov_detect() into two parts for
this purpose, and call the initial part earlier on.

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

--- a/xen/arch/x86/acpi/boot.c
+++ b/xen/arch/x86/acpi/boot.c
@@ -733,7 +733,7 @@ int __init acpi_boot_init(void)
 
 	acpi_mmcfg_init();
 
-	acpi_dmar_init();
+	acpi_iommu_init();
 
 	erst_init();
 
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -23,6 +23,7 @@
 #include <xen/pci_regs.h>
 #include <xen/paging.h>
 #include <xen/softirq.h>
+#include <asm/acpi.h>
 #include <asm/amd-iommu.h>
 #include <asm/hvm/svm/amd-iommu-proto.h>
 #include "../ats.h"
@@ -170,7 +171,7 @@ static void amd_iommu_setup_domain_devic
     }
 }
 
-int __init amd_iov_detect(void)
+int __init acpi_ivrs_init(void)
 {
     INIT_LIST_HEAD(&amd_iommu_head);
 
@@ -184,6 +185,14 @@ int __init amd_iov_detect(void)
         return -ENODEV;
     }
 
+    return 0;
+}
+
+int __init amd_iov_detect(void)
+{
+    if ( !iommu_enable && !iommu_intremap )
+        return 0;
+
     iommu_ops = amd_iommu_ops;
 
     if ( amd_iommu_init() != 0 )
--- a/xen/include/asm-x86/acpi.h
+++ b/xen/include/asm-x86/acpi.h
@@ -26,6 +26,7 @@
 #include <acpi/pdc_intel.h>
 #include <acpi/acconfig.h>
 #include <acpi/actbl.h>
+#include <xen/errno.h>
 
 #define COMPILER_DEPENDENT_INT64   long long
 #define COMPILER_DEPENDENT_UINT64  unsigned long long
@@ -145,6 +146,15 @@ extern u32 pmtmr_ioport;
 extern unsigned int pmtmr_width;
 
 int acpi_dmar_init(void);
+int acpi_ivrs_init(void);
+
+static inline int acpi_iommu_init(void)
+{
+    int ret = acpi_dmar_init();
+
+    return ret == -ENODEV ? acpi_ivrs_init() : ret;
+}
+
 void acpi_mmcfg_init(void);
 
 /* Incremented whenever we transition through S3. Value is 1 during boot. */





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

Re: [PATCH 3/7] x86/ACPI: also parse AMD IOMMU tables early

[Andrew Cooper]

On 28/03/2019 14:49, Jan Beulich wrote:
> In order to be able to initialize x2APIC mode we need to parse
> respective ACPI tables early. Split amd_iov_detect() into two parts for
> this purpose, and call the initial part earlier on.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

> --- a/xen/include/asm-x86/acpi.h
> +++ b/xen/include/asm-x86/acpi.h
> @@ -26,6 +26,7 @@
>  #include <acpi/pdc_intel.h>
>  #include <acpi/acconfig.h>
>  #include <acpi/actbl.h>
> +#include <xen/errno.h>
>  
>  #define COMPILER_DEPENDENT_INT64   long long
>  #define COMPILER_DEPENDENT_UINT64  unsigned long long
> @@ -145,6 +146,15 @@ extern u32 pmtmr_ioport;
>  extern unsigned int pmtmr_width;
>  
>  int acpi_dmar_init(void);
> +int acpi_ivrs_init(void);
> +
> +static inline int acpi_iommu_init(void)
> +{
> +    int ret = acpi_dmar_init();
> +
> +    return ret == -ENODEV ? acpi_ivrs_init() : ret;
> +}
> +

I suppose we really don't have any better option here than to try for
all APCI tables we may be interested in.  I certainly can't think of any
identifying information we could usefully switch() on instead.

~Andrew

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

Re: [PATCH 3/7] x86/ACPI: also parse AMD IOMMU tables early

[Jan Beulich]

>>> On 28.03.19 at 17:43, <andrew.cooper3@citrix.com> wrote:
> On 28/03/2019 14:49, Jan Beulich wrote:
>> In order to be able to initialize x2APIC mode we need to parse
>> respective ACPI tables early. Split amd_iov_detect() into two parts for
>> this purpose, and call the initial part earlier on.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

>> @@ -145,6 +146,15 @@ extern u32 pmtmr_ioport;
>>  extern unsigned int pmtmr_width;
>>  
>>  int acpi_dmar_init(void);
>> +int acpi_ivrs_init(void);
>> +
>> +static inline int acpi_iommu_init(void)
>> +{
>> +    int ret = acpi_dmar_init();
>> +
>> +    return ret == -ENODEV ? acpi_ivrs_init() : ret;
>> +}
>> +
> 
> I suppose we really don't have any better option here than to try for
> all APCI tables we may be interested in.  I certainly can't think of any
> identifying information we could usefully switch() on instead.

Yeah, I've been trying to think of some way to sensibly avoid
either of the attempts, but keying this off of the CPU vendor
doesn't seem right. Since we can't tolerate a mixture of IOMMUs,
the above seemed the most reasonable thing we can do, thus
ensuring that even in a strange (virtualized?) environment
where both tables exist we'd enable only one of the two kinds.

Jan



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

Re: [PATCH 3/7] x86/ACPI: also parse AMD IOMMU tables early

[Woods, Brian]

On 3/28/19 9:49 AM, Jan Beulich wrote:
> In order to be able to initialize x2APIC mode we need to parse
> respective ACPI tables early. Split amd_iov_detect() into two parts for
> this purpose, and call the initial part earlier on.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
Acked-by: Brian Woods <brian.woods@amd.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/7] x86/ACPI: also parse AMD IOMMU tables early

[Woods, Brian]

On 3/28/19 9:49 AM, Jan Beulich wrote:
> In order to be able to initialize x2APIC mode we need to parse
> respective ACPI tables early. Split amd_iov_detect() into two parts for
> this purpose, and call the initial part earlier on.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
Acked-by: Brian Woods <brian.woods@amd.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 4/7] x86/IOMMU: introduce init-ops structure

[Jan Beulich]

Do away with the CPU vendor dependency, and set the init ops pointer
based on what ACPI tables have been found.

Also take the opportunity and add __read_mostly to iommu_ops.

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

--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -30,6 +30,7 @@
 
 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)
@@ -185,10 +186,12 @@ int __init acpi_ivrs_init(void)
         return -ENODEV;
     }
 
+    iommu_init_ops = &_iommu_init_ops;
+
     return 0;
 }
 
-int __init amd_iov_detect(void)
+static int __init iov_detect(void)
 {
     if ( !iommu_enable && !iommu_intremap )
         return 0;
@@ -604,3 +607,7 @@ static const struct iommu_ops __initcons
     .crash_shutdown = amd_iommu_crash_shutdown,
     .dump_p2m_table = amd_dump_p2m_table,
 };
+
+static const struct iommu_init_ops __initconstrel _iommu_init_ops = {
+    .setup = iov_detect,
+};
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -993,7 +993,11 @@ int __init acpi_dmar_init(void)
     ret = parse_dmar_table(acpi_parse_dmar);
 
     if ( !ret )
+    {
+        iommu_init_ops = &intel_iommu_init_ops;
+
         return add_user_rmrr();
+    }
 
     return ret;
 }
--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -27,6 +27,7 @@
 
 struct pci_ats_dev;
 extern bool_t rwbf_quirk;
+extern const struct iommu_init_ops intel_iommu_init_ops;
 extern const struct iommu_ops intel_iommu_ops;
 
 void print_iommu_regs(struct acpi_drhd_unit *drhd);
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2280,7 +2280,7 @@ static void __hwdom_init setup_hwdom_rmr
     pcidevs_unlock();
 }
 
-int __init intel_vtd_setup(void)
+static int __init vtd_setup(void)
 {
     struct acpi_drhd_unit *drhd;
     struct iommu *iommu;
@@ -2735,6 +2735,10 @@ const struct iommu_ops __initconstrel in
     .dump_p2m_table = vtd_dump_p2m_table,
 };
 
+const struct iommu_init_ops __initconstrel intel_iommu_init_ops = {
+    .setup = vtd_setup,
+};
+
 /*
  * Local variables:
  * mode: C
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -23,7 +23,8 @@
 #include <asm/hvm/io.h>
 #include <asm/setup.h>
 
-struct iommu_ops iommu_ops;
+const struct iommu_init_ops *__initdata iommu_init_ops;
+struct iommu_ops __read_mostly iommu_ops;
 
 void iommu_update_ire_from_apic(
     unsigned int apic, unsigned int reg, unsigned int value)
--- a/xen/include/asm-x86/iommu.h
+++ b/xen/include/asm-x86/iommu.h
@@ -56,9 +56,6 @@ struct arch_iommu
     struct guest_iommu *g_iommu;
 };
 
-int intel_vtd_setup(void);
-int amd_iov_detect(void);
-
 extern struct iommu_ops iommu_ops;
 
 static inline const struct iommu_ops *iommu_get_ops(void)
@@ -67,17 +64,15 @@ static inline const struct iommu_ops *io
     return &iommu_ops;
 }
 
+struct iommu_init_ops {
+    int (*setup)(void);
+};
+
+extern const struct iommu_init_ops *iommu_init_ops;
+
 static inline int iommu_hardware_setup(void)
 {
-    switch ( boot_cpu_data.x86_vendor )
-    {
-    case X86_VENDOR_INTEL:
-        return intel_vtd_setup();
-    case X86_VENDOR_AMD:
-        return amd_iov_detect();
-    }
-
-    return -ENODEV;
+    return iommu_init_ops ? iommu_init_ops->setup() : -ENODEV;
 }
 
 /* Are we using the domain P2M table as its IOMMU pagetable? */





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

Re: [PATCH 4/7] x86/IOMMU: introduce init-ops structure

[Andrew Cooper]

On 28/03/2019 14:51, Jan Beulich wrote:
> Do away with the CPU vendor dependency, and set the init ops pointer
> based on what ACPI tables have been found.

"based on which APCI tables..."

>
> Also take the opportunity and add __read_mostly to iommu_ops.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

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

Re: [PATCH 4/7] x86/IOMMU: introduce init-ops structure

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Thursday, March 28, 2019 10:52 PM
> 
> Do away with the CPU vendor dependency, and set the init ops pointer
> based on what ACPI tables have been found.
> 
> Also take the opportunity and add __read_mostly to iommu_ops.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

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

Re: [PATCH 4/7] x86/IOMMU: introduce init-ops structure

[Woods, Brian]

On 3/28/19 9:51 AM, Jan Beulich wrote:
> Do away with the CPU vendor dependency, and set the init ops pointer
> based on what ACPI tables have been found.
> 
> Also take the opportunity and add __read_mostly to iommu_ops.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Brian Woods <brian.woods@amd.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 4/7] x86/IOMMU: introduce init-ops structure

[Woods, Brian]

On 3/28/19 9:51 AM, Jan Beulich wrote:
> Do away with the CPU vendor dependency, and set the init ops pointer
> based on what ACPI tables have been found.
> 
> Also take the opportunity and add __read_mostly to iommu_ops.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Brian Woods <brian.woods@amd.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 5/7] x86/IOMMU: abstract Intel-specific iommu_supports_eim()

[Jan Beulich]

Introduce a respective element in struct iommu_init_ops.

Take the liberty and also switch intel_iommu_supports_eim() to bool/
true/false, to fully match the hook's type.

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

--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -899,14 +899,14 @@ void __init x2apic_bsp_setup(void)
         printk("x2APIC: Already enabled by BIOS: Ignoring cmdline disable.\n");
     }
 
-    if ( !iommu_supports_eim() )
+    if ( !iommu_supports_x2apic() )
     {
         if ( !x2apic_enabled )
         {
-            printk("Not enabling x2APIC: depends on iommu_supports_eim.\n");
+            printk("Not enabling x2APIC: depends on IOMMU support\n");
             return;
         }
-        panic("x2APIC: already enabled by BIOS, but iommu_supports_eim failed\n");
+        panic("x2APIC: already enabled by BIOS, but no IOMMU support\n");
     }
 
     if ( (ioapic_entries = alloc_ioapic_entries()) == NULL )
--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -34,6 +34,8 @@ void print_iommu_regs(struct acpi_drhd_u
 void print_vtd_entries(struct iommu *iommu, int bus, int devfn, u64 gmfn);
 keyhandler_fn_t vtd_dump_iommu_info;
 
+bool intel_iommu_supports_eim(void);
+
 int enable_qinval(struct iommu *iommu);
 void disable_qinval(struct iommu *iommu);
 int enable_intremap(struct iommu *iommu, int eim);
--- a/xen/drivers/passthrough/vtd/intremap.c
+++ b/xen/drivers/passthrough/vtd/intremap.c
@@ -142,13 +142,13 @@ static void set_hpet_source_id(unsigned
     set_ire_sid(ire, SVT_VERIFY_SID_SQ, SQ_13_IGNORE_3, hpetid_to_bdf(id));
 }
 
-bool_t __init iommu_supports_eim(void)
+bool __init intel_iommu_supports_eim(void)
 {
     struct acpi_drhd_unit *drhd;
     unsigned int apic;
 
     if ( !iommu_qinval || !iommu_intremap || list_empty(&acpi_drhd_units) )
-        return 0;
+        return false;
 
     /* We MUST have a DRHD unit for each IOAPIC. */
     for ( apic = 0; apic < nr_ioapics; apic++ )
@@ -157,16 +157,16 @@ bool_t __init iommu_supports_eim(void)
             dprintk(XENLOG_WARNING VTDPREFIX,
                     "There is not a DRHD for IOAPIC %#x (id: %#x)!\n",
                     apic, IO_APIC_ID(apic));
-            return 0;
+            return false;
         }
 
     for_each_drhd_unit ( drhd )
         if ( !ecap_queued_inval(drhd->iommu->ecap) ||
              !ecap_intr_remap(drhd->iommu->ecap) ||
              !ecap_eim(drhd->iommu->ecap) )
-            return 0;
+            return false;
 
-    return 1;
+    return true;
 }
 
 /*
@@ -889,7 +889,7 @@ int iommu_enable_x2apic_IR(void)
 
     if ( system_state < SYS_STATE_active )
     {
-        if ( !iommu_supports_eim() )
+        if ( !intel_iommu_supports_eim() )
             return -EOPNOTSUPP;
 
         if ( !platform_supports_x2apic() )
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2737,6 +2737,7 @@ const struct iommu_ops __initconstrel in
 
 const struct iommu_init_ops __initconstrel intel_iommu_init_ops = {
     .setup = vtd_setup,
+    .supports_x2apic = intel_iommu_supports_eim,
 };
 
 /*
--- a/xen/include/asm-x86/iommu.h
+++ b/xen/include/asm-x86/iommu.h
@@ -66,6 +66,7 @@ static inline const struct iommu_ops *io
 
 struct iommu_init_ops {
     int (*setup)(void);
+    bool (*supports_x2apic)(void);
 };
 
 extern const struct iommu_init_ops *iommu_init_ops;
@@ -87,7 +88,14 @@ int iommu_setup_hpet_msi(struct msi_desc
 int adjust_vtd_irq_affinities(void);
 int __must_check iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte,
                                  int order, int present);
-bool_t iommu_supports_eim(void);
+
+static inline bool iommu_supports_x2apic(void)
+{
+    return iommu_init_ops && iommu_init_ops->supports_x2apic
+           ? iommu_init_ops->supports_x2apic()
+           : false;
+}
+
 int iommu_enable_x2apic_IR(void);
 void iommu_disable_x2apic_IR(void);
 





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

Re: [PATCH 5/7] x86/IOMMU: abstract Intel-specific iommu_supports_eim()

[Andrew Cooper]

On 28/03/2019 14:52, Jan Beulich wrote:
> Introduce a respective element in struct iommu_init_ops.
>
> Take the liberty and also switch intel_iommu_supports_eim() to bool/
> true/false, to fully match the hook's type.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

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

Re: [PATCH 5/7] x86/IOMMU: abstract Intel-specific iommu_supports_eim()

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Thursday, March 28, 2019 10:52 PM
> 
> Introduce a respective element in struct iommu_init_ops.
> 
> Take the liberty and also switch intel_iommu_supports_eim() to bool/
> true/false, to fully match the hook's type.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

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

[PATCH 6/7] x86/IOMMU: abstract Intel-specific iommu_{en, dis}able_x2apic_IR()

[Jan Beulich]

Introduce respective elements in struct iommu_init_ops as well as a
pointer to the main ops structure.

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

--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -35,6 +35,8 @@ void print_vtd_entries(struct iommu *iom
 keyhandler_fn_t vtd_dump_iommu_info;
 
 bool intel_iommu_supports_eim(void);
+int intel_iommu_enable_x2apic_IR(void);
+void intel_iommu_disable_x2apic_IR(void);
 
 int enable_qinval(struct iommu *iommu);
 void disable_qinval(struct iommu *iommu);
--- a/xen/drivers/passthrough/vtd/intremap.c
+++ b/xen/drivers/passthrough/vtd/intremap.c
@@ -882,23 +882,13 @@ out:
  * This function is used to enable Interrupt remapping when
  * enable x2apic
  */
-int iommu_enable_x2apic_IR(void)
+int intel_iommu_enable_x2apic_IR(void)
 {
     struct acpi_drhd_unit *drhd;
     struct iommu *iommu;
 
-    if ( system_state < SYS_STATE_active )
-    {
-        if ( !intel_iommu_supports_eim() )
-            return -EOPNOTSUPP;
-
-        if ( !platform_supports_x2apic() )
-            return -ENXIO;
-
-        iommu_ops = intel_iommu_ops;
-    }
-    else if ( !x2apic_enabled )
-        return -EOPNOTSUPP;
+    if ( system_state < SYS_STATE_active && !platform_supports_x2apic() )
+        return -ENXIO;
 
     for_each_drhd_unit ( drhd )
     {
@@ -946,14 +936,10 @@ int iommu_enable_x2apic_IR(void)
  * This function is used to disable Interrutp remapping when
  * suspend local apic
  */
-void iommu_disable_x2apic_IR(void)
+void intel_iommu_disable_x2apic_IR(void)
 {
     struct acpi_drhd_unit *drhd;
 
-    /* x2apic_enabled implies iommu_supports_eim(). */
-    if ( !x2apic_enabled )
-        return;
-
     for_each_drhd_unit ( drhd )
         disable_intremap(drhd->iommu);
 
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2720,6 +2720,8 @@ const struct iommu_ops __initconstrel in
     .free_page_table = iommu_free_page_table,
     .reassign_device = reassign_device_ownership,
     .get_device_group_id = intel_iommu_group_id,
+    .enable_x2apic_IR = intel_iommu_enable_x2apic_IR,
+    .disable_x2apic_IR = intel_iommu_disable_x2apic_IR,
     .update_ire_from_apic = io_apic_write_remap_rte,
     .update_ire_from_msi = msi_msg_write_remap_rte,
     .read_apic_from_ire = io_apic_read_remap_rte,
@@ -2736,6 +2738,7 @@ const struct iommu_ops __initconstrel in
 };
 
 const struct iommu_init_ops __initconstrel intel_iommu_init_ops = {
+    .ops = &intel_iommu_ops,
     .setup = vtd_setup,
     .supports_x2apic = intel_iommu_supports_eim,
 };
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -26,6 +26,24 @@
 const struct iommu_init_ops *__initdata iommu_init_ops;
 struct iommu_ops __read_mostly iommu_ops;
 
+int iommu_enable_x2apic_IR(void)
+{
+    if ( system_state < SYS_STATE_active )
+    {
+        if ( !iommu_supports_x2apic() )
+            return -EOPNOTSUPP;
+
+        iommu_ops = *iommu_init_ops->ops;
+    }
+    else if ( !x2apic_enabled )
+        return -EOPNOTSUPP;
+
+    if ( !iommu_ops.enable_x2apic_IR )
+        return -EOPNOTSUPP;
+
+    return iommu_ops.enable_x2apic_IR();
+}
+
 void iommu_update_ire_from_apic(
     unsigned int apic, unsigned int reg, unsigned int value)
 {
--- a/xen/include/asm-x86/apic.h
+++ b/xen/include/asm-x86/apic.h
@@ -29,7 +29,6 @@ enum apic_mode {
 };
 
 extern u8 apic_verbosity;
-extern bool x2apic_enabled;
 extern bool directed_eoi_enabled;
 
 void check_x2apic_preenabled(void);
--- a/xen/include/asm-x86/apicdef.h
+++ b/xen/include/asm-x86/apicdef.h
@@ -126,4 +126,6 @@
 
 #define MAX_IO_APICS 128
 
+extern bool x2apic_enabled;
+
 #endif
--- a/xen/include/asm-x86/iommu.h
+++ b/xen/include/asm-x86/iommu.h
@@ -17,6 +17,7 @@
 #include <xen/errno.h>
 #include <xen/list.h>
 #include <xen/spinlock.h>
+#include <asm/apicdef.h>
 #include <asm/processor.h>
 #include <asm/hvm/vmx/vmcs.h>
 
@@ -65,6 +66,7 @@ static inline const struct iommu_ops *io
 }
 
 struct iommu_init_ops {
+    const struct iommu_ops *ops;
     int (*setup)(void);
     bool (*supports_x2apic)(void);
 };
@@ -97,7 +99,12 @@ static inline bool iommu_supports_x2apic
 }
 
 int iommu_enable_x2apic_IR(void);
-void iommu_disable_x2apic_IR(void);
+
+static inline void iommu_disable_x2apic_IR(void)
+{
+    if ( x2apic_enabled && iommu_ops.disable_x2apic_IR )
+        iommu_ops.disable_x2apic_IR();
+}
 
 extern bool untrusted_msi;
 
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -216,11 +216,16 @@ struct iommu_ops {
                                     unsigned int *flags);
 
     void (*free_page_table)(struct page_info *);
+
 #ifdef CONFIG_X86
+    int (*enable_x2apic_IR)(void);
+    void (*disable_x2apic_IR)(void);
+
     void (*update_ire_from_apic)(unsigned int apic, unsigned int reg, unsigned int value);
     unsigned int (*read_apic_from_ire)(unsigned int apic, unsigned int reg);
     int (*setup_hpet_msi)(struct msi_desc *);
 #endif /* CONFIG_X86 */
+
     int __must_check (*suspend)(void);
     void (*resume)(void);
     void (*share_p2m)(struct domain *d);




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

Re: [PATCH 6/7] x86/IOMMU: abstract Intel-specific iommu_{en, dis}able_x2apic_IR()

[Andrew Cooper]

On 28/03/2019 14:53, Jan Beulich wrote:
> @@ -35,6 +35,8 @@ void print_vtd_entries(struct iommu *iom
>  keyhandler_fn_t vtd_dump_iommu_info;
>  
>  bool intel_iommu_supports_eim(void);
> +int intel_iommu_enable_x2apic_IR(void);
> +void intel_iommu_disable_x2apic_IR(void);

Is there any particular reason why these retain their _IR suffix?

I'd suggest going with intel_iommu_{en,dis}able_eim() to match the
supports name here, whereas...

> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -2720,6 +2720,8 @@ const struct iommu_ops __initconstrel in
>      .free_page_table = iommu_free_page_table,
>      .reassign_device = reassign_device_ownership,
>      .get_device_group_id = intel_iommu_group_id,
> +    .enable_x2apic_IR = intel_iommu_enable_x2apic_IR,
> +    .disable_x2apic_IR = intel_iommu_disable_x2apic_IR,
>      .update_ire_from_apic = io_apic_write_remap_rte,
>      .update_ire_from_msi = msi_msg_write_remap_rte,
>      .read_apic_from_ire = io_apic_read_remap_rte,
> @@ -2736,6 +2738,7 @@ const struct iommu_ops __initconstrel in
>  };
>  
>  const struct iommu_init_ops __initconstrel intel_iommu_init_ops = {
> +    .ops = &intel_iommu_ops,
>      .setup = vtd_setup,
>      .supports_x2apic = intel_iommu_supports_eim,
>  };
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -26,6 +26,24 @@
>  const struct iommu_init_ops *__initdata iommu_init_ops;
>  struct iommu_ops __read_mostly iommu_ops;
>  
> +int iommu_enable_x2apic_IR(void)

... using iommu_{en,dis}able_x2apic() here to match the
supports_x2apic() init hook.


I don't think these shorter names are any more ambiguous, and loosing
the _IR suffix does make them more consistent with the rest of Xen's
function naming conventions.

~Andrew

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

Re: [PATCH 6/7] x86/IOMMU: abstract Intel-specific iommu_{en, dis}able_x2apic_IR()

[Jan Beulich]

>>> On 28.03.19 at 18:37, <andrew.cooper3@citrix.com> wrote:
> On 28/03/2019 14:53, Jan Beulich wrote:
>> @@ -35,6 +35,8 @@ void print_vtd_entries(struct iommu *iom
>>  keyhandler_fn_t vtd_dump_iommu_info;
>>  
>>  bool intel_iommu_supports_eim(void);
>> +int intel_iommu_enable_x2apic_IR(void);
>> +void intel_iommu_disable_x2apic_IR(void);
> 
> Is there any particular reason why these retain their _IR suffix?

Well, I've too been thinking about the naming here. I decided to
keep the _IR suffixes because that's what the functions really
do: They enable/disable interrupt remapping for x2APIC mode.
They don't enable x2APIC itself in any way, and iirc x2APIC
mode could be used (without wider APIC IDs and in physical
mode) even without any IR enabled.

> I'd suggest going with intel_iommu_{en,dis}able_eim() to match the
> supports name here, whereas...
> 
>> --- a/xen/drivers/passthrough/vtd/iommu.c
>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>> @@ -2720,6 +2720,8 @@ const struct iommu_ops __initconstrel in
>>      .free_page_table = iommu_free_page_table,
>>      .reassign_device = reassign_device_ownership,
>>      .get_device_group_id = intel_iommu_group_id,
>> +    .enable_x2apic_IR = intel_iommu_enable_x2apic_IR,
>> +    .disable_x2apic_IR = intel_iommu_disable_x2apic_IR,
>>      .update_ire_from_apic = io_apic_write_remap_rte,
>>      .update_ire_from_msi = msi_msg_write_remap_rte,
>>      .read_apic_from_ire = io_apic_read_remap_rte,
>> @@ -2736,6 +2738,7 @@ const struct iommu_ops __initconstrel in
>>  };
>>  
>>  const struct iommu_init_ops __initconstrel intel_iommu_init_ops = {
>> +    .ops = &intel_iommu_ops,
>>      .setup = vtd_setup,
>>      .supports_x2apic = intel_iommu_supports_eim,
>>  };
>> --- a/xen/drivers/passthrough/x86/iommu.c
>> +++ b/xen/drivers/passthrough/x86/iommu.c
>> @@ -26,6 +26,24 @@
>>  const struct iommu_init_ops *__initdata iommu_init_ops;
>>  struct iommu_ops __read_mostly iommu_ops;
>>  
>> +int iommu_enable_x2apic_IR(void)
> 
> ... using iommu_{en,dis}able_x2apic() here to match the
> supports_x2apic() init hook.
> 
> 
> I don't think these shorter names are any more ambiguous, and loosing
> the _IR suffix does make them more consistent with the rest of Xen's
> function naming conventions.

The above said, in the end I'm not overly fussed, but before deciding
which route to go I'll wait to see whether in particular Kevin has an
opinion either way.

Jan



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

Re: [PATCH 6/7] x86/IOMMU: abstract Intel-specific iommu_{en, dis}able_x2apic_IR()

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Friday, March 29, 2019 5:13 PM
> 
> >>> On 28.03.19 at 18:37, <andrew.cooper3@citrix.com> wrote:
> > On 28/03/2019 14:53, Jan Beulich wrote:
> >> @@ -35,6 +35,8 @@ void print_vtd_entries(struct iommu *iom
> >>  keyhandler_fn_t vtd_dump_iommu_info;
> >>
> >>  bool intel_iommu_supports_eim(void);
> >> +int intel_iommu_enable_x2apic_IR(void);
> >> +void intel_iommu_disable_x2apic_IR(void);
> >
> > Is there any particular reason why these retain their _IR suffix?
> 
> Well, I've too been thinking about the naming here. I decided to
> keep the _IR suffixes because that's what the functions really
> do: They enable/disable interrupt remapping for x2APIC mode.
> They don't enable x2APIC itself in any way, and iirc x2APIC
> mode could be used (without wider APIC IDs and in physical
> mode) even without any IR enabled.
> 
> > I'd suggest going with intel_iommu_{en,dis}able_eim() to match the
> > supports name here, whereas...
> >
> >> --- a/xen/drivers/passthrough/vtd/iommu.c
> >> +++ b/xen/drivers/passthrough/vtd/iommu.c
> >> @@ -2720,6 +2720,8 @@ const struct iommu_ops __initconstrel in
> >>      .free_page_table = iommu_free_page_table,
> >>      .reassign_device = reassign_device_ownership,
> >>      .get_device_group_id = intel_iommu_group_id,
> >> +    .enable_x2apic_IR = intel_iommu_enable_x2apic_IR,
> >> +    .disable_x2apic_IR = intel_iommu_disable_x2apic_IR,
> >>      .update_ire_from_apic = io_apic_write_remap_rte,
> >>      .update_ire_from_msi = msi_msg_write_remap_rte,
> >>      .read_apic_from_ire = io_apic_read_remap_rte,
> >> @@ -2736,6 +2738,7 @@ const struct iommu_ops __initconstrel in
> >>  };
> >>
> >>  const struct iommu_init_ops __initconstrel intel_iommu_init_ops = {
> >> +    .ops = &intel_iommu_ops,
> >>      .setup = vtd_setup,
> >>      .supports_x2apic = intel_iommu_supports_eim,
> >>  };
> >> --- a/xen/drivers/passthrough/x86/iommu.c
> >> +++ b/xen/drivers/passthrough/x86/iommu.c
> >> @@ -26,6 +26,24 @@
> >>  const struct iommu_init_ops *__initdata iommu_init_ops;
> >>  struct iommu_ops __read_mostly iommu_ops;
> >>
> >> +int iommu_enable_x2apic_IR(void)
> >
> > ... using iommu_{en,dis}able_x2apic() here to match the
> > supports_x2apic() init hook.
> >
> >
> > I don't think these shorter names are any more ambiguous, and loosing
> > the _IR suffix does make them more consistent with the rest of Xen's
> > function naming conventions.
> 
> The above said, in the end I'm not overly fussed, but before deciding
> which route to go I'll wait to see whether in particular Kevin has an
> opinion either way.
> 

let's remove _IR. we have intel_iommu prefix which is sufficient
to indicate enable_x2apic in iommu context is about IR.

since renaming is small thing, here is my:

	Reviewed-by: Kevin Tian <kevin.tian@intel.com>

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

Re: [PATCH 6/7] x86/IOMMU: abstract Intel-specific iommu_{en, dis}able_x2apic_IR()

[Andrew Cooper]

On 02/04/2019 04:24, Tian, Kevin wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Friday, March 29, 2019 5:13 PM
>>
>>>>> On 28.03.19 at 18:37, <andrew.cooper3@citrix.com> wrote:
>>> On 28/03/2019 14:53, Jan Beulich wrote:
>>>> @@ -35,6 +35,8 @@ void print_vtd_entries(struct iommu *iom
>>>>  keyhandler_fn_t vtd_dump_iommu_info;
>>>>
>>>>  bool intel_iommu_supports_eim(void);
>>>> +int intel_iommu_enable_x2apic_IR(void);
>>>> +void intel_iommu_disable_x2apic_IR(void);
>>> Is there any particular reason why these retain their _IR suffix?
>> Well, I've too been thinking about the naming here. I decided to
>> keep the _IR suffixes because that's what the functions really
>> do: They enable/disable interrupt remapping for x2APIC mode.
>> They don't enable x2APIC itself in any way, and iirc x2APIC
>> mode could be used (without wider APIC IDs and in physical
>> mode) even without any IR enabled.
>>
>>> I'd suggest going with intel_iommu_{en,dis}able_eim() to match the
>>> supports name here, whereas...
>>>
>>>> --- a/xen/drivers/passthrough/vtd/iommu.c
>>>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>>>> @@ -2720,6 +2720,8 @@ const struct iommu_ops __initconstrel in
>>>>      .free_page_table = iommu_free_page_table,
>>>>      .reassign_device = reassign_device_ownership,
>>>>      .get_device_group_id = intel_iommu_group_id,
>>>> +    .enable_x2apic_IR = intel_iommu_enable_x2apic_IR,
>>>> +    .disable_x2apic_IR = intel_iommu_disable_x2apic_IR,
>>>>      .update_ire_from_apic = io_apic_write_remap_rte,
>>>>      .update_ire_from_msi = msi_msg_write_remap_rte,
>>>>      .read_apic_from_ire = io_apic_read_remap_rte,
>>>> @@ -2736,6 +2738,7 @@ const struct iommu_ops __initconstrel in
>>>>  };
>>>>
>>>>  const struct iommu_init_ops __initconstrel intel_iommu_init_ops = {
>>>> +    .ops = &intel_iommu_ops,
>>>>      .setup = vtd_setup,
>>>>      .supports_x2apic = intel_iommu_supports_eim,
>>>>  };
>>>> --- a/xen/drivers/passthrough/x86/iommu.c
>>>> +++ b/xen/drivers/passthrough/x86/iommu.c
>>>> @@ -26,6 +26,24 @@
>>>>  const struct iommu_init_ops *__initdata iommu_init_ops;
>>>>  struct iommu_ops __read_mostly iommu_ops;
>>>>
>>>> +int iommu_enable_x2apic_IR(void)
>>> ... using iommu_{en,dis}able_x2apic() here to match the
>>> supports_x2apic() init hook.
>>>
>>>
>>> I don't think these shorter names are any more ambiguous, and loosing
>>> the _IR suffix does make them more consistent with the rest of Xen's
>>> function naming conventions.
>> The above said, in the end I'm not overly fussed, but before deciding
>> which route to go I'll wait to see whether in particular Kevin has an
>> opinion either way.
>>
> let's remove _IR. we have intel_iommu prefix which is sufficient
> to indicate enable_x2apic in iommu context is about IR.
>
> since renaming is small thing, here is my:
>
> 	Reviewed-by: Kevin Tian <kevin.tian@intel.com>

No point posting this series a second time just for the rename.  With
the suggested adjustments, Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>

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

Re: [PATCH 6/7] x86/IOMMU: abstract Intel-specific iommu_{en, dis}able_x2apic_IR()

[Jan Beulich]

>>> On 02.04.19 at 05:24, <kevin.tian@intel.com> wrote:
>>  From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Friday, March 29, 2019 5:13 PM
>> 
>> >>> On 28.03.19 at 18:37, <andrew.cooper3@citrix.com> wrote:
>> > On 28/03/2019 14:53, Jan Beulich wrote:
>> >> @@ -35,6 +35,8 @@ void print_vtd_entries(struct iommu *iom
>> >>  keyhandler_fn_t vtd_dump_iommu_info;
>> >>
>> >>  bool intel_iommu_supports_eim(void);
>> >> +int intel_iommu_enable_x2apic_IR(void);
>> >> +void intel_iommu_disable_x2apic_IR(void);
>> >
>> > Is there any particular reason why these retain their _IR suffix?
>> 
>> Well, I've too been thinking about the naming here. I decided to
>> keep the _IR suffixes because that's what the functions really
>> do: They enable/disable interrupt remapping for x2APIC mode.
>> They don't enable x2APIC itself in any way, and iirc x2APIC
>> mode could be used (without wider APIC IDs and in physical
>> mode) even without any IR enabled.
>> 
>> > I'd suggest going with intel_iommu_{en,dis}able_eim() to match the
>> > supports name here, whereas...
>> >
>> >> --- a/xen/drivers/passthrough/vtd/iommu.c
>> >> +++ b/xen/drivers/passthrough/vtd/iommu.c
>> >> @@ -2720,6 +2720,8 @@ const struct iommu_ops __initconstrel in
>> >>      .free_page_table = iommu_free_page_table,
>> >>      .reassign_device = reassign_device_ownership,
>> >>      .get_device_group_id = intel_iommu_group_id,
>> >> +    .enable_x2apic_IR = intel_iommu_enable_x2apic_IR,
>> >> +    .disable_x2apic_IR = intel_iommu_disable_x2apic_IR,
>> >>      .update_ire_from_apic = io_apic_write_remap_rte,
>> >>      .update_ire_from_msi = msi_msg_write_remap_rte,
>> >>      .read_apic_from_ire = io_apic_read_remap_rte,
>> >> @@ -2736,6 +2738,7 @@ const struct iommu_ops __initconstrel in
>> >>  };
>> >>
>> >>  const struct iommu_init_ops __initconstrel intel_iommu_init_ops = {
>> >> +    .ops = &intel_iommu_ops,
>> >>      .setup = vtd_setup,
>> >>      .supports_x2apic = intel_iommu_supports_eim,
>> >>  };
>> >> --- a/xen/drivers/passthrough/x86/iommu.c
>> >> +++ b/xen/drivers/passthrough/x86/iommu.c
>> >> @@ -26,6 +26,24 @@
>> >>  const struct iommu_init_ops *__initdata iommu_init_ops;
>> >>  struct iommu_ops __read_mostly iommu_ops;
>> >>
>> >> +int iommu_enable_x2apic_IR(void)
>> >
>> > ... using iommu_{en,dis}able_x2apic() here to match the
>> > supports_x2apic() init hook.
>> >
>> >
>> > I don't think these shorter names are any more ambiguous, and loosing
>> > the _IR suffix does make them more consistent with the rest of Xen's
>> > function naming conventions.
>> 
>> The above said, in the end I'm not overly fussed, but before deciding
>> which route to go I'll wait to see whether in particular Kevin has an
>> opinion either way.
>> 
> 
> let's remove _IR. we have intel_iommu prefix which is sufficient
> to indicate enable_x2apic in iommu context is about IR.
> 
> since renaming is small thing, here is my:
> 
> 	Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Thanks, but well, I'll then follow Andrew's suggestion and also
name the VT-d functions intel_iommu_{en,dis}able_eim(). I
hope that's okay with you.

Jan



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

Re: [PATCH 6/7] x86/IOMMU: abstract Intel-specific iommu_{en, dis}able_x2apic_IR()

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Tuesday, April 2, 2019 9:17 PM
> 
> >>> On 02.04.19 at 05:24, <kevin.tian@intel.com> wrote:
> >>  From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Friday, March 29, 2019 5:13 PM
> >>
> >> >>> On 28.03.19 at 18:37, <andrew.cooper3@citrix.com> wrote:
> >> > On 28/03/2019 14:53, Jan Beulich wrote:
> >> >> @@ -35,6 +35,8 @@ void print_vtd_entries(struct iommu *iom
> >> >>  keyhandler_fn_t vtd_dump_iommu_info;
> >> >>
> >> >>  bool intel_iommu_supports_eim(void);
> >> >> +int intel_iommu_enable_x2apic_IR(void);
> >> >> +void intel_iommu_disable_x2apic_IR(void);
> >> >
> >> > Is there any particular reason why these retain their _IR suffix?
> >>
> >> Well, I've too been thinking about the naming here. I decided to
> >> keep the _IR suffixes because that's what the functions really
> >> do: They enable/disable interrupt remapping for x2APIC mode.
> >> They don't enable x2APIC itself in any way, and iirc x2APIC
> >> mode could be used (without wider APIC IDs and in physical
> >> mode) even without any IR enabled.
> >>
> >> > I'd suggest going with intel_iommu_{en,dis}able_eim() to match the
> >> > supports name here, whereas...
> >> >
> >> >> --- a/xen/drivers/passthrough/vtd/iommu.c
> >> >> +++ b/xen/drivers/passthrough/vtd/iommu.c
> >> >> @@ -2720,6 +2720,8 @@ const struct iommu_ops __initconstrel in
> >> >>      .free_page_table = iommu_free_page_table,
> >> >>      .reassign_device = reassign_device_ownership,
> >> >>      .get_device_group_id = intel_iommu_group_id,
> >> >> +    .enable_x2apic_IR = intel_iommu_enable_x2apic_IR,
> >> >> +    .disable_x2apic_IR = intel_iommu_disable_x2apic_IR,
> >> >>      .update_ire_from_apic = io_apic_write_remap_rte,
> >> >>      .update_ire_from_msi = msi_msg_write_remap_rte,
> >> >>      .read_apic_from_ire = io_apic_read_remap_rte,
> >> >> @@ -2736,6 +2738,7 @@ const struct iommu_ops __initconstrel in
> >> >>  };
> >> >>
> >> >>  const struct iommu_init_ops __initconstrel intel_iommu_init_ops = {
> >> >> +    .ops = &intel_iommu_ops,
> >> >>      .setup = vtd_setup,
> >> >>      .supports_x2apic = intel_iommu_supports_eim,
> >> >>  };
> >> >> --- a/xen/drivers/passthrough/x86/iommu.c
> >> >> +++ b/xen/drivers/passthrough/x86/iommu.c
> >> >> @@ -26,6 +26,24 @@
> >> >>  const struct iommu_init_ops *__initdata iommu_init_ops;
> >> >>  struct iommu_ops __read_mostly iommu_ops;
> >> >>
> >> >> +int iommu_enable_x2apic_IR(void)
> >> >
> >> > ... using iommu_{en,dis}able_x2apic() here to match the
> >> > supports_x2apic() init hook.
> >> >
> >> >
> >> > I don't think these shorter names are any more ambiguous, and loosing
> >> > the _IR suffix does make them more consistent with the rest of Xen's
> >> > function naming conventions.
> >>
> >> The above said, in the end I'm not overly fussed, but before deciding
> >> which route to go I'll wait to see whether in particular Kevin has an
> >> opinion either way.
> >>
> >
> > let's remove _IR. we have intel_iommu prefix which is sufficient
> > to indicate enable_x2apic in iommu context is about IR.
> >
> > since renaming is small thing, here is my:
> >
> > 	Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> 
> Thanks, but well, I'll then follow Andrew's suggestion and also
> name the VT-d functions intel_iommu_{en,dis}able_eim(). I
> hope that's okay with you.
> 

OK to me.

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

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

[Jan Beulich]

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>

--- 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);





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

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

[Andrew Cooper]

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

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

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

[Jan Beulich]

>>> 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



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

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

[Andrew Cooper]

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

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

[Tian, Kevin]

> 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>

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

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

[Woods, Brian]

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>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Woods, Brian]

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>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Ping: [PATCH 0/7] x86: eliminate Intel-isms from x2APIC setup

[Jan Beulich]

>>> On 28.03.19 at 15:43, <JBeulich@suse.com> wrote:
> This is a first preparatory step for enabling x2APIC support also
> for AMD (plus some misc cleanup).
> 
> 1: entry: drop unused header inclusions
> 2: APIC: suppress redundant "Switched to ..." messages
> 3: ACPI: also parse AMD IOMMU tables early
> 4: IOMMU: introduce init-ops structure
> 5: IOMMU: abstract Intel-specific iommu_supports_eim()
> 6: IOMMU: abstract Intel-specific iommu_{en,dis}able_x2apic_IR()
> 7: IOMMU: initialize iommu_ops in vendor-independent code

Where applicable, may I ask for an SVM / AMD IOMMU maintainer
ack (or otherwise) please?

Thanks, Jan



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

[Xen-devel] Ping: [PATCH 0/7] x86: eliminate Intel-isms from x2APIC setup

[Jan Beulich]

>>> On 28.03.19 at 15:43, <JBeulich@suse.com> wrote:
> This is a first preparatory step for enabling x2APIC support also
> for AMD (plus some misc cleanup).
> 
> 1: entry: drop unused header inclusions
> 2: APIC: suppress redundant "Switched to ..." messages
> 3: ACPI: also parse AMD IOMMU tables early
> 4: IOMMU: introduce init-ops structure
> 5: IOMMU: abstract Intel-specific iommu_supports_eim()
> 6: IOMMU: abstract Intel-specific iommu_{en,dis}able_x2apic_IR()
> 7: IOMMU: initialize iommu_ops in vendor-independent code

Where applicable, may I ask for an SVM / AMD IOMMU maintainer
ack (or otherwise) please?

Thanks, Jan



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