Re: [Qemu-devel] [PATCH v2 6/6] q35/ahci: Pick up -cdrom and -hda options

[John Snow]

On 09/30/2014 03:54 AM, Markus Armbruster wrote:
> John Snow <address@hidden> writes:
>
> > This patch implements the backend for the Q35 board
> > for us to be able to pick up and use drives defined
> > by the -cdrom, -hda, or -drive if=ide shorthand options.
> >
> > Signed-off-by: John Snow <address@hidden>
> > ---
> >   hw/i386/pc_q35.c |  4 ++++
> >   hw/ide/ahci.c    | 15 +++++++++++++++
> >   hw/ide/ahci.h    |  2 ++
> >   3 files changed, 21 insertions(+)
> >
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index b28ddbb..bb0dc8e 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -86,6 +86,7 @@ static void pc_q35_init(MachineState *machine)
> >       DeviceState *icc_bridge;
> >       PcGuestInfo *guest_info;
> >       ram_addr_t lowmem;
> > +    DriveInfo *hd[MAX_SATA_PORTS];
> >
> >       /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory
> >        * and 256 Mbytes for PCI Express Enhanced Configuration Access Mapping
> > @@ -253,6 +254,9 @@ static void pc_q35_init(MachineState *machine)
> >                                              true, "ich9-ahci");
> >       idebus[0] = qdev_get_child_bus(&ahci->qdev, "ide.0");
> >       idebus[1] = qdev_get_child_bus(&ahci->qdev, "ide.1");
> > +    g_assert_cmpint(MAX_SATA_PORTS, ==, ICH_AHCI(ahci)->ahci.ports);
> > +    ide_drive_get(hd, ICH_AHCI(ahci)->ahci.ports);
> > +    ahci_ide_create_devs(ahci, hd);
>
> The assertion is new since v1, and a bit more interesting than it looks
> on first glance.
>
> It protects the two calls following it, by ensuring the array has space
> for the ports.
>
> MAX_SATA_PORTS is defined to 6 in this file.
>
> ICH_AHCI(ahci)->ahci.ports is initialized by ahci_init() to its ports
> argument.  pci_ich9_ahci_init() passes literal 6.  Oookay.
>
> The assertion is more restrictive than required for correctness: >=
> would do.  I don't mind.

It's more of a warning that these two values are, for now, considered 
equivalent. If that should change in the future for some unthinkable 
reason, this assertion will help remind whoever changes it.

For now, Q35 uses ICH9. ICH9's AHCI controller has 6 ports. This should 
always be the case.

I could, I suppose, make the HD array in the heap, and ask the ICH9 how 
many ports it has, etc... The assertion was a quick, equally 
authoritative way that is not likely to need changing.

> It's tempting to do
>
>      ide_drive_get(hd, ARRAY_SIZE(hd));
>
> for more obvious correctness, except that'll screw with the detection of
> extra drives if ahci.ports ever becomes < MAX_SATA_PORTS.
>
> Good.
>
> >       if (usb_enabled(false)) {
> >           /* Should we create 6 UHCI according to ich9 spec? */
> > diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> > index 8978643..79abb6a 100644
> > --- a/hw/ide/ahci.c
> > +++ b/hw/ide/ahci.c
> > @@ -1419,3 +1419,18 @@ static void sysbus_ahci_register_types(void)
> >   }
> >
> >   type_init(sysbus_ahci_register_types)
> > +
> > +void ahci_ide_create_devs(PCIDevice *dev, DriveInfo **tab)
>
> Elsewhere, we call it DriveInfo **hd.  I'd stick to the common name.
>
> > +{
> > +    AHCIPCIState *d = ICH_AHCI(dev);
> > +    AHCIState *ahci = &d->ahci;
> > +    int i;
> > +
> > +    for (i = 0; i < ahci->ports; i++) {
> > +        if (tab[i] == NULL) {
> > +            continue;
> > +        }
> > +        ide_create_drive(&ahci->dev[i].port, 0, tab[i]);
> > +    }
> > +
> > +}
> > diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
> > index 1543df7..b6dc64e 100644
> > --- a/hw/ide/ahci.h
> > +++ b/hw/ide/ahci.h
> > @@ -332,4 +332,6 @@ void ahci_uninit(AHCIState *s);
> >
> >   void ahci_reset(AHCIState *s);
> >
> > +void ahci_ide_create_devs(PCIDevice *dev, DriveInfo **tab);
> > +
> >   #endif /* HW_IDE_AHCI_H */