On Wed, Jul 22, 2020 at 11:35:06AM +0530, Bharata B Rao wrote:
> On Tue, Jul 21, 2020 at 10:25:58PM +1000, Michael Ellerman wrote:
> > Bharata B Rao <bhar...@linux.ibm.com> writes:
> > > On Tue, Jul 21, 2020 at 11:45:20AM +1000, Michael Ellerman wrote:
> > >> Nathan Lynch <nath...@linux.ibm.com> writes:
> > >> > "Aneesh Kumar K.V" <aneesh.ku...@linux.ibm.com> writes:
> > >> >> This is the next version of the fixes for memory unplug on radix.
> > >> >> The issues and the fix are described in the actual patches.
> > >> >
> > >> > I guess this isn't actually causing problems at runtime right now, but
> > >> > I
> > >> > notice calls to resize_hpt_for_hotplug() from arch_add_memory() and
> > >> > arch_remove_memory(), which ought to be mmu-agnostic:
> > >> >
> > >> > int __ref arch_add_memory(int nid, u64 start, u64 size,
> > >> > struct mhp_params *params)
> > >> > {
> > >> > unsigned long start_pfn = start >> PAGE_SHIFT;
> > >> > unsigned long nr_pages = size >> PAGE_SHIFT;
> > >> > int rc;
> > >> >
> > >> > resize_hpt_for_hotplug(memblock_phys_mem_size());
> > >> >
> > >> > start = (unsigned long)__va(start);
> > >> > rc = create_section_mapping(start, start + size, nid,
> > >> > params->pgprot);
> > >> > ...
> > >>
> > >> Hmm well spotted.
> > >>
> > >> That does return early if the ops are not setup:
> > >>
> > >> int resize_hpt_for_hotplug(unsigned long new_mem_size)
> > >> {
> > >> unsigned target_hpt_shift;
> > >>
> > >> if (!mmu_hash_ops.resize_hpt)
> > >> return 0;
> > >>
> > >>
> > >> And:
> > >>
> > >> void __init hpte_init_pseries(void)
> > >> {
> > >> ...
> > >> if (firmware_has_feature(FW_FEATURE_HPT_RESIZE))
> > >> mmu_hash_ops.resize_hpt = pseries_lpar_resize_hpt;
> > >>
> > >> And that comes in via ibm,hypertas-functions:
> > >>
> > >> {FW_FEATURE_HPT_RESIZE, "hcall-hpt-resize"},
> > >>
> > >>
> > >> But firmware is not necessarily going to add/remove that call based on
> > >> whether we're using hash/radix.
> > >
> > > Correct but hpte_init_pseries() will not be called for radix guests.
> >
> > Yeah, duh. You'd think the function name would have been a sufficient
> > clue for me :)
> >
> > >> So I think a follow-up patch is needed to make this more robust.
> > >>
> > >> Aneesh/Bharata what platform did you test this series on? I'm curious
> > >> how this didn't break.
> > >
> > > I have tested memory hotplug/unplug for radix guest on zz platform and
> > > sanity-tested this for hash guest on P8.
> > >
> > > As noted above, mmu_hash_ops.resize_hpt will not be set for radix
> > > guest and hence we won't see any breakage.
> >
> > OK.
> >
> > That's probably fine as it is then. Or maybe just a comment in
> > resize_hpt_for_hotplug() pointing out that resize_hpt will be NULL if
> > we're using radix.
>
> Or we could move these calls to hpt-only routines like below?
>
> David - Do you remember if there was any particular reason to have
> these two hpt-resize calls within powerpc-generic memory hotplug code?
I don't remember, sorry.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
