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

[Markus Armbruster]

John Snow <address@hidden> writes:

> On 09/24/2014 10:35 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.
>>>
>>> Signed-off-by: John Snow <address@hidden>
>>> ---
>>>   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.

Leaving this idea to a followup patch is fine, even if we decide now we
do want it.

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

An overrun is clearly a programming error.

The intent of replacing the parameter is to make the code clearer, not
to change its behavior.

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

Okay.  Let's figure out whether we want to keep the (bus, unit) mapping
bug-compatibly broken for old machine types, and get this series
wrapped.  Then, if we still care about making ide_drive_get() clearer,
try my idea to see how it works out.