* [PATCH V3 04/10] arm64: exception: handle Synchronous External Abort
From: Baicar, Tyler @ 2016-10-13 13:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <87y41tsboh.fsf@e105922-lin.cambridge.arm.com>
Hello Punit,
On 10/12/2016 11:46 AM, Punit Agrawal wrote:
> Hi Tyler,
>
> A couple of hopefully not bike shedding comments below.
>
> Tyler Baicar <tbaicar@codeaurora.org> writes:
>
>> SEA exceptions are often caused by an uncorrected hardware
>> error, and are handled when data abort and instruction abort
>> exception classes have specific values for their Fault Status
>> Code.
>> When SEA occurs, before killing the process, go through
>> the handlers registered in the notification list.
>> Update fault_info[] with specific SEA faults so that the
>> new SEA handler is used.
>>
>> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
>> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
>> Signed-off-by: Naveen Kaje <nkaje@codeaurora.org>
>> ---
>> arch/arm64/include/asm/system_misc.h | 13 ++++++++
>> arch/arm64/mm/fault.c | 58 +++++++++++++++++++++++++++++-------
>> 2 files changed, 61 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/system_misc.h b/arch/arm64/include/asm/system_misc.h
>> index 57f110b..90daf4a 100644
>> --- a/arch/arm64/include/asm/system_misc.h
>> +++ b/arch/arm64/include/asm/system_misc.h
>> @@ -64,4 +64,17 @@ extern void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
>>
>> #endif /* __ASSEMBLY__ */
>>
>> +/*
>> + * The functions below are used to register and unregister callbacks
>> + * that are to be invoked when a Synchronous External Abort (SEA)
>> + * occurs. An SEA is raised by certain fault status codes that have
>> + * either data or instruction abort as the exception class, and
>> + * callbacks may be registered to parse or handle such hardware errors.
>> + *
>> + * Registered callbacks are run in an interrupt/atomic context. They
>> + * are not allowed to block or sleep.
>> + */
>> +int sea_register_handler_chain(struct notifier_block *nb);
>> +void sea_unregister_handler_chain(struct notifier_block *nb);
>> +
>> #endif /* __ASM_SYSTEM_MISC_H */
>> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
>> index 05d2bd7..81cb7ad 100644
>> --- a/arch/arm64/mm/fault.c
>> +++ b/arch/arm64/mm/fault.c
>> @@ -39,6 +39,22 @@
>> #include <asm/pgtable.h>
>> #include <asm/tlbflush.h>
>>
>> +/*
>> + * GHES SEA handler code may register a notifier call here to
>> + * handle HW error record passed from platform.
>> + */
>> +static ATOMIC_NOTIFIER_HEAD(sea_handler_chain);
>> +
>> +int sea_register_handler_chain(struct notifier_block *nb)
>> +{
>> + return atomic_notifier_chain_register(&sea_handler_chain, nb);
>> +}
>> +
>> +void sea_unregister_handler_chain(struct notifier_block *nb)
>> +{
>> + atomic_notifier_chain_unregister(&sea_handler_chain, nb);
>> +}
>> +
> What do you think of naming the above functions as
> [un]register_synchonous_ext_abort_notifier?
>
> For an API, I find "sea" doesn't quite convey the message.
>
> One more comment below.
Yes, those names seem easier to understand.
>> static const char *fault_name(unsigned int esr);
>>
>> #ifdef CONFIG_KPROBES
>> @@ -480,6 +496,28 @@ static int do_bad(unsigned long addr, unsigned int esr, struct pt_regs *regs)
>> return 1;
>> }
>>
>> +/*
>> + * This abort handler deals with Synchronous External Abort.
>> + * It calls notifiers, and then returns "fault".
>> + */
>> +static int do_sea(unsigned long addr, unsigned int esr, struct pt_regs *regs)
>> +{
>> + struct siginfo info;
>> +
>> + atomic_notifier_call_chain(&sea_handler_chain, 0, NULL);
>> +
>> + pr_err("Synchronous External Abort: %s (0x%08x) at 0x%016lx\n",
>> + fault_name(esr), esr, addr);
>> +
>> + info.si_signo = SIGBUS;
>> + info.si_errno = 0;
>> + info.si_code = 0;
>> + info.si_addr = (void __user *)addr;
>> + arm64_notify_die("", regs, &info, esr);
>> +
>> + return 0;
>> +}
>> +
>> static const struct fault_info {
>> int (*fn)(unsigned long addr, unsigned int esr, struct pt_regs *regs);
>> int sig;
>> @@ -502,22 +540,22 @@ static const struct fault_info {
>> { do_page_fault, SIGSEGV, SEGV_ACCERR, "level 1 permission fault" },
>> { do_page_fault, SIGSEGV, SEGV_ACCERR, "level 2 permission fault" },
>> { do_page_fault, SIGSEGV, SEGV_ACCERR, "level 3 permission fault" },
>> - { do_bad, SIGBUS, 0, "synchronous external abort" },
>> + { do_sea, SIGBUS, 0, "synchronous external abort" },
>> { do_bad, SIGBUS, 0, "unknown 17" },
>> { do_bad, SIGBUS, 0, "unknown 18" },
>> { do_bad, SIGBUS, 0, "unknown 19" },
>> - { do_bad, SIGBUS, 0, "synchronous abort (translation table walk)" },
>> - { do_bad, SIGBUS, 0, "synchronous abort (translation table walk)" },
>> - { do_bad, SIGBUS, 0, "synchronous abort (translation table walk)" },
>> - { do_bad, SIGBUS, 0, "synchronous abort (translation table walk)" },
>> - { do_bad, SIGBUS, 0, "synchronous parity error" },
>> + { do_sea, SIGBUS, 0, "level 0 SEA (trans tbl walk)" },
>> + { do_sea, SIGBUS, 0, "level 1 SEA (trans tbl walk)" },
>> + { do_sea, SIGBUS, 0, "level 2 SEA (trans tbl walk)" },
>> + { do_sea, SIGBUS, 0, "level 3 SEA (trans tbl walk)" },
> ^^^
> The comment about naming applies here as well.
>
> Thanks,
> Punit
I'll expand sea here as well. This should make it easier to understand
without knowing the code.
Thanks,
Tyler
>
> [...]
>
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
^ permalink raw reply
* [PATCH V3 05/10] acpi: apei: handle SEA notification type for ARMv8
From: Baicar, Tyler @ 2016-10-13 14:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <87shs1sb1b.fsf@e105922-lin.cambridge.arm.com>
Hello Punit,
On 10/12/2016 12:00 PM, Punit Agrawal wrote:
> Tyler Baicar <tbaicar@codeaurora.org> writes:
>
>> ARM APEI extension proposal added SEA (Synchrounous External
>> Abort) notification type for ARMv8.
>> Add a new GHES error source handling function for SEA. If an error
>> source's notification type is SEA, then this function can be registered
>> into the SEA exception handler. That way GHES will parse and report
>> SEA exceptions when they occur.
>>
>> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
>> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
>> Signed-off-by: Naveen Kaje <nkaje@codeaurora.org>
> This patch fails to apply for me on v4.8. Is there a different tree this
> is based on?
This patch was giving me some grief as well. I'm not sure why that is
because this patchset was based on the 4.8 kernel with the dependent
patch for initial APEI support.
> One comment below.
>
> [...]
>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index c8488f1..28d5a09 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -50,6 +50,10 @@
>> #include <acpi/apei.h>
>> #include <asm/tlbflush.h>
>>
>> +#ifdef CONFIG_HAVE_ACPI_APEI_SEA
>> +#include <asm/system_misc.h>
>> +#endif
>> +
>> #include "apei-internal.h"
>>
>> #define GHES_PFX "GHES: "
>> @@ -779,6 +783,62 @@ static struct notifier_block ghes_notifier_sci = {
>> .notifier_call = ghes_notify_sci,
>> };
>>
>> +#ifdef CONFIG_HAVE_ACPI_APEI_SEA
>> +static LIST_HEAD(ghes_sea);
>> +
>> +static int ghes_notify_sea(struct notifier_block *this,
>> + unsigned long event, void *data)
>> +{
>> + struct ghes *ghes;
>> + int ret = NOTIFY_DONE;
>> +
>> + rcu_read_lock();
>> + list_for_each_entry_rcu(ghes, &ghes_sea, list) {
>> + if (!ghes_proc(ghes))
>> + ret = NOTIFY_OK;
> Not something you've introduced but it looks like ghes_proc erroneously
> never returns anything other than 0. I plan to post the below fix to
> address it.
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 60746ef..caea575 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -662,7 +662,7 @@ static int ghes_proc(struct ghes *ghes)
> ghes_do_proc(ghes, ghes->estatus);
> out:
> ghes_clear_estatus(ghes);
> - return 0;
> + return rc;
> }
Yes, this definitely should be fixed :)
Thanks,
Tyler
>> + }
>> + rcu_read_unlock();
>> +
>> + return ret;
>> +}
>> +
> [...]
>
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
^ permalink raw reply
* [PATCH 3/4] arm64: Allow hw watchpoint of length 3,5,6 and 7
From: Pavel Labath @ 2016-10-13 14:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1bc0dce6-eb26-dbf5-dd0c-d36055f5f1fe@redhat.com>
On 13 October 2016 at 11:22, Pratyush Anand <panand@redhat.com> wrote:
>
>
> On Wednesday 12 October 2016 04:46 PM, Yao Qi wrote:
>>
>> On Wed, Oct 12, 2016 at 6:58 AM, Pratyush Anand <panand@redhat.com> wrote:
>>>
>>> Since, arm64 can support all offset within a double word limit.
>>> Therefore,
>>> now support other lengths within that range as well.
>>
>>
>> How does ptracer (like GDB) detect kernel has already supported all byte
>> address select values? I suppose ptrace(NT_ARM_HW_WATCH, ) with
>> len is 3 or 5 fail on current kernel but is of success after your patches
>> applied.
>>
>
> Thanks for testing these patches.
>
> I do not know if we can know that other than the failure of
> ptrace(PTRACE_SETREGSET, .., NT_ARM_HW_WATCH, ..). I do not see any such
> option in `man ptrace`.
That's how I intend to implement support for this in LLDB.
if (setExactWatchpoint())
return true; // watchpoint set, new kernel
if (setApproxWatchpoint())
return true; // watchpoint set, old kernel
return false; // watchpoints not working
seems straight-forward enough to me.
^ permalink raw reply
* [PATCH v3 1/5] arm64: mm: BUG on unsupported manipulations of live kernel mappings
From: Catalin Marinas @ 2016-10-13 14:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAKv+Gu8p06DBaDgmJ3KSSfNVDox=wOTav48kti+Y0gYtQhEC_g@mail.gmail.com>
On Thu, Oct 13, 2016 at 01:25:53PM +0100, Ard Biesheuvel wrote:
> On 12 October 2016 at 16:04, Catalin Marinas <catalin.marinas@arm.com> wrote:
> > On Wed, Oct 12, 2016 at 12:23:41PM +0100, Ard Biesheuvel wrote:
> >> +/*
> >> + * The following mapping attributes may be updated in live
> >> + * kernel mappings without the need for break-before-make.
> >> + */
> >> +static const pteval_t modifiable_attr_mask = PTE_PXN | PTE_RDONLY | PTE_WRITE;
> >> +
> >> static void alloc_init_pte(pmd_t *pmd, unsigned long addr,
> >> unsigned long end, unsigned long pfn,
> >> pgprot_t prot,
> >> @@ -115,8 +119,18 @@ static void alloc_init_pte(pmd_t *pmd, unsigned long addr,
> >>
> >> pte = pte_set_fixmap_offset(pmd, addr);
> >> do {
> >> + pte_t old_pte = *pte;
> >> +
> >> set_pte(pte, pfn_pte(pfn, prot));
> >> pfn++;
> >> +
> >> + /*
> >> + * After the PTE entry has been populated once, we
> >> + * only allow updates to the permission attributes.
> >> + */
> >> + BUG_ON(pte_val(old_pte) != 0 &&
> >> + ((pte_val(old_pte) ^ pte_val(*pte)) &
> >> + ~modifiable_attr_mask) != 0);
> >
> > Please turn this check into a single macro. You have it in three places
> > already (though with different types but a macro would do). Something
> > like below (feel free to come up with a better macro name):
> >
> > BUG_ON(!safe_pgattr_change(old_pte, *pte));
>
> Something like below? With that, I can also fold the PMD and PUD
> versions of the BUG() together.
(fixing up alignment to make it readable)
> """
> /*
> * Returns whether updating a live page table entry is safe:
> * - if the old and new values are identical,
> * - if an invalid mapping is turned into a valid one (or vice versa),
> * - if the entry is a block or page mapping, and the old and new values
> * only differ in the PXN/RDONLY/WRITE bits.
> *
> * NOTE: 'safe' does not imply that no TLB maintenance is required, it only
> * means that no TLB conflicts should occur as a result of the update.
> */
> #define __set_pgattr_is_safe(type, old, new, blocktype) \
> (type ## _val(old) == type ## _val(new) || \
> ((type ## _val(old) ^ type ## _val(new)) & PTE_VALID) != 0 || \
> (((type ## _val(old) & PTE_TYPE_MASK) == blocktype) && \
> (((type ## _val(old) ^ type ## _val(new)) & \
> ~(PTE_PXN | PTE_RDONLY | PTE_WRITE)) == 0)))
>
> static inline bool set_live_pte_is_safe(pte_t old, pte_t new)
> {
> return __set_pgattr_is_safe(pte, old, new, PTE_TYPE_PAGE);
> }
>
> static inline bool set_live_pmd_is_safe(pmd_t old, pmd_t new)
> {
> return __set_pgattr_is_safe(pmd, old, new, PMD_TYPE_SECT);
> }
>
> static inline bool set_live_pud_is_safe(pud_t old, pud_t new)
> {
> return __set_pgattr_is_safe(pud, old, new, PUD_TYPE_SECT);
> }
The set_ prefix is slightly confusing as it suggests (to me) having a
side effect. Maybe pgattr_set_is_safe()?
But it looks like we make it more complicated needed by using pte_t
instead of pteval_t as argument. How about just using the pteval_t as
argument (and it's fine to call it with pmdval_t, pudval_t as well):
#define pgattr_set_is_safe(oldval, newval) \
...
--
Catalin
^ permalink raw reply
* [PATCH v3 1/5] arm64: mm: BUG on unsupported manipulations of live kernel mappings
From: Ard Biesheuvel @ 2016-10-13 14:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161013144450.GG21592@e104818-lin.cambridge.arm.com>
On 13 October 2016 at 15:44, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Thu, Oct 13, 2016 at 01:25:53PM +0100, Ard Biesheuvel wrote:
>
> (fixing up alignment to make it readable)
>
I apologise on Gmail's behalf
>> """
>> /*
>> * Returns whether updating a live page table entry is safe:
>> * - if the old and new values are identical,
>> * - if an invalid mapping is turned into a valid one (or vice versa),
>> * - if the entry is a block or page mapping, and the old and new values
>> * only differ in the PXN/RDONLY/WRITE bits.
>> *
>> * NOTE: 'safe' does not imply that no TLB maintenance is required, it only
>> * means that no TLB conflicts should occur as a result of the update.
>> */
>> #define __set_pgattr_is_safe(type, old, new, blocktype) \
>> (type ## _val(old) == type ## _val(new) || \
>> ((type ## _val(old) ^ type ## _val(new)) & PTE_VALID) != 0 || \
>> (((type ## _val(old) & PTE_TYPE_MASK) == blocktype) && \
>> (((type ## _val(old) ^ type ## _val(new)) & \
>> ~(PTE_PXN | PTE_RDONLY | PTE_WRITE)) == 0)))
>>
>> static inline bool set_live_pte_is_safe(pte_t old, pte_t new)
>> {
>> return __set_pgattr_is_safe(pte, old, new, PTE_TYPE_PAGE);
>> }
>>
>> static inline bool set_live_pmd_is_safe(pmd_t old, pmd_t new)
>> {
>> return __set_pgattr_is_safe(pmd, old, new, PMD_TYPE_SECT);
>> }
>>
>> static inline bool set_live_pud_is_safe(pud_t old, pud_t new)
>> {
>> return __set_pgattr_is_safe(pud, old, new, PUD_TYPE_SECT);
>> }
>
> The set_ prefix is slightly confusing as it suggests (to me) having a
> side effect. Maybe pgattr_set_is_safe()?
>
> But it looks like we make it more complicated needed by using pte_t
> instead of pteval_t as argument. How about just using the pteval_t as
> argument (and it's fine to call it with pmdval_t, pudval_t as well):
>
> #define pgattr_set_is_safe(oldval, newval) \
> ...
>
Well, the only problem there is that the permission bit check should
only apply to level 3 page mappings (bit[1] == 1) and level 1/2 block
mappings (bit[1] == 0), so we would still need two versions
^ permalink raw reply
* [PATCH 2/2] power/reset: at91-poweroff: timely shitdown LPDDR memories
From: Jean-Jacques Hiblot @ 2016-10-13 15:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161013134710.rbvsiyu6ala673g4@piout.net>
2016-10-13 15:47 GMT+02:00 Alexandre Belloni
<alexandre.belloni@free-electrons.com>:
> On 13/10/2016 at 14:27:15 +0200, Jean-Jacques Hiblot wrote :
>> 2016-10-13 13:03 GMT+02:00 Alexandre Belloni
>> <alexandre.belloni@free-electrons.com>:
>> > On 12/10/2016 at 14:48:27 +0200, Jean-Jacques Hiblot wrote :
>> >> > +static void at91_lpddr_poweroff(void)
>> >> > +{
>> >> > + asm volatile(
>> >> > + /* Align to cache lines */
>> >> > + ".balign 32\n\t"
>> >> > +
>> >> > + " ldr r6, [%2, #" __stringify(AT91_SHDW_CR) "]\n\t"
>> >> At first sight, it looks useless. I assume it's used to preload the
>> >> TLB before the LPDDR is turned off.
>> >> A comment to explain why this line is useful would prevent its removal.
>> >
>> > Yes, this is the case. I can add a comment.
>> >
>> > Anyway, I would prefer the whole thing to run from SRAM, as a PIE
>> > instead of relying on the cache.
>>
>> Instead of copying into the SRAM, you can make the cache reliable by
>> preloading it, much like the TLB.
>> LDI is probably not available for most of atmel's SOC, so the only way
>> I can think of, is to execute code from the targeted area. here is an
>> example:
>> + /*
>> + * Jump to the end of the sequence to preload instruction cache
>> + * It only works because the sequence is short enough not to
>> + * sit accross more than 2 cache lines
>> + */
>> + " b end_of_sequence\n\t"
>> + "start_of_sequence:\n\t"
>> +
>> /* Power down SDRAM0 */
>> " str %1, [%0, #"
>> __stringify(AT91_DDRSDRC_LPR) "]\n\t"
>> /* Shutdown CPU */
>> " str %3, [%2, #" __stringify(AT91_SHDW_CR) "]\n\t"
>>
>> " b .\n\t"
>> +
>> + /*
>> + * we're now 100% sure that the code to shutdown the LPDDR and
>> + * the CPU is in cache, go back to do the actual job
>> + */
>> + "end_of_sequence:\n\t"
>> + " b start_of_sequence\n\t"
>> :
>>
>
> I don't think this is necessary. By aligning the instructions properly,
> we are already sure the whole code is loaded into the cache.
right I didn't see the align directive.
>
> My plan is to get rid of the assembly and use PIE so it is written in C
> and we can properly separate the RAM stuff in the ddrc driver.
>
> The mpddrc driver could load its shutdown function in SRAM. The reset
> controller driver would load the reset function in SRAM and the shutdown
> controller would load the poweroff function in SRAM. It would e quite
> cleaner than what we have here.
>
> --
> Alexandre Belloni, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
^ permalink raw reply
* [PATCH] perf: xgene: Remove bogus IS_ERR() check
From: Tai Tri Nguyen @ 2016-10-13 16:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161013102939.GA22356@remoulade>
Hi Mark,
On Thu, Oct 13, 2016 at 3:29 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Oct 12, 2016 at 09:39:27AM -0700, Tai Nguyen wrote:
>> This patch fixes the warning issue with static checker.
>> The bug is reported in [1]
>>
>> [1] https://www.spinics.net/lists/arm-kernel/msg535957.html
>>
>> Signed-off-by: Tai Nguyen <ttnguyen@apm.com>
>
> Please put the problem description in the commit message:
>
> In acpi_get_pmu_hw_inf we pass the address of a local variable to IS_ERR(),
> which doesn't make sense, as the pointer must be a real, valid pointer. This
> doesn't cause a functional problem, as IS_ERR() will evaluate as false, but
> the check is bogus and causes static checkers to complain.
>
> Remove the bogus check.
>
> Please also add a Reported-by for Dan.
>
> The patch itself is fine, so with the above, you can add:
>
> Acked-by: Mark Rutland <mark.rutland@arm.com>
>
> Please Cc Will Deacon when sending that, I expect that he will pick it up
> (though perhaps not until rc1 given this is not a critical issue).
>
Thanks. I'll send the updated patch shortly.
Regards,
Tai
[...]
>> ---
>> drivers/perf/xgene_pmu.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/perf/xgene_pmu.c b/drivers/perf/xgene_pmu.c
>> index c2ac764..a8ac4bc 100644
>> --- a/drivers/perf/xgene_pmu.c
>> +++ b/drivers/perf/xgene_pmu.c
>> @@ -1011,7 +1011,7 @@ xgene_pmu_dev_ctx *acpi_get_pmu_hw_inf(struct xgene_pmu *xgene_pmu,
>> rc = acpi_dev_get_resources(adev, &resource_list,
>> acpi_pmu_dev_add_resource, &res);
>> acpi_dev_free_resource_list(&resource_list);
>> - if (rc < 0 || IS_ERR(&res)) {
>> + if (rc < 0) {
>> dev_err(dev, "PMU type %d: No resource address found\n", type);
>> goto err;
>> }
>> --
>> 1.9.1
>>
--
Tai
^ permalink raw reply
* [PATCH v3 3/5] arm64: mm: set the contiguous bit for kernel mappings where appropriate
From: Catalin Marinas @ 2016-10-13 16:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1476271425-19401-4-git-send-email-ard.biesheuvel@linaro.org>
On Wed, Oct 12, 2016 at 12:23:43PM +0100, Ard Biesheuvel wrote:
> Now that we no longer allow live kernel PMDs to be split, it is safe to
> start using the contiguous bit for kernel mappings. So set the contiguous
> bit in the kernel page mappings for regions whose size and alignment are
> suitable for this.
>
> This enables the following contiguous range sizes for the virtual mapping
> of the kernel image, and for the linear mapping:
>
> granule size | cont PTE | cont PMD |
> -------------+------------+------------+
> 4 KB | 64 KB | 32 MB |
> 16 KB | 2 MB | 1 GB* |
> 64 KB | 2 MB | 16 GB* |
>
> * only when built for 3 or more levels of translation
I assume the limitation to have contiguous PMD only with 3 or move
levels is because of the way p*d folding was implemented in the kernel.
With nopmd, looping over pmds is done in __create_pgd_mapping() rather
than alloc_init_pmd().
A potential solution would be to replicate the contiguous pmd code to
the pud and pgd level, though we probably won't benefit from any
contiguous entries at higher level (when more than 2 levels).
Alternatively, with an #ifdef __PGTABLE_PMD_FOLDED, we could set the
PMD_CONT in prot in __create_pgd_mapping() directly (if the right
addr/phys alignment).
Anyway, it's probably not worth the effort given that 42-bit VA with 64K
pages is becoming a less likely configuration (36-bit VA with 16K pages
is even less likely, also depending on EXPERT).
--
Catalin
^ permalink raw reply
* [PATCH v5 01/14] drivers: iommu: add FWNODE_IOMMU fwnode type
From: Lorenzo Pieralisi @ 2016-10-13 16:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAJZ5v0hK2Ryo32u4S9K=78-Oot13vvVNB+p6N2YC1UMqYW9g7A@mail.gmail.com>
Hi Rafael,
On Fri, Sep 30, 2016 at 05:48:01PM +0200, Rafael J. Wysocki wrote:
> On Fri, Sep 30, 2016 at 11:07 AM, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> > On Thu, Sep 29, 2016 at 10:59:40PM +0200, Rafael J. Wysocki wrote:
> >> On Thursday, September 29, 2016 03:15:20 PM Lorenzo Pieralisi wrote:
> >> > Hi Rafael,
> >> >
> >> > On Fri, Sep 09, 2016 at 03:23:30PM +0100, Lorenzo Pieralisi wrote:
> >> > > On systems booting with a device tree, every struct device is
> >> > > associated with a struct device_node, that represents its DT
> >> > > representation. The device node can be used in generic kernel
> >> > > contexts (eg IRQ translation, IOMMU streamid mapping), to
> >> > > retrieve the properties associated with the device and carry
> >> > > out kernel operation accordingly. Owing to the 1:1 relationship
> >> > > between the device and its device_node, the device_node can also
> >> > > be used as a look-up token for the device (eg looking up a device
> >> > > through its device_node), to retrieve the device in kernel paths
> >> > > where the device_node is available.
> >> > >
> >> > > On systems booting with ACPI, the same abstraction provided by
> >> > > the device_node is required to provide look-up functionality.
> >> > >
> >> > > Therefore, mirroring the approach implemented in the IRQ domain
> >> > > kernel layer, this patch adds an additional fwnode type FWNODE_IOMMU.
> >> > >
> >> > > This patch also implements a glue kernel layer that allows to
> >> > > allocate/free FWNODE_IOMMU fwnode_handle structures and associate
> >> > > them with IOMMU devices.
> >> > >
> >> > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> >> > > Reviewed-by: Hanjun Guo <hanjun.guo@linaro.org>
> >> > > Cc: Joerg Roedel <joro@8bytes.org>
> >> > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> >> > > ---
> >> > > include/linux/fwnode.h | 1 +
> >> > > include/linux/iommu.h | 25 +++++++++++++++++++++++++
> >> > > 2 files changed, 26 insertions(+)
> >> > >
> >> > > diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> >> > > index 8516717..6e10050 100644
> >> > > --- a/include/linux/fwnode.h
> >> > > +++ b/include/linux/fwnode.h
> >> > > @@ -19,6 +19,7 @@ enum fwnode_type {
> >> > > FWNODE_ACPI_DATA,
> >> > > FWNODE_PDATA,
> >> > > FWNODE_IRQCHIP,
> >> > > + FWNODE_IOMMU,
> >> >
> >> > This patch provides groundwork for this series and it is key for
> >> > the rest of it, basically the point here is that we need a fwnode
> >> > to differentiate platform devices created out of static ACPI tables
> >> > entries (ie IORT), that represent IOMMU components.
> >> >
> >> > The corresponding device is not an ACPI device (I could fabricate one as
> >> > it is done for other static tables entries eg FADT power button, but I
> >> > do not necessarily see the reason for doing that given that all we need
> >> > the fwnode for is a token identifier), so FWNODE_ACPI does not apply
> >> > here.
> >> >
> >> > Please let me know if it is reasonable how I sorted this out (it
> >> > is basically identical to IRQCHIP, just another enum entry), the
> >> > remainder of the code depends on this.
> >>
> >> I'm not familiar with the use case, so I don't see anything unreasonable
> >> in it.
> >
> > The use case is pretty simple: on ARM SMMU devices are platform devices.
> > When booting with DT they are identified through an of_node and related
> > FWNODE_OF type. When booting with ACPI, the ARM SMMU platform devices,
> > to be equivalent to DT booting path, should be created out of static
> > IORT table entries (that's how we describe SMMUs); we need to create
> > a fwnode "token" to associate with those platform devices and that's
> > not a FWNODE_ACPI (that is for an ACPI device firmware object, here we
> > really do not need one), so this patch.
> >
> >> If you're asking about whether or not I mind adding more fwnode types in
> >> principle, then no, I don't. :-)
> >
> > Yes, that's what I was asking, the only point that bugs me is that for
> > both FWNODE_IRQCHIP and FWNODE_IOMMU the fwnode is just a "token" (ie a
> > valid pointer) used for look-up and the type in the fwnode_handle is
> > mostly there for error checking, I was wondering if we could create a
> > specific fwnode_type for this specific usage (eg FWNODE_TAG and then add
> > a type to it as part of its container struct) instead of adding an enum
> > value per subsystem - it seems there are other fwnode types in the
> > pipeline :), so I am asking:
> >
> > lkml.kernel.org/r/3D1468514043-21081-3-git-send-email-minyard at acm.org
>
> OK, I see your concern now, so thanks for presenting it so clearly.
>
> While I don't see anything wrong with having per-subsystem fwnode
> types in principle, I agree that if the only purpose of them is to
> mean "this comes from ACPI, but from a static table, not from the
> namespace", it would be better to have a single fwnode type for that,
> like FWNODE_ACPI_STATIC or similar.
Coming back to this, I updated the series with new fwnode type
FWNODE_ACPI_STATIC, which IMHO makes more sense (because that
represents the FW interface it was obtained from rather than
its content and plays better with upcoming extension above - DMI is a
different firmware interface so it will be represented with a different
fwnode type). Thanks.
However, I still have a question. The approach I took (create platform
devices out of static IORT table entries for SMMUs) is common in ACPI
(eg GHES, ACPI watchdog) even though those subsystems ignore the fwnode,
but I think that's a detail.
Still, fixed HW like power button and sleep button took a different
approach, which consists in creating struct acpi_device objects out
of FADT fixed HW features (with a NULL ACPI handle because there is
no real ACPI object in the namespace for them).
I would like to understand the reasoning behind the difference (I
think it is related to notification events and the need for an
ACPI object for them - and sysfs userspace HID IF exposure ?).
In theory (but looks crazy to me that's why I did not do it), I could
fabricate a Device object in the ACPI namespace (?) (with _HID, _CRS and
related properties - is that doable ?) out of the static table entry in
the IORT table that provides the ARM SMMU component data (ie its MMIO
space, IRQs and SMMU properties like cache coherency), this would
allow the kernel to create a struct acpi_device (and related fwnode)
+ its physical node platform device but that looks insanely complicated
(if feasible and more importantly if correct from an ACPI standpoint).
This approach would allow me to match the SMMU driver with an _HID,
the kernel would create a physical_node (ie platform_device) for
me out of the namespace ACPI device object and I would get the
FWNODE_ACPI for free (out of the struct acpi_device) instead of having
to fiddle about with a new fwnode_handle type.
I think this alternative approach (if doable at all) creates more issues
than it solves but I wanted to make sure what I am doing is kosher
from an ACPI perspective so I am asking.
I definitely think the current approach I took is much better, with its
own downsides (eg matching the ARM SMMU driver by name instead of
acpi_device_id/_HID), but I wanted to ask.
The point is: ARM SMMU drivers are platform drivers. In DT the SMMUs
are represented through DT nodes, in ACPI through _static_ IORT table
entries, somehow a platform device must be created for them, so
this whole series (and related fwnode issues).
Thanks,
Lorenzo
^ permalink raw reply
* [PATCH] arm64: kaslr: fix breakage with CONFIG_MODVERSIONS=y
From: Ard Biesheuvel @ 2016-10-13 16:42 UTC (permalink / raw)
To: linux-arm-kernel
As it turns out, the KASLR code breaks CONFIG_MODVERSIONS, since the
kcrctab has an absolute address field that is relocated at runtime
when the kernel offset is randomized.
This has been fixed already for PowerPC in the past, so simply wire up
the existing code dealing with this issue.
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
Given that I spotted this when trying to boot a distro kernel, this probably
deserves a cc stable.
Timur, could you please test this and report back? Thanks.
arch/arm64/include/asm/module.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h
index e12af6754634..06ff7fd9e81f 100644
--- a/arch/arm64/include/asm/module.h
+++ b/arch/arm64/include/asm/module.h
@@ -17,6 +17,7 @@
#define __ASM_MODULE_H
#include <asm-generic/module.h>
+#include <asm/memory.h>
#define MODULE_ARCH_VERMAGIC "aarch64"
@@ -32,6 +33,10 @@ u64 module_emit_plt_entry(struct module *mod, const Elf64_Rela *rela,
Elf64_Sym *sym);
#ifdef CONFIG_RANDOMIZE_BASE
+#ifdef CONFIG_MODVERSIONS
+#define ARCH_RELOCATES_KCRCTAB
+#define reloc_start (kimage_vaddr - KIMAGE_VADDR)
+#endif
extern u64 module_alloc_base;
#else
#define module_alloc_base ((u64)_etext - MODULES_VSIZE)
--
2.7.4
^ permalink raw reply related
* [PATCH v3 1/5] arm64: mm: BUG on unsupported manipulations of live kernel mappings
From: Catalin Marinas @ 2016-10-13 16:51 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAKv+Gu9E27mrC3k37nbGAxo_C6D7_svRFZXXzoP2Rzc+XATm0A@mail.gmail.com>
On Thu, Oct 13, 2016 at 03:48:04PM +0100, Ard Biesheuvel wrote:
> On 13 October 2016 at 15:44, Catalin Marinas <catalin.marinas@arm.com> wrote:
> > On Thu, Oct 13, 2016 at 01:25:53PM +0100, Ard Biesheuvel wrote:
> >
> > (fixing up alignment to make it readable)
> >
>
> I apologise on Gmail's behalf
>
> >> """
> >> /*
> >> * Returns whether updating a live page table entry is safe:
> >> * - if the old and new values are identical,
> >> * - if an invalid mapping is turned into a valid one (or vice versa),
> >> * - if the entry is a block or page mapping, and the old and new values
> >> * only differ in the PXN/RDONLY/WRITE bits.
> >> *
> >> * NOTE: 'safe' does not imply that no TLB maintenance is required, it only
> >> * means that no TLB conflicts should occur as a result of the update.
> >> */
> >> #define __set_pgattr_is_safe(type, old, new, blocktype) \
> >> (type ## _val(old) == type ## _val(new) || \
> >> ((type ## _val(old) ^ type ## _val(new)) & PTE_VALID) != 0 || \
> >> (((type ## _val(old) & PTE_TYPE_MASK) == blocktype) && \
> >> (((type ## _val(old) ^ type ## _val(new)) & \
> >> ~(PTE_PXN | PTE_RDONLY | PTE_WRITE)) == 0)))
> >>
> >> static inline bool set_live_pte_is_safe(pte_t old, pte_t new)
> >> {
> >> return __set_pgattr_is_safe(pte, old, new, PTE_TYPE_PAGE);
> >> }
> >>
> >> static inline bool set_live_pmd_is_safe(pmd_t old, pmd_t new)
> >> {
> >> return __set_pgattr_is_safe(pmd, old, new, PMD_TYPE_SECT);
> >> }
> >>
> >> static inline bool set_live_pud_is_safe(pud_t old, pud_t new)
> >> {
> >> return __set_pgattr_is_safe(pud, old, new, PUD_TYPE_SECT);
> >> }
> >
> > The set_ prefix is slightly confusing as it suggests (to me) having a
> > side effect. Maybe pgattr_set_is_safe()?
> >
> > But it looks like we make it more complicated needed by using pte_t
> > instead of pteval_t as argument. How about just using the pteval_t as
> > argument (and it's fine to call it with pmdval_t, pudval_t as well):
> >
> > #define pgattr_set_is_safe(oldval, newval) \
> > ...
>
> Well, the only problem there is that the permission bit check should
> only apply to level 3 page mappings (bit[1] == 1) and level 1/2 block
> mappings (bit[1] == 0), so we would still need two versions
I was suggesting that you only replace the "... & ~modifiable_attr_mask"
check in your patch to avoid writing it three times. The macro that you
proposed above does more but it is also more unreadable.
--
Catalin
^ permalink raw reply
* [PATCH v3 3/5] arm64: mm: set the contiguous bit for kernel mappings where appropriate
From: Ard Biesheuvel @ 2016-10-13 16:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161013162806.GH21592@e104818-lin.cambridge.arm.com>
On 13 October 2016 at 17:28, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Wed, Oct 12, 2016 at 12:23:43PM +0100, Ard Biesheuvel wrote:
>> Now that we no longer allow live kernel PMDs to be split, it is safe to
>> start using the contiguous bit for kernel mappings. So set the contiguous
>> bit in the kernel page mappings for regions whose size and alignment are
>> suitable for this.
>>
>> This enables the following contiguous range sizes for the virtual mapping
>> of the kernel image, and for the linear mapping:
>>
>> granule size | cont PTE | cont PMD |
>> -------------+------------+------------+
>> 4 KB | 64 KB | 32 MB |
>> 16 KB | 2 MB | 1 GB* |
>> 64 KB | 2 MB | 16 GB* |
>>
>> * only when built for 3 or more levels of translation
>
> I assume the limitation to have contiguous PMD only with 3 or move
> levels is because of the way p*d folding was implemented in the kernel.
> With nopmd, looping over pmds is done in __create_pgd_mapping() rather
> than alloc_init_pmd().
>
> A potential solution would be to replicate the contiguous pmd code to
> the pud and pgd level, though we probably won't benefit from any
> contiguous entries at higher level (when more than 2 levels).
> Alternatively, with an #ifdef __PGTABLE_PMD_FOLDED, we could set the
> PMD_CONT in prot in __create_pgd_mapping() directly (if the right
> addr/phys alignment).
>
Indeed. See the next patch :-)
> Anyway, it's probably not worth the effort given that 42-bit VA with 64K
> pages is becoming a less likely configuration (36-bit VA with 16K pages
> is even less likely, also depending on EXPERT).
>
This is the reason I put it in a separate patch: this one contains the
most useful combinations, and the next patch adds the missing ones,
but clutters up the code significantly. I'm perfectly happy to drop 4
and 5 if you don't think it is worth the trouble.
^ permalink raw reply
* [PATCH v3 1/5] arm64: mm: BUG on unsupported manipulations of live kernel mappings
From: Ard Biesheuvel @ 2016-10-13 16:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161013165112.GI21592@e104818-lin.cambridge.arm.com>
On 13 October 2016 at 17:51, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Thu, Oct 13, 2016 at 03:48:04PM +0100, Ard Biesheuvel wrote:
>> On 13 October 2016 at 15:44, Catalin Marinas <catalin.marinas@arm.com> wrote:
>> > On Thu, Oct 13, 2016 at 01:25:53PM +0100, Ard Biesheuvel wrote:
>> >
>> > (fixing up alignment to make it readable)
>> >
>>
>> I apologise on Gmail's behalf
>>
>> >> """
>> >> /*
>> >> * Returns whether updating a live page table entry is safe:
>> >> * - if the old and new values are identical,
>> >> * - if an invalid mapping is turned into a valid one (or vice versa),
>> >> * - if the entry is a block or page mapping, and the old and new values
>> >> * only differ in the PXN/RDONLY/WRITE bits.
>> >> *
>> >> * NOTE: 'safe' does not imply that no TLB maintenance is required, it only
>> >> * means that no TLB conflicts should occur as a result of the update.
>> >> */
>> >> #define __set_pgattr_is_safe(type, old, new, blocktype) \
>> >> (type ## _val(old) == type ## _val(new) || \
>> >> ((type ## _val(old) ^ type ## _val(new)) & PTE_VALID) != 0 || \
>> >> (((type ## _val(old) & PTE_TYPE_MASK) == blocktype) && \
>> >> (((type ## _val(old) ^ type ## _val(new)) & \
>> >> ~(PTE_PXN | PTE_RDONLY | PTE_WRITE)) == 0)))
>> >>
>> >> static inline bool set_live_pte_is_safe(pte_t old, pte_t new)
>> >> {
>> >> return __set_pgattr_is_safe(pte, old, new, PTE_TYPE_PAGE);
>> >> }
>> >>
>> >> static inline bool set_live_pmd_is_safe(pmd_t old, pmd_t new)
>> >> {
>> >> return __set_pgattr_is_safe(pmd, old, new, PMD_TYPE_SECT);
>> >> }
>> >>
>> >> static inline bool set_live_pud_is_safe(pud_t old, pud_t new)
>> >> {
>> >> return __set_pgattr_is_safe(pud, old, new, PUD_TYPE_SECT);
>> >> }
>> >
>> > The set_ prefix is slightly confusing as it suggests (to me) having a
>> > side effect. Maybe pgattr_set_is_safe()?
>> >
>> > But it looks like we make it more complicated needed by using pte_t
>> > instead of pteval_t as argument. How about just using the pteval_t as
>> > argument (and it's fine to call it with pmdval_t, pudval_t as well):
>> >
>> > #define pgattr_set_is_safe(oldval, newval) \
>> > ...
>>
>> Well, the only problem there is that the permission bit check should
>> only apply to level 3 page mappings (bit[1] == 1) and level 1/2 block
>> mappings (bit[1] == 0), so we would still need two versions
>
> I was suggesting that you only replace the "... & ~modifiable_attr_mask"
> check in your patch to avoid writing it three times. The macro that you
> proposed above does more but it is also more unreadable.
>
Ah ok, fair enough
^ permalink raw reply
* [PATCH 2/3] arm64: hw_breakpoint: Handle inexact watchpoint addresses
From: Pavel Labath @ 2016-10-13 17:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAHB_GupRgSgnQS+YKhKxWhf8+t+GP5+7_hoigKUQaYcnhwG8NA@mail.gmail.com>
On 13 October 2016 at 10:58, Pratyush Anand <panand@redhat.com> wrote:
> Hi Pavel,
>
> Thanks a lot for your testing.
>
> On Wed, Oct 12, 2016 at 7:20 PM, Pavel Labath <labath@google.com> wrote:
>> Hello Pratyush,
>>
>> I am sorry about the delay. I have finally had a chance to try out
>> your changes today. Response inline.
>>
>> On 8 October 2016 at 06:10, Pratyush Anand <panand@redhat.com> wrote:
>>> On Fri, Oct 7, 2016 at 10:54 PM, Pavel Labath <labath@google.com> wrote:
>>>> The thing is, I have observed different behavior here depending on the
>>>> exact hardware used. I don't have the exact parameters with me now,
>>>> but I can look it up next week.
>>>>
>>>> The thing is that the spec is imprecise about what exact address the
>>>> hardware can report for the watchpoint hit. I presume that is
>>>> deliberate to give some leeway to implementers. The spec says the
>>>> address can be anywhere in the range from the lowest memory address
>>>> accessed by the instruction to the highest address watched by the
>>>> watchpoint,
>>>
>>> I think, my patches should be able to take care of the above condition.
>> Unfortunately, the patch does not solve the problem for my hardware,
>> because of the leeway you give in watchpoint_handler is not big
>> enough. It does work however, if I change the line
>>> if (addr + 7 < val + lens || addr > val + lene)
>> to
>>> if (addr + 15 < val + lens || addr > val + lene)
>
> Yes, I missed that floating point register will be of size 16.
>
>> I do not think we can assume that address reported by the hardware
>> will be at most 7 bytes off from the address we put the watchpoint at.
>> There is nothing in the spec that guarantees that, and it does not
>> seem to be enough for some hardware. In fact, I am not sure we can
>> assume 15 is enough either, but maybe it can do for now, until we can
>
> Right. It might even be bigger, in case of cache maintenance instructions.
>
>> find hardware that does not work with that (I haven't yet tried what
>> happens it the watchpoint is triggered by cache management
>> instructions, which can access much larger blocks of memory).
>
> Not, sure, may be it can lie in cache line size range.
>
>>
>> For reference, the hardware in question is:
>>> Processor : AArch64 Processor rev 0 (aarch64)
>>> processor : 0
>>> min_vddcx : 400000
>>> min_vddmx : 490000
>>> BogoMIPS : 38.00
>>> Features : fp asimd evtstrm aes pmull sha1 sha2 crc32
>>> CPU implementer : 0x51
>>> CPU architecture: 8
>>> CPU variant : 0x2
>>> CPU part : 0x201
>>> CPU revision : 0
>>> CPU param : 300 468 468 621 939 299 445 445 621 1077
>>> Hardware : Qualcomm Technologies, Inc MSM8996pro
>>
>> And this is how it behaves:
>> The output of the test app triggering the watchpoint (I have set a
>> single-byte watchpoint at 555556705f)
>>>
>>> Writing to: 555556705f, size: 1
>>> Writing to: 555556705e, size: 2
>>> Writing to: 555556705c, size: 4
>>> Writing to: 5555567058, size: 8
>>> Writing to: 5555567050, size: 16
>>> Writing to: 5555567040, size: 32
>>
>> The addresses received by the kernel:
>> [ 251.812166] c1 3780 hw-breakpoint: watchpoint_handler: addr:
>> 555556705f, val+lens: 555556705f, val+lene: 555556705f
>> [ 251.820341] c1 3781 hw-breakpoint: watchpoint_handler: addr:
>> 555556705e, val+lens: 555556705f, val+lene: 555556705f
>> [ 251.825572] c0 3782 hw-breakpoint: watchpoint_handler: addr:
>> 555556705c, val+lens: 555556705f, val+lene: 555556705f
>> [ 251.831085] c0 3783 hw-breakpoint: watchpoint_handler: addr:
>> 5555567058, val+lens: 555556705f, val+lene: 555556705f
>> [ 251.835804] c0 3784 hw-breakpoint: watchpoint_handler: addr:
>> 5555567050, val+lens: 555556705f, val+lene: 555556705f
>> [ 251.841350] c0 3785 hw-breakpoint: watchpoint_handler: addr:
>> 5555567050, val+lens: 555556705f, val+lene: 555556705f
>>
>> Note that for the case of 16 and 32-byte access it returns the address
>> 5555567050 -- this is why the "+15" is sufficient for me.
>>
>>
>> The other thing I am not so sure about in your patch is that it has
>> potential to mis-attribute the watchpoint hit if we have two
>> watchpoints close together. For example, if I have first watchpoint on
>> 0x1008-0x100f and a second one on 0x1000-0x1007, *and* the application
>> writes one byte to 0x1004, then your code will still attribute the hit
>> to the first watchpoint, even though it was not really triggered. This
>
> Hummm..yes, thanks for pointing it out.
> There could be only two solutions for it:
> (1) We read instruction at the location regs->pc and analyse it and
> find the size of read/write.
> or(2) What you have suggested in your patch.
>
> I think, its easier to go with your implementation. So, I have taken
> your patch and updated my perf/upstream_arm64_devel branch. May be you
> can give it a test for your test cases.
I've checked out the new version of your branch, and it works great.
I'll write a patch with additional test cases to go on top of your
branch, as the tests there do not capture the bug I was fixing.
regards,
Pavel
^ permalink raw reply
* [PATCH v3 3/5] arm64: mm: set the contiguous bit for kernel mappings where appropriate
From: Catalin Marinas @ 2016-10-13 17:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAKv+Gu9WqUvtHRJvgWhsvYwFOgho97EaLV4qojaa0fjGpJT3WQ@mail.gmail.com>
On Thu, Oct 13, 2016 at 05:57:33PM +0100, Ard Biesheuvel wrote:
> On 13 October 2016 at 17:28, Catalin Marinas <catalin.marinas@arm.com> wrote:
> > On Wed, Oct 12, 2016 at 12:23:43PM +0100, Ard Biesheuvel wrote:
> >> Now that we no longer allow live kernel PMDs to be split, it is safe to
> >> start using the contiguous bit for kernel mappings. So set the contiguous
> >> bit in the kernel page mappings for regions whose size and alignment are
> >> suitable for this.
> >>
> >> This enables the following contiguous range sizes for the virtual mapping
> >> of the kernel image, and for the linear mapping:
> >>
> >> granule size | cont PTE | cont PMD |
> >> -------------+------------+------------+
> >> 4 KB | 64 KB | 32 MB |
> >> 16 KB | 2 MB | 1 GB* |
> >> 64 KB | 2 MB | 16 GB* |
> >>
> >> * only when built for 3 or more levels of translation
> >
> > I assume the limitation to have contiguous PMD only with 3 or move
> > levels is because of the way p*d folding was implemented in the kernel.
> > With nopmd, looping over pmds is done in __create_pgd_mapping() rather
> > than alloc_init_pmd().
> >
> > A potential solution would be to replicate the contiguous pmd code to
> > the pud and pgd level, though we probably won't benefit from any
> > contiguous entries at higher level (when more than 2 levels).
> > Alternatively, with an #ifdef __PGTABLE_PMD_FOLDED, we could set the
> > PMD_CONT in prot in __create_pgd_mapping() directly (if the right
> > addr/phys alignment).
>
> Indeed. See the next patch :-)
I got there eventually ;).
> > Anyway, it's probably not worth the effort given that 42-bit VA with 64K
> > pages is becoming a less likely configuration (36-bit VA with 16K pages
> > is even less likely, also depending on EXPERT).
>
> This is the reason I put it in a separate patch: this one contains the
> most useful combinations, and the next patch adds the missing ones,
> but clutters up the code significantly. I'm perfectly happy to drop 4
> and 5 if you don't think it is worth the trouble.
I'll have a look at patch 4 first.
Both 64KB contiguous pmd and 4K contiguous pud give us a 16GB range
which (AFAIK) is less likely to be optimised in hardware.
--
Catalin
^ permalink raw reply
* [PATCH v2] perf: xgene: Remove bogus IS_ERR() check
From: Tai Nguyen @ 2016-10-13 18:09 UTC (permalink / raw)
To: linux-arm-kernel
In acpi_get_pmu_hw_inf we pass the address of a local variable to IS_ERR(),
which doesn't make sense, as the pointer must be a real, valid pointer.
This doesn't cause a functional problem, as IS_ERR() will evaluate as
false, but the check is bogus and causes static checkers to complain.
Remove the bogus check.
The bug is reported by Dan Carpenter <dan.carpenter@oracle.com> in [1]
[1] https://www.spinics.net/lists/arm-kernel/msg535957.html
Signed-off-by: Tai Nguyen <ttnguyen@apm.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
---
v2:
Add more problem description in the commit message
Add Acked-by: Mark Rutland <mark.rutland@arm.com>
drivers/perf/xgene_pmu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/perf/xgene_pmu.c b/drivers/perf/xgene_pmu.c
index c2ac764..a8ac4bc 100644
--- a/drivers/perf/xgene_pmu.c
+++ b/drivers/perf/xgene_pmu.c
@@ -1011,7 +1011,7 @@ xgene_pmu_dev_ctx *acpi_get_pmu_hw_inf(struct xgene_pmu *xgene_pmu,
rc = acpi_dev_get_resources(adev, &resource_list,
acpi_pmu_dev_add_resource, &res);
acpi_dev_free_resource_list(&resource_list);
- if (rc < 0 || IS_ERR(&res)) {
+ if (rc < 0) {
dev_err(dev, "PMU type %d: No resource address found\n", type);
goto err;
}
--
1.9.1
^ permalink raw reply related
* [PATCH V2 1/3] Revert "ACPI,PCI,IRQ: reduce static IRQ array size to 16"
From: Bjorn Helgaas @ 2016-10-13 18:15 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <09f93442-89c8-7b2c-467d-d29b857739ff@codeaurora.org>
On Wed, Oct 12, 2016 at 06:46:11PM -0400, Sinan Kaya wrote:
> Hi Bjorn,
>
> On 10/12/2016 6:13 PM, Bjorn Helgaas wrote:
> > Hi Sinan,
> >
> > I have to apologize because I haven't followed all the discussion and
> > now I'm trying to figure it out from the patches and changelogs. But
> > I guess that's not all bad, because future interested folks *should*
> > be able to figure things out from that :)
>
> Sure, np. I figured you are busy with the new baseline. Then, I saw a
> series of patches coming from you.
>
> >
> > On Tue, Oct 04, 2016 at 05:15:17PM -0400, Sinan Kaya wrote:
> >> This reverts commit 5c5087a55390 ("ACPI,PCI,IRQ: reduce static IRQ array
> >> size to 16").
> >>
> >> The code maintains a fixed size array for IRQ penalties. The array
> >> gets updated by external calls such as acpi_penalize_sci_irq,
> >> acpi_penalize_isa_irq to reflect the actual interrupt usage of the
> >> system. Since the IRQ distribution is platform specific, this is
> >> not known ahead of time. The IRQs get updated based on the SCI
> >> interrupt number BIOS has chosen or the ISA IRQs that were assigned
> >> to existing peripherals.
> >>
> >> By the time ACPI gets initialized, this code tries to determine an
> >> IRQ number based on penalty values in this array. It will try to locate
> >> the IRQ with the least penalty assignment so that interrupt sharing is
> >> avoided if possible.
> >>
> >> A couple of notes about the external APIs:
> >> 1. These API can be called before the ACPI is started. Therefore, one
> >> cannot assume that the PCI link objects are initialized for calculating
> >> penalties.
> >
> > Which API are you thinking about here? pcibios_penalize_isa_irq() is
> > called by ACPI and PNP, which should both be after ACPI is started.
>
> Correct, I was talking about acpi_penalize_sci_irq function here.
>
> >
> > My guess is you're thinking about acpi_penalize_sci_irq() (added back
> > later in this series), which is called here, which is definitely
> > before ACPI objects are available:
> >
> > setup_arch
> > acpi_boot_init
> > acpi_process_madt
> > acpi_parse_madt_ioapic_entries
> > acpi_table_parse_madt
> > acpi_parse_int_src_ovr
> > acpi_sci_ioapic_setup
> > acpi_penalize_sci_irq # <---
> >
> >> 2. The polarity and trigger information passed via the
> >> acpi_penalize_sci_irq from the BIOS may not match what the IRQ subsystem
> >> is reporting as the call might have been placed before the IRQ is
> >> registered by the interrupt subsystem.
> >>
> >> The previous change was in the direction to remove these external API and
> >> try to calculate the penalties at runtime for the ISA path as well. This
> >> didn't work out well with the existing platforms.
> >>
> >> Restoring the old behavior for IRQ < 256 and the new behavior will remain
> >> effective for IRQ >= 256.
> >
> > IIRC, this all started because we needed more than 256 IRQs, but we
> > didn't know how to size a static table to be large enough without
> > being wasteful.
>
> Correct. We only need 1024 for ARM/ARM64. But, we wanted to remove this
> restriction altogether to be arch proof. One of my earlier proposal was
> to just resize the array to 1024. I was asked if I was wasting resources
> by resizing to 1024.
>
> >
> > Prior to 5c5087a55390, we tracked penalties for IRQs 0-255. After it,
> > we only tracked penalties for IRQs 0-15. I think this patch basically
> > makes it so we track 0-255 again.
>
> Yes, we went back to 256 interrupts after the revert.
>
> >
> > *This* patch only increases the range for pcibios_penalize_isa_irq()
> > (and command-line hints, but hopefully nobody cares about those). A
> > subsequent patch increases it for SCI as well.
> >
> > The name "ACPI_MAX_IRQS" is now slightly misleading (because we do
> > support more than 256 IRQs) and the 256 value is sort of an
> > unjustified magic number. 16 is explainable as the number of ISA
> > IRQs, but I don't know what 256 is based on (other than historical
> > practice, of course). ACPI device IRQs can be much larger, and I
> > think the SCI IRQ can be, too (the FADT SCI_INT field is 16 bits).
> >
> > Can you tie this back to the specific problem on the broken machine
> > somehow? Do we need a penalty for an IRQ in the 16-255 range?
>
> The problem on the broken machine was SCI IRQ and PCI IRQ happened to be
> same. It was IRQ 11. When SCI IRQ heavily penalized IRQ 11 due to
> wrong interrupt type detection, PCI IRQs no longer worked as this line
> prohibits using the IRQ.
>
>
> if (acpi_irq_get_penalty(irq) >= PIRQ_PENALTY_ISA_ALWAYS) {
> printk(KERN_ERR PREFIX "No IRQ available for %s [%s]. "
> "Try pci=noacpi or acpi=off\n",
> acpi_device_name(link->device),
> acpi_device_bid(link->device));
> return -ENODEV;
> }
It seems like the problem is that we removed acpi_penalize_sci_irq(),
which told us the polarity and trigger mode. We tried to get that
information via irq_get_trigger_type(), but that didn't work in this
case because we use the acpi_irq_get_penalty() path before the SCI is
registered.
It makes sense to me to add acpi_penalize_sci_irq() back in, which is
what patch [3/3] does.
I don't understand how *this* patch, which basically just increases
the penalty array size from 16 to 256, helps fix the problem. It
seems like this patch should only matter if the SCI were some IRQ
between 16 and 255.
> > In a subsequent patch, I see something about the IRQ type not being
> > updated at the right time, but I can't quite connect the dots.
>
> The reason why PCI IRQ 11 didn't work is above.
>
> When we detected a problem with the SCI IRQ type, we were penalizing
> the IRQ below.
>
> static int acpi_irq_get_penalty(int irq)
> {
> ...
> if (irq == acpi_gbl_FADT.sci_interrupt) {
> u32 type = irq_get_trigger_type(irq) & IRQ_TYPE_SENSE_MASK;
>
> if (type != IRQ_TYPE_LEVEL_LOW)
> penalty += PIRQ_PENALTY_ISA_ALWAYS; <---- here
> else
> penalty += PIRQ_PENALTY_PCI_USING;
> }
>
>
>
> >
> > To be clear, I'm not asking for any changes in the patch; I'm just
> > trying to understand what's going on.
>
> Sure, I hope this makes it clear now.
>
> >
> >> Tested-by: Jonathan Liu <net147@gmail.com>
> >> Tested-by: Ondrej Zary <linux@rainbow-software.org>
> >> Link: http://www.gossamer-threads.com/lists/linux/kernel/2537016#2537016
> >> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> >> ---
> >> drivers/acpi/pci_link.c | 35 ++++++++++++++++++-----------------
> >> 1 file changed, 18 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> >> index c983bf7..f3792f4 100644
> >> --- a/drivers/acpi/pci_link.c
> >> +++ b/drivers/acpi/pci_link.c
> >> @@ -438,6 +438,7 @@ static int acpi_pci_link_set(struct acpi_pci_link *link, int irq)
> >> * enabled system.
> >> */
> >>
> >> +#define ACPI_MAX_IRQS 256
> >> #define ACPI_MAX_ISA_IRQS 16
> >>
> >> #define PIRQ_PENALTY_PCI_POSSIBLE (16*16)
> >> @@ -446,7 +447,7 @@ static int acpi_pci_link_set(struct acpi_pci_link *link, int irq)
> >> #define PIRQ_PENALTY_ISA_USED (16*16*16*16*16)
> >> #define PIRQ_PENALTY_ISA_ALWAYS (16*16*16*16*16*16)
> >>
> >> -static int acpi_isa_irq_penalty[ACPI_MAX_ISA_IRQS] = {
> >> +static int acpi_irq_penalty[ACPI_MAX_IRQS] = {
> >> PIRQ_PENALTY_ISA_ALWAYS, /* IRQ0 timer */
> >> PIRQ_PENALTY_ISA_ALWAYS, /* IRQ1 keyboard */
> >> PIRQ_PENALTY_ISA_ALWAYS, /* IRQ2 cascade */
> >> @@ -511,7 +512,7 @@ static int acpi_irq_get_penalty(int irq)
> >> }
> >>
> >> if (irq < ACPI_MAX_ISA_IRQS)
> >> - return penalty + acpi_isa_irq_penalty[irq];
> >> + return penalty + acpi_irq_penalty[irq];
> >>
> >> penalty += acpi_irq_pci_sharing_penalty(irq);
> >> return penalty;
> >> @@ -538,14 +539,14 @@ int __init acpi_irq_penalty_init(void)
> >>
> >> for (i = 0; i < link->irq.possible_count; i++) {
> >> if (link->irq.possible[i] < ACPI_MAX_ISA_IRQS)
> >> - acpi_isa_irq_penalty[link->irq.
> >> + acpi_irq_penalty[link->irq.
> >> possible[i]] +=
> >> penalty;
> >> }
> >>
> >> } else if (link->irq.active &&
> >> - (link->irq.active < ACPI_MAX_ISA_IRQS)) {
> >> - acpi_isa_irq_penalty[link->irq.active] +=
> >> + (link->irq.active < ACPI_MAX_IRQS)) {
> >> + acpi_irq_penalty[link->irq.active] +=
> >> PIRQ_PENALTY_PCI_POSSIBLE;
> >> }
> >> }
> >> @@ -828,7 +829,7 @@ static void acpi_pci_link_remove(struct acpi_device *device)
> >> }
> >>
> >> /*
> >> - * modify acpi_isa_irq_penalty[] from cmdline
> >> + * modify acpi_irq_penalty[] from cmdline
> >> */
> >> static int __init acpi_irq_penalty_update(char *str, int used)
> >> {
> >> @@ -837,24 +838,24 @@ static int __init acpi_irq_penalty_update(char *str, int used)
> >> for (i = 0; i < 16; i++) {
> >> int retval;
> >> int irq;
> >> - int new_penalty;
> >>
> >> retval = get_option(&str, &irq);
> >>
> >> if (!retval)
> >> break; /* no number found */
> >>
> >> - /* see if this is a ISA IRQ */
> >> - if ((irq < 0) || (irq >= ACPI_MAX_ISA_IRQS))
> >> + if (irq < 0)
> >> + continue;
> >> +
> >> + if (irq >= ARRAY_SIZE(acpi_irq_penalty))
> >> continue;
> >>
> >> if (used)
> >> - new_penalty = acpi_irq_get_penalty(irq) +
> >> - PIRQ_PENALTY_ISA_USED;
> >> + acpi_irq_penalty[irq] = acpi_irq_get_penalty(irq) +
> >> + PIRQ_PENALTY_ISA_USED;
> >> else
> >> - new_penalty = 0;
> >> + acpi_irq_penalty[irq] = 0;
> >>
> >> - acpi_isa_irq_penalty[irq] = new_penalty;
> >> if (retval != 2) /* no next number */
> >> break;
> >> }
> >> @@ -870,14 +871,14 @@ static int __init acpi_irq_penalty_update(char *str, int used)
> >> */
> >> void acpi_penalize_isa_irq(int irq, int active)
> >> {
> >> - if ((irq >= 0) && (irq < ARRAY_SIZE(acpi_isa_irq_penalty)))
> >> - acpi_isa_irq_penalty[irq] = acpi_irq_get_penalty(irq) +
> >> - (active ? PIRQ_PENALTY_ISA_USED : PIRQ_PENALTY_PCI_USING);
> >> + if (irq >= 0 && irq < ARRAY_SIZE(acpi_irq_penalty))
> >> + acpi_irq_penalty[irq] = acpi_irq_get_penalty(irq) +
> >> + (active ? PIRQ_PENALTY_ISA_USED : PIRQ_PENALTY_PCI_USING);
> >> }
> >>
> >> bool acpi_isa_irq_available(int irq)
> >> {
> >> - return irq >= 0 && (irq >= ARRAY_SIZE(acpi_isa_irq_penalty) ||
> >> + return irq >= 0 && (irq >= ARRAY_SIZE(acpi_irq_penalty) ||
> >> acpi_irq_get_penalty(irq) < PIRQ_PENALTY_ISA_ALWAYS);
> >> }
> >>
> >> --
> >> 1.9.1
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> >> the body of a message to majordomo at vger.kernel.org
> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
>
>
> --
> Sinan Kaya
> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
> Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply
* [PATCH v2] perf: xgene: Remove bogus IS_ERR() check
From: Al Viro @ 2016-10-13 18:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1476382156-11641-1-git-send-email-ttnguyen@apm.com>
On Thu, Oct 13, 2016 at 11:09:16AM -0700, Tai Nguyen wrote:
> In acpi_get_pmu_hw_inf we pass the address of a local variable to IS_ERR(),
> which doesn't make sense, as the pointer must be a real, valid pointer.
> This doesn't cause a functional problem, as IS_ERR() will evaluate as
> false, but the check is bogus and causes static checkers to complain.
... unless the test is actually a misspelled IS_ERR(res) and the current
code is broken by effectively skipping it.
^ permalink raw reply
* [PATCH V2 3/3] Revert "ACPI,PCI,IRQ: remove SCI penalize function"
From: Bjorn Helgaas @ 2016-10-13 18:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475615720-31047-4-git-send-email-okaya@codeaurora.org>
On Tue, Oct 04, 2016 at 05:15:19PM -0400, Sinan Kaya wrote:
> The SCI function was removed in two steps (first refactor and then remove).
> This patch does the revert in one step.
>
> The commit 103544d86976 ("ACPI,PCI,IRQ: reduce resource requirements")
> refactored the original code so that SCI penalty is calculated dynamically
> by the time get_penalty function is called. This patch does a partial
> revert for the SCI functionality only.
>
> The commit 9e5ed6d1fb87 ("ACPI,PCI,IRQ: remove SCI penalize function") is
> for the removal of the function. SCI penalty API was replaced by the
> runtime penalty calculation based on the value of
> acpi_gbl_FADT.sci_interrupt.
>
> The IRQ type does not get updated at the right time for some platforms and
> results in incorrect penalty assignment for PCI IRQs as
> irq_get_trigger_type returns the wrong type.
>
> The register_gsi function delivers the IRQ found in the ACPI table to
> the interrupt controller driver. Penalties are calculated before a
> link object is enabled to find out which interrupt has the least number
> of users. By the time penalties are calculated, the IRQ is not registered
> yet and the API returns the wrong type.
>
> Tested-by: Jonathan Liu <net147@gmail.com>
> Tested-by: Ondrej Zary <linux@rainbow-software.org>
> Link: https://lkml.org/lkml/2016/10/4/283
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
> arch/x86/kernel/acpi/boot.c | 1 +
> drivers/acpi/pci_link.c | 34 ++++++++++++++--------------------
> include/linux/acpi.h | 1 +
> 3 files changed, 16 insertions(+), 20 deletions(-)
>
> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> index 90d84c3..0ffd26e 100644
> --- a/arch/x86/kernel/acpi/boot.c
> +++ b/arch/x86/kernel/acpi/boot.c
> @@ -453,6 +453,7 @@ static void __init acpi_sci_ioapic_setup(u8 bus_irq, u16 polarity, u16 trigger,
> polarity = acpi_sci_flags & ACPI_MADT_POLARITY_MASK;
>
> mp_override_legacy_irq(bus_irq, polarity, trigger, gsi);
> + acpi_penalize_sci_irq(bus_irq, trigger, polarity);
>
> /*
> * stash over-ride to indicate we've been here
> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> index 06c2a11..6a2af19 100644
> --- a/drivers/acpi/pci_link.c
> +++ b/drivers/acpi/pci_link.c
> @@ -495,27 +495,10 @@ static int acpi_irq_pci_sharing_penalty(int irq)
>
> static int acpi_irq_get_penalty(int irq)
> {
> - int penalty = 0;
> -
> - /*
> - * Penalize IRQ used by ACPI SCI. If ACPI SCI pin attributes conflict
> - * with PCI IRQ attributes, mark ACPI SCI as ISA_ALWAYS so it won't be
> - * use for PCI IRQs.
> - */
> - if (irq == acpi_gbl_FADT.sci_interrupt) {
> - u32 type = irq_get_trigger_type(irq) & IRQ_TYPE_SENSE_MASK;
> -
> - if (type != IRQ_TYPE_LEVEL_LOW)
> - penalty += PIRQ_PENALTY_ISA_ALWAYS;
> - else
> - penalty += PIRQ_PENALTY_PCI_USING;
> - }
> -
> - if (irq < ACPI_MAX_ISA_IRQS)
> - return penalty + acpi_irq_penalty[irq];
> + if (irq < ACPI_MAX_IRQS)
> + return acpi_irq_penalty[irq];
>
> - penalty += acpi_irq_pci_sharing_penalty(irq);
> - return penalty;
> + return acpi_irq_pci_sharing_penalty(irq);
> }
>
> int __init acpi_irq_penalty_init(void)
> @@ -886,6 +869,17 @@ bool acpi_isa_irq_available(int irq)
> acpi_irq_get_penalty(irq) < PIRQ_PENALTY_ISA_ALWAYS);
> }
>
> +void acpi_penalize_sci_irq(int irq, int trigger, int polarity)
> +{
> + if (irq >= 0 && irq < ARRAY_SIZE(acpi_irq_penalty)) {
> + if (trigger != ACPI_MADT_TRIGGER_LEVEL ||
> + polarity != ACPI_MADT_POLARITY_ACTIVE_LOW)
> + acpi_irq_penalty[irq] += PIRQ_PENALTY_ISA_ALWAYS;
> + else
> + acpi_irq_penalty[irq] += PIRQ_PENALTY_PCI_USING;
This would be a lot easier to read if the trigger/polarity tests were
positive, e.g.,
if (trigger == ACPI_MADT_TRIGGER_LEVEL &&
polarity == ACPI_MADT_POLARITY_ACTIVE_LOW)
acpi_irq_penalty[irq] += PIRQ_PENALTY_PCI_USING;
else
acpi_irq_penalty[irq] += PIRQ_PENALTY_ISA_ALWAYS;
> + }
> +}
> +
> /*
> * Over-ride default table to reserve additional IRQs for use by ISA
> * e.g. acpi_irq_isa=5
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 4d8452c..85ac7d5 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -318,6 +318,7 @@ struct pci_dev;
> int acpi_pci_irq_enable (struct pci_dev *dev);
> void acpi_penalize_isa_irq(int irq, int active);
> bool acpi_isa_irq_available(int irq);
> +void acpi_penalize_sci_irq(int irq, int trigger, int polarity);
> void acpi_pci_irq_disable (struct pci_dev *dev);
>
> extern int ec_read(u8 addr, u8 *val);
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v2] drm/bridge: analogix: protect power when get_modes or detect
From: Sean Paul @ 2016-10-13 19:15 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1476323438-8435-1-git-send-email-mark.yao@rock-chips.com>
On Wed, Oct 12, 2016 at 9:50 PM, Mark Yao <mark.yao@rock-chips.com> wrote:
> The drm callback ->detect and ->get_modes seems is not power safe,
> they may be called when device is power off, do register access on
> detect or get_modes will cause system die.
>
> Here is the path call ->detect before analogix_dp power on
> [<ffffff800843babc>] analogix_dp_detect+0x44/0xdc
> [<ffffff80083fd840>] drm_helper_probe_single_connector_modes_merge_bits+0xe8/0x41c
> [<ffffff80083fdb84>] drm_helper_probe_single_connector_modes+0x10/0x18
> [<ffffff8008418d24>] drm_mode_getconnector+0xf4/0x304
> [<ffffff800840cff0>] drm_ioctl+0x23c/0x390
> [<ffffff80081a8adc>] do_vfs_ioctl+0x4b8/0x58c
> [<ffffff80081a8c10>] SyS_ioctl+0x60/0x88
>
> Cc: Inki Dae <inki.dae@samsung.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> Cc: "Ville Syrj?l?" <ville.syrjala@linux.intel.com>
>
> Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
Thanks for revising, this is much better.
Reviewed-by: Sean Paul <seanpaul@chromium.org>
> ---
> Changes in v2:
> - remove sub device power on/off callback, use pm_runtime_get/put is enough
> to fix my problem, so will can avoid race to dpms.
>
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index efac8ab..ff2d328 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1062,11 +1062,15 @@ int analogix_dp_get_modes(struct drm_connector *connector)
> return 0;
> }
>
> + pm_runtime_get_sync(dp->dev);
> +
> if (analogix_dp_handle_edid(dp) == 0) {
> drm_mode_connector_update_edid_property(&dp->connector, edid);
> num_modes += drm_add_edid_modes(&dp->connector, edid);
> }
>
> + pm_runtime_put(dp->dev);
> +
> if (dp->plat_data->panel)
> num_modes += drm_panel_get_modes(dp->plat_data->panel);
>
> @@ -1106,9 +1110,13 @@ analogix_dp_detect(struct drm_connector *connector, bool force)
> return connector_status_disconnected;
> }
>
> + pm_runtime_get_sync(dp->dev);
> +
> if (!analogix_dp_detect_hpd(dp))
> status = connector_status_connected;
>
> + pm_runtime_put(dp->dev);
> +
> ret = analogix_dp_prepare_panel(dp, false, false);
> if (ret)
> DRM_ERROR("Failed to unprepare panel (%d)\n", ret);
> --
> 1.9.1
>
>
^ permalink raw reply
* [PATCH V2 1/3] Revert "ACPI, PCI, IRQ: reduce static IRQ array size to 16"
From: Sinan Kaya @ 2016-10-13 19:36 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161013181535.GB21529@localhost>
On 10/13/2016 2:15 PM, Bjorn Helgaas wrote:
> It seems like the problem is that we removed acpi_penalize_sci_irq(),
> which told us the polarity and trigger mode. We tried to get that
> information via irq_get_trigger_type(), but that didn't work in this
> case because we use the acpi_irq_get_penalty() path before the SCI is
> registered.
>
> It makes sense to me to add acpi_penalize_sci_irq() back in, which is
> what patch [3/3] does.
>
> I don't understand how *this* patch, which basically just increases
> the penalty array size from 16 to 256, helps fix the problem. It
> seems like this patch should only matter if the SCI were some IRQ
> between 16 and 255.
I see your point. The original code supported 256 interrupts.
The machine where we had the problem had an SCI interrupt of 11. So,
this patch does not necessarily fix anything for this machine alone.
However, to be safe; I wanted to go back to the old behavior to fix
the SCI issue for all existing platforms.
--
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply
* [PATCH V3 02/10] ras: acpi/apei: cper: generic error data entry v3 per ACPI 6.1
From: Baicar, Tyler @ 2016-10-13 19:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <064dca7d-0625-c0d0-09ae-72ef7abdc63f@arm.com>
Hello Suzuki,
On 10/13/2016 2:50 AM, Suzuki K Poulose wrote:
> On 12/10/16 23:10, Baicar, Tyler wrote:
>> Hello Suzuki,
>>
>> Thank you for the feedback! Responses below.
>>
>> On 10/11/2016 11:28 AM, Suzuki K Poulose wrote:
>>> On 07/10/16 22:31, Tyler Baicar wrote:
>>>> Currently when a RAS error is reported it is not timestamped.
>>>> The ACPI 6.1 spec adds the timestamp field to the generic error
>>>> data entry v3 structure. The timestamp of when the firmware
>>>> generated the error is now being reported.
>>>>
>>>> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
>>>> Signed-off-by: Richard Ruigrok <rruigrok@codeaurora.org>
>>>> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
>>>> Signed-off-by: Naveen Kaje <nkaje@codeaurora.org>
>>>
>>> Please could you keep the people who reviewed/commented on your
>>> series in the past,
>>> whenever you post a new version ?
>> Do you mean to just send the new version to their e-mail directly in
>> addition to the lists? If so, I will do that next time.
>
> If you send a new version of a series to the list, it is a good idea
> to keep
> the people who commented (significantly) on your previous version in
> Cc, especially
> when you have addressed their feedback. That will help them to keep
> track of the
> series. People can always see the new version in the list, but then it
> is so easy
> to miss something in the 100s of emails you get each day. I am sure
> people have
> special filters for the emails based on if they are in Cc/To etc.
>
Okay, understood. I'll make sure to add those who have commented in the
cc/to list to avoid the e-mail filters.
>>
>> I know you provided good feedback on the previous patchset, but I did
>> not have anyone specifically respond to add "reviewed-by:...". I
>> don't think I should add reviewed-by for someone unless they
>> specifically add it in a response :)
>
> No, I haven't yet "Reviewed-by" your patches. I had some comments on
> it, which means
> I expected it to be addressed as you committed in your response.
>
>>>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>>>> index 3021f0e..c8488f1 100644
>>>> --- a/drivers/acpi/apei/ghes.c
>>>> +++ b/drivers/acpi/apei/ghes.c
>>>> @@ -80,6 +80,10 @@
>
>> I think that should work to avoid duplication. I will move them to a
>> header file in the next patchset.
>>>> +
>>>> +static void cper_estatus_print_section_v300(const char *pfx,
>>>> + const struct acpi_hest_generic_data_v300 *gdata)
>>>> +{
>>>> + __u8 hour, min, sec, day, mon, year, century, *timestamp;
>>>> +
>>>> + if (gdata->validation_bits & ACPI_HEST_GEN_VALID_TIMESTAMP) {
>>>> + timestamp = (__u8 *)&(gdata->time_stamp);
>>>> + memcpy(&sec, timestamp, 1);
>>>> + memcpy(&min, timestamp + 1, 1);
>>>> + memcpy(&hour, timestamp + 2, 1);
>>>> + memcpy(&day, timestamp + 4, 1);
>>>> + memcpy(&mon, timestamp + 5, 1);
>>>> + memcpy(&year, timestamp + 6, 1);
>>>> + memcpy(¢ury, timestamp + 7, 1);
>>>> + printk("%stime: ", pfx);
>>>> + printk("%7s", 0x01 & *(timestamp + 3) ? "precise" : "");
>>>
>>> What format is the (timestamp + 3) stored in ? Does it need
>>> conversion ?
>> The third byte of the timestamp is currently only used to determine
>> if the time is precise or not. Bit 0 is used to specify that and the
>> other bits in this byte are marked as reserved. This is shown in
>> table 247 of the UEFI spec 2.6:
>>
>> Byte 3:
>> Bit 0 ? Timestamp is precise if this bit is set and correlates to
>> the time of the error event.
>> Bit 7:1 ? Reserved
>
> Is it always the same endianness as that of the CPU ?
It is a fair assumption that the firmware populating this record will
use a CPU of the same endianness. There is no mechanism in the spec to
indicate otherwise.
Thanks,
Tyler
>
> Cheers
> Suzuki
>
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
^ permalink raw reply
* Low network throughput on i.MX28
From: Jörg Krause @ 2016-10-13 19:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161013084807.6a231fdb@ipc1.ka-ro>
Hi Lothar,
Am 13. Oktober 2016 08:48:07 MESZ, schrieb "Lothar Wa?mann" <LW@KARO-electronics.de>:
>Hi,
>
>On Thu, 13 Oct 2016 01:09:13 +0200 J?rg Krause wrote:
>> Hi,
>>
>> I am using a custom i.MX28 board similar to the i.MX28-EVK. For Wi-Fi
>> the board assembles a BCM43362 from Broadcom and for Ethernet a
>> LAN8720A from Microchip. The board is running mainline Linux 4.7.
>>
>> While both, wireless and wired network interfaces work, the TCP
>> throughput measured with iperf is low.
>>
>> The bandwith for Ethernet is between 20-30 MBits/sec and for WLAN is
>> about 4-5 MBits/sec.
>>
>> There exists an Application Note "i.MX28 Ethernet Performance on
>> Linux" [1] which shows a bandwith of > 60 MBits/sec. A user an the
>NXP
>> forum [2] told he achieved 20 MBits/sec with some Qualcom chip.
>>
>> Note, that these values are most probably measured with the legacy
>> Linux Kernel 2.6.35 from NXP.
>>
>> Does anybody has done throughput tests on i.MX28 with mainline
>Kernel?
>> If so, what are the results? What might be the bottleneck?
>>
>
>This is the iperf output on a TX28 with current mainline kernel
>(4.8.0-rc5):
>------------------------------------------------------------
>Client connecting to 192.168.100.1, TCP port 5001
>TCP window size: 43.8 KByte (default)
>------------------------------------------------------------
>[ 3] local 192.168.100.56 port 60325 connected with 192.168.100.1 port
>5001
>[ ID] Interval Transfer Bandwidth
>[ 3] 0.0-10.0 sec 57.5 MBytes 48.2 Mbits/sec
>
>You might check your kernel DEBUG configs (especially
>CONFIG_DEBUG_PAGEALLOC).
Thanks for sharing the iperf output. What LAN transceiver does the TX28 has assembled?
I checked the config and is has no DEBUG_PAGEALLOC enabled and no DEBUG options related to network.
Best regards
J?rg Krause
^ permalink raw reply
* [PATCH] arm64: kaslr: fix breakage with CONFIG_MODVERSIONS=y
From: Timur Tabi @ 2016-10-13 19:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1476376929-29688-1-git-send-email-ard.biesheuvel@linaro.org>
Ard Biesheuvel wrote:
> As it turns out, the KASLR code breaks CONFIG_MODVERSIONS, since the
> kcrctab has an absolute address field that is relocated at runtime
> when the kernel offset is randomized.
>
> This has been fixed already for PowerPC in the past, so simply wire up
> the existing code dealing with this issue.
>
> Signed-off-by: Ard Biesheuvel<ard.biesheuvel@linaro.org>
Tested-by: Timur Tabi <timur@codeaurora.org>
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply
* [RFC] arm64: Enforce observed order for spinlock and data
From: bdegraaf at codeaurora.org @ 2016-10-13 20:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161013110256.GA436@arm.com>
On 2016-10-13 07:02, Will Deacon wrote:
> Brent,
>
> On Wed, Oct 12, 2016 at 04:01:06PM -0400, bdegraaf at codeaurora.org
> wrote:
>
> Everything from this point down needs clarification.
>
>> All arm64 lockref accesses that occur without taking the spinlock must
>> behave like true atomics, ensuring successive operations are all done
>> sequentially.
>
> What is a "true atomic"? What do you mean by "successive"? What do you
> mean by "done sequentially"?
>
Despite the use case in dentry, lockref itself is a generic locking API,
and
any problems I describe here are with the generic API itself, not
necessarily
the dentry use case. I'm not patching dentry--I'm fixing lockref.
By necessity, the API must do its update atomically, as keeping things
correct
involves potential spinlock access by other agents which may opt to use
the
spinlock API or the lockref API at their discretion. With the current
arm64
spinlock implementation, it is possible for the lockref to observe the
changed
contents of the protected count without observing the spinlock being
locked,
which could lead to missed changes to the lock_count itself, because any
calculations made on it could be overwritten or completed in a different
sequence.
(Spinlock locked access is obtained with a simple store under certain
scenarios. My attempt to fix this in the spinlock code was met with
resistance
saying it should be addressed in lockref, since that is the API that
would
encounter the issue. These changes were initiated in response to that
request.
Additional ordering problems were uncovered when I looked into lockref
itself.)
The example below involves only a single agent exactly as you explain
the
problem in commit 8e86f0b409a44193f1587e87b69c5dcf8f65be67. Even for a
single
execution agent, this means that the code below could access out of
order.
As lockref is a generic API, it doesn't matter whether dentry does this
or not.
By "done sequentially," I mean "accessed in program order."
As far as "true atomic" goes, I am referring to an atomic in the same
sense you
did in commit 8e86f0b409a44193f1587e87b69c5dcf8f65be67.
> The guarantee provided by lockref is that, if you hold the spinlock,
> then
> you don't need to use atomics to inspect the reference count, as it is
> guaranteed to be stable. You can't just go around replacing spin_lock
> calls with lockref_get -- that's not what this is about.
>
I am not sure where you got the idea that I was referring to replacing
spinlocks
with lockref calls. That is not the foundation for this fix.
>> Currently
>> the lockref accesses, when decompiled, look like the following
>> sequence:
>>
>> <Lockref "unlocked" Access [A]>
>>
>> // Lockref "unlocked" (B)
>> 1: ldxr x0, [B] // Exclusive load
>> <change lock_count B>
>> stxr w1, x0, [B]
>> cbnz w1, 1b
>>
>> <Lockref "unlocked" Access [C]>
>>
>> Even though access to the lock_count is protected by exclusives, this
>> is not
>> enough
>> to guarantee order: The lock_count must change atomically, in order,
>> so the
>> only
>> permitted ordering would be:
>> A -> B -> C
>
> Says who? Please point me at a piece of code that relies on this. I'm
> willing to believe that are bugs in this area, but waving your hands
> around
> and saying certain properties "must" hold is not helpful unless you can
> say *why* they must hold and *where* that is required.
>
The lockref code must access in order, because other agents can observe
it via
spinlock OR lockref APIs. Again, this is a generic API, not an explicit
part of
dentry. Other code will use it, and the manner in which it is used in
dentry is not
relevant. What lock_count is changed to is not proscribed by the
lockref
API. There is no guarantee whether it be an add, subtract, multiply,
divide, set
to some explicit value, etc. But the changes must be done in program
order and
observable in that same order by other agents: Therefore, the spinlock
and lock_count
must be accessed atomically, and observed to change atomically at the
system level.
I am not off base saying lockref is an atomic access. Here are some
references:
Under Documentation/filesystems/path-lookup.md, the dentry->d_lockref
mechanism
is described as an atomic access.
At the time lockref was introduced, The Linux Foundation gave a
presentation at
LinuxCon 2014 that can be found at the following link:
https://events.linuxfoundation.org/sites/events/files/slides/linuxcon-2014-locking-final.pdf
On page 46, it outlines the lockref API. The first lines of the slide
give the
relevant details.
Lockref
? *Generic* mechanism to *atomically* update a reference count that is
protected by a spinlock without actually acquiring the spinlock
itself.
While dentry's use is mentioned, this API is not restricted to the use
case of dentry.
>> Unfortunately, this is not the case by the letter of the architecture
>> and,
>> in fact,
>> the accesses to A and C are not protected by any sort of barrier, and
>> hence
>> are
>> permitted to reorder freely, resulting in orderings such as
>>
>> Bl -> A -> C -> Bs
>
> Again, why is this a problem? It's exactly the same as if you did:
>
> spin_lock(lock);
> inc_ref_cnt();
> spin_unlock(lock);
>
> Accesses outside of the critical section can still be reordered. Big
> deal.
>
Since the current code resembles but actually has *fewer* ordering
effects
compared to the example used by your atomic.h commit, even though
A->B->C is in
program order, it could access out of order according to your own commit
text
on commit 8e86f0b409a44193f1587e87b69c5dcf8f65be67.
Taking spin_lock/spin_unlock, however, includes ordering by nature of
the
load-acquire observing the store-release of a prior unlock, so ordering
is
enforced with the spinlock version of accesses. The lockref itself has
*no*
ordering enforced, unless a locked state is encountered and it falls
back
to the spinlock code. So this is a fundamental difference between
lockref and
spinlock. So, no, lockref ordering is currently not exactly the same as
spinlock--but it should be.
>> In this specific scenario, since "change lock_count" could be an
>> increment, a decrement or even a set to a specific value, there could
>> be
>> trouble.
>
> What trouble?
>
Take, for example, a use case where the ref count is either positive or
zero.
If increments and decrements hit out of order, a decrement that was
supposed
to come after an increment would instead do nothing if the value of the
lock
started at zero. Then when the increment hit later, the ref count would
remain
positive with a net effect of +1 to the ref count instead of +1-1=0.
Again,
however, the lockref does not specify how the contents of lock_count are
manipulated, it was only meant to guarantee that they are done
atomically when
the lock is not held.
>> With more agents accessing the lockref without taking the lock, even
>> scenarios where the cmpxchg passes falsely can be encountered, as
>> there is
>> no guarantee that the the "old" value will not match exactly a newer
>> value
>> due to out-of-order access by a combination of agents that increment
>> and
>> decrement the lock_count by the same amount.
>
> This is the A-B-A problem, but I don't see why it affects us here.
> We're
> dealing with a single reference count.
>
If lockref accesses were to occur on many Pe's, there are all sorts of
things that could happen in terms of who wins what, and what they set
the
lock_count to. My point is simply that each access should be atomic
because
lockref is a generic API and was intended to be a lockless atomic
access.
Leaving this problem open until someone else introduces a use that
exposes
it, which could happen in the main kernel code, is probably not a good
idea, as it could prove difficult to track down.
>> Since multiple agents are accessing this without locking the spinlock,
>> this access must have the same protections in place as atomics do in
>> the
>> arch's atomic.h.
>
> Why? I don't think that it does. Have a look at how lockref is used by
> the dcache code: it's really about keeping a reference to a dentry,
> which may be in the process of being unhashed and removed. The
> interaction with concurrent updaters to the dentry itself is handled
> using a seqlock, which does have the necessary barriers. Yes, the code
> is extremely complicated, but given that you're reporting issues based
> on code inspection, then you'll need to understand what you're
> changing.
>
Again, this is a generic API, not an API married to dentry. If it were
for
dentry's sole use, it should not be accessible outside of the dentry
code.
While the cmpxchg64_relaxed case may be OK for dentry, it is not OK for
the
generic case.
>> Fortunately, the fix is not complicated: merely removing the errant
>> _relaxed option on the cmpxchg64 is enough to introduce exactly the
>> same
>> code sequence justified in commit
>> 8e86f0b409a44193f1587e87b69c5dcf8f65be67
>> to fix arm64 atomics.
>
> I introduced cmpxchg64_relaxed precisely for the lockref case. I still
> don't see a compelling reason to strengthen it. If you think there's a
> bug,
> please spend the effort to describe how it manifests and what can
> actually
> go wrong in the existing codebase. Your previous patches fixing
> so-called
> bugs found by inspection have both turned out to be bogus, so I'm
> sorry,
> but I'm not exactly leaping on your contributions to this.
>
> Will
I have detailed the problems here, and they are with the generic case,
no
hand waving required.
On a further note, it is not accurate to say that my prior patches were
bogus: One called to attention a yet-to-be-corrected problem in the
ARMv8
Programmer's Guide, and the other was sidestepped by a refactor that
addressed the problem I set out to fix with a control flow change. Since
that problem was the fundamental reason I had worked on the gettime code
in the first place, I abandoned my effort. The refactor that fixed the
control-flow problem, however, is still missing on v4.7 and earlier
kernels
(sequence lock logic should be verified prior to the isb that demarcates
the virtual counter register read). I have confirmed this is an issue on
various armv8 hardware, sometimes obtaining identical register values
between multiple reads that were delayed such that they should have
shown
changes, evidence that the register read accessed prior to the seqlock
update having finished (the control flow problem).
Brent
^ permalink raw reply
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