On 19.06.2018 17:59, Igor Mammedov wrote:
> On Mon, 18 Jun 2018 16:47:58 +0200
> David Hildenbrand <da...@redhat.com> wrote:
>
>> We want to handle memory device address assignment without passing
>> compatibility parameters ("*align").
>>
>> As x86 and Power use different strategies to determine an alignment and
>> we need clean support for compat handling, let's introduce an enum on
>> the machine class level. This is the machine configuration on how to
>> align memory devices in guest physical memory.
>>
>> The three introduced types represent what is being done on x86 and Power
>> right now.
>
> commit message doesn't deliver purpose of the path,
"We want to handle memory device address assignment without passing
compatibility parameters ("*align")."
So in order to do patch nr 4 without this, I would basically have to
move the align parameter to pc_dimm_pre_plug, along with the code for
"detecting" the alignment in e.g. pc_memory_plug. And I want to avoid
this because ...
> So I'm no conviced it's necessary.
> we probably discussed it in previous revisions but could you reiterate
> it here WHY do you need this and 3/4
>
.. I want to get rid of the align parameter in the long run. Alignment
is some memory device specific property that can be easily detected
using a detection configuration (this patch). This approach looks much
cleaner to me. This way we can use the same alignment strategy for all
memory devices.
In follow up series I want to factor out address assignment completely
into memory_device_pre_plug(). And I also don't want to have an align
parameter at that function. I want to avoid moving the same code around
two times (pc.c).
>
>
>> Signed-off-by: David Hildenbrand <da...@redhat.com>
>> ---
>> hw/core/machine.c | 3 +++
>> hw/i386/pc.c | 13 +++++++------
>> hw/i386/pc_piix.c | 2 +-
>> include/hw/boards.h | 13 +++++++++++++
>> include/hw/i386/pc.h | 3 ---
>> 5 files changed, 24 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 617e5f8d75..d34b205125 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -531,6 +531,9 @@ static void machine_class_init(ObjectClass *oc, void
>> *data)
>> mc->numa_mem_align_shift = 23;
>> mc->numa_auto_assign_ram = numa_default_auto_assign_ram;
>>
>> + /* Default: use memory region alignment as memory devices alignment */
>> + mc->memory_device_align = MEMORY_DEVICE_ALIGN_REGION;
>> +
>> object_class_property_add_str(oc, "accel",
>> machine_get_accel, machine_set_accel, &error_abort);
>> object_class_property_set_description(oc, "accel",
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index bf986baf91..04a97e89e7 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1331,6 +1331,7 @@ void pc_memory_init(PCMachineState *pcms,
>> FWCfgState *fw_cfg;
>> MachineState *machine = MACHINE(pcms);
>> PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>> + MachineClass *mc = MACHINE_GET_CLASS(machine);
>>
>> assert(machine->ram_size == pcms->below_4g_mem_size +
>> pcms->above_4g_mem_size);
>> @@ -1363,8 +1364,6 @@ void pc_memory_init(PCMachineState *pcms,
>> if (!pcmc->has_reserved_memory &&
>> (machine->ram_slots ||
>> (machine->maxram_size > machine->ram_size))) {
>> - MachineClass *mc = MACHINE_GET_CLASS(machine);
>> -
>> error_report("\"-memory 'slots|maxmem'\" is not supported by: %s",
>> mc->name);
>> exit(EXIT_FAILURE);
>> @@ -1394,7 +1393,7 @@ void pc_memory_init(PCMachineState *pcms,
>> machine->device_memory->base =
>> ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size, 1ULL << 30);
>>
>> - if (pcmc->enforce_aligned_dimm) {
>> + if (mc->memory_device_align != MEMORY_DEVICE_ALIGN_PAGE) {
>> /* size device region assuming 1G page max alignment per slot */
>> device_mem_size += (1ULL << 30) * machine->ram_slots;
>> }
>> @@ -1705,14 +1704,16 @@ static void pc_memory_plug(HotplugHandler
>> *hotplug_dev,
>> HotplugHandlerClass *hhc;
>> Error *local_err = NULL;
>> PCMachineState *pcms = PC_MACHINE(hotplug_dev);
>> - PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>> + MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev);
>> PCDIMMDevice *dimm = PC_DIMM(dev);
>> PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>> MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
>> uint64_t align = TARGET_PAGE_SIZE;
>> bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>>
>> - if (memory_region_get_alignment(mr) && pcmc->enforce_aligned_dimm) {
>> +
>> + if (memory_region_get_alignment(mr) &&
>> + mc->memory_device_align != MEMORY_DEVICE_ALIGN_PAGE) {
>> align = memory_region_get_alignment(mr);
>> }
>>
>> @@ -2374,7 +2375,6 @@ static void pc_machine_class_init(ObjectClass *oc,
>> void *data)
>> pcmc->gigabyte_align = true;
>> pcmc->has_reserved_memory = true;
>> pcmc->kvmclock_enabled = true;
>> - pcmc->enforce_aligned_dimm = true;
>> /* BIOS ACPI tables: 128K. Other BIOS datastructures: less than 4K
>> reported
>> * to be used at the moment, 32K should be enough for a while. */
>> pcmc->acpi_data_size = 0x20000 + 0x8000;
>> @@ -2398,6 +2398,7 @@ static void pc_machine_class_init(ObjectClass *oc,
>> void *data)
>> hc->unplug = pc_machine_device_unplug_cb;
>> nc->nmi_monitor_handler = x86_nmi;
>> mc->default_cpu_type = TARGET_DEFAULT_CPU_TYPE;
>> + mc->memory_device_align = MEMORY_DEVICE_ALIGN_REGION_OR_PAGE;
>>
>> object_class_property_add(oc, PC_MACHINE_DEVMEM_REGION_SIZE, "int",
>> pc_machine_get_device_memory_region_size, NULL,
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index 3b87f3cedb..cc11856c24 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -566,7 +566,7 @@ static void pc_i440fx_2_1_machine_options(MachineClass
>> *m)
>> m->default_display = NULL;
>> SET_MACHINE_COMPAT(m, PC_COMPAT_2_1);
>> pcmc->smbios_uuid_encoded = false;
>> - pcmc->enforce_aligned_dimm = false;
>> + m->memory_device_align = MEMORY_DEVICE_ALIGN_PAGE;
>> }
>>
>> DEFINE_I440FX_MACHINE(v2_1, "pc-i440fx-2.1", pc_compat_2_1,
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index ef7457f5dd..3f151207c1 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -105,6 +105,15 @@ typedef struct {
>> CPUArchId cpus[0];
>> } CPUArchIdList;
>>
>> +typedef enum MemoryDeviceAlign {
>> + /* use specified memory region alignment */
>> + MEMORY_DEVICE_ALIGN_REGION = 0,
>> + /* use target page size as alignment */
>> + MEMORY_DEVICE_ALIGN_PAGE,
>> + /* use target page size if no memory region alignment has been
>> specified */
>> + MEMORY_DEVICE_ALIGN_REGION_OR_PAGE,
>> +} MemoryDeviceAlign;
>> +
>> /**
>> * MachineClass:
>> * @max_cpus: maximum number of CPUs supported. Default: 1
>> @@ -156,6 +165,9 @@ typedef struct {
>> * should instead use "unimplemented-device" for all memory ranges where
>> * the guest will attempt to probe for a device that QEMU doesn't
>> * implement and a stub device is required.
>> + * @memory_device_align: The alignment that will be used as default when
>> + * searching for a guest physical memory address while plugging a
>> + * memory device. This is relevant for compatibility handling.
>> */
>> struct MachineClass {
>> /*< private >*/
>> @@ -202,6 +214,7 @@ struct MachineClass {
>> const char **valid_cpu_types;
>> strList *allowed_dynamic_sysbus_devices;
>> bool auto_enable_numa_with_memhp;
>> + MemoryDeviceAlign memory_device_align;
>> void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes,
>> int nb_nodes, ram_addr_t size);
>>
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index fc8dedca12..ffb4654fc8 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -86,8 +86,6 @@ struct PCMachineState {
>> *
>> * Compat fields:
>> *
>> - * @enforce_aligned_dimm: check that DIMM's address/size is aligned by
>> - * backend's alignment value if provided
>> * @acpi_data_size: Size of the chunk of memory at the top of RAM
>> * for the BIOS ACPI tables and other BIOS
>> * datastructures.
>> @@ -124,7 +122,6 @@ struct PCMachineClass {
>> /* RAM / address space compat: */
>> bool gigabyte_align;
>> bool has_reserved_memory;
>> - bool enforce_aligned_dimm;
>> bool broken_reserved_end;
>>
>> /* TSC rate migration: */
>
--
Thanks,
David / dhildenb
