* Re: [PATCH v2 03/13] powerpc sstep: Prepare to support prefixed instructions
From: Jordan Niethe @ 2020-02-11 23:31 UTC (permalink / raw)
To: Christophe Leroy
Cc: Balamuruhan S, Alistair Popple, Daniel Axtens, linuxppc-dev
In-Reply-To: <11bf9d8f-ef21-3534-1c49-e3644a60b06d@c-s.fr>
On Tue, Feb 11, 2020 at 4:57 PM Christophe Leroy
<christophe.leroy@c-s.fr> wrote:
>
>
>
> Le 11/02/2020 à 06:33, Jordan Niethe a écrit :
> > Currently all instructions are a single word long. A future ISA version
> > will include prefixed instructions which have a double word length. The
> > functions used for analysing and emulating instructions need to be
> > modified so that they can handle these new instruction types.
> >
> > A prefixed instruction is a word prefix followed by a word suffix. All
> > prefixes uniquely have the primary op-code 1. Suffixes may be valid word
> > instructions or instructions that only exist as suffixes.
> >
> > In handling prefixed instructions it will be convenient to treat the
> > suffix and prefix as separate words. To facilitate this modify
> > analyse_instr() and emulate_step() to take a suffix as a
> > parameter. For word instructions it does not matter what is passed in
> > here - it will be ignored.
> >
> > We also define a new flag, PREFIXED, to be used in instruction_op:type.
> > This flag will indicate when emulating an analysed instruction if the
> > NIP should be advanced by word length or double word length.
> >
> > The callers of analyse_instr() and emulate_step() will need their own
> > changes to be able to support prefixed instructions. For now modify them
> > to pass in 0 as a suffix.
> >
> > Note that at this point no prefixed instructions are emulated or
> > analysed - this is just making it possible to do so.
> >
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> > ---
> > v2: - Move definition of __get_user_instr() and
> > __get_user_instr_inatomic() to "powerpc: Support prefixed instructions
> > in alignment handler."
> > - Use a macro for returning the length of an op
> > - Rename sufx -> suffix
> > - Define and use PPC_NO_SUFFIX instead of 0
> > ---
> > arch/powerpc/include/asm/ppc-opcode.h | 5 +++++
> > arch/powerpc/include/asm/sstep.h | 9 ++++++--
> > arch/powerpc/kernel/align.c | 2 +-
> > arch/powerpc/kernel/hw_breakpoint.c | 4 ++--
> > arch/powerpc/kernel/kprobes.c | 2 +-
> > arch/powerpc/kernel/mce_power.c | 2 +-
> > arch/powerpc/kernel/optprobes.c | 3 ++-
> > arch/powerpc/kernel/uprobes.c | 2 +-
> > arch/powerpc/kvm/emulate_loadstore.c | 2 +-
> > arch/powerpc/lib/sstep.c | 12 ++++++-----
> > arch/powerpc/lib/test_emulate_step.c | 30 +++++++++++++--------------
> > arch/powerpc/xmon/xmon.c | 5 +++--
> > 12 files changed, 46 insertions(+), 32 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h
> > index c1df75edde44..72783bc92e50 100644
> > --- a/arch/powerpc/include/asm/ppc-opcode.h
> > +++ b/arch/powerpc/include/asm/ppc-opcode.h
> > @@ -377,6 +377,11 @@
> > #define PPC_INST_VCMPEQUD 0x100000c7
> > #define PPC_INST_VCMPEQUB 0x10000006
> >
> > +/* macro to check if a word is a prefix */
> > +#define IS_PREFIX(x) (((x) >> 26) == 1)
>
> Can you add an OP_PREFIX in the OP list and use it instead of '1' ?
Will do.
>
> > +#define PPC_NO_SUFFIX 0
> > +#define PPC_INST_LENGTH(x) (IS_PREFIX(x) ? 8 : 4)
> > +
> > /* macros to insert fields into opcodes */
> > #define ___PPC_RA(a) (((a) & 0x1f) << 16)
> > #define ___PPC_RB(b) (((b) & 0x1f) << 11)
> > diff --git a/arch/powerpc/include/asm/sstep.h b/arch/powerpc/include/asm/sstep.h
> > index 769f055509c9..9ea8904a1549 100644
> > --- a/arch/powerpc/include/asm/sstep.h
> > +++ b/arch/powerpc/include/asm/sstep.h
> > @@ -89,11 +89,15 @@ enum instruction_type {
> > #define VSX_LDLEFT 4 /* load VSX register from left */
> > #define VSX_CHECK_VEC 8 /* check MSR_VEC not MSR_VSX for reg >= 32 */
> >
> > +/* Prefixed flag, ORed in with type */
> > +#define PREFIXED 0x800
> > +
> > /* Size field in type word */
> > #define SIZE(n) ((n) << 12)
> > #define GETSIZE(w) ((w) >> 12)
> >
> > #define GETTYPE(t) ((t) & INSTR_TYPE_MASK)
> > +#define OP_LENGTH(t) (((t) & PREFIXED) ? 8 : 4)
>
> Is it worth naming it OP_LENGTH ? Can't it be mistaken as one of the
> OP_xxx from the list in asm/opcode.h ?
>
> What about GETLENGTH() instead to be consistant with the above lines ?
Good point, will do.
>
> Christophe
^ permalink raw reply
* [Bug 206501] Kernel 5.6-rc1 fails to boot on a PowerMac G4 3,6 with CONFIG_VMAP_STACK=y: Oops! Machine check, sig: 7 [#1]
From: bugzilla-daemon @ 2020-02-11 19:26 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <bug-206501-206035@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=206501
--- Comment #1 from Erhard F. (erhard_f@mailbox.org) ---
Created attachment 287313
--> https://bugzilla.kernel.org/attachment.cgi?id=287313&action=edit
kernel .config (5.6.0-rc1, PowerMac G4 DP)
--
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* [Bug 206501] New: Kernel 5.6-rc1 fails to boot on a PowerMac G4 3,6 with CONFIG_VMAP_STACK=y: Oops! Machine check, sig: 7 [#1]
From: bugzilla-daemon @ 2020-02-11 19:25 UTC (permalink / raw)
To: linuxppc-dev
https://bugzilla.kernel.org/show_bug.cgi?id=206501
Bug ID: 206501
Summary: Kernel 5.6-rc1 fails to boot on a PowerMac G4 3,6 with
CONFIG_VMAP_STACK=y: Oops! Machine check, sig: 7 [#1]
Product: Platform Specific/Hardware
Version: 2.5
Kernel Version: 5.6.0-rc1
Hardware: PPC-32
OS: Linux
Tree: Mainline
Status: NEW
Severity: normal
Priority: P1
Component: PPC-32
Assignee: platform_ppc-32@kernel-bugs.osdl.org
Reporter: erhard_f@mailbox.org
Regression: No
Created attachment 287311
--> https://bugzilla.kernel.org/attachment.cgi?id=287311&action=edit
screenshot
The G4 boots fine with CONFIG_VMAP_STACK=n, but fails to boot with
CONFIG_VMAP_STACK=y.
[...]
NIP [c001c194] create_hpte+0xa8/0x120
LR [c001c0c4] add_hash_page+0x88/0xb0
Call Trace:
[f101dde8] [cO181568] alloc_set_pte+0x184/0x214 (unreliable)
[f101de18] [cO14d168] filemap_map_pages+0x21c/0x250
[f101de68] [c0181cf4] handle_mm_fault+0x66c/0x90c
[f101dee8] [c0019aac] do_page_fault+0x690/0x804
[f101df38] [c0014450] handle_page_fault+0x10/0x3c
--- interrupt: 401 at Oxb77ffd10
LR = 0x0
Instruction dump:
6c64003f 6884ffx0 3884fff8 7c0903a6 84x40008 7c062800 4002fff8 41a2008c
68a50040 7c0903a6 3883fff8 84c40008 <54c60001> 4002fff8 41a20070 3c80c08e
---[ end trace cd24dd23c7db9d53 ]---
Machine check in kernel mode.
Caused by (from SRR1=141020): Transfer error ack signal
Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000007
(OCRed screenshot + corrections by hand)
--
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* Re: Problem booting a PowerBook G4 Aluminum after commit cd08f109 with CONFIG_VMAP_STACK=y
From: Christophe Leroy @ 2020-02-11 19:23 UTC (permalink / raw)
To: Larry Finger; +Cc: linuxppc-dev@ozlabs.org
In-Reply-To: <608a1a18-a0ed-2059-bfbc-081c9ef1bfd8@lwfinger.net>
Le 11/02/2020 à 17:06, Larry Finger a écrit :
> On 2/11/20 12:55 AM, Christophe Leroy wrote:
>>
>>
>> Le 10/02/2020 à 13:55, Larry Finger a écrit :
>>> On 2/9/20 12:19 PM, Christophe Leroy wrote:
>>>> Do you have CONFIG_TRACE_IRQFLAGS in your config ?
>>>> If so, can you try the patch below ?
>>>>
>>>> https://patchwork.ozlabs.org/patch/1235081/
>>>>
>>>> Otherwise, can you send me your .config and tell me exactly where it
>>>> stops during the boot.
>>>
>>> Christophe,
>>>
>>> That patch did not work. My .config is attached.
>>>
>>> It does boot if CONFIG_VMAP_STACK is not set.
>>>
>>> The console display ends with the "DMA ranges" output. A screen shot
>>> is also appended.
>>>
>>> Larry
>>>
>>
>> Hi,
>>
>> I tried your config under QEMU, it works.
>>
>> In fact your console display is looping on itself, it ends at "printk:
>> bootconsole [udbg0] disabled".
>>
>> Looks like you get stuck at the time of switching to graphic mode.
>> Need to understand why.
>
> I'm not surprised that a real G4 differs from QEMU. For one thing, the
> real hardware uses i2c to connect to the graphics hardware.
>
> I realized that the screen was not scrolling and output was missing. To
> see what was missed, I added a call to btext_clearscreen(). As you
> noted, it ends at the bootconsole disabled statement.
>
> As I could not find any console output after that point, I then turned
> off the bootconsole disable. I realize this action may cause a different
> problem, but in this configuration, the computer hit a BUG Unable to
> handle kernel data access at 0x007a84fc. The faulting instruction
> address was 0x00013674. Those addresses look like physical, not virtual,
> addresses.
>
Can you send me a picture of that BUG Unable to handle kernel data
access with all the registers values etc..., together with the matching
vmlinux ?
First thing is to identify where we are when that happens. That mean see
what is at 0xc0013674. Can be done with 'ppc-linux-objdump -d vmlinux'
(Or whatever your PPC objdump is named) and get the function code.
Then we need to understand how we reach that function and why it tries
to access a physical address.
Another thing I'm thinking about, not necessarily related to that
problem: Some buggy drivers do DMA from stack. This doesn't work anymore
with CONFIG_VMAP_STACK. Most of them can be detected with
CONFIG_DEBUG_VIRTUAL so you should activate it.
Christophe
^ permalink raw reply
* Re: [PATCH v2] libnvdimm: Update persistence domain value for of_pmem and papr_scm device
From: Dan Williams @ 2020-02-11 16:38 UTC (permalink / raw)
To: Aneesh Kumar K.V; +Cc: linuxppc-dev, linux-nvdimm
In-Reply-To: <25eabdd9-410f-e4c3-6b0e-41a5e6daba10@linux.ibm.com>
On Tue, Feb 11, 2020 at 6:57 AM Aneesh Kumar K.V
<aneesh.kumar@linux.ibm.com> wrote:
>
> On 2/10/20 11:48 PM, Dan Williams wrote:
> > On Mon, Feb 10, 2020 at 6:20 AM Aneesh Kumar K.V
> > <aneesh.kumar@linux.ibm.com> wrote:
> >>
> >> Dan Williams <dan.j.williams@intel.com> writes:
> >>
> >>> On Tue, Feb 4, 2020 at 9:21 PM Aneesh Kumar K.V
> >>> <aneesh.kumar@linux.ibm.com> wrote:
> >>>>
> >>>> Currently, kernel shows the below values
> >>>> "persistence_domain":"cpu_cache"
> >>>> "persistence_domain":"memory_controller"
> >>>> "persistence_domain":"unknown"
> >>>>
> >>>> "cpu_cache" indicates no extra instructions is needed to ensure the persistence
> >>>> of data in the pmem media on power failure.
> >>>>
> >>>> "memory_controller" indicates platform provided instructions need to be issued
> >>>
> >>> No, it does not. The only requirement implied by "memory_controller"
> >>> is global visibility outside the cpu cache. If there are special
> >>> instructions beyond that then it isn't persistent memory, at least not
> >>> pmem that is safe for dax. virtio-pmem is an example of pmem-like
> >>> memory that is not enabled for userspace flushing (MAP_SYNC disabled).
> >>>
> >>
> >> Can you explain this more? The way I was expecting the application to
> >> interpret the value was, a regular store instruction doesn't guarantee
> >> persistence if you find the "memory_controller" value for
> >> persistence_domain. Instead, we need to make sure we flush data to the
> >> controller at which point the platform will take care of the persistence in
> >> case of power loss. How we flush data to the controller will also be
> >> defined by the platform.
> >
> > If the platform requires any flush mechanism outside of the base cpu
> > ISA of cache flushes and memory barriers then MAP_SYNC needs to be
> > explicitly disabled to force the application to call fsync()/msync().
> > Then those platform specific mechanisms need to be triggered through a
> > platform-aware driver.
> >
>
>
> Agreed. I was thinking we mark the persistence_domain: "Unknown" in that
> case. virtio-pmem mark it that way.
I would say the driver requirement case is persistence_domain "None",
not "Unknown". I.e. the platform provides no mechanism to flush data
to the persistence domain on power loss, it's back to typical storage
semantics.
>
>
> >>
> >>
> >>>> as per documented sequence to make sure data get flushed so that it is
> >>>> guaranteed to be on pmem media in case of system power loss.
> >>>>
> >>>> Based on the above use memory_controller for non volatile regions on ppc64.
> >>>>
> >>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> >>>> ---
> >>>> arch/powerpc/platforms/pseries/papr_scm.c | 7 ++++++-
> >>>> drivers/nvdimm/of_pmem.c | 4 +++-
> >>>> include/linux/libnvdimm.h | 1 -
> >>>> 3 files changed, 9 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> >>>> index 7525635a8536..ffcd0d7a867c 100644
> >>>> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> >>>> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> >>>> @@ -359,8 +359,13 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
> >>>>
> >>>> if (p->is_volatile)
> >>>> p->region = nvdimm_volatile_region_create(p->bus, &ndr_desc);
> >>>> - else
> >>>> + else {
> >>>> + /*
> >>>> + * We need to flush things correctly to guarantee persistance
> >>>> + */
> >>>
> >>> There are never guarantees. If you're going to comment what does
> >>> software need to flush, and how?
> >>
> >> Can you explain why you say there are never guarantees? If you follow the platform
> >> recommended instruction sequence to flush data, we can be sure of data
> >> persistence in the pmem media.
> >
> > Because storage can always fail. You can reduce risk, but never
> > eliminate it. This is similar to SSDs that use latent capacitance to
> > flush their write caches on driver power loss. Even if the application
> > successfully flushes its writes to buffers that are protected by that
> > capacitance that power source can still (and in practice does) fail.
> >
>
> ok guarantee is not the right term there. Can we say
>
> /* We need to flush tings correctly to ensure persistence */
The definition of the "memory_controller" persistence domain is: "the
platform takes care to flush writes to media once they are globally
visible outside the cache".
>
>
> What I was trying to understand/clarify was the detail an application
> can infer looking at the value of persistence_domain ?
>
> Do you agree that below can be inferred from the "memory_controller"
> value of persistence_domain
>
> 1) Application needs to use cache flush instructions and that ensures
> data is persistent across power failure.
>
>
> Or are you suggesting that application should not infer any of those
> details looking at persistence_domain value? If so what is the purpose
> of exporting that attribute?
The way the patch was worded I thought it was referring to an explicit
mechanism outside cpu cache flushes, i.e. a mechanism that required a
driver call.
>
>
> >>
> >>
> >>>
> >>>> + set_bit(ND_REGION_PERSIST_MEMCTRL, &ndr_desc.flags);
> >>>> p->region = nvdimm_pmem_region_create(p->bus, &ndr_desc);
> >>>> + }
> >>>> if (!p->region) {
> >>>> dev_err(dev, "Error registering region %pR from %pOF\n",
> >>>> ndr_desc.res, p->dn);
> >>>> diff --git a/drivers/nvdimm/of_pmem.c b/drivers/nvdimm/of_pmem.c
> >>>> index 8224d1431ea9..6826a274a1f1 100644
> >>>> --- a/drivers/nvdimm/of_pmem.c
> >>>> +++ b/drivers/nvdimm/of_pmem.c
> >>>> @@ -62,8 +62,10 @@ static int of_pmem_region_probe(struct platform_device *pdev)
> >>>>
> >>>> if (is_volatile)
> >>>> region = nvdimm_volatile_region_create(bus, &ndr_desc);
> >>>> - else
> >>>> + else {
> >>>> + set_bit(ND_REGION_PERSIST_MEMCTRL, &ndr_desc.flags);
> >>>> region = nvdimm_pmem_region_create(bus, &ndr_desc);
> >>>> + }
> >>>>
> >>>> if (!region)
> >>>> dev_warn(&pdev->dev, "Unable to register region %pR from %pOF\n",
> >>>> diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
> >>>> index 0f366706b0aa..771d888a5ed7 100644
> >>>> --- a/include/linux/libnvdimm.h
> >>>> +++ b/include/linux/libnvdimm.h
> >>>> @@ -54,7 +54,6 @@ enum {
> >>>> /*
> >>>> * Platform provides mechanisms to automatically flush outstanding
> >>>> * write data from memory controler to pmem on system power loss.
> >>>> - * (ADR)
> >>>
> >>> I'd rather not delete critical terminology for a developer / platform
> >>> owner to be able to consult documentation, or their vendor. Can you
> >>> instead add the PowerPC equivalent term for this capability? I.e. list
> >>> (x86: ADR PowerPC: foo ...).
> >>
> >> Power ISA doesn't clearly call out what mechanism will be used to ensure
> >> that a load following power loss will return the previously flushed
> >> data. Hence there is no description of details like Asynchronous DRAM
> >> Refresh. Only details specified is with respect to flush sequence that ensures
> >> that a load following power loss will return the value stored.
> >
> > What is this "flush sequence"?
> >
>
> cpu cache flush instructions "dcbf; hwsync"
Looks good, as long as the flush mechanism is defined by the cpu ISA
then MAP_SYNC is viable.
^ permalink raw reply
* Re: Problem booting a PowerBook G4 Aluminum after commit cd08f109 with CONFIG_VMAP_STACK=y
From: Larry Finger @ 2020-02-11 16:06 UTC (permalink / raw)
To: Christophe Leroy; +Cc: linuxppc-dev@ozlabs.org
In-Reply-To: <f35ea4e6-7c54-6acc-7d91-3a6eea56133c@c-s.fr>
On 2/11/20 12:55 AM, Christophe Leroy wrote:
>
>
> Le 10/02/2020 à 13:55, Larry Finger a écrit :
>> On 2/9/20 12:19 PM, Christophe Leroy wrote:
>>> Do you have CONFIG_TRACE_IRQFLAGS in your config ?
>>> If so, can you try the patch below ?
>>>
>>> https://patchwork.ozlabs.org/patch/1235081/
>>>
>>> Otherwise, can you send me your .config and tell me exactly where it stops
>>> during the boot.
>>
>> Christophe,
>>
>> That patch did not work. My .config is attached.
>>
>> It does boot if CONFIG_VMAP_STACK is not set.
>>
>> The console display ends with the "DMA ranges" output. A screen shot is also
>> appended.
>>
>> Larry
>>
>
> Hi,
>
> I tried your config under QEMU, it works.
>
> In fact your console display is looping on itself, it ends at "printk:
> bootconsole [udbg0] disabled".
>
> Looks like you get stuck at the time of switching to graphic mode. Need to
> understand why.
I'm not surprised that a real G4 differs from QEMU. For one thing, the real
hardware uses i2c to connect to the graphics hardware.
I realized that the screen was not scrolling and output was missing. To see what
was missed, I added a call to btext_clearscreen(). As you noted, it ends at the
bootconsole disabled statement.
As I could not find any console output after that point, I then turned off the
bootconsole disable. I realize this action may cause a different problem, but in
this configuration, the computer hit a BUG Unable to handle kernel data access
at 0x007a84fc. The faulting instruction address was 0x00013674. Those addresses
look like physical, not virtual, addresses.
I then added pr_info statements to bracket the failure. In file
drivers/video/fbdev/core/fb_ddc.c, the code reaches line 66, which is
algo_data->setsda(algo_data->data, 1);
Both pointers seem OK with algo_data = 0xeedfb4bc, and algo_data->data =
0xeedb25c. The code faults before returning. I then annotated that callback
routine radeon_gpio_setsda(), and found that execution is OK to the end of the
routine, but the fault happens on the return from this routine as though the
stack were corrupted.
I will be busy for about 8 hours, but if you can think of any debugging I can do
on this routine, please let me know.
Thanks,
Larry
^ permalink raw reply
* Re: [PATCH v2] libnvdimm: Update persistence domain value for of_pmem and papr_scm device
From: Aneesh Kumar K.V @ 2020-02-11 14:55 UTC (permalink / raw)
To: Dan Williams; +Cc: linuxppc-dev, linux-nvdimm
In-Reply-To: <CAPcyv4hNV88FJybgoRyM=JuKgrwYaf+CLWfFWt5X3yFMrecU=Q@mail.gmail.com>
On 2/10/20 11:48 PM, Dan Williams wrote:
> On Mon, Feb 10, 2020 at 6:20 AM Aneesh Kumar K.V
> <aneesh.kumar@linux.ibm.com> wrote:
>>
>> Dan Williams <dan.j.williams@intel.com> writes:
>>
>>> On Tue, Feb 4, 2020 at 9:21 PM Aneesh Kumar K.V
>>> <aneesh.kumar@linux.ibm.com> wrote:
>>>>
>>>> Currently, kernel shows the below values
>>>> "persistence_domain":"cpu_cache"
>>>> "persistence_domain":"memory_controller"
>>>> "persistence_domain":"unknown"
>>>>
>>>> "cpu_cache" indicates no extra instructions is needed to ensure the persistence
>>>> of data in the pmem media on power failure.
>>>>
>>>> "memory_controller" indicates platform provided instructions need to be issued
>>>
>>> No, it does not. The only requirement implied by "memory_controller"
>>> is global visibility outside the cpu cache. If there are special
>>> instructions beyond that then it isn't persistent memory, at least not
>>> pmem that is safe for dax. virtio-pmem is an example of pmem-like
>>> memory that is not enabled for userspace flushing (MAP_SYNC disabled).
>>>
>>
>> Can you explain this more? The way I was expecting the application to
>> interpret the value was, a regular store instruction doesn't guarantee
>> persistence if you find the "memory_controller" value for
>> persistence_domain. Instead, we need to make sure we flush data to the
>> controller at which point the platform will take care of the persistence in
>> case of power loss. How we flush data to the controller will also be
>> defined by the platform.
>
> If the platform requires any flush mechanism outside of the base cpu
> ISA of cache flushes and memory barriers then MAP_SYNC needs to be
> explicitly disabled to force the application to call fsync()/msync().
> Then those platform specific mechanisms need to be triggered through a
> platform-aware driver.
>
Agreed. I was thinking we mark the persistence_domain: "Unknown" in that
case. virtio-pmem mark it that way.
>>
>>
>>>> as per documented sequence to make sure data get flushed so that it is
>>>> guaranteed to be on pmem media in case of system power loss.
>>>>
>>>> Based on the above use memory_controller for non volatile regions on ppc64.
>>>>
>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>>> ---
>>>> arch/powerpc/platforms/pseries/papr_scm.c | 7 ++++++-
>>>> drivers/nvdimm/of_pmem.c | 4 +++-
>>>> include/linux/libnvdimm.h | 1 -
>>>> 3 files changed, 9 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
>>>> index 7525635a8536..ffcd0d7a867c 100644
>>>> --- a/arch/powerpc/platforms/pseries/papr_scm.c
>>>> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
>>>> @@ -359,8 +359,13 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
>>>>
>>>> if (p->is_volatile)
>>>> p->region = nvdimm_volatile_region_create(p->bus, &ndr_desc);
>>>> - else
>>>> + else {
>>>> + /*
>>>> + * We need to flush things correctly to guarantee persistance
>>>> + */
>>>
>>> There are never guarantees. If you're going to comment what does
>>> software need to flush, and how?
>>
>> Can you explain why you say there are never guarantees? If you follow the platform
>> recommended instruction sequence to flush data, we can be sure of data
>> persistence in the pmem media.
>
> Because storage can always fail. You can reduce risk, but never
> eliminate it. This is similar to SSDs that use latent capacitance to
> flush their write caches on driver power loss. Even if the application
> successfully flushes its writes to buffers that are protected by that
> capacitance that power source can still (and in practice does) fail.
>
ok guarantee is not the right term there. Can we say
/* We need to flush tings correctly to ensure persistence */
What I was trying to understand/clarify was the detail an application
can infer looking at the value of persistence_domain ?
Do you agree that below can be inferred from the "memory_controller"
value of persistence_domain
1) Application needs to use cache flush instructions and that ensures
data is persistent across power failure.
Or are you suggesting that application should not infer any of those
details looking at persistence_domain value? If so what is the purpose
of exporting that attribute?
>>
>>
>>>
>>>> + set_bit(ND_REGION_PERSIST_MEMCTRL, &ndr_desc.flags);
>>>> p->region = nvdimm_pmem_region_create(p->bus, &ndr_desc);
>>>> + }
>>>> if (!p->region) {
>>>> dev_err(dev, "Error registering region %pR from %pOF\n",
>>>> ndr_desc.res, p->dn);
>>>> diff --git a/drivers/nvdimm/of_pmem.c b/drivers/nvdimm/of_pmem.c
>>>> index 8224d1431ea9..6826a274a1f1 100644
>>>> --- a/drivers/nvdimm/of_pmem.c
>>>> +++ b/drivers/nvdimm/of_pmem.c
>>>> @@ -62,8 +62,10 @@ static int of_pmem_region_probe(struct platform_device *pdev)
>>>>
>>>> if (is_volatile)
>>>> region = nvdimm_volatile_region_create(bus, &ndr_desc);
>>>> - else
>>>> + else {
>>>> + set_bit(ND_REGION_PERSIST_MEMCTRL, &ndr_desc.flags);
>>>> region = nvdimm_pmem_region_create(bus, &ndr_desc);
>>>> + }
>>>>
>>>> if (!region)
>>>> dev_warn(&pdev->dev, "Unable to register region %pR from %pOF\n",
>>>> diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
>>>> index 0f366706b0aa..771d888a5ed7 100644
>>>> --- a/include/linux/libnvdimm.h
>>>> +++ b/include/linux/libnvdimm.h
>>>> @@ -54,7 +54,6 @@ enum {
>>>> /*
>>>> * Platform provides mechanisms to automatically flush outstanding
>>>> * write data from memory controler to pmem on system power loss.
>>>> - * (ADR)
>>>
>>> I'd rather not delete critical terminology for a developer / platform
>>> owner to be able to consult documentation, or their vendor. Can you
>>> instead add the PowerPC equivalent term for this capability? I.e. list
>>> (x86: ADR PowerPC: foo ...).
>>
>> Power ISA doesn't clearly call out what mechanism will be used to ensure
>> that a load following power loss will return the previously flushed
>> data. Hence there is no description of details like Asynchronous DRAM
>> Refresh. Only details specified is with respect to flush sequence that ensures
>> that a load following power loss will return the value stored.
>
> What is this "flush sequence"?
>
cpu cache flush instructions "dcbf; hwsync"
-aneesh
^ permalink raw reply
* Re: [PATCH v3 1/3] powerpc/tm: Fix clearing MSR[TS] in current when reclaiming on signal delivery
From: Sasha Levin @ 2020-02-11 14:17 UTC (permalink / raw)
To: Sasha Levin, Gustavo Luiz Duarte, linuxppc-dev; +Cc: mikey, stable, gromero
In-Reply-To: <20200211033831.11165-1-gustavold@linux.ibm.com>
Hi,
[This is an automated email]
This commit has been processed because it contains a "Fixes:" tag,
fixing commit: 2b0a576d15e0 ("powerpc: Add new transactional memory state to the signal context").
The bot has tested the following trees: v5.5.2, v5.4.18, v4.19.102, v4.14.170, v4.9.213, v4.4.213.
v5.5.2: Build OK!
v4.19.102: Build OK!
v4.14.170: Failed to apply! Possible dependencies:
1c200e63d055 ("powerpc/tm: Fix endianness flip on trap")
92fb8690bd04 ("powerpc/tm: P9 disable transactionally suspended sigcontexts")
v4.9.213: Failed to apply! Possible dependencies:
1c200e63d055 ("powerpc/tm: Fix endianness flip on trap")
92fb8690bd04 ("powerpc/tm: P9 disable transactionally suspended sigcontexts")
v4.4.213: Failed to apply! Possible dependencies:
1c200e63d055 ("powerpc/tm: Fix endianness flip on trap")
92fb8690bd04 ("powerpc/tm: P9 disable transactionally suspended sigcontexts")
a7d623d4d053 ("powerpc: Move part of giveup_vsx into c")
b86fd2bd0302 ("powerpc: Simplify TM restore checks")
d11994314b2b ("powerpc: signals: Stop using current in signal code")
d96f234f47af ("powerpc: Avoid load hit store in setup_sigcontext()")
e1c0d66fcb17 ("powerpc: Set used_(vsr|vr|spe) in sigreturn path when MSR bits are active")
NOTE: The patch will not be queued to stable trees until it is upstream.
How should we proceed with this patch?
--
Thanks,
Sasha
^ permalink raw reply
* Re: [PATCH RESEND] macintosh: convert to i2c_new_scanned_device
From: Michael Ellerman @ 2020-02-11 12:15 UTC (permalink / raw)
To: Wolfram Sang, linuxppc-dev; +Cc: Wolfram Sang, linux-i2c
In-Reply-To: <20200210170401.5357-1-wsa+renesas@sang-engineering.com>
Wolfram Sang <wsa+renesas@sang-engineering.com> writes:
> Move from the deprecated i2c_new_probed_device() to the new
> i2c_new_scanned_device(). No functional change for this driver because
> it doesn't check the return code anyhow.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>
> I can take this via I2C tree if this makes things easier...
Yes please. Sorry I missed it before.
Acked-by: Michael Ellerman <mpe@ellerman.id.au>
cheers
> drivers/macintosh/therm_windtunnel.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/macintosh/therm_windtunnel.c b/drivers/macintosh/therm_windtunnel.c
> index 8c744578122a..f15fec5e1cb6 100644
> --- a/drivers/macintosh/therm_windtunnel.c
> +++ b/drivers/macintosh/therm_windtunnel.c
> @@ -321,10 +321,10 @@ do_attach( struct i2c_adapter *adapter )
>
> memset(&info, 0, sizeof(struct i2c_board_info));
> strlcpy(info.type, "therm_ds1775", I2C_NAME_SIZE);
> - i2c_new_probed_device(adapter, &info, scan_ds1775, NULL);
> + i2c_new_scanned_device(adapter, &info, scan_ds1775, NULL);
>
> strlcpy(info.type, "therm_adm1030", I2C_NAME_SIZE);
> - i2c_new_probed_device(adapter, &info, scan_adm1030, NULL);
> + i2c_new_scanned_device(adapter, &info, scan_adm1030, NULL);
>
> if( x.thermostat && x.fan ) {
> x.running = 1;
> --
> 2.20.1
^ permalink raw reply
* [Bug 201723] [Bisected][Regression] THERM_WINDTUNNEL not working any longer in kernel 4.19.x (PowerMac G4 MDD)
From: bugzilla-daemon @ 2020-02-11 12:13 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <bug-201723-206035@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=201723
--- Comment #5 from Wolfram Sang (wsa@the-dreams.de) ---
I contacted Erhard by email to gather some more debug output. If we make
substantial progress, I will report it here.
Sidenote: therm_windtunnel has its own ADM1030 and DS1775 handling, so it
doesn't need the seperate drivers from HWMON. In theory, it should, but I guess
noone is up to refactoring all that old code.
--
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* Re: [PATCH V13] mm/debug: Add tests validating architecture page table helpers
From: Russell King - ARM Linux admin @ 2020-02-11 9:24 UTC (permalink / raw)
To: Christophe Leroy
Cc: Mark Rutland, uclinux-h8-devel, linux-m68k, linux-ia64, linux-sh,
Peter Zijlstra, James Hogan, Tetsuo Handa, Heiko Carstens,
Michal Hocko, linux-mm, Paul Mackerras, sparclinux, linux-hexagon,
linux-riscv, Thomas Gleixner, linux-s390, kbuild test robot,
Yoshinori Sato, Max Filippov, x86, Matthew Wilcox, Steven Price,
Jason Gunthorpe, Geert Uytterhoeven, Gerald Schaefer, Mark Salter,
Matt Turner, linux-snps-arc, Ingo Molnar, linux-xtensa, Kees Cook,
Anshuman Khandual, Masahiro Yamada, linux-alpha,
Aurelien Jacquiot, linux-c6x-dev, Mark Brown, Ivan Kokshaysky,
Paul Walmsley, Kirill A . Shutemov, Dan Williams, Guan Xuetao,
Vlastimil Babka, Richard Henderson, linux-arm-kernel,
Chris Zankel, Michal Simek, kbuild-all, Brian Cain,
Ard Biesheuvel, Greg Kroah-Hartman, Dave Hansen, linux-mips,
Ralf Baechle, linux-kernel, Paul Burton, Mike Rapoport,
Palmer Dabbelt, Vineet Gupta, Martin Schwidefsky, Qian Cai,
Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <aa540cd6-93a0-1be4-cdff-c9af43ed7d8e@c-s.fr>
On Tue, Feb 11, 2020 at 06:33:47AM +0100, Christophe Leroy wrote:
>
>
> Le 11/02/2020 à 03:25, Anshuman Khandual a écrit :
> >
> >
> > On 02/10/2020 04:36 PM, Russell King - ARM Linux admin wrote:
> > > There are good reasons for the way ARM does stuff. The generic crap was
> > > written without regard for the circumstances that ARM has, and thus is
> > > entirely unsuitable for 32-bit ARM.
> >
> > Since we dont have an agreement here, lets just settle with disabling the
> > test for now on platforms where the build fails. CONFIG_EXPERT is enabling
> > this test for better adaptability and coverage, hence how about re framing
> > the config like this ? This at the least conveys the fact that EXPERT only
> > works when platform is neither IA64 or ARM.
>
> Agreed
>
> >
> > config DEBUG_VM_PGTABLE
> > bool "Debug arch page table for semantics compliance"
> > depends on MMU
> > depends on ARCH_HAS_DEBUG_VM_PGTABLE || (EXPERT && !(IA64 || ARM))
>
> I think it's maybe better to have a dedicated depends line:
>
> depends on !IA64 && !ARM
> depends on ARCH_HAS_DEBUG_VM_PGTABLE || EXPERT
>
> The day arm and/or ia64 is ready for building the test, we can remove that
> depends.
Never going to happen as its technically infeasible, sorry.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [PATCH v3 6/7] powerpc/mm: implement set_memory_attr()
From: kbuild test robot @ 2020-02-11 7:25 UTC (permalink / raw)
To: Christophe Leroy; +Cc: kbuild-all, linux-kernel, Paul Mackerras, linuxppc-dev
In-Reply-To: <4a78ee8a7b954de0cfd44bd72b1b39c6fe34dc45.1580713729.git.christophe.leroy@c-s.fr>
[-- Attachment #1: Type: text/plain, Size: 3239 bytes --]
Hi Christophe,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on powerpc/next]
[also build test ERROR on v5.6-rc1 next-20200211]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-mm-Implement-set_memory-routines/20200204-030618
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-skiroot_defconfig (attached as .config)
compiler: powerpc64le-linux-gcc (GCC) 7.5.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.5.0 make.cross ARCH=powerpc
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from arch/powerpc/include/asm/page.h:304:0,
from arch/powerpc/include/asm/mmu.h:132,
from arch/powerpc/include/asm/lppaca.h:47,
from arch/powerpc/include/asm/paca.h:17,
from arch/powerpc/include/asm/current.h:13,
from include/linux/thread_info.h:21,
from include/asm-generic/preempt.h:5,
from ./arch/powerpc/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:78,
from include/linux/spinlock.h:51,
from include/linux/mmzone.h:8,
from include/linux/gfp.h:6,
from include/linux/mm.h:10,
from arch/powerpc/mm/pageattr.c:9:
arch/powerpc/mm/pageattr.c: In function 'set_page_attr':
>> arch/powerpc/mm/pageattr.c:85:27: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
pgprot_t prot = __pgprot((int)data);
^
arch/powerpc/include/asm/pgtable-be-types.h:69:36: note: in definition of macro '__pgprot'
#define __pgprot(x) ((pgprot_t) { (x) })
^
cc1: all warnings being treated as errors
vim +85 arch/powerpc/mm/pageattr.c
75
76 /*
77 * Set the attributes of a page:
78 *
79 * This function is used by PPC32 at the end of init to set final kernel memory
80 * protection. It includes changing the maping of the page it is executing from
81 * and data pages it is using.
82 */
83 static int set_page_attr(pte_t *ptep, unsigned long addr, void *data)
84 {
> 85 pgprot_t prot = __pgprot((int)data);
86
87 spin_lock(&init_mm.page_table_lock);
88
89 set_pte_at(&init_mm, addr, ptep, pte_modify(*ptep, prot));
90 flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
91
92 spin_unlock(&init_mm.page_table_lock);
93
94 return 0;
95 }
96
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 20746 bytes --]
^ permalink raw reply
* Re: Problem booting a PowerBook G4 Aluminum after commit cd08f109 with CONFIG_VMAP_STACK=y
From: Christophe Leroy @ 2020-02-11 6:55 UTC (permalink / raw)
To: Larry Finger; +Cc: linuxppc-dev@ozlabs.org
In-Reply-To: <6a1802b8-c6a7-d091-1036-689e089b786f@lwfinger.net>
Le 10/02/2020 à 13:55, Larry Finger a écrit :
> On 2/9/20 12:19 PM, Christophe Leroy wrote:
>> Do you have CONFIG_TRACE_IRQFLAGS in your config ?
>> If so, can you try the patch below ?
>>
>> https://patchwork.ozlabs.org/patch/1235081/
>>
>> Otherwise, can you send me your .config and tell me exactly where it
>> stops during the boot.
>
> Christophe,
>
> That patch did not work. My .config is attached.
>
> It does boot if CONFIG_VMAP_STACK is not set.
>
> The console display ends with the "DMA ranges" output. A screen shot is
> also appended.
>
> Larry
>
Hi,
I tried your config under QEMU, it works.
In fact your console display is looping on itself, it ends at "printk:
bootconsole [udbg0] disabled".
Looks like you get stuck at the time of switching to graphic mode. Need
to understand why.
Christophe
^ permalink raw reply
* Re: [PATCH v2 10/13] powerpc/kprobes: Support kprobes on prefixed instructions
From: Christophe Leroy @ 2020-02-11 6:46 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev; +Cc: bala24, alistair, mpe, dja
In-Reply-To: <20200211053355.21574-11-jniethe5@gmail.com>
Le 11/02/2020 à 06:33, Jordan Niethe a écrit :
> A prefixed instruction is composed of a word prefix followed by a word
> suffix. It does not make sense to be able to have a kprobe on the suffix
> of a prefixed instruction, so make this impossible.
>
> Kprobes work by replacing an instruction with a trap and saving that
> instruction to be single stepped out of place later. Currently there is
> not enough space allocated to keep a prefixed instruction for single
> stepping. Increase the amount of space allocated for holding the
> instruction copy.
>
> kprobe_post_handler() expects all instructions to be 4 bytes long which
> means that it does not function correctly for prefixed instructions.
> Add checks for prefixed instructions which will use a length of 8 bytes
> instead.
>
> For optprobes we normally patch in loading the instruction we put a
> probe on into r4 before calling emulate_step(). We now make space and
> patch in loading the suffix into r5 as well.
>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> arch/powerpc/include/asm/kprobes.h | 5 +--
> arch/powerpc/kernel/kprobes.c | 47 +++++++++++++++++++++-------
> arch/powerpc/kernel/optprobes.c | 32 ++++++++++---------
> arch/powerpc/kernel/optprobes_head.S | 6 ++++
> 4 files changed, 63 insertions(+), 27 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/kprobes.h b/arch/powerpc/include/asm/kprobes.h
> index 66b3f2983b22..0d44ce8a3163 100644
> --- a/arch/powerpc/include/asm/kprobes.h
> +++ b/arch/powerpc/include/asm/kprobes.h
> @@ -38,12 +38,13 @@ extern kprobe_opcode_t optprobe_template_entry[];
> extern kprobe_opcode_t optprobe_template_op_address[];
> extern kprobe_opcode_t optprobe_template_call_handler[];
> extern kprobe_opcode_t optprobe_template_insn[];
> +extern kprobe_opcode_t optprobe_template_suffix[];
> extern kprobe_opcode_t optprobe_template_call_emulate[];
> extern kprobe_opcode_t optprobe_template_ret[];
> extern kprobe_opcode_t optprobe_template_end[];
>
> -/* Fixed instruction size for powerpc */
> -#define MAX_INSN_SIZE 1
> +/* Prefixed instructions are two words */
> +#define MAX_INSN_SIZE 2
> #define MAX_OPTIMIZED_LENGTH sizeof(kprobe_opcode_t) /* 4 bytes */
> #define MAX_OPTINSN_SIZE (optprobe_template_end - optprobe_template_entry)
> #define RELATIVEJUMP_SIZE sizeof(kprobe_opcode_t) /* 4 bytes */
> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index 24a56f062d9e..b061deba4fe7 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -104,17 +104,30 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
>
> int arch_prepare_kprobe(struct kprobe *p)
> {
> + int len;
> int ret = 0;
> + struct kprobe *prev;
> kprobe_opcode_t insn = *p->addr;
> + kprobe_opcode_t prefix = *(p->addr - 1);
>
> + preempt_disable();
> if ((unsigned long)p->addr & 0x03) {
> printk("Attempt to register kprobe at an unaligned address\n");
> ret = -EINVAL;
> } else if (IS_MTMSRD(insn) || IS_RFID(insn) || IS_RFI(insn)) {
> printk("Cannot register a kprobe on rfi/rfid or mtmsr[d]\n");
> ret = -EINVAL;
> + } else if (IS_PREFIX(prefix)) {
> + printk("Cannot register a kprobe on the second word of prefixed instruction\n");
> + ret = -EINVAL;
> + }
> + prev = get_kprobe(p->addr - 1);
> + if (prev && IS_PREFIX(*prev->ainsn.insn)) {
> + printk("Cannot register a kprobe on the second word of prefixed instruction\n");
> + ret = -EINVAL;
> }
>
> +
> /* insn must be on a special executable page on ppc64. This is
> * not explicitly required on ppc32 (right now), but it doesn't hurt */
> if (!ret) {
> @@ -124,14 +137,18 @@ int arch_prepare_kprobe(struct kprobe *p)
> }
>
> if (!ret) {
> - memcpy(p->ainsn.insn, p->addr,
> - MAX_INSN_SIZE * sizeof(kprobe_opcode_t));
> + if (IS_PREFIX(insn))
> + len = MAX_INSN_SIZE * sizeof(kprobe_opcode_t);
> + else
> + len = sizeof(kprobe_opcode_t);
> + memcpy(p->ainsn.insn, p->addr, len);
This code is about to get changed, see
https://patchwork.ozlabs.org/patch/1232619/
> p->opcode = *p->addr;
> flush_icache_range((unsigned long)p->ainsn.insn,
> (unsigned long)p->ainsn.insn + sizeof(kprobe_opcode_t));
> }
>
> p->ainsn.boostable = 0;
> + preempt_enable_no_resched();
> return ret;
> }
> NOKPROBE_SYMBOL(arch_prepare_kprobe);
> @@ -216,10 +233,11 @@ NOKPROBE_SYMBOL(arch_prepare_kretprobe);
> static int try_to_emulate(struct kprobe *p, struct pt_regs *regs)
> {
> int ret;
> - unsigned int insn = *p->ainsn.insn;
> + unsigned int insn = p->ainsn.insn[0];
> + unsigned int suffix = p->ainsn.insn[1];
>
> /* regs->nip is also adjusted if emulate_step returns 1 */
> - ret = emulate_step(regs, insn, PPC_NO_SUFFIX);
> + ret = emulate_step(regs, insn, suffix);
> if (ret > 0) {
> /*
> * Once this instruction has been boosted
> @@ -233,7 +251,11 @@ static int try_to_emulate(struct kprobe *p, struct pt_regs *regs)
> * So, we should never get here... but, its still
> * good to catch them, just in case...
> */
> - printk("Can't step on instruction %x\n", insn);
> + if (!IS_PREFIX(insn))
> + printk("Can't step on instruction %x\n", insn);
> + else
> + printk("Can't step on instruction %x %x\n", insn,
> + suffix);
Maybe %x:%x as in xmon ?
Christophe
^ permalink raw reply
* Re: [PATCH V13] mm/debug: Add tests validating architecture page table helpers
From: Christophe Leroy @ 2020-02-11 5:33 UTC (permalink / raw)
To: Anshuman Khandual, Russell King - ARM Linux admin
Cc: Mark Rutland, uclinux-h8-devel, linux-m68k, linux-ia64, linux-sh,
Peter Zijlstra, James Hogan, Tetsuo Handa, Heiko Carstens,
Michal Hocko, linux-mm, Dave Hansen, Paul Mackerras, sparclinux,
linux-hexagon, linux-riscv, Thomas Gleixner, linux-s390,
kbuild test robot, Yoshinori Sato, Max Filippov, x86,
Matthew Wilcox, Steven Price, Jason Gunthorpe, Geert Uytterhoeven,
Gerald Schaefer, Mark Salter, Matt Turner, linux-snps-arc,
Ingo Molnar, linux-xtensa, Kees Cook, Masahiro Yamada,
linux-alpha, Aurelien Jacquiot, linux-c6x-dev, Mark Brown,
Ivan Kokshaysky, Paul Walmsley, Kirill A . Shutemov, Dan Williams,
Guan Xuetao, Vlastimil Babka, Richard Henderson, linux-arm-kernel,
Chris Zankel, Michal Simek, kbuild-all, Brian Cain,
Ard Biesheuvel, Greg Kroah-Hartman, linux-mips, Ralf Baechle,
linux-kernel, Paul Burton, Mike Rapoport, Palmer Dabbelt,
Vineet Gupta, Martin Schwidefsky, Qian Cai, Andrew Morton,
linuxppc-dev, David S. Miller
In-Reply-To: <82317d50-57a2-ce84-7557-21635d57448e@arm.com>
Le 11/02/2020 à 03:25, Anshuman Khandual a écrit :
>
>
> On 02/10/2020 04:36 PM, Russell King - ARM Linux admin wrote:
>> There are good reasons for the way ARM does stuff. The generic crap was
>> written without regard for the circumstances that ARM has, and thus is
>> entirely unsuitable for 32-bit ARM.
>
> Since we dont have an agreement here, lets just settle with disabling the
> test for now on platforms where the build fails. CONFIG_EXPERT is enabling
> this test for better adaptability and coverage, hence how about re framing
> the config like this ? This at the least conveys the fact that EXPERT only
> works when platform is neither IA64 or ARM.
Agreed
>
> config DEBUG_VM_PGTABLE
> bool "Debug arch page table for semantics compliance"
> depends on MMU
> depends on ARCH_HAS_DEBUG_VM_PGTABLE || (EXPERT && !(IA64 || ARM))
I think it's maybe better to have a dedicated depends line:
depends on !IA64 && !ARM
depends on ARCH_HAS_DEBUG_VM_PGTABLE || EXPERT
The day arm and/or ia64 is ready for building the test, we can remove
that depends.
> default n if !ARCH_HAS_DEBUG_VM_PGTABLE
> default y if DEBUG_VM
>
Christophe
^ permalink raw reply
* Re: [PATCH v2 09/13] powerpc/xmon: Dump prefixed instructions
From: Christophe Leroy @ 2020-02-11 6:39 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev; +Cc: bala24, alistair, mpe, dja
In-Reply-To: <20200211053355.21574-10-jniethe5@gmail.com>
Le 11/02/2020 à 06:33, Jordan Niethe a écrit :
> Currently when xmon is dumping instructions it reads a word at a time
> and then prints that instruction (either as a hex number or by
> disassembling it). For prefixed instructions it would be nice to show
> its prefix and suffix as together. Use read_instr() so that if a prefix
> is encountered its suffix is loaded too. Then print these in the form:
> prefix:suffix
> Xmon uses the disassembly routines from GNU binutils. These currently do
> not support prefixed instructions so we will not disassemble the
> prefixed instructions yet.
>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> v2: Rename sufx to suffix
> ---
> arch/powerpc/xmon/xmon.c | 50 +++++++++++++++++++++++++++++++---------
> 1 file changed, 39 insertions(+), 11 deletions(-)
>
> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> index 0b085642bbe7..513901ee18b0 100644
> --- a/arch/powerpc/xmon/xmon.c
> +++ b/arch/powerpc/xmon/xmon.c
> @@ -2903,6 +2903,21 @@ prdump(unsigned long adrs, long ndump)
> }
> }
>
> +static bool instrs_are_equal(unsigned long insta, unsigned long suffixa,
> + unsigned long instb, unsigned long suffixb)
> +{
> + if (insta != instb)
> + return false;
> +
> + if (!IS_PREFIX(insta) && !IS_PREFIX(instb))
> + return true;
> +
> + if (IS_PREFIX(insta) && IS_PREFIX(instb))
> + return suffixa == suffixb;
> +
> + return false;
> +}
> +
> typedef int (*instruction_dump_func)(unsigned long inst, unsigned long addr);
>
> static int
> @@ -2911,12 +2926,11 @@ generic_inst_dump(unsigned long adr, long count, int praddr,
> {
> int nr, dotted;
> unsigned long first_adr;
> - unsigned int inst, last_inst = 0;
> - unsigned char val[4];
> + unsigned int inst, suffix, last_inst = 0, last_suffix = 0;
>
> dotted = 0;
> - for (first_adr = adr; count > 0; --count, adr += 4) {
> - nr = mread(adr, val, 4);
> + for (first_adr = adr; count > 0; --count, adr += nr) {
> + nr = read_instr(adr, &inst, &suffix);
> if (nr == 0) {
> if (praddr) {
> const char *x = fault_chars[fault_type];
> @@ -2924,8 +2938,9 @@ generic_inst_dump(unsigned long adr, long count, int praddr,
> }
> break;
> }
> - inst = GETWORD(val);
> - if (adr > first_adr && inst == last_inst) {
> + if (adr > first_adr && instrs_are_equal(inst, suffix,
> + last_inst,
> + last_suffix)) {
> if (!dotted) {
> printf(" ...\n");
> dotted = 1;
> @@ -2934,11 +2949,24 @@ generic_inst_dump(unsigned long adr, long count, int praddr,
> }
> dotted = 0;
> last_inst = inst;
> - if (praddr)
> - printf(REG" %.8x", adr, inst);
> - printf("\t");
> - dump_func(inst, adr);
> - printf("\n");
> + last_suffix = suffix;
> + if (IS_PREFIX(inst)) {
> + if (praddr)
> + printf(REG" %.8x:%.8x", adr, inst, suffix);
> + printf("\t");
> + /*
> + * Just use this until binutils ppc disassembly
> + * prints prefixed instructions.
> + */
> + printf("%.8x:%.8x", inst, suffix);
> + printf("\n");
> + } else {
> + if (praddr)
> + printf(REG" %.8x", adr, inst);
> + printf("\t");
> + dump_func(inst, adr);
> + printf("\n");
> + }
What about:
if (pr_addr) {
printf(REG" %.8x", adr, inst);
if (IS_PREFIX(inst))
printf(":%.8x", suffix);
}
printf("\t");
if (IS_PREFIX(inst))
printf("%.8x:%.8x", inst, suffix);
else
dump_func(inst, adr);
printf("\n");
> }
> return adr - first_adr;
> }
>
Christophe
^ permalink raw reply
* Re: [PATCH v2 08/13] powerpc/xmon: Add initial support for prefixed instructions
From: Christophe Leroy @ 2020-02-11 6:32 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev; +Cc: bala24, alistair, mpe, dja
In-Reply-To: <20200211053355.21574-9-jniethe5@gmail.com>
Le 11/02/2020 à 06:33, Jordan Niethe a écrit :
> A prefixed instruction is composed of a word prefix and a word suffix.
> It does not make sense to be able to have a breakpoint on the suffix of
> a prefixed instruction, so make this impossible.
>
> When leaving xmon_core() we check to see if we are currently at a
> breakpoint. If this is the case, the breakpoint needs to be proceeded
> from. Initially emulate_step() is tried, but if this fails then we need
> to execute the saved instruction out of line. The NIP is set to the
> address of bpt::instr[] for the current breakpoint. bpt::instr[]
> contains the instruction replaced by the breakpoint, followed by a trap
> instruction. After bpt::instr[0] is executed and we hit the trap we
> enter back into xmon_bpt(). We know that if we got here and the offset
> indicates we are at bpt::instr[1] then we have just executed out of line
> so we can put the NIP back to the instruction after the breakpoint
> location and continue on.
>
> Adding prefixed instructions complicates this as the bpt::instr[1] needs
> to be used to hold the suffix. To deal with this make bpt::instr[] big
> enough for three word instructions. bpt::instr[2] contains the trap,
> and in the case of word instructions pad bpt::instr[1] with a noop.
>
> No support for disassembling prefixed instructions.
>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> v2: Rename sufx to suffix
> ---
> arch/powerpc/xmon/xmon.c | 82 ++++++++++++++++++++++++++++++++++------
> 1 file changed, 71 insertions(+), 11 deletions(-)
>
> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> index 897e512c6379..0b085642bbe7 100644
> --- a/arch/powerpc/xmon/xmon.c
> +++ b/arch/powerpc/xmon/xmon.c
> @@ -97,7 +97,8 @@ static long *xmon_fault_jmp[NR_CPUS];
> /* Breakpoint stuff */
> struct bpt {
> unsigned long address;
> - unsigned int instr[2];
> + /* Prefixed instructions can not cross 64-byte boundaries */
> + unsigned int instr[3] __aligned(64);
> atomic_t ref_count;
> int enabled;
> unsigned long pad;
> @@ -113,6 +114,7 @@ static struct bpt bpts[NBPTS];
> static struct bpt dabr;
> static struct bpt *iabr;
> static unsigned bpinstr = 0x7fe00008; /* trap */
> +static unsigned nopinstr = 0x60000000; /* nop */
Use PPC_INST_NOP instead of 0x60000000
And this nopinstr variable will never change. Why not use directly
PPC_INST_NOP in the code ?
>
> #define BP_NUM(bp) ((bp) - bpts + 1)
>
> @@ -120,6 +122,7 @@ static unsigned bpinstr = 0x7fe00008; /* trap */
> static int cmds(struct pt_regs *);
> static int mread(unsigned long, void *, int);
> static int mwrite(unsigned long, void *, int);
> +static int read_instr(unsigned long, unsigned int *, unsigned int *);
> static int handle_fault(struct pt_regs *);
> static void byterev(unsigned char *, int);
> static void memex(void);
> @@ -706,7 +709,7 @@ static int xmon_core(struct pt_regs *regs, int fromipi)
> bp = at_breakpoint(regs->nip);
> if (bp != NULL) {
> int stepped = emulate_step(regs, bp->instr[0],
> - PPC_NO_SUFFIX);
> + bp->instr[1]);
> if (stepped == 0) {
> regs->nip = (unsigned long) &bp->instr[0];
> atomic_inc(&bp->ref_count);
> @@ -761,8 +764,8 @@ static int xmon_bpt(struct pt_regs *regs)
>
> /* Are we at the trap at bp->instr[1] for some bp? */
> bp = in_breakpoint_table(regs->nip, &offset);
> - if (bp != NULL && offset == 4) {
> - regs->nip = bp->address + 4;
> + if (bp != NULL && (offset == 4 || offset == 8)) {
> + regs->nip = bp->address + offset;
> atomic_dec(&bp->ref_count);
> return 1;
> }
> @@ -864,7 +867,8 @@ static struct bpt *in_breakpoint_table(unsigned long nip, unsigned long *offp)
> return NULL;
> off %= sizeof(struct bpt);
> if (off != offsetof(struct bpt, instr[0])
> - && off != offsetof(struct bpt, instr[1]))
> + && off != offsetof(struct bpt, instr[1])
> + && off != offsetof(struct bpt, instr[2]))
> return NULL;
> *offp = off - offsetof(struct bpt, instr[0]);
> return (struct bpt *) (nip - off);
> @@ -881,9 +885,18 @@ static struct bpt *new_breakpoint(unsigned long a)
>
> for (bp = bpts; bp < &bpts[NBPTS]; ++bp) {
> if (!bp->enabled && atomic_read(&bp->ref_count) == 0) {
> + /*
> + * Prefixed instructions are two words, but regular
> + * instructions are only one. Use a nop to pad out the
> + * regular instructions so that we can place the trap
> + * at the same plac. For prefixed instructions the nop
plac ==> place
> + * will get overwritten during insert_bpts().
> + */
> bp->address = a;
> - bp->instr[1] = bpinstr;
> + bp->instr[1] = nopinstr;
> store_inst(&bp->instr[1]);
> + bp->instr[2] = bpinstr;
> + store_inst(&bp->instr[2]);
> return bp;
Not directly related to this patch, but shouldn't we use
patch_instruction() instead ?
> }
> }
> @@ -895,13 +908,15 @@ static struct bpt *new_breakpoint(unsigned long a)
> static void insert_bpts(void)
> {
> int i;
> - struct bpt *bp;
> + unsigned int prefix;
> + struct bpt *bp, *bp2;
>
> bp = bpts;
> for (i = 0; i < NBPTS; ++i, ++bp) {
> if ((bp->enabled & (BP_TRAP|BP_CIABR)) == 0)
> continue;
> - if (mread(bp->address, &bp->instr[0], 4) != 4) {
> + if (!read_instr(bp->address, &bp->instr[0],
> + &bp->instr[1])) {
> printf("Couldn't read instruction at %lx, "
> "disabling breakpoint there\n", bp->address);
> bp->enabled = 0;
> @@ -913,7 +928,34 @@ static void insert_bpts(void)
> bp->enabled = 0;
> continue;
> }
> + /*
> + * Check the address is not a suffix by looking for a prefix in
> + * front of it.
> + */
> + if ((mread(bp->address - 4, &prefix, 4) == 4) &&
> + IS_PREFIX(prefix)) {
> + printf("Breakpoint at %lx is on the second word of a "
> + "prefixed instruction, disabling it\n",
> + bp->address);
> + bp->enabled = 0;
> + continue;
> + }
> + /*
> + * We might still be a suffix - if the prefix has already been
> + * replaced by a breakpoint we won't catch it with the above
> + * test.
> + */
> + bp2 = at_breakpoint(bp->address - 4);
> + if (bp2 && IS_PREFIX(bp2->instr[0])) {
> + printf("Breakpoint at %lx is on the second word of a "
> + "prefixed instruction, disabling it\n",
> + bp->address);
> + bp->enabled = 0;
> + continue;
> + }
> store_inst(&bp->instr[0]);
> + if (IS_PREFIX(bp->instr[0]))
> + store_inst(&bp->instr[1]);
> if (bp->enabled & BP_CIABR)
> continue;
> if (patch_instruction((unsigned int *)bp->address,
> @@ -1164,14 +1206,14 @@ static int do_step(struct pt_regs *regs)
> */
> static int do_step(struct pt_regs *regs)
> {
> - unsigned int instr;
> + unsigned int instr, suffix;
> int stepped;
>
> force_enable_xmon();
> /* check we are in 64-bit kernel mode, translation enabled */
> if ((regs->msr & (MSR_64BIT|MSR_PR|MSR_IR)) == (MSR_64BIT|MSR_IR)) {
> - if (mread(regs->nip, &instr, 4) == 4) {
> - stepped = emulate_step(regs, instr, PPC_NO_SUFFIX);
> + if (read_instr(regs->nip, &instr, &suffix)) {
> + stepped = emulate_step(regs, instr, suffix);
> if (stepped < 0) {
> printf("Couldn't single-step %s instruction\n",
> (IS_RFID(instr)? "rfid": "mtmsrd"));
> @@ -2130,6 +2172,24 @@ mwrite(unsigned long adrs, void *buf, int size)
> return n;
> }
>
> +static int read_instr(unsigned long addr, unsigned int *instr,
> + unsigned int *suffix)
> +{
Don't know if it is worth it, but wouldn't it be better to define a
mread_inst() based on mread() instead of doing something that chain
calls to mread()
> + int r;
> +
> + r = mread(addr, instr, 4);
> + if (r != 4)
> + return 0;
> + if (!IS_PREFIX(*instr))
> + return 4;
> + r = mread(addr + 4, suffix, 4);
> + if (r != 4)
> + return 0;
> +
> + return 8;
> +}
> +
> +
> static int fault_type;
> static int fault_except;
> static char *fault_chars[] = { "--", "**", "##" };
>
Christophe
^ permalink raw reply
* Re: [PATCH v2 06/13] powerpc: Support prefixed instructions in alignment handler
From: Christophe Leroy @ 2020-02-11 6:14 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev; +Cc: bala24, alistair, mpe, dja
In-Reply-To: <20200211053355.21574-7-jniethe5@gmail.com>
Le 11/02/2020 à 06:33, Jordan Niethe a écrit :
> Alignment interrupts can be caused by prefixed instructions accessing
> memory. In the alignment handler the instruction that caused the
> exception is loaded and attempted emulate. If the instruction is a
> prefixed instruction load the prefix and suffix to emulate. After
> emulating increment the NIP by 8.
>
> Prefixed instructions are not permitted to cross 64-byte boundaries. If
> they do the alignment interrupt is invoked with SRR1 BOUNDARY bit set.
> If this occurs send a SIGBUS to the offending process if in user mode.
> If in kernel mode call bad_page_fault().
>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> v2: - Move __get_user_instr() and __get_user_instr_inatomic() to this
> commit (previously in "powerpc sstep: Prepare to support prefixed
> instructions").
> - Rename sufx to suffix
> - Use a macro for calculating instruction length
> ---
> arch/powerpc/include/asm/uaccess.h | 30 ++++++++++++++++++++++++++++++
> arch/powerpc/kernel/align.c | 8 +++++---
> arch/powerpc/kernel/traps.c | 21 ++++++++++++++++++++-
> 3 files changed, 55 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> index 2f500debae21..30f63a81c8d8 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -474,4 +474,34 @@ static __must_check inline bool user_access_begin(const void __user *ptr, size_t
> #define unsafe_copy_to_user(d, s, l, e) \
> unsafe_op_wrap(raw_copy_to_user_allowed(d, s, l), e)
>
Could it go close to other __get_user() and friends instead of being at
the end of the file ?
> +/*
> + * When reading an instruction iff it is a prefix, the suffix needs to be also
> + * loaded.
> + */
> +#define __get_user_instr(x, y, ptr) \
> +({ \
> + long __gui_ret = 0; \
> + y = 0; \
> + __gui_ret = __get_user(x, ptr); \
> + if (!__gui_ret) { \
> + if (IS_PREFIX(x)) \
Does this apply to PPC32 ?
If not, can we make sure IS_PREFIX is constant 0 on PPC32 so that the
second read gets dropped at compile time ?
Can we instead do :
if (!__gui_ret && IS_PREFIX(x))
> + __gui_ret = __get_user(y, ptr + 1); \
> + } \
> + \
> + __gui_ret; \
> +})
> +
> +#define __get_user_instr_inatomic(x, y, ptr) \
> +({ \
> + long __gui_ret = 0; \
> + y = 0; \
> + __gui_ret = __get_user_inatomic(x, ptr); \
> + if (!__gui_ret) { \
> + if (IS_PREFIX(x)) \
Same commments as above
> + __gui_ret = __get_user_inatomic(y, ptr + 1); \
> + } \
> + \
> + __gui_ret; \
> +})
> +
> #endif /* _ARCH_POWERPC_UACCESS_H */
> diff --git a/arch/powerpc/kernel/align.c b/arch/powerpc/kernel/align.c
> index ba3bf5c3ab62..e42cfaa616d3 100644
> --- a/arch/powerpc/kernel/align.c
> +++ b/arch/powerpc/kernel/align.c
> @@ -293,7 +293,7 @@ static int emulate_spe(struct pt_regs *regs, unsigned int reg,
>
> int fix_alignment(struct pt_regs *regs)
> {
> - unsigned int instr;
> + unsigned int instr, suffix;
> struct instruction_op op;
> int r, type;
>
> @@ -303,13 +303,15 @@ int fix_alignment(struct pt_regs *regs)
> */
> CHECK_FULL_REGS(regs);
>
> - if (unlikely(__get_user(instr, (unsigned int __user *)regs->nip)))
> + if (unlikely(__get_user_instr(instr, suffix,
> + (unsigned int __user *)regs->nip)))
> return -EFAULT;
> if ((regs->msr & MSR_LE) != (MSR_KERNEL & MSR_LE)) {
> /* We don't handle PPC little-endian any more... */
> if (cpu_has_feature(CPU_FTR_PPC_LE))
> return -EIO;
> instr = swab32(instr);
> + suffix = swab32(suffix);
> }
>
> #ifdef CONFIG_SPE
> @@ -334,7 +336,7 @@ int fix_alignment(struct pt_regs *regs)
> if ((instr & 0xfc0006fe) == (PPC_INST_COPY & 0xfc0006fe))
> return -EIO;
>
> - r = analyse_instr(&op, regs, instr, PPC_NO_SUFFIX);
> + r = analyse_instr(&op, regs, instr, suffix);
> if (r < 0)
> return -EINVAL;
>
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index 82a3438300fd..d80b82fc1ae3 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -583,6 +583,10 @@ static inline int check_io_access(struct pt_regs *regs)
> #define REASON_ILLEGAL (ESR_PIL | ESR_PUO)
> #define REASON_PRIVILEGED ESR_PPR
> #define REASON_TRAP ESR_PTR
> +#define REASON_PREFIXED 0
> +#define REASON_BOUNDARY 0
> +
> +#define inst_length(reason) 4
>
> /* single-step stuff */
> #define single_stepping(regs) (current->thread.debug.dbcr0 & DBCR0_IC)
> @@ -597,6 +601,10 @@ static inline int check_io_access(struct pt_regs *regs)
> #define REASON_ILLEGAL SRR1_PROGILL
> #define REASON_PRIVILEGED SRR1_PROGPRIV
> #define REASON_TRAP SRR1_PROGTRAP
> +#define REASON_PREFIXED SRR1_PREFIXED
> +#define REASON_BOUNDARY SRR1_BOUNDARY
> +
> +#define inst_length(reason) (((reason) & REASON_PREFIXED) ? 8 : 4)
>
> #define single_stepping(regs) ((regs)->msr & MSR_SE)
> #define clear_single_step(regs) ((regs)->msr &= ~MSR_SE)
> @@ -1593,11 +1601,20 @@ void alignment_exception(struct pt_regs *regs)
> {
> enum ctx_state prev_state = exception_enter();
> int sig, code, fixed = 0;
> + unsigned long reason;
>
> /* We restore the interrupt state now */
> if (!arch_irq_disabled_regs(regs))
> local_irq_enable();
>
> + reason = get_reason(regs);
> +
> + if (reason & REASON_BOUNDARY) {
> + sig = SIGBUS;
> + code = BUS_ADRALN;
> + goto bad;
> + }
> +
> if (tm_abort_check(regs, TM_CAUSE_ALIGNMENT | TM_CAUSE_PERSISTENT))
> goto bail;
>
> @@ -1606,7 +1623,8 @@ void alignment_exception(struct pt_regs *regs)
> fixed = fix_alignment(regs);
>
> if (fixed == 1) {
> - regs->nip += 4; /* skip over emulated instruction */
> + /* skip over emulated instruction */
> + regs->nip += inst_length(reason);
> emulate_single_step(regs);
> goto bail;
> }
> @@ -1619,6 +1637,7 @@ void alignment_exception(struct pt_regs *regs)
> sig = SIGBUS;
> code = BUS_ADRALN;
> }
> +bad:
> if (user_mode(regs))
> _exception(sig, regs, code, regs->dar);
> else
>
Christophe
^ permalink raw reply
* Re: [PATCH v2 04/13] powerpc sstep: Add support for prefixed load/stores
From: Christophe Leroy @ 2020-02-11 6:05 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev; +Cc: bala24, alistair, mpe, dja
In-Reply-To: <20200211053355.21574-5-jniethe5@gmail.com>
Le 11/02/2020 à 06:33, Jordan Niethe a écrit :
> This adds emulation support for the following prefixed integer
> load/stores:
> * Prefixed Load Byte and Zero (plbz)
> * Prefixed Load Halfword and Zero (plhz)
> * Prefixed Load Halfword Algebraic (plha)
> * Prefixed Load Word and Zero (plwz)
> * Prefixed Load Word Algebraic (plwa)
> * Prefixed Load Doubleword (pld)
> * Prefixed Store Byte (pstb)
> * Prefixed Store Halfword (psth)
> * Prefixed Store Word (pstw)
> * Prefixed Store Doubleword (pstd)
> * Prefixed Load Quadword (plq)
> * Prefixed Store Quadword (pstq)
>
> the follow prefixed floating-point load/stores:
> * Prefixed Load Floating-Point Single (plfs)
> * Prefixed Load Floating-Point Double (plfd)
> * Prefixed Store Floating-Point Single (pstfs)
> * Prefixed Store Floating-Point Double (pstfd)
>
> and for the following prefixed VSX load/stores:
> * Prefixed Load VSX Scalar Doubleword (plxsd)
> * Prefixed Load VSX Scalar Single-Precision (plxssp)
> * Prefixed Load VSX Vector [0|1] (plxv, plxv0, plxv1)
> * Prefixed Store VSX Scalar Doubleword (pstxsd)
> * Prefixed Store VSX Scalar Single-Precision (pstxssp)
> * Prefixed Store VSX Vector [0|1] (pstxv, pstxv0, pstxv1)
>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> v2: - Combine all load/store patches
> - Fix the name of Type 01 instructions
> - Remove sign extension flag from pstd/pld
> - Rename sufx -> suffix
> ---
> arch/powerpc/lib/sstep.c | 165 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 165 insertions(+)
>
> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> index 65143ab1bf64..0e21c21ff2be 100644
> --- a/arch/powerpc/lib/sstep.c
> +++ b/arch/powerpc/lib/sstep.c
> @@ -187,6 +187,44 @@ static nokprobe_inline unsigned long xform_ea(unsigned int instr,
> return ea;
> }
>
> +/*
> + * Calculate effective address for a MLS:D-form / 8LS:D-form
> + * prefixed instruction
> + */
> +static nokprobe_inline unsigned long mlsd_8lsd_ea(unsigned int instr,
> + unsigned int suffix,
> + const struct pt_regs *regs)
> +{
> + int ra, prefix_r;
> + unsigned int dd;
> + unsigned long ea, d0, d1, d;
> +
> + prefix_r = instr & (1ul << 20);
> + ra = (suffix >> 16) & 0x1f;
> +
> + d0 = instr & 0x3ffff;
> + d1 = suffix & 0xffff;
> + d = (d0 << 16) | d1;
> +
> + /*
> + * sign extend a 34 bit number
> + */
> + dd = (unsigned int) (d >> 2);
> + ea = (signed int) dd;
> + ea = (ea << 2) | (d & 0x3);
> +
> + if (!prefix_r && ra)
> + ea += regs->gpr[ra];
> + else if (!prefix_r && !ra)
> + ; /* Leave ea as is */
> + else if (prefix_r && !ra)
> + ea += regs->nip;
> + else if (prefix_r && ra)
> + ; /* Invalid form. Should already be checked for by caller! */
> +
> + return ea;
> +}
> +
> /*
> * Return the largest power of 2, not greater than sizeof(unsigned long),
> * such that x is a multiple of it.
> @@ -1166,6 +1204,7 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
> unsigned int instr, unsigned int suffix)
> {
> unsigned int opcode, ra, rb, rc, rd, spr, u;
> + unsigned int suffixopcode, prefixtype, prefix_r;
> unsigned long int imm;
> unsigned long int val, val2;
> unsigned int mb, me, sh;
> @@ -2652,6 +2691,132 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
>
> }
>
> +/*
> + * Prefixed instructions
> + */
> + switch (opcode) {
> + case 1:
Why not include it in the above switch () ?
Should it be enclosed by #ifdef __powerpc64__, or will this new ISA also
apply to 32 bits processors ?
> + prefix_r = instr & (1ul << 20);
> + ra = (suffix >> 16) & 0x1f;
> + op->update_reg = ra;
> + rd = (suffix >> 21) & 0x1f;
> + op->reg = rd;
> + op->val = regs->gpr[rd];
> +
> + suffixopcode = suffix >> 26;
> + prefixtype = (instr >> 24) & 0x3;
> + switch (prefixtype) {
> + case 0: /* Type 00 Eight-Byte Load/Store */
> + if (prefix_r && ra)
> + break;
> + op->ea = mlsd_8lsd_ea(instr, suffix, regs);
> + switch (suffixopcode) {
> + case 41: /* plwa */
> + op->type = MKOP(LOAD, PREFIXED | SIGNEXT, 4);
> + break;
> + case 42: /* plxsd */
> + op->reg = rd + 32;
> + op->type = MKOP(LOAD_VSX, PREFIXED, 8);
> + op->element_size = 8;
> + op->vsx_flags = VSX_CHECK_VEC;
> + break;
> + case 43: /* plxssp */
> + op->reg = rd + 32;
> + op->type = MKOP(LOAD_VSX, PREFIXED, 4);
> + op->element_size = 8;
> + op->vsx_flags = VSX_FPCONV | VSX_CHECK_VEC;
> + break;
> + case 46: /* pstxsd */
> + op->reg = rd + 32;
> + op->type = MKOP(STORE_VSX, PREFIXED, 8);
> + op->element_size = 8;
> + op->vsx_flags = VSX_CHECK_VEC;
> + break;
> + case 47: /* pstxssp */
> + op->reg = rd + 32;
> + op->type = MKOP(STORE_VSX, PREFIXED, 4);
> + op->element_size = 8;
> + op->vsx_flags = VSX_FPCONV | VSX_CHECK_VEC;
> + break;
> + case 51: /* plxv1 */
> + op->reg += 32;
> +
> + /* fallthru */
> + case 50: /* plxv0 */
> + op->type = MKOP(LOAD_VSX, PREFIXED, 16);
> + op->element_size = 16;
> + op->vsx_flags = VSX_CHECK_VEC;
> + break;
> + case 55: /* pstxv1 */
> + op->reg = rd + 32;
> +
> + /* fallthru */
> + case 54: /* pstxv0 */
> + op->type = MKOP(STORE_VSX, PREFIXED, 16);
> + op->element_size = 16;
> + op->vsx_flags = VSX_CHECK_VEC;
> + break;
> + case 56: /* plq */
> + op->type = MKOP(LOAD, PREFIXED, 16);
> + break;
> + case 57: /* pld */
> + op->type = MKOP(LOAD, PREFIXED, 8);
> + break;
> + case 60: /* stq */
> + op->type = MKOP(STORE, PREFIXED, 16);
> + break;
> + case 61: /* pstd */
> + op->type = MKOP(STORE, PREFIXED, 8);
> + break;
> + }
> + break;
> + case 1: /* Type 01 Eight-Byte Register-to-Register */
> + break;
> + case 2: /* Type 10 Modified Load/Store */
> + if (prefix_r && ra)
> + break;
> + op->ea = mlsd_8lsd_ea(instr, suffix, regs);
> + switch (suffixopcode) {
> + case 32: /* plwz */
> + op->type = MKOP(LOAD, PREFIXED, 4);
> + break;
> + case 34: /* plbz */
> + op->type = MKOP(LOAD, PREFIXED, 1);
> + break;
> + case 36: /* pstw */
> + op->type = MKOP(STORE, PREFIXED, 4);
> + break;
> + case 38: /* pstb */
> + op->type = MKOP(STORE, PREFIXED, 1);
> + break;
> + case 40: /* plhz */
> + op->type = MKOP(LOAD, PREFIXED, 2);
> + break;
> + case 42: /* plha */
> + op->type = MKOP(LOAD, PREFIXED | SIGNEXT, 2);
> + break;
> + case 44: /* psth */
> + op->type = MKOP(STORE, PREFIXED, 2);
> + break;
> + case 48: /* plfs */
> + op->type = MKOP(LOAD_FP, PREFIXED | FPCONV, 4);
> + break;
> + case 50: /* plfd */
> + op->type = MKOP(LOAD_FP, PREFIXED, 8);
> + break;
> + case 52: /* pstfs */
> + op->type = MKOP(STORE_FP, PREFIXED | FPCONV, 4);
> + break;
> + case 54: /* pstfd */
> + op->type = MKOP(STORE_FP, PREFIXED, 8);
> + break;
> + }
> + break;
> + case 3: /* Type 11 Modified Register-to-Register */
> + break;
> + }
> + }
> +
> #ifdef CONFIG_VSX
> if ((GETTYPE(op->type) == LOAD_VSX ||
> GETTYPE(op->type) == STORE_VSX) &&
>
Christophe
^ permalink raw reply
* Re: [PATCH v2 03/13] powerpc sstep: Prepare to support prefixed instructions
From: Christophe Leroy @ 2020-02-11 5:57 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev; +Cc: bala24, alistair, mpe, dja
In-Reply-To: <20200211053355.21574-4-jniethe5@gmail.com>
Le 11/02/2020 à 06:33, Jordan Niethe a écrit :
> Currently all instructions are a single word long. A future ISA version
> will include prefixed instructions which have a double word length. The
> functions used for analysing and emulating instructions need to be
> modified so that they can handle these new instruction types.
>
> A prefixed instruction is a word prefix followed by a word suffix. All
> prefixes uniquely have the primary op-code 1. Suffixes may be valid word
> instructions or instructions that only exist as suffixes.
>
> In handling prefixed instructions it will be convenient to treat the
> suffix and prefix as separate words. To facilitate this modify
> analyse_instr() and emulate_step() to take a suffix as a
> parameter. For word instructions it does not matter what is passed in
> here - it will be ignored.
>
> We also define a new flag, PREFIXED, to be used in instruction_op:type.
> This flag will indicate when emulating an analysed instruction if the
> NIP should be advanced by word length or double word length.
>
> The callers of analyse_instr() and emulate_step() will need their own
> changes to be able to support prefixed instructions. For now modify them
> to pass in 0 as a suffix.
>
> Note that at this point no prefixed instructions are emulated or
> analysed - this is just making it possible to do so.
>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> v2: - Move definition of __get_user_instr() and
> __get_user_instr_inatomic() to "powerpc: Support prefixed instructions
> in alignment handler."
> - Use a macro for returning the length of an op
> - Rename sufx -> suffix
> - Define and use PPC_NO_SUFFIX instead of 0
> ---
> arch/powerpc/include/asm/ppc-opcode.h | 5 +++++
> arch/powerpc/include/asm/sstep.h | 9 ++++++--
> arch/powerpc/kernel/align.c | 2 +-
> arch/powerpc/kernel/hw_breakpoint.c | 4 ++--
> arch/powerpc/kernel/kprobes.c | 2 +-
> arch/powerpc/kernel/mce_power.c | 2 +-
> arch/powerpc/kernel/optprobes.c | 3 ++-
> arch/powerpc/kernel/uprobes.c | 2 +-
> arch/powerpc/kvm/emulate_loadstore.c | 2 +-
> arch/powerpc/lib/sstep.c | 12 ++++++-----
> arch/powerpc/lib/test_emulate_step.c | 30 +++++++++++++--------------
> arch/powerpc/xmon/xmon.c | 5 +++--
> 12 files changed, 46 insertions(+), 32 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h
> index c1df75edde44..72783bc92e50 100644
> --- a/arch/powerpc/include/asm/ppc-opcode.h
> +++ b/arch/powerpc/include/asm/ppc-opcode.h
> @@ -377,6 +377,11 @@
> #define PPC_INST_VCMPEQUD 0x100000c7
> #define PPC_INST_VCMPEQUB 0x10000006
>
> +/* macro to check if a word is a prefix */
> +#define IS_PREFIX(x) (((x) >> 26) == 1)
Can you add an OP_PREFIX in the OP list and use it instead of '1' ?
> +#define PPC_NO_SUFFIX 0
> +#define PPC_INST_LENGTH(x) (IS_PREFIX(x) ? 8 : 4)
> +
> /* macros to insert fields into opcodes */
> #define ___PPC_RA(a) (((a) & 0x1f) << 16)
> #define ___PPC_RB(b) (((b) & 0x1f) << 11)
> diff --git a/arch/powerpc/include/asm/sstep.h b/arch/powerpc/include/asm/sstep.h
> index 769f055509c9..9ea8904a1549 100644
> --- a/arch/powerpc/include/asm/sstep.h
> +++ b/arch/powerpc/include/asm/sstep.h
> @@ -89,11 +89,15 @@ enum instruction_type {
> #define VSX_LDLEFT 4 /* load VSX register from left */
> #define VSX_CHECK_VEC 8 /* check MSR_VEC not MSR_VSX for reg >= 32 */
>
> +/* Prefixed flag, ORed in with type */
> +#define PREFIXED 0x800
> +
> /* Size field in type word */
> #define SIZE(n) ((n) << 12)
> #define GETSIZE(w) ((w) >> 12)
>
> #define GETTYPE(t) ((t) & INSTR_TYPE_MASK)
> +#define OP_LENGTH(t) (((t) & PREFIXED) ? 8 : 4)
Is it worth naming it OP_LENGTH ? Can't it be mistaken as one of the
OP_xxx from the list in asm/opcode.h ?
What about GETLENGTH() instead to be consistant with the above lines ?
Christophe
^ permalink raw reply
* [PATCH v2 13/13] powerpc: Add prefix support to mce_find_instr_ea_and_pfn()
From: Jordan Niethe @ 2020-02-11 5:33 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, mpe, Jordan Niethe, dja, bala24
In-Reply-To: <20200211053355.21574-1-jniethe5@gmail.com>
mce_find_instr_ea_and_pfn analyses an instruction to determine the
effective address that caused the machine check. Update this to load and
pass the suffix to analyse_instr for prefixed instructions.
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
v2: - Rename sufx to suffix
---
arch/powerpc/kernel/mce_power.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/mce_power.c b/arch/powerpc/kernel/mce_power.c
index 824eda536f5d..091bab4a5464 100644
--- a/arch/powerpc/kernel/mce_power.c
+++ b/arch/powerpc/kernel/mce_power.c
@@ -365,7 +365,7 @@ static int mce_find_instr_ea_and_phys(struct pt_regs *regs, uint64_t *addr,
* in real-mode is tricky and can lead to recursive
* faults
*/
- int instr;
+ int instr, suffix = 0;
unsigned long pfn, instr_addr;
struct instruction_op op;
struct pt_regs tmp = *regs;
@@ -374,7 +374,9 @@ static int mce_find_instr_ea_and_phys(struct pt_regs *regs, uint64_t *addr,
if (pfn != ULONG_MAX) {
instr_addr = (pfn << PAGE_SHIFT) + (regs->nip & ~PAGE_MASK);
instr = *(unsigned int *)(instr_addr);
- if (!analyse_instr(&op, &tmp, instr, PPC_NO_SUFFIX)) {
+ if (IS_PREFIX(instr))
+ suffix = *(unsigned int *)(instr_addr + 4);
+ if (!analyse_instr(&op, &tmp, instr, suffix)) {
pfn = addr_to_pfn(regs, op.ea);
*addr = op.ea;
*phys_addr = (pfn << PAGE_SHIFT);
--
2.17.1
^ permalink raw reply related
* [PATCH v2 12/13] powerpc/hw_breakpoints: Initial support for prefixed instructions
From: Jordan Niethe @ 2020-02-11 5:33 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, mpe, Jordan Niethe, dja, bala24
In-Reply-To: <20200211053355.21574-1-jniethe5@gmail.com>
Currently when getting an instruction to emulate in
hw_breakpoint_handler() we do not load the suffix of a prefixed
instruction. Ensure we load the suffix if the instruction we need to
emulate is a prefixed instruction.
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
v2: Rename sufx to suffix
---
arch/powerpc/kernel/hw_breakpoint.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index 3a7ec6760dab..c69189641b05 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -243,15 +243,15 @@ dar_range_overlaps(unsigned long dar, int size, struct arch_hw_breakpoint *info)
static bool stepping_handler(struct pt_regs *regs, struct perf_event *bp,
struct arch_hw_breakpoint *info)
{
- unsigned int instr = 0;
+ unsigned int instr = 0, suffix = 0;
int ret, type, size;
struct instruction_op op;
unsigned long addr = info->address;
- if (__get_user_inatomic(instr, (unsigned int *)regs->nip))
+ if (__get_user_instr_inatomic(instr, suffix, (unsigned int *)regs->nip))
goto fail;
- ret = analyse_instr(&op, regs, instr, PPC_NO_SUFFIX);
+ ret = analyse_instr(&op, regs, instr, suffix);
type = GETTYPE(op.type);
size = GETSIZE(op.type);
@@ -275,7 +275,7 @@ static bool stepping_handler(struct pt_regs *regs, struct perf_event *bp,
return false;
}
- if (!emulate_step(regs, instr, PPC_NO_SUFFIX))
+ if (!emulate_step(regs, instr, suffix))
goto fail;
return true;
--
2.17.1
^ permalink raw reply related
* [PATCH v2 11/13] powerpc/uprobes: Add support for prefixed instructions
From: Jordan Niethe @ 2020-02-11 5:33 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, mpe, Jordan Niethe, dja, bala24
In-Reply-To: <20200211053355.21574-1-jniethe5@gmail.com>
Uprobes can execute instructions out of line. Increase the size of the
buffer used for this so that this works for prefixed instructions. Take
into account the length of prefixed instructions when fixing up the nip.
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
v2: - Fix typo
- Use macro for instruction length
---
arch/powerpc/include/asm/uprobes.h | 16 ++++++++++++----
arch/powerpc/kernel/uprobes.c | 4 ++--
2 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/include/asm/uprobes.h b/arch/powerpc/include/asm/uprobes.h
index 2bbdf27d09b5..5516ab27db47 100644
--- a/arch/powerpc/include/asm/uprobes.h
+++ b/arch/powerpc/include/asm/uprobes.h
@@ -14,18 +14,26 @@
typedef ppc_opcode_t uprobe_opcode_t;
+/*
+ * Ensure we have enough space for prefixed instructions, which
+ * are double the size of a word instruction, i.e. 8 bytes.
+ */
#define MAX_UINSN_BYTES 4
-#define UPROBE_XOL_SLOT_BYTES (MAX_UINSN_BYTES)
+#define UPROBE_XOL_SLOT_BYTES (2 * MAX_UINSN_BYTES)
/* The following alias is needed for reference from arch-agnostic code */
#define UPROBE_SWBP_INSN BREAKPOINT_INSTRUCTION
#define UPROBE_SWBP_INSN_SIZE 4 /* swbp insn size in bytes */
struct arch_uprobe {
+ /*
+ * Ensure there is enough space for prefixed instructions. Prefixed
+ * instructions must not cross 64-byte boundaries.
+ */
union {
- u32 insn;
- u32 ixol;
- };
+ uprobe_opcode_t insn[2];
+ uprobe_opcode_t ixol[2];
+ } __aligned(64);
};
struct arch_uprobe_task {
diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c
index 4ab40c4b576f..7e0334ad5cfe 100644
--- a/arch/powerpc/kernel/uprobes.c
+++ b/arch/powerpc/kernel/uprobes.c
@@ -111,7 +111,7 @@ int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
* support doesn't exist and have to fix-up the next instruction
* to be executed.
*/
- regs->nip = utask->vaddr + MAX_UINSN_BYTES;
+ regs->nip = utask->vaddr + PPC_INST_LENGTH(auprobe->insn[0]);
user_disable_single_step(current);
return 0;
@@ -173,7 +173,7 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
* emulate_step() returns 1 if the insn was successfully emulated.
* For all other cases, we need to single-step in hardware.
*/
- ret = emulate_step(regs, auprobe->insn, PPC_NO_SUFFIX);
+ ret = emulate_step(regs, auprobe->insn[0], auprobe->insn[1]);
if (ret > 0)
return true;
--
2.17.1
^ permalink raw reply related
* [PATCH v2 10/13] powerpc/kprobes: Support kprobes on prefixed instructions
From: Jordan Niethe @ 2020-02-11 5:33 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, mpe, Jordan Niethe, dja, bala24
In-Reply-To: <20200211053355.21574-1-jniethe5@gmail.com>
A prefixed instruction is composed of a word prefix followed by a word
suffix. It does not make sense to be able to have a kprobe on the suffix
of a prefixed instruction, so make this impossible.
Kprobes work by replacing an instruction with a trap and saving that
instruction to be single stepped out of place later. Currently there is
not enough space allocated to keep a prefixed instruction for single
stepping. Increase the amount of space allocated for holding the
instruction copy.
kprobe_post_handler() expects all instructions to be 4 bytes long which
means that it does not function correctly for prefixed instructions.
Add checks for prefixed instructions which will use a length of 8 bytes
instead.
For optprobes we normally patch in loading the instruction we put a
probe on into r4 before calling emulate_step(). We now make space and
patch in loading the suffix into r5 as well.
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
arch/powerpc/include/asm/kprobes.h | 5 +--
arch/powerpc/kernel/kprobes.c | 47 +++++++++++++++++++++-------
arch/powerpc/kernel/optprobes.c | 32 ++++++++++---------
arch/powerpc/kernel/optprobes_head.S | 6 ++++
4 files changed, 63 insertions(+), 27 deletions(-)
diff --git a/arch/powerpc/include/asm/kprobes.h b/arch/powerpc/include/asm/kprobes.h
index 66b3f2983b22..0d44ce8a3163 100644
--- a/arch/powerpc/include/asm/kprobes.h
+++ b/arch/powerpc/include/asm/kprobes.h
@@ -38,12 +38,13 @@ extern kprobe_opcode_t optprobe_template_entry[];
extern kprobe_opcode_t optprobe_template_op_address[];
extern kprobe_opcode_t optprobe_template_call_handler[];
extern kprobe_opcode_t optprobe_template_insn[];
+extern kprobe_opcode_t optprobe_template_suffix[];
extern kprobe_opcode_t optprobe_template_call_emulate[];
extern kprobe_opcode_t optprobe_template_ret[];
extern kprobe_opcode_t optprobe_template_end[];
-/* Fixed instruction size for powerpc */
-#define MAX_INSN_SIZE 1
+/* Prefixed instructions are two words */
+#define MAX_INSN_SIZE 2
#define MAX_OPTIMIZED_LENGTH sizeof(kprobe_opcode_t) /* 4 bytes */
#define MAX_OPTINSN_SIZE (optprobe_template_end - optprobe_template_entry)
#define RELATIVEJUMP_SIZE sizeof(kprobe_opcode_t) /* 4 bytes */
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 24a56f062d9e..b061deba4fe7 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -104,17 +104,30 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
int arch_prepare_kprobe(struct kprobe *p)
{
+ int len;
int ret = 0;
+ struct kprobe *prev;
kprobe_opcode_t insn = *p->addr;
+ kprobe_opcode_t prefix = *(p->addr - 1);
+ preempt_disable();
if ((unsigned long)p->addr & 0x03) {
printk("Attempt to register kprobe at an unaligned address\n");
ret = -EINVAL;
} else if (IS_MTMSRD(insn) || IS_RFID(insn) || IS_RFI(insn)) {
printk("Cannot register a kprobe on rfi/rfid or mtmsr[d]\n");
ret = -EINVAL;
+ } else if (IS_PREFIX(prefix)) {
+ printk("Cannot register a kprobe on the second word of prefixed instruction\n");
+ ret = -EINVAL;
+ }
+ prev = get_kprobe(p->addr - 1);
+ if (prev && IS_PREFIX(*prev->ainsn.insn)) {
+ printk("Cannot register a kprobe on the second word of prefixed instruction\n");
+ ret = -EINVAL;
}
+
/* insn must be on a special executable page on ppc64. This is
* not explicitly required on ppc32 (right now), but it doesn't hurt */
if (!ret) {
@@ -124,14 +137,18 @@ int arch_prepare_kprobe(struct kprobe *p)
}
if (!ret) {
- memcpy(p->ainsn.insn, p->addr,
- MAX_INSN_SIZE * sizeof(kprobe_opcode_t));
+ if (IS_PREFIX(insn))
+ len = MAX_INSN_SIZE * sizeof(kprobe_opcode_t);
+ else
+ len = sizeof(kprobe_opcode_t);
+ memcpy(p->ainsn.insn, p->addr, len);
p->opcode = *p->addr;
flush_icache_range((unsigned long)p->ainsn.insn,
(unsigned long)p->ainsn.insn + sizeof(kprobe_opcode_t));
}
p->ainsn.boostable = 0;
+ preempt_enable_no_resched();
return ret;
}
NOKPROBE_SYMBOL(arch_prepare_kprobe);
@@ -216,10 +233,11 @@ NOKPROBE_SYMBOL(arch_prepare_kretprobe);
static int try_to_emulate(struct kprobe *p, struct pt_regs *regs)
{
int ret;
- unsigned int insn = *p->ainsn.insn;
+ unsigned int insn = p->ainsn.insn[0];
+ unsigned int suffix = p->ainsn.insn[1];
/* regs->nip is also adjusted if emulate_step returns 1 */
- ret = emulate_step(regs, insn, PPC_NO_SUFFIX);
+ ret = emulate_step(regs, insn, suffix);
if (ret > 0) {
/*
* Once this instruction has been boosted
@@ -233,7 +251,11 @@ static int try_to_emulate(struct kprobe *p, struct pt_regs *regs)
* So, we should never get here... but, its still
* good to catch them, just in case...
*/
- printk("Can't step on instruction %x\n", insn);
+ if (!IS_PREFIX(insn))
+ printk("Can't step on instruction %x\n", insn);
+ else
+ printk("Can't step on instruction %x %x\n", insn,
+ suffix);
BUG();
} else {
/*
@@ -275,7 +297,7 @@ int kprobe_handler(struct pt_regs *regs)
if (kprobe_running()) {
p = get_kprobe(addr);
if (p) {
- kprobe_opcode_t insn = *p->ainsn.insn;
+ kprobe_opcode_t insn = p->ainsn.insn[0];
if (kcb->kprobe_status == KPROBE_HIT_SS &&
is_trap(insn)) {
/* Turn off 'trace' bits */
@@ -448,9 +470,10 @@ static int trampoline_probe_handler(struct kprobe *p, struct pt_regs *regs)
* the link register properly so that the subsequent 'blr' in
* kretprobe_trampoline jumps back to the right instruction.
*
- * For nip, we should set the address to the previous instruction since
- * we end up emulating it in kprobe_handler(), which increments the nip
- * again.
+ * To keep the nip at the correct address we need to counter the
+ * increment that happens when we emulate the kretprobe_trampoline noop
+ * in kprobe_handler(). We do this by decrementing the address by the
+ * length of the noop which is always 4 bytes.
*/
regs->nip = orig_ret_address - 4;
regs->link = orig_ret_address;
@@ -478,12 +501,14 @@ int kprobe_post_handler(struct pt_regs *regs)
{
struct kprobe *cur = kprobe_running();
struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
+ kprobe_opcode_t insn;
if (!cur || user_mode(regs))
return 0;
+ insn = *cur->ainsn.insn;
/* make sure we got here for instruction we have a kprobe on */
- if (((unsigned long)cur->ainsn.insn + 4) != regs->nip)
+ if ((unsigned long)cur->ainsn.insn + PPC_INST_LENGTH(insn) != regs->nip)
return 0;
if ((kcb->kprobe_status != KPROBE_REENTER) && cur->post_handler) {
@@ -492,7 +517,7 @@ int kprobe_post_handler(struct pt_regs *regs)
}
/* Adjust nip to after the single-stepped instruction */
- regs->nip = (unsigned long)cur->addr + 4;
+ regs->nip = (unsigned long)cur->addr + PPC_INST_LENGTH(insn);
regs->msr |= kcb->kprobe_saved_msr;
/*Restore back the original saved kprobes variables and continue. */
diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
index f908d9422557..60cf8e8485ab 100644
--- a/arch/powerpc/kernel/optprobes.c
+++ b/arch/powerpc/kernel/optprobes.c
@@ -27,6 +27,8 @@
(optprobe_template_op_address - optprobe_template_entry)
#define TMPL_INSN_IDX \
(optprobe_template_insn - optprobe_template_entry)
+#define TMPL_SUFX_IDX \
+ (optprobe_template_suffix - optprobe_template_entry)
#define TMPL_END_IDX \
(optprobe_template_end - optprobe_template_entry)
@@ -100,8 +102,8 @@ static unsigned long can_optimize(struct kprobe *p)
* and that can be emulated.
*/
if (!is_conditional_branch(*p->ainsn.insn) &&
- analyse_instr(&op, ®s, *p->ainsn.insn,
- PPC_NO_SUFFIX) == 1) {
+ analyse_instr(&op, ®s, p->ainsn.insn[0],
+ p->ainsn.insn[1]) == 1) {
emulate_update_regs(®s, &op);
nip = regs.nip;
}
@@ -141,27 +143,27 @@ void arch_remove_optimized_kprobe(struct optimized_kprobe *op)
}
/*
- * emulate_step() requires insn to be emulated as
- * second parameter. Load register 'r4' with the
- * instruction.
+ * emulate_step() requires insn to be emulated as second parameter, and the
+ * suffix as the third parameter. Load these into registers.
*/
-void patch_imm32_load_insns(unsigned int val, kprobe_opcode_t *addr)
+static void patch_imm32_load_insns(int reg, unsigned int val,
+ kprobe_opcode_t *addr)
{
- /* addis r4,0,(insn)@h */
- patch_instruction(addr, PPC_INST_ADDIS | ___PPC_RT(4) |
+ /* addis reg,0,(insn)@h */
+ patch_instruction(addr, PPC_INST_ADDIS | ___PPC_RT(reg) |
((val >> 16) & 0xffff));
addr++;
- /* ori r4,r4,(insn)@l */
- patch_instruction(addr, PPC_INST_ORI | ___PPC_RA(4) |
- ___PPC_RS(4) | (val & 0xffff));
+ /* ori reg,reg,(insn)@l */
+ patch_instruction(addr, PPC_INST_ORI | ___PPC_RA(reg) |
+ ___PPC_RS(reg) | (val & 0xffff));
}
/*
* Generate instructions to load provided immediate 64-bit value
* to register 'r3' and patch these instructions at 'addr'.
*/
-void patch_imm64_load_insns(unsigned long val, kprobe_opcode_t *addr)
+static void patch_imm64_load_insns(unsigned long val, kprobe_opcode_t *addr)
{
/* lis r3,(op)@highest */
patch_instruction(addr, PPC_INST_ADDIS | ___PPC_RT(3) |
@@ -267,9 +269,11 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
patch_instruction(buff + TMPL_EMULATE_IDX, branch_emulate_step);
/*
- * 3. load instruction to be emulated into relevant register, and
+ * 3. load instruction and suffix to be emulated into the relevant
+ * registers, and
*/
- patch_imm32_load_insns(*p->ainsn.insn, buff + TMPL_INSN_IDX);
+ patch_imm32_load_insns(4, p->ainsn.insn[0], buff + TMPL_INSN_IDX);
+ patch_imm32_load_insns(5, p->ainsn.insn[1], buff + TMPL_SUFX_IDX);
/*
* 4. branch back from trampoline
diff --git a/arch/powerpc/kernel/optprobes_head.S b/arch/powerpc/kernel/optprobes_head.S
index cf383520843f..395d1643f59d 100644
--- a/arch/powerpc/kernel/optprobes_head.S
+++ b/arch/powerpc/kernel/optprobes_head.S
@@ -95,6 +95,12 @@ optprobe_template_insn:
nop
nop
+ .global optprobe_template_suffix
+optprobe_template_suffix:
+ /* Pass suffix to be emulated in r5 */
+ nop
+ nop
+
.global optprobe_template_call_emulate
optprobe_template_call_emulate:
/* Branch to emulate_step() */
--
2.17.1
^ permalink raw reply related
* [PATCH v2 09/13] powerpc/xmon: Dump prefixed instructions
From: Jordan Niethe @ 2020-02-11 5:33 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, mpe, Jordan Niethe, dja, bala24
In-Reply-To: <20200211053355.21574-1-jniethe5@gmail.com>
Currently when xmon is dumping instructions it reads a word at a time
and then prints that instruction (either as a hex number or by
disassembling it). For prefixed instructions it would be nice to show
its prefix and suffix as together. Use read_instr() so that if a prefix
is encountered its suffix is loaded too. Then print these in the form:
prefix:suffix
Xmon uses the disassembly routines from GNU binutils. These currently do
not support prefixed instructions so we will not disassemble the
prefixed instructions yet.
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
v2: Rename sufx to suffix
---
arch/powerpc/xmon/xmon.c | 50 +++++++++++++++++++++++++++++++---------
1 file changed, 39 insertions(+), 11 deletions(-)
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 0b085642bbe7..513901ee18b0 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -2903,6 +2903,21 @@ prdump(unsigned long adrs, long ndump)
}
}
+static bool instrs_are_equal(unsigned long insta, unsigned long suffixa,
+ unsigned long instb, unsigned long suffixb)
+{
+ if (insta != instb)
+ return false;
+
+ if (!IS_PREFIX(insta) && !IS_PREFIX(instb))
+ return true;
+
+ if (IS_PREFIX(insta) && IS_PREFIX(instb))
+ return suffixa == suffixb;
+
+ return false;
+}
+
typedef int (*instruction_dump_func)(unsigned long inst, unsigned long addr);
static int
@@ -2911,12 +2926,11 @@ generic_inst_dump(unsigned long adr, long count, int praddr,
{
int nr, dotted;
unsigned long first_adr;
- unsigned int inst, last_inst = 0;
- unsigned char val[4];
+ unsigned int inst, suffix, last_inst = 0, last_suffix = 0;
dotted = 0;
- for (first_adr = adr; count > 0; --count, adr += 4) {
- nr = mread(adr, val, 4);
+ for (first_adr = adr; count > 0; --count, adr += nr) {
+ nr = read_instr(adr, &inst, &suffix);
if (nr == 0) {
if (praddr) {
const char *x = fault_chars[fault_type];
@@ -2924,8 +2938,9 @@ generic_inst_dump(unsigned long adr, long count, int praddr,
}
break;
}
- inst = GETWORD(val);
- if (adr > first_adr && inst == last_inst) {
+ if (adr > first_adr && instrs_are_equal(inst, suffix,
+ last_inst,
+ last_suffix)) {
if (!dotted) {
printf(" ...\n");
dotted = 1;
@@ -2934,11 +2949,24 @@ generic_inst_dump(unsigned long adr, long count, int praddr,
}
dotted = 0;
last_inst = inst;
- if (praddr)
- printf(REG" %.8x", adr, inst);
- printf("\t");
- dump_func(inst, adr);
- printf("\n");
+ last_suffix = suffix;
+ if (IS_PREFIX(inst)) {
+ if (praddr)
+ printf(REG" %.8x:%.8x", adr, inst, suffix);
+ printf("\t");
+ /*
+ * Just use this until binutils ppc disassembly
+ * prints prefixed instructions.
+ */
+ printf("%.8x:%.8x", inst, suffix);
+ printf("\n");
+ } else {
+ if (praddr)
+ printf(REG" %.8x", adr, inst);
+ printf("\t");
+ dump_func(inst, adr);
+ printf("\n");
+ }
}
return adr - first_adr;
}
--
2.17.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox