On Wed, Jan 31, 2018 at 4:13 AM, Marcel Apfelbaum <mar...@redhat.com> wrote: > On 30/01/2018 19:49, Andrey Smirnov wrote: >> On Tue, Jan 30, 2018 at 5:18 AM, Marcel Apfelbaum >> <marcel.apfelb...@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.mayd...@linaro.org> >>>> Cc: Jason Wang <jasow...@redhat.com> >>>> Cc: Philippe Mathieu-Daudé <f4...@amsat.org> >>>> Cc: qemu-devel@nongnu.org >>>> Cc: qemu-...@nongnu.org >>>> Cc: yurov...@gmail.com >>>> Signed-off-by: Andrey Smirnov <andrew.smir...@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