[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] pci: fix pci_requester_id()
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH v2] pci: fix pci_requester_id() |
Date: |
Mon, 16 May 2016 10:54:13 +0300 |
On Mon, May 16, 2016 at 03:13:20PM +0800, Peter Xu wrote:
> The original pci_requester_id() is not really getting requester ID, but
> only BDF. It will not work if we enable IOMMU IR with SID verifications
> when with PCI bridges. Renaming it to pci_get_bdf().
>
> Meanwhile, we provide the correct implementation to get requester
> ID. VT-d spec 5.1.1 is a good reference to go, though it talks only
> about interrupt delivery, the rule works exactly the same for
> non-interrupt cases.
>
> Currently, there are three use cases for pci_requester_id():
>
> - PCIX status bits: here we need BDF only, not requester ID. Replacing
> with pci_get_bdf().
> - PCIe Error injection and MSI delivery: for both these cases, we are
> looking for requester IDs. Kept untouched to leverage the new impl.
>
> Signed-off-by: Peter Xu <address@hidden>
> ---
> hw/i386/kvm/pci-assign.c | 2 +-
> hw/pci/pci.c | 29 +++++++++++++++++++++++++++++
> include/hw/pci/pci.h | 7 +++++--
> 3 files changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
> index bf425a2..c40ab36 100644
> --- a/hw/i386/kvm/pci-assign.c
> +++ b/hw/i386/kvm/pci-assign.c
> @@ -1481,7 +1481,7 @@ static int assigned_device_pci_cap_init(PCIDevice
> *pci_dev, Error **errp)
> * error bits, leave the rest. */
> status = pci_get_long(pci_dev->config + pos + PCI_X_STATUS);
> status &= ~(PCI_X_STATUS_BUS | PCI_X_STATUS_DEVFN);
> - status |= pci_requester_id(pci_dev);
> + status |= pci_get_bdf(pci_dev);
> status &= ~(PCI_X_STATUS_SPL_DISC | PCI_X_STATUS_UNX_SPL |
> PCI_X_STATUS_SPL_ERR);
> pci_set_long(pci_dev->config + pos + PCI_X_STATUS, status);
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index bb605ef..7430715 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2498,6 +2498,35 @@ PCIDevice *pci_get_function_0(PCIDevice *pci_dev)
> }
> }
>
> +/* Parse bridges up to the root complex and get final Requester ID
> + * for this device. For PCIe-all topology, this works exactly as
> + * pci_get_bdf() does. However, several tricks are required for
> + * legacy PCI topology and PCIe-to-PCI bridges, to be better aligned
> + * with spec. */
> +uint16_t pci_requester_id(PCIDevice *dev)
> +{
> + uint8_t bus_n;
> + uint16_t result = pci_get_bdf(dev);
add empty line here pls.
> + while ((bus_n = pci_bus_num(dev->bus))) {
> + /* We are under PCI/PCIe bridges */
> + dev = dev->bus->parent_dev;
> + if (pci_is_express(dev)) {
> + if (pcie_cap_get_type(dev) == PCI_EXP_TYPE_PCI_BRIDGE) {
> + /* When we pass through PCIe-to-PCI/PCIX bridges, we
> + * override the requester ID using secondary bus
> + * number with zeroed devfn (pcie-to-pci bridge spec
> + * chap 2.3). */
> + result = PCI_BUILD_BDF(bus_n, 0);
> + }
> + } else {
> + /* Legacy PCI bus, override requester ID with the
> + * bridge's BDF upstream. */
Don't document what is done, pls document why.
Please add an explanation why this is the right thing to do.
> + result = pci_get_bdf(dev);
Won't dev be NULL for a root bus?
> + }
> + }
> + return result;
> +}
> +
> static const TypeInfo pci_device_type_info = {
> .name = TYPE_PCI_DEVICE,
> .parent = TYPE_DEVICE,
OK but this is used on data path by msi_send_message and
doing scans there isn't nice.
Can we store a pointer to device who's requester id is used instead?
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index ef6ba51..351266c 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -15,6 +15,7 @@
> #define PCI_DEVFN(slot, func) ((((slot) & 0x1f) << 3) | ((func) & 0x07))
> #define PCI_SLOT(devfn) (((devfn) >> 3) & 0x1f)
> #define PCI_FUNC(devfn) ((devfn) & 0x07)
> +#define PCI_BUILD_BDF(bus, devfn) ((bus << 8) | (devfn))
> #define PCI_SLOT_MAX 32
> #define PCI_FUNC_MAX 8
>
> @@ -685,11 +686,13 @@ static inline uint32_t pci_config_size(const PCIDevice
> *d)
> return pci_is_express(d) ? PCIE_CONFIG_SPACE_SIZE :
> PCI_CONFIG_SPACE_SIZE;
> }
>
> -static inline uint16_t pci_requester_id(PCIDevice *dev)
> +static inline uint16_t pci_get_bdf(PCIDevice *dev)
> {
> - return (pci_bus_num(dev->bus) << 8) | dev->devfn;
> + return PCI_BUILD_BDF(pci_bus_num(dev->bus), dev->devfn);
> }
>
> +uint16_t pci_requester_id(PCIDevice *dev);
> +
> /* DMA access functions */
> static inline AddressSpace *pci_get_address_space(PCIDevice *dev)
> {
> --
> 2.4.11
- [Qemu-devel] [PATCH v2] pci: fix pci_requester_id(), Peter Xu, 2016/05/16
- Re: [Qemu-devel] [PATCH v2] pci: fix pci_requester_id(),
Michael S. Tsirkin <=
- Re: [Qemu-devel] [PATCH v2] pci: fix pci_requester_id(), Peter Xu, 2016/05/16
- Re: [Qemu-devel] [PATCH v2] pci: fix pci_requester_id(), Michael S. Tsirkin, 2016/05/16
- Re: [Qemu-devel] [PATCH v2] pci: fix pci_requester_id(), Peter Xu, 2016/05/16
- Re: [Qemu-devel] [PATCH v2] pci: fix pci_requester_id(), Alex Williamson, 2016/05/16
- Re: [Qemu-devel] [PATCH v2] pci: fix pci_requester_id(), Michael S. Tsirkin, 2016/05/16
- Re: [Qemu-devel] [PATCH v2] pci: fix pci_requester_id(), Peter Xu, 2016/05/17