linuxppc-dev.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
From: Nicholas Piggin <npiggin@gmail.com>
To: Christophe Leroy <christophe.leroy@csgroup.eu>,
	linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH v5 02/21] powerpc/64s: move the last of the page fault handling logic to C
Date: Fri, 15 Jan 2021 10:25:03 +1000	[thread overview]
Message-ID: <1610669980.jgo71amlnt.astroid@bobo.none> (raw)
In-Reply-To: <afa9875c-e8ed-de3e-d395-435be86bd958@csgroup.eu>

Excerpts from Christophe Leroy's message of January 14, 2021 11:28 pm:
> 
> 
> Le 14/01/2021 à 14:17, Nicholas Piggin a écrit :
>> Excerpts from Christophe Leroy's message of January 14, 2021 10:25 pm:
>>>
>>>
>>> Le 14/01/2021 à 13:09, Nicholas Piggin a écrit :
>>>> Excerpts from Nicholas Piggin's message of January 14, 2021 1:24 pm:
>>>>> Excerpts from Christophe Leroy's message of January 14, 2021 12:12 am:
>>>>>>
>>>>>>
>>>>>> Le 13/01/2021 à 08:31, Nicholas Piggin a écrit :
>>>>>>> The page fault handling still has some complex logic particularly around
>>>>>>> hash table handling, in asm. Implement this in C instead.
>>>>>>>
>>>>>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>>>>> ---
>>>>>>>     arch/powerpc/include/asm/book3s/64/mmu-hash.h |   1 +
>>>>>>>     arch/powerpc/kernel/exceptions-64s.S          | 131 +++---------------
>>>>>>>     arch/powerpc/mm/book3s64/hash_utils.c         |  77 ++++++----
>>>>>>>     arch/powerpc/mm/fault.c                       |  46 ++++--
>>>>>>>     4 files changed, 107 insertions(+), 148 deletions(-)
>>>>>>>
>>>>>>> diff --git a/arch/powerpc/include/asm/book3s/64/mmu-hash.h b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
>>>>>>> index 066b1d34c7bc..60a669379aa0 100644
>>>>>>> --- a/arch/powerpc/include/asm/book3s/64/mmu-hash.h
>>>>>>> +++ b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
>>>>>>> @@ -454,6 +454,7 @@ static inline unsigned long hpt_hash(unsigned long vpn,
>>>>>>>     #define HPTE_NOHPTE_UPDATE	0x2
>>>>>>>     #define HPTE_USE_KERNEL_KEY	0x4
>>>>>>>     
>>>>>>> +int do_hash_fault(struct pt_regs *regs, unsigned long ea, unsigned long dsisr);
>>>>>>>     extern int __hash_page_4K(unsigned long ea, unsigned long access,
>>>>>>>     			  unsigned long vsid, pte_t *ptep, unsigned long trap,
>>>>>>>     			  unsigned long flags, int ssize, int subpage_prot);
>>>>>>> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
>>>>>>> index 6e53f7638737..bcb5e81d2088 100644
>>>>>>> --- a/arch/powerpc/kernel/exceptions-64s.S
>>>>>>> +++ b/arch/powerpc/kernel/exceptions-64s.S
>>>>>>> @@ -1401,14 +1401,15 @@ END_FTR_SECTION_IFSET(CPU_FTR_HVMODE)
>>>>>>>      *
>>>>>>>      * Handling:
>>>>>>>      * - Hash MMU
>>>>>>> - *   Go to do_hash_page first to see if the HPT can be filled from an entry in
>>>>>>> - *   the Linux page table. Hash faults can hit in kernel mode in a fairly
>>>>>>> + *   Go to do_hash_fault, which attempts to fill the HPT from an entry in the
>>>>>>> + *   Linux page table. Hash faults can hit in kernel mode in a fairly
>>>>>>>      *   arbitrary state (e.g., interrupts disabled, locks held) when accessing
>>>>>>>      *   "non-bolted" regions, e.g., vmalloc space. However these should always be
>>>>>>> - *   backed by Linux page tables.
>>>>>>> + *   backed by Linux page table entries.
>>>>>>>      *
>>>>>>> - *   If none is found, do a Linux page fault. Linux page faults can happen in
>>>>>>> - *   kernel mode due to user copy operations of course.
>>>>>>> + *   If no entry is found the Linux page fault handler is invoked (by
>>>>>>> + *   do_hash_fault). Linux page faults can happen in kernel mode due to user
>>>>>>> + *   copy operations of course.
>>>>>>>      *
>>>>>>>      *   KVM: The KVM HDSI handler may perform a load with MSR[DR]=1 in guest
>>>>>>>      *   MMU context, which may cause a DSI in the host, which must go to the
>>>>>>> @@ -1439,13 +1440,17 @@ EXC_COMMON_BEGIN(data_access_common)
>>>>>>>     	GEN_COMMON data_access
>>>>>>>     	ld	r4,_DAR(r1)
>>>>>>>     	ld	r5,_DSISR(r1)
>>>>>>
>>>>>> We have DSISR here. I think the dispatch between page fault or do_break() should be done here:
>>>>>> - It would be more similar to other arches
>>>>>
>>>>> Other sub-archs?
>>>>>
>>>>>> - Would avoid doing it also in instruction fault
>>>>>
>>>>> True but it's hidden under an unlikely branch so won't really help
>>>>> instruction fault.
>>>>>
>>>>>> - Would avoid that -1 return which looks more like a hack.
>>>>>
>>>>> I don't really see it as a hack, we return a code to asm caller to
>>>>> direct whether to restore registers or not, we alrady have this
>>>>> pattern.
>>>>>
>>>>> (I'm hoping all that might be go away one day by conrolling NV
>>>>> regs from C if we can get good code generation but even if not we
>>>>> still have it in the interrupt returns).
>>>>>
>>>>> That said I will give it a try here. At very least it might be a
>>>>> better intermediate step.
>>>>
>>>> Ah yes, this way doesn't work well for later patches because you end
>>>> e.g., with the do_break call having to call the interrupt handler
>>>> wrappers again when they actually expect to be in the asm entry state
>>>> (e.g., irq soft-mask state) when called, and return via interrupt_return
>>>> after the exit wrapper runs (which 64s uses to implement better context
>>>> tracking for example).
>>>>
>>>> That could possibly be hacked up to deal with multiple interrupt
>>>> wrappers per interrupt, but I'd rather not go backwards.
>>>>
>>>> That does leave the other sub archs as having this issue, but they don't
>>>> do so much in their handlers. 32 doesn't have soft-mask or context
>>>> tracking to deal with for example. We will need to fix this up though
>>>> and unify things more.
>>>>
>>>
>>> Not sure I understand what you mean exactly.
>>>
>>> On the 8xx, do_break() is called by totally different exceptions:
>>> - Exception 0x1c00 Data breakpoint ==> do_break()
>>> - Exception 0x1300 Instruction TLB error ==> handle_page_fault()
>>> - Exception 0x1400 Data TLB error ==> handle_page_fault()
>>>
>>> On book3s/32, we now (after my patch ie patch 1 in your series ) have either do_break() or
>>> handle_page_fault() being called from very early in ASM.
>>>
>>> If you do the same in book3s/64, then there is no issue with interrupt wrappers being called twice,
>>> is it ?
>> 
>> bad_page_fault is the problem, it has to go afterwards.
>> 
>> Once we have the changed 64s behaviour of do_page_fault, I don't know if
>> there is any point leaving do_break in asm is there? I guess it is neat
>> to treat it quite separately, I might need to count fast path branches...
>> I have done the split anyway already, so I will post your way first.
>> 
> 
> As far as I understand, not taken unlikely branches are costless (at least on book3s/32), so you 
> would only suffer the cost of the logical 'and.' on the value of DSISR that you already have in a 
> register. Should be in the noise.
> 
> bad_page_fault() is not in the fast path anymore since we now handle the exception fixup at the end 
> of do_page_fault(). So I think it shouldn't be a concern to call the wrapper again for bad_page_fault()

It's not performance but correctness. For example we can have interrupts 
enabled again at this time, which the interrupt wrapper does not expect.
Or the context tracking code in the entry wrapper would break if it's
called again before interrupt_return.

I think it's not too ugly to put bad_page_fault in C and having do_break 
in asm still avoids a lot of your complaints.

Thanks,
Nick

  reply	other threads:[~2021-01-15  0:27 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-13  7:31 [PATCH v5 00/21] powerpc: interrupt wrappers Nicholas Piggin
2021-01-13  7:31 ` [PATCH v5 01/21] powerpc/32s: Do DABR match out of handle_page_fault() Nicholas Piggin
2021-01-13  7:31 ` [PATCH v5 02/21] powerpc/64s: move the last of the page fault handling logic to C Nicholas Piggin
2021-01-13 14:12   ` Christophe Leroy
2021-01-14  3:24     ` Nicholas Piggin
2021-01-14 12:09       ` Nicholas Piggin
2021-01-14 12:25         ` Christophe Leroy
2021-01-14 13:17           ` Nicholas Piggin
2021-01-14 13:28             ` Christophe Leroy
2021-01-15  0:25               ` Nicholas Piggin [this message]
2021-01-13  7:31 ` [PATCH v5 03/21] powerpc: remove arguments from fault handler functions Nicholas Piggin
2021-01-14 14:12   ` Christophe Leroy
2021-01-13  7:31 ` [PATCH v5 04/21] powerpc: bad_page_fault, do_break get registers from regs Nicholas Piggin
2021-01-13 14:25   ` Christophe Leroy
2021-01-14  3:26     ` Nicholas Piggin
2021-01-13  7:31 ` [PATCH v5 05/21] powerpc/perf: move perf irq/nmi handling details into traps.c Nicholas Piggin
2021-01-13  7:32 ` [PATCH v5 06/21] powerpc: interrupt handler wrapper functions Nicholas Piggin
2021-01-13 14:45   ` Christophe Leroy
2021-01-14  3:41     ` Nicholas Piggin
2021-01-13  7:32 ` [PATCH v5 07/21] powerpc: add interrupt wrapper entry / exit stub functions Nicholas Piggin
2021-01-13  7:32 ` [PATCH v5 08/21] powerpc: add interrupt_cond_local_irq_enable helper Nicholas Piggin
2021-01-13  7:32 ` [PATCH v5 09/21] powerpc/64: context tracking remove _TIF_NOHZ Nicholas Piggin
2021-01-13 14:50   ` Christophe Leroy
2021-01-14  3:48     ` Nicholas Piggin
2021-01-13  7:32 ` [PATCH v5 10/21] powerpc/64s/hash: improve context tracking of hash faults Nicholas Piggin
2021-01-13  7:32 ` [PATCH v5 11/21] powerpc/64: context tracking move to interrupt wrappers Nicholas Piggin
2021-01-13  7:32 ` [PATCH v5 12/21] powerpc/64: add context tracking to asynchronous interrupts Nicholas Piggin
2021-01-13  7:32 ` [PATCH v5 13/21] powerpc: handle irq_enter/irq_exit in interrupt handler wrappers Nicholas Piggin
2021-01-13  7:32 ` [PATCH v5 14/21] powerpc/64s: move context tracking exit to interrupt exit path Nicholas Piggin
2021-01-13  7:32 ` [PATCH v5 15/21] powerpc/64s: reconcile interrupts in C Nicholas Piggin
2021-01-13 14:54   ` Christophe Leroy
2021-01-14  3:51     ` Nicholas Piggin
2021-01-13  7:32 ` [PATCH v5 16/21] powerpc/64: move account_stolen_time into its own function Nicholas Piggin
2021-01-13 14:59   ` Christophe Leroy
2021-01-14  3:51     ` Nicholas Piggin
2021-01-13  7:32 ` [PATCH v5 17/21] powerpc/64: entry cpu time accounting in C Nicholas Piggin
2021-01-13 15:05   ` Christophe Leroy
2021-01-14  3:58     ` Nicholas Piggin
2021-01-13  7:32 ` [PATCH v5 18/21] powerpc: move NMI entry/exit code into wrapper Nicholas Piggin
2021-01-13 15:13   ` Christophe Leroy
2021-01-14  4:00     ` Nicholas Piggin
2021-01-13  7:32 ` [PATCH v5 19/21] powerpc/64s: move NMI soft-mask handling to C Nicholas Piggin
2021-01-13  7:32 ` [PATCH v5 20/21] powerpc/64s: runlatch interrupt handling in C Nicholas Piggin
2021-01-13  7:32 ` [PATCH v5 21/21] powerpc/64s: power4 nap fixup " Nicholas Piggin

Reply instructions:

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

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

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

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

  git send-email \
    --in-reply-to=1610669980.jgo71amlnt.astroid@bobo.none \
    --to=npiggin@gmail.com \
    --cc=christophe.leroy@csgroup.eu \
    --cc=linuxppc-dev@lists.ozlabs.org \
    /path/to/YOUR_REPLY

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

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).