* Re: [PATCH] pseries/drmem: update LMBs after LPM
From: Laurent Dufour @ 2021-04-27 17:28 UTC (permalink / raw)
To: Tyrel Datwyler, mpe, benh, paulus; +Cc: nathanl, linuxppc-dev, linux-kernel
In-Reply-To: <8b36065d-e4b4-bc0c-cf69-f01c91e49061@linux.ibm.com>
Le 27/04/2021 à 19:01, Tyrel Datwyler a écrit :
> On 4/27/21 8:01 AM, Laurent Dufour wrote:
>> After a LPM, the device tree node ibm,dynamic-reconfiguration-memory may be
>> updated by the hypervisor in the case the NUMA topology of the LPAR's
>> memory is updated.
>>
>> This is caught by the kernel, but the memory's node is updated because
>> there is no way to move a memory block between nodes.
>>
>> If later a memory block is added or removed, drmem_update_dt() is called
>> and it is overwriting the DT node to match the added or removed LMB. But
>> the LMB's associativity node has not been updated after the DT node update
>> and thus the node is overwritten by the Linux's topology instead of the
>> hypervisor one.
>>
>> Introduce a hook called when the ibm,dynamic-reconfiguration-memory node is
>> updated to force an update of the LMB's associativity.
>>
>> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
>> ---
>> arch/powerpc/include/asm/drmem.h | 1 +
>> arch/powerpc/mm/drmem.c | 48 +++++++++++++++++++++++
>> arch/powerpc/platforms/pseries/mobility.c | 9 +++++
>> 3 files changed, 58 insertions(+)
>>
>> diff --git a/arch/powerpc/include/asm/drmem.h b/arch/powerpc/include/asm/drmem.h
>> index bf2402fed3e0..55c2c25085b0 100644
>> --- a/arch/powerpc/include/asm/drmem.h
>> +++ b/arch/powerpc/include/asm/drmem.h
>> @@ -111,6 +111,7 @@ int drmem_update_dt(void);
>> int __init
>> walk_drmem_lmbs_early(unsigned long node, void *data,
>> int (*func)(struct drmem_lmb *, const __be32 **, void *));
>> +void drmem_update_lmbs(void);
>> #endif
>>
>> static inline void invalidate_lmb_associativity_index(struct drmem_lmb *lmb)
>> diff --git a/arch/powerpc/mm/drmem.c b/arch/powerpc/mm/drmem.c
>> index 9af3832c9d8d..46074bdfdb3c 100644
>> --- a/arch/powerpc/mm/drmem.c
>> +++ b/arch/powerpc/mm/drmem.c
>> @@ -307,6 +307,54 @@ int __init walk_drmem_lmbs_early(unsigned long node, void *data,
>> return ret;
>> }
>>
>> +/*
>> + * Update the LMB associativity index.
>> + */
>> +static int update_lmb(struct drmem_lmb *updated_lmb,
>> + __maybe_unused const __be32 **usm,
>> + __maybe_unused void *data)
>> +{
>> + struct drmem_lmb *lmb;
>> +
>> + /*
>> + * Brut force there may be better way to fetch the LMB
>> + */
>> + for_each_drmem_lmb(lmb) {
>> + if (lmb->drc_index != updated_lmb->drc_index)
>> + continue;
>> +
>> + lmb->aa_index = updated_lmb->aa_index;
>> + break;
>> + }
>> + return 0;
>> +}
>> +
>> +/*
>> + * Update the LMB associativity index.
>> + *
>> + * This needs to be called when the hypervisor is updating the
>> + * dynamic-reconfiguration-memory node property.
>> + */
>> +void drmem_update_lmbs(void)
>> +{
>> + struct device_node *node;
>> + const __be32 *prop;
>> +
>> + node = of_find_node_by_path("/ibm,dynamic-reconfiguration-memory");
>> + if (!node)
>> + return;
>> +
>> + prop = of_get_property(node, "ibm,dynamic-memory", NULL);
>> + if (prop) {
>> + __walk_drmem_v1_lmbs(prop, NULL, NULL, update_lmb);
>> + } else {
>> + prop = of_get_property(node, "ibm,dynamic-memory-v2", NULL);
>> + if (prop)
>> + __walk_drmem_v2_lmbs(prop, NULL, NULL, update_lmb);
>> + }
>> +
>> + of_node_put(node);
>> +}
>> #endif
>>
>> static int init_drmem_lmb_size(struct device_node *dn)
>> diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
>> index ea4d6a660e0d..c68eccc6e8df 100644
>> --- a/arch/powerpc/platforms/pseries/mobility.c
>> +++ b/arch/powerpc/platforms/pseries/mobility.c
>> @@ -25,6 +25,7 @@
>>
>> #include <asm/machdep.h>
>> #include <asm/rtas.h>
>> +#include <asm/drmem.h>
>> #include "pseries.h"
>> #include "../../kernel/cacheinfo.h"
>>
>> @@ -237,6 +238,7 @@ int pseries_devicetree_update(s32 scope)
>> __be32 *data;
>> int update_nodes_token;
>> int rc;
>> + bool drmem_updated = false;
>>
>> update_nodes_token = rtas_token("ibm,update-nodes");
>> if (update_nodes_token == RTAS_UNKNOWN_SERVICE)
>> @@ -271,6 +273,10 @@ int pseries_devicetree_update(s32 scope)
>> continue;
>> }
>>
>> + if (!strcmp(np->full_name,
>> + "ibm,dynamic-reconfiguration-memory"))
>> + drmem_updated = true;
>
> Is there a reason that we can't use the existing pseries_memory_notifier()
> callback in pseries/hotplug-memory.c to trigger the drmem_update_lmbs() when
> either the ibm,dynamic-memory or ibm,dynamic-memory-v2 properties are updated?
Thanks a lot Tyrel!
That's far more elegant, I'll send a v2 soon.
Laurent.
> Something like:
>
> static int pseries_memory_notifier(struct notifier_block *nb,
> unsigned long action, void *data)
> {
> struct of_reconfig_data *rd = data;
> int err = 0;
>
> switch (action) {
> case OF_RECONFIG_ATTACH_NODE:
> err = pseries_add_mem_node(rd->dn);
> break;
> case OF_RECONFIG_DETACH_NODE:
> err = pseries_remove_mem_node(rd->dn);
> break;
> case OF_RECONFIG_UPDATE_PROPERTY:
> if (!strcmp(rd->dn->full_name, "ibm,dynamic-reconfiguration-memory"));
> drmem_update_lmbs(rd->prop);
> break;
> }
> return notifier_from_errno(err);
> }
>
> Your drmem_update_lmbs() would need to be updated to take a property and to
> check the property name against ibm,dyanmic-memory[-v2].
>
> -Tyrel
>
>> +
>> switch (action) {
>> case DELETE_DT_NODE:
>> delete_dt_node(np);
>> @@ -293,6 +299,9 @@ int pseries_devicetree_update(s32 scope)
>> } while (rc == 1);
>>
>> kfree(rtas_buf);
>> +
>> + if (drmem_updated)
>> + drmem_update_lmbs();
>> return rc;
>> }
>>
>
^ permalink raw reply
* Re: PPC476 hangs during tlb flush after calling /init in crash kernel with linux 5.4+
From: Christophe Leroy @ 2021-04-27 17:26 UTC (permalink / raw)
To: Eddie James, linuxppc-dev; +Cc: miltonm, linux-kernel, npiggin, paulus
In-Reply-To: <b973fa4768140021719e7cc3123ee873d8b2a3f1.camel@linux.ibm.com>
Hi Eddies,
Le 27/04/2021 à 19:03, Eddie James a écrit :
> Hi all,
>
> I'm having a problem in simulation and hardware where my PPC476
> processor stops executing instructions after callling /init. In my case
> this is a bash script. The code descends to flush the TLB, and
> somewhere in the loop in _tlbil_pid, the PC goes to
> InstructionTLBError47x but does not go any further. This only occurs in
> the crash kernel environment, which is using the same kernel,
> initramfs, and init script as the main kernel, which executed fine. I
> do not see this problem with linux 4.19 or 3.10. I do see it with 5.4
> and 5.10. I see a fair amount of refactoring in the PPC memory
> management area between 4.19 and 5.4. Can anyone point me in a
> direction to debug this further? My stack trace is below as I can run
> gdb in simulation.
Can you bisect to pin point the culprit commit ?
Assuming the problem is in arch/powerpc/ , you should get the result in approx 10 steps:
[root@po15610vm linux-powerpc]# git bisect start -- arch/powerpc/
[root@po15610vm linux-powerpc]# git bisect bad v5.4
[root@po15610vm linux-powerpc]# git bisect good v4.19
Bisecting: 964 revisions left to test after this (roughly 10 steps)
Christophe
^ permalink raw reply
* Re: [PATCH] pseries/drmem: update LMBs after LPM
From: Tyrel Datwyler @ 2021-04-27 17:01 UTC (permalink / raw)
To: Laurent Dufour, mpe, benh, paulus; +Cc: nathanl, linuxppc-dev, linux-kernel
In-Reply-To: <20210427150113.14368-1-ldufour@linux.ibm.com>
On 4/27/21 8:01 AM, Laurent Dufour wrote:
> After a LPM, the device tree node ibm,dynamic-reconfiguration-memory may be
> updated by the hypervisor in the case the NUMA topology of the LPAR's
> memory is updated.
>
> This is caught by the kernel, but the memory's node is updated because
> there is no way to move a memory block between nodes.
>
> If later a memory block is added or removed, drmem_update_dt() is called
> and it is overwriting the DT node to match the added or removed LMB. But
> the LMB's associativity node has not been updated after the DT node update
> and thus the node is overwritten by the Linux's topology instead of the
> hypervisor one.
>
> Introduce a hook called when the ibm,dynamic-reconfiguration-memory node is
> updated to force an update of the LMB's associativity.
>
> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
> ---
> arch/powerpc/include/asm/drmem.h | 1 +
> arch/powerpc/mm/drmem.c | 48 +++++++++++++++++++++++
> arch/powerpc/platforms/pseries/mobility.c | 9 +++++
> 3 files changed, 58 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/drmem.h b/arch/powerpc/include/asm/drmem.h
> index bf2402fed3e0..55c2c25085b0 100644
> --- a/arch/powerpc/include/asm/drmem.h
> +++ b/arch/powerpc/include/asm/drmem.h
> @@ -111,6 +111,7 @@ int drmem_update_dt(void);
> int __init
> walk_drmem_lmbs_early(unsigned long node, void *data,
> int (*func)(struct drmem_lmb *, const __be32 **, void *));
> +void drmem_update_lmbs(void);
> #endif
>
> static inline void invalidate_lmb_associativity_index(struct drmem_lmb *lmb)
> diff --git a/arch/powerpc/mm/drmem.c b/arch/powerpc/mm/drmem.c
> index 9af3832c9d8d..46074bdfdb3c 100644
> --- a/arch/powerpc/mm/drmem.c
> +++ b/arch/powerpc/mm/drmem.c
> @@ -307,6 +307,54 @@ int __init walk_drmem_lmbs_early(unsigned long node, void *data,
> return ret;
> }
>
> +/*
> + * Update the LMB associativity index.
> + */
> +static int update_lmb(struct drmem_lmb *updated_lmb,
> + __maybe_unused const __be32 **usm,
> + __maybe_unused void *data)
> +{
> + struct drmem_lmb *lmb;
> +
> + /*
> + * Brut force there may be better way to fetch the LMB
> + */
> + for_each_drmem_lmb(lmb) {
> + if (lmb->drc_index != updated_lmb->drc_index)
> + continue;
> +
> + lmb->aa_index = updated_lmb->aa_index;
> + break;
> + }
> + return 0;
> +}
> +
> +/*
> + * Update the LMB associativity index.
> + *
> + * This needs to be called when the hypervisor is updating the
> + * dynamic-reconfiguration-memory node property.
> + */
> +void drmem_update_lmbs(void)
> +{
> + struct device_node *node;
> + const __be32 *prop;
> +
> + node = of_find_node_by_path("/ibm,dynamic-reconfiguration-memory");
> + if (!node)
> + return;
> +
> + prop = of_get_property(node, "ibm,dynamic-memory", NULL);
> + if (prop) {
> + __walk_drmem_v1_lmbs(prop, NULL, NULL, update_lmb);
> + } else {
> + prop = of_get_property(node, "ibm,dynamic-memory-v2", NULL);
> + if (prop)
> + __walk_drmem_v2_lmbs(prop, NULL, NULL, update_lmb);
> + }
> +
> + of_node_put(node);
> +}
> #endif
>
> static int init_drmem_lmb_size(struct device_node *dn)
> diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
> index ea4d6a660e0d..c68eccc6e8df 100644
> --- a/arch/powerpc/platforms/pseries/mobility.c
> +++ b/arch/powerpc/platforms/pseries/mobility.c
> @@ -25,6 +25,7 @@
>
> #include <asm/machdep.h>
> #include <asm/rtas.h>
> +#include <asm/drmem.h>
> #include "pseries.h"
> #include "../../kernel/cacheinfo.h"
>
> @@ -237,6 +238,7 @@ int pseries_devicetree_update(s32 scope)
> __be32 *data;
> int update_nodes_token;
> int rc;
> + bool drmem_updated = false;
>
> update_nodes_token = rtas_token("ibm,update-nodes");
> if (update_nodes_token == RTAS_UNKNOWN_SERVICE)
> @@ -271,6 +273,10 @@ int pseries_devicetree_update(s32 scope)
> continue;
> }
>
> + if (!strcmp(np->full_name,
> + "ibm,dynamic-reconfiguration-memory"))
> + drmem_updated = true;
Is there a reason that we can't use the existing pseries_memory_notifier()
callback in pseries/hotplug-memory.c to trigger the drmem_update_lmbs() when
either the ibm,dynamic-memory or ibm,dynamic-memory-v2 properties are updated?
Something like:
static int pseries_memory_notifier(struct notifier_block *nb,
unsigned long action, void *data)
{
struct of_reconfig_data *rd = data;
int err = 0;
switch (action) {
case OF_RECONFIG_ATTACH_NODE:
err = pseries_add_mem_node(rd->dn);
break;
case OF_RECONFIG_DETACH_NODE:
err = pseries_remove_mem_node(rd->dn);
break;
case OF_RECONFIG_UPDATE_PROPERTY:
if (!strcmp(rd->dn->full_name, "ibm,dynamic-reconfiguration-memory"));
drmem_update_lmbs(rd->prop);
break;
}
return notifier_from_errno(err);
}
Your drmem_update_lmbs() would need to be updated to take a property and to
check the property name against ibm,dyanmic-memory[-v2].
-Tyrel
> +
> switch (action) {
> case DELETE_DT_NODE:
> delete_dt_node(np);
> @@ -293,6 +299,9 @@ int pseries_devicetree_update(s32 scope)
> } while (rc == 1);
>
> kfree(rtas_buf);
> +
> + if (drmem_updated)
> + drmem_update_lmbs();
> return rc;
> }
>
^ permalink raw reply
* Re: [PATCH 1/2] powerpc/sstep: Add emulation support for ‘setb’ instruction
From: Naveen N. Rao @ 2021-04-27 16:44 UTC (permalink / raw)
To: Daniel Axtens, linuxppc-dev, Michael Ellerman, Sathvika Vasireddy
In-Reply-To: <87bla5b041.fsf@mpe.ellerman.id.au>
Michael Ellerman wrote:
> "Naveen N. Rao" <naveen.n.rao@linux.ibm.com> writes:
>> Michael Ellerman wrote:
>>> "Naveen N. Rao" <naveen.n.rao@linux.ibm.com> writes:
>>>> Daniel Axtens wrote:
>>>>> Sathvika Vasireddy <sathvika@linux.vnet.ibm.com> writes:
>>>>>
>>>>>> This adds emulation support for the following instruction:
>>>>>> * Set Boolean (setb)
>>>>>>
>>>>>> Signed-off-by: Sathvika Vasireddy <sathvika@linux.vnet.ibm.com>
>>> ...
>>>>>
>>>>> If you do end up respinning the patch, I think it would be good to make
>>>>> the maths a bit clearer. I think it works because a left shift of 2 is
>>>>> the same as multiplying by 4, but it would be easier to follow if you
>>>>> used a temporary variable for btf.
>>>>
>>>> Indeed. I wonder if it is better to follow the ISA itself. Per the ISA,
>>>> the bit we are interested in is:
>>>> 4 x BFA + 32
>>>>
>>>> So, if we use that along with the PPC_BIT() macro, we get:
>>>> if (regs->ccr & PPC_BIT(ra + 32))
>>>
>>> Use of PPC_BIT risks annoying your maintainer :)
>>
>> Uh oh... that isn't good :)
>>
>> I looked up previous discussions and I think I now understand why you
>> don't prefer it.
>
> Hah, I'd forgotten I'd written (ranted :D) about this in the past.
>
>> But, I feel it helps make it easy to follow the code when referring to
>> the ISA.
>
> That's true. But I think that's much much less common than people
> reading the code in isolation.
I thought that isn't so for at least the instruction emulation
infrastructure...
>
> And ultimately it doesn't matter if the code (appears to) match the ISA,
> it matters that the code works. My worry is that too much use of those
> type of macros obscures what's actually happening.
... but, I agree on the above point. I can see why it is better to keep
it simple.
I also see precedence for what both you and Segher are suggesting in the
existing code in sstep.c
>
>> I'm wondering if it is just the name you dislike and if so,
>> does it make sense to rename PPC_BIT() to something else? We have
>> BIT_ULL(), so perhaps BIT_MSB_ULL() or MSB_BIT_ULL()?
>
> The name is part of it. But I don't really like BIT_ULL() either, it
> hides in a macro something that could just be there in front of you
> ie. (1ull << x).
>
>
> For this case of setb, I think I'd go with something like below. It
> doesn't exactly match the ISA, but I think there's minimal obfuscation
> of what's actually going on.
>
> // ra is now bfa
> ra = (ra >> 2);
>
> // Extract 4-bit CR field
> val = regs->ccr >> (CR0_SHIFT - 4 * ra);
>
> if (val & 8)
> op->val = -1;
> else if (val & 4)
> op->val = 1;
> else
> op->val = 0;
>
>
> If anything could use a macro it would be the 8 and 4, eg. CR_LT, CR_GT.
>
> Of course that's probably got a bug in it, because I just wrote it by
> eye and it's 11:28 pm :)
LGTM, thanks. I'll let Sathvika decide on which variant she wants to go
with for v2 :)
- Naveen
^ permalink raw reply
* [PATCH] pseries/drmem: update LMBs after LPM
From: Laurent Dufour @ 2021-04-27 15:01 UTC (permalink / raw)
To: mpe, benh, paulus; +Cc: nathanl, linuxppc-dev, linux-kernel
After a LPM, the device tree node ibm,dynamic-reconfiguration-memory may be
updated by the hypervisor in the case the NUMA topology of the LPAR's
memory is updated.
This is caught by the kernel, but the memory's node is updated because
there is no way to move a memory block between nodes.
If later a memory block is added or removed, drmem_update_dt() is called
and it is overwriting the DT node to match the added or removed LMB. But
the LMB's associativity node has not been updated after the DT node update
and thus the node is overwritten by the Linux's topology instead of the
hypervisor one.
Introduce a hook called when the ibm,dynamic-reconfiguration-memory node is
updated to force an update of the LMB's associativity.
Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
---
arch/powerpc/include/asm/drmem.h | 1 +
arch/powerpc/mm/drmem.c | 48 +++++++++++++++++++++++
arch/powerpc/platforms/pseries/mobility.c | 9 +++++
3 files changed, 58 insertions(+)
diff --git a/arch/powerpc/include/asm/drmem.h b/arch/powerpc/include/asm/drmem.h
index bf2402fed3e0..55c2c25085b0 100644
--- a/arch/powerpc/include/asm/drmem.h
+++ b/arch/powerpc/include/asm/drmem.h
@@ -111,6 +111,7 @@ int drmem_update_dt(void);
int __init
walk_drmem_lmbs_early(unsigned long node, void *data,
int (*func)(struct drmem_lmb *, const __be32 **, void *));
+void drmem_update_lmbs(void);
#endif
static inline void invalidate_lmb_associativity_index(struct drmem_lmb *lmb)
diff --git a/arch/powerpc/mm/drmem.c b/arch/powerpc/mm/drmem.c
index 9af3832c9d8d..46074bdfdb3c 100644
--- a/arch/powerpc/mm/drmem.c
+++ b/arch/powerpc/mm/drmem.c
@@ -307,6 +307,54 @@ int __init walk_drmem_lmbs_early(unsigned long node, void *data,
return ret;
}
+/*
+ * Update the LMB associativity index.
+ */
+static int update_lmb(struct drmem_lmb *updated_lmb,
+ __maybe_unused const __be32 **usm,
+ __maybe_unused void *data)
+{
+ struct drmem_lmb *lmb;
+
+ /*
+ * Brut force there may be better way to fetch the LMB
+ */
+ for_each_drmem_lmb(lmb) {
+ if (lmb->drc_index != updated_lmb->drc_index)
+ continue;
+
+ lmb->aa_index = updated_lmb->aa_index;
+ break;
+ }
+ return 0;
+}
+
+/*
+ * Update the LMB associativity index.
+ *
+ * This needs to be called when the hypervisor is updating the
+ * dynamic-reconfiguration-memory node property.
+ */
+void drmem_update_lmbs(void)
+{
+ struct device_node *node;
+ const __be32 *prop;
+
+ node = of_find_node_by_path("/ibm,dynamic-reconfiguration-memory");
+ if (!node)
+ return;
+
+ prop = of_get_property(node, "ibm,dynamic-memory", NULL);
+ if (prop) {
+ __walk_drmem_v1_lmbs(prop, NULL, NULL, update_lmb);
+ } else {
+ prop = of_get_property(node, "ibm,dynamic-memory-v2", NULL);
+ if (prop)
+ __walk_drmem_v2_lmbs(prop, NULL, NULL, update_lmb);
+ }
+
+ of_node_put(node);
+}
#endif
static int init_drmem_lmb_size(struct device_node *dn)
diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
index ea4d6a660e0d..c68eccc6e8df 100644
--- a/arch/powerpc/platforms/pseries/mobility.c
+++ b/arch/powerpc/platforms/pseries/mobility.c
@@ -25,6 +25,7 @@
#include <asm/machdep.h>
#include <asm/rtas.h>
+#include <asm/drmem.h>
#include "pseries.h"
#include "../../kernel/cacheinfo.h"
@@ -237,6 +238,7 @@ int pseries_devicetree_update(s32 scope)
__be32 *data;
int update_nodes_token;
int rc;
+ bool drmem_updated = false;
update_nodes_token = rtas_token("ibm,update-nodes");
if (update_nodes_token == RTAS_UNKNOWN_SERVICE)
@@ -271,6 +273,10 @@ int pseries_devicetree_update(s32 scope)
continue;
}
+ if (!strcmp(np->full_name,
+ "ibm,dynamic-reconfiguration-memory"))
+ drmem_updated = true;
+
switch (action) {
case DELETE_DT_NODE:
delete_dt_node(np);
@@ -293,6 +299,9 @@ int pseries_devicetree_update(s32 scope)
} while (rc == 1);
kfree(rtas_buf);
+
+ if (drmem_updated)
+ drmem_update_lmbs();
return rc;
}
--
2.31.1
^ permalink raw reply related
* Re: [PATCH 4/4] powerpc/pseries: warn if recursing into the hcall tracing code
From: Naveen N. Rao @ 2021-04-27 13:59 UTC (permalink / raw)
To: linuxppc-dev, Nicholas Piggin
In-Reply-To: <20210423031108.1046067-5-npiggin@gmail.com>
Nicholas Piggin wrote:
> ---
> arch/powerpc/platforms/pseries/lpar.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/lpar.c b/arch/powerpc/platforms/pseries/lpar.c
> index 835e7f661a05..a961a7ebeab3 100644
> --- a/arch/powerpc/platforms/pseries/lpar.c
> +++ b/arch/powerpc/platforms/pseries/lpar.c
> @@ -1828,8 +1828,11 @@ void hcall_tracepoint_unregfunc(void)
>
> /*
> * Since the tracing code might execute hcalls we need to guard against
> - * recursion. H_CONFER from spin locks must be treated separately though
> - * and use _notrace plpar_hcall variants, see yield_to_preempted().
> + * recursion, but this always seems risky -- __trace_hcall_entry might be
> + * ftraced, for example. So warn in this case.
__trace_hcall_[entry|exit] aren't traced anymore since they now have the
'notrace' annotation.
> + *
> + * H_CONFER from spin locks must be treated separately though and use _notrace
> + * plpar_hcall variants, see yield_to_preempted().
> */
> static DEFINE_PER_CPU(unsigned int, hcall_trace_depth);
>
> @@ -1843,7 +1846,7 @@ notrace void __trace_hcall_entry(unsigned long opcode, unsigned long *args)
>
> depth = this_cpu_ptr(&hcall_trace_depth);
>
> - if (*depth)
> + if (WARN_ON_ONCE(*depth))
> goto out;
I don't think this will be helpful. The hcall trace depth tracking is
for the tracepoint and I suspect that this warning will be triggered
quite easily. Since we have recursion protection, I don't think we
should warn here.
- Naveen
^ permalink raw reply
* Re: [PATCH 3/4] powerpc/pseries: use notrace hcall variant for H_CEDE idle
From: Naveen N. Rao @ 2021-04-27 13:53 UTC (permalink / raw)
To: linuxppc-dev, Nicholas Piggin
In-Reply-To: <20210423031108.1046067-4-npiggin@gmail.com>
Nicholas Piggin wrote:
> Rather than special-case H_CEDE in the hcall trace wrappers, make the
> idle H_CEDE call use plpar_hcall_norets_notrace().
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/include/asm/plpar_wrappers.h | 6 +++++-
> arch/powerpc/platforms/pseries/lpar.c | 10 ----------
> 2 files changed, 5 insertions(+), 11 deletions(-)
Reviewed-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
- Naveen
^ permalink raw reply
* Re: [PATCH 2/4] powerpc/pseries: Don't trace hcall tracing wrapper
From: Naveen N. Rao @ 2021-04-27 13:52 UTC (permalink / raw)
To: linuxppc-dev, Nicholas Piggin
In-Reply-To: <20210423031108.1046067-3-npiggin@gmail.com>
Nicholas Piggin wrote:
> This doesn't seem very useful to trace before the recursion check, even
> if the ftrace code has any recursion checks of its own. Be on the safe
> side and don't trace the hcall trace wrappers.
These functions exist precisely to allow hcalls to be traced, so it
doesn't make sense to "trace the tracer". Users wanting to know about
hcalls are better off enabling the tracepoint here instead.
>
> Reported-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/platforms/pseries/lpar.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
- Naveen
^ permalink raw reply
* Re: [PATCH 1/4] powerpc/pseries: Fix hcall tracing recursion in pv queued spinlocks
From: Naveen N. Rao @ 2021-04-27 13:43 UTC (permalink / raw)
To: linuxppc-dev, Nicholas Piggin
In-Reply-To: <20210423031108.1046067-2-npiggin@gmail.com>
Nicholas Piggin wrote:
> The paravit queued spinlock slow path adds itself to the queue then
> calls pv_wait to wait for the lock to become free. This is implemented
> by calling H_CONFER to donate cycles.
>
> When hcall tracing is enabled, this H_CONFER call can lead to a spin
> lock being taken in the tracing code, which will result in the lock to
> be taken again, which will also go to the slow path because it queues
> behind itself and so won't ever make progress.
>
> An example trace of a deadlock:
>
> __pv_queued_spin_lock_slowpath
> trace_clock_global
> ring_buffer_lock_reserve
> trace_event_buffer_lock_reserve
> trace_event_buffer_reserve
> trace_event_raw_event_hcall_exit
> __trace_hcall_exit
> plpar_hcall_norets_trace
> __pv_queued_spin_lock_slowpath
> trace_clock_global
> ring_buffer_lock_reserve
> trace_event_buffer_lock_reserve
> trace_event_buffer_reserve
> trace_event_raw_event_rcu_dyntick
> rcu_irq_exit
> irq_exit
> __do_irq
> call_do_irq
> do_IRQ
> hardware_interrupt_common_virt
>
> Fix this by introducing plpar_hcall_norets_notrace(), and using that to
> make SPLPAR virtual processor dispatching hcalls by the paravirt
> spinlock code.
>
> Fixes: 20c0e8269e9d ("powerpc/pseries: Implement paravirt qspinlocks for SPLPAR")
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/include/asm/hvcall.h | 3 +++
> arch/powerpc/include/asm/paravirt.h | 22 +++++++++++++++++++---
> arch/powerpc/platforms/pseries/hvCall.S | 10 ++++++++++
> arch/powerpc/platforms/pseries/lpar.c | 4 ++--
> 4 files changed, 34 insertions(+), 5 deletions(-)
Thanks for the fix! Some very minor nits below, but none the less:
Reviewed-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>
> diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h
> index ed6086d57b22..0c92b01a3c3c 100644
> --- a/arch/powerpc/include/asm/hvcall.h
> +++ b/arch/powerpc/include/asm/hvcall.h
> @@ -446,6 +446,9 @@
> */
> long plpar_hcall_norets(unsigned long opcode, ...);
>
> +/* Variant which does not do hcall tracing */
> +long plpar_hcall_norets_notrace(unsigned long opcode, ...);
> +
> /**
> * plpar_hcall: - Make a pseries hypervisor call
> * @opcode: The hypervisor call to make.
> diff --git a/arch/powerpc/include/asm/paravirt.h b/arch/powerpc/include/asm/paravirt.h
> index 5d1726bb28e7..3c13c2ec70a9 100644
> --- a/arch/powerpc/include/asm/paravirt.h
> +++ b/arch/powerpc/include/asm/paravirt.h
> @@ -30,17 +30,33 @@ static inline u32 yield_count_of(int cpu)
>
> static inline void yield_to_preempted(int cpu, u32 yield_count)
> {
It looks like yield_to_preempted() is only used by simple spin locks
today. I wonder if it makes more sense to put the below comment in
yield_to_any() which is used by the qspinlock code.
> - plpar_hcall_norets(H_CONFER, get_hard_smp_processor_id(cpu), yield_count);
> + /*
> + * Spinlock code yields and prods, so don't trace the hcalls because
> + * tracing code takes spinlocks which could recurse.
> + *
> + * These calls are made while the lock is not held, the lock slowpath
> + * yields if it can not acquire the lock, and unlock slow path might
> + * prod if a waiter has yielded). So this did not seem to be a problem
> + * for simple spin locks because technically it didn't recuse on the
^^^^^^
recurse
> + * lock. However the queued spin lock contended path is more strictly
> + * ordered: the H_CONFER hcall is made after the task has queued itself
> + * on the lock, so then recursing on the lock will queue up behind that
> + * (or worse: queued spinlocks uses tricks that assume a context never
> + * waits on more than one spinlock, so that may cause random
> + * corruption).
> + */
> + plpar_hcall_norets_notrace(H_CONFER,
> + get_hard_smp_processor_id(cpu), yield_count);
This can all be on a single line.
- Naveen
^ permalink raw reply
* Re: [PATCH 2/9] ARM: PXA: Kill use of irq_create_strict_mappings()
From: Guenter Roeck @ 2021-04-27 12:56 UTC (permalink / raw)
To: Marc Zyngier
Cc: Rich Felker, Yoshinori Sato, linux-sh, Robert Jarzmik,
linux-kernel, Haojian Zhuang, linux-mips, Thomas Bogendoerfer,
linux-arm-kernel, Thomas Gleixner, linuxppc-dev, Daniel Mack
In-Reply-To: <87o8e0nn8u.wl-maz@kernel.org>
On 4/27/21 1:30 AM, Marc Zyngier wrote:
> Hi Guenter,
>
> Thanks for the heads up.
>
> On Mon, 26 Apr 2021 23:39:42 +0100,
> Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On Tue, Apr 06, 2021 at 10:35:50AM +0100, Marc Zyngier wrote:
>>> irq_create_strict_mappings() is a poor way to allow the use of
>>> a linear IRQ domain as a legacy one. Let's be upfront about
>>> it and use a legacy domain when appropriate.
>>>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>> ---
>>
>> When running the "mainstone" qemu emulation, this patch results
>> in many (32, actually) runtime warnings such as the following.
>>
>> [ 0.528272] ------------[ cut here ]------------
>> [ 0.528285] WARNING: CPU: 0 PID: 1 at kernel/irq/irqdomain.c:550 irq_domain_associate+0x194/0x1f0
>> [ 0.528315] error: virq335 is not allocated
>
> [...]
>
> This looks like a case of CONFIG_SPARSE_IRQ, combined with a lack of
> brain engagement. I've come up with the following patch, which lets
> the kernel boot in QEMU without screaming (other than the lack of a
> rootfs...).
>
> Please let me know if this helps.
>
It does.
Tested-by: Guenter Roeck <linux@roeck-us.net>
Thanks,
Guenter
> Thanks,
>
> M.
>
> From 4d7f6ddbbfdff1c9f029bafca79020d3294dc32c Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <maz@kernel.org>
> Date: Tue, 27 Apr 2021 09:00:28 +0100
> Subject: [PATCH] ARM: PXA: Fix cplds irqdesc allocation when using legacy mode
>
> The Mainstone PXA platform uses CONFIG_SPARSE_IRQ, and thus we
> cannot rely on the irq descriptors to be readilly allocated
> before creating the irqdomain in legacy mode. The kernel then
> complains loudly about not being able to associate the interrupt
> in the domain -- can't blame it.
>
> Fix it by allocating the irqdescs upfront in the legacy case.
>
> Fixes: b68761da0111 ("ARM: PXA: Kill use of irq_create_strict_mappings()")
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> arch/arm/mach-pxa/pxa_cplds_irqs.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-pxa/pxa_cplds_irqs.c b/arch/arm/mach-pxa/pxa_cplds_irqs.c
> index ec0d9b094744..bddfc7cd5d40 100644
> --- a/arch/arm/mach-pxa/pxa_cplds_irqs.c
> +++ b/arch/arm/mach-pxa/pxa_cplds_irqs.c
> @@ -121,8 +121,13 @@ static int cplds_probe(struct platform_device *pdev)
> return fpga->irq;
>
> base_irq = platform_get_irq(pdev, 1);
> - if (base_irq < 0)
> + if (base_irq < 0) {
> base_irq = 0;
> + } else {
> + ret = devm_irq_alloc_descs(&pdev->dev, base_irq, base_irq, CPLDS_NB_IRQ, 0);
> + if (ret < 0)
> + return ret;
> + }
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> fpga->base = devm_ioremap_resource(&pdev->dev, res);
>
^ permalink raw reply
* Re: [PATCH 2/9] ARM: PXA: Kill use of irq_create_strict_mappings()
From: Marc Zyngier @ 2021-04-27 8:30 UTC (permalink / raw)
To: Guenter Roeck
Cc: Rich Felker, Yoshinori Sato, linux-sh, Robert Jarzmik,
linux-kernel, Haojian Zhuang, linux-mips, Thomas Bogendoerfer,
linux-arm-kernel, Thomas Gleixner, linuxppc-dev, Daniel Mack
In-Reply-To: <20210426223942.GA213931@roeck-us.net>
Hi Guenter,
Thanks for the heads up.
On Mon, 26 Apr 2021 23:39:42 +0100,
Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Tue, Apr 06, 2021 at 10:35:50AM +0100, Marc Zyngier wrote:
> > irq_create_strict_mappings() is a poor way to allow the use of
> > a linear IRQ domain as a legacy one. Let's be upfront about
> > it and use a legacy domain when appropriate.
> >
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
>
> When running the "mainstone" qemu emulation, this patch results
> in many (32, actually) runtime warnings such as the following.
>
> [ 0.528272] ------------[ cut here ]------------
> [ 0.528285] WARNING: CPU: 0 PID: 1 at kernel/irq/irqdomain.c:550 irq_domain_associate+0x194/0x1f0
> [ 0.528315] error: virq335 is not allocated
[...]
This looks like a case of CONFIG_SPARSE_IRQ, combined with a lack of
brain engagement. I've come up with the following patch, which lets
the kernel boot in QEMU without screaming (other than the lack of a
rootfs...).
Please let me know if this helps.
Thanks,
M.
From 4d7f6ddbbfdff1c9f029bafca79020d3294dc32c Mon Sep 17 00:00:00 2001
From: Marc Zyngier <maz@kernel.org>
Date: Tue, 27 Apr 2021 09:00:28 +0100
Subject: [PATCH] ARM: PXA: Fix cplds irqdesc allocation when using legacy mode
The Mainstone PXA platform uses CONFIG_SPARSE_IRQ, and thus we
cannot rely on the irq descriptors to be readilly allocated
before creating the irqdomain in legacy mode. The kernel then
complains loudly about not being able to associate the interrupt
in the domain -- can't blame it.
Fix it by allocating the irqdescs upfront in the legacy case.
Fixes: b68761da0111 ("ARM: PXA: Kill use of irq_create_strict_mappings()")
Reported-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
arch/arm/mach-pxa/pxa_cplds_irqs.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-pxa/pxa_cplds_irqs.c b/arch/arm/mach-pxa/pxa_cplds_irqs.c
index ec0d9b094744..bddfc7cd5d40 100644
--- a/arch/arm/mach-pxa/pxa_cplds_irqs.c
+++ b/arch/arm/mach-pxa/pxa_cplds_irqs.c
@@ -121,8 +121,13 @@ static int cplds_probe(struct platform_device *pdev)
return fpga->irq;
base_irq = platform_get_irq(pdev, 1);
- if (base_irq < 0)
+ if (base_irq < 0) {
base_irq = 0;
+ } else {
+ ret = devm_irq_alloc_descs(&pdev->dev, base_irq, base_irq, CPLDS_NB_IRQ, 0);
+ if (ret < 0)
+ return ret;
+ }
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
fpga->base = devm_ioremap_resource(&pdev->dev, res);
--
2.30.2
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply related
* Re: [PATCH v7] powerpc/irq: Inline call_do_irq() and call_do_softirq()
From: Christophe Leroy @ 2021-04-27 6:39 UTC (permalink / raw)
To: Nathan Chancellor, Michael Ellerman; +Cc: clang-built-linux, linuxppc-dev
In-Reply-To: <YIcLcujmoK6Yet9d@archlinux-ax161>
Le 26/04/2021 à 20:50, Nathan Chancellor a écrit :
> On Sat, Mar 20, 2021 at 11:22:27PM +1100, Michael Ellerman wrote:
>> From: Christophe Leroy <christophe.leroy@csgroup.eu>
>>
>> call_do_irq() and call_do_softirq() are simple enough to be
>> worth inlining.
>>
>> Inlining them avoids an mflr/mtlr pair plus a save/reload on stack. It
>> also allows GCC to keep the saved ksp_limit in an nonvolatile reg.
>>
>> This is inspired from S390 arch. Several other arches do more or
>> less the same. The way sparc arch does seems odd thought.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>
>
> This change caused our ppc44x_defconfig builds to hang when powering
> down in QEMU:
>
> https://github.com/ClangBuiltLinux/continuous-integration2/runs/2304364629?check_suite_focus=true#logs
>
> This is probably something with clang given that GCC 10.3.0 works fine
> but due to the nature of the change, I have no idea how to tell what is
> going wrong. I tried to do some rudimentary debugging with gdb but that
> did not really get me anywhere.
>
> The kernel was built with just 'CC=clang' and it is reproducible with
> all versions of clang that the kernel supports.
>
> The QEMU invocation is visible at the link above, it is done with our
> boot-qemu.sh in this repo, which also houses the rootfs:
>
> https://github.com/ClangBuiltLinux/boot-utils
>
> Happy to provide any other information or debug/test as directed!
>
With GCC:
000003f0 <do_softirq_own_stack>:
3f0: 94 21 ff f0 stwu r1,-16(r1)
3f4: 7c 08 02 a6 mflr r0
3f8: 3d 20 00 00 lis r9,0
3fa: R_PPC_ADDR16_HA .data..read_mostly+0x4
3fc: 93 e1 00 0c stw r31,12(r1)
400: 90 01 00 14 stw r0,20(r1)
404: 83 e9 00 00 lwz r31,0(r9)
406: R_PPC_ADDR16_LO .data..read_mostly+0x4
408: 94 3f 1f f0 stwu r1,8176(r31)
40c: 7f e1 fb 78 mr r1,r31
410: 48 00 00 01 bl 410 <do_softirq_own_stack+0x20>
410: R_PPC_REL24 __do_softirq
414: 80 21 00 00 lwz r1,0(r1)
418: 80 01 00 14 lwz r0,20(r1)
41c: 83 e1 00 0c lwz r31,12(r1)
420: 38 21 00 10 addi r1,r1,16
424: 7c 08 03 a6 mtlr r0
428: 4e 80 00 20 blr
With CLANG:
000003e8 <do_softirq_own_stack>:
3e8: 94 21 ff f0 stwu r1,-16(r1)
3ec: 93 c1 00 08 stw r30,8(r1)
3f0: 3c 60 00 00 lis r3,0
3f2: R_PPC_ADDR16_HA softirq_ctx
3f4: 83 c3 00 00 lwz r30,0(r3)
3f6: R_PPC_ADDR16_LO softirq_ctx
3f8: 94 3e 1f f0 stwu r1,8176(r30)
3fc: 7f c1 f3 78 mr r1,r30
400: 48 00 00 01 bl 400 <do_softirq_own_stack+0x18>
400: R_PPC_REL24 __do_softirq
404: 80 21 00 00 lwz r1,0(r1)
408: 83 c1 00 08 lwz r30,8(r1)
40c: 38 21 00 10 addi r1,r1,16
410: 4e 80 00 20 blr
As you can see, CLANG doesn't save/restore 'lr' allthought 'lr' is explicitely listed in the
registers clobbered by the inline assembly:
>> +static __always_inline void call_do_softirq(const void *sp)
>> +{
>> + /* Temporarily switch r1 to sp, call __do_softirq() then restore r1. */
>> + asm volatile (
>> + PPC_STLU " %%r1, %[offset](%[sp]) ;"
>> + "mr %%r1, %[sp] ;"
>> + "bl %[callee] ;"
>> + PPC_LL " %%r1, 0(%%r1) ;"
>> + : // Outputs
>> + : // Inputs
>> + [sp] "b" (sp), [offset] "i" (THREAD_SIZE - STACK_FRAME_OVERHEAD),
>> + [callee] "i" (__do_softirq)
>> + : // Clobbers
>> + "lr", "xer", "ctr", "memory", "cr0", "cr1", "cr5", "cr6",
>> + "cr7", "r0", "r3", "r4", "r5", "r6", "r7", "r8", "r9", "r10",
>> + "r11", "r12"
>> + );
^ permalink raw reply
* Re: [PATCH] powerpc: Avoid clang uninitialized warning in __get_user_size_allowed
From: Christophe Leroy @ 2021-04-27 5:05 UTC (permalink / raw)
To: Nathan Chancellor, Michael Ellerman
Cc: clang-built-linux, linuxppc-dev, Nick Desaulniers, linux-kernel,
Paul Mackerras
In-Reply-To: <20210426203518.981550-1-nathan@kernel.org>
Le 26/04/2021 à 22:35, Nathan Chancellor a écrit :
> Commit 9975f852ce1b ("powerpc/uaccess: Remove calls to __get_user_bad()
> and __put_user_bad()") switch to BUILD_BUG() in the default case, which
> leaves x uninitialized. This will not be an issue because the build will
> be broken in that case but clang does static analysis before it realizes
> the default case will be done so it warns about x being uninitialized
> (trimmed for brevity):
>
> In file included from mm/mprotect.c:13:
> In file included from ./include/linux/hugetlb.h:28:
> In file included from ./include/linux/mempolicy.h:16:
> ./include/linux/pagemap.h:772:16: warning: variable '__gu_val' is used
> uninitialized whenever switch default is taken [-Wsometimes-uninitialized]
> if (unlikely(__get_user(c, uaddr) != 0))
> ^~~~~~~~~~~~~~~~~~~~
> ./arch/powerpc/include/asm/uaccess.h:266:2: note: expanded from macro '__get_user'
> __get_user_size_allowed(__gu_val, __gu_addr, __gu_size, __gu_err); \
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ./arch/powerpc/include/asm/uaccess.h:235:2: note: expanded from macro
> '__get_user_size_allowed'
> default: BUILD_BUG(); \
> ^~~~~~~
>
> Commit 5cd29b1fd3e8 ("powerpc/uaccess: Use asm goto for get_user when
> compiler supports it") added an initialization for x because of the same
> reason. Do the same thing here so there is no warning across all
> versions of clang.
Ah yes, I tested with Clang 11 which has CONFIG_CC_HAS_ASM_GOTO_OUTPUT, that's the reason why I hit
that warning only in the CONFIG_CC_HAS_ASM_GOTO_OUTPUT branch.
But regardless, is that normal that Clang warns that on a never taken branch ? That's puzzling.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1359
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
Acked-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/include/asm/uaccess.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> index a4e791bcd3fe..a09e4240c5b1 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -232,7 +232,7 @@ do { \
> case 2: __get_user_asm(x, (u16 __user *)ptr, retval, "lhz"); break; \
> case 4: __get_user_asm(x, (u32 __user *)ptr, retval, "lwz"); break; \
> case 8: __get_user_asm2(x, (u64 __user *)ptr, retval); break; \
> - default: BUILD_BUG(); \
> + default: x = 0; BUILD_BUG(); \
> } \
> } while (0)
>
>
> base-commit: ee6b25fa7c037e42cc5f3b5c024b2a779edab6dd
>
^ permalink raw reply
* [PATCH v6] powerpc/kexec_file: use current CPU info while setting up FDT
From: Sourabh Jain @ 2021-04-27 4:51 UTC (permalink / raw)
To: mpe; +Cc: mahesh, Sourabh Jain, linuxppc-dev, stable, hbathini, bauerman
kexec_file_load uses initial_boot_params in setting up the device-tree
for the kernel to be loaded. Though initial_boot_params holds info
about CPUs at the time of boot, it doesn't account for hot added CPUs.
So, kexec'ing with kexec_file_load syscall would leave the kexec'ed
kernel with inaccurate CPU info. Also, if kdump kernel is loaded with
kexec_file_load syscall and the system crashes on a hot added CPU,
capture kernel hangs failing to identify the boot CPU.
Kernel panic - not syncing: sysrq triggered crash
CPU: 24 PID: 6065 Comm: echo Kdump: loaded Not tainted 5.12.0-rc5upstream #54
Call Trace:
[c0000000e590fac0] [c0000000007b2400] dump_stack+0xc4/0x114 (unreliable)
[c0000000e590fb00] [c000000000145290] panic+0x16c/0x41c
[c0000000e590fba0] [c0000000008892e0] sysrq_handle_crash+0x30/0x40
[c0000000e590fc00] [c000000000889cdc] __handle_sysrq+0xcc/0x1f0
[c0000000e590fca0] [c00000000088a538] write_sysrq_trigger+0xd8/0x178
[c0000000e590fce0] [c0000000005e9b7c] proc_reg_write+0x10c/0x1b0
[c0000000e590fd10] [c0000000004f26d0] vfs_write+0xf0/0x330
[c0000000e590fd60] [c0000000004f2aec] ksys_write+0x7c/0x140
[c0000000e590fdb0] [c000000000031ee0] system_call_exception+0x150/0x290
[c0000000e590fe10] [c00000000000ca5c] system_call_common+0xec/0x278
--- interrupt: c00 at 0x7fff905b9664
NIP: 00007fff905b9664 LR: 00007fff905320c4 CTR: 0000000000000000
REGS: c0000000e590fe80 TRAP: 0c00 Not tainted (5.12.0-rc5upstream)
MSR: 800000000280f033 <SF,VEC,VSX,EE,PR,FP,ME,IR,DR,RI,LE> CR: 28000242
XER: 00000000
IRQMASK: 0
GPR00: 0000000000000004 00007ffff5fedf30 00007fff906a7300 0000000000000001
GPR04: 000001002a7355b0 0000000000000002 0000000000000001 00007ffff5fef616
GPR08: 0000000000000001 0000000000000000 0000000000000000 0000000000000000
GPR12: 0000000000000000 00007fff9073a160 0000000000000000 0000000000000000
GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR20: 0000000000000000 00007fff906a4ee0 0000000000000002 0000000000000001
GPR24: 00007fff906a0898 0000000000000000 0000000000000002 000001002a7355b0
GPR28: 0000000000000002 00007fff906a1790 000001002a7355b0 0000000000000002
NIP [00007fff905b9664] 0x7fff905b9664
LR [00007fff905320c4] 0x7fff905320c4
--- interrupt: c00
To avoid this from happening, extract current CPU info from of_root
device node and use it for setting up the fdt in kexec_file_load case.
Fixes: 6ecd0163d360 ("powerpc/kexec_file: Add appropriate regions for memory reserve map")
Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
Reviewed-by: Hari Bathini <hbathini@linux.ibm.com>
Cc: <stable@vger.kernel.org>
---
arch/powerpc/kexec/file_load_64.c | 88 +++++++++++++++++++++++++++++++
1 file changed, 88 insertions(+)
---
Changelog:
v1 -> v5
- https://lists.ozlabs.org/pipermail/linuxppc-dev/2021-April/227950.html
v5 -> v6
- use exiting macro (for_each_property_of_node) to loop through all
properties of a node.
- removed devtree_lock while accessing the node properties.
- function name update, add_node_prop to add_node_props.
---
diff --git a/arch/powerpc/kexec/file_load_64.c b/arch/powerpc/kexec/file_load_64.c
index 02b9e4d0dc40..4f7d4c10f939 100644
--- a/arch/powerpc/kexec/file_load_64.c
+++ b/arch/powerpc/kexec/file_load_64.c
@@ -960,6 +960,89 @@ unsigned int kexec_fdt_totalsize_ppc64(struct kimage *image)
return fdt_size;
}
+/**
+ * add_node_props - Reads node properties from device node structure and add
+ * them to fdt.
+ * @fdt: Flattened device tree of the kernel
+ * @node_offset: offset of the node to add a property at
+ * @dn: device node pointer
+ *
+ * Returns 0 on success, negative errno on error.
+ */
+static int add_node_props(void *fdt, int node_offset, const struct device_node *dn)
+{
+ int ret = 0;
+ struct property *pp;
+
+ if (!dn)
+ return -EINVAL;
+
+ for_each_property_of_node(dn, pp) {
+ ret = fdt_setprop(fdt, node_offset, pp->name, pp->value, pp->length);
+ if (ret < 0) {
+ pr_err("Unable to add %s property: %s\n", pp->name, fdt_strerror(ret));
+ return ret;
+ }
+ }
+ return ret;
+}
+
+/**
+ * update_cpus_node - Update cpus node of flattened device tree using of_root
+ * device node.
+ * @fdt: Flattened device tree of the kernel.
+ *
+ * Returns 0 on success, negative errno on error.
+ */
+static int update_cpus_node(void *fdt)
+{
+ struct device_node *cpus_node, *dn;
+ int cpus_offset, cpus_subnode_offset, ret = 0;
+
+ cpus_offset = fdt_path_offset(fdt, "/cpus");
+ if (cpus_offset < 0 && cpus_offset != -FDT_ERR_NOTFOUND) {
+ pr_err("Malformed device tree: error reading /cpus node: %s\n",
+ fdt_strerror(cpus_offset));
+ return cpus_offset;
+ }
+
+ if (cpus_offset > 0) {
+ ret = fdt_del_node(fdt, cpus_offset);
+ if (ret < 0) {
+ pr_err("Error deleting /cpus node: %s\n", fdt_strerror(ret));
+ return -EINVAL;
+ }
+ }
+
+ /* Add cpus node to fdt */
+ cpus_offset = fdt_add_subnode(fdt, fdt_path_offset(fdt, "/"), "cpus");
+ if (cpus_offset < 0) {
+ pr_err("Error creating /cpus node: %s\n", fdt_strerror(cpus_offset));
+ return -EINVAL;
+ }
+
+ /* Add cpus node properties */
+ cpus_node = of_find_node_by_path("/cpus");
+ ret = add_node_props(fdt, cpus_offset, cpus_node);
+ if (ret < 0)
+ return ret;
+
+ /* Loop through all subnodes of cpus and add them to fdt */
+ for_each_node_by_type(dn, "cpu") {
+ cpus_subnode_offset = fdt_add_subnode(fdt, cpus_offset, dn->full_name);
+ if (cpus_subnode_offset < 0) {
+ pr_err("Unable to add %s subnode: %s\n", dn->full_name,
+ fdt_strerror(cpus_subnode_offset));
+ return cpus_subnode_offset;
+ }
+ ret = add_node_props(fdt, cpus_subnode_offset, dn);
+ if (ret < 0)
+ return ret;
+ }
+ of_node_put(dn);
+ return ret;
+}
+
/**
* setup_new_fdt_ppc64 - Update the flattend device-tree of the kernel
* being loaded.
@@ -1020,6 +1103,11 @@ int setup_new_fdt_ppc64(const struct kimage *image, void *fdt,
}
}
+ /* Update cpus nodes information to account hotplug CPUs. */
+ ret = update_cpus_node(fdt);
+ if (ret < 0)
+ return ret;
+
/* Update memory reserve map */
ret = get_reserved_memory_ranges(&rmem);
if (ret)
--
2.26.3
^ permalink raw reply related
* Re: [PATCH net-next v4 2/2] of: net: fix of_get_mac_addr_nvmem() for non-platform devices
From: Benjamin Herrenschmidt @ 2021-04-26 23:44 UTC (permalink / raw)
To: Michael Walle, Rob Herring
Cc: Andrew Lunn, Paul Mackerras, Rafał Miłecki,
Nobuhiro Iwamatsu, moderated list:ARM/STM32 ARCHITECTURE,
Jerome Brunet, Neil Armstrong, Michal Simek, Jose Abreu,
NXP Linux Team, Mark Lee, Vadym Kochan, Sascha Hauer,
Lorenzo Bianconi, linux-omap, Greg Kroah-Hartman, linux-wireless,
linux-kernel, Pengutronix Kernel Team, Vladimir Oltean,
Claudiu Beznea, Jérôme Pouiller, Kunihiko Hayashi,
Chris Snook, Frank Rowand, Gregory Clement, Madalin Bucur,
Martin Blumenstingl, Murali Karicheri, Yisen Zhuang,
Alexandre Torgue, Wingman Kwok, Sean Wang, Maxime Ripard,
Claudiu Manoil, open list:ARM/Amlogic Meson..., Kalle Valo,
Mirko Lindner, Fugang Duan, Bryan Whitehead,
QCA ath9k Development, Microchip Linux Driver Support,
Taras Chornyi, Maxime Coquelin, Kevin Hilman, Heiner Kallweit,
Andreas Larsson, Giuseppe Cavallaro, Fabio Estevam,
Stanislaw Gruszka, Florian Fainelli, linux-staging, Chen-Yu Tsai,
maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, linux-arm-kernel,
Grygorii Strashko, Byungho An, Radhey Shyam Pandey,
Vladimir Zapolskiy, John Crispin, Salil Mehta, Sergei Shtylyov,
linux-oxnas, Shawn Guo, David S . Miller, Helmut Schaa,
Thomas Petazzoni, open list:MEDIA DRIVERS FOR RENESAS - FCP,
Ryder Lee, Russell King, Hauke Mehrtens, Jakub Kicinski,
Vivien Didelot, Sunil Goutham, Sebastian Hesselbarth, devicetree,
moderated list:ARM/Mediatek SoC support, Matthias Brugger,
Jernej Skrabec, netdev, Nicolas Ferre, Li Yang, Stephen Hemminger,
Vinod Koul, Joyce Ooi, linuxppc-dev, Felix Fietkau
In-Reply-To: <108f268a35843368466004f7fe5f9f88@walle.cc>
On Mon, 2021-04-26 at 12:54 +0200, Michael Walle wrote:
> Before I'll try to come up with a patch for this, I'd like to get
> your opinion on it.
>
> (1) replacing of_get_mac_address(node) with eth_get_mac_address(dev)
> might sometimes lead to confusing comments like in
> drivers/net/ethernet/allwinner/sun4i-emac.c:
>
> /* Read MAC-address from DT */
> ret = of_get_mac_address(np, ndev->dev_addr);
You could leave it or turn it into "from platform", doesn't matter...
> (2) What do you think of eth_get_mac_address(ndev). That is, the
Not sure what you mean, eth_platform_get_mac_address() takes the
address as an argument. I think what you want is a consolidated
nvmem_get_mac_address + eth_platform_get_mac_address that takes a
device, which would have no requirement of the bus_type at all.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH 2/9] ARM: PXA: Kill use of irq_create_strict_mappings()
From: Guenter Roeck @ 2021-04-26 22:39 UTC (permalink / raw)
To: Marc Zyngier
Cc: Rich Felker, Yoshinori Sato, linux-sh, Robert Jarzmik,
linux-kernel, Haojian Zhuang, linux-mips, Thomas Bogendoerfer,
linux-arm-kernel, Thomas Gleixner, linuxppc-dev, Daniel Mack
In-Reply-To: <20210406093557.1073423-3-maz@kernel.org>
On Tue, Apr 06, 2021 at 10:35:50AM +0100, Marc Zyngier wrote:
> irq_create_strict_mappings() is a poor way to allow the use of
> a linear IRQ domain as a legacy one. Let's be upfront about
> it and use a legacy domain when appropriate.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
When running the "mainstone" qemu emulation, this patch results
in many (32, actually) runtime warnings such as the following.
[ 0.528272] ------------[ cut here ]------------
[ 0.528285] WARNING: CPU: 0 PID: 1 at kernel/irq/irqdomain.c:550 irq_domain_associate+0x194/0x1f0
[ 0.528315] error: virq335 is not allocated
[ 0.528325] Modules linked in:
[ 0.528351] CPU: 0 PID: 1 Comm: swapper Tainted: G W 5.12.0-rc8-next-20210423 #1
[ 0.528372] Hardware name: Intel HCDDBBVA0 Development Platform (aka Mainstone)
[ 0.528387] Backtrace:
[ 0.528406] [<c06bd188>] (dump_backtrace) from [<c06bd468>] (show_stack+0x20/0x24)
[ 0.528441] r7:00000226 r6:c00796e8 r5:00000009 r4:c088d2a0
[ 0.528454] [<c06bd448>] (show_stack) from [<c06c11dc>] (dump_stack+0x28/0x30)
[ 0.528479] [<c06c11b4>] (dump_stack) from [<c002a2b8>] (__warn+0xe8/0x110)
[ 0.528507] r5:00000009 r4:c0872698
[ 0.528520] [<c002a1d0>] (__warn) from [<c06bdbc0>] (warn_slowpath_fmt+0xa0/0xe0)
[ 0.528551] r7:c00796e8 r6:00000226 r5:c0872698 r4:c0872700
[ 0.528564] [<c06bdb24>] (warn_slowpath_fmt) from [<c00796e8>] (irq_domain_associate+0x194/0x1f0)
[ 0.528597] r8:00000130 r7:0000014f r6:0000001f r5:00000000 r4:c11bd780
[ 0.528610] [<c0079554>] (irq_domain_associate) from [<c00797a4>] (irq_domain_associate_many+0x60/0xa4)
[ 0.528642] r8:00000130 r7:c11bd780 r6:fffffed0 r5:00000150 r4:00000150
[ 0.528655] [<c0079744>] (irq_domain_associate_many) from [<c0079e5c>] (irq_domain_create_legacy+0x5c/0x68)
[ 0.528687] r8:00000130 r7:00000130 r6:00000020 r5:00000000 r4:c11bd780
[ 0.528699] [<c0079e00>] (irq_domain_create_legacy) from [<c0079e9c>] (irq_domain_add_legacy+0x34/0x3c)
[ 0.528730] r7:c09b1370 r6:c09b1360 r5:c11bd3a0 r4:00000000
[ 0.528743] [<c0079e68>] (irq_domain_add_legacy) from [<c0024f28>] (cplds_probe+0x170/0x1ac)
[ 0.528768] [<c0024db8>] (cplds_probe) from [<c0432cec>] (platform_probe+0x50/0xb0)
[ 0.528800] r8:c09d2c94 r7:c0aa4f88 r6:c09d2c94 r5:c09b1370 r4:00000000
[ 0.528814] [<c0432c9c>] (platform_probe) from [<c042fc70>] (really_probe+0x100/0x4d4)
[ 0.528844] r7:c0aa4f88 r6:00000000 r5:00000000 r4:c09b1370
[ 0.528858] [<c042fb70>] (really_probe) from [<c04300cc>] (driver_probe_device+0x88/0x20c)
[ 0.528892] r10:c0974830 r9:c0a70000 r8:c093b224 r7:c0a31de8 r6:c09d2c94 r5:c09d2c94
[ 0.528907] r4:c09b1370
[ 0.528919] [<c0430044>] (driver_probe_device) from [<c04306cc>] (device_driver_attach+0x68/0x70)
[ 0.528953] r9:c0a70000 r8:c093b224 r7:c0a31de8 r6:c09d2c94 r5:00000000 r4:c09b1370
[ 0.528969] [<c0430664>] (device_driver_attach) from [<c0430794>] (__driver_attach+0xc0/0x164)
[ 0.528997] r7:c0a31de8 r6:c09b1370 r5:c09d2c94 r4:00000000
[ 0.529009] [<c04306d4>] (__driver_attach) from [<c042d8d0>] (bus_for_each_dev+0x84/0xcc)
[ 0.529039] r7:c0a31de8 r6:c04306d4 r5:c09d2c94 r4:00000000
[ 0.529052] [<c042d84c>] (bus_for_each_dev) from [<c042f494>] (driver_attach+0x28/0x30)
[ 0.529082] r6:00000000 r5:c11bd200 r4:c09d2c94
[ 0.529095] [<c042f46c>] (driver_attach) from [<c042edac>] (bus_add_driver+0x168/0x210)
[ 0.529122] [<c042ec44>] (bus_add_driver) from [<c0431304>] (driver_register+0x88/0x120)
[ 0.529152] r7:c0a5c7e0 r6:00000000 r5:ffffe000 r4:c09d2c94
[ 0.529165] [<c043127c>] (driver_register) from [<c04329a0>] (__platform_driver_register+0x2c/0x34)
[ 0.529191] r5:ffffe000 r4:c094ba64
[ 0.529204] [<c0432974>] (__platform_driver_register) from [<c094ba84>] (cplds_driver_init+0x20/0x28)
[ 0.529230] [<c094ba64>] (cplds_driver_init) from [<c000a2a8>] (do_one_initcall+0x60/0x27c)
[ 0.529255] [<c000a248>] (do_one_initcall) from [<c093f244>] (kernel_init_freeable+0x158/0x1e4)
[ 0.529284] r7:c0974850 r6:00000007 r5:c0c0f720 r4:c09a02fc
[ 0.529297] [<c093f0ec>] (kernel_init_freeable) from [<c06c5274>] (kernel_init+0x18/0x110)
[ 0.529328] r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c06c525c
[ 0.529343] r4:00000000
[ 0.529354] [<c06c525c>] (kernel_init) from [<c0008348>] (ret_from_fork+0x14/0x2c)
[ 0.529387] Exception stack(0xc0bdffb0 to 0xc0bdfff8)
[ 0.529467] ffa0: 00000000 00000000 00000000 00000000
[ 0.529587] ffc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[ 0.529684] ffe0: 00000000 00000000 00000000 00000000 00000013 00000000
[ 0.529726] r5:c06c525c r4:00000000
[ 0.529752] ---[ end trace d199929d2b87e077 ]---
Bisect log attached.
Guenter
---
# bad: [e3d35712f85ac84fb06234848f6c043ab418cf8b] Add linux-next specific files for 20210423
# good: [bf05bf16c76bb44ab5156223e1e58e26dfe30a88] Linux 5.12-rc8
git bisect start 'HEAD' 'v5.12-rc8'
# good: [d4b5d9d94679a18bfa4ccdafd19876d58777911e] Merge remote-tracking branch 'crypto/master'
git bisect good d4b5d9d94679a18bfa4ccdafd19876d58777911e
# good: [27628e42fe59a698e66b671bf1e1f01f6a3fe765] Merge remote-tracking branch 'tip/auto-latest'
git bisect good 27628e42fe59a698e66b671bf1e1f01f6a3fe765
# bad: [bc6c3ae4f662fc719d0bf144f150f72cab8912d4] Merge remote-tracking branch 'vfio/next'
git bisect bad bc6c3ae4f662fc719d0bf144f150f72cab8912d4
# bad: [5ff5b00609c64a043ccd5bc92273c132b33f7f9a] Merge remote-tracking branch 'driver-core/driver-core-next'
git bisect bad 5ff5b00609c64a043ccd5bc92273c132b33f7f9a
# bad: [c878be9c883153797d5749620e58f180cc429e88] Merge remote-tracking branch 'kvm/next'
git bisect bad c878be9c883153797d5749620e58f180cc429e88
# good: [52acd22faa1af8a0514ccd075a6978ac97986425] KVM: Boost vCPU candidate in user mode which is delivering interrupt
git bisect good 52acd22faa1af8a0514ccd075a6978ac97986425
# good: [988aab640a6c46ab9552e65c2c3a8d577a4e30f3] rcu: Make rcu_gp_cleanup() be noinline for tracing
git bisect good 988aab640a6c46ab9552e65c2c3a8d577a4e30f3
# bad: [6603c2d8bd6cc7fa591fd3b4232bf25b65a0ea8f] Merge remote-tracking branch 'ftrace/for-next'
git bisect bad 6603c2d8bd6cc7fa591fd3b4232bf25b65a0ea8f
# good: [c658797f1a70561205a224be0c8be64977ed64e8] tracing: Add method for recording "func_repeats" events
git bisect good c658797f1a70561205a224be0c8be64977ed64e8
# good: [46135d6f878ab00261d4a2082d620bfb41019aab] irqchip/gic-v4.1: Disable vSGI upon (GIC CPUIF < v4.1) detection
git bisect good 46135d6f878ab00261d4a2082d620bfb41019aab
# bad: [05d7bf817019890e4d049e0b851940c596adbd9b] dt-bindings: interrupt-controller: Add IDT 79RC3243x Interrupt Controller
git bisect bad 05d7bf817019890e4d049e0b851940c596adbd9b
# bad: [1a0b05e435544cd53cd3936bdab425d88784b71a] irqdomain: Get rid of irq_create_strict_mappings()
git bisect bad 1a0b05e435544cd53cd3936bdab425d88784b71a
# bad: [5f8b938bd790cff6542c7fe3c1495c71f89fef1b] irqchip/jcore-aic: Kill use of irq_create_strict_mappings()
git bisect bad 5f8b938bd790cff6542c7fe3c1495c71f89fef1b
# bad: [b68761da01114a64b9c521975c3bca6d10eeb950] ARM: PXA: Kill use of irq_create_strict_mappings()
git bisect bad b68761da01114a64b9c521975c3bca6d10eeb950
# first bad commit: [b68761da01114a64b9c521975c3bca6d10eeb950] ARM: PXA: Kill use of irq_create_strict_mappings()
^ permalink raw reply
* Re: [PATCH] kbuild: replace LANG=C with LC_ALL=C
From: Matthieu Baerts @ 2021-04-26 19:30 UTC (permalink / raw)
To: Masahiro Yamada, linux-kbuild
Cc: linuxppc-dev, Mat Martineau, Matthias Maennich, linux-kernel,
netdev, Paul Mackerras, mptcp, linux-kselftest, Jakub Kicinski,
Shuah Khan, David S. Miller
In-Reply-To: <20210424114841.394239-1-masahiroy@kernel.org>
Hi,
Thank you for the patch!
On 24/04/2021 13:48, Masahiro Yamada wrote:
> LANG gives a weak default to each LC_* in case it is not explicitly
> defined. LC_ALL, if set, overrides all other LC_* variables.
>
> LANG < LC_CTYPE, LC_COLLATE, LC_MONETARY, LC_NUMERIC, ... < LC_ALL
>
> This is why documentation such as [1] suggests to set LC_ALL in build
> scripts to get the deterministic result.
>
> LANG=C is not strong enough to override LC_* that may be set by end
> users.
>
> [1]: https://reproducible-builds.org/docs/locales/
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
> arch/powerpc/boot/wrapper | 2 +-
> scripts/nsdeps | 2 +-
> scripts/recordmcount.pl | 2 +-
> scripts/setlocalversion | 2 +-
> scripts/tags.sh | 2 +-
> tools/testing/selftests/net/mptcp/mptcp_connect.sh | 2 +-
Acked-by: Matthieu Baerts <matthieu.baerts@tessares.net> (mptcp)
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
^ permalink raw reply
* Re: [PATCH v2 2/2] powerpc: If kexec_build_elf_info() fails return immediately from elf64_load()
From: Rob Herring @ 2021-04-26 21:26 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: devicetree, kbuild-all, lkp, bauerman, linuxppc-dev,
dan.carpenter, dja
In-Reply-To: <20210421163610.23775-2-nramas@linux.microsoft.com>
On Wed, 21 Apr 2021 09:36:10 -0700, Lakshmi Ramasubramanian wrote:
> Uninitialized local variable "elf_info" would be passed to
> kexec_free_elf_info() if kexec_build_elf_info() returns an error
> in elf64_load().
>
> If kexec_build_elf_info() returns an error, return the error
> immediately.
>
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Reviewed-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
> arch/powerpc/kexec/elf_64.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Applied, thanks!
^ permalink raw reply
* Re: [PATCH v2 1/2] powerpc: Free fdt on error in elf64_load()
From: Rob Herring @ 2021-04-26 21:25 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: devicetree, kbuild-all, lkp, bauerman, linuxppc-dev,
dan.carpenter, dja
In-Reply-To: <20210421163610.23775-1-nramas@linux.microsoft.com>
On Wed, 21 Apr 2021 09:36:09 -0700, Lakshmi Ramasubramanian wrote:
> There are a few "goto out;" statements before the local variable "fdt"
> is initialized through the call to of_kexec_alloc_and_setup_fdt() in
> elf64_load(). This will result in an uninitialized "fdt" being passed
> to kvfree() in this function if there is an error before the call to
> of_kexec_alloc_and_setup_fdt().
>
> If there is any error after fdt is allocated, but before it is
> saved in the arch specific kimage struct, free the fdt.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> ---
> arch/powerpc/kexec/elf_64.c | 16 ++++++----------
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
Applied, thanks!
^ permalink raw reply
* [PATCH] powerpc: Avoid clang uninitialized warning in __get_user_size_allowed
From: Nathan Chancellor @ 2021-04-26 20:35 UTC (permalink / raw)
To: Michael Ellerman
Cc: Nick Desaulniers, linux-kernel, Nathan Chancellor,
clang-built-linux, Paul Mackerras, linuxppc-dev
Commit 9975f852ce1b ("powerpc/uaccess: Remove calls to __get_user_bad()
and __put_user_bad()") switch to BUILD_BUG() in the default case, which
leaves x uninitialized. This will not be an issue because the build will
be broken in that case but clang does static analysis before it realizes
the default case will be done so it warns about x being uninitialized
(trimmed for brevity):
In file included from mm/mprotect.c:13:
In file included from ./include/linux/hugetlb.h:28:
In file included from ./include/linux/mempolicy.h:16:
./include/linux/pagemap.h:772:16: warning: variable '__gu_val' is used
uninitialized whenever switch default is taken [-Wsometimes-uninitialized]
if (unlikely(__get_user(c, uaddr) != 0))
^~~~~~~~~~~~~~~~~~~~
./arch/powerpc/include/asm/uaccess.h:266:2: note: expanded from macro '__get_user'
__get_user_size_allowed(__gu_val, __gu_addr, __gu_size, __gu_err); \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./arch/powerpc/include/asm/uaccess.h:235:2: note: expanded from macro
'__get_user_size_allowed'
default: BUILD_BUG(); \
^~~~~~~
Commit 5cd29b1fd3e8 ("powerpc/uaccess: Use asm goto for get_user when
compiler supports it") added an initialization for x because of the same
reason. Do the same thing here so there is no warning across all
versions of clang.
Link: https://github.com/ClangBuiltLinux/linux/issues/1359
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
arch/powerpc/include/asm/uaccess.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index a4e791bcd3fe..a09e4240c5b1 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -232,7 +232,7 @@ do { \
case 2: __get_user_asm(x, (u16 __user *)ptr, retval, "lhz"); break; \
case 4: __get_user_asm(x, (u32 __user *)ptr, retval, "lwz"); break; \
case 8: __get_user_asm2(x, (u64 __user *)ptr, retval); break; \
- default: BUILD_BUG(); \
+ default: x = 0; BUILD_BUG(); \
} \
} while (0)
base-commit: ee6b25fa7c037e42cc5f3b5c024b2a779edab6dd
--
2.31.1.362.g311531c9de
^ permalink raw reply related
* Re: [PATCH v7] powerpc/irq: Inline call_do_irq() and call_do_softirq()
From: Nathan Chancellor @ 2021-04-26 18:50 UTC (permalink / raw)
To: Michael Ellerman; +Cc: clang-built-linux, linuxppc-dev
In-Reply-To: <20210320122227.345427-1-mpe@ellerman.id.au>
On Sat, Mar 20, 2021 at 11:22:27PM +1100, Michael Ellerman wrote:
> From: Christophe Leroy <christophe.leroy@csgroup.eu>
>
> call_do_irq() and call_do_softirq() are simple enough to be
> worth inlining.
>
> Inlining them avoids an mflr/mtlr pair plus a save/reload on stack. It
> also allows GCC to keep the saved ksp_limit in an nonvolatile reg.
>
> This is inspired from S390 arch. Several other arches do more or
> less the same. The way sparc arch does seems odd thought.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>
> v2: no change.
> v3: no change.
> v4:
> - comment reminding the purpose of the inline asm block.
> - added r2 as clobbered reg
> v5:
> - Limiting the change to PPC32 for now.
> - removed r2 from the clobbered regs list (on PPC32 r2 points to current all the time)
> - Removed patch 1 and merged ksp_limit handling in here.
> v6:
> - Rebase on top of merge-test (ca6e327fefb2).
> - Remove the ksp_limit stuff as it's doesn't exist anymore.
>
> v7:
> mpe:
> - Enable for 64-bit too. This all in-kernel code calling in-kernel
> code, and must use the kernel TOC.
> - Use named parameters for the inline asm.
> - Reformat inline asm.
> - Mark as always_inline.
> - Drop unused ret from call_do_softirq(), add r3 as clobbered.
> ---
> arch/powerpc/include/asm/irq.h | 2 --
> arch/powerpc/kernel/irq.c | 41 ++++++++++++++++++++++++++++++++++
> arch/powerpc/kernel/misc_32.S | 25 ---------------------
> arch/powerpc/kernel/misc_64.S | 22 ------------------
> 4 files changed, 41 insertions(+), 49 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/irq.h b/arch/powerpc/include/asm/irq.h
> index f3f264e441a7..b2bd58830430 100644
> --- a/arch/powerpc/include/asm/irq.h
> +++ b/arch/powerpc/include/asm/irq.h
> @@ -53,8 +53,6 @@ extern void *mcheckirq_ctx[NR_CPUS];
> extern void *hardirq_ctx[NR_CPUS];
> extern void *softirq_ctx[NR_CPUS];
>
> -void call_do_softirq(void *sp);
> -void call_do_irq(struct pt_regs *regs, void *sp);
> extern void do_IRQ(struct pt_regs *regs);
> extern void __init init_IRQ(void);
> extern void __do_irq(struct pt_regs *regs);
> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
> index 5b72abbff96c..260effc0a435 100644
> --- a/arch/powerpc/kernel/irq.c
> +++ b/arch/powerpc/kernel/irq.c
> @@ -667,6 +667,47 @@ static inline void check_stack_overflow(void)
> }
> }
>
> +static __always_inline void call_do_softirq(const void *sp)
> +{
> + /* Temporarily switch r1 to sp, call __do_softirq() then restore r1. */
> + asm volatile (
> + PPC_STLU " %%r1, %[offset](%[sp]) ;"
> + "mr %%r1, %[sp] ;"
> + "bl %[callee] ;"
> + PPC_LL " %%r1, 0(%%r1) ;"
> + : // Outputs
> + : // Inputs
> + [sp] "b" (sp), [offset] "i" (THREAD_SIZE - STACK_FRAME_OVERHEAD),
> + [callee] "i" (__do_softirq)
> + : // Clobbers
> + "lr", "xer", "ctr", "memory", "cr0", "cr1", "cr5", "cr6",
> + "cr7", "r0", "r3", "r4", "r5", "r6", "r7", "r8", "r9", "r10",
> + "r11", "r12"
> + );
> +}
> +
> +static __always_inline void call_do_irq(struct pt_regs *regs, void *sp)
> +{
> + register unsigned long r3 asm("r3") = (unsigned long)regs;
> +
> + /* Temporarily switch r1 to sp, call __do_irq() then restore r1. */
> + asm volatile (
> + PPC_STLU " %%r1, %[offset](%[sp]) ;"
> + "mr %%r1, %[sp] ;"
> + "bl %[callee] ;"
> + PPC_LL " %%r1, 0(%%r1) ;"
> + : // Outputs
> + "+r" (r3)
> + : // Inputs
> + [sp] "b" (sp), [offset] "i" (THREAD_SIZE - STACK_FRAME_OVERHEAD),
> + [callee] "i" (__do_irq)
> + : // Clobbers
> + "lr", "xer", "ctr", "memory", "cr0", "cr1", "cr5", "cr6",
> + "cr7", "r0", "r4", "r5", "r6", "r7", "r8", "r9", "r10",
> + "r11", "r12"
> + );
> +}
> +
> void __do_irq(struct pt_regs *regs)
> {
> unsigned int irq;
> diff --git a/arch/powerpc/kernel/misc_32.S b/arch/powerpc/kernel/misc_32.S
> index acc410043b96..6a076bef2932 100644
> --- a/arch/powerpc/kernel/misc_32.S
> +++ b/arch/powerpc/kernel/misc_32.S
> @@ -27,31 +27,6 @@
>
> .text
>
> -_GLOBAL(call_do_softirq)
> - mflr r0
> - stw r0,4(r1)
> - stwu r1,THREAD_SIZE-STACK_FRAME_OVERHEAD(r3)
> - mr r1,r3
> - bl __do_softirq
> - lwz r1,0(r1)
> - lwz r0,4(r1)
> - mtlr r0
> - blr
> -
> -/*
> - * void call_do_irq(struct pt_regs *regs, void *sp);
> - */
> -_GLOBAL(call_do_irq)
> - mflr r0
> - stw r0,4(r1)
> - stwu r1,THREAD_SIZE-STACK_FRAME_OVERHEAD(r4)
> - mr r1,r4
> - bl __do_irq
> - lwz r1,0(r1)
> - lwz r0,4(r1)
> - mtlr r0
> - blr
> -
> /*
> * This returns the high 64 bits of the product of two 64-bit numbers.
> */
> diff --git a/arch/powerpc/kernel/misc_64.S b/arch/powerpc/kernel/misc_64.S
> index 070465825c21..4b761a18a74d 100644
> --- a/arch/powerpc/kernel/misc_64.S
> +++ b/arch/powerpc/kernel/misc_64.S
> @@ -27,28 +27,6 @@
>
> .text
>
> -_GLOBAL(call_do_softirq)
> - mflr r0
> - std r0,16(r1)
> - stdu r1,THREAD_SIZE-STACK_FRAME_OVERHEAD(r3)
> - mr r1,r3
> - bl __do_softirq
> - ld r1,0(r1)
> - ld r0,16(r1)
> - mtlr r0
> - blr
> -
> -_GLOBAL(call_do_irq)
> - mflr r0
> - std r0,16(r1)
> - stdu r1,THREAD_SIZE-STACK_FRAME_OVERHEAD(r4)
> - mr r1,r4
> - bl __do_irq
> - ld r1,0(r1)
> - ld r0,16(r1)
> - mtlr r0
> - blr
> -
> _GLOBAL(__bswapdi2)
> EXPORT_SYMBOL(__bswapdi2)
> srdi r8,r3,32
> --
> 2.25.1
>
This change caused our ppc44x_defconfig builds to hang when powering
down in QEMU:
https://github.com/ClangBuiltLinux/continuous-integration2/runs/2304364629?check_suite_focus=true#logs
This is probably something with clang given that GCC 10.3.0 works fine
but due to the nature of the change, I have no idea how to tell what is
going wrong. I tried to do some rudimentary debugging with gdb but that
did not really get me anywhere.
The kernel was built with just 'CC=clang' and it is reproducible with
all versions of clang that the kernel supports.
The QEMU invocation is visible at the link above, it is done with our
boot-qemu.sh in this repo, which also houses the rootfs:
https://github.com/ClangBuiltLinux/boot-utils
Happy to provide any other information or debug/test as directed!
Cheers,
Nathan
^ permalink raw reply
* Re: [PATCH v5 16/16] of: Add plumbing for restricted DMA pool
From: Claire Chang @ 2021-04-26 16:38 UTC (permalink / raw)
To: Robin Murphy
Cc: heikki.krogerus, thomas.hellstrom, peterz, joonas.lahtinen,
dri-devel, lkml, grant.likely, paulus, Will Deacon, mingo,
Marek Szyprowski, sstabellini, Saravana Kannan, xypron.glpk,
Joerg Roedel, Rafael J . Wysocki, Christoph Hellwig,
Bartosz Golaszewski, bskeggs, linux-pci, xen-devel,
Thierry Reding, intel-gfx, matthew.auld, linux-devicetree,
Jianxiong Gao, Daniel Vetter, Konrad Rzeszutek Wilk,
maarten.lankhorst, airlied, Dan Williams, jani.nikula,
Nicolas Boichat, rodrigo.vivi, Bjorn Helgaas, boris.ostrovsky,
Andy Shevchenko, jgross, chris, nouveau, Greg KH, Randy Dunlap,
Frank Rowand, Tomasz Figa, list@263.net:IOMMU DRIVERS,
Jim Quinlan, linuxppc-dev, bauerman
In-Reply-To: <03c5bc8a-3965-bf1d-01a4-97d074dfbe2b@arm.com>
On Fri, Apr 23, 2021 at 9:35 PM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2021-04-22 09:15, Claire Chang wrote:
> > If a device is not behind an IOMMU, we look up the device node and set
> > up the restricted DMA when the restricted-dma-pool is presented.
> >
> > Signed-off-by: Claire Chang <tientzu@chromium.org>
> > ---
> > drivers/of/address.c | 25 +++++++++++++++++++++++++
> > drivers/of/device.c | 3 +++
> > drivers/of/of_private.h | 5 +++++
> > 3 files changed, 33 insertions(+)
> >
> > diff --git a/drivers/of/address.c b/drivers/of/address.c
> > index 54f221dde267..fff3adfe4986 100644
> > --- a/drivers/of/address.c
> > +++ b/drivers/of/address.c
> > @@ -8,6 +8,7 @@
> > #include <linux/logic_pio.h>
> > #include <linux/module.h>
> > #include <linux/of_address.h>
> > +#include <linux/of_reserved_mem.h>
> > #include <linux/pci.h>
> > #include <linux/pci_regs.h>
> > #include <linux/sizes.h>
> > @@ -1109,6 +1110,30 @@ bool of_dma_is_coherent(struct device_node *np)
> > }
> > EXPORT_SYMBOL_GPL(of_dma_is_coherent);
> >
> > +int of_dma_set_restricted_buffer(struct device *dev)
> > +{
> > + struct device_node *node;
> > + int count, i;
> > +
> > + if (!dev->of_node)
> > + return 0;
> > +
> > + count = of_property_count_elems_of_size(dev->of_node, "memory-region",
> > + sizeof(phandle));
> > + for (i = 0; i < count; i++) {
> > + node = of_parse_phandle(dev->of_node, "memory-region", i);
> > + /* There might be multiple memory regions, but only one
> > + * restriced-dma-pool region is allowed.
> > + */
>
> What's the use-case for having multiple regions if the restricted pool
> is by definition the only one accessible?
There might be a device coherent pool (shared-dma-pool) and
dma_alloc_attrs might allocate memory from that pool [1].
I'm not sure if it makes sense to have another device coherent pool
while using restricted DMA pool though.
[1] https://elixir.bootlin.com/linux/v5.12/source/kernel/dma/mapping.c#L435
>
> Robin.
>
> > + if (of_device_is_compatible(node, "restricted-dma-pool") &&
> > + of_device_is_available(node))
> > + return of_reserved_mem_device_init_by_idx(
> > + dev, dev->of_node, i);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > /**
> > * of_mmio_is_nonposted - Check if device uses non-posted MMIO
> > * @np: device node
> > diff --git a/drivers/of/device.c b/drivers/of/device.c
> > index c5a9473a5fb1..d8d865223e51 100644
> > --- a/drivers/of/device.c
> > +++ b/drivers/of/device.c
> > @@ -165,6 +165,9 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
> >
> > arch_setup_dma_ops(dev, dma_start, size, iommu, coherent);
> >
> > + if (!iommu)
> > + return of_dma_set_restricted_buffer(dev);
> > +
> > return 0;
> > }
> > EXPORT_SYMBOL_GPL(of_dma_configure_id);
> > diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> > index d717efbd637d..e9237f5eff48 100644
> > --- a/drivers/of/of_private.h
> > +++ b/drivers/of/of_private.h
> > @@ -163,12 +163,17 @@ struct bus_dma_region;
> > #if defined(CONFIG_OF_ADDRESS) && defined(CONFIG_HAS_DMA)
> > int of_dma_get_range(struct device_node *np,
> > const struct bus_dma_region **map);
> > +int of_dma_set_restricted_buffer(struct device *dev);
> > #else
> > static inline int of_dma_get_range(struct device_node *np,
> > const struct bus_dma_region **map)
> > {
> > return -ENODEV;
> > }
> > +static inline int of_dma_get_restricted_buffer(struct device *dev)
> > +{
> > + return -ENODEV;
> > +}
> > #endif
> >
> > #endif /* _LINUX_OF_PRIVATE_H */
> >
^ permalink raw reply
* Re: [PATCH v5 05/16] swiotlb: Add restricted DMA pool initialization
From: Claire Chang @ 2021-04-26 16:37 UTC (permalink / raw)
To: Steven Price
Cc: heikki.krogerus, thomas.hellstrom, peterz, joonas.lahtinen,
dri-devel, lkml, grant.likely, paulus, Will Deacon, mingo,
Marek Szyprowski, sstabellini, Saravana Kannan, xypron.glpk,
Joerg Roedel, Rafael J . Wysocki, Christoph Hellwig,
Bartosz Golaszewski, bskeggs, linux-pci, xen-devel,
Thierry Reding, intel-gfx, matthew.auld, linux-devicetree,
Jianxiong Gao, Daniel Vetter, Konrad Rzeszutek Wilk,
maarten.lankhorst, airlied, Dan Williams, linuxppc-dev,
jani.nikula, Nicolas Boichat, rodrigo.vivi, Bjorn Helgaas,
boris.ostrovsky, Andy Shevchenko, jgross, chris, nouveau, Greg KH,
Randy Dunlap, Frank Rowand, Tomasz Figa,
list@263.net:IOMMU DRIVERS, Jim Quinlan, Robin Murphy, bauerman
In-Reply-To: <c9abca62-328d-d0d6-a8a6-a67475171f92@arm.com>
On Fri, Apr 23, 2021 at 7:34 PM Steven Price <steven.price@arm.com> wrote:
>
> On 22/04/2021 09:14, Claire Chang wrote:
> > Add the initialization function to create restricted DMA pools from
> > matching reserved-memory nodes.
> >
> > Signed-off-by: Claire Chang <tientzu@chromium.org>
> > ---
> > include/linux/device.h | 4 +++
> > include/linux/swiotlb.h | 3 +-
> > kernel/dma/swiotlb.c | 80 +++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 86 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index 38a2071cf776..4987608ea4ff 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -416,6 +416,7 @@ struct dev_links_info {
> > * @dma_pools: Dma pools (if dma'ble device).
> > * @dma_mem: Internal for coherent mem override.
> > * @cma_area: Contiguous memory area for dma allocations
> > + * @dma_io_tlb_mem: Internal for swiotlb io_tlb_mem override.
> > * @archdata: For arch-specific additions.
> > * @of_node: Associated device tree node.
> > * @fwnode: Associated device node supplied by platform firmware.
> > @@ -521,6 +522,9 @@ struct device {
> > #ifdef CONFIG_DMA_CMA
> > struct cma *cma_area; /* contiguous memory area for dma
> > allocations */
> > +#endif
> > +#ifdef CONFIG_DMA_RESTRICTED_POOL
> > + struct io_tlb_mem *dma_io_tlb_mem;
> > #endif
> > /* arch specific additions */
> > struct dev_archdata archdata;
> > diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
> > index 216854a5e513..03ad6e3b4056 100644
> > --- a/include/linux/swiotlb.h
> > +++ b/include/linux/swiotlb.h
> > @@ -72,7 +72,8 @@ extern enum swiotlb_force swiotlb_force;
> > * range check to see if the memory was in fact allocated by this
> > * API.
> > * @nslabs: The number of IO TLB blocks (in groups of 64) between @start and
> > - * @end. This is command line adjustable via setup_io_tlb_npages.
> > + * @end. For default swiotlb, this is command line adjustable via
> > + * setup_io_tlb_npages.
> > * @used: The number of used IO TLB block.
> > * @list: The free list describing the number of free entries available
> > * from each index.
> > diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> > index 57a9adb920bf..ffbb8724e06c 100644
> > --- a/kernel/dma/swiotlb.c
> > +++ b/kernel/dma/swiotlb.c
> > @@ -39,6 +39,13 @@
> > #ifdef CONFIG_DEBUG_FS
> > #include <linux/debugfs.h>
> > #endif
> > +#ifdef CONFIG_DMA_RESTRICTED_POOL
> > +#include <linux/io.h>
> > +#include <linux/of.h>
> > +#include <linux/of_fdt.h>
> > +#include <linux/of_reserved_mem.h>
> > +#include <linux/slab.h>
> > +#endif
> >
> > #include <asm/io.h>
> > #include <asm/dma.h>
> > @@ -681,3 +688,76 @@ static int __init swiotlb_create_default_debugfs(void)
> > late_initcall(swiotlb_create_default_debugfs);
> >
> > #endif
> > +
> > +#ifdef CONFIG_DMA_RESTRICTED_POOL
> > +static int rmem_swiotlb_device_init(struct reserved_mem *rmem,
> > + struct device *dev)
> > +{
> > + struct io_tlb_mem *mem = rmem->priv;
> > + unsigned long nslabs = rmem->size >> IO_TLB_SHIFT;
> > +
> > + if (dev->dma_io_tlb_mem)
> > + return 0;
> > +
> > + /* Since multiple devices can share the same pool, the private data,
> > + * io_tlb_mem struct, will be initialized by the first device attached
> > + * to it.
> > + */
> > + if (!mem) {
> > + mem = kzalloc(struct_size(mem, slots, nslabs), GFP_KERNEL);
> > + if (!mem)
> > + return -ENOMEM;
> > +#ifdef CONFIG_ARM
> > + if (!PageHighMem(pfn_to_page(PHYS_PFN(rmem->base)))) {
> > + kfree(mem);
> > + return -EINVAL;
> > + }
> > +#endif /* CONFIG_ARM */
> > + swiotlb_init_io_tlb_mem(mem, rmem->base, nslabs, false);
> > +
> > + rmem->priv = mem;
> > + }
> > +
> > +#ifdef CONFIG_DEBUG_FS
> > + if (!io_tlb_default_mem->debugfs)
> > + io_tlb_default_mem->debugfs =
> > + debugfs_create_dir("swiotlb", NULL);
>
> At this point it's possible for io_tlb_default_mem to be NULL, leading
> to a splat.
Thanks for pointing this out.
>
> But even then if it's not and we have the situation where debugfs==NULL
> then the debugfs_create_dir() here will cause a subsequent attempt in
> swiotlb_create_debugfs() to fail (directory already exists) leading to
> mem->debugfs being assigned an error value. I suspect the creation of
> the debugfs directory needs to be separated from io_tlb_default_mem
> being set.
debugfs creation should move into the if (!mem) {...} above to avoid
duplication.
I think having a separated struct dentry pointer for the default
debugfs should be enough?
if (!debugfs)
debugfs = debugfs_create_dir("swiotlb", NULL);
swiotlb_create_debugfs(mem, rmem->name, debugfs);
>
> Other than that I gave this series a go with our prototype of Arm's
> Confidential Computer Architecture[1] - since the majority of the
> guest's memory is protected from the host the restricted DMA pool allows
> (only) a small area to be shared with the host.
>
> After fixing (well hacking round) the above it all seems to be working
> fine with virtio drivers.
>
> Thanks,
>
> Steve
>
> [1]
> https://www.arm.com/why-arm/architecture/security-features/arm-confidential-compute-architecture
^ permalink raw reply
* Re: [PATCH v5 08/16] swiotlb: Update is_swiotlb_active to add a struct device argument
From: Claire Chang @ 2021-04-26 16:37 UTC (permalink / raw)
To: Robin Murphy
Cc: heikki.krogerus, thomas.hellstrom, peterz, joonas.lahtinen,
dri-devel, lkml, grant.likely, paulus, Will Deacon, mingo,
Marek Szyprowski, sstabellini, Saravana Kannan, xypron.glpk,
Joerg Roedel, Rafael J . Wysocki, Christoph Hellwig,
Bartosz Golaszewski, bskeggs, linux-pci, xen-devel,
Thierry Reding, intel-gfx, matthew.auld, linux-devicetree,
Jianxiong Gao, Daniel Vetter, Konrad Rzeszutek Wilk,
maarten.lankhorst, airlied, Dan Williams, jani.nikula,
Nicolas Boichat, rodrigo.vivi, Bjorn Helgaas, boris.ostrovsky,
Andy Shevchenko, jgross, chris, nouveau, Greg KH, Randy Dunlap,
Frank Rowand, Tomasz Figa, list@263.net:IOMMU DRIVERS,
Jim Quinlan, linuxppc-dev, bauerman
In-Reply-To: <1f84aa4c-f966-0986-b5a4-eecbf3b454ec@arm.com>
On Fri, Apr 23, 2021 at 9:31 PM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2021-04-22 09:15, Claire Chang wrote:
> > Update is_swiotlb_active to add a struct device argument. This will be
> > useful later to allow for restricted DMA pool.
> >
> > Signed-off-by: Claire Chang <tientzu@chromium.org>
> > ---
> > drivers/gpu/drm/i915/gem/i915_gem_internal.c | 2 +-
> > drivers/gpu/drm/nouveau/nouveau_ttm.c | 2 +-
> > drivers/pci/xen-pcifront.c | 2 +-
> > include/linux/swiotlb.h | 4 ++--
> > kernel/dma/direct.c | 2 +-
> > kernel/dma/swiotlb.c | 4 ++--
> > 6 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_internal.c b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
> > index ce6b664b10aa..7d48c433446b 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_internal.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
> > @@ -42,7 +42,7 @@ static int i915_gem_object_get_pages_internal(struct drm_i915_gem_object *obj)
> >
> > max_order = MAX_ORDER;
> > #ifdef CONFIG_SWIOTLB
> > - if (is_swiotlb_active()) {
> > + if (is_swiotlb_active(NULL)) {
> > unsigned int max_segment;
> >
> > max_segment = swiotlb_max_segment();
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c b/drivers/gpu/drm/nouveau/nouveau_ttm.c
> > index e8b506a6685b..2a2ae6d6cf6d 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_ttm.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c
> > @@ -321,7 +321,7 @@ nouveau_ttm_init(struct nouveau_drm *drm)
> > }
> >
> > #if IS_ENABLED(CONFIG_SWIOTLB) && IS_ENABLED(CONFIG_X86)
> > - need_swiotlb = is_swiotlb_active();
> > + need_swiotlb = is_swiotlb_active(NULL);
> > #endif
> >
> > ret = ttm_device_init(&drm->ttm.bdev, &nouveau_bo_driver, drm->dev->dev,
> > diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
> > index b7a8f3a1921f..6d548ce53ce7 100644
> > --- a/drivers/pci/xen-pcifront.c
> > +++ b/drivers/pci/xen-pcifront.c
> > @@ -693,7 +693,7 @@ static int pcifront_connect_and_init_dma(struct pcifront_device *pdev)
> >
> > spin_unlock(&pcifront_dev_lock);
> >
> > - if (!err && !is_swiotlb_active()) {
> > + if (!err && !is_swiotlb_active(NULL)) {
> > err = pci_xen_swiotlb_init_late();
> > if (err)
> > dev_err(&pdev->xdev->dev, "Could not setup SWIOTLB!\n");
> > diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
> > index 2a6cca07540b..c530c976d18b 100644
> > --- a/include/linux/swiotlb.h
> > +++ b/include/linux/swiotlb.h
> > @@ -123,7 +123,7 @@ static inline bool is_swiotlb_buffer(struct device *dev, phys_addr_t paddr)
> > void __init swiotlb_exit(void);
> > unsigned int swiotlb_max_segment(void);
> > size_t swiotlb_max_mapping_size(struct device *dev);
> > -bool is_swiotlb_active(void);
> > +bool is_swiotlb_active(struct device *dev);
> > void __init swiotlb_adjust_size(unsigned long size);
> > #else
> > #define swiotlb_force SWIOTLB_NO_FORCE
> > @@ -143,7 +143,7 @@ static inline size_t swiotlb_max_mapping_size(struct device *dev)
> > return SIZE_MAX;
> > }
> >
> > -static inline bool is_swiotlb_active(void)
> > +static inline bool is_swiotlb_active(struct device *dev)
> > {
> > return false;
> > }
> > diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> > index 84c9feb5474a..7a88c34d0867 100644
> > --- a/kernel/dma/direct.c
> > +++ b/kernel/dma/direct.c
> > @@ -495,7 +495,7 @@ int dma_direct_supported(struct device *dev, u64 mask)
> > size_t dma_direct_max_mapping_size(struct device *dev)
> > {
> > /* If SWIOTLB is active, use its maximum mapping size */
> > - if (is_swiotlb_active() &&
> > + if (is_swiotlb_active(dev) &&
> > (dma_addressing_limited(dev) || swiotlb_force == SWIOTLB_FORCE))
>
> I wonder if it's worth trying to fold these other conditions into
> is_swiotlb_active() itself? I'm not entirely sure what matters for Xen,
> but for the other cases it seems like they probably only care about
> whether bouncing may occur for their particular device or not (possibly
> they want to be using dma_max_mapping_size() now anyway - TBH I'm
> struggling to make sense of what the swiotlb_max_segment business is
> supposed to mean).
I think leaving those conditions outside of is_swiotlb_active() might
help avoid confusion with is_dev_swiotlb_force() in patch #9? We need
is_dev_swiotlb_force() only because the restricted DMA pool supports
memory allocation, but the default swiotlb doesn't.
>
> Otherwise, patch #9 will need to touch here as well to make sure that
> per-device forced bouncing is reflected correctly.
You're right. Otherwise, is_dev_swiotlb_force is needed here.
>
> Robin.
>
> > return swiotlb_max_mapping_size(dev);
> > return SIZE_MAX;
> > diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> > index ffbb8724e06c..1d221343f1c8 100644
> > --- a/kernel/dma/swiotlb.c
> > +++ b/kernel/dma/swiotlb.c
> > @@ -659,9 +659,9 @@ size_t swiotlb_max_mapping_size(struct device *dev)
> > return ((size_t)IO_TLB_SIZE) * IO_TLB_SEGSIZE;
> > }
> >
> > -bool is_swiotlb_active(void)
> > +bool is_swiotlb_active(struct device *dev)
> > {
> > - return io_tlb_default_mem != NULL;
> > + return get_io_tlb_mem(dev) != NULL;
> > }
> > EXPORT_SYMBOL_GPL(is_swiotlb_active);
> >
> >
^ permalink raw reply
* Re: linux-next: boot failure in today's linux-next
From: Jens Axboe @ 2021-04-26 12:46 UTC (permalink / raw)
To: Stephen Rothwell
Cc: Linux Kernel Mailing List, Linus, Linux Next Mailing List,
Changheun Lee, Andrew Morton, PowerPC
In-Reply-To: <20210426174310.6f03345b@canb.auug.org.au>
On 4/26/21 1:43 AM, Stephen Rothwell wrote:
> Hi all,
>
> On Mon, 26 Apr 2021 16:36:06 +1000 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>>
>> Today's linux-next build (ipowerpc_pseries_le_defconfig)
>> failed its qemu boot tests like this:
>>
>> [ 1.833361][ T1] ibmvscsi 71000003: SRP_VERSION: 16.a
>> [ 1.834439][ T1] ibmvscsi 71000003: Maximum ID: 64 Maximum LUN: 32 Maximum Channel: 3
>> [ 1.834683][ T1] scsi host0: IBM POWER Virtual SCSI Adapter 1.5.9
>> [ 1.842605][ C0] ibmvscsi 71000003: partner initialization complete
>> [ 1.844979][ C0] ibmvscsi 71000003: host srp version: 16.a, host partition qemu (0), OS 2, max io 2097152
>> [ 1.845502][ C0] ibmvscsi 71000003: sent SRP login
>> [ 1.845853][ C0] ibmvscsi 71000003: SRP_LOGIN succeeded
>> [ 1.851447][ T1] BUG: Kernel NULL pointer dereference on write at 0x00000390
>> [ 1.851577][ T1] Faulting instruction address: 0xc00000000070386c
>> [ 1.852171][ T1] Oops: Kernel access of bad area, sig: 11 [#1]
>> [ 1.852324][ T1] LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
>> [ 1.852689][ T1] Modules linked in:
>> [ 1.853136][ T1] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.12.0 #2
>> [ 1.853555][ T1] NIP: c00000000070386c LR: c000000000703a6c CTR: 0000000000000000
>> [ 1.853679][ T1] REGS: c0000000063a2f40 TRAP: 0380 Not tainted (5.12.0)
>> [ 1.853870][ T1] MSR: 8000000002009033 <SF,VEC,EE,ME,IR,DR,RI,LE> CR: 44002240 XER: 00000000
>> [ 1.854305][ T1] CFAR: c000000000703a68 IRQMASK: 0
>> [ 1.854305][ T1] GPR00: c000000000703a6c c0000000063a31e0 c00000000146b200 c0000000080ca800
>> [ 1.854305][ T1] GPR04: c000000006067380 c00c000000020180 0000000000000024 0000000000008500
>> [ 1.854305][ T1] GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> [ 1.854305][ T1] GPR12: 0000000000002000 c000000001640000 c000000008068508 0000000000000020
>> [ 1.854305][ T1] GPR16: 0000000000000000 0000000000000024 c000000000f85f78 c000000000f0d998
>> [ 1.854305][ T1] GPR20: c0000000013b59e0 0000000000000003 c0000000063a340c 0000000000000001
>> [ 1.854305][ T1] GPR24: 0000000000000000 c0000000084a3000 c0000000080ca800 c00c000000020180
>> [ 1.854305][ T1] GPR28: 0000000000008500 c0000000080ca800 0000000000000024 c000000006067380
>> [ 1.855486][ T1] NIP [c00000000070386c] bio_add_hw_page+0x7c/0x240
>> [ 1.856357][ T1] LR [c000000000703a6c] bio_add_pc_page+0x3c/0x70
>> [ 1.856723][ T1] Call Trace:
>> [ 1.856890][ T1] [c0000000063a31e0] [0000000000000c00] 0xc00 (unreliable)
>> [ 1.857390][ T1] [c0000000063a3230] [c00000000070105c] bio_kmalloc+0x3c/0xd0
>> [ 1.857514][ T1] [c0000000063a3260] [c000000000713014] blk_rq_map_kern+0x164/0x4a0
>> [ 1.857630][ T1] [c0000000063a32d0] [c0000000008e17dc] __scsi_execute+0x1cc/0x270
>> [ 1.857746][ T1] [c0000000063a3350] [c0000000008e7bf0] scsi_probe_and_add_lun+0x250/0xd90
>> [ 1.857887][ T1] [c0000000063a34c0] [c0000000008e921c] __scsi_scan_target+0x17c/0x630
>> [ 1.858007][ T1] [c0000000063a35d0] [c0000000008e9900] scsi_scan_channel+0x90/0xe0
>> [ 1.858133][ T1] [c0000000063a3620] [c0000000008e9ba8] scsi_scan_host_selected+0x138/0x1a0
>> [ 1.858258][ T1] [c0000000063a3670] [c0000000008e9fec] scsi_scan_host+0x2dc/0x320
>> [ 1.858367][ T1] [c0000000063a3710] [c00000000091b2a0] ibmvscsi_probe+0xa70/0xa80
>> [ 1.858487][ T1] [c0000000063a3800] [c0000000000eb8ac] vio_bus_probe+0x9c/0x460
>> [ 1.858616][ T1] [c0000000063a38a0] [c0000000008979bc] really_probe+0x12c/0x6b0
>> [ 1.858749][ T1] [c0000000063a3950] [c000000000897fd4] driver_probe_device+0x94/0x130
>> [ 1.858874][ T1] [c0000000063a3980] [c00000000089896c] device_driver_attach+0x11c/0x130
>> [ 1.858999][ T1] [c0000000063a39c0] [c000000000898a38] __driver_attach+0xb8/0x1a0
>> [ 1.859123][ T1] [c0000000063a3a10] [c0000000008941a8] bus_for_each_dev+0xa8/0x130
>> [ 1.859257][ T1] [c0000000063a3a70] [c000000000896ef4] driver_attach+0x34/0x50
>> [ 1.859381][ T1] [c0000000063a3a90] [c000000000896510] bus_add_driver+0x170/0x2b0
>> [ 1.859503][ T1] [c0000000063a3b20] [c000000000899b04] driver_register+0xb4/0x1c0
>> [ 1.859626][ T1] [c0000000063a3b90] [c0000000000ea808] __vio_register_driver+0x68/0x90
>> [ 1.859754][ T1] [c0000000063a3bb0] [c0000000010cee74] ibmvscsi_module_init+0xa4/0xdc
>> [ 1.859931][ T1] [c0000000063a3bf0] [c000000000012190] do_one_initcall+0x60/0x2c0
>> [ 1.860071][ T1] [c0000000063a3cc0] [c0000000010846e4] kernel_init_freeable+0x300/0x3a0
>> [ 1.860207][ T1] [c0000000063a3da0] [c000000000012764] kernel_init+0x2c/0x168
>> [ 1.860336][ T1] [c0000000063a3e10] [c00000000000d5ec] ret_from_kernel_thread+0x5c/0x70
>> [ 1.860690][ T1] Instruction dump:
>> [ 1.861072][ T1] fba10038 7cbb2b78 7c7d1b78 7cfc3b78 a1440048 2c2a0000 4082008c a13f004a
>> [ 1.861328][ T1] 7c095040 40810110 e93f0008 811f0028 <e9290390> e9290050 812903d8 7d3e4850
>> [ 1.863000][ T1] ---[ end trace c49ca2d91ee47d7f ]---
>> [ 1.879456][ T1]
>> [ 2.880941][ T1] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
>>
>> I don't know what caused this, but it is some change since Friday.
>>
>> I have left it like this.
>
> Bisections leads to commit
>
> 42fb54fbc707 ("bio: limit bio max size")
>
> from the block tree. Reverting that commit on top of today's
> linux-next allows to the boot to work again.
The patch has been dropped, thanks Stephen.
--
Jens Axboe
^ 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