Re: [Qemu-arm] [Qemu-devel] [PATCH v4 09/14] pci: Add support for Designware IP block

[Marcel Apfelbaum]

On 30/01/2018 19:49, Andrey Smirnov wrote:
> On Tue, Jan 30, 2018 at 5:18 AM, Marcel Apfelbaum
> <address@hidden> wrote:
>> Hi Andrei,
>>
>> Sorry for letting you wait,
>> I have some comments/questions below.
>>
>>
>> On 16/01/2018 3:37, Andrey Smirnov wrote:
>>>
>>> Add code needed to get a functional PCI subsytem when using in
>>> conjunction with upstream Linux guest (4.13+). Tested to work against
>>> "e1000e" (network adapter, using MSI interrupts) as well as
>>> "usb-ehci" (USB controller, using legacy PCI interrupts).
>>>
>>> Cc: Peter Maydell <address@hidden>
>>> Cc: Jason Wang <address@hidden>
>>> Cc: Philippe Mathieu-Daudé <address@hidden>
>>> Cc: address@hidden
>>> Cc: address@hidden
>>> Cc: address@hidden
>>> Signed-off-by: Andrey Smirnov <address@hidden>
>>> ---
>>>   default-configs/arm-softmmu.mak  |   2 +
>>>   hw/pci-host/Makefile.objs        |   2 +
>>>   hw/pci-host/designware.c         | 618
>>> +++++++++++++++++++++++++++++++++++++++
>>>   include/hw/pci-host/designware.h |  93 ++++++
>>>   include/hw/pci/pci_ids.h         |   2 +
>>>   5 files changed, 717 insertions(+)
>>>   create mode 100644 hw/pci-host/designware.c
>>>   create mode 100644 include/hw/pci-host/designware.h
>>>
>>> diff --git a/default-configs/arm-softmmu.mak
>>> b/default-configs/arm-softmmu.mak
>>> index b0d6e65038..0c5ae914ed 100644
>>> --- a/default-configs/arm-softmmu.mak
>>> +++ b/default-configs/arm-softmmu.mak
>>> @@ -132,3 +132,5 @@ CONFIG_GPIO_KEY=y
>>>   CONFIG_MSF2=y
>>>   CONFIG_FW_CFG_DMA=y
>>>   CONFIG_XILINX_AXI=y
>>> +CONFIG_PCI_DESIGNWARE=y
>>> +
>>> diff --git a/hw/pci-host/Makefile.objs b/hw/pci-host/Makefile.objs
>>> index 9c7909cf44..0e2c0a123b 100644
>>> --- a/hw/pci-host/Makefile.objs
>>> +++ b/hw/pci-host/Makefile.objs
>>> @@ -17,3 +17,5 @@ common-obj-$(CONFIG_PCI_PIIX) += piix.o
>>>   common-obj-$(CONFIG_PCI_Q35) += q35.o
>>>   common-obj-$(CONFIG_PCI_GENERIC) += gpex.o
>>>   common-obj-$(CONFIG_PCI_XILINX) += xilinx-pcie.o
>>> +
>>> +common-obj-$(CONFIG_PCI_DESIGNWARE) += designware.o
>>> diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
>>> new file mode 100644
>>> index 0000000000..98fff5e5f3
>>> --- /dev/null
>>> +++ b/hw/pci-host/designware.c
>>> @@ -0,0 +1,618 @@
>>> +/*
>>> + * Copyright (c) 2017, Impinj, Inc.
>>
>> 2018 :)
>>
>>> + *
>>> + * Designware PCIe IP block emulation
>>> + *
>>> + * This library is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU Lesser General Public
>>> + * License as published by the Free Software Foundation; either
>>> + * version 2 of the License, or (at your option) any later version.
>>> + *
>>> + * This library is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>> + * Lesser General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU Lesser General Public
>>> + * License along with this library; if not, see
>>> + * <http://www.gnu.org/licenses/>.
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "qapi/error.h"
>>> +#include "hw/pci/msi.h"
>>> +#include "hw/pci/pci_bridge.h"
>>> +#include "hw/pci/pci_host.h"
>>> +#include "hw/pci/pcie_port.h"
>>> +#include "hw/pci-host/designware.h"
>>> +
>>> +#define PCIE_PORT_LINK_CONTROL          0x710
>>> +
>>> +#define PCIE_PHY_DEBUG_R1               0x72C
>>> +#define PCIE_PHY_DEBUG_R1_XMLH_LINK_UP  BIT(4)
>>> +
>>> +#define PCIE_LINK_WIDTH_SPEED_CONTROL   0x80C
>>> +
>>> +#define PCIE_MSI_ADDR_LO                0x820
>>> +#define PCIE_MSI_ADDR_HI                0x824
>>> +#define PCIE_MSI_INTR0_ENABLE           0x828
>>> +#define PCIE_MSI_INTR0_MASK             0x82C
>>> +#define PCIE_MSI_INTR0_STATUS           0x830
>>> +
>>> +#define PCIE_ATU_VIEWPORT               0x900
>>> +#define PCIE_ATU_REGION_INBOUND         (0x1 << 31)
>>> +#define PCIE_ATU_REGION_OUTBOUND        (0x0 << 31)
>>> +#define PCIE_ATU_REGION_INDEX2          (0x2 << 0)
>>> +#define PCIE_ATU_REGION_INDEX1          (0x1 << 0)
>>> +#define PCIE_ATU_REGION_INDEX0          (0x0 << 0)
>>> +#define PCIE_ATU_CR1                    0x904
>>> +#define PCIE_ATU_TYPE_MEM               (0x0 << 0)
>>> +#define PCIE_ATU_TYPE_IO                (0x2 << 0)
>>> +#define PCIE_ATU_TYPE_CFG0              (0x4 << 0)
>>> +#define PCIE_ATU_TYPE_CFG1              (0x5 << 0)
>>> +#define PCIE_ATU_CR2                    0x908
>>> +#define PCIE_ATU_ENABLE                 (0x1 << 31)
>>> +#define PCIE_ATU_BAR_MODE_ENABLE        (0x1 << 30)
>>> +#define PCIE_ATU_LOWER_BASE             0x90C
>>> +#define PCIE_ATU_UPPER_BASE             0x910
>>> +#define PCIE_ATU_LIMIT                  0x914
>>> +#define PCIE_ATU_LOWER_TARGET           0x918
>>> +#define PCIE_ATU_BUS(x)                 (((x) >> 24) & 0xff)
>>> +#define PCIE_ATU_DEVFN(x)               (((x) >> 16) & 0xff)
>>> +#define PCIE_ATU_UPPER_TARGET           0x91C
>>> +
>>> +static DesignwarePCIEHost *
>>> +designware_pcie_root_to_host(DesignwarePCIERoot *root)
>>> +{
>>> +    BusState *bus = qdev_get_parent_bus(DEVICE(root));
>>> +    return DESIGNWARE_PCIE_HOST(bus->parent);
>>> +}
>>> +
>>> +static void designware_pcie_root_msi_write(void *opaque, hwaddr addr,
>>> +                                           uint64_t val, unsigned len)
>>> +{
>>> +    DesignwarePCIERoot *root = DESIGNWARE_PCIE_ROOT(opaque);
>>> +    DesignwarePCIEHost *host = designware_pcie_root_to_host(root);
>>> +
>>> +    root->msi.intr[0].status |= (1 << val) & root->msi.intr[0].enable;
>>> +
>>> +    if (root->msi.intr[0].status & ~root->msi.intr[0].mask) {
>>> +        qemu_set_irq(host->pci.irqs[0], 1);
>>
>>
>> Sorry for the possibly dumb question, but "msi_write"
>> will result in raising an INTx ?
> 
> Correct, that's the intention. I wouldn't be surprised if I missed a
> better/canonical way to implement this.
> 

Not sure of a "better" way. The point was the "msi" naming
that suggests edge interrupts, while resulting into level interrupts.
I was wondering about the naming, nothing more.

>>>
>>> +    }
>>> +}
>>> +
>>> +const MemoryRegionOps designware_pci_host_msi_ops = {
>>> +    .write = designware_pcie_root_msi_write,
>>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>>
>>
>> You may want to limit the access size.
> 
> Good point, will do.
> 
>>>
>>> +};
>>> +
>>> +static void designware_pcie_root_update_msi_mapping(DesignwarePCIERoot
>>> *root)
>>> +
>>> +{
>>> +    DesignwarePCIEHost *host = designware_pcie_root_to_host(root);
>>> +    MemoryRegion *address_space = &host->pci.memory;
>>> +    MemoryRegion *mem = &root->msi.iomem;
>>> +    const uint64_t base = root->msi.base;
>>> +    const bool enable = root->msi.intr[0].enable;
>>> +
>>> +    if (memory_region_is_mapped(mem)) {
>>> +        memory_region_del_subregion(address_space, mem);
>>> +        object_unparent(OBJECT(mem));
>>> +    }
>>> +
>>> +    if (enable) {
>>> +        memory_region_init_io(mem, OBJECT(root),
>>> +                              &designware_pci_host_msi_ops,
>>> +                              root, "pcie-msi", 0x1000);
>>> +
>>> +        memory_region_add_subregion(address_space, base, mem);
>>
>>
>> What happens if is enabled twice?
> 
> Ideally this shouldn't happen since this function is only called when
> PCIE_MSI_INTR0_ENABLE transitions from "All IRQs disabled" to "At
> least one IRQ enabled" or the inverse (via "update_msi_mapping" in
> designware_pcie_root_config_write).
> 
> But, assuming I got my logic wrong there, my thinking was that if it
> gets enabled for the second time, first "if" statement in
> designware_pcie_root_update_msi_mapping() would remove old
> MemoryRegion and second one would add it back, so it end up being a
> "no-op". I might be violating some API usage rules, so, please don't
> hesitate to call me out on this if I do.
> 

OK, so I am pretty sure we call "memory_region_init_io" only once
in the init/realize part.

Then, if the address mappings is getting changed between the calls
you can use memory_region_del_subregion/memory_region_add_subregion on update.

If the mappings remains the same you can use memory_region_set_enabled
to disable/enable the memory region.

>> Is it possible to be also disabled?
>>
> 
> Yes, similar to the case above, except the "if (enabled)" is not going
> to be executed and there'd be no MemoryRegion to trigger MSI
> interrupt.
> 
>>
>>> +    }
>>> +}
>>> +
>>> +static DesignwarePCIEViewport *
>>> +designware_pcie_root_get_current_viewport(DesignwarePCIERoot *root)
>>> +{
>>> +    const unsigned int idx = root->atu_viewport & 0xF;
>>> +    const unsigned int dir = !!(root->atu_viewport &
>>> PCIE_ATU_REGION_INBOUND);
>>> +    return &root->viewports[dir][idx];
>>> +}
>>> +
>>> +static uint32_t
>>> +designware_pcie_root_config_read(PCIDevice *d, uint32_t address, int len)
>>> +{
>>> +    DesignwarePCIERoot *root = DESIGNWARE_PCIE_ROOT(d);
>>> +    DesignwarePCIEViewport *viewport =
>>> +        designware_pcie_root_get_current_viewport(root);
>>> +
>>> +    uint32_t val;
>>> +
>>> +    switch (address) {
>>> +    case PCIE_PORT_LINK_CONTROL:
>>> +    case PCIE_LINK_WIDTH_SPEED_CONTROL:
>>> +        val = 0xDEADBEEF;
>>> +        /* No-op */
>>
>>
>> Not really a no-op
>>
> 
> What I meant by "no-op" is that those registers do not implement their
> actual function and instead return obviously bogus value. I'll change
> the comment to state that in a more explicit way.
> 
>>> +        break;
>>> +
>>> +    case PCIE_MSI_ADDR_LO:
>>> +        val = root->msi.base;
>>> +        break;
>>> +
>>> +    case PCIE_MSI_ADDR_HI:
>>> +        val = root->msi.base >> 32;
>>> +        break;
>>> +
>>> +    case PCIE_MSI_INTR0_ENABLE:
>>> +        val = root->msi.intr[0].enable;
>>> +        break;
>>> +
>>> +    case PCIE_MSI_INTR0_MASK:
>>> +        val = root->msi.intr[0].mask;
>>> +        break;
>>> +
>>> +    case PCIE_MSI_INTR0_STATUS:
>>> +        val = root->msi.intr[0].status;
>>> +        break;
>>> +
>>> +    case PCIE_PHY_DEBUG_R1:
>>> +        val = PCIE_PHY_DEBUG_R1_XMLH_LINK_UP;
>>> +        break;
>>> +
>>> +    case PCIE_ATU_VIEWPORT:
>>> +        val = root->atu_viewport;
>>> +        break;
>>> +
>>> +    case PCIE_ATU_LOWER_BASE:
>>> +        val = viewport->base;
>>> +        break;
>>> +
>>> +    case PCIE_ATU_UPPER_BASE:
>>> +        val = viewport->base >> 32;
>>> +        break;
>>> +
>>> +    case PCIE_ATU_LOWER_TARGET:
>>> +        val = viewport->target;
>>> +        break;
>>> +
>>> +    case PCIE_ATU_UPPER_TARGET:
>>> +        val = viewport->target >> 32;
>>> +        break;
>>> +
>>> +    case PCIE_ATU_LIMIT:
>>> +        val = viewport->limit;
>>> +        break;
>>> +
>>> +    case PCIE_ATU_CR1:
>>> +    case PCIE_ATU_CR2:          /* FALLTHROUGH */
>>> +        val = viewport->cr[(address - PCIE_ATU_CR1) / sizeof(uint32_t)];
>>> +        break;
>>> +
>>> +    default:
>>> +        val = pci_default_read_config(d, address, len);
>>> +        break;
>>> +    }
>>> +
>>> +    return val;
>>> +}
>>> +
>>> +static uint64_t designware_pcie_root_data_read(void *opaque,
>>> +                                               hwaddr addr, unsigned len)
>>> +{
>>> +    DesignwarePCIERoot *root = DESIGNWARE_PCIE_ROOT(opaque);
>>> +    DesignwarePCIEViewport *viewport =
>>> +        designware_pcie_root_get_current_viewport(root);
>>> +
>>> +    const uint8_t busnum = PCIE_ATU_BUS(viewport->target);
>>> +    const uint8_t devfn  = PCIE_ATU_DEVFN(viewport->target);
>>> +    PCIBus    *pcibus    = pci_get_bus(PCI_DEVICE(root));
>>> +    PCIDevice *pcidev    = pci_find_device(pcibus, busnum, devfn);
>>> +
>>> +    if (pcidev) {
>>> +        addr &= PCI_CONFIG_SPACE_SIZE - 1;
>>> +
>>> +        return pci_host_config_read_common(pcidev, addr,
>>> +                                           PCI_CONFIG_SPACE_SIZE, len);
>>> +    }
>>
>> You can use "pci_data_read" instead
> 
> Good to know, will change.
> 
>>>
>>> +
>>> +    return UINT64_MAX;
>>> +}
>>> +
>>> +static void designware_pcie_root_data_write(void *opaque, hwaddr addr,
>>> +                                            uint64_t val, unsigned len)
>>> +{
>>> +    DesignwarePCIERoot *root = DESIGNWARE_PCIE_ROOT(opaque);
>>> +    DesignwarePCIEViewport *viewport =
>>> +        designware_pcie_root_get_current_viewport(root);
>>> +    const uint8_t busnum = PCIE_ATU_BUS(viewport->target);
>>> +    const uint8_t devfn  = PCIE_ATU_DEVFN(viewport->target);
>>> +    PCIBus    *pcibus    = pci_get_bus(PCI_DEVICE(root));
>>> +    PCIDevice *pcidev    = pci_find_device(pcibus, busnum, devfn);
>>> +
>>> +    if (pcidev) {
>>> +        addr &= PCI_CONFIG_SPACE_SIZE - 1;
>>> +        pci_host_config_write_common(pcidev, addr,
>>> +                                     PCI_CONFIG_SPACE_SIZE,
>>> +                                     val, len);
>>> +    }
>>
>> You can use pci_data_write instead.
>>
> 
> Ditto.
> 
>>> +}
>>> +
>>> +const MemoryRegionOps designware_pci_host_conf_ops = {
>>> +    .read = designware_pcie_root_data_read,
>>> +    .write = designware_pcie_root_data_write,
>>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>>
>>
>> Maybe you want to limit the access size also here.
>>
> 
> OK, will do.
> 
>>> +};
>>> +
>>> +static void designware_pcie_update_viewport(DesignwarePCIERoot *root,
>>> +                                            DesignwarePCIEViewport
>>> *viewport)
>>> +{
>>> +    DesignwarePCIEHost *host = designware_pcie_root_to_host(root);
>>> +
>>> +    MemoryRegion *mem     = &viewport->memory;
>>> +    const uint64_t target = viewport->target;
>>> +    const uint64_t base   = viewport->base;
>>> +    const uint64_t size   = (uint64_t)viewport->limit - base + 1;
>>> +    const bool inbound    = viewport->inbound;
>>> +
>>> +    MemoryRegion *source, *destination;
>>> +    const char *direction;
>>> +    char *name;
>>> +
>>> +    if (inbound) {
>>> +        source      = &host->pci.address_space_root;
>>> +        destination = get_system_memory();
>>> +        direction   = "Inbound";
>>> +    } else {
>>> +        source      = get_system_memory();
>>> +        destination = &host->pci.memory;
>>> +        direction   = "Outbound";
>>> +    }
>>> +
>>> +    if (memory_region_is_mapped(mem)) {
>>> +        /* Before we modify anything, unmap and destroy the region */
>>
>>
>> I saw this also before. Can you please explain a little
>> why/when do you need to destroy prev mappings?
>>
> 
> They are going to be updated every time a viewport (inbound or
> outbound) in address translation unit (iATU) is reconfigured. Because
> PCIE_ATU_*_TARGET register is used to configure which deivce/bus to
> address outgoing configuration TLP to, they (viewports) get
> reconfigured quite a bit. Corresponding functions in Linux kernel
> would be dw_pcie_prog_outbound_atu() and dw_pcie_rd_other_conf(). I
> wouldn't be surprised that the way I went about implementing it is far
> from optimal, so let me know if it is.
> 

The same as above, I think memory_region_init_io should be used once.


>>> +        memory_region_del_subregion(source, mem);
>>> +        object_unparent(OBJECT(mem));
>>> +    }
>>> +
>>> +    name = g_strdup_printf("PCI %s Viewport %p", direction, viewport);
>>> +
>>> +    switch (viewport->cr[0]) {
>>> +    case PCIE_ATU_TYPE_MEM:
>>> +        memory_region_init_alias(mem, OBJECT(root), name,
>>> +                                 destination, target, size);
>>> +        break;
>>> +    case PCIE_ATU_TYPE_CFG0:
>>> +    case PCIE_ATU_TYPE_CFG1:    /* FALLTHROUGH */
>>> +        if (inbound) {
>>> +            goto exit;
>>> +        }
>>> +
>>> +        memory_region_init_io(mem, OBJECT(root),
>>> +                              &designware_pci_host_conf_ops,
>>> +                              root, name, size);
>>> +        break;
>>> +    }
>>> +
>>> +    if (inbound) {
>>> +        memory_region_add_subregion_overlap(source, base,
>>> +                                            mem, -1);
>>> +    } else {
>>> +        memory_region_add_subregion(source, base, mem);
>>> +    }
>>> +
>>> + exit:
>>> +    g_free(name);
>>> +}
>>> +
>>> +static void designware_pcie_root_config_write(PCIDevice *d, uint32_t
>>> address,
>>> +                                              uint32_t val, int len)
>>> +{
>>> +    DesignwarePCIERoot *root = DESIGNWARE_PCIE_ROOT(d);
>>> +    DesignwarePCIEHost *host = designware_pcie_root_to_host(root);
>>> +    DesignwarePCIEViewport *viewport =
>>> +        designware_pcie_root_get_current_viewport(root);
>>> +
>>> +    switch (address) {
>>> +    case PCIE_PORT_LINK_CONTROL:
>>> +    case PCIE_LINK_WIDTH_SPEED_CONTROL:
>>> +    case PCIE_PHY_DEBUG_R1:
>>> +        /* No-op */
>>> +        break;
>>> +
>>> +    case PCIE_MSI_ADDR_LO:
>>> +        root->msi.base &= 0xFFFFFFFF00000000ULL;
>>> +        root->msi.base |= val;
>>> +        break;
>>> +
>>> +    case PCIE_MSI_ADDR_HI:
>>> +        root->msi.base &= 0x00000000FFFFFFFFULL;
>>> +        root->msi.base |= (uint64_t)val << 32;
>>> +        break;
>>> +
>>> +    case PCIE_MSI_INTR0_ENABLE: {
>>> +        const bool update_msi_mapping = !root->msi.intr[0].enable ^
>>> !!val;
>>> +
>>> +        root->msi.intr[0].enable = val;
>>> +
>>> +        if (update_msi_mapping) {
>>> +            designware_pcie_root_update_msi_mapping(root);
>>> +        }
>>> +        break;
>>> +    }
>>> +
>>> +    case PCIE_MSI_INTR0_MASK:
>>> +        root->msi.intr[0].mask = val;
>>> +        break;
>>> +
>>> +    case PCIE_MSI_INTR0_STATUS:
>>> +        root->msi.intr[0].status ^= val;
>>> +        if (!root->msi.intr[0].status) {
>>> +            qemu_set_irq(host->pci.irqs[0], 0);
>>> +        }
>>> +        break;
>>> +
>>> +    case PCIE_ATU_VIEWPORT:
>>> +        root->atu_viewport = val;
>>> +        break;
>>> +
>>> +    case PCIE_ATU_LOWER_BASE:
>>> +        viewport->base &= 0xFFFFFFFF00000000ULL;
>>> +        viewport->base |= val;
>>> +        break;
>>> +
>>> +    case PCIE_ATU_UPPER_BASE:
>>> +        viewport->base &= 0x00000000FFFFFFFFULL;
>>> +        viewport->base |= (uint64_t)val << 32;
>>> +        break;
>>> +
>>> +    case PCIE_ATU_LOWER_TARGET:
>>> +        viewport->target &= 0xFFFFFFFF00000000ULL;
>>> +        viewport->target |= val;
>>> +        break;
>>> +
>>> +    case PCIE_ATU_UPPER_TARGET:
>>> +        viewport->target &= 0x00000000FFFFFFFFULL;
>>> +        viewport->target |= val;
>>> +        break;
>>> +
>>> +    case PCIE_ATU_LIMIT:
>>> +        viewport->limit = val;
>>> +        break;
>>> +
>>> +    case PCIE_ATU_CR1:
>>> +        viewport->cr[0] = val;
>>> +        break;
>>> +    case PCIE_ATU_CR2:
>>> +        viewport->cr[1] = val;
>>> +
>>> +        if (viewport->cr[1] & PCIE_ATU_ENABLE) {
>>> +            designware_pcie_update_viewport(root, viewport);
>>> +         }
>>> +        break;
>>> +
>>> +    default:
>>> +        pci_bridge_write_config(d, address, val, len);
>>> +        break;
>>> +    }
>>> +}
>>> +
>>> +static int designware_pcie_root_init(PCIDevice *dev)
>>> +{
>>
>>
>> Please use "realize" function rather than init.
>> We want to get rid of it eventually.
> 
> OK, will do.
> 
>>>
>>> +    DesignwarePCIERoot *root = DESIGNWARE_PCIE_ROOT(dev);
>>> +    PCIBridge *br = PCI_BRIDGE(dev);
>>> +    DesignwarePCIEViewport *viewport;
>>> +    size_t i;
>>> +
>>> +    br->bus_name  = "dw-pcie";
>>> +
>>> +    pci_set_word(dev->config + PCI_COMMAND,
>>> +                 PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
>>> +
>>> +    pci_config_set_interrupt_pin(dev->config, 1);
>>> +    pci_bridge_initfn(dev, TYPE_PCI_BUS);
>>
>> So this is a PCI Express Root Port "sitting" on PCI bus?
> 
> Yes, it is a built-in PCIe bridge, whose configuration space is mapped
> into CPU's address space (designware_pci_host_conf_ops) and the rest
> of PCIe hierarchy is presented through it.

My point was: is the bus PCIe or PCI? Because you used:
  pci_bridge_initfn(dev, TYPE_PCI_BUS);
and not
  pci_bridge_initfn(dev, TYPE_PCIE_BUS);

> 
>>
>>> +
>>> +    pcie_port_init_reg(dev);
>>> +
>>> +    pcie_cap_init(dev, 0x70, PCI_EXP_TYPE_ROOT_PORT,
>>> +                  0, &error_fatal);
>>> +
>>> +    msi_nonbroken = true;
>>> +    msi_init(dev, 0x50, 32, true, true, &error_fatal);
>>> +
>>> +    for (i = 0; i < DESIGNWARE_PCIE_NUM_VIEWPORTS; i++) {
>>> +        viewport = &root->viewports[DESIGNWARE_PCIE_VIEWPORT_INBOUND][i];
>>> +        viewport->inbound = true;
>>> +    }
>>> +
>>> +    /*
>>> +     * If no inbound iATU windows are configured, HW defaults to
>>> +     * letting inbound TLPs to pass in. We emulate that by exlicitly
>>> +     * configuring first inbound window to cover all of target's
>>> +     * address space.
>>> +     *
>>> +     * NOTE: This will not work correctly for the case when first
>>> +     * configured inbound window is window 0
>>> +     */
>>> +    viewport = &root->viewports[DESIGNWARE_PCIE_VIEWPORT_INBOUND][0];
>>> +    viewport->base   = 0x0000000000000000ULL;
>>> +    viewport->target = 0x0000000000000000ULL;
>>> +    viewport->limit  = UINT32_MAX;
>>> +    viewport->cr[0]  = PCIE_ATU_TYPE_MEM;
>>> +
>>> +    designware_pcie_update_viewport(root, viewport);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static void designware_pcie_set_irq(void *opaque, int irq_num, int level)
>>> +{
>>> +    DesignwarePCIEHost *host = DESIGNWARE_PCIE_HOST(opaque);
>>> +
>>> +    qemu_set_irq(host->pci.irqs[irq_num], level);
>>> +}
>>> +
>>> +static const char *designware_pcie_host_root_bus_path(PCIHostState
>>> *host_bridge,
>>> +                                                      PCIBus *rootbus)
>>> +{
>>> +    return "0000:00";
>>> +}
>>> +
>>> +
>>> +static void designware_pcie_root_class_init(ObjectClass *klass, void
>>> *data)
>>> +{
>>> +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>> +
>>> +    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>>> +
>>> +    k->vendor_id = PCI_VENDOR_ID_SYNOPSYS;
>>> +    k->device_id = 0xABCD;
>>> +    k->revision = 0;
>>> +    k->class_id = PCI_CLASS_BRIDGE_HOST;
>>
>> So is a Root Port with call is "BRIDGE_HOST" ?
>>
> 
> I think I am missing some PCI subsystem knowledge to understand that
> question, would you mind re-phrasing it?

Sure, a Root Port is a type of PCI bridge, so I was expecting
the class id to be: PCI_CLASS_BRIDGE_PCI and not PCI_CLASS_BRIDGE_HOST.


> 
>>> +    k->is_express = true;
>>> +    k->is_bridge = true;
>>> +    k->init = designware_pcie_root_init;
>>> +    k->exit = pci_bridge_exitfn;
>>> +    dc->reset = pci_bridge_reset;
>>> +    k->config_read = designware_pcie_root_config_read;
>>> +    k->config_write = designware_pcie_root_config_write;
>>
>>
>> Please add category:
>> set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> 
> It's already there, line 4 of the function.
> 

Sorry for the noise

>>>
>>> +
>>> +    /*
>>>
>>> +     * PCI-facing part of the host bridge, not usable without the
>>> +     * host-facing part, which can't be device_add'ed, yet.
>>> +     */
>>> +    dc->user_creatable = false;
>>> +}
>>> +
>>> +static uint64_t designware_pcie_host_mmio_read(void *opaque, hwaddr addr,
>>> +                                               unsigned int size)
>>> +{
>>> +    PCIHostState *pci = PCI_HOST_BRIDGE(opaque);
>>> +    PCIDevice *device = pci_find_device(pci->bus, 0, 0);
>>> +
>>> +    return pci_host_config_read_common(device,
>>> +                                       addr,
>>> +                                       pci_config_size(device),
>>> +                                       size);
>>> +}
>>> +
>>> +static void designware_pcie_host_mmio_write(void *opaque, hwaddr addr,
>>> +                                            uint64_t val, unsigned int
>>> size)
>>> +{
>>> +    PCIHostState *pci = PCI_HOST_BRIDGE(opaque);
>>> +    PCIDevice *device = pci_find_device(pci->bus, 0, 0);
>>> +
>>> +    return pci_host_config_write_common(device,
>>> +                                        addr,
>>> +                                        pci_config_size(device),
>>> +                                        val, size);
>>> +}
>>> +
>>> +static const MemoryRegionOps designware_pci_mmio_ops = {
>>> +    .read       = designware_pcie_host_mmio_read,
>>> +    .write      = designware_pcie_host_mmio_write,
>>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>>> +};
>>> +
>>> +static AddressSpace *designware_pcie_host_set_iommu(PCIBus *bus, void
>>> *opaque,
>>> +                                                    int devfn)
>>> +{
>>> +    DesignwarePCIEHost *s = DESIGNWARE_PCIE_HOST(opaque);
>>> +
>>> +    return &s->pci.address_space;
>>> +}
>>> +
>>> +static void designware_pcie_host_realize(DeviceState *dev, Error **errp)
>>> +{
>>> +    PCIHostState *pci = PCI_HOST_BRIDGE(dev);
>>> +    DesignwarePCIEHost *s = DESIGNWARE_PCIE_HOST(dev);
>>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>>> +    size_t i;
>>> +
>>> +    for (i = 0; i < ARRAY_SIZE(s->pci.irqs); i++) {
>>> +        sysbus_init_irq(sbd, &s->pci.irqs[i]);
>>> +    }
>>> +
>>> +    memory_region_init_io(&s->mmio,
>>> +                          OBJECT(s),
>>> +                          &designware_pci_mmio_ops,
>>> +                          s,
>>> +                          "pcie.reg", 4 * 1024);
>>> +    sysbus_init_mmio(sbd, &s->mmio);
>>> +
>>> +    memory_region_init(&s->pci.io, OBJECT(s), "pcie-pio", 16);
>>> +    memory_region_init(&s->pci.memory, OBJECT(s),
>>> +                       "pcie-bus-memory",
>>> +                       UINT64_MAX);
>>> +
>>> +    pci->bus = pci_register_root_bus(dev, "pcie",
>>> +                                     designware_pcie_set_irq,
>>> +                                     pci_swizzle_map_irq_fn,
>>> +                                     s,
>>> +                                     &s->pci.memory,
>>> +                                     &s->pci.io,
>>> +                                     0, 4,
>>> +                                     TYPE_PCIE_BUS);
>>> +
>>> +    memory_region_init(&s->pci.address_space_root,
>>> +                       OBJECT(s),
>>> +                       "pcie-bus-address-space-root",
>>> +                       UINT64_MAX);
>>> +    memory_region_add_subregion(&s->pci.address_space_root,
>>> +                                0x0, &s->pci.memory);
>>> +    address_space_init(&s->pci.address_space,
>>> +                       &s->pci.address_space_root,
>>> +                       "pcie-bus-address-space");
>>> +    pci_setup_iommu(pci->bus, designware_pcie_host_set_iommu, s);
>>> +
>>> +    qdev_set_parent_bus(DEVICE(&s->root), BUS(pci->bus));
>>> +    qdev_init_nofail(DEVICE(&s->root));
>>> +}
>>> +
>>> +static void designware_pcie_host_class_init(ObjectClass *klass, void
>>> *data)
>>> +{
>>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>> +    PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
>>> +
>>> +    hc->root_bus_path = designware_pcie_host_root_bus_path;
>>> +    dc->realize = designware_pcie_host_realize;
>>> +    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>>> +    dc->fw_name = "pci";
>>> +}
>>> +
>>> +static void designware_pcie_host_init(Object *obj)
>>> +{
>>> +    DesignwarePCIEHost *s = DESIGNWARE_PCIE_HOST(obj);
>>> +    DesignwarePCIERoot *root = &s->root;
>>> +
>>> +    object_initialize(root, sizeof(*root), TYPE_DESIGNWARE_PCIE_ROOT);
>>> +    object_property_add_child(obj, "root", OBJECT(root), NULL);
>>> +    qdev_prop_set_int32(DEVICE(root), "addr", PCI_DEVFN(0, 0));
>>> +    qdev_prop_set_bit(DEVICE(root), "multifunction", false);
>>> +}
>>> +
>>> +static const TypeInfo designware_pcie_root_info = {
>>> +    .name = TYPE_DESIGNWARE_PCIE_ROOT,
>>> +    .parent = TYPE_PCI_BRIDGE,
>>> +    .instance_size = sizeof(DesignwarePCIERoot),
>>> +    .class_init = designware_pcie_root_class_init,
>>> +    .interfaces = (InterfaceInfo[]) {
>>> +        { INTERFACE_PCIE_DEVICE },
>>> +        { }
>>> +    },
>>> +};
>>> +
>>> +static const TypeInfo designware_pcie_host_info = {
>>> +    .name       = TYPE_DESIGNWARE_PCIE_HOST,
>>> +    .parent     = TYPE_PCI_HOST_BRIDGE,
>>> +    .instance_size = sizeof(DesignwarePCIEHost),
>>> +    .instance_init = designware_pcie_host_init,
>>> +    .class_init = designware_pcie_host_class_init,
>>> +};
>>> +
>>> +static void designware_pcie_register(void)
>>> +{
>>> +    type_register_static(&designware_pcie_root_info);
>>> +    type_register_static(&designware_pcie_host_info);
>>> +}
>>> +type_init(designware_pcie_register)
>>> +
>>> +/* 00:00.0 Class 0604: 16c3:abcd */
>>> diff --git a/include/hw/pci-host/designware.h
>>> b/include/hw/pci-host/designware.h
>>> new file mode 100644
>>> index 0000000000..55e45fcba0
>>> --- /dev/null
>>> +++ b/include/hw/pci-host/designware.h
>>> @@ -0,0 +1,93 @@
>>> +/*
>>> + * Copyright (c) 2017, Impinj, Inc.
>>> + *
>>> + * Designware PCIe IP block emulation
>>> + *
>>> + * This library is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU Lesser General Public
>>> + * License as published by the Free Software Foundation; either
>>> + * version 2 of the License, or (at your option) any later version.
>>> + *
>>> + * This library is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>> + * Lesser General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU Lesser General Public
>>> + * License along with this library; if not, see
>>> + * <http://www.gnu.org/licenses/>.
>>> + */
>>> +
>>> +#ifndef DESIGNWARE_H
>>> +#define DESIGNWARE_H
>>> +
>>> +#include "hw/hw.h"
>>> +#include "hw/sysbus.h"
>>> +#include "hw/pci/pci.h"
>>> +#include "hw/pci/pci_bus.h"
>>> +#include "hw/pci/pcie_host.h"
>>> +#include "hw/pci/pci_bridge.h"
>>> +
>>> +#define TYPE_DESIGNWARE_PCIE_HOST "designware-pcie-host"
>>> +#define DESIGNWARE_PCIE_HOST(obj) \
>>> +     OBJECT_CHECK(DesignwarePCIEHost, (obj), TYPE_DESIGNWARE_PCIE_HOST)
>>> +
>>> +#define TYPE_DESIGNWARE_PCIE_ROOT "designware-pcie-root"
>>> +#define DESIGNWARE_PCIE_ROOT(obj) \
>>> +     OBJECT_CHECK(DesignwarePCIERoot, (obj), TYPE_DESIGNWARE_PCIE_ROOT)
>>> +
>>> +typedef struct DesignwarePCIEViewport {
>>> +    MemoryRegion memory;
>>> +
>>> +    uint64_t base;
>>> +    uint64_t target;
>>> +    uint32_t limit;
>>> +    uint32_t cr[2];
>>> +
>>> +    bool inbound;
>>> +} DesignwarePCIEViewport;
>>> +
>>> +typedef struct DesignwarePCIERoot {
>>> +    PCIBridge parent_obj;
>>> +
>>> +    uint32_t atu_viewport;
>>> +
>>> +#define DESIGNWARE_PCIE_VIEWPORT_OUTBOUND    0
>>> +#define DESIGNWARE_PCIE_VIEWPORT_INBOUND     1
>>> +#define DESIGNWARE_PCIE_NUM_VIEWPORTS        4
>>> +
>>> +    DesignwarePCIEViewport viewports[2][DESIGNWARE_PCIE_NUM_VIEWPORTS];
>>> +
>>> +    struct {
>>> +        uint64_t     base;
>>> +        MemoryRegion iomem;
>>> +
>>> +        struct {
>>> +            uint32_t enable;
>>> +            uint32_t mask;
>>> +            uint32_t status;
>>> +        } intr[1];
>>> +    } msi;
>>
>>
>> I think I didn't understand the msi struct above.
>> Is it some special msi implementation?
>> (related to the dumb question above)
> 
> Yes, it represents Designware specific MSI registers (part of a block
> of vendor specific registers in configuration space of the root PCIe
> bridge) as follows:
> 
>  - PCIE_PL_MSICA, PCIE_PL_MSICUA via msi.base
>  - PCIE_PL_MSICI0_ENB via msi.intr[0].enable
>  - PCIE_PL_MSICI0_MASK via msi.intr[0].mask
>  -  PCIE_PL_MSICI0_STATUS via msi.intr[0].status
> 
> i.MX6/7 specifies 8 sets of PCIE_PL_MSICn_* registers, so I defined it
> as an array, but since Linux only uses first set I limited the number
> of elements to 1.
> 

Got it, thanks.
Marcel

> Thanks,
> Andrey Smirnov
>