From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45142) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XZ6iA-0004cF-HI for qemu-devel@nongnu.org; Tue, 30 Sep 2014 19:20:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XZ6i1-00081E-H9 for qemu-devel@nongnu.org; Tue, 30 Sep 2014 19:20:02 -0400 Received: from mx1.redhat.com ([209.132.183.28]:11558) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XZ6i1-00080t-8f for qemu-devel@nongnu.org; Tue, 30 Sep 2014 19:19:53 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s8UNJqbo001754 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Tue, 30 Sep 2014 19:19:52 -0400 Message-ID: <542B3A93.8040605@redhat.com> Date: Tue, 30 Sep 2014 19:19:47 -0400 From: John Snow MIME-Version: 1.0 References: <1412009238-13530-1-git-send-email-jsnow@redhat.com> <1412009238-13530-5-git-send-email-jsnow@redhat.com> <878ul1d0cq.fsf@blackfin.pond.sub.org> In-Reply-To: <878ul1d0cq.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 4/6] ide: Update ide_drive_get to be HBA agnostic List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, mst@redhat.com On 09/30/2014 03:38 AM, Markus Armbruster wrote: > John Snow 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 >> --- >> 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! > > [...] >