| Message ID | 1412009238-13530-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. > > 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 <jsnow@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? > } > > - 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 < n; i++) { > + hd[i] = drive_get_by_index(IF_IDE, i); > } > + Stray blank line. > } Function is much nicer now, thanks! [...]
On 09/30/2014 03:38 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. >> >> 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 <jsnow@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. 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. 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. >> } >> >> - 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 < n; i++) { >> + hd[i] = drive_get_by_index(IF_IDE, i); >> } >> + > > Stray blank line. > >> } > > Function is much nicer now, thanks! > > [...] >
John Snow <jsnow@redhat.com> writes: > On 09/30/2014 03:38 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. >>> >>> 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 <jsnow@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 :) [...]
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; +} + 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); 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; + int n_buses = n / drive_get_max_devs(IF_IDE); - if (drive_get_max_bus(IF_IDE) >= max_bus) { - fprintf(stderr, "qemu: too many IDE bus: %d\n", max_bus); - exit(1); + /* 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); } - 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 < n; i++) { + hd[i] = drive_get_by_index(IF_IDE, i); } + } diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c index be286da..0239438 100644 --- a/hw/mips/mips_fulong2e.c +++ b/hw/mips/mips_fulong2e.c @@ -350,7 +350,7 @@ static void mips_fulong2e_init(MachineState *machine) pci_bus = bonito_init((qemu_irq *)&(env->irq[2])); /* South bridge */ - ide_drive_get(hd, MAX_IDE_BUS); + ide_drive_get(hd, MAX_IDE_BUS * MAX_IDE_DEVS); isa_bus = vt82c686b_init(pci_bus, PCI_DEVFN(FULONG2E_VIA_SLOT, 0)); if (!isa_bus) { diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c index 2d87de9..e19a125 100644 --- a/hw/mips/mips_malta.c +++ b/hw/mips/mips_malta.c @@ -1147,7 +1147,7 @@ void mips_malta_init(MachineState *machine) pci_bus = gt64120_register(isa_irq); /* Southbridge */ - ide_drive_get(hd, MAX_IDE_BUS); + ide_drive_get(hd, MAX_IDE_BUS * MAX_IDE_DEVS); piix4_devfn = piix4_init(pci_bus, &isa_bus, 80); diff --git a/hw/mips/mips_r4k.c b/hw/mips/mips_r4k.c index e219766..0d6f7ed 100644 --- a/hw/mips/mips_r4k.c +++ b/hw/mips/mips_r4k.c @@ -294,7 +294,7 @@ void mips_r4k_init(MachineState *machine) if (nd_table[0].used) isa_ne2000_init(isa_bus, 0x300, 9, &nd_table[0]); - ide_drive_get(hd, MAX_IDE_BUS); + ide_drive_get(hd, MAX_IDE_BUS * MAX_IDE_DEVS); for(i = 0; i < MAX_IDE_BUS; i++) isa_ide_init(isa_bus, ide_iobase[i], ide_iobase2[i], ide_irq[i], hd[MAX_IDE_DEVS * i], diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c index 1626db4..d291fee 100644 --- a/hw/ppc/mac_newworld.c +++ b/hw/ppc/mac_newworld.c @@ -400,7 +400,7 @@ static void ppc_core99_init(MachineState *machine) macio_init(macio, pic_mem, escc_bar); /* We only emulate 2 out of 3 IDE controllers for now */ - ide_drive_get(hd, MAX_IDE_BUS); + ide_drive_get(hd, MAX_IDE_BUS * MAX_IDE_DEVS); macio_ide = MACIO_IDE(object_resolve_path_component(OBJECT(macio), "ide[0]")); diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c index be9a194..9dd7940 100644 --- a/hw/ppc/mac_oldworld.c +++ b/hw/ppc/mac_oldworld.c @@ -278,7 +278,7 @@ static void ppc_heathrow_init(MachineState *machine) pci_nic_init_nofail(&nd_table[i], pci_bus, "ne2k_pci", NULL); - ide_drive_get(hd, MAX_IDE_BUS); + ide_drive_get(hd, MAX_IDE_BUS * MAX_IDE_DEVS); macio = pci_create(pci_bus, -1, TYPE_OLDWORLD_MACIO); dev = DEVICE(macio); diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c index f0ef1af..3e87cf9 100644 --- a/hw/ppc/prep.c +++ b/hw/ppc/prep.c @@ -519,7 +519,7 @@ static void ppc_prep_init(MachineState *machine) } } - ide_drive_get(hd, MAX_IDE_BUS); + ide_drive_get(hd, MAX_IDE_BUS * MAX_IDE_DEVS); for(i = 0; i < MAX_IDE_BUS; i++) { isa_ide_init(isa_bus, ide_iobase[i], ide_iobase2[i], ide_irq[i], hd[2 * i], diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c index 9c77e18..c1b84bc 100644 --- a/hw/sparc64/sun4u.c +++ b/hw/sparc64/sun4u.c @@ -864,7 +864,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem, for(i = 0; i < nb_nics; i++) pci_nic_init_nofail(&nd_table[i], pci_bus, "ne2k_pci", NULL); - ide_drive_get(hd, MAX_IDE_BUS); + ide_drive_get(hd, MAX_IDE_BUS * MAX_IDE_DEVS); pci_cmd646_ide_init(pci_bus, hd, 1); diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h index f23d495..81ec4fb 100644 --- a/include/sysemu/blockdev.h +++ b/include/sysemu/blockdev.h @@ -52,6 +52,7 @@ DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit); bool drive_check_orphaned(void); DriveInfo *drive_get_by_index(BlockInterfaceType type, int index); int drive_get_max_bus(BlockInterfaceType type); +int drive_get_max_devs(BlockInterfaceType type); DriveInfo *drive_get_next(BlockInterfaceType type); DriveInfo *drive_get_by_blockdev(BlockDriverState *bs);
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 <jsnow@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(-)