Re: [PATCH v3 3/4] pc & q35: Add new object pc-memory-layout.

[Don Slutz <dslutz@verizon.com>]

On 04/18/14 11:45, Andreas Färber wrote:
> Am 25.03.2014 00:55, schrieb Don Slutz:
>> This new object has the property max-ram-below-4g.
>>
>> If you add enough PCI devices then all mmio for them will not fit
>> below 4G which may not be the layout the user wanted. This allows
>> you to increase the below 4G address space that PCI devices can use
>> (aka decrease ram below 4G) and therefore in more cases not have any
>> mmio that is above 4G.
>>
>> For example adding "-global pc-memory-layout.max-ram-below-4g=2G" to
>> the command line will limit the amount of ram that is below 4G to
>> 2G.
>>
>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>> ---
>>   hw/i386/pc.c         | 42 ++++++++++++++++++++++++++++++++++++++++++
>>   hw/i386/pc_piix.c    | 23 +++++++++++++++++++++--
>>   hw/i386/pc_q35.c     | 23 +++++++++++++++++++++--
>>   include/hw/i386/pc.h |  2 ++
>>   4 files changed, 86 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 14f0d91..45339f3 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1429,3 +1429,45 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
>>           gsi_state->ioapic_irq[i] = qdev_get_gpio_in(dev, i);
>>       }
>>   }
>> +
>> +/* pc-memory-layout stuff */
>> +
>> +typedef struct PcMemoryLayoutState PcMemoryLayoutState;
> PCMemoryLayoutState?

Fine with me.

>> +
>> +/**
>> + * @PcMemoryLayoutState:
> Please drop @.

Will do.

>> + */
>> +struct PcMemoryLayoutState {
>> +    /*< private >*/
>> +    Object parent_obj;
>> +    /*< public >*/
>> +
>> +    uint64_t max_ram_below_4g;
>> +};
>> +
>> +static Property pc_memory_layout_props[] = {
>> +    DEFINE_PROP_SIZE("max-ram-below-4g", PcMemoryLayoutState,
>> +                     max_ram_below_4g, 0),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void pc_memory_layout_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    dc->props = pc_memory_layout_props;
>> +}
>> +
>> +static const TypeInfo pc_memory_layout_info = {
>> +    .name = TYPE_PC_MEMORY_LAYOUT,
>> +    .parent = TYPE_DEVICE,
>> +    .instance_size = sizeof(PcMemoryLayoutState),
>> +    .class_init = pc_memory_layout_class_init,
>> +};
>> +
>> +static void pc_memory_layout_register_types(void)
>> +{
>> +    type_register_static(&pc_memory_layout_info);
>> +}
>> +
>> +type_init(pc_memory_layout_register_types)
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index bd52f6e..81d730d 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -95,6 +95,23 @@ static void pc_init1(QEMUMachineInitArgs *args,
>>       DeviceState *icc_bridge;
>>       FWCfgState *fw_cfg = NULL;
>>       PcGuestInfo *guest_info;
>> +    Object *pc_memory_layout;
>> +    uint64_t max_ram_below_4g;
>> +    ram_addr_t lowmem = 0xe0000000;
>> +
>> +    pc_memory_layout = object_new(TYPE_PC_MEMORY_LAYOUT);
>> +    max_ram_below_4g = object_property_get_int(pc_memory_layout,
>> +                                               "max-ram-below-4g",
>> +                                               NULL);
>> +    if (max_ram_below_4g) {
> Isn't this always zero due to the default value?

Nope. I forget if object_new() or object_property_get_int()
check the global list.  So if the user sets this, the value is
found at this time.

>> +        if (max_ram_below_4g > (1ULL << 32)) {
>> +            fprintf(stderr,
>> +                    "%s: max_ram_below_4g=%lld too big adjusted to %lld\n",
>> +                    __func__, (long long)max_ram_below_4g, 1ULL << 32);
> Is this forgotten debug output? For a user, __func__ is not helpful and
> error_report() would be preferable. Also please just use PRIx64 for
> max_ram_below_4g rather than casting.

Nope.  Will change to use error_report() without __func__ and PRIx64.


>> +            max_ram_below_4g = 1ULL << 32;
>> +        }
>> +        lowmem = max_ram_below_4g;
>> +    }
>>   
>>       /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory).
>>        * If it doesn't, we need to split it in chunks below and above 4G.
>> @@ -103,8 +120,10 @@ static void pc_init1(QEMUMachineInitArgs *args,
>>        * For old machine types, use whatever split we used historically to avoid
>>        * breaking migration.
>>        */
>> -    if (args->ram_size >= 0xe0000000) {
>> -        ram_addr_t lowmem = gigabyte_align ? 0xc0000000 : 0xe0000000;
>> +    if (args->ram_size >= lowmem) {
>> +        if (!max_ram_below_4g && gigabyte_align) {
>> +            lowmem = 0xc0000000;
>> +        }
>>           above_4g_mem_size = args->ram_size - lowmem;
>>           below_4g_mem_size = lowmem;
>>       } else {
> We don't have central, recursive realization of devices yet. That means
> your device is instantiated but never realized, and while you're not
> exposing any global state it allows to tweak the property value
> throughout device lifetime.

Not sure how to mark this as read only.  I have not considered
how this can be changed.  There are a lot of other internals that
are generated off of this like ram_size_below_4g.

>
> Also you're forgetting to add this device to the composition tree via
> object_property_add_child(). Neither ACPI code nor future QOM code can
> discover it that way.

Ok, will add a call on object_property_add_child(). Or is the
fact that this ends up in PcGuestInfoState and passed directly
to acpi_setup() mean that it should not be?



>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index 6e34fe1..529f53d 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -82,6 +82,23 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
>>       PCIDevice *ahci;
>>       DeviceState *icc_bridge;
>>       PcGuestInfo *guest_info;
>> +    Object *pc_memory_layout;
>> +    uint64_t max_ram_below_4g;
>> +    ram_addr_t lowmem = 0xb0000000;
>> +
>> +    pc_memory_layout = object_new(TYPE_PC_MEMORY_LAYOUT);
>> +    max_ram_below_4g = object_property_get_int(pc_memory_layout,
>> +                                               "max-ram-below-4g",
>> +                                               NULL);
>> +    if (max_ram_below_4g) {
>> +        if (max_ram_below_4g > (1ULL << 32)) {
>> +            fprintf(stderr,
>> +                    "%s: max_ram_below_4g=%lld too big adjusted to %lld\n",
>> +                    __func__, (long long)max_ram_below_4g, 1ULL << 32);
>> +            max_ram_below_4g = 1ULL << 32;
>> +        }
>> +        lowmem = max_ram_below_4g;
>> +    }
>>   
>>       /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory
>>        * and 256 Mbytes for PCI Express Enhanced Configuration Access Mapping
>> @@ -92,8 +109,10 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
>>        * For old machine types, use whatever split we used historically to avoid
>>        * breaking migration.
>>        */
>> -    if (args->ram_size >= 0xb0000000) {
>> -        ram_addr_t lowmem = gigabyte_align ? 0x80000000 : 0xb0000000;
>> +    if (args->ram_size >= lowmem) {
>> +        if (!max_ram_below_4g && gigabyte_align) {
>> +            lowmem = 0x80000000;
>> +        }
>>           above_4g_mem_size = args->ram_size - lowmem;
>>           below_4g_mem_size = lowmem;
>>       } else {
> Dito here for all pc_piix.c comments.

Same as above.
     -Don Slutz

>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 9010246..9162d61 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -163,6 +163,8 @@ void cpu_smm_register(cpu_set_smm_t callback, void *arg);
>>   
>>   void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name);
>>   
>> +#define TYPE_PC_MEMORY_LAYOUT "pc-memory-layout"
>> +
>>   /* acpi_piix.c */
>>   
>>   I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
> Regards,
> Andreas
>