Re: [PATCH v2 10/14] s390x/s390-virtio-ccw: prepare for memory devices

[Thomas Huth <thuth@redhat.com>]

On 13/12/2024 15.21, David Hildenbrand wrote:
> On 13.12.24 13:40, Thomas Huth wrote:
>> On 08/10/2024 12.54, David Hildenbrand wrote:
>>> Let's prepare our address space for memory devices if enabled via
>>> "maxmem" and if we have CONFIG_MEM_DEVICE enabled at all. Note that
>>> CONFIG_MEM_DEVICE will be selected automatically once we add support
>>> for devices.
>>>
>>> Just like on other architectures, the region container for memory devices
>>> is placed directly above our initial memory. For now, we only align the
>>> start address of the region up to 1 GiB, but we won't add any additional
>>> space to the region for internal alignment purposes; this can be done in
>>> the future if really required.
>>>
>>> The RAM size returned via SCLP is not modified, as this only
>>> covers initial RAM (and standby memory we don't implement) and not memory
>>> devices; clarify that in the docs of read_SCP_info(). Existing OSes without
>>> support for memory devices will keep working as is, even when memory
>>> devices would be attached the VM.
>>>
>>> Guest OSs which support memory devices, such as virtio-mem, will
>>> consult diag500(), to find out the maximum possible pfn. Guest OSes that
>>> don't support memory devices, don't have to be changed and will continue
>>> relying on information provided by SCLP.
>>>
>>> There are no remaining maxram_size users in s390x code, and the remaining
>>> ram_size users only care about initial RAM:
>>> * hw/s390x/ipl.c
>>> * hw/s390x/s390-hypercall.c
>>> * hw/s390x/sclp.c
>>> * target/s390x/kvm/pv.c
>>>
>>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>    hw/s390x/s390-virtio-ccw.c | 23 ++++++++++++++++++++++-
>>>    hw/s390x/sclp.c            |  6 +++++-
>>>    2 files changed, 27 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>> index 749d46e700..2031c4cf29 100644
>>> --- a/hw/s390x/s390-virtio-ccw.c
>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>> @@ -156,6 +156,7 @@ static void s390_memory_init(MachineState *machine)
>>>        MemoryRegion *sysmem = get_system_memory();
>>>        MemoryRegion *ram = machine->ram;
>>>        uint64_t ram_size = memory_region_size(ram);
>>> +    uint64_t devmem_base, devmem_size;
>>>        if (!QEMU_IS_ALIGNED(ram_size, 1 * MiB)) {
>>>            /*
>>> @@ -168,11 +169,31 @@ static void s390_memory_init(MachineState *machine)
>>>            exit(EXIT_FAILURE);
>>>        }
>>> -    s390_set_memory_limit(s390ms, ram_size);
>>> +    devmem_size = 0;
>>> +    devmem_base = ram_size;
>>> +#ifdef CONFIG_MEM_DEVICE
>>> +    if (machine->ram_size < machine->maxram_size) {
>>> +
>>> +        /*
>>> +         * Make sure memory devices have a sane default alignment, even
>>> +         * when weird initial memory sizes are specified.
>>> +         */
>>> +        devmem_base = QEMU_ALIGN_UP(devmem_base, 1 * GiB);
>>> +        devmem_size = machine->maxram_size - machine->ram_size;
>>
>> Shouldn't that rather be:
>>
>>              devmem_size = machine->maxram_size - devmem_base;
>>
>> instead?
> 
> For example, having ram_size == 1.5 GiB and maxram_size == 3 GiB means: we 
> want to be able to hotplug 1.5 GiB, which is how we are going to size the 
> region for memory devices.

Ah, ok, sorry, I think my mind somehow mixed up maxram_size with the highest 
possible address in memory ... time for the weekend ;-)

Reviewed-by: Thomas Huth <thuth@redhat.com>