Message-ID: <87r2pwae7z.fsf@xmission.com>
Date:   Wed, 07 Feb 2018 11:42:24 -0600
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Khalid Aziz <khalid.aziz@...cle.com>
Cc:     davem@...emloft.net, dave.hansen@...ux.intel.com,
        aarcange@...hat.com, akpm@...ux-foundation.org,
        allen.pais@...cle.com, anthony.yznaga@...cle.com, arnd@...db.de,
        babu.moger@...cle.com, benh@...nel.crashing.org,
        bob.picco@...cle.com, bsingharora@...il.com, corbet@....net,
        dan.j.williams@...el.com, dave.jiang@...el.com,
        david.j.aldridge@...cle.com, elena.reshetova@...el.com,
        glx@...utronix.de, gregkh@...uxfoundation.org, hannes@...xchg.org,
        hillf.zj@...baba-inc.com, hpa@...or.com, hughd@...gle.com,
        imbrenda@...ux.vnet.ibm.com, jack@...e.cz, jag.raman@...cle.com,
        jane.chu@...cle.com, jglisse@...hat.com, jroedel@...e.de,
        khalid@...ehiking.org, khandual@...ux.vnet.ibm.com,
        kirill.shutemov@...ux.intel.com, kstewart@...uxfoundation.org,
        ktkhai@...tuozzo.com, liam.merwick@...cle.com,
        linux-arch@...r.kernel.org, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        linuxppc-dev@...ts.ozlabs.org, linux@...ck-us.net, me@...in.cc,
        mgorman@...e.de, mgorman@...hsingularity.net, mhocko@...e.com,
        mike.kravetz@...cle.com, minchan@...nel.org, mingo@...nel.org,
        mingo@...hat.com, mpe@...erman.id.au, nadav.amit@...il.com,
        nagarathnam.muthusamy@...cle.com, nborisov@...e.com,
        n-horiguchi@...jp.nec.com, nick.alcock@...cle.com,
        nitin.m.gupta@...cle.com, ombredanne@...b.com,
        pasha.tatashin@...cle.com, paulus@...ba.org, pombredanne@...b.com,
        punit.agrawal@....com, rob.gardner@...cle.com,
        ross.zwisler@...ux.intel.com, shannon.nelson@...cle.com,
        shli@...com, sparclinux@...r.kernel.org, steven.sistare@...cle.com,
        tglx@...utronix.de, thomas.tai@...cle.com, tklauser@...tanz.ch,
        tom.hromatka@...cle.com, vegard.nossum@...cle.com,
        vijay.ac.kumar@...cle.com, willy@...radead.org, x86@...nel.org,
        zi.yan@...rutgers.edu
Subject: Re: [PATCH v11 00/10] Application Data Integrity feature introduced by SPARC M7

Khalid Aziz <khalid.aziz@...cle.com> writes:

> On 02/07/2018 12:38 AM, ebiederm@...ssion.com wrote:
>> Khalid Aziz <khalid.aziz@...cle.com> writes:
>>
>>> On 02/01/2018 07:29 PM, ebiederm@...ssion.com wrote:
>>>> Khalid Aziz <khalid.aziz@...cle.com> writes:
>>>>
>>>>> V11 changes:
>>>>> This series is same as v10 and was simply rebased on 4.15 kernel. Can
>>>>> mm maintainers please review patches 2, 7, 8 and 9 which are arch
>>>>> independent, and include/linux/mm.h and mm/ksm.c changes in patch 10
>>>>> and ack these if everything looks good?
>>>>
>>>> I am a bit puzzled how this differs from the pkey's that other
>>>> architectures are implementing to achieve a similar result.
>>>>
>>>> I am a bit mystified why you don't store the tag in a vma
>>>> instead of inventing a new way to store data on page out.
>>>
>>> Hello Eric,
>>>
>>> As Steven pointed out, sparc sets tags per cacheline unlike pkey. This results
>>> in much finer granularity for tags that pkey and hence requires larger tag
>>> storage than what we can do in a vma.
>>
>> *Nod*   I am a bit mystified where you keep the information in memory.
>> I would think the tags would need to be stored per cacheline or per
>> tlb entry, in some kind of cache that could overflow.  So I would be
>> surprised if swapping is the only time this information needs stored
>> in memory.  Which makes me wonder if you have the proper data
>> structures.
>>
>> I would think an array per vma or something in the page tables would
>> tend to make sense.
>>
>> But perhaps I am missing something.
>
> The ADI tags are stored in spare bits in the RAM. ADI tag storage is
> managed entirely by memory controller which maintains these tags per
> ADI block. An ADI block is the same size as cacheline on M7. Tags for
> each ADI block are associated with the physical ADI block, not the
> virtual address. When a physical page is reused, the physical ADI tag
> storage for that page is overwritten with new ADI tags, hence we need
> to store away the tags when we swap out a page. Kernel updates the ADI
> tags for physical page when it swaps a new page in. Each vma can cover
> variable number of pages so it is best to store a pointer to the tag
> storage in vma as opposed to actual tags in an array. Each 8K page can
> have 128 tags on it. Since each tag is 4 bits, we need 64 bytes per
> page to store the tags. That can add up for a large vma.

If the tags are already stored in RAM I can see why it does not make any
sense to store them except on page out.  Management wise this feels a
lot like the encrypted memory options I have been seeing on x86.

>>>> Can you please use force_sig_fault to send these signals instead
>>>> of force_sig_info.  Emperically I have found that it is very
>>>> error prone to generate siginfo's by hand, especially on code
>>>> paths where several different si_codes may apply.  So it helps
>>>> to go through a helper function to ensure the fiddly bits are
>>>> all correct.  AKA the unused bits all need to be set to zero before
>>>> struct siginfo is copied to userspace.
>>>>
>>>
>>> What you say makes sense. I followed the same code as other fault handlers for
>>> sparc. I could change just the fault handlers for ADI related faults. Would it
>>> make more sense to change all the fault handlers in a separate patch and keep
>>> the code in arch/sparc/kernel/traps_64.c consistent? Dave M, do you have a
>>> preference?
>>
>> It is my intention post -rc1 to start sending out patches to get the
>> rest of not just sparc but all of the architectures using the new
>> helpers.  I have the code I just ran out of time befor the merge
>> window opened to ensure everything had a good thorough review.
>>
>> So if you can handle the your new changes I expect I will handle the
>> rest.
>>
>
> I can add a patch at the end of my series to update all
> force_sig_info() in my patchset to force_sig_fault(). That will sync
> my patches up with your changes cleanly. Does that work for you? I can
> send an updated series with this change. Can you review and ack the
> patches after this change.

One additional patch would be fine.  I can certainly review and ack that
part.  You probably want to wait until post -rc1 so that you have a
clean base to work off of.

Eric

Powered by blists - more mailing lists