From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from [140.186.70.92] (port=43553 helo=eggs.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1PIzRc-0000jm-Ro
	for qemu-devel@nongnu.org; Thu, 18 Nov 2010 03:02:13 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <kraxel@redhat.com>) id 1PIzRb-0004qx-JQ
	for qemu-devel@nongnu.org; Thu, 18 Nov 2010 03:02:12 -0500
Received: from mx1.redhat.com ([209.132.183.28]:64283)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <kraxel@redhat.com>) id 1PIzRb-0004qj-BG
	for qemu-devel@nongnu.org; Thu, 18 Nov 2010 03:02:11 -0500
Message-ID: <4CE4DD74.3070801@redhat.com>
Date: Thu, 18 Nov 2010 09:01:56 +0100
From: Gerd Hoffmann <kraxel@redhat.com>
MIME-Version: 1.0
References: <1290050875-23848-1-git-send-email-agraf@suse.de>
	<1290050875-23848-9-git-send-email-agraf@suse.de>
In-Reply-To: <1290050875-23848-9-git-send-email-agraf@suse.de>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Subject: [Qemu-devel] Re: [PATCH 08/10] ahci: add ahci emulation
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Alexander Graf <agraf@suse.de>
Cc: Joerg Roedel <Joerg.Roedel@amd.com>, QEMU-devel Developers <qemu-devel@nongnu.org>, Stefan Hajnoczi <stefanha@gmail.com>, tj@kernel.org, Roland Elek <elek.roland@gmail.com>, Sebastian Herbszt <herbszt@gmx.de>

   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