Re: [Qemu-devel] [RFC v2 3/3] ahci: implement -cdrom and -hd[a-d]

[Markus Armbruster]

John Snow <address@hidden> writes:

> Signed-off-by: John Snow <address@hidden>
> ---
>  hw/i386/pc_q35.c |  3 +++
>  hw/ide/ahci.c    | 31 +++++++++++++++++++++++++++++++
>  hw/ide/ahci.h    |  3 +++
>  3 files changed, 37 insertions(+)
>
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index fd26fe1..0f33696 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,8 @@ 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");
> +    ahci_drive_get(ahci, hd);
> +    ahci_ide_create_devs(ahci, hd);
>  
>      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 ba69de3..ae28de4 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1402,3 +1402,34 @@ static void sysbus_ahci_register_types(void)
>  }
>  
>  type_init(sysbus_ahci_register_types)
> +
> +void ahci_drive_get(PCIDevice *dev, DriveInfo **tab)
> +{
> +    AHCIPCIState *d = ICH_AHCI(dev);
> +    AHCIState *ahci = &d->ahci;
> +    unsigned i;
> +
> +    if ((i = drive_get_max_bus(IF_IDE)) >= ahci->ports) {

I might be one of the strongest advocates for brevity on this list, but
even I frown on embedding assignments in conditionals without a genuine
need, and on reusing loop counters for unrelated purposes.

Moreover, you're mixing signed and unsigned: drive_get_max_bus() returns
int, ahci->ports is int32_t, but your i is unsigned.  Breaks when
drive_get_max_bus() returns -1 because no IF_IDE drives are defined:

    $ qemu -vnc :0 -M q35 -nodefaults
    AHCI: Too many IDE buses defined for AHCI (-1 > 5)

Stick to int.

    int n, i;

    n = drive_get_max_bus(IF_IDE);
    if (n >= ahci->ports) {

> +        fprintf(stderr, "AHCI: Too many IDE buses defined for AHCI (%d > 
> %d)\n",
> +                i, ahci->ports - 1);
> +    }
> +
> +    for (i = 0; i < ahci->ports; ++i) {

Compares unsigned i with signed ahci->ports.  Stick to int.

> +        tab[i] = drive_get_by_index(IF_IDE, i);
> +    }
> +}
> +
> +void ahci_ide_create_devs(PCIDevice *dev, DriveInfo **tab)
> +{
> +    AHCIPCIState *d = ICH_AHCI(dev);
> +    AHCIState *ahci = &d->ahci;
> +    unsigned i;
> +
> +    for (i = 0; i < ahci->ports; i++) {

Likewise.

> +        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..06a18de 100644
> --- a/hw/ide/ahci.h
> +++ b/hw/ide/ahci.h
> @@ -332,4 +332,7 @@ void ahci_uninit(AHCIState *s);
>  
>  void ahci_reset(AHCIState *s);
>  
> +void ahci_drive_get(PCIDevice *dev, DriveInfo **tab);
> +void ahci_ide_create_devs(PCIDevice *dev, DriveInfo **tab);
> +
>  #endif /* HW_IDE_AHCI_H */