qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 4/6] ide: Update ide_drive_get to be HBA agno


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH v2 4/6] ide: Update ide_drive_get to be HBA agnostic
Date: Tue, 30 Sep 2014 09:38:29 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux)

John Snow <address@hidden> writes:

> Instead of duplicating the logic for the if_ide
> (bus,unit) mappings, rely on the blockdev layer
> for managing those mappings for us, and use the
> drive_get_by_index call instead.
>
> This allows ide_drive_get to work for AHCI HBAs
> as well, and can be used in the Q35 initialization.
>
> Lastly, change the nature of the argument to
> ide_drive_get so that represents the number of
> total drives we can support, and not the total
> number of buses. This will prevent array overflows
> if the units-per-default-bus property ever needs
> to be adjusted for compatibility reasons.
>
> Signed-off-by: John Snow <address@hidden>
> ---
>  blockdev.c                |  9 +++++++++
>  hw/alpha/dp264.c          |  2 +-
>  hw/i386/pc_piix.c         |  2 +-
>  hw/ide/core.c             | 21 +++++++++++++++------
>  hw/mips/mips_fulong2e.c   |  2 +-
>  hw/mips/mips_malta.c      |  2 +-
>  hw/mips/mips_r4k.c        |  2 +-
>  hw/ppc/mac_newworld.c     |  2 +-
>  hw/ppc/mac_oldworld.c     |  2 +-
>  hw/ppc/prep.c             |  2 +-
>  hw/sparc64/sun4u.c        |  2 +-
>  include/sysemu/blockdev.h |  1 +
>  12 files changed, 34 insertions(+), 15 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 9b05f1b..ffaad39 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -135,6 +135,15 @@ void blockdev_auto_del(BlockDriverState *bs)
>      }
>  }
>  
> +int drive_get_max_devs(BlockInterfaceType type)
> +{
> +    if (type >= IF_IDE && type < IF_COUNT) {
> +        return if_max_devs[type];
> +    }
> +
> +    return 0;
> +}
> +

drive_get_max_bus() returns -1 for a type without drives.  Includes
invalid types.  When it returns a non-negative number, a drive on that
bus exists.

Your drive_get_max_devs() has a similar name, but different semantics:
it returns a positive number when the implied HBA supports multiple
buses, else zero.  The "else" includes invalid types.  When it returns a
positive number, then the HBA can take that many units per bus.

No big deal, but functions comments would be nice.

Should invalid type be treated as a programming error instead?

>  static int drive_index_to_bus_id(BlockInterfaceType type, int index)
>  {
>      int max_devs = if_max_devs[type];
> diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
> index b178a03..ab61bb6 100644
> --- a/hw/alpha/dp264.c
> +++ b/hw/alpha/dp264.c
> @@ -97,7 +97,7 @@ static void clipper_init(MachineState *machine)
>      /* IDE disk setup.  */
>      {
>          DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
> -        ide_drive_get(hd, MAX_IDE_BUS);
> +        ide_drive_get(hd, MAX_IDE_BUS * MAX_IDE_DEVS);

More obviously correct would be

        ide_drive_get(hd, ARRAY_SIZE(hd));

Less so here, because the declaration is right next to the use, more so
elsewhere, where it isn't.

>  
>          pci_cmd646_ide_init(pci_bus, hd, 0);
>      }
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 103d756..2760c81 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -239,7 +239,7 @@ static void pc_init1(MachineState *machine,
>  
>      pc_nic_init(isa_bus, pci_bus);
>  
> -    ide_drive_get(hd, MAX_IDE_BUS);
> +    ide_drive_get(hd, MAX_IDE_BUS * MAX_IDE_DEVS);
>      if (pci_enabled) {
>          PCIDevice *dev;
>          if (xen_enabled()) {
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 190700a..e7c1050 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -2558,16 +2558,25 @@ const VMStateDescription vmstate_ide_bus = {
>      }
>  };
>  
> -void ide_drive_get(DriveInfo **hd, int max_bus)
> +void ide_drive_get(DriveInfo **hd, int n)
>  {
>      int i;
> +    int highest_bus = drive_get_max_bus(IF_IDE) + 1;

Actually, this is the "highest bus" + 1 :)

> +    int n_buses = n / drive_get_max_devs(IF_IDE);

What if drive_get_max_devs(IF_IDE) returns 0?

You could side-step the question by using drive_index_to_bus_id(n).

>  
> -    if (drive_get_max_bus(IF_IDE) >= max_bus) {
> -        fprintf(stderr, "qemu: too many IDE bus: %d\n", max_bus);
> -        exit(1);

Before: fatal.

> +    /* Note: The number of actual buses available is not known.
> +     * We compute this based on the size of the DriveInfo* array, n.
> +     * If it is less than (drive_get_max_devs(IF_IDE) * num_real_buses),
> +     * We will stop looking for drives prematurely instead of overfilling
> +     * the array. */
> +

You might want to consider winged comments:

    /*
     * Note: The number of actual buses available is not known.
     * We compute this based on the size of the DriveInfo* array, n.
     * If it is less than (drive_get_max_devs(IF_IDE) * num_real_buses),
     * We will stop looking for drives prematurely instead of overfilling
     * the array.
     */

> +    if (highest_bus > n_buses) {
> +        error_report("Warning: Too many IDE buses defined (%d > %d)",
> +                     highest_bus, n_buses);

After: warning.

Why?

>      }
>  
> -    for(i = 0; i < max_bus * MAX_IDE_DEVS; i++) {
> -        hd[i] = drive_get(IF_IDE, i / MAX_IDE_DEVS, i % MAX_IDE_DEVS);
> +    for (i = 0; i < n; i++) {
> +        hd[i] = drive_get_by_index(IF_IDE, i);
>      }
> +

Stray blank line.

>  }

Function is much nicer now, thanks!

[...]



reply via email to

[Prev in Thread] Current Thread [Next in Thread]