On 03/02/2016 05:48 PM, Julian Calaby wrote:

Hi Khalid,

On Thu, Mar 3, 2016 at 11:25 AM, Khalid Aziz [off-list ref] wrote:

quoted

Thanks, Julian! I really appreciate your feedback.

No problem!

quoted

My comments below.

On 03/02/2016 04:08 PM, Julian Calaby wrote:

quoted

Hi Khalid,

On Thu, Mar 3, 2016 at 7:39 AM, Khalid Aziz [off-list ref]
wrote:

quoted

Enable Application Data Integrity (ADI) support in the sparc
kernel for applications to use ADI in userspace. ADI is a new
feature supported on sparc M7 and newer processors. ADI is supported
for data fetches only and not instruction fetches. This patch adds
prctl commands to enable and disable ADI (TSTATE.mcde), return ADI
parameters to userspace, enable/disable MCD (Memory Corruption
Detection) on selected memory ranges and enable TTE.mcd in PTEs. It
also adds handlers for all traps related to MCD. ADI is not enabled
by default for any task and a task must explicitly enable ADI
(TSTATE.mcde), turn MCD on on a memory range and set version tag
for ADI to be effective for the task. This patch adds support for
ADI for hugepages only. Addresses passed into system calls must be
non-ADI tagged addresses.

I can't comment on the actual functionality here, but I do see a few
minor style issues in your patch.

My big concern is that you're defining a lot of new code that is ADI
specific but isn't inside a CONFIG_SPARC_ADI ifdef. (That said,
handling ADI specific traps if ADI isn't enabled looks like a good
idea to me, however most of the other stuff is just dead code if
CONFIG_SPARC_ADI isn't enabled.)

Some of the code will be executed when CONFIG_SPARC_ADI is not enabled, for
instance init_adi() which will parse machine description to determine if
platform supports ADI. On the other hand, it might still make sense to
enclose this code in #ifdef. More on that below.

quoted

quoted

Signed-off-by: Khalid Aziz <redacted>
---
NOTES: ADI is a new feature added to M7 processor to allow hardware
          to catch rogue accesses to memory. An app can enable ADI on
          its data pages, set version tags on them and use versioned
          addresses (bits 63-60 of the address contain a version tag)
          to access the data pages. If a rogue app attempts to access
          ADI enabled data pages, its access is blocked and processor
          generates an exception. Enabling this functionality for all
          data pages of an app requires adding infrastructure to save
          version tags for any data pages that get swapped out and
          restoring those tags when pages are swapped back in. In this
          first implementation I am enabling ADI for hugepages only
          since these pages are locked in memory and hence avoid the
          issue of saving and restoring tags. Once this core functionality
          is stable, ADI for other memory pages can be enabled more
          easily.

v2:
          - Fixed a build error

   Documentation/prctl/sparc_adi.txt     |  62 ++++++++++
   Documentation/sparc/adi.txt           | 206
+++++++++++++++++++++++++++++++
   arch/sparc/Kconfig                    |  12 ++
   arch/sparc/include/asm/hugetlb.h      |  14 +++
   arch/sparc/include/asm/hypervisor.h   |   2 +
   arch/sparc/include/asm/mmu_64.h       |   1 +
   arch/sparc/include/asm/pgtable_64.h   |  15 +++
   arch/sparc/include/asm/processor_64.h |  19 +++
   arch/sparc/include/asm/ttable.h       |  10 ++
   arch/sparc/include/uapi/asm/asi.h     |   3 +
   arch/sparc/include/uapi/asm/pstate.h  |  10 ++
   arch/sparc/kernel/entry.h             |   3 +
   arch/sparc/kernel/head_64.S           |   1 +
   arch/sparc/kernel/mdesc.c             |  81 +++++++++++++
   arch/sparc/kernel/process_64.c        | 222
++++++++++++++++++++++++++++++++++
   arch/sparc/kernel/sun4v_mcd.S         |  16 +++
   arch/sparc/kernel/traps_64.c          |  96 ++++++++++++++-
   arch/sparc/kernel/ttable_64.S         |   6 +-
   include/linux/mm.h                    |   2 +
   include/uapi/asm-generic/siginfo.h    |   5 +-
   include/uapi/linux/prctl.h            |  16 +++
   kernel/sys.c                          |  30 +++++
   22 files changed, 826 insertions(+), 6 deletions(-)
   create mode 100644 Documentation/prctl/sparc_adi.txt
   create mode 100644 Documentation/sparc/adi.txt
   create mode 100644 arch/sparc/kernel/sun4v_mcd.S

I must admit that I'm slightly impressed that the documentation is
over a quarter of the lines added. =)

quoted

diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig
index 56442d2..0aac0ae 100644
--- a/arch/sparc/Kconfig
+++ b/arch/sparc/Kconfig

@@ -80,6 +80,7 @@ config SPARC64
          select NO_BOOTMEM
          select HAVE_ARCH_AUDITSYSCALL
          select ARCH_SUPPORTS_ATOMIC_RMW
+       select SPARC_ADI

This doesn't look right.

quoted

   config ARCH_DEFCONFIG
          string

@@ -314,6 +315,17 @@ if SPARC64
   source "kernel/power/Kconfig"
   endif

+config SPARC_ADI
+       bool "Application Data Integrity support"
+       def_bool y if SPARC64

def_bool is for config options without names (i.e. "this is a boolean
value and it's default is...")

So if you want people to be able to disable this option, then you
should remove the select above and just have:

bool "Application Data Integrity support"
default y if SPARC64

If you don't want people disabling it, then there's no point in having
a separate Kconfig symbol.

Ah, I see. I do not want people disabling it. I will make changes.

Why don't you want people disabling it? I must acknowledge that it's
not a lot of code, but I can see people wanting to build "minimal"
kernels for processors without ADI or to run some specific thing that
doesn't use ADI. Providing the kernel responds appropriately if
there's an unexpected ADI fault I don't see why the code would be
needed if it'll never be used.

Hi Julian,

My goal in making CONFIG_SPARC_ADI auto-selected was to not add yet 
another config option that end user has to understand and figure out 
what to do with, and make the kernel self-configuring where ADI simply 
becomes available if platform supports it. Kernel auto-detecting 
platform features is especially useful for distro kernels. I do see your 
point in being able to build a minimal kernel when building a custom 
kernel. Both options of making CONFIG_SPARC_ADI auto-selected or not, 
have pros and cons. I don't have a strong feeling about it one way or 
the other and can go either way.

Thanks,
Khalid

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>