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

[Andrey Smirnov <andrew.smirnov@gmail.com>]

On Wed, Jan 31, 2018 at 4:13 AM, Marcel Apfelbaum <marcel@redhat.com> wrote:
> On 30/01/2018 19:49, Andrey Smirnov wrote:
>> On Tue, Jan 30, 2018 at 5:18 AM, Marcel Apfelbaum
>> <marcel.apfelbaum@zoho.com> 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 <peter.maydell@linaro.org>
>>>> Cc: Jason Wang <jasowang@redhat.com>
>>>> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>> Cc: qemu-devel@nongnu.org
>>>> Cc: qemu-arm@nongnu.org
>>>> Cc: yurovsky@gmail.com
>>>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>>>> ---
>>>>   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.
>

Sounds good. Will do in v5.

>>> 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.
>

Will do in v5.

>
>>>> +        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);
>

Oops, my bad. Will fix in v5.

>>
>>>
>>>> +
>>>> +    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.
>

Yeah, that just mistake on my part since real HW reports
0604/PCI_CLASS_BRIDGE_PCI. Will fix in v5.

Thanks,
Andrey Smirnov