Re: [Qemu-devel] [PATCH v3 2/2] ahci: add support for non-PCI based controllers

[Alexander Graf]

On 05.01.2012, at 16:11, Rob Herring wrote:

> On 01/05/2012 08:35 AM, Mark Langsdorf wrote:
>> On 01/05/2012 08:26 AM, Alexander Graf wrote:
>>> 
>>> On 05.01.2012, at 15:16, Andreas Färber wrote:
>>> 
>>>> Am 05.01.2012 14:52, schrieb Mark Langsdorf:
>>>>> From: Rob Herring <address@hidden>
>>>>> 
>>>>> Add support for ahci on sysbus.
>>>>> 
>>>>> Signed-off-by: Rob Herring <address@hidden>
>>>>> Signed-off-by: Mark Langsdorf <address@hidden>
>>>>> ---
>>>>> Changes from v1, v2
>>>>>   Corrected indentation of PlatAHCIState members
>>>>>   Made plat_ahci_info into a single structure, not a list
>>>>> 
>>>>> hw/ide/ahci.c |   31 +++++++++++++++++++++++++++++++
>>>>> 1 files changed, 31 insertions(+), 0 deletions(-)
>>>>> 
>>>>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>>>>> index 135d0ee..f052e55 100644
>>>>> --- a/hw/ide/ahci.c
>>>>> +++ b/hw/ide/ahci.c
>>>>> @@ -25,6 +25,7 @@
>>>>> #include <hw/msi.h>
>>>>> #include <hw/pc.h>
>>>>> #include <hw/pci.h>
>>>>> +#include <hw/sysbus.h>
>>>>> 
>>>>> #include "monitor.h"
>>>>> #include "dma.h"
>>>>> @@ -1214,3 +1215,33 @@ void ahci_reset(void *opaque)
>>>>>        ahci_reset_port(s, i);
>>>>>    }
>>>>> }
>>>>> +
>>>>> +typedef struct PlatAHCIState {
>>>>> +    SysBusDevice busdev;
>>>>> +    AHCIState ahci;
>>>>> +} PlatAHCIState;
>>>>> +
>>>>> +static int plat_ahci_init(SysBusDevice *dev)
>>>>> +{
>>>>> +    PlatAHCIState *s = FROM_SYSBUS(PlatAHCIState, dev);
>>>>> +    ahci_init(&s->ahci, &dev->qdev, 1);
>>>>> +
>>>>> +    sysbus_init_mmio(dev, &s->ahci.mem);
>>>>> +    sysbus_init_irq(dev, &s->ahci.irq);
>>> 
>>> It's still unclear to me how you connect an irq line on the command line. 
>>> How do you instantiate this device?
>> 
>> I'm not sure how it's done on the command line. In the SoC model that
>> this is intended for, I call sysbus_create_simple().
>> 
>>>>> +    qemu_register_reset(ahci_reset, &s->ahci);
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static SysBusDeviceInfo plat_ahci_info = {
>>>>> +    .qdev.name    = "plat-ahci",
>>>> 
>>>> The commit message does not given an indication where "plat" comes from
>>>> - is that an ARM device name?
>>> 
>>> "plat" here means "platform device". I'm not sure I like the naming though. 
>>> Basically it's a sysbus version of AHCI, similar to how virtio-mmio is a 
>>> sysbus version of virtio.
>>> 
>>> How about "sysbus-ahci"?
>> 
>> Sure. I'll make that change.
>> 
>>>> Does this patch actually make something work? If yes, please state so,
>>>> including usage instructions. If not, then I would suggest to hold this
>>>> one back and to send it together with any follow-on patches that wire it
>>>> up on some machine.
>>> 
>>> You can always just create the device manually with -device and hook it up 
>>> in the guest device tree or machine description manually.
>>> 
>>> However the question still stands: Have you verified this code works?
>> 
>> It's used in the Highbank SoC model, which hasn't been released yet. I'm
>> waiting on some other patches to make it upstream, mostly Trustzone
>> support now.
>> 
> Which has been extensively tested for some time with the Linux AHCI
> platform driver. The only issue we've seen are guest "DMA timeouts" when
> the disk image file is on an NFS share, but that should not be caused by
> this change.
> 
> Mark, there is not a qemu code dependency on Trustzone support, so we
> don't need to wait for that to add highbank support.

I agree, the SoC model should still work without trustzone. Just post the whole 
thing including this patch, so we make sure we don't have dead code in the 
tree. If the other patches you're waiting on look like they're basically 
accepted but need a bit to actually work their way through to upstream, just 
post the SoC patches with a comment saying that they are based on the others.


Alex