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

[John Snow <jsnow@redhat.com>]

On 09/30/2014 03:38 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> 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 <jsnow@redhat.com>
>> ---
>>   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?

I'll fix the divide by zero and address the other comments. I can adjust 
the semantics to match the other function -- sometimes I don't realize 
I've accidentally created a function that is similar to, but behaves 
differently from, another.

The reasoning behind a warning instead of a hard error was that if we 
provide less slots in the HD table than is necessary for covering the 
full number of buses/units in the board, we're going to stop early. It's 
not necessarily a fatal error, so I replaced the hard exit() with a warning.

If we do want a hard exit, I should probably start baking in the error 
pathways down this far to do it appropriately instead of terminating 
execution.

>>       }
>>
>> -    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!
>
> [...]
>