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

["Andreas Färber" <afaerber@suse.de>]

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?

> +
> +/**
> + * @PcMemoryLayoutState:

Please drop @.

> + */
> +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?

> +        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.

> +            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.

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.

> 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.

> 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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg