* Re: [v5 2/6] powerpc/mce: Fix MCE handling for huge pages
From: Nicholas Piggin @ 2019-07-11 9:38 UTC (permalink / raw)
To: linux-kernel, linuxppc-dev, Santosh Sivaraj
Cc: Aneesh Kumar K.V, Mahesh Salgaonkar, Chandan Rajendra, Reza Arbab
In-Reply-To: <20190709121524.18762-3-santosh@fossix.org>
Santosh Sivaraj's on July 9, 2019 10:15 pm:
> From: Balbir Singh <bsingharora@gmail.com>
>
> The current code would fail on huge pages addresses, since the shift
> would be incorrect. Use the correct page shift value returned by
> __find_linux_pte() to get the correct pfn. The code is more generic
> and can handle both regular and compound pages.
>
> Fixes: ba41e1e1ccb9 ("powerpc/mce: Hookup derror (load/store) UE errors")
>
> Signed-off-by: Balbir Singh <bsingharora@gmail.com>
> [arbab@linux.ibm.com: Fixup pseries_do_memory_failure()]
> Signed-off-by: Reza Arbab <arbab@linux.ibm.com>
> Signed-off-by: Santosh Sivaraj <santosh@fossix.org>
> ---
> arch/powerpc/include/asm/mce.h | 3 ++-
> arch/powerpc/kernel/mce_power.c | 26 ++++++++++++++++----------
> arch/powerpc/platforms/pseries/ras.c | 6 ++++--
> 3 files changed, 22 insertions(+), 13 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/mce.h b/arch/powerpc/include/asm/mce.h
> index a4c6a74ad2fb..94888a7025b3 100644
> --- a/arch/powerpc/include/asm/mce.h
> +++ b/arch/powerpc/include/asm/mce.h
> @@ -209,7 +209,8 @@ extern void release_mce_event(void);
> extern void machine_check_queue_event(void);
> extern void machine_check_print_event_info(struct machine_check_event *evt,
> bool user_mode, bool in_guest);
> -unsigned long addr_to_pfn(struct pt_regs *regs, unsigned long addr);
> +unsigned long addr_to_pfn(struct pt_regs *regs, unsigned long addr,
> + unsigned int *shift);
> #ifdef CONFIG_PPC_BOOK3S_64
> void flush_and_reload_slb(void);
> #endif /* CONFIG_PPC_BOOK3S_64 */
> diff --git a/arch/powerpc/kernel/mce_power.c b/arch/powerpc/kernel/mce_power.c
> index e39536aad30d..04666c0b40a8 100644
> --- a/arch/powerpc/kernel/mce_power.c
> +++ b/arch/powerpc/kernel/mce_power.c
> @@ -23,7 +23,8 @@
> * Convert an address related to an mm to a PFN. NOTE: we are in real
> * mode, we could potentially race with page table updates.
> */
> -unsigned long addr_to_pfn(struct pt_regs *regs, unsigned long addr)
> +unsigned long addr_to_pfn(struct pt_regs *regs, unsigned long addr,
> + unsigned int *shift)
> {
> pte_t *ptep;
> unsigned long flags;
> @@ -36,13 +37,15 @@ unsigned long addr_to_pfn(struct pt_regs *regs, unsigned long addr)
>
> local_irq_save(flags);
> if (mm == current->mm)
> - ptep = find_current_mm_pte(mm->pgd, addr, NULL, NULL);
> + ptep = find_current_mm_pte(mm->pgd, addr, NULL, shift);
> else
> - ptep = find_init_mm_pte(addr, NULL);
> + ptep = find_init_mm_pte(addr, shift);
> local_irq_restore(flags);
> if (!ptep || pte_special(*ptep))
> return ULONG_MAX;
> - return pte_pfn(*ptep);
> + if (!*shift)
> + *shift = PAGE_SHIFT;
> + return (pte_val(*ptep) & PTE_RPN_MASK) >> *shift;
> }
>
> /* flush SLBs and reload */
Ah, the comment I made earlier to this patch I think missed some detail.
But what we should do here is return the pfn (which is always units
of PAGE_SIZE). So you have to adjust by the lower part of the address
here, rather than returning shift which is unnecessary.
Possibly even better is to just return the real address, which is
what all callers seem to want anyway.
> @@ -358,15 +361,16 @@ static int mce_find_instr_ea_and_pfn(struct pt_regs *regs, uint64_t *addr,
> unsigned long pfn, instr_addr;
> struct instruction_op op;
> struct pt_regs tmp = *regs;
> + unsigned int shift;
>
> - pfn = addr_to_pfn(regs, regs->nip);
> + pfn = addr_to_pfn(regs, regs->nip, &shift);
> if (pfn != ULONG_MAX) {
> - instr_addr = (pfn << PAGE_SHIFT) + (regs->nip & ~PAGE_MASK);
> + instr_addr = (pfn << shift) + (regs->nip & ((1 << shift) - 1));
This wants the exact real address.
> instr = *(unsigned int *)(instr_addr);
> if (!analyse_instr(&op, &tmp, instr)) {
> - pfn = addr_to_pfn(regs, op.ea);
> + pfn = addr_to_pfn(regs, op.ea, &shift);
> *addr = op.ea;
> - *phys_addr = (pfn << PAGE_SHIFT);
> + *phys_addr = (pfn << shift);
> return 0;
> }
I'm not sure this is really what we want. You do really want the
PAGE_SIZE pfn here. Say you have a failure in the nth small page
of a large page mapping, this gives the physical address of the
start of the large page, so memory failure will fail out the 0th
small page won't it?
> /*
> @@ -442,12 +446,14 @@ static int mce_handle_ierror(struct pt_regs *regs,
> if (mce_err->sync_error &&
> table[i].error_type == MCE_ERROR_TYPE_UE) {
> unsigned long pfn;
> + unsigned int shift;
>
> if (get_paca()->in_mce < MAX_MCE_DEPTH) {
> - pfn = addr_to_pfn(regs, regs->nip);
> + pfn = addr_to_pfn(regs, regs->nip,
> + &shift);
> if (pfn != ULONG_MAX) {
> *phys_addr =
> - (pfn << PAGE_SHIFT);
> + (pfn << shift);
> }
> }
> }
> diff --git a/arch/powerpc/platforms/pseries/ras.c b/arch/powerpc/platforms/pseries/ras.c
> index f16fdd0f71f7..5e43283d3300 100644
> --- a/arch/powerpc/platforms/pseries/ras.c
> +++ b/arch/powerpc/platforms/pseries/ras.c
> @@ -740,12 +740,14 @@ static void pseries_do_memory_failure(struct pt_regs *regs,
> paddr = be64_to_cpu(mce_log->logical_address);
> } else if (mce_log->sub_err_type & UE_EFFECTIVE_ADDR_PROVIDED) {
> unsigned long pfn;
> + unsigned int shift;
>
> pfn = addr_to_pfn(regs,
> - be64_to_cpu(mce_log->effective_address));
> + be64_to_cpu(mce_log->effective_address),
> + &shift);
> if (pfn == ULONG_MAX)
> return;
> - paddr = pfn << PAGE_SHIFT;
> + paddr = pfn << shift;
> } else {
> return;
> }
Same for all these.
Thanks,
Nick
^ permalink raw reply
* Re: [v5 4/6] extable: Add function to search only kernel exception table
From: Nicholas Piggin @ 2019-07-11 9:47 UTC (permalink / raw)
To: linux-kernel, linuxppc-dev, Santosh Sivaraj
Cc: Aneesh Kumar K.V, Mahesh Salgaonkar, Chandan Rajendra,
Thomas Gleixner, Reza Arbab, Ingo Molnar
In-Reply-To: <20190709121524.18762-5-santosh@fossix.org>
Santosh Sivaraj's on July 9, 2019 10:15 pm:
> In real mode, the search_exception tables cannot be called because
> it also searches the module exception tables if entry is not found
> in the kernel exception tables.
This is a patch for generic kernel code, it may not go through
powerpc tree and someone doing git blame or something won't have
any idea what this changelog means.
Can you re-word this and just say something like, "certain architecture
$specific operating modes (e.g., in powerpc machine check handler that
is unable to access vmalloc memory)."
>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Signed-off-by: Santosh Sivaraj <santosh@fossix.org>
> ---
> include/linux/extable.h | 2 ++
> kernel/extable.c | 16 +++++++++++++---
> 2 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/extable.h b/include/linux/extable.h
> index 41c5b3a25f67..0c2819ba67f0 100644
> --- a/include/linux/extable.h
> +++ b/include/linux/extable.h
> @@ -19,6 +19,8 @@ void trim_init_extable(struct module *m);
>
> /* Given an address, look for it in the exception tables */
> const struct exception_table_entry *search_exception_tables(unsigned long add);
> +const struct
> +exception_table_entry *search_kernel_exception_table(unsigned long addr);
>
> #ifdef CONFIG_MODULES
> /* For extable.c to search modules' exception tables. */
> diff --git a/kernel/extable.c b/kernel/extable.c
> index e23cce6e6092..6d544cb79fff 100644
> --- a/kernel/extable.c
> +++ b/kernel/extable.c
> @@ -40,13 +40,23 @@ void __init sort_main_extable(void)
> }
> }
>
> -/* Given an address, look for it in the exception tables. */
> +/* For the given address, look for it in the kernel exception table */
Nitpick, no reason to vary the structure of the comment.
Code wise it looks fine, so with the comments and changelog fixed,
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> +const
> +struct exception_table_entry *search_kernel_exception_table(unsigned long addr)
> +{
> + return search_extable(__start___ex_table,
> + __stop___ex_table - __start___ex_table, addr);
> +}
> +
> +/*
> + * Given an address, look for it in the kernel and the module exception
> + * tables.
> + */
> const struct exception_table_entry *search_exception_tables(unsigned long addr)
> {
> const struct exception_table_entry *e;
>
> - e = search_extable(__start___ex_table,
> - __stop___ex_table - __start___ex_table, addr);
> + e = search_kernel_exception_table(addr);
> if (!e)
> e = search_module_extables(addr);
> return e;
> --
> 2.20.1
>
>
^ permalink raw reply
* Re: [v5 3/6] powerpc/memcpy: Add memcpy_mcsafe for pmem
From: Nicholas Piggin @ 2019-07-11 9:50 UTC (permalink / raw)
To: linux-kernel, linuxppc-dev, Santosh Sivaraj
Cc: Aneesh Kumar K.V, Mahesh Salgaonkar, Chandan Rajendra, Reza Arbab
In-Reply-To: <20190709121524.18762-4-santosh@fossix.org>
Santosh Sivaraj's on July 9, 2019 10:15 pm:
> From: Balbir Singh <bsingharora@gmail.com>
>
> The pmem infrastructure uses memcpy_mcsafe in the pmem layer so as to
> convert machine check exceptions into a return value on failure in case
> a machine check exception is encountered during the memcpy. The return
> value is the number of bytes remaining to be copied.
>
> This patch largely borrows from the copyuser_power7 logic and does not add
> the VMX optimizations, largely to keep the patch simple. If needed those
> optimizations can be folded in.
Shouldn't this patch go after the exception table stuff now if you
squashed them together?
Thanks,
Nick
^ permalink raw reply
* Re: [v5 5/6] powerpc/mce: Handle UE event for memcpy_mcsafe
From: Nicholas Piggin @ 2019-07-11 9:52 UTC (permalink / raw)
To: linux-kernel, linuxppc-dev, Santosh Sivaraj
Cc: Aneesh Kumar K.V, Mahesh Salgaonkar, Chandan Rajendra, Reza Arbab
In-Reply-To: <20190709121524.18762-6-santosh@fossix.org>
Santosh Sivaraj's on July 9, 2019 10:15 pm:
> If we take a UE on one of the instructions with a fixup entry, set nip
> to continue execution at the fixup entry. Stop processing the event
> further or print it.
So... what happens if we take a machine check while we happen to be
executing some other kernel operation with a fixup entry?
Or the other way around, what happens if memcpy_mcsafe takes a page
fault for some reason (e.g., kernel bug it tries to access unmapped
memory).
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH] mm/kprobes: Add generic kprobe_fault_handler() fallback definition
From: Anshuman Khandual @ 2019-07-11 9:58 UTC (permalink / raw)
To: linux-mm, Andrew Morton
Cc: Kate Stewart, Mark Rutland, Rich Felker, linux-ia64, linux-sh,
Heiko Carstens, linux-mips, Richard Fontana, Paul Mackerras,
H. Peter Anvin, sparclinux, Will Deacon, linux-s390,
Yoshinori Sato, x86, Russell King, Anil S Keshavamurthy,
Christian Borntraeger, Ingo Molnar, linux-arm-kernel,
Catalin Marinas, James Hogan, linux-snps-arc, Guenter Roeck,
Fenghua Yu, Vasily Gorbik, linuxppc-dev, Naveen N. Rao,
Borislav Petkov, Thomas Gleixner, Allison Randal, Tony Luck,
Vineet Gupta, linux-kernel, Ralf Baechle, Paul Burton,
Masami Hiramatsu, Greg Kroah-Hartman, Andrew Morton,
Enrico Weigelt, David S. Miller
In-Reply-To: <1562304629-29376-1-git-send-email-anshuman.khandual@arm.com>
On 07/05/2019 11:00 AM, Anshuman Khandual wrote:
> Architectures like parisc enable CONFIG_KROBES without having a definition
> for kprobe_fault_handler() which results in a build failure. Arch needs to
> provide kprobe_fault_handler() as it is platform specific and cannot have
> a generic working alternative. But in the event when platform lacks such a
> definition there needs to be a fallback.
>
> This adds a stub kprobe_fault_handler() definition which not only prevents
> a build failure but also makes sure that kprobe_page_fault() if called will
> always return negative in absence of a sane platform specific alternative.
>
> While here wrap kprobe_page_fault() in CONFIG_KPROBES. This enables stud
> definitions for generic kporbe_fault_handler() and kprobes_built_in() can
> just be dropped. Only on x86 it needs to be added back locally as it gets
> used in a !CONFIG_KPROBES function do_general_protection().
>
> Cc: Vineet Gupta <vgupta@synopsys.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Fenghua Yu <fenghua.yu@intel.com>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Paul Burton <paul.burton@mips.com>
> Cc: James Hogan <jhogan@kernel.org>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> Cc: Vasily Gorbik <gor@linux.ibm.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
> Cc: Rich Felker <dalias@libc.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: "Naveen N. Rao" <naveen.n.rao@linux.ibm.com>
> Cc: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Allison Randal <allison@lohutok.net>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Enrico Weigelt <info@metux.net>
> Cc: Richard Fontana <rfontana@redhat.com>
> Cc: Kate Stewart <kstewart@linuxfoundation.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: x86@kernel.org
> Cc: linux-snps-arc@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-ia64@vger.kernel.org
> Cc: linux-mips@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-s390@vger.kernel.org
> Cc: linux-sh@vger.kernel.org
> Cc: sparclinux@vger.kernel.org
>
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
Any updates or suggestions on this patch ? Currently there is a build failure on
parisc architecture due to the lack of a kprobe_fault_handler() definition when
CONFIG_KPROBES is enabled and this build failure needs to be fixed.
This patch solves the build problem. But otherwise I am also happy to just define
a stub definition for kprobe_fault_handler() on parisc arch when CONFIG_KPROBES
is enabled, which will avoid the build failure. Please suggest.
^ permalink raw reply
* [PATCH V4 0/2] recover the channel swap after xrun
From: shengjiu.wang @ 2019-07-11 10:49 UTC (permalink / raw)
To: timur, nicoleotsuka, Xiubo.Lee, festevam, lgirdwood, perex, tiwai,
broonie, alsa-devel
Cc: linuxppc-dev, linux-kernel
From: Shengjiu Wang <shengjiu.wang@nxp.com>
recover the channel swap after xrun
Shengjiu Wang (2):
ASoC: fsl_esai: Wrap some operations to be functions
ASoC: fsl_esai: recover the channel swap after xrun
sound/soc/fsl/fsl_esai.c | 266 ++++++++++++++++++++++++++++-----------
1 file changed, 193 insertions(+), 73 deletions(-)
Changes in v4
-add more comments and remove read saisr
Changes in V3
- update code sytle.
Changes in v2
- add one patch for wrap operations to functions.
- fix some coding style issue
--
2.21.0
^ permalink raw reply
* [PATCH V4 2/2] ASoC: fsl_esai: recover the channel swap after xrun
From: shengjiu.wang @ 2019-07-11 10:49 UTC (permalink / raw)
To: timur, nicoleotsuka, Xiubo.Lee, festevam, lgirdwood, perex, tiwai,
broonie, alsa-devel
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <cover.1562842206.git.shengjiu.wang@nxp.com>
From: Shengjiu Wang <shengjiu.wang@nxp.com>
There is chip errata ERR008000, the reference doc is
(https://www.nxp.com/docs/en/errata/IMX6DQCE.pdf),
The issue is "While using ESAI transmit or receive and
an underrun/overrun happens, channel swap may occur.
The only recovery mechanism is to reset the ESAI."
This issue exist in imx3/imx5/imx6(partial) series.
In this commit add a tasklet to handle reset of ESAI
after xrun happens to recover the channel swap.
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
sound/soc/fsl/fsl_esai.c | 74 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 74 insertions(+)
diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index ab460d6d7432..4ce8ac769244 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -32,6 +32,7 @@
* @extalclk: esai clock source to derive HCK, SCK and FS
* @fsysclk: system clock source to derive HCK, SCK and FS
* @spbaclk: SPBA clock (optional, depending on SoC design)
+ * @task: tasklet to handle the reset operation
* @fifo_depth: depth of tx/rx FIFO
* @slot_width: width of each DAI slot
* @slots: number of slots
@@ -42,6 +43,7 @@
* @sck_div: if using PSR/PM dividers for SCKx clock
* @slave_mode: if fully using DAI slave mode
* @synchronous: if using tx/rx synchronous mode
+ * @reset_at_xrun: flags for enable reset operaton
* @name: driver name
*/
struct fsl_esai {
@@ -53,6 +55,7 @@ struct fsl_esai {
struct clk *extalclk;
struct clk *fsysclk;
struct clk *spbaclk;
+ struct tasklet_struct task;
u32 fifo_depth;
u32 slot_width;
u32 slots;
@@ -65,6 +68,7 @@ struct fsl_esai {
bool sck_div[2];
bool slave_mode;
bool synchronous;
+ bool reset_at_xrun;
char name[32];
};
@@ -73,8 +77,16 @@ static irqreturn_t esai_isr(int irq, void *devid)
struct fsl_esai *esai_priv = (struct fsl_esai *)devid;
struct platform_device *pdev = esai_priv->pdev;
u32 esr;
+ u32 saisr;
regmap_read(esai_priv->regmap, REG_ESAI_ESR, &esr);
+ regmap_read(esai_priv->regmap, REG_ESAI_SAISR, &saisr);
+
+ if ((saisr & (ESAI_SAISR_TUE | ESAI_SAISR_ROE)) &&
+ esai_priv->reset_at_xrun) {
+ dev_dbg(&pdev->dev, "reset module for xrun\n");
+ tasklet_schedule(&esai_priv->task);
+ }
if (esr & ESAI_ESR_TINIT_MASK)
dev_dbg(&pdev->dev, "isr: Transmission Initialized\n");
@@ -635,10 +647,17 @@ static void fsl_esai_trigger_start(struct fsl_esai *esai_priv, bool tx)
ESAI_xSMB_xS_MASK, ESAI_xSMB_xS(mask));
regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMA(tx),
ESAI_xSMA_xS_MASK, ESAI_xSMA_xS(mask));
+
+ /* Enable Exception interrupt */
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
+ ESAI_xCR_xEIE_MASK, ESAI_xCR_xEIE);
}
static void fsl_esai_trigger_stop(struct fsl_esai *esai_priv, bool tx)
{
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
+ ESAI_xCR_xEIE_MASK, 0);
+
regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
tx ? ESAI_xCR_TE_MASK : ESAI_xCR_RE_MASK, 0);
regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMA(tx),
@@ -653,6 +672,51 @@ static void fsl_esai_trigger_stop(struct fsl_esai *esai_priv, bool tx)
ESAI_xFCR_xFR, 0);
}
+static void fsl_esai_hw_reset(unsigned long arg)
+{
+ struct fsl_esai *esai_priv = (struct fsl_esai *)arg;
+ bool tx = true, rx = false, enabled[2];
+ u32 tfcr, rfcr;
+
+ /* Save the registers */
+ regmap_read(esai_priv->regmap, REG_ESAI_TFCR, &tfcr);
+ regmap_read(esai_priv->regmap, REG_ESAI_RFCR, &rfcr);
+ enabled[tx] = tfcr & ESAI_xFCR_xFEN;
+ enabled[rx] = rfcr & ESAI_xFCR_xFEN;
+
+ /* Stop the tx & rx */
+ fsl_esai_trigger_stop(esai_priv, tx);
+ fsl_esai_trigger_stop(esai_priv, rx);
+
+ /* Reset the esai, and ignore return value */
+ fsl_esai_hw_init(esai_priv);
+
+ /* Enforce ESAI personal resets for both TX and RX */
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
+ ESAI_xCR_xPR_MASK, ESAI_xCR_xPR);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_RCR,
+ ESAI_xCR_xPR_MASK, ESAI_xCR_xPR);
+
+ /* Restore registers by regcache_sync, and ignore return value */
+ fsl_esai_register_restore(esai_priv);
+
+ /* Remove ESAI personal resets by configuring PCRC and PRRC also */
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
+ ESAI_xCR_xPR_MASK, 0);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_RCR,
+ ESAI_xCR_xPR_MASK, 0);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_PRRC,
+ ESAI_PRRC_PDC_MASK, ESAI_PRRC_PDC(ESAI_GPIO));
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_PCRC,
+ ESAI_PCRC_PC_MASK, ESAI_PCRC_PC(ESAI_GPIO));
+
+ /* Restart tx / rx, if they already enabled */
+ if (enabled[tx])
+ fsl_esai_trigger_start(esai_priv, tx);
+ if (enabled[rx])
+ fsl_esai_trigger_start(esai_priv, rx);
+}
+
static int fsl_esai_trigger(struct snd_pcm_substream *substream, int cmd,
struct snd_soc_dai *dai)
{
@@ -857,6 +921,10 @@ static int fsl_esai_probe(struct platform_device *pdev)
esai_priv->pdev = pdev;
snprintf(esai_priv->name, sizeof(esai_priv->name), "%pOFn", np);
+ if (of_device_is_compatible(np, "fsl,vf610-esai") ||
+ of_device_is_compatible(np, "fsl,imx35-esai"))
+ esai_priv->reset_at_xrun = true;
+
/* Get the addresses and IRQ */
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
regs = devm_ioremap_resource(&pdev->dev, res);
@@ -956,6 +1024,9 @@ static int fsl_esai_probe(struct platform_device *pdev)
return ret;
}
+ tasklet_init(&esai_priv->task, fsl_esai_hw_reset,
+ (unsigned long)esai_priv);
+
pm_runtime_enable(&pdev->dev);
regcache_cache_only(esai_priv->regmap, true);
@@ -969,7 +1040,10 @@ static int fsl_esai_probe(struct platform_device *pdev)
static int fsl_esai_remove(struct platform_device *pdev)
{
+ struct fsl_esai *esai_priv = platform_get_drvdata(pdev);
+
pm_runtime_disable(&pdev->dev);
+ tasklet_kill(&esai_priv->task);
return 0;
}
--
2.21.0
^ permalink raw reply related
* [PATCH V4 1/2] ASoC: fsl_esai: Wrap some operations to be functions
From: shengjiu.wang @ 2019-07-11 10:49 UTC (permalink / raw)
To: timur, nicoleotsuka, Xiubo.Lee, festevam, lgirdwood, perex, tiwai,
broonie, alsa-devel
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <cover.1562842206.git.shengjiu.wang@nxp.com>
From: Shengjiu Wang <shengjiu.wang@nxp.com>
Extract the operation to be functions, to improve the
readability.
In this patch, fsl_esai_hw_init, fsl_esai_register_restore,
fsl_esai_trigger_start and fsl_esai_trigger_stop are
extracted.
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
Acked-by: Nicolin Chen <nicoleotsuka@gmail.com>
---
sound/soc/fsl/fsl_esai.c | 192 ++++++++++++++++++++++++---------------
1 file changed, 119 insertions(+), 73 deletions(-)
diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index 10d2210c91ef..ab460d6d7432 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -35,6 +35,7 @@
* @fifo_depth: depth of tx/rx FIFO
* @slot_width: width of each DAI slot
* @slots: number of slots
+ * @channels: channel num for tx or rx
* @hck_rate: clock rate of desired HCKx clock
* @sck_rate: clock rate of desired SCKx clock
* @hck_dir: the direction of HCKx pads
@@ -57,6 +58,7 @@ struct fsl_esai {
u32 slots;
u32 tx_mask;
u32 rx_mask;
+ u32 channels[2];
u32 hck_rate[2];
u32 sck_rate[2];
bool hck_dir[2];
@@ -543,64 +545,132 @@ static int fsl_esai_hw_params(struct snd_pcm_substream *substream,
return 0;
}
-static int fsl_esai_trigger(struct snd_pcm_substream *substream, int cmd,
- struct snd_soc_dai *dai)
+static int fsl_esai_hw_init(struct fsl_esai *esai_priv)
{
- struct fsl_esai *esai_priv = snd_soc_dai_get_drvdata(dai);
- bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
- u8 i, channels = substream->runtime->channels;
+ struct platform_device *pdev = esai_priv->pdev;
+ int ret;
+
+ /* Reset ESAI unit */
+ ret = regmap_update_bits(esai_priv->regmap, REG_ESAI_ECR,
+ ESAI_ECR_ESAIEN_MASK | ESAI_ECR_ERST_MASK,
+ ESAI_ECR_ESAIEN | ESAI_ECR_ERST);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to reset ESAI: %d\n", ret);
+ return ret;
+ }
+
+ /*
+ * We need to enable ESAI so as to access some of its registers.
+ * Otherwise, we would fail to dump regmap from user space.
+ */
+ ret = regmap_update_bits(esai_priv->regmap, REG_ESAI_ECR,
+ ESAI_ECR_ESAIEN_MASK | ESAI_ECR_ERST_MASK,
+ ESAI_ECR_ESAIEN);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to enable ESAI: %d\n", ret);
+ return ret;
+ }
+
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_PRRC,
+ ESAI_PRRC_PDC_MASK, 0);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_PCRC,
+ ESAI_PCRC_PC_MASK, 0);
+
+ return 0;
+}
+
+static int fsl_esai_register_restore(struct fsl_esai *esai_priv)
+{
+ int ret;
+
+ /* FIFO reset for safety */
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TFCR,
+ ESAI_xFCR_xFR, ESAI_xFCR_xFR);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_RFCR,
+ ESAI_xFCR_xFR, ESAI_xFCR_xFR);
+
+ regcache_mark_dirty(esai_priv->regmap);
+ ret = regcache_sync(esai_priv->regmap);
+ if (ret)
+ return ret;
+
+ /* FIFO reset done */
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TFCR, ESAI_xFCR_xFR, 0);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_RFCR, ESAI_xFCR_xFR, 0);
+
+ return 0;
+}
+
+static void fsl_esai_trigger_start(struct fsl_esai *esai_priv, bool tx)
+{
+ u8 i, channels = esai_priv->channels[tx];
u32 pins = DIV_ROUND_UP(channels, esai_priv->slots);
u32 mask;
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_xFCR(tx),
+ ESAI_xFCR_xFEN_MASK, ESAI_xFCR_xFEN);
+
+ /* Write initial words reqiured by ESAI as normal procedure */
+ for (i = 0; tx && i < channels; i++)
+ regmap_write(esai_priv->regmap, REG_ESAI_ETDR, 0x0);
+
+ /*
+ * When set the TE/RE in the end of enablement flow, there
+ * will be channel swap issue for multi data line case.
+ * In order to workaround this issue, we switch the bit
+ * enablement sequence to below sequence
+ * 1) clear the xSMB & xSMA: which is done in probe and
+ * stop state.
+ * 2) set TE/RE
+ * 3) set xSMB
+ * 4) set xSMA: xSMA is the last one in this flow, which
+ * will trigger esai to start.
+ */
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
+ tx ? ESAI_xCR_TE_MASK : ESAI_xCR_RE_MASK,
+ tx ? ESAI_xCR_TE(pins) : ESAI_xCR_RE(pins));
+ mask = tx ? esai_priv->tx_mask : esai_priv->rx_mask;
+
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMB(tx),
+ ESAI_xSMB_xS_MASK, ESAI_xSMB_xS(mask));
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMA(tx),
+ ESAI_xSMA_xS_MASK, ESAI_xSMA_xS(mask));
+}
+
+static void fsl_esai_trigger_stop(struct fsl_esai *esai_priv, bool tx)
+{
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
+ tx ? ESAI_xCR_TE_MASK : ESAI_xCR_RE_MASK, 0);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMA(tx),
+ ESAI_xSMA_xS_MASK, 0);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMB(tx),
+ ESAI_xSMB_xS_MASK, 0);
+
+ /* Disable and reset FIFO */
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_xFCR(tx),
+ ESAI_xFCR_xFR | ESAI_xFCR_xFEN, ESAI_xFCR_xFR);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_xFCR(tx),
+ ESAI_xFCR_xFR, 0);
+}
+
+static int fsl_esai_trigger(struct snd_pcm_substream *substream, int cmd,
+ struct snd_soc_dai *dai)
+{
+ struct fsl_esai *esai_priv = snd_soc_dai_get_drvdata(dai);
+ bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
+
+ esai_priv->channels[tx] = substream->runtime->channels;
+
switch (cmd) {
case SNDRV_PCM_TRIGGER_START:
case SNDRV_PCM_TRIGGER_RESUME:
case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
- regmap_update_bits(esai_priv->regmap, REG_ESAI_xFCR(tx),
- ESAI_xFCR_xFEN_MASK, ESAI_xFCR_xFEN);
-
- /* Write initial words reqiured by ESAI as normal procedure */
- for (i = 0; tx && i < channels; i++)
- regmap_write(esai_priv->regmap, REG_ESAI_ETDR, 0x0);
-
- /*
- * When set the TE/RE in the end of enablement flow, there
- * will be channel swap issue for multi data line case.
- * In order to workaround this issue, we switch the bit
- * enablement sequence to below sequence
- * 1) clear the xSMB & xSMA: which is done in probe and
- * stop state.
- * 2) set TE/RE
- * 3) set xSMB
- * 4) set xSMA: xSMA is the last one in this flow, which
- * will trigger esai to start.
- */
- regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
- tx ? ESAI_xCR_TE_MASK : ESAI_xCR_RE_MASK,
- tx ? ESAI_xCR_TE(pins) : ESAI_xCR_RE(pins));
- mask = tx ? esai_priv->tx_mask : esai_priv->rx_mask;
-
- regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMB(tx),
- ESAI_xSMB_xS_MASK, ESAI_xSMB_xS(mask));
- regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMA(tx),
- ESAI_xSMA_xS_MASK, ESAI_xSMA_xS(mask));
-
+ fsl_esai_trigger_start(esai_priv, tx);
break;
case SNDRV_PCM_TRIGGER_SUSPEND:
case SNDRV_PCM_TRIGGER_STOP:
case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
- regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
- tx ? ESAI_xCR_TE_MASK : ESAI_xCR_RE_MASK, 0);
- regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMA(tx),
- ESAI_xSMA_xS_MASK, 0);
- regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMB(tx),
- ESAI_xSMB_xS_MASK, 0);
-
- /* Disable and reset FIFO */
- regmap_update_bits(esai_priv->regmap, REG_ESAI_xFCR(tx),
- ESAI_xFCR_xFR | ESAI_xFCR_xFEN, ESAI_xFCR_xFR);
- regmap_update_bits(esai_priv->regmap, REG_ESAI_xFCR(tx),
- ESAI_xFCR_xFR, 0);
+ fsl_esai_trigger_stop(esai_priv, tx);
break;
default:
return -EINVAL;
@@ -866,22 +936,9 @@ static int fsl_esai_probe(struct platform_device *pdev)
dev_set_drvdata(&pdev->dev, esai_priv);
- /* Reset ESAI unit */
- ret = regmap_write(esai_priv->regmap, REG_ESAI_ECR, ESAI_ECR_ERST);
- if (ret) {
- dev_err(&pdev->dev, "failed to reset ESAI: %d\n", ret);
+ ret = fsl_esai_hw_init(esai_priv);
+ if (ret)
return ret;
- }
-
- /*
- * We need to enable ESAI so as to access some of its registers.
- * Otherwise, we would fail to dump regmap from user space.
- */
- ret = regmap_write(esai_priv->regmap, REG_ESAI_ECR, ESAI_ECR_ESAIEN);
- if (ret) {
- dev_err(&pdev->dev, "failed to enable ESAI: %d\n", ret);
- return ret;
- }
esai_priv->tx_mask = 0xFFFFFFFF;
esai_priv->rx_mask = 0xFFFFFFFF;
@@ -955,20 +1012,10 @@ static int fsl_esai_runtime_resume(struct device *dev)
regcache_cache_only(esai->regmap, false);
- /* FIFO reset for safety */
- regmap_update_bits(esai->regmap, REG_ESAI_TFCR,
- ESAI_xFCR_xFR, ESAI_xFCR_xFR);
- regmap_update_bits(esai->regmap, REG_ESAI_RFCR,
- ESAI_xFCR_xFR, ESAI_xFCR_xFR);
-
- ret = regcache_sync(esai->regmap);
+ ret = fsl_esai_register_restore(esai);
if (ret)
goto err_regcache_sync;
- /* FIFO reset done */
- regmap_update_bits(esai->regmap, REG_ESAI_TFCR, ESAI_xFCR_xFR, 0);
- regmap_update_bits(esai->regmap, REG_ESAI_RFCR, ESAI_xFCR_xFR, 0);
-
return 0;
err_regcache_sync:
@@ -991,7 +1038,6 @@ static int fsl_esai_runtime_suspend(struct device *dev)
struct fsl_esai *esai = dev_get_drvdata(dev);
regcache_cache_only(esai->regmap, true);
- regcache_mark_dirty(esai->regmap);
if (!IS_ERR(esai->fsysclk))
clk_disable_unprepare(esai->fsysclk);
--
2.21.0
^ permalink raw reply related
* Re: [PATCH v3 5/7] kexec_elf: remove elf_addr_to_cpu macro
From: Michael Ellerman @ 2019-07-11 11:08 UTC (permalink / raw)
To: Sven Schnelle, Christophe Leroy; +Cc: kexec, deller, linuxppc-dev
In-Reply-To: <20190710180518.GA6343@t470p.stackframe.org>
Sven Schnelle <svens@stackframe.org> writes:
> On Wed, Jul 10, 2019 at 05:09:29PM +0200, Christophe Leroy wrote:
>> Le 10/07/2019 à 16:29, Sven Schnelle a écrit :
>> > It had only one definition, so just use the function directly.
>>
>> It had only one definition because it was for ppc64 only.
>> But as far as I understand (at least from the name of the new file), you
>> want it to be generic, don't you ? Therefore I get on 32 bits it would be
>> elf32_to_cpu().
>
> That brings up the question whether we need those endianess conversions. I would
> assume that the ELF file has always the same endianess as the running kernel. So
> i think we could just drop them. What do you think?
We should be able to kexec from big to little endian or vice versa, so
they are necessary.
cheers
^ permalink raw reply
* Re: [PATCH v4 1/8] KVM: PPC: Ultravisor: Introduce the MSR_S bit
From: Michael Ellerman @ 2019-07-11 12:57 UTC (permalink / raw)
To: Claudio Carvalho, linuxppc-dev
Cc: Madhavan Srinivasan, Michael Anderson, Ram Pai, Claudio Carvalho,
kvm-ppc, Bharata B Rao, Ryan Grimm, Sukadev Bhattiprolu,
Thiago Bauermann, Anshuman Khandual
In-Reply-To: <20190628200825.31049-2-cclaudio@linux.ibm.com>
Claudio Carvalho <cclaudio@linux.ibm.com> writes:
> From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
>
> The ultravisor processor mode is introduced in POWER platforms that
> supports the Protected Execution Facility (PEF). Ultravisor is higher
> privileged than hypervisor mode.
>
> In PEF enabled platforms, the MSR_S bit is used to indicate if the
> thread is in secure state. With the MSR_S bit, the privilege state of
> the thread is now determined by MSR_S, MSR_HV and MSR_PR, as follows:
>
> S HV PR
> -----------------------
> 0 x 1 problem
> 1 0 1 problem
> x x 0 privileged
> x 1 0 hypervisor
> 1 1 0 ultravisor
> 1 1 1 reserved
What are you trying to express with the 'x' value?
I guess you mean it as "either" or "don't care" - but then you have
cases where it could only have one value, such as hypervisor. I think it
would be clearer if you spelled it out more explicitly.
> The hypervisor doesn't (and can't) run with the MSR_S bit set, but a
> secure guest and the ultravisor firmware do.
I know you're trying to be helpful, but this comment is really just
confusing to someone who doesn't have all the documentation.
I'd really like to see something in Documentation/powerpc describing at
least the outline of how the system works. I'm pretty sure most of that
is public, so even if it's mostly a list of references to other
documentations or presentations that would be fine. But I'm not really
happy with a whole new processor mode appearing with zero documentation
in the tree.
> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> [ Update the commit message ]
It's normal to prefix these comments with your handle to make it clear
who is saying it.
> Signed-off-by: Claudio Carvalho <cclaudio@linux.ibm.com>
> ---
> arch/powerpc/include/asm/reg.h | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
> index 10caa145f98b..39b4c0a519f5 100644
> --- a/arch/powerpc/include/asm/reg.h
> +++ b/arch/powerpc/include/asm/reg.h
> @@ -38,6 +38,7 @@
> #define MSR_TM_LG 32 /* Trans Mem Available */
> #define MSR_VEC_LG 25 /* Enable AltiVec */
> #define MSR_VSX_LG 23 /* Enable VSX */
> +#define MSR_S_LG 22 /* Secure VM bit */
I don't think that's the best description, because it's also the
Ultravisor bit when MSR[HV] = 1.
So "Secure state" as you have below would be better IMO.
cheers
> #define MSR_POW_LG 18 /* Enable Power Management */
> #define MSR_WE_LG 18 /* Wait State Enable */
> #define MSR_TGPR_LG 17 /* TLB Update registers in use */
> @@ -71,11 +72,13 @@
> #define MSR_SF __MASK(MSR_SF_LG) /* Enable 64 bit mode */
> #define MSR_ISF __MASK(MSR_ISF_LG) /* Interrupt 64b mode valid on 630 */
> #define MSR_HV __MASK(MSR_HV_LG) /* Hypervisor state */
> +#define MSR_S __MASK(MSR_S_LG) /* Secure state */
> #else
> /* so tests for these bits fail on 32-bit */
> #define MSR_SF 0
> #define MSR_ISF 0
> #define MSR_HV 0
> +#define MSR_S 0
> #endif
>
> /*
> --
> 2.20.1
^ permalink raw reply
* Re: [PATCH v4 3/8] KVM: PPC: Ultravisor: Add generic ultravisor call handler
From: Michael Ellerman @ 2019-07-11 12:57 UTC (permalink / raw)
To: Claudio Carvalho, linuxppc-dev
Cc: Madhavan Srinivasan, Michael Anderson, Ram Pai, Claudio Carvalho,
kvm-ppc, Bharata B Rao, Ryan Grimm, Sukadev Bhattiprolu,
Thiago Bauermann, Anshuman Khandual
In-Reply-To: <20190628200825.31049-4-cclaudio@linux.ibm.com>
Claudio Carvalho <cclaudio@linux.ibm.com> writes:
> From: Ram Pai <linuxram@us.ibm.com>
>
> Add the ucall() function, which can be used to make ultravisor calls
> with varied number of in and out arguments. Ultravisor calls can be made
> from the host or guests.
>
> This copies the implementation of plpar_hcall().
.. with quite a few changes?
This is one of the things I'd like to see in a Documentation file, so
that people can review the implementation vs the specification.
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> [ Change ucall.S to not save CR, rename and move headers, build ucall.S
> if CONFIG_PPC_POWERNV set, use R3 for the ucall number and add some
> comments in the code ]
Why are we not saving CR? See previous comment about Documentation :)
> Signed-off-by: Claudio Carvalho <cclaudio@linux.ibm.com>
> ---
> arch/powerpc/include/asm/ultravisor-api.h | 20 +++++++++++++++
> arch/powerpc/include/asm/ultravisor.h | 20 +++++++++++++++
> arch/powerpc/kernel/Makefile | 2 +-
> arch/powerpc/kernel/ucall.S | 30 +++++++++++++++++++++++
> arch/powerpc/kernel/ultravisor.c | 4 +++
> 5 files changed, 75 insertions(+), 1 deletion(-)
> create mode 100644 arch/powerpc/include/asm/ultravisor-api.h
> create mode 100644 arch/powerpc/kernel/ucall.S
>
> diff --git a/arch/powerpc/include/asm/ultravisor-api.h b/arch/powerpc/include/asm/ultravisor-api.h
> new file mode 100644
> index 000000000000..49e766adabc7
> --- /dev/null
> +++ b/arch/powerpc/include/asm/ultravisor-api.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Ultravisor API.
> + *
> + * Copyright 2019, IBM Corporation.
> + *
> + */
> +#ifndef _ASM_POWERPC_ULTRAVISOR_API_H
> +#define _ASM_POWERPC_ULTRAVISOR_API_H
> +
> +#include <asm/hvcall.h>
> +
> +/* Return codes */
> +#define U_NOT_AVAILABLE H_NOT_AVAILABLE
> +#define U_SUCCESS H_SUCCESS
> +#define U_FUNCTION H_FUNCTION
> +#define U_PARAMETER H_PARAMETER
Is there any benefit in redefining these?
> diff --git a/arch/powerpc/include/asm/ultravisor.h b/arch/powerpc/include/asm/ultravisor.h
> index e5009b0d84ea..a78a2dacfd0b 100644
> --- a/arch/powerpc/include/asm/ultravisor.h
> +++ b/arch/powerpc/include/asm/ultravisor.h
> @@ -8,8 +8,28 @@
> #ifndef _ASM_POWERPC_ULTRAVISOR_H
> #define _ASM_POWERPC_ULTRAVISOR_H
>
> +#include <asm/ultravisor-api.h>
> +
> +#if !defined(__ASSEMBLY__)
Just #ifndef is fine.
> /* Internal functions */
How is it internal?
> extern int early_init_dt_scan_ultravisor(unsigned long node, const char *uname,
> int depth, void *data);
>
> +/* API functions */
> +#define UCALL_BUFSIZE 4
Please don't copy this design from the hcall code, it has led to bugs in
the past.
See my (still unmerged) attempt to fix this for the hcall case:
https://patchwork.ozlabs.org/patch/683577/
Basically instead of asking callers nicely to define a certain sized
buffer, and them forgetting, define a proper type that has the right size.
> +/**
> + * ucall: Make a powerpc ultravisor call.
> + * @opcode: The ultravisor call to make.
> + * @retbuf: Buffer to store up to 4 return arguments in.
> + *
> + * This call supports up to 6 arguments and 4 return arguments. Use
> + * UCALL_BUFSIZE to size the return argument buffer.
> + */
> +#if defined(CONFIG_PPC_POWERNV)
#ifdef
> +long ucall(unsigned long opcode, unsigned long *retbuf, ...);
> +#endif
> +
> +#endif /* !__ASSEMBLY__ */
> +
> #endif /* _ASM_POWERPC_ULTRAVISOR_H */
> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> index f0caa302c8c0..f28baccc0a79 100644
> --- a/arch/powerpc/kernel/Makefile
> +++ b/arch/powerpc/kernel/Makefile
> @@ -154,7 +154,7 @@ endif
>
> obj-$(CONFIG_EPAPR_PARAVIRT) += epapr_paravirt.o epapr_hcalls.o
> obj-$(CONFIG_KVM_GUEST) += kvm.o kvm_emul.o
> -obj-$(CONFIG_PPC_POWERNV) += ultravisor.o
> +obj-$(CONFIG_PPC_POWERNV) += ultravisor.o ucall.o
Same comment about being platforms/powernv ?
>
> diff --git a/arch/powerpc/kernel/ucall.S b/arch/powerpc/kernel/ucall.S
> new file mode 100644
> index 000000000000..1678f6eb7230
> --- /dev/null
> +++ b/arch/powerpc/kernel/ucall.S
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Generic code to perform an ultravisor call.
> + *
> + * Copyright 2019, IBM Corporation.
> + *
> + */
> +#include <asm/ppc_asm.h>
> +
> +/*
> + * This function is based on the plpar_hcall()
I don't think it meaningfully is any more.
> + */
> +_GLOBAL_TOC(ucall)
You don't need the TOC setup here (unless a later patch does?).
> + std r4,STK_PARAM(R4)(r1) /* Save ret buffer */
> + mr r4,r5
> + mr r5,r6
> + mr r6,r7
> + mr r7,r8
> + mr r8,r9
> + mr r9,r10
Below you space the arguments, here you don't. Pick one or the other please.
> +
> + sc 2 /* Invoke the ultravisor */
> +
> + ld r12,STK_PARAM(R4)(r1)
> + std r4, 0(r12)
> + std r5, 8(r12)
> + std r6, 16(r12)
> + std r7, 24(r12)
> +
> + blr /* Return r3 = status */
> diff --git a/arch/powerpc/kernel/ultravisor.c b/arch/powerpc/kernel/ultravisor.c
> index dc6021f63c97..02ddf79a9522 100644
> --- a/arch/powerpc/kernel/ultravisor.c
> +++ b/arch/powerpc/kernel/ultravisor.c
> @@ -8,10 +8,14 @@
> #include <linux/init.h>
> #include <linux/printk.h>
> #include <linux/string.h>
> +#include <linux/export.h>
>
> #include <asm/ultravisor.h>
> #include <asm/firmware.h>
>
> +/* in ucall.S */
> +EXPORT_SYMBOL_GPL(ucall);
This should be in ucall.S
cheers
^ permalink raw reply
* Re: [PATCH v4 2/8] powerpc: Introduce FW_FEATURE_ULTRAVISOR
From: Michael Ellerman @ 2019-07-11 12:57 UTC (permalink / raw)
To: Claudio Carvalho, linuxppc-dev
Cc: Madhavan Srinivasan, Michael Anderson, Ram Pai, Claudio Carvalho,
kvm-ppc, Bharata B Rao, Ryan Grimm, Sukadev Bhattiprolu,
Thiago Bauermann, Anshuman Khandual
In-Reply-To: <20190628200825.31049-3-cclaudio@linux.ibm.com>
Claudio Carvalho <cclaudio@linux.ibm.com> writes:
> diff --git a/arch/powerpc/include/asm/ultravisor.h b/arch/powerpc/include/asm/ultravisor.h
> new file mode 100644
> index 000000000000..e5009b0d84ea
> --- /dev/null
> +++ b/arch/powerpc/include/asm/ultravisor.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Ultravisor definitions
> + *
> + * Copyright 2019, IBM Corporation.
> + *
> + */
> +#ifndef _ASM_POWERPC_ULTRAVISOR_H
> +#define _ASM_POWERPC_ULTRAVISOR_H
> +
> +/* Internal functions */
> +extern int early_init_dt_scan_ultravisor(unsigned long node, const char *uname,
> + int depth, void *data);
Please don't use extern in new headers.
> diff --git a/arch/powerpc/kernel/ultravisor.c b/arch/powerpc/kernel/ultravisor.c
> new file mode 100644
> index 000000000000..dc6021f63c97
> --- /dev/null
> +++ b/arch/powerpc/kernel/ultravisor.c
Is there a reason this (and other later files) aren't in platforms/powernv ?
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Ultravisor high level interfaces
> + *
> + * Copyright 2019, IBM Corporation.
> + *
> + */
> +#include <linux/init.h>
> +#include <linux/printk.h>
> +#include <linux/string.h>
> +
> +#include <asm/ultravisor.h>
> +#include <asm/firmware.h>
> +
> +int __init early_init_dt_scan_ultravisor(unsigned long node, const char *uname,
> + int depth, void *data)
> +{
> + if (depth != 1 || strcmp(uname, "ibm,ultravisor") != 0)
> + return 0;
I know you're following the example of OPAL, but this is not the best
way to search for the ultravisor node.
It makes the location and name of the node part of the ABI, when there's
no need for it to be.
If instead you just scan the tree for a node that is *compatible* with
"ibm,ultravisor" (or whatever compatible string) then the node can be
placed any where in the tree and have any name, which gives us the most
flexibility in future to change the location of the device tree node.
cheers
^ permalink raw reply
* Re: [PATCH v4 4/8] KVM: PPC: Ultravisor: Use UV_WRITE_PATE ucall to register a PATE
From: Michael Ellerman @ 2019-07-11 12:57 UTC (permalink / raw)
To: Claudio Carvalho, linuxppc-dev
Cc: Ryan Grimm, Madhavan Srinivasan, Michael Anderson, Ram Pai,
Claudio Carvalho, kvm-ppc, Bharata B Rao, Ryan Grimm,
Sukadev Bhattiprolu, Thiago Bauermann, Anshuman Khandual
In-Reply-To: <20190628200825.31049-5-cclaudio@linux.ibm.com>
Claudio Carvalho <cclaudio@linux.ibm.com> writes:
> From: Michael Anderson <andmike@linux.ibm.com>
>
> When running under an ultravisor, the ultravisor controls the real
> partition table and has it in secure memory where the hypervisor can't
> access it, and therefore we (the HV) have to do a ucall whenever we want
> to update an entry.
>
> The HV still keeps a copy of its view of the partition table in normal
> memory so that the nest MMU can access it.
>
> Both partition tables will have PATE entries for HV and normal virtual
Can you expand novel acronyms on their first usage please, ie. PATE.
> machines.
>
> Suggested-by: Ryan Grimm <grimm@linux.vnet.ibm.com>
"Suggested" implies this is optional, but it's not :)
I'm not sure exactly what Ryan's input was, but feel free to make up a
better tag :)
> Signed-off-by: Michael Anderson <andmike@linux.ibm.com>
> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> [ Write the pate in HV's table before doing that in UV's ]
> Signed-off-by: Claudio Carvalho <cclaudio@linux.ibm.com>
> ---
> arch/powerpc/include/asm/ultravisor-api.h | 5 +++-
> arch/powerpc/include/asm/ultravisor.h | 14 ++++++++++
> arch/powerpc/mm/book3s64/hash_utils.c | 3 +-
> arch/powerpc/mm/book3s64/pgtable.c | 34 +++++++++++++++++++++--
> arch/powerpc/mm/book3s64/radix_pgtable.c | 9 ++++--
> 5 files changed, 57 insertions(+), 8 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/ultravisor-api.h b/arch/powerpc/include/asm/ultravisor-api.h
> index 49e766adabc7..141940771add 100644
> --- a/arch/powerpc/include/asm/ultravisor-api.h
> +++ b/arch/powerpc/include/asm/ultravisor-api.h
> @@ -15,6 +15,9 @@
> #define U_SUCCESS H_SUCCESS
> #define U_FUNCTION H_FUNCTION
> #define U_PARAMETER H_PARAMETER
> +#define U_PERMISSION H_PERMISSION
>
> -#endif /* _ASM_POWERPC_ULTRAVISOR_API_H */
What happened there? Just a diff artifact?
> +/* opcodes */
> +#define UV_WRITE_PATE 0xF104
>
> +#endif /* _ASM_POWERPC_ULTRAVISOR_API_H */
> diff --git a/arch/powerpc/include/asm/ultravisor.h b/arch/powerpc/include/asm/ultravisor.h
> index a78a2dacfd0b..996c1efd6c6d 100644
> --- a/arch/powerpc/include/asm/ultravisor.h
> +++ b/arch/powerpc/include/asm/ultravisor.h
> @@ -12,6 +12,8 @@
>
> #if !defined(__ASSEMBLY__)
>
> +#include <linux/types.h>
> +
> /* Internal functions */
> extern int early_init_dt_scan_ultravisor(unsigned long node, const char *uname,
> int depth, void *data);
> @@ -28,8 +30,20 @@ extern int early_init_dt_scan_ultravisor(unsigned long node, const char *uname,
> */
> #if defined(CONFIG_PPC_POWERNV)
> long ucall(unsigned long opcode, unsigned long *retbuf, ...);
> +#else
> +static long ucall(unsigned long opcode, unsigned long *retbuf, ...)
^
inline
> +{
> + return U_NOT_AVAILABLE;
> +}
> #endif
That should have been in the previous patch.
> +static inline int uv_register_pate(u64 lpid, u64 dw0, u64 dw1)
> +{
> + unsigned long retbuf[UCALL_BUFSIZE];
> +
> + return ucall(UV_WRITE_PATE, retbuf, lpid, dw0, dw1);
You probably want a ucall_norets().
> +}
> +
> #endif /* !__ASSEMBLY__ */
>
> #endif /* _ASM_POWERPC_ULTRAVISOR_H */
> diff --git a/arch/powerpc/mm/book3s64/hash_utils.c b/arch/powerpc/mm/book3s64/hash_utils.c
> index 1ff451892d7f..220a4e133240 100644
> --- a/arch/powerpc/mm/book3s64/hash_utils.c
> +++ b/arch/powerpc/mm/book3s64/hash_utils.c
> @@ -1080,9 +1080,10 @@ void hash__early_init_mmu_secondary(void)
>
> if (!cpu_has_feature(CPU_FTR_ARCH_300))
> mtspr(SPRN_SDR1, _SDR1);
> - else
> + else if (!firmware_has_feature(FW_FEATURE_ULTRAVISOR))
> mtspr(SPRN_PTCR,
> __pa(partition_tb) | (PATB_SIZE_SHIFT - 12));
Needs a comment for the else case.
> }
> /* Initialize SLB */
> slb_initialize();
> diff --git a/arch/powerpc/mm/book3s64/pgtable.c b/arch/powerpc/mm/book3s64/pgtable.c
> index ad3dd977c22d..224c5c7c2e3d 100644
> --- a/arch/powerpc/mm/book3s64/pgtable.c
> +++ b/arch/powerpc/mm/book3s64/pgtable.c
> @@ -16,6 +16,8 @@
> #include <asm/tlb.h>
> #include <asm/trace.h>
> #include <asm/powernv.h>
> +#include <asm/firmware.h>
> +#include <asm/ultravisor.h>
>
> #include <mm/mmu_decl.h>
> #include <trace/events/thp.h>
> @@ -206,12 +208,25 @@ void __init mmu_partition_table_init(void)
> * 64 K size.
> */
> ptcr = __pa(partition_tb) | (PATB_SIZE_SHIFT - 12);
> - mtspr(SPRN_PTCR, ptcr);
> + /*
> + * If ultravisor is available, it is responsible for creating and
> + * managing partition table
> + */
But we just created the partition table?!
This comment and the one below would probably make more sense if they
were merged and appeared further up, where we allocate the partition
table.
> + if (!firmware_has_feature(FW_FEATURE_ULTRAVISOR))
> + mtspr(SPRN_PTCR, ptcr);
> +
> + /*
> + * Since nestMMU cannot access secure memory. Create
> + * and manage our own partition table. This table
But we just said the UV was responsible for it :)
> + * contains entries for nonsecure and hypervisor
> + * partition.
> + */
> powernv_set_nmmu_ptcr(ptcr);
> }
>
> -void mmu_partition_table_set_entry(unsigned int lpid, unsigned long dw0,
> - unsigned long dw1)
> +static void __mmu_partition_table_set_entry(unsigned int lpid,
> + unsigned long dw0,
> + unsigned long dw1)
> {
> unsigned long old = be64_to_cpu(partition_tb[lpid].patb0);
>
> @@ -238,6 +253,19 @@ void mmu_partition_table_set_entry(unsigned int lpid, unsigned long dw0,
> /* do we need fixup here ?*/
> asm volatile("eieio; tlbsync; ptesync" : : : "memory");
> }
> +
> +void mmu_partition_table_set_entry(unsigned int lpid, unsigned long dw0,
> + unsigned long dw1)
> +{
> + __mmu_partition_table_set_entry(lpid, dw0, dw1);
> +
> + if (firmware_has_feature(FW_FEATURE_ULTRAVISOR)) {
> + uv_register_pate(lpid, dw0, dw1);
> + pr_info("PATE registered by ultravisor: dw0 = 0x%lx, dw1 = 0x%lx\n",
> + dw0, dw1);
> + }
> +}
I agree with Alexey that this patch and the next should be merged and
the result cleaned up a bit.
> +
No extra blank please.
> EXPORT_SYMBOL_GPL(mmu_partition_table_set_entry);
>
> static pmd_t *get_pmd_from_cache(struct mm_struct *mm)
> diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c b/arch/powerpc/mm/book3s64/radix_pgtable.c
> index 8904aa1243d8..da6a6b76a040 100644
> --- a/arch/powerpc/mm/book3s64/radix_pgtable.c
> +++ b/arch/powerpc/mm/book3s64/radix_pgtable.c
> @@ -656,8 +656,10 @@ void radix__early_init_mmu_secondary(void)
> lpcr = mfspr(SPRN_LPCR);
> mtspr(SPRN_LPCR, lpcr | LPCR_UPRT | LPCR_HR);
>
> - mtspr(SPRN_PTCR,
> - __pa(partition_tb) | (PATB_SIZE_SHIFT - 12));
> + if (!firmware_has_feature(FW_FEATURE_ULTRAVISOR))
> + mtspr(SPRN_PTCR, __pa(partition_tb) |
> + (PATB_SIZE_SHIFT - 12));
> +
> radix_init_amor();
> }
>
> @@ -673,7 +675,8 @@ void radix__mmu_cleanup_all(void)
> if (!firmware_has_feature(FW_FEATURE_LPAR)) {
> lpcr = mfspr(SPRN_LPCR);
> mtspr(SPRN_LPCR, lpcr & ~LPCR_UPRT);
> - mtspr(SPRN_PTCR, 0);
> + if (!firmware_has_feature(FW_FEATURE_ULTRAVISOR))
> + mtspr(SPRN_PTCR, 0);
> powernv_set_nmmu_ptcr(0);
> radix__flush_tlb_all();
> }
There's four of these case where we skip touching the PTCR, which is
right on the borderline of warranting an accessor. I guess we can do it
as a cleanup later.
cheers
^ permalink raw reply
* Re: [PATCH v4 6/8] KVM: PPC: Ultravisor: Restrict LDBAR access
From: Michael Ellerman @ 2019-07-11 12:57 UTC (permalink / raw)
To: Claudio Carvalho, linuxppc-dev
Cc: Madhavan Srinivasan, Michael Anderson, Ram Pai, Claudio Carvalho,
kvm-ppc, Bharata B Rao, Ryan Grimm, Sukadev Bhattiprolu,
Thiago Bauermann, Anshuman Khandual
In-Reply-To: <20190628200825.31049-7-cclaudio@linux.ibm.com>
Claudio Carvalho <cclaudio@linux.ibm.com> writes:
> When the ultravisor firmware is available, it takes control over the
> LDBAR register. In this case, thread-imc updates and save/restore
> operations on the LDBAR register are handled by ultravisor.
Please roll up the replies to Alexey's question about LDBAR into the
change log.
> diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> index f9b2620fbecd..cffb365d9d02 100644
> --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> @@ -375,8 +375,10 @@ BEGIN_FTR_SECTION
> mtspr SPRN_RPR, r0
> ld r0, KVM_SPLIT_PMMAR(r6)
> mtspr SPRN_PMMAR, r0
> +BEGIN_FW_FTR_SECTION_NESTED(70)
> ld r0, KVM_SPLIT_LDBAR(r6)
> mtspr SPRN_LDBAR, r0
> +END_FW_FTR_SECTION_NESTED(FW_FEATURE_ULTRAVISOR, 0, 70)
That's in Power8 code isn't it? Which will never have an ultravisor.
> diff --git a/arch/powerpc/platforms/powernv/opal-imc.c b/arch/powerpc/platforms/powernv/opal-imc.c
> index 1b6932890a73..5fe2d4526cbc 100644
> --- a/arch/powerpc/platforms/powernv/opal-imc.c
> +++ b/arch/powerpc/platforms/powernv/opal-imc.c
> @@ -254,6 +254,10 @@ static int opal_imc_counters_probe(struct platform_device *pdev)
> bool core_imc_reg = false, thread_imc_reg = false;
> u32 type;
>
> + /* Disable IMC devices, when Ultravisor is enabled. */
> + if (firmware_has_feature(FW_FEATURE_ULTRAVISOR))
> + return -EACCES;
I don't mind taking this change. But at the same time should the IMC
stuff just be omitted from the device tree when we're in ultravisor mode?
cheers
^ permalink raw reply
* Re: [PATCH v4 7/8] KVM: PPC: Ultravisor: Enter a secure guest
From: Michael Ellerman @ 2019-07-11 12:57 UTC (permalink / raw)
To: Claudio Carvalho, linuxppc-dev
Cc: Madhavan Srinivasan, Michael Anderson, Ram Pai, Claudio Carvalho,
kvm-ppc, Bharata B Rao, Ryan Grimm, Sukadev Bhattiprolu,
Thiago Bauermann, Anshuman Khandual
In-Reply-To: <20190628200825.31049-8-cclaudio@linux.ibm.com>
Claudio Carvalho <cclaudio@linux.ibm.com> writes:
> From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
>
> To enter a secure guest, we have to go through the ultravisor, therefore
> we do a ucall when we are entering a secure guest.
>
> This change is needed for any sort of entry to the secure guest from the
> hypervisor, whether it is a return from an hcall, a return from a
> hypervisor interrupt, or the first time that a secure guest vCPU is run.
>
> If we are returning from an hcall, the results are already in the
> appropriate registers R3:12, except for R3, R6 and R7. R3 has the status
> of the reflected hcall, therefore we move it to R0 for the ultravisor and
> set R3 to the UV_RETURN ucall number. R6,7 were used as temporary
> registers, hence we restore them.
This is another case where some documentation would help people to
review the code.
> Have fast_guest_return check the kvm_arch.secure_guest field so that a
> new CPU enters UV when started (in response to a RTAS start-cpu call).
>
> Thanks to input from Paul Mackerras, Ram Pai and Mike Anderson.
>
> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> [ Pass SRR1 in r11 for UV_RETURN, fix kvmppc_msr_interrupt to preserve
> the MSR_S bit ]
> Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
> [ Fix UV_RETURN ucall number and arch.secure_guest check ]
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> [ Save the actual R3 in R0 for the ultravisor and use R3 for the
> UV_RETURN ucall number. Update commit message and ret_to_ultra comment ]
> Signed-off-by: Claudio Carvalho <cclaudio@linux.ibm.com>
> ---
> arch/powerpc/include/asm/kvm_host.h | 1 +
> arch/powerpc/include/asm/ultravisor-api.h | 1 +
> arch/powerpc/kernel/asm-offsets.c | 1 +
> arch/powerpc/kvm/book3s_hv_rmhandlers.S | 40 +++++++++++++++++++----
> 4 files changed, 37 insertions(+), 6 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> index cffb365d9d02..89813ca987c2 100644
> --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> @@ -36,6 +36,7 @@
> #include <asm/asm-compat.h>
> #include <asm/feature-fixups.h>
> #include <asm/cpuidle.h>
> +#include <asm/ultravisor-api.h>
>
> /* Sign-extend HDEC if not on POWER9 */
> #define EXTEND_HDEC(reg) \
> @@ -1092,16 +1093,12 @@ BEGIN_FTR_SECTION
> END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>
> ld r5, VCPU_LR(r4)
> - ld r6, VCPU_CR(r4)
> mtlr r5
> - mtcr r6
>
> ld r1, VCPU_GPR(R1)(r4)
> ld r2, VCPU_GPR(R2)(r4)
> ld r3, VCPU_GPR(R3)(r4)
> ld r5, VCPU_GPR(R5)(r4)
> - ld r6, VCPU_GPR(R6)(r4)
> - ld r7, VCPU_GPR(R7)(r4)
> ld r8, VCPU_GPR(R8)(r4)
> ld r9, VCPU_GPR(R9)(r4)
> ld r10, VCPU_GPR(R10)(r4)
> @@ -1119,10 +1116,38 @@ BEGIN_FTR_SECTION
> mtspr SPRN_HDSISR, r0
> END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
>
> + ld r6, VCPU_KVM(r4)
> + lbz r7, KVM_SECURE_GUEST(r6)
> + cmpdi r7, 0
You could hoist the load of r6 and r7 to here?
> + bne ret_to_ultra
> +
> + lwz r6, VCPU_CR(r4)
> + mtcr r6
> +
> + ld r7, VCPU_GPR(R7)(r4)
> + ld r6, VCPU_GPR(R6)(r4)
> ld r0, VCPU_GPR(R0)(r4)
> ld r4, VCPU_GPR(R4)(r4)
> HRFI_TO_GUEST
> b .
> +/*
> + * We are entering a secure guest, so we have to invoke the ultravisor to do
> + * that. If we are returning from a hcall, the results are already in the
> + * appropriate registers R3:12, except for R3, R6 and R7. R3 has the status of
> + * the reflected hcall, therefore we move it to R0 for the ultravisor and set
> + * R3 to the UV_RETURN ucall number. R6,7 were used as temporary registers
> + * above, hence we restore them.
> + */
> +ret_to_ultra:
> + lwz r6, VCPU_CR(r4)
> + mtcr r6
> + mfspr r11, SPRN_SRR1
> + mr r0, r3
> + LOAD_REG_IMMEDIATE(r3, UV_RETURN)
Worth open coding to save three instructions?
> + ld r7, VCPU_GPR(R7)(r4)
> + ld r6, VCPU_GPR(R6)(r4)
> + ld r4, VCPU_GPR(R4)(r4)
> + sc 2
>
> /*
> * Enter the guest on a P9 or later system where we have exactly
> @@ -3318,13 +3343,16 @@ END_MMU_FTR_SECTION_IFSET(MMU_FTR_TYPE_RADIX)
> * r0 is used as a scratch register
> */
> kvmppc_msr_interrupt:
> + andis. r0, r11, MSR_S@h
> rldicl r0, r11, 64 - MSR_TS_S_LG, 62
> - cmpwi r0, 2 /* Check if we are in transactional state.. */
> + cmpwi cr1, r0, 2 /* Check if we are in transactional state.. */
> ld r11, VCPU_INTR_MSR(r9)
> - bne 1f
> + bne cr1, 1f
> /* ... if transactional, change to suspended */
> li r0, 1
> 1: rldimi r11, r0, MSR_TS_S_LG, 63 - MSR_TS_T_LG
> + beqlr
> + oris r11, r11, MSR_S@h /* preserve MSR_S bit setting */
> blr
I don't see this part mentioned in the change log?
It's also pretty subtle, a comment might be helpful.
cheers
^ permalink raw reply
* Re: [PATCH v4 8/8] KVM: PPC: Ultravisor: Check for MSR_S during hv_reset_msr
From: Michael Ellerman @ 2019-07-11 12:57 UTC (permalink / raw)
To: Claudio Carvalho, linuxppc-dev
Cc: Madhavan Srinivasan, Michael Anderson, Ram Pai, Claudio Carvalho,
kvm-ppc, Bharata B Rao, Ryan Grimm, Sukadev Bhattiprolu,
Thiago Bauermann, Anshuman Khandual
In-Reply-To: <20190628200825.31049-9-cclaudio@linux.ibm.com>
Claudio Carvalho <cclaudio@linux.ibm.com> writes:
> From: Michael Anderson <andmike@linux.ibm.com>
>
> - Check for MSR_S so that kvmppc_set_msr will include it. Prior to this
> change return to guest would not have the S bit set.
That sounds like it would be bad?
Please spell out what the practical impact of the patch is, ie.
somewhere on the spectrum from "without this patch everything catches
fire", to "this is not a bug but makes things clearer because ..."
cheers
> - Patch based on comment from Paul Mackerras <pmac@au1.ibm.com>
>
> Signed-off-by: Michael Anderson <andmike@linux.ibm.com>
> Signed-off-by: Claudio Carvalho <cclaudio@linux.ibm.com>
> Acked-by: Paul Mackerras <paulus@ozlabs.org>
> ---
> arch/powerpc/kvm/book3s_64_mmu_hv.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> index ab3d484c5e2e..ab62a66f9b4e 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> @@ -295,6 +295,7 @@ static void kvmppc_mmu_book3s_64_hv_reset_msr(struct kvm_vcpu *vcpu)
> msr |= MSR_TS_S;
> else
> msr |= vcpu->arch.shregs.msr & MSR_TS_MASK;
> + msr |= vcpu->arch.shregs.msr & MSR_S;
> kvmppc_set_msr(vcpu, msr);
> }
>
> --
> 2.20.1
^ permalink raw reply
* [PATCH -next] powerpc/powernv/ioda: using kfree_rcu() to simplify the code
From: YueHaibing @ 2019-07-11 14:18 UTC (permalink / raw)
To: benh, paulus, mpe, aik, david; +Cc: YueHaibing, linuxppc-dev, linux-kernel
The callback function of call_rcu() just calls a kfree(), so we
can use kfree_rcu() instead of call_rcu() + callback function.
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
arch/powerpc/platforms/powernv/pci-ioda-tce.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/pci-ioda-tce.c b/arch/powerpc/platforms/powernv/pci-ioda-tce.c
index e28f03e..05f80b1 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda-tce.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda-tce.c
@@ -332,14 +332,6 @@ long pnv_pci_ioda2_table_alloc_pages(int nid, __u64 bus_offset,
return -ENOMEM;
}
-static void pnv_iommu_table_group_link_free(struct rcu_head *head)
-{
- struct iommu_table_group_link *tgl = container_of(head,
- struct iommu_table_group_link, rcu);
-
- kfree(tgl);
-}
-
void pnv_pci_unlink_table_and_group(struct iommu_table *tbl,
struct iommu_table_group *table_group)
{
@@ -355,7 +347,7 @@ void pnv_pci_unlink_table_and_group(struct iommu_table *tbl,
list_for_each_entry_rcu(tgl, &tbl->it_group_list, next) {
if (tgl->table_group == table_group) {
list_del_rcu(&tgl->next);
- call_rcu(&tgl->rcu, pnv_iommu_table_group_link_free);
+ kfree_rcu(tgl, rcu);
found = true;
break;
}
--
2.7.4
^ permalink raw reply related
* Re: Coccinelle: Checking of_node_put() calls with SmPL
From: Tyrel Datwyler @ 2019-07-11 14:41 UTC (permalink / raw)
To: wen.yang99, Markus.Elfring, julia.lawall
Cc: wang.yi59, linux-pm, rjw, daniel.lezcano, kernel-janitors,
linux-kernel, oss, paulus, xue.zhihong, cheng.shengyu,
linuxppc-dev
In-Reply-To: <201907111435459627761@zte.com.cn>
On 07/10/2019 11:35 PM, wen.yang99@zte.com.cn wrote:
>>> we developed a coccinelle script to detect such problems.
>>
>> Would you find the implementation of the function “dt_init_idle_driver”
>> suspicious according to discussed source code search patterns?
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/cpuidle/dt_idle_states.c?id=e9a83bd2322035ed9d7dcf35753d3f984d76c6a5#n208
>> https://elixir.bootlin.com/linux/v5.2/source/drivers/cpuidle/dt_idle_states.c#L208
>>
>>
>>> This script is still being improved.
>>
>> Will corresponding software development challenges become more interesting?
>
> Hello Markus,
> This is the simplified code pattern for it:
>
> 172 for (i = 0; ; i++) {
This loop can only be exited on a break.
> 173 state_node = of_parse_phandle(...); ---> Obtain here
> ...
> 177 match_id = of_match_node(matches, state_node);
> 178 if (!match_id) {
> 179 err = -ENODEV;
> 180 break; ---> Jump out of the loop without releasing it
> 181 }
> 182
> 183 if (!of_device_is_available(state_node)) {
> 184 of_node_put(state_node);
> 185 continue; ---> Release the object references within a loop
> 186 }
> ...
> 208 of_node_put(state_node); --> Release the object references within a loop
This is required at the end of every loop or continue to free the reference.
Only a break will exit the loop where we hit the below of_node_put().
> 209 }
> 210
> 211 of_node_put(state_node); --> There may be double free here.
None of the break conditions call of_node_put(), so it needs to be called here.
-Tyrel
>
> This code pattern is very interesting and the coccinelle software should also recognize this pattern.
>
> Regards,
> Wen
>
^ permalink raw reply
* [PATCH] powerpc/nvdimm: Pick the nearby online node if the device node is not online
From: Aneesh Kumar K.V @ 2019-07-11 14:56 UTC (permalink / raw)
To: npiggin, paulus, mpe
Cc: Oliver O'Halloran, linuxppc-dev, Aneesh Kumar K.V
This is similar to what ACPI does. Nvdimm layer doesn't bring the SCM device
numa node online. Hence we need to make sure we always use an online node
as ndr_desc.numa_node. Otherwise this result in kernel crashes. The target
node is used by dax/kmem and that will bring up the numa node online correctly.
Without this patch, we do hit kernel crash as below because we try to access
uninitialized NODE_DATA in different code paths.
cpu 0x0: Vector: 300 (Data Access) at [c0000000fac53170]
pc: c0000000004bbc50: ___slab_alloc+0x120/0xca0
lr: c0000000004bc834: __slab_alloc+0x64/0xc0
sp: c0000000fac53400
msr: 8000000002009033
dar: 73e8
dsisr: 80000
current = 0xc0000000fabb6d80
paca = 0xc000000003870000 irqmask: 0x03 irq_happened: 0x01
pid = 7, comm = kworker/u16:0
Linux version 5.2.0-06234-g76bd729b2644 (kvaneesh@ltc-boston123) (gcc version 7.4.0 (Ubuntu 7.4.0-1ubuntu1~18.04.1)) #135 SMP Thu Jul 11 05:36:30 CDT 2019
enter ? for help
[link register ] c0000000004bc834 __slab_alloc+0x64/0xc0
[c0000000fac53400] c0000000fac53480 (unreliable)
[c0000000fac53500] c0000000004bc818 __slab_alloc+0x48/0xc0
[c0000000fac53560] c0000000004c30a0 __kmalloc_node_track_caller+0x3c0/0x6b0
[c0000000fac535d0] c000000000cfafe4 devm_kmalloc+0x74/0xc0
[c0000000fac53600] c000000000d69434 nd_region_activate+0x144/0x560
[c0000000fac536d0] c000000000d6b19c nd_region_probe+0x17c/0x370
[c0000000fac537b0] c000000000d6349c nvdimm_bus_probe+0x10c/0x230
[c0000000fac53840] c000000000cf3cc4 really_probe+0x254/0x4e0
[c0000000fac538d0] c000000000cf429c driver_probe_device+0x16c/0x1e0
[c0000000fac53950] c000000000cf0b44 bus_for_each_drv+0x94/0x130
[c0000000fac539b0] c000000000cf392c __device_attach+0xdc/0x200
[c0000000fac53a50] c000000000cf231c bus_probe_device+0x4c/0xf0
[c0000000fac53a90] c000000000ced268 device_add+0x528/0x810
[c0000000fac53b60] c000000000d62a58 nd_async_device_register+0x28/0xa0
[c0000000fac53bd0] c0000000001ccb8c async_run_entry_fn+0xcc/0x1f0
[c0000000fac53c50] c0000000001bcd9c process_one_work+0x46c/0x860
[c0000000fac53d20] c0000000001bd4f4 worker_thread+0x364/0x5f0
[c0000000fac53db0] c0000000001c7260 kthread+0x1b0/0x1c0
[c0000000fac53e20] c00000000000b954 ret_from_kernel_thread+0x5c/0x68
With the patch we get
# numactl -H
available: 2 nodes (0-1)
node 0 cpus:
node 0 size: 0 MB
node 0 free: 0 MB
node 1 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31
node 1 size: 130865 MB
node 1 free: 129130 MB
node distances:
node 0 1
0: 10 20
1: 20 10
# cat /sys/bus/nd/devices/region0/numa_node
0
# dmesg | grep papr_scm
[ 91.332305] papr_scm ibm,persistent-memory:ibm,pmemory@44104001: Region registered with target node 2 and online node 0
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/platforms/pseries/papr_scm.c | 30 +++++++++++++++++++++--
1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index c8ec670ee924..4abb0ecda30a 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -255,12 +255,32 @@ static const struct attribute_group *papr_scm_dimm_groups[] = {
NULL,
};
+static inline int papr_scm_node(int node)
+{
+ int min_dist = INT_MAX, dist;
+ int nid, min_node;
+
+ if (node_online(node))
+ return node;
+
+ min_node = first_online_node;
+ for_each_online_node(nid) {
+ dist = node_distance(node, nid);
+ if (dist < min_dist) {
+ min_dist = dist;
+ min_node = nid;
+ }
+ }
+ return min_node;
+}
+
static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
{
struct device *dev = &p->pdev->dev;
struct nd_mapping_desc mapping;
struct nd_region_desc ndr_desc;
unsigned long dimm_flags;
+ int target_nid, online_nid;
p->bus_desc.ndctl = papr_scm_ndctl;
p->bus_desc.module = THIS_MODULE;
@@ -299,8 +319,11 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
memset(&ndr_desc, 0, sizeof(ndr_desc));
ndr_desc.attr_groups = region_attr_groups;
- ndr_desc.numa_node = dev_to_node(&p->pdev->dev);
- ndr_desc.target_node = ndr_desc.numa_node;
+ target_nid = dev_to_node(&p->pdev->dev);
+ online_nid = papr_scm_node(target_nid);
+ set_dev_node(&p->pdev->dev, online_nid);
+ ndr_desc.numa_node = online_nid;
+ ndr_desc.target_node = target_nid;
ndr_desc.res = &p->res;
ndr_desc.of_node = p->dn;
ndr_desc.provider_data = p;
@@ -318,6 +341,9 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
ndr_desc.res, p->dn);
goto err;
}
+ if (target_nid != online_nid)
+ dev_info(dev, "Region registered with target node %d and online node %d",
+ target_nid, online_nid);
return 0;
--
2.21.0
^ permalink raw reply related
* [PATCH v2] powerpc/book3s/mm: Update Oops message to print the correct translation in use
From: Aneesh Kumar K.V @ 2019-07-11 14:58 UTC (permalink / raw)
To: npiggin, paulus, mpe; +Cc: Aneesh Kumar K.V, linuxppc-dev
Avoids confusion when printing Oops message like below
Faulting instruction address: 0xc00000000008bdb4
Oops: Kernel access of bad area, sig: 11 [#1]
LE PAGE_SIZE=64K MMU=Radix MMU=Hash SMP NR_CPUS=2048 NUMA PowerNV
This was because we never clear the MMU_FTR_HPTE_TABLE feature flag
even if we run with radix translation. It was discussed that we should
look at this feature flag as an indication of the capability to run
hash translation and we should not clear the flag even if we run in
radix translation. All the code paths check for radix_enabled() check and
if found true consider we are running with radix translation. Follow the
same sequence for finding the MMU translation string to be used in Oops
message.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
Changes from V1:
* Don't clear the HPTE feature flag.
arch/powerpc/kernel/traps.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 11caa0291254..b181d6860f28 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -250,15 +250,22 @@ static void oops_end(unsigned long flags, struct pt_regs *regs,
}
NOKPROBE_SYMBOL(oops_end);
+static char *get_mmu_str(void)
+{
+ if (early_radix_enabled())
+ return " MMU=Radix";
+ if (early_mmu_has_feature(MMU_FTR_HPTE_TABLE))
+ return " MMU=Hash";
+ return "";
+}
+
static int __die(const char *str, struct pt_regs *regs, long err)
{
printk("Oops: %s, sig: %ld [#%d]\n", str, err, ++die_counter);
- printk("%s PAGE_SIZE=%luK%s%s%s%s%s%s%s %s\n",
+ printk("%s PAGE_SIZE=%luK%s%s%s%s%s%s %s\n",
IS_ENABLED(CONFIG_CPU_LITTLE_ENDIAN) ? "LE" : "BE",
- PAGE_SIZE / 1024,
- early_radix_enabled() ? " MMU=Radix" : "",
- early_mmu_has_feature(MMU_FTR_HPTE_TABLE) ? " MMU=Hash" : "",
+ PAGE_SIZE / 1024, get_mmu_str(),
IS_ENABLED(CONFIG_PREEMPT) ? " PREEMPT" : "",
IS_ENABLED(CONFIG_SMP) ? " SMP" : "",
IS_ENABLED(CONFIG_SMP) ? (" NR_CPUS=" __stringify(NR_CPUS)) : "",
--
2.21.0
^ permalink raw reply related
* [PATCH v3] tpm: tpm_ibm_vtpm: Fix unallocated banks
From: Nayna Jain @ 2019-07-11 16:13 UTC (permalink / raw)
To: linux-integrity, linuxppc-dev
Cc: Sachin Sant, Michal Suchanek, Nayna Jain, linux-kernel,
Jarkko Sakkinen, Jason Gunthorpe, Mimi Zohar, Peter Huewe,
George Wilson
The nr_allocated_banks and allocated banks are initialized as part of
tpm_chip_register. Currently, this is done as part of auto startup
function. However, some drivers, like the ibm vtpm driver, do not run
auto startup during initialization. This results in uninitialized memory
issue and causes a kernel panic during boot.
This patch moves the pcr allocation outside the auto startup function
into tpm_chip_register. This ensures that allocated banks are initialized
in any case.
Fixes: 879b589210a9 ("tpm: retrieve digest size of unknown algorithms with
PCR read")
Reported-by: Michal Suchanek <msuchanek@suse.de>
Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
Tested-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
Tested-by: Michal Suchánek <msuchanek@suse.de>
---
Changelog:
v3:
* Includes Stefan's feedback correctly:
* Fixed handling of rc > 0 error
* Includes Jarkko's feedback related to comment and the function.
v2:
* Includes Jarkko's feedbacks
* fixes the function name to tpm_get_pcr_allocation()
* adds new function tpm1_get_pcr_allocation()
* updates patch summary line
* fixes alignment
* adds Reported-by: Michal Suchanek <msuchanek@suse.de>
* Includes Stefan's feedbacks
* Fixes overwriting of return code
* Fixes misplacing of tpm_chip_stop()
* Adds Reviewed-by, Tested-by
drivers/char/tpm/tpm-chip.c | 20 ++++++++++++++++++++
drivers/char/tpm/tpm.h | 2 ++
drivers/char/tpm/tpm1-cmd.c | 36 ++++++++++++++++++++++++------------
drivers/char/tpm/tpm2-cmd.c | 6 +-----
4 files changed, 47 insertions(+), 17 deletions(-)
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 8804c9e916fd..5a0396d6560d 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -550,6 +550,20 @@ static int tpm_add_hwrng(struct tpm_chip *chip)
return hwrng_register(&chip->hwrng);
}
+static int tpm_get_pcr_allocation(struct tpm_chip *chip)
+{
+ int rc;
+
+ rc = (chip->flags & TPM_CHIP_FLAG_TPM2) ?
+ tpm2_get_pcr_allocation(chip) :
+ tpm1_get_pcr_allocation(chip);
+
+ if (rc > 0)
+ return -ENODEV;
+
+ return rc;
+}
+
/*
* tpm_chip_register() - create a character device for the TPM chip
* @chip: TPM chip to use.
@@ -569,6 +583,12 @@ int tpm_chip_register(struct tpm_chip *chip)
if (rc)
return rc;
rc = tpm_auto_startup(chip);
+ if (rc) {
+ tpm_chip_stop(chip);
+ return rc;
+ }
+
+ rc = tpm_get_pcr_allocation(chip);
tpm_chip_stop(chip);
if (rc)
return rc;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 2cce072f25b5..d571df3694c3 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -399,6 +399,7 @@ int tpm1_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf);
ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap,
const char *desc, size_t min_cap_length);
int tpm1_get_random(struct tpm_chip *chip, u8 *out, size_t max);
+int tpm1_get_pcr_allocation(struct tpm_chip *chip);
unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
int tpm_pm_suspend(struct device *dev);
int tpm_pm_resume(struct device *dev);
@@ -454,6 +455,7 @@ int tpm2_unseal_trusted(struct tpm_chip *chip,
ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,
u32 *value, const char *desc);
+ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip);
int tpm2_auto_startup(struct tpm_chip *chip);
void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type);
unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
index 85dcf2654d11..260a3917f0fe 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c
@@ -696,18 +696,6 @@ int tpm1_auto_startup(struct tpm_chip *chip)
goto out;
}
- chip->allocated_banks = kcalloc(1, sizeof(*chip->allocated_banks),
- GFP_KERNEL);
- if (!chip->allocated_banks) {
- rc = -ENOMEM;
- goto out;
- }
-
- chip->allocated_banks[0].alg_id = TPM_ALG_SHA1;
- chip->allocated_banks[0].digest_size = hash_digest_size[HASH_ALGO_SHA1];
- chip->allocated_banks[0].crypto_id = HASH_ALGO_SHA1;
- chip->nr_allocated_banks = 1;
-
return rc;
out:
if (rc > 0)
@@ -776,3 +764,27 @@ int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
return rc;
}
+/**
+ * tpm1_get_pcr_allocation() - initialize the allocated bank
+ * @chip: TPM chip to use.
+ *
+ * The function initializes the SHA1 allocated bank to extend PCR
+ *
+ * Return:
+ * * 0 on success,
+ * * < 0 on error.
+ */
+int tpm1_get_pcr_allocation(struct tpm_chip *chip)
+{
+ chip->allocated_banks = kcalloc(1, sizeof(*chip->allocated_banks),
+ GFP_KERNEL);
+ if (!chip->allocated_banks)
+ return -ENOMEM;
+
+ chip->allocated_banks[0].alg_id = TPM_ALG_SHA1;
+ chip->allocated_banks[0].digest_size = hash_digest_size[HASH_ALGO_SHA1];
+ chip->allocated_banks[0].crypto_id = HASH_ALGO_SHA1;
+ chip->nr_allocated_banks = 1;
+
+ return 0;
+}
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index e74c5b7b64bf..b4384d0e3741 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -841,7 +841,7 @@ struct tpm2_pcr_selection {
u8 pcr_select[3];
} __packed;
-static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
+ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
{
struct tpm2_pcr_selection pcr_selection;
struct tpm_buf buf;
@@ -1041,10 +1041,6 @@ int tpm2_auto_startup(struct tpm_chip *chip)
goto out;
}
- rc = tpm2_get_pcr_allocation(chip);
- if (rc)
- goto out;
-
rc = tpm2_get_cc_attrs_tbl(chip);
out:
--
2.20.1
^ permalink raw reply related
* Re: [PATCH V4 2/2] ASoC: fsl_esai: recover the channel swap after xrun
From: Nicolin Chen @ 2019-07-11 16:54 UTC (permalink / raw)
To: shengjiu.wang
Cc: alsa-devel, timur, lgirdwood, linuxppc-dev, Xiubo.Lee, tiwai,
perex, broonie, festevam, linux-kernel
In-Reply-To: <326035cb99975361699d9ed748054b08bc06a341.1562842206.git.shengjiu.wang@nxp.com>
On Thu, Jul 11, 2019 at 06:49:46PM +0800, shengjiu.wang@nxp.com wrote:
> From: Shengjiu Wang <shengjiu.wang@nxp.com>
>
> There is chip errata ERR008000, the reference doc is
> (https://www.nxp.com/docs/en/errata/IMX6DQCE.pdf),
>
> The issue is "While using ESAI transmit or receive and
> an underrun/overrun happens, channel swap may occur.
> The only recovery mechanism is to reset the ESAI."
>
> This issue exist in imx3/imx5/imx6(partial) series.
>
> In this commit add a tasklet to handle reset of ESAI
> after xrun happens to recover the channel swap.
>
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
Acked-by: Nicolin Chen <nicoleotsuka@gmail.com>
Thanks
> ---
> sound/soc/fsl/fsl_esai.c | 74 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 74 insertions(+)
>
> diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
> index ab460d6d7432..4ce8ac769244 100644
> --- a/sound/soc/fsl/fsl_esai.c
> +++ b/sound/soc/fsl/fsl_esai.c
> @@ -32,6 +32,7 @@
> * @extalclk: esai clock source to derive HCK, SCK and FS
> * @fsysclk: system clock source to derive HCK, SCK and FS
> * @spbaclk: SPBA clock (optional, depending on SoC design)
> + * @task: tasklet to handle the reset operation
> * @fifo_depth: depth of tx/rx FIFO
> * @slot_width: width of each DAI slot
> * @slots: number of slots
> @@ -42,6 +43,7 @@
> * @sck_div: if using PSR/PM dividers for SCKx clock
> * @slave_mode: if fully using DAI slave mode
> * @synchronous: if using tx/rx synchronous mode
> + * @reset_at_xrun: flags for enable reset operaton
> * @name: driver name
> */
> struct fsl_esai {
> @@ -53,6 +55,7 @@ struct fsl_esai {
> struct clk *extalclk;
> struct clk *fsysclk;
> struct clk *spbaclk;
> + struct tasklet_struct task;
> u32 fifo_depth;
> u32 slot_width;
> u32 slots;
> @@ -65,6 +68,7 @@ struct fsl_esai {
> bool sck_div[2];
> bool slave_mode;
> bool synchronous;
> + bool reset_at_xrun;
> char name[32];
> };
>
> @@ -73,8 +77,16 @@ static irqreturn_t esai_isr(int irq, void *devid)
> struct fsl_esai *esai_priv = (struct fsl_esai *)devid;
> struct platform_device *pdev = esai_priv->pdev;
> u32 esr;
> + u32 saisr;
>
> regmap_read(esai_priv->regmap, REG_ESAI_ESR, &esr);
> + regmap_read(esai_priv->regmap, REG_ESAI_SAISR, &saisr);
> +
> + if ((saisr & (ESAI_SAISR_TUE | ESAI_SAISR_ROE)) &&
> + esai_priv->reset_at_xrun) {
> + dev_dbg(&pdev->dev, "reset module for xrun\n");
> + tasklet_schedule(&esai_priv->task);
> + }
>
> if (esr & ESAI_ESR_TINIT_MASK)
> dev_dbg(&pdev->dev, "isr: Transmission Initialized\n");
> @@ -635,10 +647,17 @@ static void fsl_esai_trigger_start(struct fsl_esai *esai_priv, bool tx)
> ESAI_xSMB_xS_MASK, ESAI_xSMB_xS(mask));
> regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMA(tx),
> ESAI_xSMA_xS_MASK, ESAI_xSMA_xS(mask));
> +
> + /* Enable Exception interrupt */
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
> + ESAI_xCR_xEIE_MASK, ESAI_xCR_xEIE);
> }
>
> static void fsl_esai_trigger_stop(struct fsl_esai *esai_priv, bool tx)
> {
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
> + ESAI_xCR_xEIE_MASK, 0);
> +
> regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
> tx ? ESAI_xCR_TE_MASK : ESAI_xCR_RE_MASK, 0);
> regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMA(tx),
> @@ -653,6 +672,51 @@ static void fsl_esai_trigger_stop(struct fsl_esai *esai_priv, bool tx)
> ESAI_xFCR_xFR, 0);
> }
>
> +static void fsl_esai_hw_reset(unsigned long arg)
> +{
> + struct fsl_esai *esai_priv = (struct fsl_esai *)arg;
> + bool tx = true, rx = false, enabled[2];
> + u32 tfcr, rfcr;
> +
> + /* Save the registers */
> + regmap_read(esai_priv->regmap, REG_ESAI_TFCR, &tfcr);
> + regmap_read(esai_priv->regmap, REG_ESAI_RFCR, &rfcr);
> + enabled[tx] = tfcr & ESAI_xFCR_xFEN;
> + enabled[rx] = rfcr & ESAI_xFCR_xFEN;
> +
> + /* Stop the tx & rx */
> + fsl_esai_trigger_stop(esai_priv, tx);
> + fsl_esai_trigger_stop(esai_priv, rx);
> +
> + /* Reset the esai, and ignore return value */
> + fsl_esai_hw_init(esai_priv);
> +
> + /* Enforce ESAI personal resets for both TX and RX */
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> + ESAI_xCR_xPR_MASK, ESAI_xCR_xPR);
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_RCR,
> + ESAI_xCR_xPR_MASK, ESAI_xCR_xPR);
> +
> + /* Restore registers by regcache_sync, and ignore return value */
> + fsl_esai_register_restore(esai_priv);
> +
> + /* Remove ESAI personal resets by configuring PCRC and PRRC also */
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> + ESAI_xCR_xPR_MASK, 0);
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_RCR,
> + ESAI_xCR_xPR_MASK, 0);
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_PRRC,
> + ESAI_PRRC_PDC_MASK, ESAI_PRRC_PDC(ESAI_GPIO));
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_PCRC,
> + ESAI_PCRC_PC_MASK, ESAI_PCRC_PC(ESAI_GPIO));
> +
> + /* Restart tx / rx, if they already enabled */
> + if (enabled[tx])
> + fsl_esai_trigger_start(esai_priv, tx);
> + if (enabled[rx])
> + fsl_esai_trigger_start(esai_priv, rx);
> +}
> +
> static int fsl_esai_trigger(struct snd_pcm_substream *substream, int cmd,
> struct snd_soc_dai *dai)
> {
> @@ -857,6 +921,10 @@ static int fsl_esai_probe(struct platform_device *pdev)
> esai_priv->pdev = pdev;
> snprintf(esai_priv->name, sizeof(esai_priv->name), "%pOFn", np);
>
> + if (of_device_is_compatible(np, "fsl,vf610-esai") ||
> + of_device_is_compatible(np, "fsl,imx35-esai"))
> + esai_priv->reset_at_xrun = true;
> +
> /* Get the addresses and IRQ */
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> regs = devm_ioremap_resource(&pdev->dev, res);
> @@ -956,6 +1024,9 @@ static int fsl_esai_probe(struct platform_device *pdev)
> return ret;
> }
>
> + tasklet_init(&esai_priv->task, fsl_esai_hw_reset,
> + (unsigned long)esai_priv);
> +
> pm_runtime_enable(&pdev->dev);
>
> regcache_cache_only(esai_priv->regmap, true);
> @@ -969,7 +1040,10 @@ static int fsl_esai_probe(struct platform_device *pdev)
>
> static int fsl_esai_remove(struct platform_device *pdev)
> {
> + struct fsl_esai *esai_priv = platform_get_drvdata(pdev);
> +
> pm_runtime_disable(&pdev->dev);
> + tasklet_kill(&esai_priv->task);
>
> return 0;
> }
> --
> 2.21.0
>
^ permalink raw reply
* Re: [PATCH v2] tpm: tpm_ibm_vtpm: Fix unallocated banks
From: Nayna @ 2019-07-11 17:59 UTC (permalink / raw)
To: Jarkko Sakkinen, Sachin Sant, Michal Suchanek
Cc: Nayna Jain, linux-kernel, Mimi Zohar, Christoph Hellwig,
Jason Gunthorpe, linux-integrity, George Wilson, linuxppc-dev,
Peter Huewe
In-Reply-To: <20190709163827.2u6jeflrhg44q7dy@linux.intel.com>
Hi Jarkko,
On 07/09/2019 12:38 PM, Jarkko Sakkinen wrote:
> On Mon, Jul 08, 2019 at 03:43:04PM -0700, Christoph Hellwig wrote:
>> On Mon, Jul 08, 2019 at 06:24:04PM -0400, Mimi Zohar wrote:
>>>> static int tpm_get_pcr_allocation(struct tpm_chip *chip)
>>>> {
>>>> int rc;
>>>>
>>>> rc = (chip->flags & TPM_CHIP_FLAG_TPM2) ?
>>>> tpm2_get_pcr_allocation(chip) :
>>>> tpm1_get_pcr_allocation(chip);
>>>> return rc > 0 ? -ENODEV : rc;
>>>> }
>>>>
>>>> This addresses the issue that Stefan also pointed out. You have to
>>>> deal with the TPM error codes.
>>> Hm, in the past I was told by Christoph not to use the ternary
>>> operator. Have things changed? Other than removing the comment, the
>>> only other difference is the return.
>> In the end it is a matter of personal preference, but I find the
>> quote version above using the ternary horribly obsfucated.
> I fully agree that the return statement is an obsfucated mess and
> not a good place at all for using ternary operator.
I have posted the v3 version that includes the suggested corrections by
you and Stefan. Sorry for some delay.
Michal and Sachin, I would appreciate if you can test the v3 version,
please ?
Thanks & Regards,
- Nayna
^ permalink raw reply
* Re: [PATCH v3] tpm: tpm_ibm_vtpm: Fix unallocated banks
From: Jarkko Sakkinen @ 2019-07-11 20:28 UTC (permalink / raw)
To: Nayna Jain
Cc: Sachin Sant, Michal Suchanek, linux-kernel, Mimi Zohar,
Jason Gunthorpe, linux-integrity, George Wilson, linuxppc-dev,
Peter Huewe
In-Reply-To: <1562861615-11391-1-git-send-email-nayna@linux.ibm.com>
On Thu, Jul 11, 2019 at 12:13:35PM -0400, Nayna Jain wrote:
> The nr_allocated_banks and allocated banks are initialized as part of
> tpm_chip_register. Currently, this is done as part of auto startup
> function. However, some drivers, like the ibm vtpm driver, do not run
> auto startup during initialization. This results in uninitialized memory
> issue and causes a kernel panic during boot.
>
> This patch moves the pcr allocation outside the auto startup function
> into tpm_chip_register. This ensures that allocated banks are initialized
> in any case.
>
> Fixes: 879b589210a9 ("tpm: retrieve digest size of unknown algorithms with
> PCR read")
> Reported-by: Michal Suchanek <msuchanek@suse.de>
> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> Tested-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
> Tested-by: Michal Suchánek <msuchanek@suse.de>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
/Jarkko
^ permalink raw reply
* [PATCH] treewide: Rename rcu_dereference_raw_notrace to _check
From: Joel Fernandes (Google) @ 2019-07-11 20:45 UTC (permalink / raw)
To: linux-kernel
Cc: rcu, kernel-team, Jonathan Corbet, linux-doc, Lai Jiangshan,
Josh Triplett, kvm-ppc, Ingo Molnar, Mathieu Desnoyers,
Steven Rostedt, Joel Fernandes (Google), Paul E. McKenney,
linuxppc-dev, byungchul.park
The rcu_dereference_raw_notrace() API name is confusing.
It is equivalent to rcu_dereference_raw() except that it also does
sparse pointer checking.
There are only a few users of rcu_dereference_raw_notrace(). This
patches renames all of them to be rcu_dereference_raw_check with the
"check" indicating sparse checking.
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
Previous discussion is here:
https://lore.kernel.org/linuxppc-dev/20190528200014.GV28207@linux.ibm.com/T/
Documentation/RCU/Design/Requirements/Requirements.html | 2 +-
arch/powerpc/include/asm/kvm_book3s_64.h | 2 +-
include/linux/rculist.h | 4 ++--
include/linux/rcupdate.h | 2 +-
kernel/trace/ftrace_internal.h | 8 ++++----
kernel/trace/trace.c | 4 ++--
6 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/Documentation/RCU/Design/Requirements/Requirements.html b/Documentation/RCU/Design/Requirements/Requirements.html
index f04c467e55c5..467251f7fef6 100644
--- a/Documentation/RCU/Design/Requirements/Requirements.html
+++ b/Documentation/RCU/Design/Requirements/Requirements.html
@@ -2514,7 +2514,7 @@ disabled across the entire RCU read-side critical section.
<p>
It is possible to use tracing on RCU code, but tracing itself
uses RCU.
-For this reason, <tt>rcu_dereference_raw_notrace()</tt>
+For this reason, <tt>rcu_dereference_raw_check()</tt>
is provided for use by tracing, which avoids the destructive
recursion that could otherwise ensue.
This API is also used by virtualization in some architectures,
diff --git a/arch/powerpc/include/asm/kvm_book3s_64.h b/arch/powerpc/include/asm/kvm_book3s_64.h
index 21b1ed5df888..53388a311967 100644
--- a/arch/powerpc/include/asm/kvm_book3s_64.h
+++ b/arch/powerpc/include/asm/kvm_book3s_64.h
@@ -546,7 +546,7 @@ static inline void note_hpte_modification(struct kvm *kvm,
*/
static inline struct kvm_memslots *kvm_memslots_raw(struct kvm *kvm)
{
- return rcu_dereference_raw_notrace(kvm->memslots[0]);
+ return rcu_dereference_raw_check(kvm->memslots[0]);
}
extern void kvmppc_mmu_debugfs_init(struct kvm *kvm);
diff --git a/include/linux/rculist.h b/include/linux/rculist.h
index e91ec9ddcd30..10aab1d2d471 100644
--- a/include/linux/rculist.h
+++ b/include/linux/rculist.h
@@ -642,10 +642,10 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n,
* not do any RCU debugging or tracing.
*/
#define hlist_for_each_entry_rcu_notrace(pos, head, member) \
- for (pos = hlist_entry_safe (rcu_dereference_raw_notrace(hlist_first_rcu(head)),\
+ for (pos = hlist_entry_safe (rcu_dereference_raw_check(hlist_first_rcu(head)),\
typeof(*(pos)), member); \
pos; \
- pos = hlist_entry_safe(rcu_dereference_raw_notrace(hlist_next_rcu(\
+ pos = hlist_entry_safe(rcu_dereference_raw_check(hlist_next_rcu(\
&(pos)->member)), typeof(*(pos)), member))
/**
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 0c9b92799abc..e5161e377ad4 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -478,7 +478,7 @@ do { \
* The no-tracing version of rcu_dereference_raw() must not call
* rcu_read_lock_held().
*/
-#define rcu_dereference_raw_notrace(p) __rcu_dereference_check((p), 1, __rcu)
+#define rcu_dereference_raw_check(p) __rcu_dereference_check((p), 1, __rcu)
/**
* rcu_dereference_protected() - fetch RCU pointer when updates prevented
diff --git a/kernel/trace/ftrace_internal.h b/kernel/trace/ftrace_internal.h
index 0515a2096f90..0456e0a3dab1 100644
--- a/kernel/trace/ftrace_internal.h
+++ b/kernel/trace/ftrace_internal.h
@@ -6,22 +6,22 @@
/*
* Traverse the ftrace_global_list, invoking all entries. The reason that we
- * can use rcu_dereference_raw_notrace() is that elements removed from this list
+ * can use rcu_dereference_raw_check() is that elements removed from this list
* are simply leaked, so there is no need to interact with a grace-period
- * mechanism. The rcu_dereference_raw_notrace() calls are needed to handle
+ * mechanism. The rcu_dereference_raw_check() calls are needed to handle
* concurrent insertions into the ftrace_global_list.
*
* Silly Alpha and silly pointer-speculation compiler optimizations!
*/
#define do_for_each_ftrace_op(op, list) \
- op = rcu_dereference_raw_notrace(list); \
+ op = rcu_dereference_raw_check(list); \
do
/*
* Optimized for just a single item in the list (as that is the normal case).
*/
#define while_for_each_ftrace_op(op) \
- while (likely(op = rcu_dereference_raw_notrace((op)->next)) && \
+ while (likely(op = rcu_dereference_raw_check((op)->next)) && \
unlikely((op) != &ftrace_list_end))
extern struct ftrace_ops __rcu *ftrace_ops_list;
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 2c92b3d9ea30..1d69110d9e5b 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2642,10 +2642,10 @@ static void ftrace_exports(struct ring_buffer_event *event)
preempt_disable_notrace();
- export = rcu_dereference_raw_notrace(ftrace_exports_list);
+ export = rcu_dereference_raw_check(ftrace_exports_list);
while (export) {
trace_process_export(export, event);
- export = rcu_dereference_raw_notrace(export->next);
+ export = rcu_dereference_raw_check(export->next);
}
preempt_enable_notrace();
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox