On Fri, Aug 29, 2014 at 09:17:07AM +0200, Knut Omang wrote:
> This patch provides the building blocks for creating an SR/IOV
> PCIe Extended Capability header and creating and removing
> SR/IOV Virtual Functions.
>
> Signed-off-by: Knut Omang <knut.om...@oracle.com>
> ---
> hw/pci/pci.c | 107 +++++++++++++++++++-------
> hw/pci/pcie.c | 205
> +++++++++++++++++++++++++++++++++++++++++++++++++-
> include/hw/pci/pci.h | 6 +-
> include/hw/pci/pcie.h | 26 +++++++
> 4 files changed, 311 insertions(+), 33 deletions(-)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index daeaeac..071ab81 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -35,7 +35,6 @@
> #include "hw/pci/msi.h"
> #include "hw/pci/msix.h"
> #include "exec/address-spaces.h"
> -#include "hw/hotplug.h"
>
> //#define DEBUG_PCI
> #ifdef DEBUG_PCI
> @@ -126,6 +125,9 @@ static int pci_bar(PCIDevice *d, int reg)
> {
> uint8_t type;
>
> + /* PCIe virtual functions do not have their own BARs */
> + assert(!d->exp.is_vf);
> +
> if (reg != PCI_ROM_SLOT)
> return PCI_BASE_ADDRESS_0 + reg * 4;
>
> @@ -184,22 +186,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 (dev->exp.is_vf) {
> + 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) {
> @@ -213,6 +206,27 @@ static void pci_do_device_reset(PCIDevice *dev)
> pci_set_long(dev->config + pci_bar(dev, r), region->type);
> }
> }
> +}
> +
> +static void pci_do_device_reset(PCIDevice *dev)
> +{
> + qdev_reset_all(&dev->qdev);
> +
> + dev->irq_state = 0;
> + pci_update_irq_status(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);
> @@ -734,6 +748,14 @@ static int pci_init_multifunction(PCIBus *bus, PCIDevice
> *dev)
> dev->config[PCI_HEADER_TYPE] |= PCI_HEADER_TYPE_MULTI_FUNCTION;
> }
>
> + /* With SR/IOV and ARI, subsequent function 0's are only
> + * another VF which the physical function is placed in the initial
> + * function (0.0)
Couldn't parse this. Could you please reword?
> + */
> + if (dev->exp.pf && dev->exp.pf->cap_present &
> QEMU_PCI_CAP_MULTIFUNCTION) {
> + return 0;
> + }
> +
> /*
> * multifunction bit is interpreted in two ways as follows.
> * - all functions must set the bit to 1.
> @@ -920,6 +942,7 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
> uint64_t wmask;
> pcibus_t size = memory_region_size(memory);
>
> + assert(!pci_dev->exp.is_vf); /* VFs must use pcie_register_vf_bar */
> assert(region_num >= 0);
> assert(region_num < PCI_NUM_REGIONS);
> if (size & (size-1)) {
> @@ -1018,18 +1041,51 @@ 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 (!d->exp.is_vf) {
> + 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 {
> + int bar = d->exp.pf->exp.sriov_cap + PCI_SRIOV_BAR + reg * 4;
> + uint32_t vf_num = d->devfn - (d->exp.pf->devfn + 1);
> + if (type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> + new_addr = pci_get_quad(d->exp.pf->config + bar);
> + } else {
> + new_addr = pci_get_long(d->exp.pf->config + bar);
> + }
> + new_addr += vf_num * size;
> +
> + PCI_DPRINTF("%02d:%02d.%d: (vf %d) found config reg %d, "
> + "size 0x%lx addr 0x%lx\n",
> + pci_bus_num(d->bus), d->devfn >> 3, d->devfn & 7,
> + vf_num, reg, size, new_addr);
I don't think this is needed, assuming it is, don't we want this
for all functions, bot just vfs?
> + }
> + 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);
>
> if (type & PCI_BASE_ADDRESS_SPACE_IO) {
> 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);
Hmm this is IO, VFs don't have IO, do they? Change really necessary?
> last_addr = new_addr + size - 1;
> /* Check if 32 bit BAR wraps around explicitly.
> * TODO: make priorities correct and remove this work around.
> @@ -1043,11 +1099,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;
> @@ -1146,9 +1198,10 @@ uint32_t pci_default_read_config(PCIDevice *d,
> return le32_to_cpu(val);
> }
>
> -void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int
> l)
> +void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val_in,
> int l)
> {
> int i, was_irq_disabled = pci_irq_disabled(d);
> + uint32_t val = val_in;
>
> for (i = 0; i < l; val >>= 8, ++i) {
> uint8_t wmask = d->wmask[addr + i];
> @@ -1170,8 +1223,9 @@ void pci_default_write_config(PCIDevice *d, uint32_t
> addr, uint32_t val, int l)
> & PCI_COMMAND_MASTER);
> }
>
> - msi_write_config(d, addr, val, l);
> - msix_write_config(d, addr, val, l);
> + 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);
> }
Not sure I get this chunk.
A code comment might be helpful.
>
> /***********************************************************/
> @@ -1776,7 +1830,6 @@ static int pci_qdev_init(DeviceState *qdev)
> is_default_rom = true;
> }
> pci_add_option_rom(pci_dev, is_default_rom);
> -
> return 0;
> }
>
generally, don't make unrelated changes, review is
hard enough.
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 6cb6e0c..b7af693 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -254,7 +254,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_dev->exp.is_vf);
>
> pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
> PCI_EXP_SLTSTA_PDS);
> @@ -266,10 +266,11 @@ void pcie_cap_slot_hot_unplug_cb(HotplugHandler
> *hotplug_dev, DeviceState *dev,
> Error **errp)
> {
> uint8_t *exp_cap;
> + PCIDevice *pdev = PCI_DEVICE(hotplug_dev);
>
> - pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap,
> errp);
> + pcie_cap_slot_hotplug_common(pdev, dev, &exp_cap, errp);
>
> - pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev));
> + pcie_cap_slot_push_attention_button(pdev);
> }
>
> /* pci express slot for pci express root/downstream port
> @@ -409,7 +410,7 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
> }
>
> /*
> - * If the slot is polulated, power indicator is off and power
> + * If the slot is populated, power indicator is off and power
> * controller is off, it is safe to detach the devices.
> */
> if ((sltsta & PCI_EXP_SLTSTA_PDS) && (val & PCI_EXP_SLTCTL_PCC) &&
> @@ -633,3 +634,199 @@ void pcie_ari_init(PCIDevice *dev, uint16_t offset,
> uint16_t nextfn)
> offset, PCI_ARI_SIZEOF);
> pci_set_long(dev->config + offset + PCI_ARI_CAP, (nextfn & 0xff) << 8);
> }
> +
> +
> +
don't add > 1 empty line in a row.
> +/* SR/IOV */
this comment is really useless, function name makes it clear.
> +void pcie_sriov_init(PCIDevice *dev, uint16_t offset,
> + const char *vfname, uint16_t vf_dev_id,
> + uint16_t init_vfs, uint16_t total_vfs)
> +{
> + uint8_t *cfg = dev->config + offset;
> + uint8_t *wmask;
empty line here after variables.
> + pcie_add_capability(dev, PCI_EXT_CAP_ID_SRIOV, 1,
> + offset, PCI_EXT_CAP_SRIOV_SIZEOF);
> + dev->exp.sriov_cap = offset;
> + dev->exp.num_vfs = 0;
> + dev->exp.vfname = g_strdup(vfname);
I don't see a free for this field anywhere.
> + dev->exp.vf = NULL;
> +
> + /* set some sensible defaults - devices can override later */
do they in fact override?
> + pci_set_word(cfg + PCI_SRIOV_VF_OFFSET, 0x1);
> + pci_set_word(cfg + PCI_SRIOV_VF_STRIDE, 0x1);
> + pci_set_word(cfg + PCI_SRIOV_SUP_PGSIZE, 0x553);
what's this magic constant?
needs a comment.
> + 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);
> +
> + /* 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);
> +
> + qdev_prop_set_bit(&dev->qdev, "multifunction", true);
> +}
> +
> +
2 empty lines again
> +void pcie_sriov_exit(PCIDevice *dev)
> +{
> + PCIE_DPRINTF("\n");
> + pcie_sriov_reset_vfs(dev);
> +}
> +
> +void pcie_sriov_init_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.vf_bar_type[region_num] = type;
> +}
> +
> +void pcie_register_vf_bar(PCIDevice *dev, int region_num,
> + MemoryRegion *memory)
> +{
> + PCIIORegion *r;
> + uint8_t type;
> + pcibus_t size = memory_region_size(memory);
> +
> + assert(dev->exp.is_vf); /* PFs must use pci_register_bar */
> + assert(region_num >= 0);
> + assert(region_num < PCI_NUM_REGIONS);
> + type = dev->exp.pf->exp.vf_bar_type[region_num];
> +
> + if (size & (size-1)) {
> + fprintf(stderr, "ERROR: PCI region size must be pow2 "
power of 2
eschew abbreviation
> + "type=0x%x, size=0x%"FMT_PCIBUS"\n", 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 *pcie_create_vf(PCIDevice *pf, int devfn, const char *name)
> +{
> + int ret;
> + PCIDevice *dev = pci_create(pf->bus, devfn, name);
> + dev->exp.is_vf = true;
> + dev->exp.pf = pf;
> +
> + ret = qdev_init(&dev->qdev);
> + if (ret)
> + 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;
> +}
> +
> +
> +void pcie_sriov_create_vfs(PCIDevice *dev)
> +{
> + uint16_t num_vfs;
> + uint16_t i;
> + int32_t devfn = dev->devfn + 1;
> + uint16_t sriov_cap = dev->exp.sriov_cap;
> +
> + assert(sriov_cap > 0);
> + num_vfs = pci_get_word(dev->config + sriov_cap + PCI_SRIOV_NUM_VF);
> +
> + dev->exp.vf = g_malloc(sizeof(PCIDevice*) * num_vfs);
Don't see a free to match this malloc call.
> + assert(dev->exp.vf);
> +
> + PCIE_DEV_PRINTF(dev, "creating %d vf devs\n", num_vfs);
> + for (i = 0; i < num_vfs; i++) {
> + dev->exp.vf[i] = pcie_create_vf(dev, devfn++, dev->exp.vfname);
> + if (!dev->exp.vf[i]) {
> + PCIE_DEV_PRINTF(dev, "Failed to create VF %d\n", i);
> + num_vfs = i;
> + break;
> + }
> + }
> + dev->exp.num_vfs = num_vfs;
> +}
> +
> +
> +void pcie_sriov_reset_vfs(PCIDevice *dev)
> +{
> + Error *local_err = NULL;
> + uint16_t num_vfs = dev->exp.num_vfs;
> + uint16_t i;
> + PCIE_DEV_PRINTF(dev, "Resetting %d vf devs\n", num_vfs);
> + for (i = 0; i < num_vfs; i++) {
> + qdev_unplug(&dev->exp.vf[i]->qdev, &local_err);
> + if (local_err) {
> + fprintf(stderr, "Failed to unplug: %s\n",
> + error_get_pretty(local_err));
> + error_free(local_err);
> + }
> + }
> + dev->exp.num_vfs = 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;
> + }
> +
> + PCIE_DEV_PRINTF(dev, "cap at 0x%x sriov offset 0x%x val 0x%x len %d\n",
> + sriov_cap, off, val, len);
> +
> + if (range_covers_byte(off, len, PCI_SRIOV_CTRL)) {
> + if (dev->exp.num_vfs) {
> + if (!(val & PCI_SRIOV_CTRL_VFE)) {
> + pcie_sriov_reset_vfs(dev);
> + }
> + } else {
> + if (val & PCI_SRIOV_CTRL_VFE) {
> + pcie_sriov_create_vfs(dev);
> + }
> + }
> + }
> +}
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index c352c7b..d36c68d 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))
> @@ -127,6 +125,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
why move it here?
> @@ -413,6 +412,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)
> {
> diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
> index d139d58..b34d831 100644
> --- a/include/hw/pci/pcie.h
> +++ b/include/hw/pci/pcie.h
> @@ -74,6 +74,15 @@ struct PCIExpressDevice {
> /* AER */
> uint16_t aer_cap;
> PCIEAERLog aer_log;
> +
> + /* SR/IOV */
> + uint16_t sriov_cap;
> + uint16_t num_vfs; /* Number of virtual functions created */
> + bool is_vf; /* Set if this device is a virtual function
> */
Checking pf pointer not enough?
> + const char *vfname; /* Reference to the device type used for the
> VFs */
> + PCIDevice **vf; /* Pointer to an array of num_vfs VF devices
> */
> + PCIDevice *pf; /* Pointer back to owner physical function */
> + uint8_t vf_bar_type[PCI_NUM_REGIONS]; /* Store type for each VF bar */
There are two types of fields here: some are per-vf,
some are sriov fields in pf.
Pls make this clearer:
- create structures for each group
- name fields sriov_pf and sriov_vf accordingly.
> };
>
> #define COMPAT_PROP_PCP "power_controller_present"
> @@ -115,6 +124,23 @@ void pcie_add_capability(PCIDevice *dev,
> uint16_t offset, uint16_t size);
>
> void pcie_ari_init(PCIDevice *dev, uint16_t offset, uint16_t nextfn);
> +void pcie_sriov_init(PCIDevice *dev, uint16_t offset,
> + const char *vfname, uint16_t vf_dev_id,
> + uint16_t init_vfs, uint16_t total_vfs);
> +void pcie_sriov_exit(PCIDevice *dev);
> +
Assumption is, this applies to pf only?
> +/* Set up a VF bar in the SR/IOV bar area */
> +void pcie_sriov_init_bar(PCIDevice *s, int region_num,
> + uint8_t type, dma_addr_t size);
> +
> +/* Instantiate a bar for a VF */
> +void pcie_register_vf_bar(PCIDevice *pci_dev, int region_num,
> + MemoryRegion *memory);
> +
Again, this isn't very clear. prefix everything with
pcie_sriov_pf_ and pcie_sriov_vf_ to make it clear
what each applies to.
> +void pcie_sriov_create_vfs(PCIDevice *dev);
Do we need to export this one?
> +void pcie_sriov_reset_vfs(PCIDevice *dev);
How about pcie_sriov_reset? Callers don't need to know
what you do internally I think.
> +void pcie_sriov_config_write(PCIDevice *dev, uint32_t address,
> + uint32_t val, int len);
>
> extern const VMStateDescription vmstate_pcie_device;
I'm not sure which functions apply to pfs,vfs, and which to both
pfs and vfs. function names should make this clear.
>
> --
> 1.9.0
