Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1556683 > unrolled thread

[PATCH v4 0/4] Application Data Integrity feature introduced by SPARC M7

Started byKhalid Aziz <khalid.aziz@oracle.com>
First post2017-01-11 17:20 +0100
Last post2017-01-18 01:20 +0100
Articles 6 on this page of 26 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v4 0/4] Application Data Integrity feature introduced by SPARC M7 Khalid Aziz <khalid.aziz@oracle.com> - 2017-01-11 17:20 +0100
    [PATCH v4 1/4] signals, sparc: Add signal codes for ADI violations Khalid Aziz <khalid.aziz@oracle.com> - 2017-01-11 17:20 +0100
    Re: [PATCH v4 0/4] Application Data Integrity feature introduced by  SPARC M7 Dave Hansen <dave.hansen@linux.intel.com> - 2017-01-11 17:40 +0100
      Re: [PATCH v4 0/4] Application Data Integrity feature introduced by  SPARC M7 Khalid Aziz <khalid.aziz@oracle.com> - 2017-01-11 18:00 +0100
        Re: [PATCH v4 0/4] Application Data Integrity feature introduced by  SPARC M7 Dave Hansen <dave.hansen@linux.intel.com> - 2017-01-11 19:20 +0100
          Re: [PATCH v4 0/4] Application Data Integrity feature introduced by  SPARC M7 Khalid Aziz <khalid.aziz@oracle.com> - 2017-01-11 20:00 +0100
            Re: [PATCH v4 0/4] Application Data Integrity feature introduced by  SPARC M7 Dave Hansen <dave.hansen@linux.intel.com> - 2017-01-11 20:20 +0100
              Re: [PATCH v4 0/4] Application Data Integrity feature introduced by  SPARC M7 Khalid Aziz <khalid.aziz@oracle.com> - 2017-01-12 01:30 +0100
                Re: [PATCH v4 0/4] Application Data Integrity feature introduced by  SPARC M7 Dave Hansen <dave.hansen@linux.intel.com> - 2017-01-12 01:50 +0100
                  Re: [PATCH v4 0/4] Application Data Integrity feature introduced by  SPARC M7 Khalid Aziz <khalid.aziz@oracle.com> - 2017-01-12 18:00 +0100
                    Re: [PATCH v4 0/4] Application Data Integrity feature introduced by  SPARC M7 Dave Hansen <dave.hansen@linux.intel.com> - 2017-01-12 19:00 +0100
                      Re: [PATCH v4 0/4] Application Data Integrity feature introduced by  SPARC M7 Khalid Aziz <khalid.aziz@oracle.com> - 2017-01-13 01:30 +0100
                        Re: [PATCH v4 0/4] Application Data Integrity feature introduced by  SPARC M7 Rob Gardner <rob.gardner@oracle.com> - 2017-01-13 02:40 +0100
                          Re: [PATCH v4 0/4] Application Data Integrity feature introduced by  SPARC M7 Khalid Aziz <khalid.aziz@oracle.com> - 2017-01-13 15:50 +0100
                            Re: [PATCH v4 0/4] Application Data Integrity feature introduced by  SPARC M7 Rob Gardner <rob.gardner@oracle.com> - 2017-01-13 16:40 +0100
                              Re: [PATCH v4 0/4] Application Data Integrity feature introduced by  SPARC M7 Dave Hansen <dave.hansen@linux.intel.com> - 2017-01-13 17:10 +0100
                                Re: [PATCH v4 0/4] Application Data Integrity feature introduced by  SPARC M7 Rob Gardner <rob.gardner@oracle.com> - 2017-01-13 18:40 +0100
                              Re: [PATCH v4 0/4] Application Data Integrity feature introduced by  SPARC M7 Khalid Aziz <khalid.aziz@oracle.com> - 2017-01-13 17:10 +0100
          Re: [PATCH v4 0/4] Application Data Integrity feature introduced  by SPARC M7 David Miller <davem@davemloft.net> - 2017-01-17 05:50 +0100
            Re: [PATCH v4 0/4] Application Data Integrity feature introduced by  SPARC M7 Khalid Aziz <khalid.aziz@oracle.com> - 2017-01-17 22:50 +0100
      Re: [PATCH v4 0/4] Application Data Integrity feature introduced  by SPARC M7 David Miller <davem@davemloft.net> - 2017-01-17 06:00 +0100
    Re: [PATCH v4 4/4] sparc64: Add support for ADI (Application Data  Integrity) David Miller <davem@davemloft.net> - 2017-01-17 05:40 +0100
      Re: [PATCH v4 4/4] sparc64: Add support for ADI (Application Data  Integrity) Khalid Aziz <khalid.aziz@oracle.com> - 2017-01-17 20:40 +0100
        Re: [PATCH v4 4/4] sparc64: Add support for ADI (Application Data  Integrity) David Miller <davem@davemloft.net> - 2017-01-17 21:00 +0100
          Re: [PATCH v4 4/4] sparc64: Add support for ADI (Application Data  Integrity) Khalid Aziz <khalid.aziz@oracle.com> - 2017-01-17 21:20 +0100
      Re: [PATCH v4 4/4] sparc64: Add support for ADI (Application Data  Integrity) Khalid Aziz <khalid.aziz@oracle.com> - 2017-01-18 01:20 +0100

Page 2 of 2 — ← Prev page 1 [2]

#1560257 — Re: [PATCH v4 0/4] Application Data Integrity feature introduced by SPARC M7

FromDavid Miller <davem@davemloft.net>
Date2017-01-17 06:00 +0100
SubjectRe: [PATCH v4 0/4] Application Data Integrity feature introduced by SPARC M7
Message-ID<t0u2d-2gM-1@gated-at.bofh.it>
In reply to#1556699
From: Dave Hansen <dave.hansen@linux.intel.com>
Date: Wed, 11 Jan 2017 08:33:30 -0800

> Is there a cost in the hardware associated with doing this "ADI
> checking"?  For instance, instead of having this new mprotect()
> interface, why not just always set TTE.mcd on all PTEs?

If we did this then for every page mapped into userspace we'd have
to explicitly set all of the tags to zero, otherwise we'd get TAG
mismatch exceptions.

That would be like clearing the every mapped anonymous page twice, or
worse.

[toc] | [prev] | [next] | [standalone]

#1560252 — Re: [PATCH v4 4/4] sparc64: Add support for ADI (Application Data Integrity)

FromDavid Miller <davem@davemloft.net>
Date2017-01-17 05:40 +0100
SubjectRe: [PATCH v4 4/4] sparc64: Add support for ADI (Application Data Integrity)
Message-ID<t0tIS-2as-13@gated-at.bofh.it>
In reply to#1556683
From: Khalid Aziz <khalid.aziz@oracle.com>
Date: Wed, 11 Jan 2017 09:12:54 -0700

> +		__asm__ __volatile__(
> +			".word 0xa1438000\n\t"	/* rd  %mcdper, %l0 */

Just use "rd %%asr14, %0" this way you don't have to play all of these
fixed register games which kill the code generated by gcc.  If you
forcefully clobber a windowed register like %l0 it means the function
being emitted can never be a leaf function, tail calls are no longer
allowed, etc.

> +			".word 0x9d800011\n\t"	/* wr  %g0, %l1, %mcdper */

Likewise use "wr %%g0, %0, %%asr14"

> +		".word 0xaf900001\n\t"	/* wrpr  %g0, %g1, %pmcdper */

Hmmm, which %asr encodes %pmcdper?

> diff --git a/arch/sparc/kernel/mdesc.c b/arch/sparc/kernel/mdesc.c
> index 8a6982d..68b03bf 100644
> --- a/arch/sparc/kernel/mdesc.c
> +++ b/arch/sparc/kernel/mdesc.c
> @@ -20,6 +20,7 @@
>  #include <asm/uaccess.h>
>  #include <asm/oplib.h>
>  #include <asm/smp.h>
> +#include <asm/adi.h>
>  
>  /* Unlike the OBP device tree, the machine description is a full-on
>   * DAG.  An arbitrary number of ARCs are possible from one
> @@ -1104,5 +1105,8 @@ void __init sun4v_mdesc_init(void)
>  
>  	cur_mdesc = hp;
>  
> +#ifdef CONFIG_SPARC64

mdesc.c is only built on sparc64, this ifdef is superfluous.

> +/* Update the state of MCDPER register in current task's mm context before
> + * dup so the dup'd task will inherit flags in this register correctly.
> + * Current task may have updated flags since it started running.
> + */
> +int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
> +{
> +	if (adi_capable() && src->mm) {
> +		register unsigned long tmp_mcdper;
> +
> +		__asm__ __volatile__(
> +			".word 0x83438000\n\t"	/* rd %mcdper, %g1 */
> +			"mov %%g1, %0\n\t"
> +			: "=r" (tmp_mcdper)
> +			:
> +			: "g1");
> +		src->mm->context.mcdper = tmp_mcdper;

I don't like the idea of duplicating 'mm' state using the task struct
copy.  Why do not the MM handling interfaces handle this properly?

Maybe it means you've abstracted the ADI register handling in the
wrong place.  Maybe it's a thread property which is "pushed" from
the MM context.

[toc] | [prev] | [next] | [standalone]

#1560964 — Re: [PATCH v4 4/4] sparc64: Add support for ADI (Application Data Integrity)

FromKhalid Aziz <khalid.aziz@oracle.com>
Date2017-01-17 20:40 +0100
SubjectRe: [PATCH v4 4/4] sparc64: Add support for ADI (Application Data Integrity)
Message-ID<t0HLQ-2F3-15@gated-at.bofh.it>
In reply to#1560252
On 01/16/2017 09:39 PM, David Miller wrote:
> From: Khalid Aziz <khalid.aziz@oracle.com>
> Date: Wed, 11 Jan 2017 09:12:54 -0700
>
>> +		__asm__ __volatile__(
>> +			".word 0xa1438000\n\t"	/* rd  %mcdper, %l0 */
>
> Just use "rd %%asr14, %0" this way you don't have to play all of these
> fixed register games which kill the code generated by gcc.  If you
> forcefully clobber a windowed register like %l0 it means the function
> being emitted can never be a leaf function, tail calls are no longer
> allowed, etc.

Hi David,

"rd %%asr14, %0" should work but does not due to bugs in assembler - 
<https://sourceware.org/ml/binutils/2016-03/msg00302.html>, and 
<https://sourceware.org/ml/binutils/2016-03/msg00303.html>. These bugs 
were fixed in binutils 2.27 but older assemblers will cause kernel build 
to fail. Using byte coded equivalent is the safest option.

>
>> +			".word 0x9d800011\n\t"	/* wr  %g0, %l1, %mcdper */
>
> Likewise use "wr %%g0, %0, %%asr14"
>
>> +		".word 0xaf900001\n\t"	/* wrpr  %g0, %g1, %pmcdper */
>
> Hmmm, which %asr encodes %pmcdper?

%pmcdper is not an asr, rather a privileged register (pr23).

Thanks,
Khalid

>
>> diff --git a/arch/sparc/kernel/mdesc.c b/arch/sparc/kernel/mdesc.c
>> index 8a6982d..68b03bf 100644
>> --- a/arch/sparc/kernel/mdesc.c
>> +++ b/arch/sparc/kernel/mdesc.c
>> @@ -20,6 +20,7 @@
>>  #include <asm/uaccess.h>
>>  #include <asm/oplib.h>
>>  #include <asm/smp.h>
>> +#include <asm/adi.h>
>>
>>  /* Unlike the OBP device tree, the machine description is a full-on
>>   * DAG.  An arbitrary number of ARCs are possible from one
>> @@ -1104,5 +1105,8 @@ void __init sun4v_mdesc_init(void)
>>
>>  	cur_mdesc = hp;
>>
>> +#ifdef CONFIG_SPARC64
>
> mdesc.c is only built on sparc64, this ifdef is superfluous.
>
>> +/* Update the state of MCDPER register in current task's mm context before
>> + * dup so the dup'd task will inherit flags in this register correctly.
>> + * Current task may have updated flags since it started running.
>> + */
>> +int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
>> +{
>> +	if (adi_capable() && src->mm) {
>> +		register unsigned long tmp_mcdper;
>> +
>> +		__asm__ __volatile__(
>> +			".word 0x83438000\n\t"	/* rd %mcdper, %g1 */
>> +			"mov %%g1, %0\n\t"
>> +			: "=r" (tmp_mcdper)
>> +			:
>> +			: "g1");
>> +		src->mm->context.mcdper = tmp_mcdper;
>
> I don't like the idea of duplicating 'mm' state using the task struct
> copy.  Why do not the MM handling interfaces handle this properly?
>
> Maybe it means you've abstracted the ADI register handling in the
> wrong place.  Maybe it's a thread property which is "pushed" from
> the MM context.
> --
> To unsubscribe from this list: send the line "unsubscribe sparclinux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[toc] | [prev] | [next] | [standalone]

#1560972 — Re: [PATCH v4 4/4] sparc64: Add support for ADI (Application Data Integrity)

FromDavid Miller <davem@davemloft.net>
Date2017-01-17 21:00 +0100
SubjectRe: [PATCH v4 4/4] sparc64: Add support for ADI (Application Data Integrity)
Message-ID<t0I5b-2Md-15@gated-at.bofh.it>
In reply to#1560964
From: Khalid Aziz <khalid.aziz@oracle.com>
Date: Tue, 17 Jan 2017 12:32:46 -0700

> On 01/16/2017 09:39 PM, David Miller wrote:
>> From: Khalid Aziz <khalid.aziz@oracle.com>
>> Date: Wed, 11 Jan 2017 09:12:54 -0700
>>
>>> +		__asm__ __volatile__(
>>> +			".word 0xa1438000\n\t"	/* rd  %mcdper, %l0 */
>>
>> Just use "rd %%asr14, %0" this way you don't have to play all of these
>> fixed register games which kill the code generated by gcc.  If you
>> forcefully clobber a windowed register like %l0 it means the function
>> being emitted can never be a leaf function, tail calls are no longer
>> allowed, etc.
> 
> Hi David,
> 
> "rd %%asr14, %0" should work but does not due to bugs in assembler -
> <https://sourceware.org/ml/binutils/2016-03/msg00302.html>, and
> <https://sourceware.org/ml/binutils/2016-03/msg00303.html>. These bugs
> were fixed in binutils 2.27 but older assemblers will cause kernel
> build to fail. Using byte coded equivalent is the safest option.

Fair enough.

Then please at least use %g1 or another usable global register to
avoid at least some of the problems I mentioned.

[toc] | [prev] | [next] | [standalone]

#1560983 — Re: [PATCH v4 4/4] sparc64: Add support for ADI (Application Data Integrity)

FromKhalid Aziz <khalid.aziz@oracle.com>
Date2017-01-17 21:20 +0100
SubjectRe: [PATCH v4 4/4] sparc64: Add support for ADI (Application Data Integrity)
Message-ID<t0Ioy-38E-35@gated-at.bofh.it>
In reply to#1560972
On 01/17/2017 12:42 PM, David Miller wrote:
> From: Khalid Aziz <khalid.aziz@oracle.com>
> Date: Tue, 17 Jan 2017 12:32:46 -0700
>
>> On 01/16/2017 09:39 PM, David Miller wrote:
>>> From: Khalid Aziz <khalid.aziz@oracle.com>
>>> Date: Wed, 11 Jan 2017 09:12:54 -0700
>>>
>>>> +		__asm__ __volatile__(
>>>> +			".word 0xa1438000\n\t"	/* rd  %mcdper, %l0 */
>>>
>>> Just use "rd %%asr14, %0" this way you don't have to play all of these
>>> fixed register games which kill the code generated by gcc.  If you
>>> forcefully clobber a windowed register like %l0 it means the function
>>> being emitted can never be a leaf function, tail calls are no longer
>>> allowed, etc.
>>
>> Hi David,
>>
>> "rd %%asr14, %0" should work but does not due to bugs in assembler -
>> <https://sourceware.org/ml/binutils/2016-03/msg00302.html>, and
>> <https://sourceware.org/ml/binutils/2016-03/msg00303.html>. These bugs
>> were fixed in binutils 2.27 but older assemblers will cause kernel
>> build to fail. Using byte coded equivalent is the safest option.
>
> Fair enough.
>
> Then please at least use %g1 or another usable global register to
> avoid at least some of the problems I mentioned.
>

Sure, I will do that. Thanks for the review and feedback.

--
Khalid

[toc] | [prev] | [next] | [standalone]

#1561170 — Re: [PATCH v4 4/4] sparc64: Add support for ADI (Application Data Integrity)

FromKhalid Aziz <khalid.aziz@oracle.com>
Date2017-01-18 01:20 +0100
SubjectRe: [PATCH v4 4/4] sparc64: Add support for ADI (Application Data Integrity)
Message-ID<t0M8O-5qs-11@gated-at.bofh.it>
In reply to#1560252
On 01/16/2017 09:39 PM, David Miller wrote:
> From: Khalid Aziz <khalid.aziz@oracle.com>
> Date: Wed, 11 Jan 2017 09:12:54 -0700
>
>> diff --git a/arch/sparc/kernel/mdesc.c b/arch/sparc/kernel/mdesc.c
>> index 8a6982d..68b03bf 100644
>> --- a/arch/sparc/kernel/mdesc.c
>> +++ b/arch/sparc/kernel/mdesc.c
>> @@ -20,6 +20,7 @@
>>  #include <asm/uaccess.h>
>>  #include <asm/oplib.h>
>>  #include <asm/smp.h>
>> +#include <asm/adi.h>
>>
>>  /* Unlike the OBP device tree, the machine description is a full-on
>>   * DAG.  An arbitrary number of ARCs are possible from one
>> @@ -1104,5 +1105,8 @@ void __init sun4v_mdesc_init(void)
>>
>>  	cur_mdesc = hp;
>>
>> +#ifdef CONFIG_SPARC64
>
> mdesc.c is only built on sparc64, this ifdef is superfluous.

Good point. I will fix it.

>
>> +/* Update the state of MCDPER register in current task's mm context before
>> + * dup so the dup'd task will inherit flags in this register correctly.
>> + * Current task may have updated flags since it started running.
>> + */
>> +int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
>> +{
>> +	if (adi_capable() && src->mm) {
>> +		register unsigned long tmp_mcdper;
>> +
>> +		__asm__ __volatile__(
>> +			".word 0x83438000\n\t"	/* rd %mcdper, %g1 */
>> +			"mov %%g1, %0\n\t"
>> +			: "=r" (tmp_mcdper)
>> +			:
>> +			: "g1");
>> +		src->mm->context.mcdper = tmp_mcdper;
>
> I don't like the idea of duplicating 'mm' state using the task struct
> copy.  Why do not the MM handling interfaces handle this properly?
>
> Maybe it means you've abstracted the ADI register handling in the
> wrong place.  Maybe it's a thread property which is "pushed" from
> the MM context.

I see what you are saying. This code updates mm->context.mcdper for the 
source thread with the current state of MCDPER since MCDPER can be 
changed by a userspace process any time. When userspace changes MCDPER, 
it is not saved into mm->context.mcdper until a context switch happens. 
This means during the timeslice for a thread, its mm->context.mcdper may 
not reflect the current value of MCDPER. Updating it ensures dup_mm() 
will copy the real current value of MCDPER into the newly forked thread. 
arch_dup_mmap() looks like a more appropriate place to do this. Do you 
agree?

Thanks,
Khalid

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web