| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <60066a80-4758-4034-bbd8-25d9c24861b9@redhat.com>
Date: Wed, 8 Oct 2025 10:05:37 +0200
From: David Hildenbrand <david@...hat.com>
To: Sumanth Korikkar <sumanthk@...ux.ibm.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>, linux-mm <linux-mm@...ck.org>,
LKML <linux-kernel@...r.kernel.org>, linux-s390
<linux-s390@...r.kernel.org>, Gerald Schaefer
<gerald.schaefer@...ux.ibm.com>, Heiko Carstens <hca@...ux.ibm.com>,
Vasily Gorbik <gor@...ux.ibm.com>, Alexander Gordeev <agordeev@...ux.ibm.com>
Subject: Re: [PATCH 2/4] s390/sclp: Add support for dynamic (de)configuration
of memory
>> Is there any reson this notifier is still needed? I'd assume we can just allow
>> for offlining + re-onlining as we please now.
>>
>> In fact, I'd assume we can get rid of the notifier entirely now?
>
> I was initially uncertain about contains_standby_increment() use case
> and didnt change it here. However, after testing by removing the
> contains_standby_increment() checks, I observed that the common memory
> hotplug code already prevents offlining a memory block that contains
> holes. This ensures safety without relying on these checks.
>
> c5e79ef561b0 ("mm/memory_hotplug.c: don't allow to online/offline memory blocks with holes")
Rings a bell :)
>
> i.e. #cp define storage 3504M standby 2148M
> This leads to a configuration where memory block 27 contains both
> assigned and standby incr.
>
> But, offlining it will not succeed:
> chmem -d 0x00000000d8000000-0x00000000dfffffff
> chmem: Memory Block 27 (0x00000000d8000000-0x00000000dfffffff) disable
> failed: Invalid argument
>
> Hence, I will remove it. Thanks.
Cool!
>
>>> - case MEM_PREPARE_ONLINE:
>>> - /*
>>> - * Access the altmap_start_pfn and altmap_nr_pages fields
>>> - * within the struct memory_notify specifically when dealing
>>> - * with only MEM_PREPARE_ONLINE/MEM_FINISH_OFFLINE notifiers.
>>> - *
>>> - * When altmap is in use, take the specified memory range
>>> - * online, which includes the altmap.
>>> - */
>>> - if (arg->altmap_nr_pages) {
>>> - start = PFN_PHYS(arg->altmap_start_pfn);
>>> - size += PFN_PHYS(arg->altmap_nr_pages);
>>> - }
>>> - rc = sclp_mem_change_state(start, size, 1);
>>> - if (rc || !arg->altmap_nr_pages)
>>> - break;
>>> - /*
>>> - * Set CMMA state to nodat here, since the struct page memory
>>> - * at the beginning of the memory block will not go through the
>>> - * buddy allocator later.
>>> - */
>>> - __arch_set_page_nodat((void *)__va(start), arg->altmap_nr_pages);
>>> + default:
>>> break;
>>> - case MEM_FINISH_OFFLINE:
>>> + }
>>> + return rc ? NOTIFY_BAD : NOTIFY_OK;
>>> +}
>>> +
>>> +static ssize_t config_mblock_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
>>> +{
>>> + struct mblock *mblock = container_of(kobj, struct mblock, kobj);
>>> +
>>> + return sysfs_emit(buf, "%u\n", READ_ONCE(mblock->config));
>>> +}
>>> +
>>> +static ssize_t config_mblock_store(struct kobject *kobj, struct kobj_attribute *attr,
>>> + const char *buf, size_t count)
>>> +{
>>> + unsigned long long addr, block_size;
>>
>> "unsigned long" should be sufficient I'm sure :)
>
> Left over. I will do so.
>
>>> + struct memory_block *mem;
>>> + struct mblock *mblock;
>>> + unsigned char id;
>>> + bool value;
>>> + int rc;
>>> +
>>> + rc = kstrtobool(buf, &value);
>>> + if (rc)
>>> + return rc;
>>> + mblock = container_of(kobj, struct mblock, kobj);
>>> + block_size = memory_block_size_bytes();
>>> + addr = mblock->id * block_size;
>>> + /*
>>> + * Hold device_hotplug_lock when adding/removing memory blocks.
>>> + * Additionally, also protect calls to find_memory_block() and
>>> + * sclp_attach_storage().
>>> + */
>>> + rc = lock_device_hotplug_sysfs();
>>> + if (rc)
>>> + goto out;
>>> + for_each_clear_bit(id, sclp_storage_ids, sclp_max_storage_id + 1)
>>> + sclp_attach_storage(id);
>>> + if (value) {
>>> + if (mblock->config)
>>> + goto out_unlock;
>>> + rc = sclp_mem_change_state(addr, block_size, 1);
>>> + if (rc)
>>> + goto out_unlock;
>>> /*
>>> - * When altmap is in use, take the specified memory range
>>> - * offline, which includes the altmap.
>>> + * Set entire memory block CMMA state to nodat. Later, when
>>> + * page tables pages are allocated via __add_memory(), those
>>> + * regions are marked __arch_set_page_dat().
>>> */
>>> - if (arg->altmap_nr_pages) {
>>> - start = PFN_PHYS(arg->altmap_start_pfn);
>>> - size += PFN_PHYS(arg->altmap_nr_pages);
>>> + __arch_set_page_nodat((void *)__va(addr), block_size >> PAGE_SHIFT);
>>> + rc = __add_memory(0, addr, block_size,
>>> + mblock->memmap_on_memory ?
>>> + MHP_MEMMAP_ON_MEMORY | MHP_OFFLINE_INACCESSIBLE : MHP_NONE);
>>> + if (rc)
>>> + goto out_unlock;
>>
>> Do we have to undo the state change?
>
> Intention was to keep error handling simple. In case of failure in
> add_memory(), we would have state set to 1 (not given back). But,
> subsequent configuration request for that block will not have an impact.
I mean, if we can cleanup easily here by doing another
sclp_mem_change_state(), I think we should just do that.
I'd assume that sclp_mem_change_state() to 0 will usually not fail (I
might be wrong :) ).
[...]
>>> -static int __init sclp_detect_standby_memory(void)
>>> +static int __init sclp_setup_memory(void)
>>> {
>>> struct read_storage_sccb *sccb;
>>> int i, id, assigned, rc;
>>> + struct mblock *mblocks;
>>> + struct kset *kset;
>>> /* No standby memory in kdump mode */
>>> if (oldmem_data.start)
>>
>> Wouldn't we still want to create the ones for initial memory at least?
>
> Intention was the following:
> configuration and deconfiguration of memory with optional
> memmap-on-memory is mostly needed for only standby memory.
>
> If standby memory is absent or sclp is unavailable, we continue using
> the previous behavior (only software offline/online), since the sclp
> memory notifier was not registered in that case before either.
I mean, probably nobody in the kdump kernel cares about it either way,
agreed.
--
Cheers
David / dhildenb
Powered by blists - more mailing lists