qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v5 2/4] pcie: Add support for Single Root I/O Vi


From: Knut Omang
Subject: Re: [Qemu-devel] [PATCH v5 2/4] pcie: Add support for Single Root I/O Virtualization (SR/IOV)
Date: Sun, 18 Oct 2015 17:00:32 +0200

On Sun, 2015-10-18 at 14:02 +0300, Marcel Apfelbaum wrote:
> On 10/14/2015 06:51 PM, Knut Omang wrote:
> > This patch provides the building blocks for creating an SR/IOV
> > PCIe Extended Capability header and register/unregister
> > SR/IOV Virtual Functions.
> > 
> > Signed-off-by: Knut Omang <address@hidden>
> > ---
> >   hw/pci/Makefile.objs        |   2 +-
> >   hw/pci/pci.c                | 102 ++++++++++++-----
> >   hw/pci/pcie.c               |   2 +-
> >   hw/pci/pcie_sriov.c         | 263
> > ++++++++++++++++++++++++++++++++++++++++++++
> >   include/hw/pci/pci.h        |  11 +-
> >   include/hw/pci/pcie.h       |   6 +
> >   include/hw/pci/pcie_sriov.h |  58 ++++++++++
> >   include/qemu/typedefs.h     |   2 +
> >   trace-events                |   5 +
> >   9 files changed, 419 insertions(+), 32 deletions(-)
> >   create mode 100644 hw/pci/pcie_sriov.c
> >   create mode 100644 include/hw/pci/pcie_sriov.h
> > 
> > diff --git a/hw/pci/Makefile.objs b/hw/pci/Makefile.objs
> > index 9f905e6..2226980 100644
> > --- a/hw/pci/Makefile.objs
> > +++ b/hw/pci/Makefile.objs
> > @@ -3,7 +3,7 @@ common-obj-$(CONFIG_PCI) += msix.o msi.o
> >   common-obj-$(CONFIG_PCI) += shpc.o
> >   common-obj-$(CONFIG_PCI) += slotid_cap.o
> >   common-obj-$(CONFIG_PCI) += pci_host.o pcie_host.o
> > -common-obj-$(CONFIG_PCI) += pcie.o pcie_aer.o pcie_port.o
> > +common-obj-$(CONFIG_PCI) += pcie.o pcie_aer.o pcie_port.o
> > pcie_sriov.o
> > 
> >   common-obj-$(call lnot,$(CONFIG_PCI)) += pci-stub.o
> >   common-obj-$(CONFIG_ALL) += pci-stub.o
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index b095cfe..4fb5fcf 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -153,6 +153,9 @@ int pci_bar(PCIDevice *d, int reg)
> >   {
> >       uint8_t type;
> > 
> > +    /* PCIe virtual functions do not have their own BARs */
> > +    assert(!pci_is_vf(d));
> > +
> >       if (reg != PCI_ROM_SLOT)
> >           return PCI_BASE_ADDRESS_0 + reg * 4;
> > 
> > @@ -211,22 +214,13 @@ void pci_device_deassert_intx(PCIDevice *dev)
> >       }
> >   }
> > 
> > -static void pci_do_device_reset(PCIDevice *dev)
> > +static void pci_reset_regions(PCIDevice *dev)
> >   {
> >       int r;
> > +    if (pci_is_vf(dev)) {
> > +        return;
> > +    }
> > 
> > -    pci_device_deassert_intx(dev);
> > -    assert(dev->irq_state == 0);
> > -
> > -    /* Clear all writable bits */
> > -    pci_word_test_and_clear_mask(dev->config + PCI_COMMAND,
> > -                                 pci_get_word(dev->wmask +
> > PCI_COMMAND) |
> > -                                 pci_get_word(dev->w1cmask +
> > PCI_COMMAND));
> > -    pci_word_test_and_clear_mask(dev->config + PCI_STATUS,
> > -                                 pci_get_word(dev->wmask +
> > PCI_STATUS) |
> > -                                 pci_get_word(dev->w1cmask +
> > PCI_STATUS));
> > -    dev->config[PCI_CACHE_LINE_SIZE] = 0x0;
> > -    dev->config[PCI_INTERRUPT_LINE] = 0x0;
> >       for (r = 0; r < PCI_NUM_REGIONS; ++r) {
> >           PCIIORegion *region = &dev->io_regions[r];
> >           if (!region->size) {
> > @@ -240,10 +234,6 @@ static void pci_do_device_reset(PCIDevice
> > *dev)
> >               pci_set_long(dev->config + pci_bar(dev, r), region
> > ->type);
> >           }
> >       }
> > -    pci_update_mappings(dev);
> > -
> > -    msi_reset(dev);
> > -    msix_reset(dev);
> >   }
> > 
> >   /*
> > @@ -253,7 +243,23 @@ static void pci_do_device_reset(PCIDevice
> > *dev)
> >   void pci_device_reset(PCIDevice *dev)
> >   {
> >       qdev_reset_all(&dev->qdev);
> > -    pci_do_device_reset(dev);
> > +    pci_device_deassert_intx(dev);
> > +    assert(dev->irq_state == 0);
> > +
> > +    /* Clear all writable bits */
> > +    pci_word_test_and_clear_mask(dev->config + PCI_COMMAND,
> > +                                 pci_get_word(dev->wmask +
> > PCI_COMMAND) |
> > +                                 pci_get_word(dev->w1cmask +
> > PCI_COMMAND));
> > +    pci_word_test_and_clear_mask(dev->config + PCI_STATUS,
> > +                                 pci_get_word(dev->wmask +
> > PCI_STATUS) |
> > +                                 pci_get_word(dev->w1cmask +
> > PCI_STATUS));
> > +    dev->config[PCI_CACHE_LINE_SIZE] = 0x0;
> > +    dev->config[PCI_INTERRUPT_LINE] = 0x0;
> > +    pci_reset_regions(dev);
> > +    pci_update_mappings(dev);
> > +
> > +    msi_reset(dev);
> > +    msix_reset(dev);
> >   }
> > 
> >   /*
> > @@ -268,7 +274,7 @@ static void pcibus_reset(BusState *qbus)
> > 
> >       for (i = 0; i < ARRAY_SIZE(bus->devices); ++i) {
> >           if (bus->devices[i]) {
> > -            pci_do_device_reset(bus->devices[i]);
> > +            pci_device_reset(bus->devices[i]);
> 
> Hi,
> 
> Here you made the re-factoring:
>   - replaced pci_do_device_reset to a subset : pci_reset_regions.
>   - moved all the other code inside pci_device_reset.
> but now pcibus_reset will call pci_device_reset that has an extra
> call to qdev_reset_all.
> However pcibus_reset has several calling sites. My question is:
> 
> Is this necessary? 

No, I will fix it. I finally realize what has happened here,..

My intention is just to factor out the for loop that resets the BAR
registers, which is not necessary/possible in the VF case. That seemed
cleaner and more readable than wrapping it all in an if ( ... vf .. )
construct.

> If yes, I am only thinking how it will affect the other calling
> sites.
> Why did they split it in the first place? Maybe somebody knows.

Paolo's commit dcc20931 gives the answer to that.

My original patch set and Paolo's commit refactored this code in
slightly different ways which is the root of the problem, some of which
Marcel pointed at in the review of v3 (the bogus dev->irq_state = 0)

>  From what I saw it should be OK.

I have several virtual machines running with this code now but probably
just haven't hit the right use case yet, I suppose..

Thanks,
Knut

> 
> >           }
> >       }
> > 
> > @@ -771,6 +777,15 @@ static void pci_init_multifunction(PCIBus
> > *bus, PCIDevice *dev, Error **errp)
> >           dev->config[PCI_HEADER_TYPE] |=
> > PCI_HEADER_TYPE_MULTI_FUNCTION;
> >       }
> > 
> > +    /* With SR/IOV and ARI, a device at function 0 need not be a
> > multifunction
> > +     * device, as it may just be a VF that ended up with function
> > 0 in
> > +     * the legacy PCI interpretation. Avoid failing in such cases:
> > +     */
> > +    if (pci_is_vf(dev) &&
> > +        dev->exp.sriov_vf.pf->cap_present &
> > QEMU_PCI_CAP_MULTIFUNCTION) {
> > +        return;
> > +    }
> > +
> >       /*
> >        * multifunction bit is interpreted in two ways as follows.
> >        *   - all functions must set the bit to 1.
> > @@ -962,6 +977,7 @@ void pci_register_bar(PCIDevice *pci_dev, int
> > region_num,
> >       uint64_t wmask;
> >       pcibus_t size = memory_region_size(memory);
> > 
> > +    assert(!pci_is_vf(pci_dev)); /* VFs must use
> > pcie_sriov_vf_register_bar */
> >       assert(region_num >= 0);
> >       assert(region_num < PCI_NUM_REGIONS);
> >       if (size & (size-1)) {
> > @@ -1060,11 +1076,44 @@ pcibus_t pci_get_bar_addr(PCIDevice
> > *pci_dev, int region_num)
> >       return pci_dev->io_regions[region_num].addr;
> >   }
> > 
> > -static pcibus_t pci_bar_address(PCIDevice *d,
> > -                           int reg, uint8_t type, pcibus_t
> > size)
> > +
> > +static pcibus_t pci_config_get_bar_addr(PCIDevice *d, int reg,
> > +                                        uint8_t type, pcibus_t
> > size)
> > +{
> > +    pcibus_t new_addr;
> > +    if (!pci_is_vf(d)) {
> > +        int bar = pci_bar(d, reg);
> > +        if (type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> > +            new_addr = pci_get_quad(d->config + bar);
> > +        } else {
> > +            new_addr = pci_get_long(d->config + bar);
> > +        }
> > +    } else {
> > +        PCIDevice *pf = d->exp.sriov_vf.pf;
> > +        uint16_t sriov_cap = pf->exp.sriov_cap;
> > +        int bar = sriov_cap + PCI_SRIOV_BAR + reg * 4;
> > +        uint16_t vf_offset = pci_get_word(pf->config + sriov_cap +
> > PCI_SRIOV_VF_OFFSET);
> > +        uint16_t vf_stride = pci_get_word(pf->config + sriov_cap +
> > PCI_SRIOV_VF_STRIDE);
> > +        uint32_t vf_num = (d->devfn - (pf->devfn + vf_offset)) /
> > vf_stride;
> > +
> > +        if (type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> > +            new_addr = pci_get_quad(pf->config + bar);
> > +        } else {
> > +            new_addr = pci_get_long(pf->config + bar);
> > +        }
> > +        new_addr += vf_num * size;
> > +    }
> > +    if (reg != PCI_ROM_SLOT) {
> > +        /* Preserve the rom enable bit */
> > +        new_addr &= ~(size - 1);
> > +    }
> > +    return new_addr;
> > +}
> > +
> > +pcibus_t pci_bar_address(PCIDevice *d,
> > +                         int reg, uint8_t type, pcibus_t size)
> >   {
> >       pcibus_t new_addr, last_addr;
> > -    int bar = pci_bar(d, reg);
> >       uint16_t cmd = pci_get_word(d->config + PCI_COMMAND);
> >       Object *machine = qdev_get_machine();
> >       ObjectClass *oc = object_get_class(machine);
> > @@ -1075,7 +1124,7 @@ static pcibus_t pci_bar_address(PCIDevice *d,
> >           if (!(cmd & PCI_COMMAND_IO)) {
> >               return PCI_BAR_UNMAPPED;
> >           }
> > -        new_addr = pci_get_long(d->config + bar) & ~(size - 1);
> > +        new_addr = pci_config_get_bar_addr(d, reg, type, size);
> >           last_addr = new_addr + size - 1;
> >           /* Check if 32 bit BAR wraps around explicitly.
> >            * TODO: make priorities correct and remove this work
> > around.
> > @@ -1090,11 +1139,7 @@ static pcibus_t pci_bar_address(PCIDevice
> > *d,
> >       if (!(cmd & PCI_COMMAND_MEMORY)) {
> >           return PCI_BAR_UNMAPPED;
> >       }
> > -    if (type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> > -        new_addr = pci_get_quad(d->config + bar);
> > -    } else {
> > -        new_addr = pci_get_long(d->config + bar);
> > -    }
> > +    new_addr = pci_config_get_bar_addr(d, reg, type, size);
> >       /* the ROM slot has a specific enable bit */
> >       if (reg == PCI_ROM_SLOT && !(new_addr &
> > PCI_ROM_ADDRESS_ENABLE)) {
> >           return PCI_BAR_UNMAPPED;
> > @@ -1228,6 +1273,7 @@ void pci_default_write_config(PCIDevice *d,
> > uint32_t addr, uint32_t val_in, int
> > 
> >       msi_write_config(d, addr, val_in, l);
> >       msix_write_config(d, addr, val_in, l);
> > +    pcie_sriov_config_write(d, addr, val_in, l);
> >   }
> > 
> >   /***********************************************************/
> > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > index 6e28985..774b9ed 100644
> > --- a/hw/pci/pcie.c
> > +++ b/hw/pci/pcie.c
> > @@ -253,7 +253,7 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler
> > *hotplug_dev, DeviceState *dev,
> >        * Right now, only a device of function = 0 is allowed to be
> >        * hot plugged/unplugged.
> >        */
> > -    assert(PCI_FUNC(pci_dev->devfn) == 0);
> > +    assert(PCI_FUNC(pci_dev->devfn) == 0 || pci_is_vf(pci_dev));
> > 
> >       pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
> >                                  PCI_EXP_SLTSTA_PDS);
> > diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
> > new file mode 100644
> > index 0000000..25de8d9
> > --- /dev/null
> > +++ b/hw/pci/pcie_sriov.c
> > @@ -0,0 +1,263 @@
> > +/*
> > + * pcie_sriov.c:
> > + *
> > + * Implementation of SR/IOV emulation support.
> > + *
> > + * Copyright (c) 2015 Knut Omang <address@hidden>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2
> > or later.
> > + * See the COPYING file in the top-level directory.
> > + *
> > + */
> > +
> > +#include "hw/pci/pci.h"
> > +#include "hw/pci/pcie.h"
> > +#include "hw/pci/pci_bus.h"
> > +#include "qemu/error-report.h"
> > +#include "qemu/range.h"
> > +#include "trace.h"
> > +
> > +#define SRIOV_ID(dev) \
> > +    (dev)->name, PCI_SLOT((dev)->devfn), PCI_FUNC((dev)->devfn)
> 
> This is a little "hacky" but it is used only for tracing and inside a
> C file.
> I am OK with it.
> 
> > +
> > +static PCIDevice *register_vf(PCIDevice *pf, int devfn, const char
> > *name);
> > +static void unregister_vfs(PCIDevice *dev);
> > +
> > +void pcie_sriov_pf_init(PCIDevice *dev, uint16_t offset,
> > +                        const char *vfname, uint16_t vf_dev_id,
> > +                        uint16_t init_vfs, uint16_t total_vfs,
> > +                        uint16_t vf_offset, uint16_t vf_stride)
> > +{
> > +    uint8_t *cfg = dev->config + offset;
> > +    uint8_t *wmask;
> > +
> > +    pcie_add_capability(dev, PCI_EXT_CAP_ID_SRIOV, 1,
> > +                        offset, PCI_EXT_CAP_SRIOV_SIZEOF);
> > +    dev->exp.sriov_cap = offset;
> > +    dev->exp.sriov_pf.num_vfs = 0;
> > +    dev->exp.sriov_pf.vfname = g_strdup(vfname);
> > +    dev->exp.sriov_pf.vf = NULL;
> > +
> > +    pci_set_word(cfg + PCI_SRIOV_VF_OFFSET, vf_offset);
> > +    pci_set_word(cfg + PCI_SRIOV_VF_STRIDE, vf_stride);
> > +
> > +    /* Mandatory page sizes to support.
> > +     * Device implementations can call
> > pcie_sriov_pf_add_sup_pgsize()
> > +     * to set more bits:
> > +     */
> > +    pci_set_word(cfg + PCI_SRIOV_SUP_PGSIZE,
> > SRIOV_SUP_PGSIZE_MINREQ);
> > +
> > +    /* Default is to use 4K pages, software can modify it
> > +     * to any of the supported bits
> > +     */
> > +    pci_set_word(cfg + PCI_SRIOV_SYS_PGSIZE, 0x1);
> > +
> > +    /* Set up device ID and initial/total number of VFs available
> > */
> > +    pci_set_word(cfg + PCI_SRIOV_VF_DID, vf_dev_id);
> > +    pci_set_word(cfg + PCI_SRIOV_INITIAL_VF, init_vfs);
> > +    pci_set_word(cfg + PCI_SRIOV_TOTAL_VF, total_vfs);
> > +    pci_set_word(cfg + PCI_SRIOV_NUM_VF, 0);
> > +
> > +    /* Write enable control bits */
> > +    wmask = dev->wmask + offset;
> > +    pci_set_word(wmask + PCI_SRIOV_CTRL,
> > +                 PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE |
> > PCI_SRIOV_CTRL_ARI);
> > +    pci_set_word(wmask + PCI_SRIOV_NUM_VF, 0xffff);
> > +    pci_set_word(wmask + PCI_SRIOV_SYS_PGSIZE, 0x553);
> > +
> > +    qdev_prop_set_bit(&dev->qdev, "multifunction", true);
> > +}
> > +
> > +void pcie_sriov_pf_exit(PCIDevice *dev)
> > +{
> > +    unregister_vfs(dev);
> > +    g_free((char *)dev->exp.sriov_pf.vfname);
> > +    dev->exp.sriov_pf.vfname = NULL;
> > +}
> > +
> > +void pcie_sriov_pf_init_vf_bar(PCIDevice *dev, int region_num,
> > +                               uint8_t type, dma_addr_t size)
> > +{
> > +    uint32_t addr;
> > +    uint64_t wmask;
> > +    uint16_t sriov_cap = dev->exp.sriov_cap;
> > +
> > +    assert(sriov_cap > 0);
> > +    assert(region_num >= 0);
> > +    assert(region_num < PCI_NUM_REGIONS);
> > +    assert(region_num != PCI_ROM_SLOT);
> > +
> > +    wmask = ~(size - 1);
> > +    addr = sriov_cap + PCI_SRIOV_BAR + region_num * 4;
> > +
> > +    pci_set_long(dev->config + addr, type);
> > +    if (!(type & PCI_BASE_ADDRESS_SPACE_IO) &&
> > +        type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> > +        pci_set_quad(dev->wmask + addr, wmask);
> > +        pci_set_quad(dev->cmask + addr, ~0ULL);
> > +    } else {
> > +        pci_set_long(dev->wmask + addr, wmask & 0xffffffff);
> > +        pci_set_long(dev->cmask + addr, 0xffffffff);
> > +    }
> > +    dev->exp.sriov_pf.vf_bar_type[region_num] = type;
> > +}
> > +
> > +void pcie_sriov_vf_register_bar(PCIDevice *dev, int region_num,
> > +                                MemoryRegion *memory)
> > +{
> > +    PCIIORegion *r;
> > +    uint8_t type;
> > +    pcibus_t size = memory_region_size(memory);
> > +
> > +    assert(pci_is_vf(dev)); /* PFs must use pci_register_bar */
> > +    assert(region_num >= 0);
> > +    assert(region_num < PCI_NUM_REGIONS);
> > +    type = dev->exp.sriov_vf.pf
> > ->exp.sriov_pf.vf_bar_type[region_num];
> > +
> > +    if (!is_power_of_2(size)) {
> > +        error_report("%s: PCI region size must be a power"
> > +                     " of two - type=0x%x,
> > size=0x%"FMT_PCIBUS"\n",
> > +                     __func__, type, size);
> > +        exit(1);
> > +    }
> > +
> > +    r = &dev->io_regions[region_num];
> > +    r->memory = memory;
> > +    r->address_space =
> > +        type & PCI_BASE_ADDRESS_SPACE_IO
> > +        ? dev->bus->address_space_io
> > +        : dev->bus->address_space_mem;
> > +    r->size = size;
> > +    r->type = type;
> > +
> > +    r->addr = pci_bar_address(dev, region_num, r->type, r->size);
> > +    if (r->addr != PCI_BAR_UNMAPPED) {
> > +        memory_region_add_subregion_overlap(r->address_space,
> > +                                            r->addr, r->memory,
> > 1);
> > +    }
> > +}
> > +
> > +static PCIDevice *register_vf(PCIDevice *pf, int devfn, const char
> > *name)
> > +{
> > +    PCIDevice *dev = pci_create(pf->bus, devfn, name);
> > +    dev->exp.sriov_vf.pf = pf;
> > +    Error *local_err = NULL;
> > +
> > +    object_property_set_bool(OBJECT(&dev->qdev), true, "realized",
> > &local_err);
> > +    if (local_err) {
> > +        error_report_err(local_err);
> > +        return NULL;
> > +    }
> > +
> > +    /* set vid/did according to sr/iov spec - they are not used */
> > +    pci_config_set_vendor_id(dev->config, 0xffff);
> > +    pci_config_set_device_id(dev->config, 0xffff);
> > +    return dev;
> > +}
> > +
> > +static void register_vfs(PCIDevice *dev)
> > +{
> > +    uint16_t num_vfs;
> > +    uint16_t i;
> > +    uint16_t sriov_cap = dev->exp.sriov_cap;
> > +    uint16_t vf_offset = pci_get_word(dev->config + sriov_cap +
> > PCI_SRIOV_VF_OFFSET);
> > +    uint16_t vf_stride = pci_get_word(dev->config + sriov_cap +
> > PCI_SRIOV_VF_STRIDE);
> > +    int32_t devfn = dev->devfn + vf_offset;
> > +
> > +    assert(sriov_cap > 0);
> > +    num_vfs = pci_get_word(dev->config + sriov_cap +
> > PCI_SRIOV_NUM_VF);
> > +
> > +    dev->exp.sriov_pf.vf = g_malloc(sizeof(PCIDevice *) *
> > num_vfs);
> > +    assert(dev->exp.sriov_pf.vf);
> > +
> > +    trace_sriov_register_vfs(SRIOV_ID(dev), num_vfs);
> > +    for (i = 0; i < num_vfs; i++) {
> > +        dev->exp.sriov_pf.vf[i] = register_vf(dev, devfn, dev
> > ->exp.sriov_pf.vfname);
> > +        if (!dev->exp.sriov_pf.vf[i]) {
> > +            num_vfs = i;
> > +            break;
> > +        }
> > +        devfn += vf_stride;
> > +    }
> > +    dev->exp.sriov_pf.num_vfs = num_vfs;
> > +}
> > +
> > +static void unregister_vfs(PCIDevice *dev)
> > +{
> > +    Error *local_err = NULL;
> > +    uint16_t num_vfs = dev->exp.sriov_pf.num_vfs;
> > +    uint16_t i;
> > +
> > +    trace_sriov_unregister_vfs(SRIOV_ID(dev), num_vfs);
> > +    for (i = 0; i < num_vfs; i++) {
> > +        object_property_set_bool(OBJECT(&dev->exp.sriov_pf.vf[i]
> > ->qdev), false, "realized", &local_err);
> > +        if (local_err) {
> > +            fprintf(stderr, "Failed to unplug: %s\n",
> > +                    error_get_pretty(local_err));
> > +            error_free(local_err);
> > +        }
> > +    }
> > +    g_free(dev->exp.sriov_pf.vf);
> > +    dev->exp.sriov_pf.vf = NULL;
> > +    dev->exp.sriov_pf.num_vfs = 0;
> > +    pci_set_word(dev->config + dev->exp.sriov_cap +
> > PCI_SRIOV_NUM_VF, 0);
> > +}
> > +
> > +void pcie_sriov_config_write(PCIDevice *dev, uint32_t address,
> > uint32_t val, int len)
> > +{
> > +    uint32_t off;
> > +    uint16_t sriov_cap = dev->exp.sriov_cap;
> > +
> > +    if (!sriov_cap || address < sriov_cap) {
> > +        return;
> > +    }
> > +    off = address - sriov_cap;
> > +    if (off >= PCI_EXT_CAP_SRIOV_SIZEOF) {
> > +        return;
> > +    }
> > +
> > +    trace_sriov_config_write(SRIOV_ID(dev), off, val, len);
> > +
> > +    if (range_covers_byte(off, len, PCI_SRIOV_CTRL)) {
> > +        if (dev->exp.sriov_pf.num_vfs) {
> > +            if (!(val & PCI_SRIOV_CTRL_VFE)) {
> > +                unregister_vfs(dev);
> > +            }
> > +        } else {
> > +            if (val & PCI_SRIOV_CTRL_VFE) {
> > +                register_vfs(dev);
> > +            }
> > +        }
> > +    }
> > +}
> > +
> > +
> > +/* Reset SR/IOV VF Enable bit to trigger an unregister of all VFs
> > */
> > +void pcie_sriov_pf_disable_vfs(PCIDevice *dev)
> > +{
> > +    uint16_t sriov_cap = dev->exp.sriov_cap;
> > +    if (sriov_cap) {
> > +        uint32_t val = pci_get_byte(dev->config + sriov_cap +
> > PCI_SRIOV_CTRL);
> > +        if (val & PCI_SRIOV_CTRL_VFE) {
> > +            val &= ~PCI_SRIOV_CTRL_VFE;
> > +            pcie_sriov_config_write(dev, sriov_cap +
> > PCI_SRIOV_CTRL, val, 1);
> > +        }
> > +    }
> > +}
> > +
> > +/* Add optional supported page sizes to the mask of supported page
> > sizes */
> > +void pcie_sriov_pf_add_sup_pgsize(PCIDevice *dev, uint16_t
> > opt_sup_pgsize)
> > +{
> > +    uint8_t *cfg = dev->config + dev->exp.sriov_cap;
> > +    uint8_t *wmask = dev->wmask + dev->exp.sriov_cap;
> > +
> > +    uint16_t sup_pgsize = pci_get_word(cfg +
> > PCI_SRIOV_SUP_PGSIZE);
> > +
> > +    sup_pgsize |= opt_sup_pgsize;
> > +
> > +    /* Make sure the new bits are set, and that system page size
> > +     * also can be set to any of the new values according to spec:
> > +     */
> > +    pci_set_word(cfg + PCI_SRIOV_SUP_PGSIZE, sup_pgsize);
> > +    pci_set_word(wmask + PCI_SRIOV_SYS_PGSIZE, sup_pgsize);
> > +}
> > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > index 551cb3d..2e9d8ba 100644
> > --- a/include/hw/pci/pci.h
> > +++ b/include/hw/pci/pci.h
> > @@ -11,8 +11,6 @@
> >   /* PCI includes legacy ISA access.  */
> >   #include "hw/isa/isa.h"
> > 
> > -#include "hw/pci/pcie.h"
> > -
> >   /* PCI bus */
> > 
> >   #define PCI_DEVFN(slot, func)   ((((slot) & 0x1f) << 3) | ((func)
> > & 0x07))
> > @@ -132,6 +130,7 @@ enum {
> >   #define QEMU_PCI_VGA_IO_HI_SIZE 0x20
> > 
> >   #include "hw/pci/pci_regs.h"
> > +#include "hw/pci/pcie.h"
> > 
> >   /* PCI HEADER_TYPE */
> >   #define  PCI_HEADER_TYPE_MULTI_FUNCTION 0x80
> > @@ -421,6 +420,9 @@ typedef AddressSpace *(*PCIIOMMUFunc)(PCIBus *,
> > void *, int);
> >   AddressSpace *pci_device_iommu_address_space(PCIDevice *dev);
> >   void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque);
> > 
> > +pcibus_t pci_bar_address(PCIDevice *d,
> > +                         int reg, uint8_t type, pcibus_t size);
> > +
> >   static inline void
> >   pci_set_byte(uint8_t *config, uint8_t val)
> >   {
> > @@ -672,6 +674,11 @@ static inline int pci_is_express(const
> > PCIDevice *d)
> >       return d->cap_present & QEMU_PCI_CAP_EXPRESS;
> >   }
> > 
> > +static inline int pci_is_vf(const PCIDevice *d)
> > +{
> > +    return d->exp.sriov_vf.pf != NULL;
> > +}
> > +
> >   static inline uint32_t pci_config_size(const PCIDevice *d)
> >   {
> >       return pci_is_express(d) ? PCIE_CONFIG_SPACE_SIZE :
> > PCI_CONFIG_SPACE_SIZE;
> > diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
> > index b48a7a2..b09f79a 100644
> > --- a/include/hw/pci/pcie.h
> > +++ b/include/hw/pci/pcie.h
> > @@ -25,6 +25,7 @@
> >   #include "hw/pci/pci_regs.h"
> >   #include "hw/pci/pcie_regs.h"
> >   #include "hw/pci/pcie_aer.h"
> > +#include "hw/pci/pcie_sriov.h"
> >   #include "hw/hotplug.h"
> > 
> >   typedef enum {
> > @@ -74,6 +75,11 @@ struct PCIExpressDevice {
> >       /* AER */
> >       uint16_t aer_cap;
> >       PCIEAERLog aer_log;
> > +
> > +    /* SR/IOV */
> > +    uint16_t sriov_cap;
> > +    PCIESriovPF sriov_pf;
> > +    PCIESriovVF sriov_vf;
> >   };
> > 
> >   #define COMPAT_PROP_PCP "power_controller_present"
> > diff --git a/include/hw/pci/pcie_sriov.h
> > b/include/hw/pci/pcie_sriov.h
> > new file mode 100644
> > index 0000000..71c2b00
> > --- /dev/null
> > +++ b/include/hw/pci/pcie_sriov.h
> > @@ -0,0 +1,58 @@
> > +/*
> > + * pcie_sriov.h:
> > + *
> > + * Implementation of SR/IOV emulation support.
> > + *
> > + * Copyright (c) 2015 Knut Omang <address@hidden>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2
> > or later.
> > + * See the COPYING file in the top-level directory.
> > + *
> > + */
> > +
> > +#ifndef QEMU_PCIE_SRIOV_H
> > +#define QEMU_PCIE_SRIOV_H
> > +
> > +struct PCIESriovPF {
> > +    uint16_t num_vfs;           /* Number of virtual functions
> > created */
> > +    uint8_t vf_bar_type[PCI_NUM_REGIONS];  /* Store type for each
> > VF bar */
> > +    const char *vfname;         /* Reference to the device type
> > used for the VFs */
> > +    PCIDevice **vf;             /* Pointer to an array of num_vfs
> > VF devices */
> > +};
> > +
> > +struct PCIESriovVF {
> > +    PCIDevice *pf;              /* Pointer back to owner physical
> > function */
> > +};
> 
> 
> Regarding naming conventions, maybe you it should be PCIESRIOVPF o Ir
> PCIESRIOVPf,
> but this would not justify another version (in my opinion)
> 
> > +
> > +/* Optionally add supported page sizes to the mask of supported
> > page sizes
> > + * Page size values are interpreted as opt_sup_pgsize << 12.
> > + */
> > +void pcie_sriov_pf_add_sup_pgsize(PCIDevice *dev, uint16_t
> > opt_sup_pgsize);
> > +
> > +/* SR/IOV capability config write handler */
> > +void pcie_sriov_config_write(PCIDevice *dev, uint32_t address,
> > +                             uint32_t val, int len);
> > +
> > +/* Reset SR/IOV VF Enable bit to unregister all VFs */
> > +void pcie_sriov_pf_disable_vfs(PCIDevice *dev);
> > +
> > +#endif /* QEMU_PCIE_SRIOV_H */
> > diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> > index ee1ce1d..0b89316 100644
> > --- a/include/qemu/typedefs.h
> > +++ b/include/qemu/typedefs.h
> > @@ -54,6 +54,8 @@ typedef struct PCIDevice PCIDevice;
> >   typedef struct PCIEAERErr PCIEAERErr;
> >   typedef struct PCIEAERLog PCIEAERLog;
> >   typedef struct PCIEAERMsg PCIEAERMsg;
> > +typedef struct PCIESriovPF PCIESriovPF;
> > +typedef struct PCIESriovVF PCIESriovVF;
> >   typedef struct PCIEPort PCIEPort;
> >   typedef struct PCIESlot PCIESlot;
> >   typedef struct PCIExpressDevice PCIExpressDevice;
> > diff --git a/trace-events b/trace-events
> > index a0ddc6b..fd51c5f 100644
> > --- a/trace-events
> > +++ b/trace-events
> > @@ -1531,6 +1531,11 @@ xen_pv_mmio_write(uint64_t addr) "WARNING:
> > write to Xen PV Device MMIO space (ad
> >   pci_cfg_read(const char *dev, unsigned devid, unsigned fnid,
> > unsigned offs, unsigned val) "%s %02u:%u @0x%x -> 0x%x"
> >   pci_cfg_write(const char *dev, unsigned devid, unsigned fnid,
> > unsigned offs, unsigned val) "%s %02u:%u @0x%x <- 0x%x"
> > 
> > +# hw/pci/pcie_sriov.c
> > +sriov_register_vfs(const char *name, int slot, int function, int
> > num_vfs) "%s %02x:%x: creating %d vf devs"
> > +sriov_unregister_vfs(const char *name, int slot, int function, int
> > num_vfs) "%s %02x:%x: Unregistering %d vf devs"
> > +sriov_config_write(const char *name, int slot, int fun, uint32_t
> > offset, uint32_t val, uint32_t len) "%s %02x:%x: sriov offset 0x%x
> > val 0x%x len %d"
> > +
> >   # hw/vfio/pci.c
> >   vfio_intx_interrupt(const char *name, char line) " (%s) Pin %c"
> >   vfio_intx_eoi(const char *name) " (%s) EOI"
> > 
> 
> Thanks for all the work, besides the questions above, from my point
> of view:
> 
> 
> Reviewed-by: Marcel Apfelbaum <address@hidden>
> 
> Thanks,
> Marcel



reply via email to

[Prev in Thread] Current Thread [Next in Thread]