[PATCH v2] arm64: allow building with kcov coverage on ARM64

[PATCH v2] arm64: allow building with kcov coverage on ARM64

[Alexander Potapenko]

Add ARCH_HAS_KCOV to ARM64 config. To avoid crashes, disable
instrumentation of the following files:

arch/arm64/boot/*
arch/arm64/kvm/hyp/*

Signed-off-by: Alexander Potapenko <glider@google.com>
---
v2: - disable instrumentation of arch/arm64/{boot,kvm/hyp}
    - enable instrumentation of arch/arm64/lib/delay.c
---
 arch/arm64/Kconfig          | 1 +
 arch/arm64/boot/Makefile    | 4 ++++
 arch/arm64/kvm/hyp/Makefile | 4 ++++
 3 files changed, 9 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 5a0a691..eb0b0a0 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -7,6 +7,7 @@ config ARM64
 	select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
 	select ARCH_HAS_ELF_RANDOMIZE
 	select ARCH_HAS_GCOV_PROFILE_ALL
+	select ARCH_HAS_KCOV
 	select ARCH_HAS_SG_CHAIN
 	select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
 	select ARCH_USE_CMPXCHG_LOCKREF
diff --git a/arch/arm64/boot/Makefile b/arch/arm64/boot/Makefile
index 305c552..74cec89 100644
--- a/arch/arm64/boot/Makefile
+++ b/arch/arm64/boot/Makefile
@@ -14,6 +14,10 @@
 # Based on the ia64 boot/Makefile.
 #
 
+# Avoid potential boot-time problems with kcov instrumentation. We are mostly
+# interested in syscall coverage, so boot code is not interesting anyway.
+KCOV_INSTRUMENT		:= n
+
 targets := Image Image.gz
 
 $(obj)/Image: vmlinux FORCE
diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
index 778d0ef..0c85feb 100644
--- a/arch/arm64/kvm/hyp/Makefile
+++ b/arch/arm64/kvm/hyp/Makefile
@@ -17,6 +17,10 @@ obj-$(CONFIG_KVM_ARM_HOST) += tlb.o
 obj-$(CONFIG_KVM_ARM_HOST) += hyp-entry.o
 obj-$(CONFIG_KVM_ARM_HOST) += s2-setup.o
 
+# KVM code is run at a different exception code with a different map, so
+# compiler instrumentation that inserts callbacks or checks into the code may
+# cause crashes. Just disable it.
 GCOV_PROFILE	:= n
 KASAN_SANITIZE	:= n
 UBSAN_SANITIZE	:= n
+KCOV_INSTRUMENT	:= n
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH v2] arm64: allow building with kcov coverage on ARM64

[Alexander Potapenko]

Hi all,

On Tue, Jun 14, 2016 at 6:57 PM, Alexander Potapenko <glider@google.com> wrote:
> Add ARCH_HAS_KCOV to ARM64 config. To avoid crashes, disable
> instrumentation of the following files:
>
> arch/arm64/boot/*
> arch/arm64/kvm/hyp/*
>
> Signed-off-by: Alexander Potapenko <glider@google.com>
> ---
> v2: - disable instrumentation of arch/arm64/{boot,kvm/hyp}
>     - enable instrumentation of arch/arm64/lib/delay.c
> ---
>  arch/arm64/Kconfig          | 1 +
>  arch/arm64/boot/Makefile    | 4 ++++
>  arch/arm64/kvm/hyp/Makefile | 4 ++++
>  3 files changed, 9 insertions(+)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 5a0a691..eb0b0a0 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -7,6 +7,7 @@ config ARM64
>         select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
>         select ARCH_HAS_ELF_RANDOMIZE
>         select ARCH_HAS_GCOV_PROFILE_ALL
> +       select ARCH_HAS_KCOV
>         select ARCH_HAS_SG_CHAIN
>         select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
>         select ARCH_USE_CMPXCHG_LOCKREF
> diff --git a/arch/arm64/boot/Makefile b/arch/arm64/boot/Makefile
> index 305c552..74cec89 100644
> --- a/arch/arm64/boot/Makefile
> +++ b/arch/arm64/boot/Makefile
> @@ -14,6 +14,10 @@
>  # Based on the ia64 boot/Makefile.
>  #
>
> +# Avoid potential boot-time problems with kcov instrumentation. We are mostly
> +# interested in syscall coverage, so boot code is not interesting anyway.
> +KCOV_INSTRUMENT                := n
> +
>  targets := Image Image.gz
>
>  $(obj)/Image: vmlinux FORCE
> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
> index 778d0ef..0c85feb 100644
> --- a/arch/arm64/kvm/hyp/Makefile
> +++ b/arch/arm64/kvm/hyp/Makefile
> @@ -17,6 +17,10 @@ obj-$(CONFIG_KVM_ARM_HOST) += tlb.o
>  obj-$(CONFIG_KVM_ARM_HOST) += hyp-entry.o
>  obj-$(CONFIG_KVM_ARM_HOST) += s2-setup.o
>
> +# KVM code is run at a different exception code with a different map, so
> +# compiler instrumentation that inserts callbacks or checks into the code may
> +# cause crashes. Just disable it.
>  GCOV_PROFILE   := n
>  KASAN_SANITIZE := n
>  UBSAN_SANITIZE := n
> +KCOV_INSTRUMENT        := n
> --
> 2.8.0.rc3.226.g39d4020
>
Following the previous patch discussion, I have dropped the delay.c
exception, which isn't strictly required for kcov to function.
We can decide what to do with it later.
Instead I've added rules to avoid instrumentation of arch/arm64/boot
and arch/arm64/kvm


-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Re: [PATCH v2] arm64: allow building with kcov coverage on ARM64

[Mark Rutland]

On Tue, Jun 14, 2016 at 06:57:21PM +0200, Alexander Potapenko wrote:
> Add ARCH_HAS_KCOV to ARM64 config. To avoid crashes, disable
> instrumentation of the following files:
> 
> arch/arm64/boot/*
> arch/arm64/kvm/hyp/*
> 
> Signed-off-by: Alexander Potapenko <glider@google.com>
> ---
> v2: - disable instrumentation of arch/arm64/{boot,kvm/hyp}
>     - enable instrumentation of arch/arm64/lib/delay.c
> ---
>  arch/arm64/Kconfig          | 1 +
>  arch/arm64/boot/Makefile    | 4 ++++
>  arch/arm64/kvm/hyp/Makefile | 4 ++++
>  3 files changed, 9 insertions(+)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 5a0a691..eb0b0a0 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -7,6 +7,7 @@ config ARM64
>  	select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
>  	select ARCH_HAS_ELF_RANDOMIZE
>  	select ARCH_HAS_GCOV_PROFILE_ALL
> +	select ARCH_HAS_KCOV
>  	select ARCH_HAS_SG_CHAIN
>  	select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
>  	select ARCH_USE_CMPXCHG_LOCKREF
> diff --git a/arch/arm64/boot/Makefile b/arch/arm64/boot/Makefile
> index 305c552..74cec89 100644
> --- a/arch/arm64/boot/Makefile
> +++ b/arch/arm64/boot/Makefile
> @@ -14,6 +14,10 @@
>  # Based on the ia64 boot/Makefile.
>  #
>  
> +# Avoid potential boot-time problems with kcov instrumentation. We are mostly
> +# interested in syscall coverage, so boot code is not interesting anyway.
> +KCOV_INSTRUMENT		:= n

We have no code under our boot directory, so I don't think the changes
to arch/arm64/boot are necessary.

> +
>  targets := Image Image.gz
>  
>  $(obj)/Image: vmlinux FORCE
> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
> index 778d0ef..0c85feb 100644
> --- a/arch/arm64/kvm/hyp/Makefile
> +++ b/arch/arm64/kvm/hyp/Makefile
> @@ -17,6 +17,10 @@ obj-$(CONFIG_KVM_ARM_HOST) += tlb.o
>  obj-$(CONFIG_KVM_ARM_HOST) += hyp-entry.o
>  obj-$(CONFIG_KVM_ARM_HOST) += s2-setup.o
>  
> +# KVM code is run at a different exception code with a different map, so
> +# compiler instrumentation that inserts callbacks or checks into the code may
> +# cause crashes. Just disable it.
>  GCOV_PROFILE	:= n
>  KASAN_SANITIZE	:= n
>  UBSAN_SANITIZE	:= n
> +KCOV_INSTRUMENT	:= n

This looks sane to me.

With VHE this code _may_ run in the same memory map as the kernel, but
it's not something we can determine at compile time.

Otherwise, I believe that the rest of the C code under arch/arm64 runs
in the usual kernel memory map (including the special case of kaslr.c),
and the EFI stub code has already been covered, so I'm not immediately
aware of anything else that needs to be special-cased.

I built and booted (via EFI) a kernel with this feature enabled (also
with the boot/Makefile change removed). I haven't tested the feature
itself as such, as I'm not sure how to do that.

FWIW, with the boot/Makefile change removed, feel free to add:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

Re: [PATCH v2] arm64: allow building with kcov coverage on ARM64

[Alexander Potapenko]

On Tue, Jun 14, 2016 at 7:55 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Jun 14, 2016 at 06:57:21PM +0200, Alexander Potapenko wrote:
>> Add ARCH_HAS_KCOV to ARM64 config. To avoid crashes, disable
>> instrumentation of the following files:
>>
>> arch/arm64/boot/*
>> arch/arm64/kvm/hyp/*
>>
>> Signed-off-by: Alexander Potapenko <glider@google.com>
>> ---
>> v2: - disable instrumentation of arch/arm64/{boot,kvm/hyp}
>>     - enable instrumentation of arch/arm64/lib/delay.c
>> ---
>>  arch/arm64/Kconfig          | 1 +
>>  arch/arm64/boot/Makefile    | 4 ++++
>>  arch/arm64/kvm/hyp/Makefile | 4 ++++
>>  3 files changed, 9 insertions(+)
>>
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 5a0a691..eb0b0a0 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -7,6 +7,7 @@ config ARM64
>>       select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
>>       select ARCH_HAS_ELF_RANDOMIZE
>>       select ARCH_HAS_GCOV_PROFILE_ALL
>> +     select ARCH_HAS_KCOV
>>       select ARCH_HAS_SG_CHAIN
>>       select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
>>       select ARCH_USE_CMPXCHG_LOCKREF
>> diff --git a/arch/arm64/boot/Makefile b/arch/arm64/boot/Makefile
>> index 305c552..74cec89 100644
>> --- a/arch/arm64/boot/Makefile
>> +++ b/arch/arm64/boot/Makefile
>> @@ -14,6 +14,10 @@
>>  # Based on the ia64 boot/Makefile.
>>  #
>>
>> +# Avoid potential boot-time problems with kcov instrumentation. We are mostly
>> +# interested in syscall coverage, so boot code is not interesting anyway.
>> +KCOV_INSTRUMENT              := n
>
> We have no code under our boot directory, so I don't think the changes
> to arch/arm64/boot are necessary.
Indeed we don't! Removed that.
>> +
>>  targets := Image Image.gz
>>
>>  $(obj)/Image: vmlinux FORCE
>> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
>> index 778d0ef..0c85feb 100644
>> --- a/arch/arm64/kvm/hyp/Makefile
>> +++ b/arch/arm64/kvm/hyp/Makefile
>> @@ -17,6 +17,10 @@ obj-$(CONFIG_KVM_ARM_HOST) += tlb.o
>>  obj-$(CONFIG_KVM_ARM_HOST) += hyp-entry.o
>>  obj-$(CONFIG_KVM_ARM_HOST) += s2-setup.o
>>
>> +# KVM code is run at a different exception code with a different map, so
>> +# compiler instrumentation that inserts callbacks or checks into the code may
>> +# cause crashes. Just disable it.
>>  GCOV_PROFILE := n
>>  KASAN_SANITIZE       := n
>>  UBSAN_SANITIZE       := n
>> +KCOV_INSTRUMENT      := n
>
> This looks sane to me.
>
> With VHE this code _may_ run in the same memory map as the kernel, but
> it's not something we can determine at compile time.
>
> Otherwise, I believe that the rest of the C code under arch/arm64 runs
> in the usual kernel memory map (including the special case of kaslr.c),
> and the EFI stub code has already been covered, so I'm not immediately
> aware of anything else that needs to be special-cased.
>
> I built and booted (via EFI) a kernel with this feature enabled (also
> with the boot/Makefile change removed). I haven't tested the feature
> itself as such, as I'm not sure how to do that.
You can test it by running the test program from Documentation/kcov.txt.

> FWIW, with the boot/Makefile change removed, feel free to add:
>
> Acked-by: Mark Rutland <mark.rutland@arm.com>
Thank you!
I'll wait till tomorrow for others to comment, and then will send the
updated version.
> Thanks,
> Mark.



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Re: [PATCH v2] arm64: allow building with kcov coverage on ARM64

[Mark Rutland]

On Tue, Jun 14, 2016 at 08:16:08PM +0200, Alexander Potapenko wrote:
> On Tue, Jun 14, 2016 at 7:55 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > I built and booted (via EFI) a kernel with this feature enabled (also
> > with the boot/Makefile change removed). I haven't tested the feature
> > itself as such, as I'm not sure how to do that.
> You can test it by running the test program from Documentation/kcov.txt.

Ah, I hadn't spotted that. If I get the chance I'll try to give that a
go.

> > FWIW, with the boot/Makefile change removed, feel free to add:
> >
> > Acked-by: Mark Rutland <mark.rutland@arm.com>
> Thank you!
> I'll wait till tomorrow for others to comment, and then will send the
> updated version.

Sounds good to me.

Mark.

Re: [PATCH v2] arm64: allow building with kcov coverage on ARM64

[Mark Rutland]

On Wed, Jun 15, 2016 at 10:25:10AM +0100, Mark Rutland wrote:
> On Tue, Jun 14, 2016 at 08:16:08PM +0200, Alexander Potapenko wrote:
> > On Tue, Jun 14, 2016 at 7:55 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > > I built and booted (via EFI) a kernel with this feature enabled (also
> > > with the boot/Makefile change removed). I haven't tested the feature
> > > itself as such, as I'm not sure how to do that.
> > You can test it by running the test program from Documentation/kcov.txt.
> 
> Ah, I hadn't spotted that. If I get the chance I'll try to give that a
> go.

I just had a go (with this applied atop of v4.7-rc3), and I get:

root@ribbensteg:/home/nanook# ./kcov 
mmap: No such device

The device exists (it was able to open the fd, evidently):

root@ribbensteg:/home/nanook# ls -al /sys/kernel/debug/kcov 
-rw------- 1 root root 0 Jan  1  1970 /sys/kernel/debug/kcov

Strace show me:

openat(AT_FDCWD, "/sys/kernel/debug/kcov", O_RDWR) = 3
ioctl(3, CHIOMOVE or CM_IOCGATR, 0x10000) = 0
mmap(NULL, 524288, PROT_READ|PROT_WRITE, MAP_SHARED, 3, 0) = -1 ENODEV (No such device)

It doesn't look like the error paths in kcov_mmap are hitting.

Any ideas?

Thanks,
Mark.

Re: [PATCH v2] arm64: allow building with kcov coverage on ARM64

[Alexander Potapenko]

On Wed, Jun 15, 2016 at 1:44 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Jun 15, 2016 at 10:25:10AM +0100, Mark Rutland wrote:
>> On Tue, Jun 14, 2016 at 08:16:08PM +0200, Alexander Potapenko wrote:
>> > On Tue, Jun 14, 2016 at 7:55 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > > I built and booted (via EFI) a kernel with this feature enabled (also
>> > > with the boot/Makefile change removed). I haven't tested the feature
>> > > itself as such, as I'm not sure how to do that.
>> > You can test it by running the test program from Documentation/kcov.txt.
>>
>> Ah, I hadn't spotted that. If I get the chance I'll try to give that a
>> go.
>
> I just had a go (with this applied atop of v4.7-rc3), and I get:
>
> root@ribbensteg:/home/nanook# ./kcov
> mmap: No such device
>
> The device exists (it was able to open the fd, evidently):
>
> root@ribbensteg:/home/nanook# ls -al /sys/kernel/debug/kcov
> -rw------- 1 root root 0 Jan  1  1970 /sys/kernel/debug/kcov
>
> Strace show me:
>
> openat(AT_FDCWD, "/sys/kernel/debug/kcov", O_RDWR) = 3
> ioctl(3, CHIOMOVE or CM_IOCGATR, 0x10000) = 0
> mmap(NULL, 524288, PROT_READ|PROT_WRITE, MAP_SHARED, 3, 0) = -1 ENODEV (No such device)
>
> It doesn't look like the error paths in kcov_mmap are hitting.
>
> Any ideas?
According to Dmitry (thanks, Dmitry!) this has regressed recently, but
there's a pending patch that should probably fix the problem:
http://lkml.iu.edu/hypermail/linux/kernel/1605.2/04379.html
> Thanks,
> Mark.


-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Re: [PATCH v2] arm64: allow building with kcov coverage on ARM64

[Mark Rutland]

On Wed, Jun 15, 2016 at 01:53:03PM +0200, Alexander Potapenko wrote:
> On Wed, Jun 15, 2016 at 1:44 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Wed, Jun 15, 2016 at 10:25:10AM +0100, Mark Rutland wrote:
> >> On Tue, Jun 14, 2016 at 08:16:08PM +0200, Alexander Potapenko wrote:
> >> > On Tue, Jun 14, 2016 at 7:55 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> >> > > I built and booted (via EFI) a kernel with this feature enabled (also
> >> > > with the boot/Makefile change removed). I haven't tested the feature
> >> > > itself as such, as I'm not sure how to do that.
> >> > You can test it by running the test program from Documentation/kcov.txt.
> >>
> >> Ah, I hadn't spotted that. If I get the chance I'll try to give that a
> >> go.
> >
> > I just had a go (with this applied atop of v4.7-rc3), and I get:
> >
> > root@ribbensteg:/home/nanook# ./kcov
> > mmap: No such device
> >
> > The device exists (it was able to open the fd, evidently):
> >
> > root@ribbensteg:/home/nanook# ls -al /sys/kernel/debug/kcov
> > -rw------- 1 root root 0 Jan  1  1970 /sys/kernel/debug/kcov
> >
> > Strace show me:
> >
> > openat(AT_FDCWD, "/sys/kernel/debug/kcov", O_RDWR) = 3
> > ioctl(3, CHIOMOVE or CM_IOCGATR, 0x10000) = 0
> > mmap(NULL, 524288, PROT_READ|PROT_WRITE, MAP_SHARED, 3, 0) = -1 ENODEV (No such device)
> >
> > It doesn't look like the error paths in kcov_mmap are hitting.
> >
> > Any ideas?
> According to Dmitry (thanks, Dmitry!) this has regressed recently, but
> there's a pending patch that should probably fix the problem:
> http://lkml.iu.edu/hypermail/linux/kernel/1605.2/04379.html

Thanks for the pointer! With that applied, the program runs.

However, it looks like I missed a warning from the kernel build system, 
and my toolchain doesn't actually support -fsanitize-coverage=trace-pc,
so I'm not going to be able to test that further.

It would be great if we could deliberately not register the debugfs file
when there was no compiler support for the feature, for those like me
who miss the build time warning. We do something like that for the LSE
atomics on arm64.

Thanks,
Mark.

Re: [PATCH v2] arm64: allow building with kcov coverage on ARM64

[Dmitry Vyukov]

On Wed, Jun 15, 2016 at 4:25 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> >> > > I built and booted (via EFI) a kernel with this feature enabled (also
>> >> > > with the boot/Makefile change removed). I haven't tested the feature
>> >> > > itself as such, as I'm not sure how to do that.
>> >> > You can test it by running the test program from Documentation/kcov.txt.
>> >>
>> >> Ah, I hadn't spotted that. If I get the chance I'll try to give that a
>> >> go.
>> >
>> > I just had a go (with this applied atop of v4.7-rc3), and I get:
>> >
>> > root@ribbensteg:/home/nanook# ./kcov
>> > mmap: No such device
>> >
>> > The device exists (it was able to open the fd, evidently):
>> >
>> > root@ribbensteg:/home/nanook# ls -al /sys/kernel/debug/kcov
>> > -rw------- 1 root root 0 Jan  1  1970 /sys/kernel/debug/kcov
>> >
>> > Strace show me:
>> >
>> > openat(AT_FDCWD, "/sys/kernel/debug/kcov", O_RDWR) = 3
>> > ioctl(3, CHIOMOVE or CM_IOCGATR, 0x10000) = 0
>> > mmap(NULL, 524288, PROT_READ|PROT_WRITE, MAP_SHARED, 3, 0) = -1 ENODEV (No such device)
>> >
>> > It doesn't look like the error paths in kcov_mmap are hitting.
>> >
>> > Any ideas?
>> According to Dmitry (thanks, Dmitry!) this has regressed recently, but
>> there's a pending patch that should probably fix the problem:
>> http://lkml.iu.edu/hypermail/linux/kernel/1605.2/04379.html
>
> Thanks for the pointer! With that applied, the program runs.
>
> However, it looks like I missed a warning from the kernel build system,
> and my toolchain doesn't actually support -fsanitize-coverage=trace-pc,
> so I'm not going to be able to test that further.
>
> It would be great if we could deliberately not register the debugfs file
> when there was no compiler support for the feature, for those like me
> who miss the build time warning. We do something like that for the LSE
> atomics on arm64.

Hi Mark,

It's a common problem and it would be great to detect this.
But I think it's better to return ENOTSUP from open rather than not
registering the file at all. This way higher level tools will be able
to more easily diagnose the issue and properly report to user. A
missing file looks like not mounted debugfs (which another common
issue).

I am not sure how to do it.
Compiler does not provide any define for this option. And I am not
familiar enough with kernel makefiles. Would it be possible to add a
define to CLAGS in the makefile along with printing the warning?

Re: [PATCH v2] arm64: allow building with kcov coverage on ARM64

[Mark Rutland]

On Wed, Jun 15, 2016 at 04:36:18PM +0200, Dmitry Vyukov wrote:
> > However, it looks like I missed a warning from the kernel build system,
> > and my toolchain doesn't actually support -fsanitize-coverage=trace-pc,
> > so I'm not going to be able to test that further.
> >
> > It would be great if we could deliberately not register the debugfs file
> > when there was no compiler support for the feature, for those like me
> > who miss the build time warning. We do something like that for the LSE
> > atomics on arm64.
> 
> Hi Mark,
> 
> It's a common problem and it would be great to detect this.
> But I think it's better to return ENOTSUP from open rather than not
> registering the file at all. This way higher level tools will be able
> to more easily diagnose the issue and properly report to user. A
> missing file looks like not mounted debugfs (which another common
> issue).

I have no strong feeling either way, so long as we don't silently carry
on in a case that makes no sense.

> I am not sure how to do it.
> Compiler does not provide any define for this option. And I am not
> familiar enough with kernel makefiles. Would it be possible to add a
> define to CLAGS in the makefile along with printing the warning?

If you follow the arm64 LSE example, you can do something like the
below.

Note that this relies on CFLAGS_KCOV being cleared when not supported by
the toolchain. I used ENOTSUPP rather than ENOTSUP as there's no
standard definition for the later in the Linux headers.

Mark.

---->8----
diff --git a/Makefile b/Makefile
index 0f70de6..e6ef260 100644
--- a/Makefile
+++ b/Makefile
@@ -369,7 +369,7 @@ LDFLAGS_MODULE  =
 CFLAGS_KERNEL  =
 AFLAGS_KERNEL  =
 CFLAGS_GCOV    = -fprofile-arcs -ftest-coverage -fno-tree-loop-im -Wno-maybe-uninitialized
-CFLAGS_KCOV    = -fsanitize-coverage=trace-pc
+CFLAGS_KCOV    = -fsanitize-coverage=trace-pc -DCONFIG_KCOV_CC
 
 
 # Use USERINCLUDE when you must reference the UAPI directories only.
diff --git a/kernel/kcov.c b/kernel/kcov.c
index a02f2dd..df2cafd 100644
--- a/kernel/kcov.c
+++ b/kernel/kcov.c
@@ -3,6 +3,7 @@
 #define DISABLE_BRANCH_PROFILING
 #include <linux/compiler.h>
 #include <linux/types.h>
+#include <linux/errno.h>
 #include <linux/file.h>
 #include <linux/fs.h>
 #include <linux/mm.h>
@@ -160,6 +161,13 @@ static int kcov_open(struct inode *inode, struct file *filep)
 {
        struct kcov *kcov;
 
+       /*
+        * CONFIG_KCOV was selected, but the compiler does not support the
+        * options KCOV requires.
+        */
+       if (!IS_ENABLED(CONFIG_KCOV_CC))
+               return -ENOTSUPP;
+
        kcov = kzalloc(sizeof(*kcov), GFP_KERNEL);
        if (!kcov)
                return -ENOMEM;

Re: [PATCH v2] arm64: allow building with kcov coverage on ARM64

[Dmitry Vyukov]

On Wed, Jun 15, 2016 at 5:05 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Jun 15, 2016 at 04:36:18PM +0200, Dmitry Vyukov wrote:
>> > However, it looks like I missed a warning from the kernel build system,
>> > and my toolchain doesn't actually support -fsanitize-coverage=trace-pc,
>> > so I'm not going to be able to test that further.
>> >
>> > It would be great if we could deliberately not register the debugfs file
>> > when there was no compiler support for the feature, for those like me
>> > who miss the build time warning. We do something like that for the LSE
>> > atomics on arm64.
>>
>> Hi Mark,
>>
>> It's a common problem and it would be great to detect this.
>> But I think it's better to return ENOTSUP from open rather than not
>> registering the file at all. This way higher level tools will be able
>> to more easily diagnose the issue and properly report to user. A
>> missing file looks like not mounted debugfs (which another common
>> issue).
>
> I have no strong feeling either way, so long as we don't silently carry
> on in a case that makes no sense.
>
>> I am not sure how to do it.
>> Compiler does not provide any define for this option. And I am not
>> familiar enough with kernel makefiles. Would it be possible to add a
>> define to CLAGS in the makefile along with printing the warning?
>
> If you follow the arm64 LSE example, you can do something like the
> below.
>
> Note that this relies on CFLAGS_KCOV being cleared when not supported by
> the toolchain. I used ENOTSUPP rather than ENOTSUP as there's no
> standard definition for the later in the Linux headers.
>
> Mark.
>
> ---->8----
> diff --git a/Makefile b/Makefile
> index 0f70de6..e6ef260 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -369,7 +369,7 @@ LDFLAGS_MODULE  =
>  CFLAGS_KERNEL  =
>  AFLAGS_KERNEL  =
>  CFLAGS_GCOV    = -fprofile-arcs -ftest-coverage -fno-tree-loop-im -Wno-maybe-uninitialized
> -CFLAGS_KCOV    = -fsanitize-coverage=trace-pc
> +CFLAGS_KCOV    = -fsanitize-coverage=trace-pc -DCONFIG_KCOV_CC
>
>
>  # Use USERINCLUDE when you must reference the UAPI directories only.
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index a02f2dd..df2cafd 100644
> --- a/kernel/kcov.c
> +++ b/kernel/kcov.c
> @@ -3,6 +3,7 @@
>  #define DISABLE_BRANCH_PROFILING
>  #include <linux/compiler.h>
>  #include <linux/types.h>
> +#include <linux/errno.h>
>  #include <linux/file.h>
>  #include <linux/fs.h>
>  #include <linux/mm.h>
> @@ -160,6 +161,13 @@ static int kcov_open(struct inode *inode, struct file *filep)
>  {
>         struct kcov *kcov;
>
> +       /*
> +        * CONFIG_KCOV was selected, but the compiler does not support the
> +        * options KCOV requires.
> +        */
> +       if (!IS_ENABLED(CONFIG_KCOV_CC))
> +               return -ENOTSUPP;
> +
>         kcov = kzalloc(sizeof(*kcov), GFP_KERNEL);
>         if (!kcov)
>                 return -ENOMEM;


Looks great. Please mail it.

Re: [PATCH v2] arm64: allow building with kcov coverage on ARM64

[James Morse]

On 15/06/16 15:25, Mark Rutland wrote:
> On Wed, Jun 15, 2016 at 01:53:03PM +0200, Alexander Potapenko wrote:
>> On Wed, Jun 15, 2016 at 1:44 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>>> On Wed, Jun 15, 2016 at 10:25:10AM +0100, Mark Rutland wrote:
>>>> On Tue, Jun 14, 2016 at 08:16:08PM +0200, Alexander Potapenko wrote:
>>>>> On Tue, Jun 14, 2016 at 7:55 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>>>>>> I built and booted (via EFI) a kernel with this feature enabled (also
>>>>>> with the boot/Makefile change removed). I haven't tested the feature
>>>>>> itself as such, as I'm not sure how to do that.
>>>>> You can test it by running the test program from Documentation/kcov.txt.
>>>>
>>>> Ah, I hadn't spotted that. If I get the chance I'll try to give that a
>>>> go.
>>>
>>> I just had a go (with this applied atop of v4.7-rc3), and I get:
>>>
>>> root@ribbensteg:/home/nanook# ./kcov
>>> mmap: No such device
>>>
>>> The device exists (it was able to open the fd, evidently):
>>>
>>> root@ribbensteg:/home/nanook# ls -al /sys/kernel/debug/kcov
>>> -rw------- 1 root root 0 Jan  1  1970 /sys/kernel/debug/kcov
>>>
>>> Strace show me:
>>>
>>> openat(AT_FDCWD, "/sys/kernel/debug/kcov", O_RDWR) = 3
>>> ioctl(3, CHIOMOVE or CM_IOCGATR, 0x10000) = 0
>>> mmap(NULL, 524288, PROT_READ|PROT_WRITE, MAP_SHARED, 3, 0) = -1 ENODEV (No such device)
>>>
>>> It doesn't look like the error paths in kcov_mmap are hitting.
>>>
>>> Any ideas?
>> According to Dmitry (thanks, Dmitry!) this has regressed recently, but
>> there's a pending patch that should probably fix the problem:
>> http://lkml.iu.edu/hypermail/linux/kernel/1605.2/04379.html
> 
> Thanks for the pointer! With that applied, the program runs.
> 
> However, it looks like I missed a warning from the kernel build system, 
> and my toolchain doesn't actually support -fsanitize-coverage=trace-pc,
> so I'm not going to be able to test that further.

I dusted off a compiler that supports this, and ran the sample program under
Documentation with the above unproxify patch.

Tested-by: James Morse <james.morse@arm.com>


Thanks,

James

Re: [PATCH v2] arm64: allow building with kcov coverage on ARM64

[Alexander Potapenko]

On Thu, Jun 16, 2016 at 12:47 PM, James Morse <james.morse@arm.com> wrote:
> On 15/06/16 15:25, Mark Rutland wrote:
>> On Wed, Jun 15, 2016 at 01:53:03PM +0200, Alexander Potapenko wrote:
>>> On Wed, Jun 15, 2016 at 1:44 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>>>> On Wed, Jun 15, 2016 at 10:25:10AM +0100, Mark Rutland wrote:
>>>>> On Tue, Jun 14, 2016 at 08:16:08PM +0200, Alexander Potapenko wrote:
>>>>>> On Tue, Jun 14, 2016 at 7:55 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>>>>>>> I built and booted (via EFI) a kernel with this feature enabled (also
>>>>>>> with the boot/Makefile change removed). I haven't tested the feature
>>>>>>> itself as such, as I'm not sure how to do that.
>>>>>> You can test it by running the test program from Documentation/kcov.txt.
>>>>>
>>>>> Ah, I hadn't spotted that. If I get the chance I'll try to give that a
>>>>> go.
>>>>
>>>> I just had a go (with this applied atop of v4.7-rc3), and I get:
>>>>
>>>> root@ribbensteg:/home/nanook# ./kcov
>>>> mmap: No such device
>>>>
>>>> The device exists (it was able to open the fd, evidently):
>>>>
>>>> root@ribbensteg:/home/nanook# ls -al /sys/kernel/debug/kcov
>>>> -rw------- 1 root root 0 Jan  1  1970 /sys/kernel/debug/kcov
>>>>
>>>> Strace show me:
>>>>
>>>> openat(AT_FDCWD, "/sys/kernel/debug/kcov", O_RDWR) = 3
>>>> ioctl(3, CHIOMOVE or CM_IOCGATR, 0x10000) = 0
>>>> mmap(NULL, 524288, PROT_READ|PROT_WRITE, MAP_SHARED, 3, 0) = -1 ENODEV (No such device)
>>>>
>>>> It doesn't look like the error paths in kcov_mmap are hitting.
>>>>
>>>> Any ideas?
>>> According to Dmitry (thanks, Dmitry!) this has regressed recently, but
>>> there's a pending patch that should probably fix the problem:
>>> http://lkml.iu.edu/hypermail/linux/kernel/1605.2/04379.html
>>
>> Thanks for the pointer! With that applied, the program runs.
>>
>> However, it looks like I missed a warning from the kernel build system,
>> and my toolchain doesn't actually support -fsanitize-coverage=trace-pc,
>> so I'm not going to be able to test that further.
>
> I dusted off a compiler that supports this, and ran the sample program under
> Documentation with the above unproxify patch.
>
> Tested-by: James Morse <james.morse@arm.com>
I think it's time to ask now :)
If I receive "Tested-by" or "Acked-by" responses, do I need to send
out a patch adding them, or should I rely on the maintainer taking the
patch to the tree?
The first option reduces the amount of work done by the maintainer,
while the second one reduces the traffic in the list.
Sorry, I couldn't find the answer in the manuals.

Thank you,
Alex
>
> Thanks,
>
> James



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Re: [PATCH v2] arm64: allow building with kcov coverage on ARM64

[Mark Rutland]

On Thu, Jun 16, 2016 at 05:20:03PM +0200, Alexander Potapenko wrote:
> On Thu, Jun 16, 2016 at 12:47 PM, James Morse <james.morse@arm.com> wrote:
> > On 15/06/16 15:25, Mark Rutland wrote:
> >> On Wed, Jun 15, 2016 at 01:53:03PM +0200, Alexander Potapenko wrote:
> >>> On Wed, Jun 15, 2016 at 1:44 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> >>>> On Wed, Jun 15, 2016 at 10:25:10AM +0100, Mark Rutland wrote:
> >>>>> On Tue, Jun 14, 2016 at 08:16:08PM +0200, Alexander Potapenko wrote:
> >>>>>> On Tue, Jun 14, 2016 at 7:55 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> >>>>>>> I built and booted (via EFI) a kernel with this feature enabled (also
> >>>>>>> with the boot/Makefile change removed). I haven't tested the feature
> >>>>>>> itself as such, as I'm not sure how to do that.
> >>>>>> You can test it by running the test program from Documentation/kcov.txt.
> >>>>>
> >>>>> Ah, I hadn't spotted that. If I get the chance I'll try to give that a
> >>>>> go.
> >>>>
> >>>> I just had a go (with this applied atop of v4.7-rc3), and I get:
> >>>>
> >>>> root@ribbensteg:/home/nanook# ./kcov
> >>>> mmap: No such device
> >>>>
> >>>> The device exists (it was able to open the fd, evidently):
> >>>>
> >>>> root@ribbensteg:/home/nanook# ls -al /sys/kernel/debug/kcov
> >>>> -rw------- 1 root root 0 Jan  1  1970 /sys/kernel/debug/kcov
> >>>>
> >>>> Strace show me:
> >>>>
> >>>> openat(AT_FDCWD, "/sys/kernel/debug/kcov", O_RDWR) = 3
> >>>> ioctl(3, CHIOMOVE or CM_IOCGATR, 0x10000) = 0
> >>>> mmap(NULL, 524288, PROT_READ|PROT_WRITE, MAP_SHARED, 3, 0) = -1 ENODEV (No such device)
> >>>>
> >>>> It doesn't look like the error paths in kcov_mmap are hitting.
> >>>>
> >>>> Any ideas?
> >>> According to Dmitry (thanks, Dmitry!) this has regressed recently, but
> >>> there's a pending patch that should probably fix the problem:
> >>> http://lkml.iu.edu/hypermail/linux/kernel/1605.2/04379.html
> >>
> >> Thanks for the pointer! With that applied, the program runs.
> >>
> >> However, it looks like I missed a warning from the kernel build system,
> >> and my toolchain doesn't actually support -fsanitize-coverage=trace-pc,
> >> so I'm not going to be able to test that further.
> >
> > I dusted off a compiler that supports this, and ran the sample program under
> > Documentation with the above unproxify patch.
> >
> > Tested-by: James Morse <james.morse@arm.com>
> I think it's time to ask now :)
> If I receive "Tested-by" or "Acked-by" responses, do I need to send
> out a patch adding them, or should I rely on the maintainer taking the
> patch to the tree?
> The first option reduces the amount of work done by the maintainer,
> while the second one reduces the traffic in the list.
> Sorry, I couldn't find the answer in the manuals.

It's up to the maintainer, so it varies. The best thing to do is to ask
the maintainer what they'd prefer.

>From my experience, Catalin is usually happy to add tags, so I suspect
he'd be happy to do so for this patch (assuming he's happy to pick it
up). I'll leave it for him to say either way.

Thanks,
Mark.

Re: [PATCH v2] arm64: allow building with kcov coverage on ARM64

[Catalin Marinas]

On Thu, Jun 16, 2016 at 04:44:12PM +0100, Mark Rutland wrote:
> On Thu, Jun 16, 2016 at 05:20:03PM +0200, Alexander Potapenko wrote:
> > I think it's time to ask now :)
> > If I receive "Tested-by" or "Acked-by" responses, do I need to send
> > out a patch adding them, or should I rely on the maintainer taking the
> > patch to the tree?
> > The first option reduces the amount of work done by the maintainer,
> > while the second one reduces the traffic in the list.
> > Sorry, I couldn't find the answer in the manuals.
> 
> It's up to the maintainer, so it varies. The best thing to do is to ask
> the maintainer what they'd prefer.
> 
> From my experience, Catalin is usually happy to add tags, so I suspect
> he'd be happy to do so for this patch (assuming he's happy to pick it
> up). I'll leave it for him to say either way.

I usually cherry-pick tags that I see in reply to the *latest* version
of the patch (I have a rudimentary script to do this).

I noticed that there was an ack on v1 form Marc Z that's missing in v2.
Maybe it no longer applies, I can't tell, but I usually expect
subsequent versions of a patch to include all the previously given acks
(of course, if they still apply, sometimes a patch rewrite means
dropping those tags).

-- 
Catalin

Re: [PATCH v2] arm64: allow building with kcov coverage on ARM64

[Mark Rutland]

On Thu, Jun 16, 2016 at 05:25:31PM +0100, Catalin Marinas wrote:
> On Thu, Jun 16, 2016 at 04:44:12PM +0100, Mark Rutland wrote:
> > On Thu, Jun 16, 2016 at 05:20:03PM +0200, Alexander Potapenko wrote:
> > > I think it's time to ask now :)
> > > If I receive "Tested-by" or "Acked-by" responses, do I need to send
> > > out a patch adding them, or should I rely on the maintainer taking the
> > > patch to the tree?
> > > The first option reduces the amount of work done by the maintainer,
> > > while the second one reduces the traffic in the list.
> > > Sorry, I couldn't find the answer in the manuals.
> > 
> > It's up to the maintainer, so it varies. The best thing to do is to ask
> > the maintainer what they'd prefer.
> > 
> > From my experience, Catalin is usually happy to add tags, so I suspect
> > he'd be happy to do so for this patch (assuming he's happy to pick it
> > up). I'll leave it for him to say either way.
> 
> I usually cherry-pick tags that I see in reply to the *latest* version
> of the patch (I have a rudimentary script to do this).
> 
> I noticed that there was an ack on v1 form Marc Z that's missing in v2.

I believe Marc's reply [1] was to v3 [2], it's just that the version was
missing form the subject, and discussions continued on v2 in the mean
time.

> Maybe it no longer applies, I can't tell, but I usually expect
> subsequent versions of a patch to include all the previously given acks
> (of course, if they still apply, sometimes a patch rewrite means
> dropping those tags).

I guess the simplest thing to do is for Alexander to send a v4 with the
tags accumulated, assuming James's Tested-by is applicable to v3 with
the boot/Makefile hunk removed. James?

My ack stands.

Thanks,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/436551.html
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/436512.html

Re: [PATCH v2] arm64: allow building with kcov coverage on ARM64

[Alexander Potapenko]

On Thu, Jun 16, 2016 at 6:32 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Thu, Jun 16, 2016 at 05:25:31PM +0100, Catalin Marinas wrote:
>> On Thu, Jun 16, 2016 at 04:44:12PM +0100, Mark Rutland wrote:
>> > On Thu, Jun 16, 2016 at 05:20:03PM +0200, Alexander Potapenko wrote:
>> > > I think it's time to ask now :)
>> > > If I receive "Tested-by" or "Acked-by" responses, do I need to send
>> > > out a patch adding them, or should I rely on the maintainer taking the
>> > > patch to the tree?
>> > > The first option reduces the amount of work done by the maintainer,
>> > > while the second one reduces the traffic in the list.
>> > > Sorry, I couldn't find the answer in the manuals.
>> >
>> > It's up to the maintainer, so it varies. The best thing to do is to ask
>> > the maintainer what they'd prefer.
>> >
>> > From my experience, Catalin is usually happy to add tags, so I suspect
>> > he'd be happy to do so for this patch (assuming he's happy to pick it
>> > up). I'll leave it for him to say either way.
>>
>> I usually cherry-pick tags that I see in reply to the *latest* version
>> of the patch (I have a rudimentary script to do this).
>>
>> I noticed that there was an ack on v1 form Marc Z that's missing in v2.
>
> I believe Marc's reply [1] was to v3 [2], it's just that the version was
> missing form the subject, and discussions continued on v2 in the mean
> time.
Yes, this is correct.
>> Maybe it no longer applies, I can't tell, but I usually expect
>> subsequent versions of a patch to include all the previously given acks
>> (of course, if they still apply, sometimes a patch rewrite means
>> dropping those tags).
>
> I guess the simplest thing to do is for Alexander to send a v4 with the
> tags accumulated, assuming James's Tested-by is applicable to v3 with
> the boot/Makefile hunk removed. James?
I think it's safe to assume James's Tested-by is still valid, as
boot/Makefile hunk did virtually nothing.
I'll send the new patch version now.
> My ack stands.
>
> Thanks,
> Mark.
>
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/436551.html
> [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/436512.html



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Re: [PATCH v2] arm64: allow building with kcov coverage on ARM64

[James Morse]

On 16/06/16 17:36, Alexander Potapenko wrote:
> On Thu, Jun 16, 2016 at 6:32 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> On Thu, Jun 16, 2016 at 05:25:31PM +0100, Catalin Marinas wrote:
>>> I noticed that there was an ack on v1 form Marc Z that's missing in v2.
>>
>> I believe Marc's reply [1] was to v3 [2], it's just that the version was
>> missing form the subject, and discussions continued on v2 in the mean
>> time.

Ah, that's what happened...


> Yes, this is correct.
>>> Maybe it no longer applies, I can't tell, but I usually expect
>>> subsequent versions of a patch to include all the previously given acks
>>> (of course, if they still apply, sometimes a patch rewrite means
>>> dropping those tags).
>>
>> I guess the simplest thing to do is for Alexander to send a v4 with the
>> tags accumulated, assuming James's Tested-by is applicable to v3 with
>> the boot/Makefile hunk removed. James?

> I think it's safe to assume James's Tested-by is still valid, as
> boot/Makefile hunk did virtually nothing.

I agree!

> I'll send the new patch version now.


Thanks,

James

Re: [PATCH v2] arm64: allow building with kcov coverage on ARM64

[Catalin Marinas]

On Thu, Jun 16, 2016 at 05:32:59PM +0100, Mark Rutland wrote:
> On Thu, Jun 16, 2016 at 05:25:31PM +0100, Catalin Marinas wrote:
> > On Thu, Jun 16, 2016 at 04:44:12PM +0100, Mark Rutland wrote:
> > > On Thu, Jun 16, 2016 at 05:20:03PM +0200, Alexander Potapenko wrote:
> > > > I think it's time to ask now :)
> > > > If I receive "Tested-by" or "Acked-by" responses, do I need to send
> > > > out a patch adding them, or should I rely on the maintainer taking the
> > > > patch to the tree?
> > > > The first option reduces the amount of work done by the maintainer,
> > > > while the second one reduces the traffic in the list.
> > > > Sorry, I couldn't find the answer in the manuals.
> > > 
> > > It's up to the maintainer, so it varies. The best thing to do is to ask
> > > the maintainer what they'd prefer.
> > > 
> > > From my experience, Catalin is usually happy to add tags, so I suspect
> > > he'd be happy to do so for this patch (assuming he's happy to pick it
> > > up). I'll leave it for him to say either way.
> > 
> > I usually cherry-pick tags that I see in reply to the *latest* version
> > of the patch (I have a rudimentary script to do this).
> > 
> > I noticed that there was an ack on v1 form Marc Z that's missing in v2.
> 
> I believe Marc's reply [1] was to v3 [2], it's just that the version was
> missing form the subject, and discussions continued on v2 in the mean
> time.

OK, thanks for clarification. I thought the one with most replies was
the latest ;).

> > Maybe it no longer applies, I can't tell, but I usually expect
> > subsequent versions of a patch to include all the previously given acks
> > (of course, if they still apply, sometimes a patch rewrite means
> > dropping those tags).
> 
> I guess the simplest thing to do is for Alexander to send a v4 with the
> tags accumulated, assuming James's Tested-by is applicable to v3 with
> the boot/Makefile hunk removed. James?

Even better.

-- 
Catalin