intel-iommu: restrict EIM to quirkless KVM

PATCH In review Radim Krčmář 5 participants 17 messages Aug 9, 2016 – Aug 11, 2016 Checkpatch

Share & tools

Radim Krčmář

author

2016-08-09 15:05 UTC #0 lore raw

cover intel-iommu: restrict EIM to quirkless KVM

EIM should have been disabled because it doesn't work with KVM nor QEMU APIC. This series proposes a way to keep it enabled on KVM v4.8+.
(Xen is silently ignored.)

Radim Krčmář (2):
linux-headers: update to v4.8-rc1
intel-iommu: restrict EIM to quirkless KVM

 hw/i386/intel_iommu.c     | 10 +++++++++-
 linux-headers/linux/kvm.h | 18 ++++++++++++++++--
 target-i386/kvm-stub.c    |  5 +++++
 target-i386/kvm.c         | 12 ++++++++++++
 target-i386/kvm_i386.h    |  1 +
 5 files changed, 43 insertions(+), 3 deletions(-)

Signature · 1 line

2.9.2

Radim Krčmář

author

2016-08-09 15:05 UTC #1 lore raw

PATCH 1/2 linux-headers: update to v4.8-rc1

Signed-off-by Radim Krčmář <[email protected]>

---
 linux-headers/linux/kvm.h | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

linux-headers/linux/kvm.h +16 -2

index e60e21ba227e..4806e069e749 100644
@@ -866,6 +866,10 @@ struct kvm_ppc_smmu_info {
 #define KVM_CAP_ARM_PMU_V3 126
 #define KVM_CAP_VCPU_ATTRIBUTES 127
 #define KVM_CAP_MAX_VCPU_ID 128
+#define KVM_CAP_X2APIC_API 129
+#define KVM_CAP_S390_USER_INSTR0 130
+#define KVM_CAP_MSI_DEVID 131
+#define KVM_CAP_PPC_HTM 132
 
 #ifdef KVM_CAP_IRQ_ROUTING

@@ -878,7 +882,10 @@ struct kvm_irq_routing_msi {
 	__u32 address_lo;
 	__u32 address_hi;
 	__u32 data;
-	__u32 pad;
+	union {
+		__u32 pad;
+		__u32 devid;
+	};
 };
 
 struct kvm_irq_routing_s390_adapter {

@@ -1024,12 +1031,14 @@ struct kvm_one_reg {
 	__u64 addr;
 };
 
+#define KVM_MSI_VALID_DEVID	(1U << 0)
 struct kvm_msi {
 	__u32 address_lo;
 	__u32 address_hi;
 	__u32 data;
 	__u32 flags;
-	__u8  pad[16];
+	__u32 devid;
+	__u8  pad[12];
 };
 
 struct kvm_arm_device_addr {

@@ -1074,6 +1083,8 @@ enum kvm_device_type {
 #define KVM_DEV_TYPE_FLIC		KVM_DEV_TYPE_FLIC
 	KVM_DEV_TYPE_ARM_VGIC_V3,
 #define KVM_DEV_TYPE_ARM_VGIC_V3	KVM_DEV_TYPE_ARM_VGIC_V3
+	KVM_DEV_TYPE_ARM_VGIC_ITS,
+#define KVM_DEV_TYPE_ARM_VGIC_ITS	KVM_DEV_TYPE_ARM_VGIC_ITS
 	KVM_DEV_TYPE_MAX,
 };

@@ -1313,4 +1324,7 @@ struct kvm_assigned_msix_entry {
 	__u16 padding[3];
 };
 
+#define KVM_X2APIC_API_USE_32BIT_IDS            (1ULL << 0)
+#define KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK  (1ULL << 1)
+
 #endif /* __LINUX_KVM_H */

Signature · 1 line

2.9.2

Radim Krčmář

author

2016-08-09 16:11 UTC #2 lore raw

Re: [Qemu-devel] [PATCH 1/2] linux-headers: update to v4.8-rc1

2016-08-09 17:03+0200, Radim Krčmář:

Quoted 3 lines

Signed-off-by Radim Krčmář <[email protected]>
---
 linux-headers/linux/kvm.h | 18 ++++++++++++++++--

This patch did not update all headers. v2 will have

 include/standard-headers/linux/input-event-codes.h | 32 +++++++++++++++++
 include/standard-headers/linux/input.h             |  1 +
 include/standard-headers/linux/virtio_config.h     | 10 +++++-
 include/standard-headers/linux/virtio_ids.h        |  1 +
 include/standard-headers/linux/virtio_net.h        |  3 ++
 linux-headers/asm-arm/kvm.h                        |  4 +--
 linux-headers/asm-arm64/kvm.h                      |  2 ++
 linux-headers/asm-s390/kvm.h                       | 41 ++++++++++++++++++++++
 linux-headers/asm-x86/unistd_x32.h                 |  4 +--
 linux-headers/linux/kvm.h                          | 18 ++++++++--
 linux-headers/linux/vhost.h                        | 33 +++++++++++++++++

Radim Krčmář

author

2016-08-09 15:05 UTC #3 lore raw

PATCH 2/2 intel-iommu: restrict EIM to quirkless KVM

In reply to Radim Krčmář

APIC in QEMU doesn't support x2APIC so exposing EIM is pointless and KVM has a quirk that needs to be disabled unless we want x2APIC message with destination 0xff to be misinterpreted as a broadcast.

Signed-off-by Radim Krčmář <[email protected]>

---
 hw/i386/intel_iommu.c  | 10 +++++++++-
 target-i386/kvm-stub.c |  5 +++++
 target-i386/kvm.c      | 12 ++++++++++++
 target-i386/kvm_i386.h |  1 +
 4 files changed, 27 insertions(+), 1 deletion(-)

hw/i386/intel_iommu.c +9 -1

index 28c31a2cdfa3..733751923233 100644
@@ -31,6 +31,7 @@
 #include "hw/i386/x86-iommu.h"
 #include "hw/pci-host/q35.h"
 #include "sysemu/kvm.h"
+#include "kvm_i386.h"
 
 /*#define DEBUG_INTEL_IOMMU*/
 #ifdef DEBUG_INTEL_IOMMU

@@ -2364,7 +2365,14 @@ static void vtd_init(IntelIOMMUState *s)
     s->ecap = VTD_ECAP_QI | VTD_ECAP_IRO;
 
     if (x86_iommu->intr_supported) {
-        s->ecap |= VTD_ECAP_IR | VTD_ECAP_EIM | VTD_ECAP_MHMV;
+        s->ecap |= VTD_ECAP_IR | VTD_ECAP_MHMV;
+        /* QEMU APIC does not support x2APIC and KVM does not work well without
+         * disabling a quirk.  IOMMU is unmigratable so we unconditionally use
+         * optional KVM features.
+         */
+        if (kvm_irqchip_in_kernel() && kvm_disable_x2apic_broadcast_quirk()) {
+            s->ecap |= VTD_ECAP_EIM;
+        }
     }
 
     vtd_reset_context_cache(s);

target-i386/kvm-stub.c +5 -0

index cdf15061091d..71be0bd94ddc 100644
@@ -25,6 +25,11 @@ bool kvm_has_smm(void)
     return 1;
 }
 
+bool kvm_disable_x2apic_broadcast_quirk(void)
+{
+    return false;
+}
+
 /* This function is only called inside conditionals which we
  * rely on the compiler to optimize out when CONFIG_KVM is not
  * defined.

target-i386/kvm.c +12 -0

index 0b2016a77a3c..050c850d77d3 100644
@@ -128,6 +128,18 @@ bool kvm_allows_irq0_override(void)
     return !kvm_irqchip_in_kernel() || kvm_has_gsi_routing();
 }
 
+static bool kvm_x2apic_api_set_flags(uint64_t flags)
+{
+    KVMState *s = KVM_STATE(current_machine->accelerator);
+
+    return !kvm_vm_enable_cap(s, KVM_CAP_X2APIC_API, 0, flags);
+}
+
+bool kvm_disable_x2apic_broadcast_quirk(void)
+{
+    return kvm_x2apic_api_set_flags(KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK);
+}
+
 static int kvm_get_tsc(CPUState *cs)
 {
     X86CPU *cpu = X86_CPU(cs);

target-i386/kvm_i386.h +1 -0

index 42b00af1b1c3..1bc445d59c83 100644
@@ -41,4 +41,5 @@ int kvm_device_msix_set_vector(KVMState *s, uint32_t dev_id, uint32_t vector,
 int kvm_device_msix_assign(KVMState *s, uint32_t dev_id);
 int kvm_device_msix_deassign(KVMState *s, uint32_t dev_id);
 
+bool kvm_disable_x2apic_broadcast_quirk(void);
 #endif

Signature · 1 line

2.9.2

Peter Xu

2016-08-10 03:29 UTC #4 lore raw

Quoted 27 lines · Radim Krčmář

APIC in QEMU doesn't support x2APIC so exposing EIM is pointless and KVM has a quirk that needs to be disabled unless we want x2APIC message with destination 0xff to be misinterpreted as a broadcast.
Signed-off-by Radim Krčmář <[email protected]>
---
 hw/i386/intel_iommu.c  | 10 +++++++++-
 target-i386/kvm-stub.c |  5 +++++
 target-i386/kvm.c      | 12 ++++++++++++
 target-i386/kvm_i386.h |  1 +
 4 files changed, 27 insertions(+), 1 deletion(-)
hw/i386/intel_iommu.c +1 -0
index 28c31a2cdfa3..733751923233 100644
@@ -31,6 +31,7 @@
 #include "hw/i386/x86-iommu.h"
 #include "hw/pci-host/q35.h"
 #include "sysemu/kvm.h"
+#include "kvm_i386.h"
 
 /*#define DEBUG_INTEL_IOMMU*/
 #ifdef DEBUG_INTEL_IOMMU
@@ -2364,7 +2365,14 @@ static void vtd_init(IntelIOMMUState *s)
     s->ecap = VTD_ECAP_QI | VTD_ECAP_IRO;
 

hw/i386/intel_iommu.c static void vtd_init(IntelIOMMUState *s) · line 2375

Quoted 10 lines

hw/i386/intel_iommu.c +8 -1

     if (x86_iommu->intr_supported) {
-        s->ecap |= VTD_ECAP_IR | VTD_ECAP_EIM | VTD_ECAP_MHMV;
+        s->ecap |= VTD_ECAP_IR | VTD_ECAP_MHMV;
+        /* QEMU APIC does not support x2APIC and KVM does not work well without
+         * disabling a quirk.  IOMMU is unmigratable so we unconditionally use
+         * optional KVM features.
+         */
+        if (kvm_irqchip_in_kernel() && kvm_disable_x2apic_broadcast_quirk()) {
+            s->ecap |= VTD_ECAP_EIM;
+        }

Good to me if this patch is only going to disable x2apic when we failed to disable the x2apic broadcast quirk in KVM.

Question: still not too clear about how KVM treats the case when x2apic and xapic are used in a single VM. E.g., if dest_id of an interrupt is 0xff from a peripheral device, how should I know this is a x2apic broadcast to 0-7 cpu in cluster 0, or an apic broadcast to all?

Thanks,

-- peterx

Radim Krčmář

author

2016-08-10 16:59 UTC #5 lore raw

2016-08-10 11:29+0800, Peter Xu:

Quoted 19 lines · Radim Krčmář

Quoted 18 lines
APIC in QEMU doesn't support x2APIC so exposing EIM is pointless and KVM has a quirk that needs to be disabled unless we want x2APIC message with destination 0xff to be misinterpreted as a broadcast.
Signed-off-by Radim Krčmář <[email protected]>
---
hw/i386/intel_iommu.c +1 -0
@@ -31,6 +31,7 @@
 #include "hw/i386/x86-iommu.h"
 #include "hw/pci-host/q35.h"
 #include "sysemu/kvm.h"
+#include "kvm_i386.h"
 
 /*#define DEBUG_INTEL_IOMMU*/
 #ifdef DEBUG_INTEL_IOMMU
@@ -2364,7 +2365,14 @@ static void vtd_init(IntelIOMMUState *s)
     s->ecap = VTD_ECAP_QI | VTD_ECAP_IRO;
 

Quoted 13 lines

hw/i386/intel_iommu.c static void vtd_init(IntelIOMMUState *s) · line 2375

Quoted 10 lines

hw/i386/intel_iommu.c +8 -1

     if (x86_iommu->intr_supported) {
-        s->ecap |= VTD_ECAP_IR | VTD_ECAP_EIM | VTD_ECAP_MHMV;
+        s->ecap |= VTD_ECAP_IR | VTD_ECAP_MHMV;
+        /* QEMU APIC does not support x2APIC and KVM does not work well without
+         * disabling a quirk.  IOMMU is unmigratable so we unconditionally use
+         * optional KVM features.
+         */
+        if (kvm_irqchip_in_kernel() && kvm_disable_x2apic_broadcast_quirk()) {
+            s->ecap |= VTD_ECAP_EIM;
+        }

Good to me if this patch is only going to disable x2apic when we failed to disable the x2apic broadcast quirk in KVM.

Do you mean to also allow QEMU's APIC?

  if (!kvm_irqchip_in_kernel() || kvm_disable_x2apic_broadcast_quirk())

Thanks.

Quoted 5 lines

Question: still not too clear about how KVM treats the case when x2apic and xapic are used in a single VM. E.g., if dest_id of an interrupt is 0xff from a peripheral device, how should I know this is a x2apic broadcast to 0-7 cpu in cluster 0, or an apic broadcast to all?

If a KVM guest has LAPICs in both x and x2 modes, then every interrupt arrives to all LAPICs and is accepted according to ID/LDR/DFR where every LAPIC assumes that the sender matches LAPIC's mode => all xLAPICs would accept 0xff and x2LAPICs with ID 0-7 would as well. kvm_apic_match_dest() is the function that decides and kvm_apic_mda() does most of the magic. The quirk disables a case that translated 0xff to 0xffffffff for x2LAPICs.

I don't know how real hardware does it and the behavior might even differ between FSB and QPI. I think KVM differs from both of them, but it's not that any behavior makes a difference in practice, so running a test kernel to figure it out has never been a priority ...

Peter Xu

Reviewed-by

2016-08-11 03:34 UTC #6 lore raw

Quoted 34 lines · Radim Krčmář

2016-08-10 11:29+0800, Peter Xu:
Quoted 19 lines · Radim Krčmář
Quoted 18 lines
APIC in QEMU doesn't support x2APIC so exposing EIM is pointless and KVM has a quirk that needs to be disabled unless we want x2APIC message with destination 0xff to be misinterpreted as a broadcast.
Signed-off-by Radim Krčmář <[email protected]>
---
hw/i386/intel_iommu.c +1 -0
@@ -31,6 +31,7 @@
 #include "hw/i386/x86-iommu.h"
 #include "hw/pci-host/q35.h"
 #include "sysemu/kvm.h"
+#include "kvm_i386.h"
 
 /*#define DEBUG_INTEL_IOMMU*/
 #ifdef DEBUG_INTEL_IOMMU
@@ -2364,7 +2365,14 @@ static void vtd_init(IntelIOMMUState *s)
     s->ecap = VTD_ECAP_QI | VTD_ECAP_IRO;
 
Quoted 13 lines
hw/i386/intel_iommu.c static void vtd_init(IntelIOMMUState *s) · line 2375
Quoted 10 lines
hw/i386/intel_iommu.c +8 -1
     if (x86_iommu->intr_supported) {
-        s->ecap |= VTD_ECAP_IR | VTD_ECAP_EIM | VTD_ECAP_MHMV;
+        s->ecap |= VTD_ECAP_IR | VTD_ECAP_MHMV;
+        /* QEMU APIC does not support x2APIC and KVM does not work well without
+         * disabling a quirk.  IOMMU is unmigratable so we unconditionally use
+         * optional KVM features.
+         */
+        if (kvm_irqchip_in_kernel() && kvm_disable_x2apic_broadcast_quirk()) {
+            s->ecap |= VTD_ECAP_EIM;
+        }
Good to me if this patch is only going to disable x2apic when we failed to disable the x2apic broadcast quirk in KVM.

Quoted 5 lines

Do you mean to also allow QEMU's APIC?
  if (!kvm_irqchip_in_kernel() || kvm_disable_x2apic_broadcast_quirk())
Thanks.

Nop. But, now I understand your mean, and please take this:

Reviewed-by Peter Xu <[email protected]>

Quoted 15 lines

Quoted 6 lines
Question: still not too clear about how KVM treats the case when x2apic and xapic are used in a single VM. E.g., if dest_id of an interrupt is 0xff from a peripheral device, how should I know this is a x2apic broadcast to 0-7 cpu in cluster 0, or an apic broadcast to all?
If a KVM guest has LAPICs in both x and x2 modes, then every interrupt arrives to all LAPICs and is accepted according to ID/LDR/DFR where every LAPIC assumes that the sender matches LAPIC's mode => all xLAPICs would accept 0xff and x2LAPICs with ID 0-7 would as well. kvm_apic_match_dest() is the function that decides and kvm_apic_mda() does most of the magic. The quirk disables a case that translated 0xff to 0xffffffff for x2LAPICs.

Quoted 4 lines

I don't know how real hardware does it and the behavior might even differ between FSB and QPI. I think KVM differs from both of them, but it's not that any behavior makes a difference in practice, so running a test kernel to figure it out has never been a priority ...

Thank you for the explaination!

I feel I am getting close to the nightmares. :)

-- peterx

[email protected]

bot

2016-08-09 15:32 UTC #7 lore raw

In reply to Radim Krčmář

Your series seems to have some coding style problems. See output below for more information:

Hi,

Your series seems to have some coding style problems. See output below for more information:

Message-id: [email protected]
Type: series
Subject: [Qemu-devel] [PATCH for-2.7 0/2] intel-iommu: restrict EIM to quirkless KVM

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do

    echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))

done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test'
e018fb0 intel-iommu: restrict EIM to quirkless KVM 5ef6f2f linux-headers: update to v4.8-rc1

=== OUTPUT BEGIN ===
Checking PATCH 1/2: linux-headers: update to v4.8-rc1...
ERROR: code indent should never use tabs
#32: FILE: linux-headers/linux/kvm.h:885:
+^Iunion {$

ERROR: code indent should never use tabs
#33: FILE: linux-headers/linux/kvm.h:886:
+^I^I__u32 pad;$

ERROR: code indent should never use tabs
#34: FILE: linux-headers/linux/kvm.h:887:
+^I^I__u32 devid;$

ERROR: code indent should never use tabs
#35: FILE: linux-headers/linux/kvm.h:888:
+^I};$

ERROR: code indent should never use tabs
#43: FILE: linux-headers/linux/kvm.h:1034:
+#define KVM_MSI_VALID_DEVID^I(1U << 0)$

ERROR: code indent should never use tabs
#50: FILE: linux-headers/linux/kvm.h:1040:
+^I__u32 devid;$

ERROR: code indent should never use tabs
#51: FILE: linux-headers/linux/kvm.h:1041:
+^I__u8 pad[12];$

ERROR: code indent should never use tabs
#59: FILE: linux-headers/linux/kvm.h:1086:
+^IKVM_DEV_TYPE_ARM_VGIC_ITS,$

ERROR: code indent should never use tabs
#60: FILE: linux-headers/linux/kvm.h:1087:
+#define KVM_DEV_TYPE_ARM_VGIC_ITS^IKVM_DEV_TYPE_ARM_VGIC_ITS$

total: 9 errors, 0 warnings, 51 lines checked

Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS.

Checking PATCH 2/2: intel-iommu: restrict EIM to quirkless KVM... === OUTPUT END ===

Test command exited with code: 1

---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to [email protected]

Radim Krčmář

author

2016-08-09 16:07 UTC #8 lore raw

PATCH 1/1 checkpatch: allow tabs in linux-headers

2016-08-09 08:31-0700, [email protected]:

Quoted 73 lines

Hi,
Your series seems to have some coding style problems. See output below for more information:
Message-id: [email protected]
Type: series
Subject: [Qemu-devel] [PATCH for-2.7 0/2] intel-iommu: restrict EIM to quirkless KVM
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do
    echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test'
e018fb0 intel-iommu: restrict EIM to quirkless KVM 5ef6f2f linux-headers: update to v4.8-rc1
=== OUTPUT BEGIN ===
Checking PATCH 1/2: linux-headers: update to v4.8-rc1...
ERROR: code indent should never use tabs
#32: FILE: linux-headers/linux/kvm.h:885:
+^Iunion {$
ERROR: code indent should never use tabs
#33: FILE: linux-headers/linux/kvm.h:886:
+^I^I__u32 pad;$
ERROR: code indent should never use tabs
#34: FILE: linux-headers/linux/kvm.h:887:
+^I^I__u32 devid;$
ERROR: code indent should never use tabs
#35: FILE: linux-headers/linux/kvm.h:888:
+^I};$
ERROR: code indent should never use tabs
#43: FILE: linux-headers/linux/kvm.h:1034:
+#define KVM_MSI_VALID_DEVID^I(1U << 0)$
ERROR: code indent should never use tabs
#50: FILE: linux-headers/linux/kvm.h:1040:
+^I__u32 devid;$
ERROR: code indent should never use tabs
#51: FILE: linux-headers/linux/kvm.h:1041:
+^I__u8 pad[12];$
ERROR: code indent should never use tabs
#59: FILE: linux-headers/linux/kvm.h:1086:
+^IKVM_DEV_TYPE_ARM_VGIC_ITS,$
ERROR: code indent should never use tabs
#60: FILE: linux-headers/linux/kvm.h:1087:
+#define KVM_DEV_TYPE_ARM_VGIC_ITS^IKVM_DEV_TYPE_ARM_VGIC_ITS$

Quoted 5 lines

total: 9 errors, 0 warnings, 51 lines checked
Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS.

These indentation errors are false positives.
---8<---
Make scripts/checkpatch.pl accept tabs in linux-headers/, instead of changing scripts/update-linux-headers.sh to expand tabs when importing.

Signed-off-by Radim Krčmář <[email protected]>

---

scripts/checkpatch.pl +1 -1

index 929708721299..38232d4b25c3 100755
@@ -1355,7 +1355,7 @@ sub process {
 		next if ($realfile !~ /\.(h|c|cpp|pl)$/);
 
 # in QEMU, no tabs are allowed
-		if ($rawline =~ /^\+.*\t/) {
+		if ($rawline =~ /^\+.*\t/ && $realfile !~ /^linux-headers\//) {
 			my $herevet = "$here\n" . cat_vet($rawline) . "\n";
 			ERROR("code indent should never use tabs\n" . $herevet);
 			$rpt_cleaners = 1;

Paolo Bonzini

2016-08-09 16:15 UTC #9 lore raw

Re: [Qemu-devel] [PATCH] checkpatch: allow tabs in linux-headers

----- Original Message -----

Quoted 103 lines

From: "Radim Krčmář" <[email protected]>
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], "jan kiszka" <[email protected]>, [email protected], [email protected]
Sent: Tuesday, August 9, 2016 6:07:04 PM
Subject: [PATCH] checkpatch: allow tabs in linux-headers
2016-08-09 08:31-0700, [email protected]:
Quoted 76 lines
Hi,
Your series seems to have some coding style problems. See output below for more information:
Message-id: [email protected]
Type: series
Subject: [Qemu-devel] [PATCH for-2.7 0/2] intel-iommu: restrict EIM to quirkless KVM
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do
    echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s
    $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -;
    then
        failed=1
        echo
    fi
    n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test'
e018fb0 intel-iommu: restrict EIM to quirkless KVM 5ef6f2f linux-headers: update to v4.8-rc1
=== OUTPUT BEGIN ===
Checking PATCH 1/2: linux-headers: update to v4.8-rc1...
ERROR: code indent should never use tabs
#32: FILE: linux-headers/linux/kvm.h:885:
+^Iunion {$
ERROR: code indent should never use tabs
#33: FILE: linux-headers/linux/kvm.h:886:
+^I^I__u32 pad;$
ERROR: code indent should never use tabs
#34: FILE: linux-headers/linux/kvm.h:887:
+^I^I__u32 devid;$
ERROR: code indent should never use tabs
#35: FILE: linux-headers/linux/kvm.h:888:
+^I};$
ERROR: code indent should never use tabs
#43: FILE: linux-headers/linux/kvm.h:1034:
+#define KVM_MSI_VALID_DEVID^I(1U << 0)$
ERROR: code indent should never use tabs
#50: FILE: linux-headers/linux/kvm.h:1040:
+^I__u32 devid;$
ERROR: code indent should never use tabs
#51: FILE: linux-headers/linux/kvm.h:1041:
+^I__u8 pad[12];$
ERROR: code indent should never use tabs
#59: FILE: linux-headers/linux/kvm.h:1086:
+^IKVM_DEV_TYPE_ARM_VGIC_ITS,$
ERROR: code indent should never use tabs
#60: FILE: linux-headers/linux/kvm.h:1087:
+#define KVM_DEV_TYPE_ARM_VGIC_ITS^IKVM_DEV_TYPE_ARM_VGIC_ITS$
Quoted 5 lines
total: 9 errors, 0 warnings, 51 lines checked
Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS.
These indentation errors are false positives.
---8<---
Make scripts/checkpatch.pl accept tabs in linux-headers/, instead of changing scripts/update-linux-headers.sh to expand tabs when importing.
Signed-off-by Radim Krčmář <[email protected]>
---
scripts/checkpatch.pl +0 -0
index 929708721299..38232d4b25c3 100755

scripts/checkpatch.pl sub process { · line 1361

Quoted 8 lines

scripts/checkpatch.pl +1 -1

@@ -1355,7 +1355,7 @@ sub process {
 		next if ($realfile !~ /\.(h|c|cpp|pl)$/);
 
 # in QEMU, no tabs are allowed
-		if ($rawline =~ /^\+.*\t/) {
+		if ($rawline =~ /^\+.*\t/ && $realfile !~ /^linux-headers\//) {
 			my $herevet = "$here\n" . cat_vet($rawline) . "\n";
 			ERROR("code indent should never use tabs\n" . $herevet);
 			$rpt_cleaners = 1;

Could you do the same for standard-headers/ too?

Thanks,

Paolo

Radim Krčmář

author

2016-08-09 16:37 UTC #10 lore raw

Re: [Qemu-devel] [PATCH] checkpatch: allow tabs in linux-headers

Quoted 12 lines

Quoted 12 lines
Make scripts/checkpatch.pl accept tabs in linux-headers/, instead of changing scripts/update-linux-headers.sh to expand tabs when importing.
Signed-off-by Radim Krčmář <[email protected]>
---
scripts/checkpatch.pl +0 -0
index 929708721299..38232d4b25c3 100755
@@ -1355,7 +1355,7 @@ sub process {
 		next if ($realfile !~ /\.(h|c|cpp|pl)$/);
 

Quoted 9 lines

scripts/checkpatch.pl sub process { · line 1361

Quoted 7 lines

scripts/checkpatch.pl +1 -1

 # in QEMU, no tabs are allowed
-		if ($rawline =~ /^\+.*\t/) {
+		if ($rawline =~ /^\+.*\t/ && $realfile !~ /^linux-headers\//) {
 			my $herevet = "$here\n" . cat_vet($rawline) . "\n";
 			ERROR("code indent should never use tabs\n" . $herevet);
 			$rpt_cleaners = 1;

Could you do the same for standard-headers/ too?

Sure, and when we are at it ... are *.pl files going to be reindented?

e.g. checkpatch.pl uses tabs consistently, get_maintainer.pl is a horrible mix of tabs and spaces [1], and clean-header-guards.pl uses 4 spaces for an indent.

---
1: get_maintainer.pl assumes that tab is 8 spaces and uses 1 tab for
every two consecutive indents and 4 spaces for indents that cannot be tabified, then tops the ugliness by also using 8 and 12 spaces for 2 and 3 indents, respectively.

Paolo Bonzini

2016-08-09 16:39 UTC #11 lore raw

Re: [Qemu-devel] [PATCH] checkpatch: allow tabs in linux-headers

Quoted 22 lines · Radim Krčmář

Quoted 12 lines
Quoted 12 lines
Make scripts/checkpatch.pl accept tabs in linux-headers/, instead of changing scripts/update-linux-headers.sh to expand tabs when importing.
Signed-off-by Radim Krčmář <[email protected]>
---
scripts/checkpatch.pl +0 -0
index 929708721299..38232d4b25c3 100755
@@ -1355,7 +1355,7 @@ sub process {
 		next if ($realfile !~ /\.(h|c|cpp|pl)$/);
 
Quoted 9 lines
scripts/checkpatch.pl sub process { · line 1361
Quoted 7 lines
scripts/checkpatch.pl +1 -1
 # in QEMU, no tabs are allowed
-		if ($rawline =~ /^\+.*\t/) {
+		if ($rawline =~ /^\+.*\t/ && $realfile !~ /^linux-headers\//) {
 			my $herevet = "$here\n" . cat_vet($rawline) . "\n";
 			ERROR("code indent should never use tabs\n" . $herevet);
 			$rpt_cleaners = 1;
Could you do the same for standard-headers/ too?

Quoted 5 lines

Sure, and when we are at it ... are *.pl files going to be reindented?
e.g. checkpatch.pl uses tabs consistently, get_maintainer.pl is a horrible mix of tabs and spaces [1], and clean-header-guards.pl uses 4 spaces for an indent.

I've sent a patch series to fix the most obvious issues in automated checkpatch email, it skips the tab check on imported Perl scripts.

Paolo

Radim Krčmář

author

2016-08-09 16:57 UTC #12 lore raw

Re: [Qemu-devel] [PATCH] checkpatch: allow tabs in linux-headers

2016-08-09 18:39+0200, Paolo Bonzini:

Quoted 23 lines · Radim Krčmář

Quoted 22 lines
Quoted 21 lines
Quoted 11 lines
Make scripts/checkpatch.pl accept tabs in linux-headers/, instead of changing scripts/update-linux-headers.sh to expand tabs when importing.
Signed-off-by Radim Krčmář <[email protected]>
---
scripts/checkpatch.pl +0 -0
index 929708721299..38232d4b25c3 100755
scripts/checkpatch.pl sub process { · line 1361
Quoted 8 lines
scripts/checkpatch.pl +1 -1
@@ -1355,7 +1355,7 @@ sub process {
 		next if ($realfile !~ /\.(h|c|cpp|pl)$/);
 
 # in QEMU, no tabs are allowed
-		if ($rawline =~ /^\+.*\t/) {
+		if ($rawline =~ /^\+.*\t/ && $realfile !~ /^linux-headers\//) {
 			my $herevet = "$here\n" . cat_vet($rawline) . "\n";
 			ERROR("code indent should never use tabs\n" . $herevet);
 			$rpt_cleaners = 1;
Could you do the same for standard-headers/ too?

Quoted 8 lines

Quoted 5 lines
Sure, and when we are at it ... are *.pl files going to be reindented?
e.g. checkpatch.pl uses tabs consistently, get_maintainer.pl is a horrible mix of tabs and spaces [1], and clean-header-guards.pl uses 4 spaces for an indent.
I've sent a patch series to fix the most obvious issues in automated checkpatch email, it skips the tab check on imported Perl scripts.

I will base on that, thanks.

Cornelia Huck

2016-08-10 07:10 UTC #13 lore raw

Re: [Qemu-devel] [PATCH] checkpatch: allow tabs in linux-headers

In reply to Paolo Bonzini

Quoted 107 lines · On Tue, 9 Aug 2016 12:14:14 -0400 (EDT) Paolo Bonzini <[email protected]>…

----- Original Message -----
Quoted 104 lines
From: "Radim Krčmář" <[email protected]>
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], "jan kiszka" <[email protected]>, [email protected], [email protected]
Sent: Tuesday, August 9, 2016 6:07:04 PM
Subject: [PATCH] checkpatch: allow tabs in linux-headers
2016-08-09 08:31-0700, [email protected]:
Quoted 76 lines
Hi,
Your series seems to have some coding style problems. See output below for more information:
Message-id: [email protected]
Type: series
Subject: [Qemu-devel] [PATCH for-2.7 0/2] intel-iommu: restrict EIM to quirkless KVM
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do
    echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s
    $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -;
    then
        failed=1
        echo
    fi
    n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test'
e018fb0 intel-iommu: restrict EIM to quirkless KVM 5ef6f2f linux-headers: update to v4.8-rc1
=== OUTPUT BEGIN ===
Checking PATCH 1/2: linux-headers: update to v4.8-rc1...
ERROR: code indent should never use tabs
#32: FILE: linux-headers/linux/kvm.h:885:
+^Iunion {$
ERROR: code indent should never use tabs
#33: FILE: linux-headers/linux/kvm.h:886:
+^I^I__u32 pad;$
ERROR: code indent should never use tabs
#34: FILE: linux-headers/linux/kvm.h:887:
+^I^I__u32 devid;$
ERROR: code indent should never use tabs
#35: FILE: linux-headers/linux/kvm.h:888:
+^I};$
ERROR: code indent should never use tabs
#43: FILE: linux-headers/linux/kvm.h:1034:
+#define KVM_MSI_VALID_DEVID^I(1U << 0)$
ERROR: code indent should never use tabs
#50: FILE: linux-headers/linux/kvm.h:1040:
+^I__u32 devid;$
ERROR: code indent should never use tabs
#51: FILE: linux-headers/linux/kvm.h:1041:
+^I__u8 pad[12];$
ERROR: code indent should never use tabs
#59: FILE: linux-headers/linux/kvm.h:1086:
+^IKVM_DEV_TYPE_ARM_VGIC_ITS,$
ERROR: code indent should never use tabs
#60: FILE: linux-headers/linux/kvm.h:1087:
+#define KVM_DEV_TYPE_ARM_VGIC_ITS^IKVM_DEV_TYPE_ARM_VGIC_ITS$
Quoted 5 lines
total: 9 errors, 0 warnings, 51 lines checked
Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS.
These indentation errors are false positives.
---8<---
Make scripts/checkpatch.pl accept tabs in linux-headers/, instead of changing scripts/update-linux-headers.sh to expand tabs when importing.
Signed-off-by Radim Krčmář <[email protected]>
---
scripts/checkpatch.pl +0 -0
index 929708721299..38232d4b25c3 100755
@@ -1355,7 +1355,7 @@ sub process {
 		next if ($realfile !~ /\.(h|c|cpp|pl)$/);
 

Quoted 9 lines

scripts/checkpatch.pl sub process { · line 1361

Quoted 7 lines

scripts/checkpatch.pl +1 -1

 # in QEMU, no tabs are allowed
-		if ($rawline =~ /^\+.*\t/) {
+		if ($rawline =~ /^\+.*\t/ && $realfile !~ /^linux-headers\//) {
 			my $herevet = "$here\n" . cat_vet($rawline) . "\n";
 			ERROR("code indent should never use tabs\n" . $herevet);
 			$rpt_cleaners = 1;

Could you do the same for standard-headers/ too?

I think it would be better to not apply any qemu coding style checks to a headers update. Something like 'check if this contains header updates _only_' would make more sense, but that is beyond my nonexisting perl skills...

Radim Krčmář

author

2016-08-10 13:55 UTC #14 lore raw

Re: [Qemu-devel] [PATCH] checkpatch: allow tabs in linux-headers

2016-08-10 09:09+0200, Cornelia Huck:

Quoted 24 lines

Quoted 12 lines · On Tue, 9 Aug 2016 12:14:14 -0400 (EDT) Paolo Bonzini <[email protected]>…
Quoted 12 lines
Make scripts/checkpatch.pl accept tabs in linux-headers/, instead of changing scripts/update-linux-headers.sh to expand tabs when importing.
Signed-off-by Radim Krčmář <[email protected]>
---
scripts/checkpatch.pl +0 -0
index 929708721299..38232d4b25c3 100755
@@ -1355,7 +1355,7 @@ sub process {
 		next if ($realfile !~ /\.(h|c|cpp|pl)$/);
 
Quoted 9 lines
scripts/checkpatch.pl sub process { · line 1361
Quoted 7 lines
scripts/checkpatch.pl +1 -1
 # in QEMU, no tabs are allowed
-		if ($rawline =~ /^\+.*\t/) {
+		if ($rawline =~ /^\+.*\t/ && $realfile !~ /^linux-headers\//) {
 			my $herevet = "$here\n" . cat_vet($rawline) . "\n";
 			ERROR("code indent should never use tabs\n" . $herevet);
 			$rpt_cleaners = 1;
Could you do the same for standard-headers/ too?

Quoted 4 lines

I think it would be better to not apply any qemu coding style checks to a headers update. Something like 'check if this contains header updates _only_' would make more sense, but that is beyond my nonexisting perl skills...

I have posted another vesion that does not check for any code style in hunks that modify linux-headers and include/standard-headers,
http://lists.nongnu.org/archive/html/qemu-devel/2016-08/msg01824.html

We still want to check header-only updates in other headers ... Your condition would draw attention to linux header updates that also touch other files, but I think that a diffstat is enough.

The script would need some preprocessing to know that only headers are modified or buffering of errors until the script knows that only headers were modified; neither is hard, but the added complexity is not compensated by usefulness, IMO.

Cornelia Huck

2016-08-10 14:01 UTC #15 lore raw

Re: [Qemu-devel] [PATCH] checkpatch: allow tabs in linux-headers

Quoted 34 lines · On Wed, 10 Aug 2016 15:55:28 +0200 Radim Krčmář <[email protected]> wrote:

2016-08-10 09:09+0200, Cornelia Huck:
Quoted 24 lines
Quoted 12 lines · On Tue, 9 Aug 2016 12:14:14 -0400 (EDT) Paolo Bonzini <[email protected]>…
Quoted 12 lines
Make scripts/checkpatch.pl accept tabs in linux-headers/, instead of changing scripts/update-linux-headers.sh to expand tabs when importing.
Signed-off-by Radim Krčmář <[email protected]>
---
scripts/checkpatch.pl +0 -0
index 929708721299..38232d4b25c3 100755
@@ -1355,7 +1355,7 @@ sub process {
 		next if ($realfile !~ /\.(h|c|cpp|pl)$/);
 
Quoted 9 lines
scripts/checkpatch.pl sub process { · line 1361
Quoted 7 lines
scripts/checkpatch.pl +1 -1
 # in QEMU, no tabs are allowed
-		if ($rawline =~ /^\+.*\t/) {
+		if ($rawline =~ /^\+.*\t/ && $realfile !~ /^linux-headers\//) {
 			my $herevet = "$here\n" . cat_vet($rawline) . "\n";
 			ERROR("code indent should never use tabs\n" . $herevet);
 			$rpt_cleaners = 1;
Could you do the same for standard-headers/ too?
Quoted 4 lines
I think it would be better to not apply any qemu coding style checks to a headers update. Something like 'check if this contains header updates _only_' would make more sense, but that is beyond my nonexisting perl skills...
I have posted another vesion that does not check for any code style in hunks that modify linux-headers and include/standard-headers,
http://lists.nongnu.org/archive/html/qemu-devel/2016-08/msg01824.html

Quoted 9 lines

We still want to check header-only updates in other headers ... Your condition would draw attention to linux header updates that also touch other files, but I think that a diffstat is enough.
The script would need some preprocessing to know that only headers are modified or buffering of errors until the script knows that only headers were modified; neither is hard, but the added complexity is not compensated by usefulness, IMO.

If there's no quick way to check, it's not worth spending too much time on it, I agree.

[email protected]

bot

2016-08-09 17:32 UTC #16 lore raw

Re: [Qemu-devel] [PATCH] checkpatch: allow tabs in linux-headers

In reply to Radim Krčmář

Your series seems to have some coding style problems. See output below for more information:

Hi,

Your series seems to have some coding style problems. See output below for more information:

Message-id: 20160809160703.GA10637@potion
Type: series
Subject: [Qemu-devel] [PATCH] checkpatch: allow tabs in linux-headers

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do

    echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))

done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
e923294 checkpatch: allow tabs in linux-headers

=== OUTPUT BEGIN ===
Checking PATCH 1/1: checkpatch: allow tabs in linux-headers...
ERROR: code indent should never use tabs
#106: FILE: scripts/checkpatch.pl:1358:
+^I^Iif ($rawline =~ /^\+.*\t/ && $realfile !~ /^linux-headers\//) {$

total: 1 errors, 0 warnings, 8 lines checked

Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1

---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to [email protected]