* [PATCH] powerpc/pseries: fix oops in hotplug memory notifier @ 2019-06-07 5:04 Nathan Lynch 2019-06-07 13:08 ` Michael Ellerman ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Nathan Lynch @ 2019-06-07 5:04 UTC (permalink / raw) To: linuxppc-dev During post-migration device tree updates, we can oops in pseries_update_drconf_memory if the source device tree has an ibm,dynamic-memory-v2 property and the destination has a ibm,dynamic_memory (v1) property. The notifier processes an "update" for the ibm,dynamic-memory property but it's really an add in this scenario. So make sure the old property object is there before dereferencing it. Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com> --- arch/powerpc/platforms/pseries/hotplug-memory.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c index 47087832f8b2..e6bd172bcf30 100644 --- a/arch/powerpc/platforms/pseries/hotplug-memory.c +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c @@ -980,6 +980,9 @@ static int pseries_update_drconf_memory(struct of_reconfig_data *pr) if (!memblock_size) return -EINVAL; + if (!pr->old_prop) + return 0; + p = (__be32 *) pr->old_prop->value; if (!p) return -EINVAL; -- 2.20.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] powerpc/pseries: fix oops in hotplug memory notifier 2019-06-07 5:04 [PATCH] powerpc/pseries: fix oops in hotplug memory notifier Nathan Lynch @ 2019-06-07 13:08 ` Michael Ellerman 2019-06-07 14:59 ` Nathan Lynch 2019-06-07 22:23 ` Tyrel Datwyler 2019-06-15 13:36 ` Michael Ellerman 2 siblings, 1 reply; 8+ messages in thread From: Michael Ellerman @ 2019-06-07 13:08 UTC (permalink / raw) To: Nathan Lynch, linuxppc-dev Nathan Lynch <nathanl@linux.ibm.com> writes: > During post-migration device tree updates, we can oops in > pseries_update_drconf_memory if the source device tree has an > ibm,dynamic-memory-v2 property and the destination has a > ibm,dynamic_memory (v1) property. The notifier processes an "update" > for the ibm,dynamic-memory property but it's really an add in this > scenario. So make sure the old property object is there before > dereferencing it. > > Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com> Can you pinpoint a commit that introduced the bug? Should we backport this to stable? Perhaps? Fixes: 2b31e3aec1db ("powerpc/drmem: Add support for ibm, dynamic-memory-v2 property") cheers > diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c > index 47087832f8b2..e6bd172bcf30 100644 > --- a/arch/powerpc/platforms/pseries/hotplug-memory.c > +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c > @@ -980,6 +980,9 @@ static int pseries_update_drconf_memory(struct of_reconfig_data *pr) > if (!memblock_size) > return -EINVAL; > > + if (!pr->old_prop) > + return 0; > + > p = (__be32 *) pr->old_prop->value; > if (!p) > return -EINVAL; > -- > 2.20.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] powerpc/pseries: fix oops in hotplug memory notifier 2019-06-07 13:08 ` Michael Ellerman @ 2019-06-07 14:59 ` Nathan Lynch 0 siblings, 0 replies; 8+ messages in thread From: Nathan Lynch @ 2019-06-07 14:59 UTC (permalink / raw) To: Michael Ellerman, linuxppc-dev Michael Ellerman <mpe@ellerman.id.au> writes: > Nathan Lynch <nathanl@linux.ibm.com> writes: > >> During post-migration device tree updates, we can oops in >> pseries_update_drconf_memory if the source device tree has an >> ibm,dynamic-memory-v2 property and the destination has a >> ibm,dynamic_memory (v1) property. The notifier processes an "update" >> for the ibm,dynamic-memory property but it's really an add in this >> scenario. So make sure the old property object is there before >> dereferencing it. >> >> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com> > > Can you pinpoint a commit that introduced the bug? Should we backport > this to stable? > > Perhaps? > Fixes: 2b31e3aec1db ("powerpc/drmem: Add support for ibm, dynamic-memory-v2 property") I guess I had considered it a latent bug, but you're right - we wouldn't hit it until the v2 support was added. So that commit is correct. Let me know if I should resend. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] powerpc/pseries: fix oops in hotplug memory notifier 2019-06-07 5:04 [PATCH] powerpc/pseries: fix oops in hotplug memory notifier Nathan Lynch 2019-06-07 13:08 ` Michael Ellerman @ 2019-06-07 22:23 ` Tyrel Datwyler 2019-06-07 23:39 ` Nathan Lynch 2019-06-15 13:36 ` Michael Ellerman 2 siblings, 1 reply; 8+ messages in thread From: Tyrel Datwyler @ 2019-06-07 22:23 UTC (permalink / raw) To: Nathan Lynch, linuxppc-dev On 06/06/2019 10:04 PM, Nathan Lynch wrote: > During post-migration device tree updates, we can oops in > pseries_update_drconf_memory if the source device tree has an > ibm,dynamic-memory-v2 property and the destination has a > ibm,dynamic_memory (v1) property. The notifier processes an "update" > for the ibm,dynamic-memory property but it's really an add in this > scenario. So make sure the old property object is there before > dereferencing it. > > Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com> > --- Ok, so I think there are a couple things to address here in regards to the nice mess PFW has gotten us into. Yes, this patch solves the oops, but I worry it just papers over some short comings in our code. After some poking around unless I'm mistaken our memory notifier only handles v1 versions of the ibm,dynamic-memory property. So, on newer firmware we aren't doing any memory fixups if v2 (ibm,dynamic-memory-v2) of the property is updated. For older PFW if we have source and target that only support v1 we will update the memory in response to any update to ibm,dynamic-memory. It also appears to be the case if we start with v1 and migrate to a target with newer PFW that supports both v1 and v2 that the PFW will continue with v1 on the target and as a result we update memory in accordance to a property update to ibm,dynamic-memory. Now, if we have source and targets that both support v2 after a migration we will do no update in response to ibm,dynamic-memory-v2 changes. And then there is the case of a source with v2 support migrating to a target with only v1 support where we observe this oops. The oops is a result of ibm,dynamic-memory being a new property that is added and there for no old property date exists. However, simply returning in response also has the side effect that we do not update memory in response to a device tree update of dynamic memory. Maybe we are ok with this behavior as I haven't dug deep enough into the memory subsystem here to really understand what the memory code is updating, but it is concerning that we are doing it in some cases, but not all. -Tyrel > arch/powerpc/platforms/pseries/hotplug-memory.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c > index 47087832f8b2..e6bd172bcf30 100644 > --- a/arch/powerpc/platforms/pseries/hotplug-memory.c > +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c > @@ -980,6 +980,9 @@ static int pseries_update_drconf_memory(struct of_reconfig_data *pr) > if (!memblock_size) > return -EINVAL; > > + if (!pr->old_prop) > + return 0; > + > p = (__be32 *) pr->old_prop->value; > if (!p) > return -EINVAL; > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] powerpc/pseries: fix oops in hotplug memory notifier 2019-06-07 22:23 ` Tyrel Datwyler @ 2019-06-07 23:39 ` Nathan Lynch 2019-06-14 1:05 ` Nathan Lynch 0 siblings, 1 reply; 8+ messages in thread From: Nathan Lynch @ 2019-06-07 23:39 UTC (permalink / raw) To: Tyrel Datwyler; +Cc: linuxppc-dev Tyrel Datwyler <tyreld@linux.vnet.ibm.com> writes: > On 06/06/2019 10:04 PM, Nathan Lynch wrote: >> During post-migration device tree updates, we can oops in >> pseries_update_drconf_memory if the source device tree has an >> ibm,dynamic-memory-v2 property and the destination has a >> ibm,dynamic_memory (v1) property. The notifier processes an "update" >> for the ibm,dynamic-memory property but it's really an add in this >> scenario. So make sure the old property object is there before >> dereferencing it. >> >> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com> >> --- > > Yes, this patch solves the oops, but I worry it just papers over some short > comings in our code. > > After some poking around unless I'm mistaken our memory notifier only handles v1 > versions of the ibm,dynamic-memory property. So, on newer firmware we aren't > doing any memory fixups if v2 (ibm,dynamic-memory-v2) of the property > is updated. It's not clear to me what the notifier is meant to accomplish: for (i = 0; i < entries; i++) { if ((be32_to_cpu(old_drmem[i].flags) & DRCONF_MEM_ASSIGNED) && (!(be32_to_cpu(new_drmem[i].flags) & DRCONF_MEM_ASSIGNED))) { rc = pseries_remove_memblock( be64_to_cpu(old_drmem[i].base_addr), memblock_size); break; } else if ((!(be32_to_cpu(old_drmem[i].flags) & DRCONF_MEM_ASSIGNED)) && (be32_to_cpu(new_drmem[i].flags) & DRCONF_MEM_ASSIGNED)) { rc = memblock_add(be64_to_cpu(old_drmem[i].base_addr), memblock_size); rc = (rc < 0) ? -EINVAL : 0; break; } } This compares the 'assigned' flag for each LMB in the old vs new properties and adds or removes the block accordingly. However: - Migration and PRRNs are specified only to change LMBs' NUMA affinity information. This notifier should be a no-op for those scenarios since the assigned flags should not change. - The memory hotplug/DLPAR path has a hack which inhibits the execution of the notifier: dlpar_memory() ... rtas_hp_event = true; drmem_update_dt() of_update_property() pseries_memory_notifier() pseries_update_drconf_memory() if (rtas_hp_event) return; So what's it for? My best guess is that it's a holdover from when more of the DLPAR work was done in user space. I don't see a purpose for it now. > For older PFW if we have source and target that only support v1 we will update > the memory in response to any update to ibm,dynamic-memory. It also appears to > be the case if we start with v1 and migrate to a target with newer PFW that > supports both v1 and v2 that the PFW will continue with v1 on the target and as > a result we update memory in accordance to a property update to ibm,dynamic-memory. > > Now, if we have source and targets that both support v2 after a migration we > will do no update in response to ibm,dynamic-memory-v2 changes. And then there > is the case of a source with v2 support migrating to a target with only v1 > support where we observe this oops. The oops is a result of ibm,dynamic-memory > being a new property that is added and there for no old property date exists. > However, simply returning in response also has the side effect that we do not > update memory in response to a device tree update of dynamic memory. > > Maybe we are ok with this behavior as I haven't dug deep enough into the memory > subsystem here to really understand what the memory code is updating, but it is > concerning that we are doing it in some cases, but not all. I hope I've made a good case above that the notifier does not do any useful work, and a counterpart for the v2 format isn't needed. Do you agree? If so, I'll send a patch later to remove the notifier altogether. In the near term I would still like this minimal fix to go in. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] powerpc/pseries: fix oops in hotplug memory notifier 2019-06-07 23:39 ` Nathan Lynch @ 2019-06-14 1:05 ` Nathan Lynch 2019-06-15 22:22 ` Tyrel Datwyler 0 siblings, 1 reply; 8+ messages in thread From: Nathan Lynch @ 2019-06-14 1:05 UTC (permalink / raw) To: Tyrel Datwyler; +Cc: linuxppc-dev Nathan Lynch <nathanl@linux.ibm.com> writes: > Tyrel Datwyler <tyreld@linux.vnet.ibm.com> writes: > >> Maybe we are ok with this behavior as I haven't dug deep enough into the memory >> subsystem here to really understand what the memory code is updating, but it is >> concerning that we are doing it in some cases, but not all. > > I hope I've made a good case above that the notifier does not do any > useful work, and a counterpart for the v2 format isn't needed. Do you > agree? > > If so, I'll send a patch later to remove the notifier altogether. In the > near term I would still like this minimal fix to go in. Tyrel, ack? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] powerpc/pseries: fix oops in hotplug memory notifier 2019-06-14 1:05 ` Nathan Lynch @ 2019-06-15 22:22 ` Tyrel Datwyler 0 siblings, 0 replies; 8+ messages in thread From: Tyrel Datwyler @ 2019-06-15 22:22 UTC (permalink / raw) To: Nathan Lynch, Tyrel Datwyler; +Cc: linuxppc-dev On 06/13/2019 06:05 PM, Nathan Lynch wrote: > Nathan Lynch <nathanl@linux.ibm.com> writes: > >> Tyrel Datwyler <tyreld@linux.vnet.ibm.com> writes: >> >>> Maybe we are ok with this behavior as I haven't dug deep enough into the memory >>> subsystem here to really understand what the memory code is updating, but it is >>> concerning that we are doing it in some cases, but not all. >> >> I hope I've made a good case above that the notifier does not do any >> useful work, and a counterpart for the v2 format isn't needed. Do you >> agree? >> >> If so, I'll send a patch later to remove the notifier altogether. In the >> near term I would still like this minimal fix to go in. > > Tyrel, ack? > Sure, lets start with simple case to fix the crash. -Tyrel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] powerpc/pseries: fix oops in hotplug memory notifier 2019-06-07 5:04 [PATCH] powerpc/pseries: fix oops in hotplug memory notifier Nathan Lynch 2019-06-07 13:08 ` Michael Ellerman 2019-06-07 22:23 ` Tyrel Datwyler @ 2019-06-15 13:36 ` Michael Ellerman 2 siblings, 0 replies; 8+ messages in thread From: Michael Ellerman @ 2019-06-15 13:36 UTC (permalink / raw) To: Nathan Lynch, linuxppc-dev On Fri, 2019-06-07 at 05:04:07 UTC, Nathan Lynch wrote: > During post-migration device tree updates, we can oops in > pseries_update_drconf_memory if the source device tree has an > ibm,dynamic-memory-v2 property and the destination has a > ibm,dynamic_memory (v1) property. The notifier processes an "update" > for the ibm,dynamic-memory property but it's really an add in this > scenario. So make sure the old property object is there before > dereferencing it. > > Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com> Applied to powerpc next, thanks. https://git.kernel.org/powerpc/c/0aa82c482ab2ece530a6f44897b63b27 cheers ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-06-15 22:24 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-07 5:04 [PATCH] powerpc/pseries: fix oops in hotplug memory notifier Nathan Lynch 2019-06-07 13:08 ` Michael Ellerman 2019-06-07 14:59 ` Nathan Lynch 2019-06-07 22:23 ` Tyrel Datwyler 2019-06-07 23:39 ` Nathan Lynch 2019-06-14 1:05 ` Nathan Lynch 2019-06-15 22:22 ` Tyrel Datwyler 2019-06-15 13:36 ` Michael Ellerman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).