Re: [Qemu-devel] [RFC PATCH 1/8] qtest: add libqos

[Anthony Liguori <aliguori@us.ibm.com>]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 05.03.2013 um 14:53 hat Anthony Liguori geschrieben:
>> This includes basic PCI support.
>> 
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>
>> +static void *qpci_pc_iomap(QPCIBus *bus, QPCIDevice *dev, int barno)
>> +{
>> +    QPCIBusPC *s = container_of(bus, QPCIBusPC, bus);
>> +    static const int bar_reg_map[] = {
>> +        PCI_BASE_ADDRESS_0, PCI_BASE_ADDRESS_1, PCI_BASE_ADDRESS_2,
>> +        PCI_BASE_ADDRESS_3, PCI_BASE_ADDRESS_4, PCI_BASE_ADDRESS_5,
>> +    };
>> +    int bar_reg;
>> +    uint32_t addr;
>> +    uint64_t size;
>> +
>> +    g_assert(barno >= 0 && barno <= 5);
>> +    bar_reg = bar_reg_map[barno];
>> +
>> +    qpci_config_writel(dev, bar_reg, 0xFFFFFFFF);
>> +    addr = qpci_config_readl(dev, bar_reg);
>> +
>> +    size = (1ULL << ctol(addr));
>
> This doesn't look right. It should be something like:
>
>     size = (1ULL << ctzl(addr & ~0x3));
>
> In fact, what must be masked out differs for I/O (0x3) and memory
> (0xf).

You are correct, it should look something like:

if (addr & PCI_BASE_ADDRESS_SPACE_IO) {
    size = (1ULL << ctzl(addr & PCI_BASE_ADDRESS_IO_MASK));
} else {
    size = (1ULL << ctzl(addr & PCI_BASE_ADDRESS_MEM_MASK));
}

This doesn't deal with 64-bit bars though.

I'll update in the next round.  I think this has worked for me because I
have only done single device testing.  I did switch from ffz when I
rebased but I think ffz would still have this problem.

>> +QPCIDevice *qpci_device_find(QPCIBus *bus, int devfn)
>> +{
>> +    QPCIDevice *dev;
>> +
>> +    dev = g_malloc0(sizeof(*dev));
>
> Where is the matching free? I can't seem to destroy a device I
> once queried.

It's missing, I'll add it in the next round.

>> +    dev->bus = bus;
>> +    dev->devfn = devfn;
>> +
>> +    if (qpci_config_readw(dev, PCI_VENDOR_ID) == 0xFFFF) {
>> +        printf("vendor id is %x\n", qpci_config_readw(dev, PCI_VENDOR_ID));
>> +        g_free(dev);
>> +        return NULL;
>> +    }
>> +
>> +    return dev;
>> +}
>> +
>> +void qpci_device_enable(QPCIDevice *dev)
>> +{
>> +    uint16_t cmd;
>> +
>> +    /* FIXME -- does this need to be a bus callout? */
>> +    cmd = qpci_config_readw(dev, PCI_COMMAND);
>> +    cmd |= PCI_COMMAND_IO | PCI_COMMAND_MEMORY;
>> +    qpci_config_writew(dev, PCI_COMMAND, cmd);
>> +}
>
> Wouldn't it make sense to enable bus mastering here as well? Forgetting
> to do this manually is a trap that's easy to fall in...

Indeed, thanks for pointing it out.

Regards,

Anthony Liguori

>
> Kevin