[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 02/26] blockdev: Introduce IF_AHCI
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v3 02/26] blockdev: Introduce IF_AHCI |
Date: |
Wed, 24 Oct 2012 17:50:25 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) |
Jason Baron <address@hidden> writes:
> On Mon, Oct 22, 2012 at 01:40:21PM +0200, Kevin Wolf wrote:
>> >> From: Jason Baron <address@hidden>
>> >>
>> >> Introduce IF_AHCI so that q35 can differentiate between ide and ahci
>> >> disks.
>> >> This allows q35 to specify its default disk type. It also allows q35 to
>> >> differentiate between ahci and ide disks, such that -drive if=ide does not
>> >> result in the creating of an ahci disk. This is important, since
>> >> we don't want
>> >> to have the meaning of if=ide changing once q35 is introduced. Thus, its
>> >> important for this to be applied before we introduce q35.
This isn't the real argument for IF_AHCI. The real argument is that the
(bus, unit) namespace for if=ide makes no sense for AHCI.
A board can have any number of IDE controllers. Each IDE controller
provides one or two buses, and each bus takes up to two units.
Together, we get a a board-specific number of buses, where each bus
takes up to two units.
A board can have any number of AHCI controllers. Each AHCI controller
provides a device-specific number of ports. Together, we get a
board-specific number of buses, where each bus can takes a bus-specific
number of units.
Plain q35 doesn't really need to differentiate between IDE and AHCI; it
has only AHCI. What it needs is a "one bus with six units" namespace.
We could fix this by relaxing if=ide's rigid "two units per bus" for new
machine types.
You fix it by introducing if=ahci. Probably simpler. But what's the
plan for the next generation of controller? Yet another interface type?
Note that SCSI suffers from similar rigidity: seven units per bus.
That's fine for SCSI-1, but not for some of the other variants.
"Differentiate" comes into play when a board sports both AHCI and IDE
controllers. Which q35 doesn't, does it?
Then, the new if=ahci lets users select the kind of controller more
easily than bus numbers would.
Drawback: existing command lines upgrade from pc to plain q35 gracefully
only if they don't specify if=ide explicitly. Upgrading to some q35
variant with IDE controller on board is even worse: it works, but
performance sucks. Some regard this as a feature.
>> >> This patch also adds:
>> >>
>> >> pci_ahci_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table)
>> >>
>> >> Which provides a convient way of attaching ahci drives to an
>> >> ahci controller.
>> >>
>> >> Reviewed-by: Paolo Bonzini <address@hidden>
>> >> Signed-off-by: Jason Baron <address@hidden>
>> >> ---
>> >
>> > Kevin, could you review/ack this patch pls?
>> >
>> >> blockdev.c | 13 ++++++++++++-
>> >> blockdev.h | 2 ++
>> >> hw/ide.h | 6 ++++++
>> >> hw/ide/ahci.c | 18 ++++++++++++++++++
>> >> hw/ide/core.c | 23 ++++++++++++++++++-----
>> >> 5 files changed, 56 insertions(+), 6 deletions(-)
>> >>
>> >> diff --git a/blockdev.c b/blockdev.c
>> >> index c9a49c8..b684348 100644
>> >> --- a/blockdev.c
>> >> +++ b/blockdev.c
>> >> @@ -33,6 +33,7 @@ static const char *const if_name[IF_COUNT] = {
>> >> [IF_SD] = "sd",
>> >> [IF_VIRTIO] = "virtio",
>> >> [IF_XEN] = "xen",
>> >> + [IF_AHCI] = "ahci",
>> >> };
>> >>
>> >> static const int if_max_devs[IF_COUNT] = {
>> >> @@ -52,8 +53,17 @@ static const int if_max_devs[IF_COUNT] = {
/*
* Do not change these numbers! They govern how drive option
* index maps to unit and bus. That mapping is ABI.
*
* All controllers used to imlement if=T drives need to support
* if_max_devs[T] units, for any T with if_max_devs[T] != 0.
* Otherwise, some index values map to "impossible" bus, unit
* values.
*
* For instance, if you change [IF_SCSI] to 255, -drive
* if=scsi,index=12 no longer means bus=1,unit=5, but
* bus=0,unit=12. With an lsi53c895a controller (7 units max),
* the drive can't be set up. Regression.
>> >> */
>> >> [IF_IDE] = 2,
>> >> [IF_SCSI] = 7,
>> >> + [IF_AHCI] = 6,
>> >> };
>>
>> What are the implications of this if we decided to add another AHCI
>> controller which had a different number of ports? I suspect that a
>> controller with less than 6 ports breaks when you add more drives than a
>> single controller can handle, and one with more than 6 ports doesn't use
>> up all of its ports before it adds another controller.
>>
>> Markus?
>>
>
> My plan was to make this field, machine dependent if/when we wanted a
> different
> size. I don't think it breaks anything to make this change at a later
> point. But please correct me, if I am wrong.
I'm afraid you are. See the comment immediately above, and commit
27d6bf40.
As far as I can see, the least bad solution is leaving
if_max_devs[IF_AHCI] zero. Makes index=N an alias for unit=N,bus=0, and
leaves rejecting invalid (bus,unit) to the board.
>> >> +int get_if_max_devs(BlockInterfaceType if_type)
>> >> +{
>> >> + assert(if_type < IF_COUNT);
>> >> + assert(if_type >= IF_DEFAULT);
>> >> +
>> >> + return if_max_devs[if_type];
>> >> +}
>>
>> if_max_devs has a specific obvious meaning within blockdev.c, but
>> outside it's not as obvious. So this function could use a rename.
>
> ok.
No, get rid of it. if_max_devs[]'s purpose is mapping between index and
(bus, unit), no more. See below.
>> >> /*
>> >> * We automatically delete the drive when a device using it gets
>> >> * unplugged. Questionable feature, but we can't just drop it.
>> >> @@ -518,7 +528,7 @@ DriveInfo *drive_init(QemuOpts *opts, int mach_if)
>> >> } else {
>> >> /* no id supplied -> create one */
>> >> dinfo->id = g_malloc0(32);
>> >> - if (type == IF_IDE || type == IF_SCSI)
>> >> + if (type == IF_IDE || type == IF_SCSI || type == IF_AHCI)
>> >> mediastr = (media == MEDIA_CDROM) ? "-cd" : "-hd";
>> >> if (max_devs)
>> >> snprintf(dinfo->id, 32, "%s%i%s%i",
>> >> @@ -550,6 +560,7 @@ DriveInfo *drive_init(QemuOpts *opts, int mach_if)
>> >>
>> >> switch(type) {
>> >> case IF_IDE:
>> >> + case IF_AHCI:
>> >> case IF_SCSI:
>> >> case IF_XEN:
>> >> case IF_NONE:
>> >> diff --git a/blockdev.h b/blockdev.h
>> >> index 8b126ad..bbd1017 100644
>> >> --- a/blockdev.h
>> >> +++ b/blockdev.h
>> >> @@ -21,6 +21,7 @@ typedef enum {
>> >> IF_DEFAULT = -1, /* for use with drive_add() only */
>> >> IF_NONE,
>> >> IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO,
>> >> IF_XEN,
>> >> + IF_AHCI,
>> >> IF_COUNT
>> >> } BlockInterfaceType;
>> >>
>> >> @@ -56,6 +57,7 @@ static inline int get_mach_if(int mach_if)
>> >> return mach_if;
>> >> }
>> >>
>> >> +int get_if_max_devs(BlockInterfaceType if_type);
>> >> DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
>> >> DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);
>> >> int drive_get_max_bus(BlockInterfaceType type);
>> >> diff --git a/hw/ide.h b/hw/ide.h
>> >> index 2db4079..0b7e000 100644
>> >> --- a/hw/ide.h
>> >> +++ b/hw/ide.h
>> >> @@ -4,6 +4,7 @@
>> >> #include "isa.h"
>> >> #include "pci.h"
>> >> #include "memory.h"
>> >> +#include "blockdev.h"
>> >>
>> >> #define MAX_IDE_DEVS 2
>> >>
>> >> @@ -34,6 +35,11 @@ int ide_get_geometry(BusState *bus, int unit,
>> >> int ide_get_bios_chs_trans(BusState *bus, int unit);
>> >>
>> >> /* ide/core.c */
>> >> +void ata_drive_get(DriveInfo **hd, int max_bus, BlockInterfaceType type);
>> >> void ide_drive_get(DriveInfo **hd, int max_bus);
>> >> +void ahci_drive_get(DriveInfo **hd, int max_bus);
>> >> +
>> >> +/* ide/ahci.c */
>> >> +void pci_ahci_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table);
Shouldn't this go next to pci_ide_create_devs()'s declaration?
Currently in hw/ide/pci.h.
>> >>
>> >> #endif /* HW_IDE_H */
>> >> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>> >> index 68671bc..824b86f 100644
>> >> --- a/hw/ide/ahci.c
>> >> +++ b/hw/ide/ahci.c
>> >> @@ -26,6 +26,7 @@
>> >> #include <hw/pc.h>
>> >> #include <hw/pci.h>
>> >> #include <hw/sysbus.h>
>> >> +#include <blockdev.h>
>> >>
>> >> #include "monitor.h"
>> >> #include "dma.h"
>> >> @@ -1260,3 +1261,20 @@ static void sysbus_ahci_register_types(void)
>> >> }
>> >>
>> >> type_init(sysbus_ahci_register_types)
>> >> +
>> >> +void pci_ahci_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table)
>> >> +{
>> >> + struct AHCIPCIState *dev = DO_UPCAST(struct AHCIPCIState, card,
>> >> pci_dev);
>> >> + int i;
>> >> + DriveInfo *drive;
>> >> +
>> >> + for (i = 0; i < dev->ahci.ports; i++) {
>> >> + if (hd_table[i] == NULL) {
>> >> + continue;
>> >> + }
>> >> + drive = hd_table[i];
>> >> + assert(drive->type == IF_AHCI);
>> >> + ide_create_drive(&dev->ahci.dev[i].port, 0,
>> >> + hd_table[i]);
>> >> + }
>> >> +}
Shouldn't this go next to pci_ide_create_devs()'s definition? Currently
in hw/ide/pci.c.
Consider adding a parameter for the number of elements in hd_table[],
for robustness. As is, the caller has to ensure hd_table[] has at least
dev->ahci.ports elements, which is not obvious from the function
signature.
>> >> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> >> index d683a8c..044da3c 100644
>> >> --- a/hw/ide/core.c
>> >> +++ b/hw/ide/core.c
>> >> @@ -2341,16 +2341,29 @@ const VMStateDescription vmstate_ide_bus = {
>> >> }
>> >> };
>> >>
>> >> -void ide_drive_get(DriveInfo **hd, int max_bus)
>> >> +void ata_drive_get(DriveInfo **hd, int max_bus, BlockInterfaceType type)
>>
>> Could be static?
>>
>
> Yes.
>
>> >> {
>> >> int i;
>> >> + int max_devs;
>> >> +
>> >> + assert((type == IF_IDE) || type == IF_AHCI);
>>
>> Interesting use of brackets.
>
> oops.
>
>>
>> >>
>> >> - if (drive_get_max_bus(IF_IDE) >= max_bus) {
>> >> + if (drive_get_max_bus(type) >= max_bus) {
>> >> fprintf(stderr, "qemu: too many IDE bus: %d\n", max_bus);
>>
>> "Too many %s buses", type == IF_IDE ? "IDE" : "AHCI"
>
> ok.
>
>>
>> >> exit(1);
>> >> }
>> >> -
>> >> - for(i = 0; i < max_bus * MAX_IDE_DEVS; i++) {
>> >> - hd[i] = drive_get(IF_IDE, i / MAX_IDE_DEVS, i % MAX_IDE_DEVS);
>> >> + max_devs = get_if_max_devs(type);
>> >> + for (i = 0; i < max_bus * max_devs; i++) {
>> >> + hd[i] = drive_get(type, i / max_devs, i % max_devs);
>> >> }
>> >> }
>> >> +
>> >> +void ide_drive_get(DriveInfo **hd, int max_bus)
>> >> +{
>> >> + ata_drive_get(hd, max_bus, IF_IDE);
>> >> +}
>> >> +
>> >> +void ahci_drive_get(DriveInfo **hd, int max_bus)
>> >> +{
>> >> + ata_drive_get(hd, max_bus, IF_AHCI);
>> >> +}
For IDE, we always have two devices per bus. The boards number of buses
varies in theory, and is always two in practice. Thus we have "#define
MAX_IDE_DEVS 2" in ide.h, and "#define MAX_IDE_BUS 2" in all the
<board>.c. The latter is passed to ide_drive_get() as parameter.
Getting the well-known MAX_IDE_DEVS from get_if_max_devs() buys us
nothing.
For AHCI, I figure we have a device-specific number of ports per device,
and a board-specific number of devices (typically one). If there are
multiple devices, they don't necessarily sport the same number of ports.
Thus, a nested loop does not work.
Suggest
void ahci_drive_get(DriveInfo **hd, int bus, int max_ports)
which the board can call once for each device, with the device's true
number of ports as argument. Look ma, no get_if_max_devs()!
- [Qemu-devel] [PATCH v3 04/26] pci: introduce pci_swizzle_map_irq_fn() for standardized interrupt pin swizzle, (continued)
[Qemu-devel] [PATCH v3 06/26] pc: Move ioapic_init() from pc_piix.c to pc.c, Jason Baron, 2012/10/19
[Qemu-devel] [PATCH v3 08/26] pci_ids: add intel 82801BA pci-to-pci bridge id, Jason Baron, 2012/10/19
[Qemu-devel] [PATCH v3 01/26] blockdev: Introduce a default machine blockdev interface field, QEMUMachine->mach_if, Jason Baron, 2012/10/19