[Qemu-devel] [PATCH for-2.7 0/2] intel-iommu: restrict EIM to quirkless KVM

[Qemu-devel] [PATCH for-2.7 0/2] intel-iommu: restrict EIM to quirkless KVM

[Radim Krčmář]

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

-- 
2.9.2

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

[Radim Krčmář]

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 linux-headers/linux/kvm.h | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index e60e21ba227e..4806e069e749 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -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 */
-- 
2.9.2

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

[Radim Krčmář]

2016-08-09 17:03+0200, Radim Krčmář:
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  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 +++++++++++++++++

[Qemu-devel] [PATCH 2/2] intel-iommu: restrict EIM to quirkless KVM

[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ář <rkrcmar@redhat.com>
---
 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(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 28c31a2cdfa3..733751923233 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -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);
diff --git a/target-i386/kvm-stub.c b/target-i386/kvm-stub.c
index cdf15061091d..71be0bd94ddc 100644
--- a/target-i386/kvm-stub.c
+++ b/target-i386/kvm-stub.c
@@ -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.
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 0b2016a77a3c..050c850d77d3 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -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);
diff --git a/target-i386/kvm_i386.h b/target-i386/kvm_i386.h
index 42b00af1b1c3..1bc445d59c83 100644
--- a/target-i386/kvm_i386.h
+++ b/target-i386/kvm_i386.h
@@ -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
-- 
2.9.2

Re: [Qemu-devel] [PATCH 2/2] intel-iommu: restrict EIM to quirkless KVM

[Peter Xu]

On Tue, Aug 09, 2016 at 05:03:33PM +0200, Radim Krčmář wrote:
> 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ář <rkrcmar@redhat.com>
> ---
>  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(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 28c31a2cdfa3..733751923233 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -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;
> +        }

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

Re: [Qemu-devel] [PATCH 2/2] intel-iommu: restrict EIM to quirkless KVM

[Radim Krčmář]

2016-08-10 11:29+0800, Peter Xu:
> On Tue, Aug 09, 2016 at 05:03:33PM +0200, Radim Krčmář wrote:
>> 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ář <rkrcmar@redhat.com>
>> ---
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> @@ -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;
>> +        }
> 
> 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.

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

Re: [Qemu-devel] [PATCH 2/2] intel-iommu: restrict EIM to quirkless KVM

[Peter Xu]

On Wed, Aug 10, 2016 at 06:59:14PM +0200, Radim Krčmář wrote:
> 2016-08-10 11:29+0800, Peter Xu:
> > On Tue, Aug 09, 2016 at 05:03:33PM +0200, Radim Krčmář wrote:
> >> 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ář <rkrcmar@redhat.com>
> >> ---
> >> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> >> @@ -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;
> >> +        }
> > 
> > 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.

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

Reviewed-by: Peter Xu <peterx@redhat.com>

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

Thank you for the explaination!

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

-- peterx

Re: [Qemu-devel] [PATCH for-2.7 0/2] intel-iommu: restrict EIM to quirkless KVM

[no-reply]

Hi,

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

Message-id: 20160809150333.9991-1-rkrcmar@redhat.com
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 patchew-devel@freelists.org

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

[Radim Krčmář]

2016-08-09 08:31-0700, no-reply@ec2-52-6-146-230.compute-1.amazonaws.com:
> Hi,
> 
> Your series seems to have some coding style problems. See output below for
> more information:
> 
> Message-id: 20160809150333.9991-1-rkrcmar@redhat.com
> 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.

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ář <rkrcmar@redhat.com>
---
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 929708721299..38232d4b25c3 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -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;

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

[Paolo Bonzini]

----- Original Message -----
> From: "Radim Krčmář" <rkrcmar@redhat.com>
> To: no-reply@ec2-52-6-146-230.compute-1.amazonaws.com
> Cc: famz@redhat.com, ehabkost@redhat.com, mst@redhat.com, qemu-devel@nongnu.org, peterx@redhat.com, "jan kiszka"
> <jan.kiszka@web.de>, pbonzini@redhat.com, rth@twiddle.net
> Sent: Tuesday, August 9, 2016 6:07:04 PM
> Subject: [PATCH] checkpatch: allow tabs in linux-headers
> 
> 2016-08-09 08:31-0700, no-reply@ec2-52-6-146-230.compute-1.amazonaws.com:
> > Hi,
> > 
> > Your series seems to have some coding style problems. See output below for
> > more information:
> > 
> > Message-id: 20160809150333.9991-1-rkrcmar@redhat.com
> > 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.
> 
> 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ář <rkrcmar@redhat.com>
> ---
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 929708721299..38232d4b25c3 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -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

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

[Radim Krčmář]

>> 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ář <rkrcmar@redhat.com>
>> ---
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index 929708721299..38232d4b25c3 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -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?

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.

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

[Paolo Bonzini]

On 09/08/2016 18:37, Radim Krčmář wrote:
>>> 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ář <rkrcmar@redhat.com>
>>> ---
>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>>> index 929708721299..38232d4b25c3 100755
>>> --- a/scripts/checkpatch.pl
>>> +++ b/scripts/checkpatch.pl
>>> @@ -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?
> 
> 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

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

[Radim Krčmář]

2016-08-09 18:39+0200, Paolo Bonzini:
> On 09/08/2016 18:37, Radim Krčmář wrote:
>>>> 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ář <rkrcmar@redhat.com>
>>>> ---
>>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>>>> index 929708721299..38232d4b25c3 100755
>>>> --- a/scripts/checkpatch.pl
>>>> +++ b/scripts/checkpatch.pl
>>>> @@ -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?
>> 
>> 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.

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

[Cornelia Huck]

On Tue, 9 Aug 2016 12:14:14 -0400 (EDT)
Paolo Bonzini <pbonzini@redhat.com> wrote:

> 
> 
> ----- Original Message -----
> > From: "Radim Krčmář" <rkrcmar@redhat.com>
> > To: no-reply@ec2-52-6-146-230.compute-1.amazonaws.com
> > Cc: famz@redhat.com, ehabkost@redhat.com, mst@redhat.com, qemu-devel@nongnu.org, peterx@redhat.com, "jan kiszka"
> > <jan.kiszka@web.de>, pbonzini@redhat.com, rth@twiddle.net
> > Sent: Tuesday, August 9, 2016 6:07:04 PM
> > Subject: [PATCH] checkpatch: allow tabs in linux-headers
> > 
> > 2016-08-09 08:31-0700, no-reply@ec2-52-6-146-230.compute-1.amazonaws.com:
> > > Hi,
> > > 
> > > Your series seems to have some coding style problems. See output below for
> > > more information:
> > > 
> > > Message-id: 20160809150333.9991-1-rkrcmar@redhat.com
> > > 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.
> > 
> > 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ář <rkrcmar@redhat.com>
> > ---
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 929708721299..38232d4b25c3 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -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?

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

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

[Radim Krčmář]

2016-08-10 09:09+0200, Cornelia Huck:
> On Tue, 9 Aug 2016 12:14:14 -0400 (EDT)
> Paolo Bonzini <pbonzini@redhat.com> wrote:
>> > 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ář <rkrcmar@redhat.com>
>> > ---
>> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> > index 929708721299..38232d4b25c3 100755
>> > --- a/scripts/checkpatch.pl
>> > +++ b/scripts/checkpatch.pl
>> > @@ -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?
> 
> 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.

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

[Cornelia Huck]

On Wed, 10 Aug 2016 15:55:28 +0200
Radim Krčmář <rkrcmar@redhat.com> wrote:

> 2016-08-10 09:09+0200, Cornelia Huck:
> > On Tue, 9 Aug 2016 12:14:14 -0400 (EDT)
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> >> > 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ář <rkrcmar@redhat.com>
> >> > ---
> >> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> >> > index 929708721299..38232d4b25c3 100755
> >> > --- a/scripts/checkpatch.pl
> >> > +++ b/scripts/checkpatch.pl
> >> > @@ -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?
> > 
> > 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.
> 

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

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

[no-reply]

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 patchew-devel@freelists.org