[Qemu-devel] Re: [PATCH 08/10] ahci: add ahci emulation

[Gerd Hoffmann]

  Hi,

> +static void ahci_trigger_irq(AHCIState *s, AHCIDevice *d,
> +                             int irq_type)

> +static void ahci_check_irq(AHCIState *s)

MSI support would be nice to have.

> +#ifndef min
> +#define min(a, b) ((a)<  (b) ? (a) : (b))
> +#endif

osdep.h has MIN/MAX macros.

> +static void ahci_init(AHCIState *s, DeviceState *qdev)
> +{

> +        ide_bus_new(&ad->port, qdev);
> +        ide_init2(&ad->port, 0);

Good.

> +        if (dinfo) {
> +            ide_create_drive(&ad->port, 0, dinfo);
> +        }

That doesn't belong into the qdev init function.

Look how ide/isa.c handles it:  The qdev init function (isa_ide_initfn) 
doesn't create ide-drives at all.  And there is a convinience function 
(isa_ide_init) which first creates the controller, then attaches the 
drives.  The later is called from pc_init() with the drives specified 
via -drive if=ide (or -hda).

Does AHCI support drive hotplug btw?

> +static void ahci_reset(void *opaque)
> +{

> +    pci_config_set_vendor_id(d->card.config, PCI_VENDOR_ID_INTEL);
> +    pci_config_set_device_id(d->card.config, PCI_DEVICE_ID_INTEL_ICH7M_AHCI);

> +    pci_config_set_class(d->card.config, PCI_CLASS_STORAGE_SATA);
> +    pci_config_set_prog_interface(d->card.config, AHCI_PROGMODE_MAJOR_REV_1);

> +    d->card.config[PCI_HEADER_TYPE]     = PCI_HEADER_TYPE_NORMAL;
> +    d->card.config[PCI_INTERRUPT_PIN]   = 1;     /* interrupt pin 0 */

Why is that in the reset function instead of init?  It should never ever 
change, should it?

> +static PCIDeviceInfo ahci_info = {
> +    .qdev.name  = "ahci",
> +    .qdev.size  = sizeof(AHCIPciState),
> +    .init       = pci_ahci_init,
> +    .exit       = pci_ahci_uninit,
> +    .qdev.props = (Property[]) {
> +        DEFINE_PROP_END_OF_LIST(),
> +    }

If there are no properties you can zap the last tree lines altogether ;)

cheers,
  Gerd