John Snow <js...@redhat.com> writes:
> On 09/24/2014 10:35 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.
>>>
>>> Signed-off-by: John Snow <js...@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.
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.
