Re: [Qemu-devel] [PATCH 3/6] pc/vl: Add units-per-default-bus property

[Markus Armbruster]

John Snow <address@hidden> writes:

> This patch adds the 'units_per_default_bus' property
> which allows individual boards to declare their desired
> index => (bus,unit) mapping for their default HBA, so
> that boards such as Q35 can specify that its default
> if_ide HBA, AHCI, only accepts one unit per bus.
>
> This property only overrides the mapping for drives
> matching the block_default_type interface.
>
> Signed-off-by: John Snow <address@hidden>

A bit more detail would be nice here.  Suggest to give a -drive example,
and discuss its meaning before and after the patch with Q35: bogus unit#
before, sane one after.

> ---
>  hw/i386/pc.c        | 1 +
>  hw/i386/pc_q35.c    | 3 ++-
>  include/hw/boards.h | 2 ++
>  vl.c                | 8 ++++++++
>  4 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 2c2e9dc..ab578bf 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1524,6 +1524,7 @@ static void pc_generic_machine_class_init(ObjectClass 
> *oc, void *data)
>      mc->hot_add_cpu = qm->hot_add_cpu;
>      mc->kvm_type = qm->kvm_type;
>      mc->block_default_type = qm->block_default_type;
> +    mc->units_per_default_bus = qm->units_per_default_bus;
>      mc->max_cpus = qm->max_cpus;
>      mc->no_serial = qm->no_serial;
>      mc->no_parallel = qm->no_parallel;
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index d4a907c..b28ddbb 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -344,7 +344,8 @@ static void pc_q35_init_1_4(MachineState *machine)
>  #define PC_Q35_MACHINE_OPTIONS \
>      PC_DEFAULT_MACHINE_OPTIONS, \
>      .desc = "Standard PC (Q35 + ICH9, 2009)", \
> -    .hot_add_cpu = pc_hot_add_cpu
> +    .hot_add_cpu = pc_hot_add_cpu, \
> +    .units_per_default_bus = 1

Changes the (bus, unit) mapping for all Q35 machine types.

Overriding the overide by adding .units_per_default_bus = 0 to
PC_Q35_2_1_MACHINE_OPTIONS would limit the change to new machine types,
if we want that for bug compatibility.  I'm not sure it would be useful,
I'm just spelling it out for the benefit of other reviewers.

Any incompatible change to old machine types must feature *prominently*
in the commit message, even when it changes things from "not useful" to
"actually sane".

>  #define PC_Q35_2_2_MACHINE_OPTIONS                      \
>      PC_Q35_MACHINE_OPTIONS,                             \
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index dfb6718..663f16a 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -28,6 +28,7 @@ struct QEMUMachine {
>      QEMUMachineHotAddCPUFunc *hot_add_cpu;
>      QEMUMachineGetKvmtypeFunc *kvm_type;
>      BlockInterfaceType block_default_type;
> +    int units_per_default_bus;
>      int max_cpus;
>      unsigned int no_serial:1,
>          no_parallel:1,
> @@ -86,6 +87,7 @@ struct MachineClass {
>      int (*kvm_type)(const char *arg);
>  
>      BlockInterfaceType block_default_type;
> +    int units_per_default_bus;
>      int max_cpus;
>      unsigned int no_serial:1,
>          no_parallel:1,
> diff --git a/vl.c b/vl.c
> index eaef240..6fffa1f 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1588,6 +1588,7 @@ static void machine_class_init(ObjectClass *oc, void 
> *data)
>      mc->hot_add_cpu = qm->hot_add_cpu;
>      mc->kvm_type = qm->kvm_type;
>      mc->block_default_type = qm->block_default_type;
> +    mc->units_per_default_bus = qm->units_per_default_bus;
>      mc->max_cpus = qm->max_cpus;
>      mc->no_serial = qm->no_serial;
>      mc->no_parallel = qm->no_parallel;
> @@ -4377,6 +4378,13 @@ int main(int argc, char **argv, char **envp)
>      blk_mig_init();
>      ram_mig_init();
>  
> +    /* If the currently selected machine wishes to override the units-per-bus
> +     * property of its default HBA interface type, do so now. */
> +    if (machine_class->units_per_default_bus) {
> +        override_max_devs(machine_class->block_default_type,
> +                          machine_class->units_per_default_bus);
> +    }
> +
>      /* open the virtual block devices */
>      if (snapshot)
>          qemu_opts_foreach(qemu_find_opts("drive"), drive_enable_snapshot, 
> NULL, 0);

Looks good.