LinuxPPC-Dev Archive on lore.kernel.org
 help / color / Atom feed
From: Ganesh <ganeshgr@linux.ibm.com>
To: Michael Ellerman <mpe@ellerman.id.au>
Cc: "Michal Suchánek" <msuchanek@suse.de>,
	linuxppc-dev@lists.ozlabs.org, keescook@chromium.org,
	npiggin@gmail.com, mahesh@linux.ibm.com
Subject: Re: [PATCH v4 2/2] lkdtm/powerpc: Add SLB multihit test
Date: Thu, 26 Nov 2020 18:57:08 +0530
Message-ID: <8e4c8bc6-8949-3627-d6d1-a468f9946501@linux.ibm.com> (raw)
In-Reply-To: <20201019131541.GL29778@kitsune.suse.cz>


On 10/19/20 6:45 PM, Michal Suchánek wrote:

> On Mon, Oct 19, 2020 at 09:59:57PM +1100, Michael Ellerman wrote:
>> Hi Ganesh,
>>
>> Some comments below ...
>>
>> Ganesh Goudar <ganeshgr@linux.ibm.com> writes:
>>> To check machine check handling, add support to inject slb
>>> multihit errors.
>>>
>>> Cc: Kees Cook <keescook@chromium.org>
>>> Reviewed-by: Michal Suchánek <msuchanek@suse.de>
>>> Co-developed-by: Mahesh Salgaonkar <mahesh@linux.ibm.com>
>>> Signed-off-by: Mahesh Salgaonkar <mahesh@linux.ibm.com>
>>> Signed-off-by: Ganesh Goudar <ganeshgr@linux.ibm.com>
>>> ---
>>>   drivers/misc/lkdtm/Makefile             |   1 +
>>>   drivers/misc/lkdtm/core.c               |   3 +
>>>   drivers/misc/lkdtm/lkdtm.h              |   3 +
>>>   drivers/misc/lkdtm/powerpc.c            | 156 ++++++++++++++++++++++++
>>>   tools/testing/selftests/lkdtm/tests.txt |   1 +
>>>   5 files changed, 164 insertions(+)
>>>   create mode 100644 drivers/misc/lkdtm/powerpc.c
>>>
>> ..
>>> diff --git a/drivers/misc/lkdtm/powerpc.c b/drivers/misc/lkdtm/powerpc.c
>>> new file mode 100644
>>> index 000000000000..f388b53dccba
>>> --- /dev/null
>>> +++ b/drivers/misc/lkdtm/powerpc.c
>>> @@ -0,0 +1,156 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +
>>> +#include "lkdtm.h"
>>> +#include <linux/slab.h>
>>> +#include <linux/vmalloc.h>
>> Usual style is to include the linux headers first and then the local header.

ok

>>> +
>>> +/* Gets index for new slb entry */
>>> +static inline unsigned long get_slb_index(void)
>>> +{
>>> +	unsigned long index;
>>> +
>>> +	index = get_paca()->stab_rr;
>>> +
>>> +	/*
>>> +	 * simple round-robin replacement of slb starting at SLB_NUM_BOLTED.
>>> +	 */
>>> +	if (index < (mmu_slb_size - 1))
>>> +		index++;
>>> +	else
>>> +		index = SLB_NUM_BOLTED;
>>> +	get_paca()->stab_rr = index;
>>> +	return index;
>>> +}
>> I'm not sure we need that really?
>>
>> We can just always insert at SLB_MUM_BOLTED and SLB_NUM_BOLTED + 1.
>>
>> Or we could allocate from the top down using mmu_slb_size - 1, and
>> mmu_slb_size - 2.

Ok, We can do that.

>>> +#define slb_esid_mask(ssize)	\
>>> +	(((ssize) == MMU_SEGSIZE_256M) ? ESID_MASK : ESID_MASK_1T)
>>> +
>>> +/* Form the operand for slbmte */
>>> +static inline unsigned long mk_esid_data(unsigned long ea, int ssize,
>>> +					 unsigned long slot)
>>> +{
>>> +	return (ea & slb_esid_mask(ssize)) | SLB_ESID_V | slot;
>>> +}
>>> +
>>> +#define slb_vsid_shift(ssize)	\
>>> +	((ssize) == MMU_SEGSIZE_256M ? SLB_VSID_SHIFT : SLB_VSID_SHIFT_1T)
>>> +
>>> +/* Form the operand for slbmte */
>>> +static inline unsigned long mk_vsid_data(unsigned long ea, int ssize,
>>> +					 unsigned long flags)
>>> +{
>>> +	return (get_kernel_vsid(ea, ssize) << slb_vsid_shift(ssize)) | flags |
>>> +		((unsigned long)ssize << SLB_VSID_SSIZE_SHIFT);
>>> +}
>> I realise it's not much code, but I'd rather those were in a header,
>> rather than copied from slb.c. That way they can never skew vs the
>> versions in slb.c
>>
>> Best place I think would be arch/powerpc/include/asm/book3s/64/mmu-hash.h

Ok, ill move them.

>>> +
>>> +/* Inserts new slb entry */
>> It inserts two.

Right.

>>> +static void insert_slb_entry(char *p, int ssize)
>>> +{
>>> +	unsigned long flags, entry;
>>> +
>>> +	flags = SLB_VSID_KERNEL | mmu_psize_defs[MMU_PAGE_64K].sllp;
>> That won't work if the kernel is built for 4K pages. Or at least it
>> won't work the way we want it to.
>>
>> You should use mmu_linear_psize.
>>
>> But for vmalloc you should use mmu_vmalloc_psize, so it will need to be
>> a parameter.

Sure, Thanks

>>> +	preempt_disable();
>>> +
>>> +	entry = get_slb_index();
>>> +	asm volatile("slbmte %0,%1" :
>>> +			: "r" (mk_vsid_data((unsigned long)p, ssize, flags)),
>>> +			  "r" (mk_esid_data((unsigned long)p, ssize, entry))
>>> +			: "memory");
>>> +
>>> +	entry = get_slb_index();
>>> +	asm volatile("slbmte %0,%1" :
>>> +			: "r" (mk_vsid_data((unsigned long)p, ssize, flags)),
>>> +			  "r" (mk_esid_data((unsigned long)p, ssize, entry))
>>> +			: "memory");
>>> +	preempt_enable();
>>> +	/*
>>> +	 * This triggers exception, If handled correctly we must recover
>>> +	 * from this error.
>>> +	 */
>>> +	p[0] = '!';
>> That doesn't belong in here, it should be done by the caller.
>>
>> That would also mean p could be unsigned long in here, so you wouldn't
>> have to cast it four times.

Sure, ill change it.

>>> +}
>>> +
>>> +/* Inject slb multihit on vmalloc-ed address i.e 0xD00... */
>>> +static void inject_vmalloc_slb_multihit(void)
>>> +{
>>> +	char *p;
>>> +
>>> +	p = vmalloc(2048);
>> vmalloc() allocates whole pages, so it may as well be vmalloc(PAGE_SIZE).

ok

>>> +	if (!p)
>>> +		return;
>> That's unlikely, but it should be an error that's propagated up to the caller.

ok

>>> +
>>> +	insert_slb_entry(p, MMU_SEGSIZE_1T);
>>> +	vfree(p);
>>> +}
>>> +
>>> +/* Inject slb multihit on kmalloc-ed address i.e 0xC00... */
>>> +static void inject_kmalloc_slb_multihit(void)
>>> +{
>>> +	char *p;
>>> +
>>> +	p = kmalloc(2048, GFP_KERNEL);
>>> +	if (!p)
>>> +		return;
>>> +
>>> +	insert_slb_entry(p, MMU_SEGSIZE_1T);
>>> +	kfree(p);
>>> +}
>>> +
>>> +/*
>>> + * Few initial SLB entries are bolted. Add a test to inject
>>> + * multihit in bolted entry 0.
>>> + */
>>> +static void insert_dup_slb_entry_0(void)
>>> +{
>>> +	unsigned long test_address = 0xC000000000000000;
>> Should use PAGE_OFFSET;

ok

>>> +	volatile unsigned long *test_ptr;
>> Does it need to be a volatile?
>> The slbmte should act as a compiler barrier (it has a memory clobber)
>> and a CPU barrier as well?

Yes, volatile is not required, ill remove it.

>>> +	unsigned long entry, i = 0;
>>> +	unsigned long esid, vsid;
>> Please group your variables:
>>
>>    unsigned long esid, vsid, entry, test_address, i;
>>    volatile unsigned long *test_ptr;
>>
>> And then initialise them as appropriate.

ok

>>> +	test_ptr = (unsigned long *)test_address;
>>> +	preempt_disable();
>>> +
>>> +	asm volatile("slbmfee  %0,%1" : "=r" (esid) : "r" (i));
>>> +	asm volatile("slbmfev  %0,%1" : "=r" (vsid) : "r" (i));
>> Why do we need to read them out of the SLB rather than just computing
>> the values?
> It ensures that the entry is perfect duplicate without copying even more
> code from other parts of the kernel, doesn't it?
>
> Especially when inserting only one duplicate as suggested later it
> ensures that the test really does what it should.
>>> +	entry = get_slb_index();
>>> +
>>> +	/* for i !=0 we would need to mask out the old entry number */
>> Or you could just compute esid and then it wouldn't be an issue.
>>
>>> +	asm volatile("slbmte %0,%1" :
>>> +			: "r" (vsid),
>>> +			  "r" (esid | entry)
>>> +			: "memory");
>> At this point we've just inserted a duplicate of entry 0. So you don't
>> need to insert a third entry do you?
> This code was obviously adapted from the previous one which needed two
> entries in case there was none for the memory region to start with.
>
> Addin only one duplicate should suffice and it can be easily tested that
> it still generates a MCE.
>
>>> +	asm volatile("slbmfee  %0,%1" : "=r" (esid) : "r" (i));
>>> +	asm volatile("slbmfev  %0,%1" : "=r" (vsid) : "r" (i));
>>> +	entry = get_slb_index();
>>> +
>>> +	/* for i !=0 we would need to mask out the old entry number */
>>> +	asm volatile("slbmte %0,%1" :
>>> +			: "r" (vsid),
>>> +			  "r" (esid | entry)
>>> +			: "memory");
>>> +
>>> +	pr_info("%s accessing test address 0x%lx: 0x%lx\n",
>>> +		__func__, test_address, *test_ptr);
>> This prints the first two instructions of the kernel. I happen to know
>> what values they should have, but most people won't understand what
>> they're seeing. A better test would be to read the value at the top of
>> the function and then load it again here and check we got the right
>> thing.
> It does not really matter what we read back so long as the compiler does
> not optimize out the read. The point here is to access an address in the
> range covered by the SLB entry 0. The failure case is that the system
> crashes and the test never finishes.
>
> Thanks
>
> Michal

  reply index

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-09  6:40 [PATCH v4 0/2] powerpc/mce: Fix mce handler and add selftest Ganesh Goudar
2020-10-09  6:40 ` [PATCH v4 1/2] powerpc/mce: remove nmi_enter/exit from real mode handler Ganesh Goudar
2020-10-09  6:40 ` [PATCH v4 2/2] lkdtm/powerpc: Add SLB multihit test Ganesh Goudar
2020-10-19 10:59   ` Michael Ellerman
2020-10-19 13:15     ` Michal Suchánek
2020-11-26 13:27       ` Ganesh [this message]
2020-10-16 11:32 ` [PATCH v4 0/2] powerpc/mce: Fix mce handler and add selftest Michael Ellerman
2020-10-19  5:26   ` Ganesh

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8e4c8bc6-8949-3627-d6d1-a468f9946501@linux.ibm.com \
    --to=ganeshgr@linux.ibm.com \
    --cc=keescook@chromium.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=mahesh@linux.ibm.com \
    --cc=mpe@ellerman.id.au \
    --cc=msuchanek@suse.de \
    --cc=npiggin@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LinuxPPC-Dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linuxppc-dev/0 linuxppc-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linuxppc-dev linuxppc-dev/ https://lore.kernel.org/linuxppc-dev \
		linuxppc-dev@lists.ozlabs.org linuxppc-dev@ozlabs.org
	public-inbox-index linuxppc-dev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.ozlabs.lists.linuxppc-dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git