| Message ID | 1411490885-29782-5-git-send-email-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
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?
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?
John Snow <jsnow@redhat.com> writes: > 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. 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.
On 09/25/2014 02:13 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> 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. > > 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. On the subject of compatibly broken: Is there any circumstance where the mapping has *any* effect on the current working behavior? Since we do not support the shorthand syntax at all currently, there is no code that USES this mapping to do anything. Even if you specify -drive file=...,index=42; the -device that you currently *must* add simply ignores any index/bus/unit mappings you've already given the drive -- it doesn't seem to touch or use any information within the DriveInfo structure at all. That said, why not make this property retroactive? It won't affect anything. Or rather, the property alone won't. What *will* break older command lines is the fact that we are now supporting the syntactic sugar at all, but I think this is clearly a bug, and we should not go out of our way to tolerate broken syntax. Based on the above, I think I will: (A) Implement the property for all versions (B) Change the behavior of ide_drive_get to take the number of elements in the array, and imply the number of buses instead. If we decide to roll back older versions to be "compatibly broken," this will in effect limit us to three drives instead of six (index 0, 2, and 4) being mapped to ports 0, 1, and 2. This requires me touching the board init to adjust the call to drive_get slightly.
John Snow <jsnow@redhat.com> writes: > On the subject of compatibly broken: > Is there any circumstance where the mapping has *any* effect on the > current working behavior? Since we do not support the shorthand syntax > at all currently, there is no code that USES this mapping to do > anything. > > Even if you specify -drive file=...,index=42; the -device that you > currently *must* add simply ignores any index/bus/unit mappings you've > already given the drive -- it doesn't seem to touch or use any > information within the DriveInfo structure at all. I didn't think this through, and I knew it, so I chose to err on the side of excessive caution by posing the bug-compatibility question. You're right: since -drive if=ide was ignored before, we can change the mapping. > That said, why not make this property retroactive? It won't affect anything. > > Or rather, the property alone won't. What *will* break older command > lines is the fact that we are now supporting the syntactic sugar at > all, but I think this is clearly a bug, and we should not go out of > our way to tolerate broken syntax. Agreed. > Based on the above, I think I will: > > (A) Implement the property for all versions Yes, please. > (B) Change the behavior of ide_drive_get to take the number of > elements in the array, and imply the number of buses instead. If we > decide to roll back older versions to be "compatibly broken," this > will in effect limit us to three drives instead of six (index 0, 2, > and 4) being mapped to ports 0, 1, and 2. > > This requires me touching the board init to adjust the call to > drive_get slightly. Thanks!
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; + 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); } - 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); } }
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(-)