John Snow <js...@redhat.com> writes:
> On 09/30/2014 03:38 AM, Markus Armbruster wrote:
>> John Snow <js...@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 <js...@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.
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 :)
[...]
