Re: [PATCH] powerpc/pseries: fix oops in hotplug memory notifier

Tyrel Datwyler [off-list ref] writes:

On 06/06/2019 10:04 PM, Nathan Lynch wrote:

quoted

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 <redacted>
---

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.