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

[John Snow <jsnow@redhat.com>]

On 09/24/2014 10:35 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.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   hw/ide/core.c | 12 +++++++-----
>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 6fba056..1e43d50 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -2551,13 +2551,15 @@ const VMStateDescription vmstate_ide_bus = {
>>   void ide_drive_get(DriveInfo **hd, int max_bus)
>>   {
>>       int i;
>> +    int max_devs = if_get_max_devs(IF_IDE) * max_bus;
>
> Okay, here you need if_get_max_devs().  Suggest to move its introduction
> from PATCH 2 to this one.  Hmm, I guess we better change its name to
> start with drive_.
>
>> +    int buses = drive_get_max_bus(IF_IDE) + 1;
>>
>> -    if (drive_get_max_bus(IF_IDE) >= max_bus) {
>> -        fprintf(stderr, "qemu: too many IDE bus: %d\n", max_bus);
>> -        exit(1);
>> +    if (buses > max_bus) {
>> +        fprintf(stderr, "Warning: Too many IDE buses defined (%d > %d)\n",
>> +                buses, max_bus);
>
> New!  Error message now English!
>
> Since you touch it, you could use error_report().  Not important, as
> doesn't make much of a difference in this case.
>
>>       }
>>
>> -    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 < max_devs; i++) {
>> +        hd[i] = drive_get_by_index(IF_IDE, i);
>>       }
>>   }
>
> Maybe parameter max_bus should be replaced by the number of slots in
> hd[].  What do you think?
>

This is your only recommendation I haven't implemented yet for V2. I 
think this makes sense from a mechanical perspective because it would be 
nice to have the hard guarantee of not running over the array boundary 
instead of relying on the numbers in different places to be consistent.

The only reason I didn't do this is because I didn't want to touch calls 
to drive_get in 10 boards.

On the other hand, if the numbers are out of alignment and we run over 
the end of the array here, it's a board property not aligning with how 
the board init function works -- Not really a runtime issue, and 
something we can avoid relatively easily.

However, this could be a problem if we decide NOT to make the 
units-per-bus property constant across all Q35 types, for example, if we 
don't also then update how we generate this HD array.

(If Q35 1.6 does not use this property, we'll have 12 max devices 
implied, which is clearly wrong and why I advocate instating this 
property backwards for all versions.)

I think I am inclined to leave it as-is for now provided we agree that 
creating this property for all versions makes sense. If we wish to add 
the ability to have different numbers of units-per-bus properties per 
version, then the ide drive pickup code on all boards will have to get 
smarter.

Thoughts?