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

[Markus Armbruster]

John Snow <address@hidden> writes:

> On 09/30/2014 03:38 AM, Markus Armbruster wrote:
>> 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?
>
> 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.

Easy to miss :)

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

I have no strong opinion on fatal vs. warning here.  But such a change
should be a separate patch.

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

Calling exit() deep down a call chain is never nice, but it's not
actually a problem in ide_drive_get(), because it gets called only
during board initialization.  That said, if you *want* to make it nice,
go right ahead :)

[...]