Re: [Qemu-ppc] [Qemu-devel] [PATCH 09/15] hw/ide: Emulate SiI3112 SATA controller

[John Snow]

On 08/22/2017 07:08 AM, BALATON Zoltan wrote:
> Hello,
> 
> Thanks for the review.
> 
> On Mon, 21 Aug 2017, John Snow wrote:
>> On 08/20/2017 01:23 PM, BALATON Zoltan wrote:
>>> This is a common generic PCI SATA conroller that is also used in PCs
>>> but more importantly guests running on the Sam460ex board prefer this
>>> card and have a driver for it (unlike for other SATA controllers
>>> already emulated).
>>
>> Oh, interesting. This is basically an alternative to the PCI BMDMA
>> interface and not really an alternative to AHCI. It doesn't really seem
> 
> Yes, this is a simple PCI SATA interface similar to IDE and not like
> AHCI. I think it's an updated version of the CMD640 IDE interface or
> more closely related to that. The Linux driver references CMD680 as well
> so it's probably a SATA version of that chip.
> 
>> to use any of the SATA-specific interfaces to SATA drives (cough, not
>> that we currently emulate a difference in QEMU... ATA and SATA both are
>> simply IDEState*) so this really seems like another PCI IDE interface
>> akin to the PCI BMDMA adapter we already have.
>>
>> It's just that guests will think they're using SATA, except... not. Not
>> a big deal, *I think*...
>>
>> ...It isn't a problem that our support for NCQ commands is tied to AHCI,
>> is it? Some of our current "SATA" support is tied very directly to AHCI
>> (see is_ncq() in ahci.c) -- is that relevant here?
> 
> I don't think any of the clients on Sam460ex tries to use NCQ so maybe
> it's OK. Linux might have a better driver but I'm not sure. This could
> be fixed later. Although I've seen a hang in Linux which I haven't
> debugged yet and don't know what causes it or if it's related to SATA at
> all.
> 
> I'm not sure I've implemented everything correctly but at least the
> firmware of the board can find disks and load files to boot OSes so
> basic functions should be working. There are some other bugs elsewhere
> which prevent me from testing more OSes at the moment. One of them is
> QEMU crashing sometimes while accessing this SATA controller but it's
> triggered from TCG generated code and depends on some timing because it
> goes away if I change code running in the client or add some debug logs.
> This is described here:
> 

"Elsewhere" as in "Not seemingly related to this controller," it seems.

> http://lists.nongnu.org/archive/html/qemu-ppc/2017-08/msg00249.html
> 
> You don't happen to have an idea or seen similar before, do you? (Likely
> it's not related to IDE but more likely some io memory region
> interaction with TCG.)
> 

Sorry, the traces look foreign to me.

> I intend to improve this device model later when found necessary but I
> don't have much free time for it now so I'd like to submit it now to get
> some more testing and maybe contributions from others.
> 

Sure, but be advised that if the device causes problems outside of this
use case and there's nobody willing or able to review it, that it may
get removed again.

I don't have a lot of free time to go through the register list point by
point and make sure this is implemented correctly either, but if this
helps your work I'm OK not holding it up.

>>> Signed-off-by: BALATON Zoltan <address@hidden>
>>
>> I'd like to invite you to create a Sam460ex MAINTAINERS stanza, add
>> yourself, and add hw/ide/sii3112.c to that stanza.
> 
> OK, I'll send a patch for that. David, is it enough to add sii3112 for
> now and leave the others under PPC? I'd get cc as main contributor for
> them anyway I think.
> 
>> --js
>>
>>> ---
>>>  hw/ide/Makefile.objs |   1 +
>>>  hw/ide/sii3112.c     | 369
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 370 insertions(+)
>>>  create mode 100644 hw/ide/sii3112.c
>>>
>>> diff --git a/hw/ide/Makefile.objs b/hw/ide/Makefile.objs
>>> index 729e9bd..76f3d6d 100644
>>> --- a/hw/ide/Makefile.objs
>>> +++ b/hw/ide/Makefile.objs
>>> @@ -10,3 +10,4 @@ common-obj-$(CONFIG_IDE_VIA) += via.o
>>>  common-obj-$(CONFIG_MICRODRIVE) += microdrive.o
>>>  common-obj-$(CONFIG_AHCI) += ahci.o
>>>  common-obj-$(CONFIG_AHCI) += ich.o
>>> +common-obj-$(CONFIG_IDE_SII3112) += sii3112.o
>>> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
>>> new file mode 100644
>>> index 0000000..9ec8cd1
>>> --- /dev/null
>>> +++ b/hw/ide/sii3112.c
>>> @@ -0,0 +1,369 @@
>>> +/*
>>> + * QEMU SiI3112A PCI to Serial ATA Controller Emulation
>>> + *
>>> + * Copyright (C) 2017 BALATON Zoltan <address@hidden>
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2
>>> or later.
>>> + * See the COPYING file in the top-level directory.
>>> + *
>>> + */
>>> +
>>> +/* For documentation on this and similar cards see:
>>> + * http://wiki.osdev.org/User:Quok/Silicon_Image_Datasheets
>>> + */
>>
>> Thank you so much for including docs!
> 
> Thank David who asked for it in the RFC review. :-)
> 
>>> +
>>> +#include <qemu/osdep.h>
>>> +#include <hw/ide/pci.h>
>>> +
>>> +#ifdef DEBUG_IDE
>>> +#define DPRINTF(fmt, ...) fprintf(stderr, fmt, ## __VA_ARGS__);
>>> +#else
>>> +#define DPRINTF(fmt, ...)
>>> +#endif /* DEBUG */
>>> +
>>
>> Please use trace events instead of DPRINTF; I'm in the process of
>> removing them from the rest of IDE.
> 
> OK. I found working with DPRINTF easier but I'll convert this to trace
> then.
> 

yeah, they're easier to implement but they're more prone to bitrot, and
harder to control with any finesse, so they're coming out for 2.11.

>>> +#define TYPE_SII3112_PCI "sii3112"
>>> +#define SII3112_PCI(obj) OBJECT_CHECK(SiI3112PCIState, (obj), \
>>> +                         TYPE_SII3112_PCI)
>>> +
>>> +typedef struct SiI3112Regs {
>>> +    uint32_t confstat;
>>> +    uint32_t scontrol;
>>> +    uint16_t sien;
>>> +    uint8_t swdata;
>>> +} SiI3112Regs;
>>> +
>>> +typedef struct SiI3112PCIState {
>>> +    PCIIDEState i;
>>> +    MemoryRegion mmio;
>>> +    SiI3112Regs regs[2];
>>> +} SiI3112PCIState;
>>> +
>>> +static uint64_t sii3112_reg_read(void *opaque, hwaddr addr,
>>> +                                unsigned int size)
>>> +{
>>> +    SiI3112PCIState *d = opaque;
>>> +    uint64_t val = 0;
>>> +
>>> +    switch (addr) {
>>> +    case 0x00:
>>> +        val = d->i.bmdma[0].cmd;
>>> +        break;
>>> +    case 0x01:
>>> +        val = d->regs[0].swdata;
>>> +        break;
>>> +    case 0x02:
>>> +        val = d->i.bmdma[0].status;
>>> +        break;
>>> +    case 0x03:
>>> +        val = 0;
>>> +        break;
>>> +    case 0x04 ... 0x07:
>>> +        val = bmdma_addr_ioport_ops.read(&d->i.bmdma[0], addr - 4,
>>> size);
>>> +        break;
>>> +    case 0x08:
>>> +        val = d->i.bmdma[1].cmd;
>>> +        break;
>>> +    case 0x09:
>>> +        val = d->regs[1].swdata;
>>> +        break;
>>> +    case 0x0a:
>>> +        val = d->i.bmdma[1].status;
>>> +        break;
>>> +    case 0x0b:
>>> +        val = 0;
>>> +        break;
>>> +    case 0x0c ... 0x0f:
>>> +        val = bmdma_addr_ioport_ops.read(&d->i.bmdma[1], addr - 12,
>>> size);
>>> +        break;
>>> +    case 0x10:
>>> +        val = d->i.bmdma[0].cmd;
>>> +        val |= (d->regs[0].confstat & (1UL << 11) ? (1 << 4) : 0);
>>> /*SATAINT0*/
>>> +        val |= (d->regs[1].confstat & (1UL << 11) ? (1 << 6) : 0);
>>> /*SATAINT1*/
>>> +        val |= (d->i.bmdma[1].status & BM_STATUS_INT ? (1 << 14) : 0);
>>> +        val |= d->i.bmdma[0].status << 16;
>>> +        val |= d->i.bmdma[1].status << 24;
>>> +        break;
>>> +    case 0x18:
>>> +        val = d->i.bmdma[1].cmd;
>>> +        val |= (d->regs[1].confstat & (1UL << 11) ? (1 << 4) : 0);
>>> +        val |= d->i.bmdma[1].status << 16;
>>> +        break;
>>> +    case 0x80 ... 0x87:
>>> +        if (size == 1) {
>>> +            val = ide_ioport_read(&d->i.bus[0], addr - 0x80);
>>> +        } else if (addr == 0x80) {
>>> +            val = (size == 2) ? ide_data_readw(&d->i.bus[0], 0) :
>>> +                                ide_data_readl(&d->i.bus[0], 0);
>>> +        } else {
>>> +            val = (1ULL << (size * 8)) - 1;
>>> +        }
>>> +        break;
>>> +    case 0x8a:
>>> +        val = (size == 1) ? ide_status_read(&d->i.bus[0], 4) :
>>> +                            (1ULL << (size * 8)) - 1;
>>> +        break;
>>> +    case 0xa0:
>>> +        val = d->regs[0].confstat;
>>> +        break;
>>> +    case 0xc0 ... 0xc7:
>>> +        if (size == 1) {
>>> +            val = ide_ioport_read(&d->i.bus[1], addr - 0xc0);
>>> +        } else if (addr == 0xc0) {
>>> +            val = (size == 2) ? ide_data_readw(&d->i.bus[1], 0) :
>>> +                                ide_data_readl(&d->i.bus[1], 0);
>>> +        } else {
>>> +            val = (1ULL << (size * 8)) - 1;
>>> +        }
>>> +        break;
>>> +    case 0xca:
>>> +        val = (size == 1) ? ide_status_read(&d->i.bus[0], 4) :
>>> +                            (1ULL << (size * 8)) - 1;
>>> +        break;
>>> +    case 0xe0:
>>> +        val = d->regs[1].confstat;
>>> +        break;
>>> +    case 0x100:
>>> +        val = d->regs[0].scontrol;
>>> +        break;
>>> +    case 0x104:
>>> +        val = (d->i.bus[0].ifs[0].blk) ? 0x113 : 0;
>>> +        break;
>>> +    case 0x148:
>>> +        val = d->regs[0].sien << 16;
>>> +        break;
>>> +    case 0x180:
>>> +        val = d->regs[1].scontrol;
>>> +        break;
>>> +    case 0x184:
>>> +        val = (d->i.bus[1].ifs[0].blk) ? 0x113 : 0;
>>> +        break;
>>> +    case 0x1c8:
>>> +        val = d->regs[1].sien << 16;
>>> +        break;
>>> +    default:
>>> +        val = 0;
>>> +    }
>>> +    DPRINTF("%s: addr 0x%"PRIx64 " size %d = %"PRIx64 "\n",
>>> +            __func__, addr, size, val);
>>> +    return val;
>>> +}
>>> +
>>> +static void sii3112_reg_write(void *opaque, hwaddr addr,
>>> +                              uint64_t val, unsigned int size)
>>> +{
>>> +    SiI3112PCIState *d = opaque;
>>> +
>>> +    DPRINTF("%s: addr 0x%"PRIx64 " size %d = %"PRIx64 "\n",
>>> +            __func__, addr, size, val);
>>> +    switch (addr) {
>>> +    case 0x00:
>>> +    case 0x10:
>>> +        bmdma_cmd_writeb(&d->i.bmdma[0], val);
>>> +        break;
>>> +    case 0x01:
>>> +    case 0x11:
>>> +        d->regs[0].swdata = val & 0x3f;
>>> +        break;
>>> +    case 0x02:
>>> +    case 0x12:
>>> +        d->i.bmdma[0].status = (val & 0x60) | (d->i.bmdma[0].status
>>> & 1) |
>>> +                               (d->i.bmdma[0].status & ~val & 6);
>>> +        break;
>>> +    case 0x04 ... 0x07:
>>> +        bmdma_addr_ioport_ops.write(&d->i.bmdma[0], addr - 4, val,
>>> size);
>>> +        break;
>>> +    case 0x08:
>>> +    case 0x18:
>>> +        bmdma_cmd_writeb(&d->i.bmdma[1], val);
>>> +        break;
>>> +    case 0x09:
>>> +    case 0x19:
>>> +        d->regs[1].swdata = val & 0x3f;
>>> +        break;
>>> +    case 0x0a:
>>> +    case 0x1a:
>>> +        d->i.bmdma[1].status = (val & 0x60) | (d->i.bmdma[1].status
>>> & 1) |
>>> +                               (d->i.bmdma[1].status & ~val & 6);
>>> +        break;
>>> +    case 0x0c ... 0x0f:
>>> +        bmdma_addr_ioport_ops.write(&d->i.bmdma[1], addr - 12, val,
>>> size);
>>> +        break;
>>
>>
>> What register(s) sit at [0x10 - 0x1a]?
>> Isn't BAR4 only 0x00 through 0x0F?
>>
>>
>>> +    case 0x80 ... 0x87:
>>> +        if (size == 1) {
>>> +            ide_ioport_write(&d->i.bus[0], addr - 0x80, val);
>>> +        } else if (addr == 0x80) {
>>> +            if (size == 2) {
>>> +                ide_data_writew(&d->i.bus[0], 0, val);
>>> +            } else {
>>> +                ide_data_writel(&d->i.bus[0], 0, val);
>>> +            }
>>> +        }
>>> +        break;
>>> +    case 0x8a:
>>> +        if (size == 1) {
>>> +            ide_cmd_write(&d->i.bus[0], 4, val);
>>> +        }
>>> +        break;
>>
>> IDE0 stuff.
>>
>>> +    case 0xc0 ... 0xc7:
>>> +        if (size == 1) {
>>> +            ide_ioport_write(&d->i.bus[1], addr - 0xc0, val);
>>> +        } else if (addr == 0xc0) {
>>> +            if (size == 2) {
>>> +                ide_data_writew(&d->i.bus[1], 0, val);
>>> +            } else {
>>> +                ide_data_writel(&d->i.bus[1], 0, val);
>>> +            }
>>> +        }
>>> +        break;
>>> +    case 0xca:
>>> +        if (size == 1) {
>>> +            ide_cmd_write(&d->i.bus[1], 4, val);
>>> +        }
>>> +        break;
>>
>> IDE1 stuff.
>>
>>> +    case 0x100:
>>> +        d->regs[0].scontrol = val & 0xfff;
>>> +        if (val & 1) {
>>> +            ide_bus_reset(&d->i.bus[0]);
>>> +        }
>>> +        break;
>>> +    case 0x148:
>>> +        d->regs[0].sien = (val >> 16) & 0x3eed;
>>> +        break;
>>> +    case 0x180:
>>> +        d->regs[1].scontrol = val & 0xfff;
>>> +        if (val & 1) {
>>> +            ide_bus_reset(&d->i.bus[1]);
>>> +        }
>>> +        break;
>>> +    case 0x1c8:
>>> +        d->regs[1].sien = (val >> 16) & 0x3eed;
>>> +        break;
>>
>> Not sure what these ones are.
> 
> These are some SATA specific registers mostly controlling features we
> don't emulate (such as power management, SATA standard level and also
> used to reset the connected drive which is the only thing emulated
> because drivers use this). 0x100 and 0x148 are for channel 0 the other
> two are corresponding registers for channel 1.
> 
>>> +    default:
>>> +        val = 0;
>>> +    }
>>> +}
>>> +
>>> +static const MemoryRegionOps sii3112_reg_ops = {
>>> +    .read = sii3112_reg_read,
>>> +    .write = sii3112_reg_write,
>>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>>> +};
>>> +
>>> +/* the PCI irq level is the logical OR of the two channels */
>>> +static void sii3112_update_irq(SiI3112PCIState *s)
>>> +{
>>> +    int i, set = 0;
>>> +
>>> +    for (i = 0; i < 2; i++) {
>>> +        set |= s->regs[i].confstat & (1UL << 11);
>>> +    }
>>> +    pci_set_irq(PCI_DEVICE(s), (set ? 1 : 0));
>>> +}
>>> +
>>> +static void sii3112_set_irq(void *opaque, int channel, int level)
>>> +{
>>> +    SiI3112PCIState *s = opaque;
>>> +
>>> +    DPRINTF("%s: channel %d level %d\n", __func__, channel, level);
>>> +    if (level) {
>>> +        s->regs[channel].confstat |= (1UL << 11);
>>> +    } else {
>>> +        s->regs[channel].confstat &= ~(1UL << 11);
>>> +    }
>>> +
>>> +    sii3112_update_irq(s);
>>> +}
>>> +
>>> +static void sii3112_reset(void *opaque)
>>> +{
>>> +    SiI3112PCIState *s = opaque;
>>> +    int i;
>>> +
>>> +    for (i = 0; i < 2; i++) {
>>> +        s->regs[i].confstat = 0x6515 << 16;
>>> +        ide_bus_reset(&s->i.bus[i]);
>>> +    }
>>> +}
>>> +
>>> +static void sii3112_pci_realize(PCIDevice *dev, Error **errp)
>>> +{
>>> +    SiI3112PCIState *d = SII3112_PCI(dev);
>>> +    PCIIDEState *s = PCI_IDE(dev);
>>> +    MemoryRegion *mr;
>>> +    qemu_irq *irq;
>>> +    int i;
>>> +
>>> +    pci_config_set_interrupt_pin(dev->config, 1);
>>> +    pci_set_byte(dev->config + PCI_CACHE_LINE_SIZE, 8);
>>> +
>>> +    memory_region_init_io(&d->mmio, OBJECT(d), &sii3112_reg_ops, d,
>>> +                         "sii3112.bar5", 0x200);
>>> +    pci_register_bar(dev, 5, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
>>> +
>>> +    mr = g_new(MemoryRegion, 1);
>>> +    memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0",
>>> &d->mmio, 0x80, 8);
>>> +    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, mr);
>>> +    mr = g_new(MemoryRegion, 1);
>>> +    memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1",
>>> &d->mmio, 0x88, 4);
>>> +    pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, mr);
>>> +    mr = g_new(MemoryRegion, 1);
>>
>> BARS 0 and 1 for IDE0 occupy 0x80 through 0x8C;
>>
>>> +    memory_region_init_alias(mr, OBJECT(d), "sii3112.bar2",
>>> &d->mmio, 0xc0, 8);
>>> +    pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, mr);
>>> +    mr = g_new(MemoryRegion, 1);
>>> +    memory_region_init_alias(mr, OBJECT(d), "sii3112.bar3",
>>> &d->mmio, 0xc8, 4);
>>> +    pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, mr);
>>> +    mr = g_new(MemoryRegion, 1);
>>
>> BARS 1 and 2 for IDE1 occupy 0xC0 through 0xCC;
>>
>>> +    memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4",
>>> &d->mmio, 0, 16);
>>> +    pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr);
>>
>> BAR4 at 0x00 through 0x0F which houses the BMDMA registers.
>>
>> What occupies 0x10 through 0x7F or 0x8D through 0x1FF?
> 
> I'm not sure, there's a table in the datasheet that details this.
> Basically everything is in BAR5 which is an mmio region and BAR0-4 are
> io space aliases into this mmio region. (I think this is to support
> accessing it both via mmio and io space or to make it similar to IDE to
> make it easier to port older drivers.) Most of the empty areas are
> documented as Reserved in the datasheet.
> 

oh, I did actually miss that you register BAR5 as MMIO first, thanks,
that clears this all up.

So we've got BAR5 that occupies 0x200 bytes of PCI MMIO space and for which:

[0x00 - 0x0F]: Duplicated as PCI I/O space via BAR4
[0x80 - 0x87]: Duplicated as PCI I/O space via BAR0
[0x88 - 0x8B]: Duplicated as PCI I/O space via BAR1
[0xC0 - 0xC7]: Duplicated as PCI I/O space via BAR2
[0xC8 - 0xCB]: Duplicated as PCI I/O space via BAR3

I might appreciate a comment somewhere near the read or write (or both)
with a quick pointer to check out section 6.7 of the spec for a listing
of the BAR5 offsets and meanings, with a brief mention of the
dual-mapping for the registers in BAR0-4.

Oh, as a last question, does the dual-mapping here work correctly with
respects to the offsets? I imagine the read/write functions get offsets
into the region mapped (0x200) and not necessarily offsets into the
region accessed (4, 8, 16 bytes etc) right?

>>> +
>>> +    irq = qemu_allocate_irqs(sii3112_set_irq, d, 2);
>>> +    for (i = 0; i < 2; i++) {
>>> +        ide_bus_new(&s->bus[i], sizeof(s->bus[i]), DEVICE(dev), i, 1);
>>> +        ide_init2(&s->bus[i], irq[i]);
>>> +
>>> +        bmdma_init(&s->bus[i], &s->bmdma[i], s);
>>> +        s->bmdma[i].bus = &s->bus[i];
>>> +        ide_register_restart_cb(&s->bus[i]);
>>> +    }
>>> +    qemu_register_reset(sii3112_reset, s);
>>> +}
>>> +
>>> +static void sii3112_pci_exitfn(PCIDevice *dev)
>>> +{
>>> +    PCIIDEState *d = PCI_IDE(dev);
>>> +    int i;
>>> +
>>> +    for (i = 0; i < 2; ++i) {
>>> +        memory_region_del_subregion(&d->bmdma_bar,
>>> &d->bmdma[i].extra_io);
>>> +        memory_region_del_subregion(&d->bmdma_bar,
>>> &d->bmdma[i].addr_ioport);
>>> +    }
>>> +}
>>> +
>>> +static void sii3112_pci_class_init(ObjectClass *klass, void *data)
>>> +{
>>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>> +    PCIDeviceClass *pd = PCI_DEVICE_CLASS(klass);
>>> +
>>> +    pd->vendor_id = 0x1095;
>>> +    pd->device_id = 0x3112;
>>> +    pd->class_id = PCI_CLASS_STORAGE_RAID;
>>> +    pd->revision = 1;
>>> +    pd->realize = sii3112_pci_realize;
>>> +    pd->exit = sii3112_pci_exitfn;
>>> +    dc->desc = "SiI3112A SATA controller";
>>> +    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>>> +}
>>> +
>>> +static const TypeInfo sii3112_pci_info = {
>>> +    .name = TYPE_SII3112_PCI,
>>> +    .parent = TYPE_PCI_IDE,
>>> +    .instance_size = sizeof(SiI3112PCIState),
>>> +    .class_init = sii3112_pci_class_init,
>>> +};
>>> +
>>> +static void sii3112_register_types(void)
>>> +{
>>> +    type_register_static(&sii3112_pci_info);
>>> +}
>>> +
>>> +type_init(sii3112_register_types)
>>>
>>
>>

-- 
—js