[Qemu-devel] [PATCH 1/1] Add support for PCI Enhanced Allocation "BARs"

[Qemu-devel] [PATCH 1/1] Add support for PCI Enhanced Allocation "BARs"

[Sean O. Stalley]

PCI Enhanced Allocation is a new method of allocating MMIO & IO
resources for PCI devices & bridges. It can be used instead
of the traditional PCI method of using BARs.

EA entries are hardware-initialized to a fixed address.
Unlike BARs, regions described by EA are cannot be moved.
Because of this, only devices which are perminately connected to
the PCI bus can use EA. A removeable PCI card must not use EA.

This patchset enables any existing QEMU PCI model to use EA in leiu of
BARs. It adds EA options to the PCI device paramaters.

The Enhanced Allocation ECN is publicly available here:
https://www.pcisig.com/specifications/conventional/ECN_Enhanced_Allocation_23_Oct_2014_Final.pdf

Signed-off-by: Sean O. Stalley <sean.stalley@intel.com>
---
 hw/pci/Makefile.objs      |   2 +-
 hw/pci/pci.c              |  96 ++++++++++++++++------
 hw/pci/pci_ea.c           | 203 ++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/pci/pci.h      |   7 ++
 include/hw/pci/pci_ea.h   |  39 +++++++++
 include/hw/pci/pci_regs.h |   4 +
 include/qemu/typedefs.h   |   1 +
 7 files changed, 328 insertions(+), 24 deletions(-)
 create mode 100644 hw/pci/pci_ea.c
 create mode 100644 include/hw/pci/pci_ea.h

diff --git a/hw/pci/Makefile.objs b/hw/pci/Makefile.objs
index 9f905e6..e5d80cf 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 pci_ea.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 b3d5100..c7552ca 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -23,6 +23,7 @@
  */
 #include "hw/hw.h"
 #include "hw/pci/pci.h"
+#include "hw/pci/pci_ea.h"
 #include "hw/pci/pci_bridge.h"
 #include "hw/pci/pci_bus.h"
 #include "hw/pci/pci_host.h"
@@ -58,6 +59,10 @@ static Property pci_props[] = {
                     QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false),
     DEFINE_PROP_BIT("command_serr_enable", PCIDevice, cap_present,
                     QEMU_PCI_CAP_SERR_BITNR, true),
+    DEFINE_PROP_BOOL("enhanced_allocation", PCIDevice, enhanced, false),
+    DEFINE_PROP_ARRAY("ea_addr", PCIDevice, ea_addr_len, ea_addr,
+                      qdev_prop_uint64, hwaddr),
+
     DEFINE_PROP_END_OF_LIST()
 };
 
@@ -882,6 +887,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
         config_read = pci_default_read_config;
     if (!config_write)
         config_write = pci_default_write_config;
+
     pci_dev->config_read = config_read;
     pci_dev->config_write = config_write;
     bus->devices[devfn] = pci_dev;
@@ -929,40 +935,72 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
 
     assert(region_num >= 0);
     assert(region_num < PCI_NUM_REGIONS);
-    if (size & (size-1)) {
+
+
+    /* TODO: these checks should be done earlier */
+    if (!pci_dev->enhanced && (size & (size-1))) {
         fprintf(stderr, "ERROR: PCI region size must be pow2 "
                     "type=0x%x, size=0x%"FMT_PCIBUS"\n", type, size);
         exit(1);
     }
 
+    if (pci_dev->enhanced) {
+
+        if (pci_dev->ea_addr_len <= region_num) {
+            fprintf(stderr, "ERROR: Address for EA entry %i not given\n",
+                    region_num);
+            exit(1);
+        }
+
+        if (pci_dev->ea_addr[region_num] == 0x0) {
+            fprintf(stderr, "ERROR: Address for EA entry %i not set\n",
+                    region_num);
+            exit(1);
+        }
+
+        if (pci_ea_invalid_addr(pci_dev->ea_addr[region_num])) {
+            fprintf(stderr, "ERROR: Address for EA entry %i not valid\n",
+                    region_num);
+            exit(1);
+        }
+    }
+
     r = &pci_dev->io_regions[region_num];
     r->addr = PCI_BAR_UNMAPPED;
     r->size = size;
     r->type = type;
-    r->memory = NULL;
-
-    wmask = ~(size - 1);
-    addr = pci_bar(pci_dev, region_num);
-    if (region_num == PCI_ROM_SLOT) {
-        /* ROM enable bit is writable */
-        wmask |= PCI_ROM_ADDRESS_ENABLE;
-    }
-    pci_set_long(pci_dev->config + addr, type);
-    if (!(r->type & PCI_BASE_ADDRESS_SPACE_IO) &&
-        r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
-        pci_set_quad(pci_dev->wmask + addr, wmask);
-        pci_set_quad(pci_dev->cmask + addr, ~0ULL);
-    } else {
-        pci_set_long(pci_dev->wmask + addr, wmask & 0xffffffff);
-        pci_set_long(pci_dev->cmask + addr, 0xffffffff);
+    r->enhanced = pci_dev->enhanced;
+    r->memory = memory;
+    r->address_space = type & PCI_BASE_ADDRESS_SPACE_IO ?
+        pci_dev->bus->address_space_io : pci_dev->bus->address_space_mem;
+
+    if (!pci_dev->enhanced) {
+
+        wmask = ~(size - 1);
+        addr = pci_bar(pci_dev, region_num);
+        if (region_num == PCI_ROM_SLOT) {
+            /* ROM enable bit is writable */
+            wmask |= PCI_ROM_ADDRESS_ENABLE;
+        }
+        pci_set_long(pci_dev->config + addr, type);
+        if (!(r->type & PCI_BASE_ADDRESS_SPACE_IO) &&
+            r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
+            pci_set_quad(pci_dev->wmask + addr, wmask);
+            pci_set_quad(pci_dev->cmask + addr, ~0ULL);
+        } else {
+            pci_set_long(pci_dev->wmask + addr, wmask & 0xffffffff);
+            pci_set_long(pci_dev->cmask + addr, 0xffffffff);
+        }
+    } else { /* enhanced */
+
+        /* add the memory space now */
+        r->addr = pci_dev->ea_addr[region_num];
+        memory_region_add_subregion(r->address_space, r->addr, r->memory);
     }
-    pci_dev->io_regions[region_num].memory = memory;
-    pci_dev->io_regions[region_num].address_space
-        = type & PCI_BASE_ADDRESS_SPACE_IO
-        ? pci_dev->bus->address_space_io
-        : pci_dev->bus->address_space_mem;
 }
 
+
+
 static void pci_update_vga(PCIDevice *pci_dev)
 {
     uint16_t cmd;
@@ -1107,6 +1145,11 @@ static void pci_update_mappings(PCIDevice *d)
 
         new_addr = pci_bar_address(d, i, r->type, r->size);
 
+        /* EA BARs cannot be moved */
+        if (r->enhanced) {
+            continue;
+        }
+
         /* This bar isn't changed */
         if (new_addr == r->addr)
             continue;
@@ -1785,8 +1828,9 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
     pci_dev = do_pci_register_device(pci_dev, bus,
                                      object_get_typename(OBJECT(qdev)),
                                      pci_dev->devfn, errp);
-    if (pci_dev == NULL)
+    if (pci_dev == NULL) {
         return;
+    }
 
     if (pc->realize) {
         pc->realize(pci_dev, &local_err);
@@ -1797,6 +1841,12 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
         }
     }
 
+    /* write the EA entry */
+    if (pci_dev->enhanced) {
+        pci_ea_cap_init(pci_dev);
+    }
+
+
     /* rom loading */
     is_default_rom = false;
     if (pci_dev->romfile == NULL && pc->romfile != NULL) {
diff --git a/hw/pci/pci_ea.c b/hw/pci/pci_ea.c
new file mode 100644
index 0000000..a052519
--- /dev/null
+++ b/hw/pci/pci_ea.c
@@ -0,0 +1,203 @@
+/*
+ * PCI Enhanced Allocation (EA) Helper functions
+ * Copyright (c) 2015, Intel Corporation.
+ *
+ * Written by Sean O. Stalley <sean.stalley@intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+/*
+ * Contians a lot of code taken from pcie.c
+ */
+
+#include "qemu-common.h"
+#include "hw/pci/pci.h"
+#include "hw/pci/pci_ea.h"
+
+bool pci_ea_invalid_addr(uint64_t addr)
+{
+    return (addr != (addr & PCI_EA_ADDR_MASK_64));
+}
+
+/* determines if a 64 bit address is necessary for this address */
+static bool pci_ea_addr_is_64(uint64_t addr)
+{
+    return (addr != (uint32_t)addr);
+}
+
+/* Sets the first 32 bits of the Base or MaxOffset field
+ * Returns the length of the address set
+ */
+static uint8_t pci_ea_set_addr(PCIDevice *dev, uint8_t pos, uint64_t val)
+{
+    uint8_t *current_reg = dev->config + pos;
+
+    val = val & PCI_EA_ADDR_MASK_64;
+
+    if (pci_ea_addr_is_64(val)) {
+        val |= PCI_EA_FIELD_SIZE_FLAG;
+    }
+
+    pci_set_long(current_reg, val);
+    return sizeof(int32_t);
+}
+
+/* Determines EA entry length for this IORegion in bytes
+ * (including the first DWORD) */
+static int pci_ea_entry_length(PCIIORegion *r)
+{
+
+    int len = 3 * sizeof(int32_t); /* First 32 bits of Base & Offset */
+
+    if (pci_ea_addr_is_64(r->addr)) {
+        len += sizeof(int32_t); /* Base (if 64 bit) */
+    }
+
+    if (pci_ea_addr_is_64(r->size - 1)) {
+        len += sizeof(int32_t); /* Offset (if 64 bit) */
+    }
+
+    return len;
+
+}
+
+/* determines the value to be set in the Entry Size field */
+static int pci_ea_get_es(PCIIORegion *r)
+{
+
+    int es = (pci_ea_entry_length(r) / sizeof(int32_t)) - 1;
+
+    assert(PCI_EA_MAX_ES >= es);
+
+    return es;
+}
+
+/* Initialize an EA entry for the given PCIIORegion
+ *
+ * @dev: The PCI Device
+ * @bei: The BAR Equivalent Indicator
+ * @pos: The offset of this entry in PCI Configspace;
+ */
+
+static int pci_ea_fill_entry(PCIDevice *dev, int bei, int pos)
+{
+    PCIIORegion *r = &dev->io_regions[bei];
+    uint32_t dw0 = 0; /* used for setting first DWORD */
+    int len = 0;
+
+    assert(bei <= PCI_EA_MAX_BEI);
+
+    /* Check that the base and size are DWORD aligned */
+    assert(!pci_ea_invalid_addr(r->addr));
+    assert(!pci_ea_invalid_addr(r->size));
+
+    dw0 |= pci_ea_get_es(r);
+    dw0 |= (bei << PCI_EA_BEI_OFFSET);
+    dw0 |= (PCI_EA_ENABLE);
+
+    /* base Primary Properties off of type field */
+    if (r->type & PCI_BASE_ADDRESS_SPACE_IO) {
+        dw0 |= (PCI_EA_IO << PCI_EA_PP_OFFSET);
+
+    } else if (r->type & PCI_BASE_ADDRESS_MEM_PREFETCH) {
+        dw0 |= (PCI_EA_MEM_PREFETCH << PCI_EA_PP_OFFSET);
+
+    } else {
+        dw0 |= (PCI_EA_MEM << PCI_EA_PP_OFFSET);
+    }
+
+    /* Secondary Properties should be ignored */
+    dw0 |= (PCI_EA_UNAVAL << PCI_EA_SP_OFFSET);
+
+    pci_set_long(dev->config + pos, dw0);
+
+    len += sizeof(int32_t);
+
+    len += pci_ea_set_addr(dev, pos + len, r->addr);
+    len += pci_ea_set_addr(dev, pos + len, r->size - 1);
+
+    /* Base (if 64 bit) */
+    if (pci_ea_addr_is_64(r->addr)) {
+        pci_set_long(dev->config + pos + len, (r->addr >> 32));
+        len += sizeof(int32_t);
+    }
+
+    /* Offset (if 64 bit) */
+    if (pci_ea_addr_is_64(r->size - 4)) {
+        pci_set_long(dev->config + pos + len, (r->size >> 32));
+        len += sizeof(int32_t);
+    }
+
+    assert(len == pci_ea_entry_length(r));
+
+    return len;
+
+}
+
+/*
+ * Initialize the EA capability descriptor
+ * must be done after the EA memory regions are initialized
+ * (or else the EA entries won't get written)
+ */
+int pci_ea_cap_init(PCIDevice *dev)
+{
+    int pos;
+    int ret;
+    int i;
+    int num_entries = 0;
+    uint8_t length; /* length of this cap entry */
+    PCIIORegion *r;
+
+    /* Determine the length of the capability entry */
+
+    length = sizeof(int32_t); /* DWORD 0: Cap ID, Next Pointer, Num Entries */
+    /* TODO: DWORD 1 for Type 1 Functions */
+
+    /* add the length of every entry */
+    for (i = 0; i < PCI_NUM_REGIONS; ++i) {
+        r = &dev->io_regions[i];
+
+        if (r->enhanced) {
+            num_entries++;
+            length += pci_ea_entry_length(r);
+        }
+    }
+
+    /* add the EA CAP (sets DWORD 0) */
+    pos = pci_add_capability(dev, PCI_CAP_ID_EA, 0, length);
+    if (0 > pos) {
+        return pos;
+    }
+
+    /* DWORD 0: Cap ID, Next Pointer, Num Entries */
+    assert(num_entries <= PCI_EA_MAX_NUM_ENTRIES); /* overflow */
+    pci_set_byte(dev->config + pos + PCI_EA_NUM_ENTRIES_OFFSET, num_entries);
+    pos += sizeof(int32_t);
+
+    for (i = 0; i < PCI_NUM_REGIONS; ++i) {
+        r = &dev->io_regions[i];
+
+        if (r->enhanced) {
+            ret = pci_ea_fill_entry(dev, i, pos);
+
+            if (0 > ret) {
+                return ret;
+            }
+            pos += ret;
+        }
+
+    }
+
+    assert(pos == pci_find_capability(dev, PCI_CAP_ID_EA) + length);
+
+    return pos;
+
+}
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index d4ffead..da64435 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -12,6 +12,7 @@
 #include "hw/isa/isa.h"
 
 #include "hw/pci/pcie.h"
+#include "hw/pci/pci_ea.h"
 
 /* PCI bus */
 
@@ -109,6 +110,7 @@ typedef struct PCIIORegion {
     uint8_t type;
     MemoryRegion *memory;
     MemoryRegion *address_space;
+    bool          enhanced;
 } PCIIORegion;
 
 #define PCI_ROM_SLOT 6
@@ -288,6 +290,11 @@ struct PCIDevice {
     /* PCI Express */
     PCIExpressDevice exp;
 
+    /* Enhanced Allocation */
+    bool enhanced;
+    uint32_t ea_addr_len;
+    hwaddr *ea_addr;
+
     /* SHPC */
     SHPCDevice *shpc;
 
diff --git a/include/hw/pci/pci_ea.h b/include/hw/pci/pci_ea.h
new file mode 100644
index 0000000..2b63012
--- /dev/null
+++ b/include/hw/pci/pci_ea.h
@@ -0,0 +1,39 @@
+/* Constants for pci enhanced allocation (EA) capability descriptor */
+
+#ifndef QEMU_PCI_EA_H
+#define QEMU_PCI_EA_H
+
+#define PCI_EA_FIELD_SIZE_FLAG 0x00000002
+
+/* the address' must be DWORD aligned */
+#define PCI_EA_ADDR_MASK_32 0xfffffffc
+#define PCI_EA_ADDR_MASK_64 0xfffffffffffffffc
+
+#define PCI_EA_MAX_NUM_ENTRIES 0x3f
+
+#define PCI_EA_BEI_OFFSET 4 /* In bits from beginning of EA entry */
+#define PCI_EA_ENABLE 0x80000000
+#define PCI_EA_MAX_BEI 0x8
+#define PCI_EA_MAX_ES  0x4
+
+/* Primary & Secondary Properties, per table 6-1 */
+#define PCI_EA_PP_OFFSET 8
+#define PCI_EA_SP_OFFSET 16
+
+/* Primary & Secondary Properties Fields, per table 6-1 */
+#define PCI_EA_MEM               0x00 /* Non-Prefetchable */
+#define PCI_EA_MEM_PREFETCH      0x01
+#define PCI_EA_IO                0x02
+#define PCI_EA_VIRT_MEM_PREFETCH 0x03
+#define PCI_EA_VIRT_MEM          0x04 /* Non-Prefetchable */
+#define PCI_EA_UNAVAL_MEM        0xFD
+#define PCI_EA_UNAVAL_IO         0xFE
+#define PCI_EA_UNAVAL            0xFF
+
+/* checks to see if ea_address is invalid */
+bool pci_ea_invalid_addr(uint64_t addr);
+
+/* Initialize the ea capability descriptor */
+int pci_ea_cap_init(PCIDevice *dev);
+
+#endif /* QEMU_PCI_EA_REG_H */
diff --git a/include/hw/pci/pci_regs.h b/include/hw/pci/pci_regs.h
index 56a404b..746ba1a 100644
--- a/include/hw/pci/pci_regs.h
+++ b/include/hw/pci/pci_regs.h
@@ -213,6 +213,7 @@
 #define  PCI_CAP_ID_MSIX	0x11	/* MSI-X */
 #define  PCI_CAP_ID_SATA	0x12	/* Serial ATA */
 #define  PCI_CAP_ID_AF		0x13	/* PCI Advanced Features */
+#define  PCI_CAP_ID_EA		0x14	/* PCI Enhanced Allocation */
 #define PCI_CAP_LIST_NEXT	1	/* Next capability in the list */
 #define PCI_CAP_FLAGS		2	/* Capability defined flags (16 bits) */
 #define PCI_CAP_SIZEOF		4
@@ -340,6 +341,9 @@
 #define PCI_AF_STATUS		5
 #define  PCI_AF_STATUS_TP	0x01
 
+/* PCI Enhanced Allocation registers */
+#define PCI_EA_NUM_ENTRIES_OFFSET 2
+
 /* PCI-X registers */
 
 #define PCI_X_CMD		2	/* Modes & Features */
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index cde3314..70a9f3f 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -48,6 +48,7 @@ typedef struct PcGuestInfo PcGuestInfo;
 typedef struct PCIBridge PCIBridge;
 typedef struct PCIBus PCIBus;
 typedef struct PCIDevice PCIDevice;
+typedef struct PCIEADevice PCIEADevice;
 typedef struct PCIEAERErr PCIEAERErr;
 typedef struct PCIEAERLog PCIEAERLog;
 typedef struct PCIEAERMsg PCIEAERMsg;
-- 
1.9.1

Re: [Qemu-devel] [PATCH 1/1] Add support for PCI Enhanced Allocation "BARs"

[Michael S. Tsirkin]

On Mon, May 11, 2015 at 11:56:44AM -0700, Sean O. Stalley wrote:
> PCI Enhanced Allocation is a new method of allocating MMIO & IO
> resources for PCI devices & bridges. It can be used instead
> of the traditional PCI method of using BARs.
> 
> EA entries are hardware-initialized to a fixed address.
> Unlike BARs, regions described by EA are cannot be moved.
> Because of this, only devices which are perminately connected to
> the PCI bus can use EA. A removeable PCI card must not use EA.
> 
> This patchset enables any existing QEMU PCI model to use EA in leiu of
> BARs. It adds EA options to the PCI device paramaters.
> 
> The Enhanced Allocation ECN is publicly available here:
> https://www.pcisig.com/specifications/conventional/ECN_Enhanced_Allocation_23_Oct_2014_Final.pdf
> 
> Signed-off-by: Sean O. Stalley <sean.stalley@intel.com>


I will review this, thanks.  Will you also be sending a seabios patch to
support these registers?  When you do, please Cc me.


> ---
>  hw/pci/Makefile.objs      |   2 +-
>  hw/pci/pci.c              |  96 ++++++++++++++++------
>  hw/pci/pci_ea.c           | 203 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/pci/pci.h      |   7 ++
>  include/hw/pci/pci_ea.h   |  39 +++++++++
>  include/hw/pci/pci_regs.h |   4 +
>  include/qemu/typedefs.h   |   1 +
>  7 files changed, 328 insertions(+), 24 deletions(-)
>  create mode 100644 hw/pci/pci_ea.c
>  create mode 100644 include/hw/pci/pci_ea.h
> 
> diff --git a/hw/pci/Makefile.objs b/hw/pci/Makefile.objs
> index 9f905e6..e5d80cf 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 pci_ea.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 b3d5100..c7552ca 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -23,6 +23,7 @@
>   */
>  #include "hw/hw.h"
>  #include "hw/pci/pci.h"
> +#include "hw/pci/pci_ea.h"
>  #include "hw/pci/pci_bridge.h"
>  #include "hw/pci/pci_bus.h"
>  #include "hw/pci/pci_host.h"
> @@ -58,6 +59,10 @@ static Property pci_props[] = {
>                      QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false),
>      DEFINE_PROP_BIT("command_serr_enable", PCIDevice, cap_present,
>                      QEMU_PCI_CAP_SERR_BITNR, true),
> +    DEFINE_PROP_BOOL("enhanced_allocation", PCIDevice, enhanced, false),
> +    DEFINE_PROP_ARRAY("ea_addr", PCIDevice, ea_addr_len, ea_addr,
> +                      qdev_prop_uint64, hwaddr),
> +
>      DEFINE_PROP_END_OF_LIST()
>  };
>  
> @@ -882,6 +887,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>          config_read = pci_default_read_config;
>      if (!config_write)
>          config_write = pci_default_write_config;
> +
>      pci_dev->config_read = config_read;
>      pci_dev->config_write = config_write;
>      bus->devices[devfn] = pci_dev;
> @@ -929,40 +935,72 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
>  
>      assert(region_num >= 0);
>      assert(region_num < PCI_NUM_REGIONS);
> -    if (size & (size-1)) {
> +
> +
> +    /* TODO: these checks should be done earlier */
> +    if (!pci_dev->enhanced && (size & (size-1))) {
>          fprintf(stderr, "ERROR: PCI region size must be pow2 "
>                      "type=0x%x, size=0x%"FMT_PCIBUS"\n", type, size);
>          exit(1);
>      }
>  
> +    if (pci_dev->enhanced) {
> +
> +        if (pci_dev->ea_addr_len <= region_num) {
> +            fprintf(stderr, "ERROR: Address for EA entry %i not given\n",
> +                    region_num);
> +            exit(1);
> +        }
> +
> +        if (pci_dev->ea_addr[region_num] == 0x0) {
> +            fprintf(stderr, "ERROR: Address for EA entry %i not set\n",
> +                    region_num);
> +            exit(1);
> +        }
> +
> +        if (pci_ea_invalid_addr(pci_dev->ea_addr[region_num])) {
> +            fprintf(stderr, "ERROR: Address for EA entry %i not valid\n",
> +                    region_num);
> +            exit(1);
> +        }
> +    }
> +
>      r = &pci_dev->io_regions[region_num];
>      r->addr = PCI_BAR_UNMAPPED;
>      r->size = size;
>      r->type = type;
> -    r->memory = NULL;
> -
> -    wmask = ~(size - 1);
> -    addr = pci_bar(pci_dev, region_num);
> -    if (region_num == PCI_ROM_SLOT) {
> -        /* ROM enable bit is writable */
> -        wmask |= PCI_ROM_ADDRESS_ENABLE;
> -    }
> -    pci_set_long(pci_dev->config + addr, type);
> -    if (!(r->type & PCI_BASE_ADDRESS_SPACE_IO) &&
> -        r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> -        pci_set_quad(pci_dev->wmask + addr, wmask);
> -        pci_set_quad(pci_dev->cmask + addr, ~0ULL);
> -    } else {
> -        pci_set_long(pci_dev->wmask + addr, wmask & 0xffffffff);
> -        pci_set_long(pci_dev->cmask + addr, 0xffffffff);
> +    r->enhanced = pci_dev->enhanced;
> +    r->memory = memory;
> +    r->address_space = type & PCI_BASE_ADDRESS_SPACE_IO ?
> +        pci_dev->bus->address_space_io : pci_dev->bus->address_space_mem;
> +
> +    if (!pci_dev->enhanced) {
> +
> +        wmask = ~(size - 1);
> +        addr = pci_bar(pci_dev, region_num);
> +        if (region_num == PCI_ROM_SLOT) {
> +            /* ROM enable bit is writable */
> +            wmask |= PCI_ROM_ADDRESS_ENABLE;
> +        }
> +        pci_set_long(pci_dev->config + addr, type);
> +        if (!(r->type & PCI_BASE_ADDRESS_SPACE_IO) &&
> +            r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> +            pci_set_quad(pci_dev->wmask + addr, wmask);
> +            pci_set_quad(pci_dev->cmask + addr, ~0ULL);
> +        } else {
> +            pci_set_long(pci_dev->wmask + addr, wmask & 0xffffffff);
> +            pci_set_long(pci_dev->cmask + addr, 0xffffffff);
> +        }
> +    } else { /* enhanced */
> +
> +        /* add the memory space now */
> +        r->addr = pci_dev->ea_addr[region_num];
> +        memory_region_add_subregion(r->address_space, r->addr, r->memory);
>      }
> -    pci_dev->io_regions[region_num].memory = memory;
> -    pci_dev->io_regions[region_num].address_space
> -        = type & PCI_BASE_ADDRESS_SPACE_IO
> -        ? pci_dev->bus->address_space_io
> -        : pci_dev->bus->address_space_mem;
>  }
>  
> +
> +
>  static void pci_update_vga(PCIDevice *pci_dev)
>  {
>      uint16_t cmd;
> @@ -1107,6 +1145,11 @@ static void pci_update_mappings(PCIDevice *d)
>  
>          new_addr = pci_bar_address(d, i, r->type, r->size);
>  
> +        /* EA BARs cannot be moved */
> +        if (r->enhanced) {
> +            continue;
> +        }
> +
>          /* This bar isn't changed */
>          if (new_addr == r->addr)
>              continue;
> @@ -1785,8 +1828,9 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>      pci_dev = do_pci_register_device(pci_dev, bus,
>                                       object_get_typename(OBJECT(qdev)),
>                                       pci_dev->devfn, errp);
> -    if (pci_dev == NULL)
> +    if (pci_dev == NULL) {
>          return;
> +    }
>  
>      if (pc->realize) {
>          pc->realize(pci_dev, &local_err);
> @@ -1797,6 +1841,12 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>          }
>      }
>  
> +    /* write the EA entry */
> +    if (pci_dev->enhanced) {
> +        pci_ea_cap_init(pci_dev);
> +    }
> +
> +
>      /* rom loading */
>      is_default_rom = false;
>      if (pci_dev->romfile == NULL && pc->romfile != NULL) {
> diff --git a/hw/pci/pci_ea.c b/hw/pci/pci_ea.c
> new file mode 100644
> index 0000000..a052519
> --- /dev/null
> +++ b/hw/pci/pci_ea.c
> @@ -0,0 +1,203 @@
> +/*
> + * PCI Enhanced Allocation (EA) Helper functions
> + * Copyright (c) 2015, Intel Corporation.
> + *
> + * Written by Sean O. Stalley <sean.stalley@intel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */
> +
> +/*
> + * Contians a lot of code taken from pcie.c
> + */
> +
> +#include "qemu-common.h"
> +#include "hw/pci/pci.h"
> +#include "hw/pci/pci_ea.h"
> +
> +bool pci_ea_invalid_addr(uint64_t addr)
> +{
> +    return (addr != (addr & PCI_EA_ADDR_MASK_64));
> +}
> +
> +/* determines if a 64 bit address is necessary for this address */
> +static bool pci_ea_addr_is_64(uint64_t addr)
> +{
> +    return (addr != (uint32_t)addr);
> +}
> +
> +/* Sets the first 32 bits of the Base or MaxOffset field
> + * Returns the length of the address set
> + */
> +static uint8_t pci_ea_set_addr(PCIDevice *dev, uint8_t pos, uint64_t val)
> +{
> +    uint8_t *current_reg = dev->config + pos;
> +
> +    val = val & PCI_EA_ADDR_MASK_64;
> +
> +    if (pci_ea_addr_is_64(val)) {
> +        val |= PCI_EA_FIELD_SIZE_FLAG;
> +    }
> +
> +    pci_set_long(current_reg, val);
> +    return sizeof(int32_t);
> +}
> +
> +/* Determines EA entry length for this IORegion in bytes
> + * (including the first DWORD) */
> +static int pci_ea_entry_length(PCIIORegion *r)
> +{
> +
> +    int len = 3 * sizeof(int32_t); /* First 32 bits of Base & Offset */
> +
> +    if (pci_ea_addr_is_64(r->addr)) {
> +        len += sizeof(int32_t); /* Base (if 64 bit) */
> +    }
> +
> +    if (pci_ea_addr_is_64(r->size - 1)) {
> +        len += sizeof(int32_t); /* Offset (if 64 bit) */
> +    }
> +
> +    return len;
> +
> +}
> +
> +/* determines the value to be set in the Entry Size field */
> +static int pci_ea_get_es(PCIIORegion *r)
> +{
> +
> +    int es = (pci_ea_entry_length(r) / sizeof(int32_t)) - 1;
> +
> +    assert(PCI_EA_MAX_ES >= es);
> +
> +    return es;
> +}
> +
> +/* Initialize an EA entry for the given PCIIORegion
> + *
> + * @dev: The PCI Device
> + * @bei: The BAR Equivalent Indicator
> + * @pos: The offset of this entry in PCI Configspace;
> + */
> +
> +static int pci_ea_fill_entry(PCIDevice *dev, int bei, int pos)
> +{
> +    PCIIORegion *r = &dev->io_regions[bei];
> +    uint32_t dw0 = 0; /* used for setting first DWORD */
> +    int len = 0;
> +
> +    assert(bei <= PCI_EA_MAX_BEI);
> +
> +    /* Check that the base and size are DWORD aligned */
> +    assert(!pci_ea_invalid_addr(r->addr));
> +    assert(!pci_ea_invalid_addr(r->size));
> +
> +    dw0 |= pci_ea_get_es(r);
> +    dw0 |= (bei << PCI_EA_BEI_OFFSET);
> +    dw0 |= (PCI_EA_ENABLE);
> +
> +    /* base Primary Properties off of type field */
> +    if (r->type & PCI_BASE_ADDRESS_SPACE_IO) {
> +        dw0 |= (PCI_EA_IO << PCI_EA_PP_OFFSET);
> +
> +    } else if (r->type & PCI_BASE_ADDRESS_MEM_PREFETCH) {
> +        dw0 |= (PCI_EA_MEM_PREFETCH << PCI_EA_PP_OFFSET);
> +
> +    } else {
> +        dw0 |= (PCI_EA_MEM << PCI_EA_PP_OFFSET);
> +    }
> +
> +    /* Secondary Properties should be ignored */
> +    dw0 |= (PCI_EA_UNAVAL << PCI_EA_SP_OFFSET);
> +
> +    pci_set_long(dev->config + pos, dw0);
> +
> +    len += sizeof(int32_t);
> +
> +    len += pci_ea_set_addr(dev, pos + len, r->addr);
> +    len += pci_ea_set_addr(dev, pos + len, r->size - 1);
> +
> +    /* Base (if 64 bit) */
> +    if (pci_ea_addr_is_64(r->addr)) {
> +        pci_set_long(dev->config + pos + len, (r->addr >> 32));
> +        len += sizeof(int32_t);
> +    }
> +
> +    /* Offset (if 64 bit) */
> +    if (pci_ea_addr_is_64(r->size - 4)) {
> +        pci_set_long(dev->config + pos + len, (r->size >> 32));
> +        len += sizeof(int32_t);
> +    }
> +
> +    assert(len == pci_ea_entry_length(r));
> +
> +    return len;
> +
> +}
> +
> +/*
> + * Initialize the EA capability descriptor
> + * must be done after the EA memory regions are initialized
> + * (or else the EA entries won't get written)
> + */
> +int pci_ea_cap_init(PCIDevice *dev)
> +{
> +    int pos;
> +    int ret;
> +    int i;
> +    int num_entries = 0;
> +    uint8_t length; /* length of this cap entry */
> +    PCIIORegion *r;
> +
> +    /* Determine the length of the capability entry */
> +
> +    length = sizeof(int32_t); /* DWORD 0: Cap ID, Next Pointer, Num Entries */
> +    /* TODO: DWORD 1 for Type 1 Functions */
> +
> +    /* add the length of every entry */
> +    for (i = 0; i < PCI_NUM_REGIONS; ++i) {
> +        r = &dev->io_regions[i];
> +
> +        if (r->enhanced) {
> +            num_entries++;
> +            length += pci_ea_entry_length(r);
> +        }
> +    }
> +
> +    /* add the EA CAP (sets DWORD 0) */
> +    pos = pci_add_capability(dev, PCI_CAP_ID_EA, 0, length);
> +    if (0 > pos) {
> +        return pos;
> +    }
> +
> +    /* DWORD 0: Cap ID, Next Pointer, Num Entries */
> +    assert(num_entries <= PCI_EA_MAX_NUM_ENTRIES); /* overflow */
> +    pci_set_byte(dev->config + pos + PCI_EA_NUM_ENTRIES_OFFSET, num_entries);
> +    pos += sizeof(int32_t);
> +
> +    for (i = 0; i < PCI_NUM_REGIONS; ++i) {
> +        r = &dev->io_regions[i];
> +
> +        if (r->enhanced) {
> +            ret = pci_ea_fill_entry(dev, i, pos);
> +
> +            if (0 > ret) {
> +                return ret;
> +            }
> +            pos += ret;
> +        }
> +
> +    }
> +
> +    assert(pos == pci_find_capability(dev, PCI_CAP_ID_EA) + length);
> +
> +    return pos;
> +
> +}
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index d4ffead..da64435 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -12,6 +12,7 @@
>  #include "hw/isa/isa.h"
>  
>  #include "hw/pci/pcie.h"
> +#include "hw/pci/pci_ea.h"
>  
>  /* PCI bus */
>  
> @@ -109,6 +110,7 @@ typedef struct PCIIORegion {
>      uint8_t type;
>      MemoryRegion *memory;
>      MemoryRegion *address_space;
> +    bool          enhanced;
>  } PCIIORegion;
>  
>  #define PCI_ROM_SLOT 6
> @@ -288,6 +290,11 @@ struct PCIDevice {
>      /* PCI Express */
>      PCIExpressDevice exp;
>  
> +    /* Enhanced Allocation */
> +    bool enhanced;
> +    uint32_t ea_addr_len;
> +    hwaddr *ea_addr;
> +
>      /* SHPC */
>      SHPCDevice *shpc;
>  
> diff --git a/include/hw/pci/pci_ea.h b/include/hw/pci/pci_ea.h
> new file mode 100644
> index 0000000..2b63012
> --- /dev/null
> +++ b/include/hw/pci/pci_ea.h
> @@ -0,0 +1,39 @@
> +/* Constants for pci enhanced allocation (EA) capability descriptor */
> +
> +#ifndef QEMU_PCI_EA_H
> +#define QEMU_PCI_EA_H
> +
> +#define PCI_EA_FIELD_SIZE_FLAG 0x00000002
> +
> +/* the address' must be DWORD aligned */
> +#define PCI_EA_ADDR_MASK_32 0xfffffffc
> +#define PCI_EA_ADDR_MASK_64 0xfffffffffffffffc
> +
> +#define PCI_EA_MAX_NUM_ENTRIES 0x3f
> +
> +#define PCI_EA_BEI_OFFSET 4 /* In bits from beginning of EA entry */
> +#define PCI_EA_ENABLE 0x80000000
> +#define PCI_EA_MAX_BEI 0x8
> +#define PCI_EA_MAX_ES  0x4
> +
> +/* Primary & Secondary Properties, per table 6-1 */
> +#define PCI_EA_PP_OFFSET 8
> +#define PCI_EA_SP_OFFSET 16
> +
> +/* Primary & Secondary Properties Fields, per table 6-1 */
> +#define PCI_EA_MEM               0x00 /* Non-Prefetchable */
> +#define PCI_EA_MEM_PREFETCH      0x01
> +#define PCI_EA_IO                0x02
> +#define PCI_EA_VIRT_MEM_PREFETCH 0x03
> +#define PCI_EA_VIRT_MEM          0x04 /* Non-Prefetchable */
> +#define PCI_EA_UNAVAL_MEM        0xFD
> +#define PCI_EA_UNAVAL_IO         0xFE
> +#define PCI_EA_UNAVAL            0xFF
> +
> +/* checks to see if ea_address is invalid */
> +bool pci_ea_invalid_addr(uint64_t addr);
> +
> +/* Initialize the ea capability descriptor */
> +int pci_ea_cap_init(PCIDevice *dev);
> +
> +#endif /* QEMU_PCI_EA_REG_H */
> diff --git a/include/hw/pci/pci_regs.h b/include/hw/pci/pci_regs.h
> index 56a404b..746ba1a 100644
> --- a/include/hw/pci/pci_regs.h
> +++ b/include/hw/pci/pci_regs.h
> @@ -213,6 +213,7 @@
>  #define  PCI_CAP_ID_MSIX	0x11	/* MSI-X */
>  #define  PCI_CAP_ID_SATA	0x12	/* Serial ATA */
>  #define  PCI_CAP_ID_AF		0x13	/* PCI Advanced Features */
> +#define  PCI_CAP_ID_EA		0x14	/* PCI Enhanced Allocation */
>  #define PCI_CAP_LIST_NEXT	1	/* Next capability in the list */
>  #define PCI_CAP_FLAGS		2	/* Capability defined flags (16 bits) */
>  #define PCI_CAP_SIZEOF		4
> @@ -340,6 +341,9 @@
>  #define PCI_AF_STATUS		5
>  #define  PCI_AF_STATUS_TP	0x01
>  
> +/* PCI Enhanced Allocation registers */
> +#define PCI_EA_NUM_ENTRIES_OFFSET 2
> +
>  /* PCI-X registers */
>  
>  #define PCI_X_CMD		2	/* Modes & Features */
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index cde3314..70a9f3f 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -48,6 +48,7 @@ typedef struct PcGuestInfo PcGuestInfo;
>  typedef struct PCIBridge PCIBridge;
>  typedef struct PCIBus PCIBus;
>  typedef struct PCIDevice PCIDevice;
> +typedef struct PCIEADevice PCIEADevice;
>  typedef struct PCIEAERErr PCIEAERErr;
>  typedef struct PCIEAERLog PCIEAERLog;
>  typedef struct PCIEAERMsg PCIEAERMsg;
> -- 
> 1.9.1

Re: [Qemu-devel] [PATCH 1/1] Add support for PCI Enhanced Allocation "BARs"

[Sean O. Stalley]

On Mon, May 11, 2015 at 09:26:08PM +0200, Michael S. Tsirkin wrote:
> On Mon, May 11, 2015 at 11:56:44AM -0700, Sean O. Stalley wrote:
> > PCI Enhanced Allocation is a new method of allocating MMIO & IO
> > resources for PCI devices & bridges. It can be used instead
> > of the traditional PCI method of using BARs.
> > 
> > EA entries are hardware-initialized to a fixed address.
> > Unlike BARs, regions described by EA are cannot be moved.
> > Because of this, only devices which are perminately connected to
> > the PCI bus can use EA. A removeable PCI card must not use EA.
> > 
> > This patchset enables any existing QEMU PCI model to use EA in leiu of
> > BARs. It adds EA options to the PCI device paramaters.
> > 
> > The Enhanced Allocation ECN is publicly available here:
> > https://www.pcisig.com/specifications/conventional/ECN_Enhanced_Allocation_23_Oct_2014_Final.pdf
> > 
> > Signed-off-by: Sean O. Stalley <sean.stalley@intel.com>
> 
> 
> I will review this, thanks.  Will you also be sending a seabios patch to
> support these registers?  When you do, please Cc me.

Thanks Michael,

I wasn't planning on sending a seabios patch.
I do have some EDKII/OVMF patches I hope to send out soon. I will Cc you on those.

-Sean

> 
> 
> > ---
> >  hw/pci/Makefile.objs      |   2 +-
> >  hw/pci/pci.c              |  96 ++++++++++++++++------
> >  hw/pci/pci_ea.c           | 203 ++++++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/pci/pci.h      |   7 ++
> >  include/hw/pci/pci_ea.h   |  39 +++++++++
> >  include/hw/pci/pci_regs.h |   4 +
> >  include/qemu/typedefs.h   |   1 +
> >  7 files changed, 328 insertions(+), 24 deletions(-)
> >  create mode 100644 hw/pci/pci_ea.c
> >  create mode 100644 include/hw/pci/pci_ea.h

Re: [Qemu-devel] [PATCH 1/1] Add support for PCI Enhanced Allocation "BARs"

[Michael S. Tsirkin]

On Mon, May 11, 2015 at 01:08:05PM -0700, Sean O. Stalley wrote:
> On Mon, May 11, 2015 at 09:26:08PM +0200, Michael S. Tsirkin wrote:
> > On Mon, May 11, 2015 at 11:56:44AM -0700, Sean O. Stalley wrote:
> > > PCI Enhanced Allocation is a new method of allocating MMIO & IO
> > > resources for PCI devices & bridges. It can be used instead
> > > of the traditional PCI method of using BARs.
> > > 
> > > EA entries are hardware-initialized to a fixed address.
> > > Unlike BARs, regions described by EA are cannot be moved.
> > > Because of this, only devices which are perminately connected to
> > > the PCI bus can use EA. A removeable PCI card must not use EA.
> > > 
> > > This patchset enables any existing QEMU PCI model to use EA in leiu of
> > > BARs. It adds EA options to the PCI device paramaters.
> > > 
> > > The Enhanced Allocation ECN is publicly available here:
> > > https://www.pcisig.com/specifications/conventional/ECN_Enhanced_Allocation_23_Oct_2014_Final.pdf
> > > 
> > > Signed-off-by: Sean O. Stalley <sean.stalley@intel.com>
> > 
> > 
> > I will review this, thanks.  Will you also be sending a seabios patch to
> > support these registers?  When you do, please Cc me.
> 
> Thanks Michael,
> 
> I wasn't planning on sending a seabios patch.

Won't this mean that bios might allocate PCI BARs conflicting with
EA entries, causing guest to crash?

> I do have some EDKII/OVMF patches I hope to send out soon. I will Cc you on those.
> 
> -Sean
> 
> > 
> > 
> > > ---
> > >  hw/pci/Makefile.objs      |   2 +-
> > >  hw/pci/pci.c              |  96 ++++++++++++++++------
> > >  hw/pci/pci_ea.c           | 203 ++++++++++++++++++++++++++++++++++++++++++++++
> > >  include/hw/pci/pci.h      |   7 ++
> > >  include/hw/pci/pci_ea.h   |  39 +++++++++
> > >  include/hw/pci/pci_regs.h |   4 +
> > >  include/qemu/typedefs.h   |   1 +
> > >  7 files changed, 328 insertions(+), 24 deletions(-)
> > >  create mode 100644 hw/pci/pci_ea.c
> > >  create mode 100644 include/hw/pci/pci_ea.h

Re: [Qemu-devel] [PATCH 1/1] Add support for PCI Enhanced Allocation "BARs"

[Sean O. Stalley]

On Tue, May 12, 2015 at 11:33:49AM +0200, Michael S. Tsirkin wrote:
> On Mon, May 11, 2015 at 01:08:05PM -0700, Sean O. Stalley wrote:
> > On Mon, May 11, 2015 at 09:26:08PM +0200, Michael S. Tsirkin wrote:
> > > On Mon, May 11, 2015 at 11:56:44AM -0700, Sean O. Stalley wrote:
> > > > PCI Enhanced Allocation is a new method of allocating MMIO & IO
> > > > resources for PCI devices & bridges. It can be used instead
> > > > of the traditional PCI method of using BARs.
> > > > 
> > > > EA entries are hardware-initialized to a fixed address.
> > > > Unlike BARs, regions described by EA are cannot be moved.
> > > > Because of this, only devices which are perminately connected to
> > > > the PCI bus can use EA. A removeable PCI card must not use EA.
> > > > 
> > > > This patchset enables any existing QEMU PCI model to use EA in leiu of
> > > > BARs. It adds EA options to the PCI device paramaters.
> > > > 
> > > > The Enhanced Allocation ECN is publicly available here:
> > > > https://www.pcisig.com/specifications/conventional/ECN_Enhanced_Allocation_23_Oct_2014_Final.pdf
> > > > 
> > > > Signed-off-by: Sean O. Stalley <sean.stalley@intel.com>
> > > 
> > > 
> > > I will review this, thanks.  Will you also be sending a seabios patch to
> > > support these registers?  When you do, please Cc me.
> > 
> > Thanks Michael,
> > 
> > I wasn't planning on sending a seabios patch.
> 
> Won't this mean that bios might allocate PCI BARs conflicting with
> EA entries, causing guest to crash?

The short answer is yes, having EA hardware without an EA-aware bios could cause the guest to crash.

To make matters worse, EA may cause old OSes may crash as well.
If the OS supports hot-swap, and tries to remap the BARs, it could have the same problem.

This patch allows the user to add EA devices to an existing machine,
which is something that would never happen with real EA hardware.

EA devices will only be present on new platforms.
Because of this, I thought it made more sense to add EA support to EDKII than to seabios.
Will adding EA support to seabios be necessary as well?

> > I do have some EDKII/OVMF patches I hope to send out soon. I will Cc you on those.
> > 
> > -Sean
> > 
> > > 
> > > 
> > > > ---
> > > >  hw/pci/Makefile.objs      |   2 +-
> > > >  hw/pci/pci.c              |  96 ++++++++++++++++------
> > > >  hw/pci/pci_ea.c           | 203 ++++++++++++++++++++++++++++++++++++++++++++++
> > > >  include/hw/pci/pci.h      |   7 ++
> > > >  include/hw/pci/pci_ea.h   |  39 +++++++++
> > > >  include/hw/pci/pci_regs.h |   4 +
> > > >  include/qemu/typedefs.h   |   1 +
> > > >  7 files changed, 328 insertions(+), 24 deletions(-)
> > > >  create mode 100644 hw/pci/pci_ea.c
> > > >  create mode 100644 include/hw/pci/pci_ea.h

Re: [Qemu-devel] [PATCH 1/1] Add support for PCI Enhanced Allocation "BARs"

[Michael S. Tsirkin]

On Tue, May 12, 2015 at 02:23:07PM -0700, Sean O. Stalley wrote:
> On Tue, May 12, 2015 at 11:33:49AM +0200, Michael S. Tsirkin wrote:
> > On Mon, May 11, 2015 at 01:08:05PM -0700, Sean O. Stalley wrote:
> > > On Mon, May 11, 2015 at 09:26:08PM +0200, Michael S. Tsirkin wrote:
> > > > On Mon, May 11, 2015 at 11:56:44AM -0700, Sean O. Stalley wrote:
> > > > > PCI Enhanced Allocation is a new method of allocating MMIO & IO
> > > > > resources for PCI devices & bridges. It can be used instead
> > > > > of the traditional PCI method of using BARs.
> > > > > 
> > > > > EA entries are hardware-initialized to a fixed address.
> > > > > Unlike BARs, regions described by EA are cannot be moved.
> > > > > Because of this, only devices which are perminately connected to
> > > > > the PCI bus can use EA. A removeable PCI card must not use EA.
> > > > > 
> > > > > This patchset enables any existing QEMU PCI model to use EA in leiu of
> > > > > BARs. It adds EA options to the PCI device paramaters.
> > > > > 
> > > > > The Enhanced Allocation ECN is publicly available here:
> > > > > https://www.pcisig.com/specifications/conventional/ECN_Enhanced_Allocation_23_Oct_2014_Final.pdf
> > > > > 
> > > > > Signed-off-by: Sean O. Stalley <sean.stalley@intel.com>
> > > > 
> > > > 
> > > > I will review this, thanks.  Will you also be sending a seabios patch to
> > > > support these registers?  When you do, please Cc me.
> > > 
> > > Thanks Michael,
> > > 
> > > I wasn't planning on sending a seabios patch.
> > 
> > Won't this mean that bios might allocate PCI BARs conflicting with
> > EA entries, causing guest to crash?
> 
> The short answer is yes, having EA hardware without an EA-aware bios could cause the guest to crash.
> 
> To make matters worse, EA may cause old OSes may crash as well.
> If the OS supports hot-swap, and tries to remap the BARs, it could have the same problem.

Should _CRS in ACPI be updated to exclude the EA regions?
If we do this, this problem will go away, won't it?

> 
> This patch allows the user to add EA devices to an existing machine,
> which is something that would never happen with real EA hardware.
> 
> EA devices will only be present on new platforms.
> Because of this, I thought it made more sense to add EA support to EDKII than to seabios.
> Will adding EA support to seabios be necessary as well?

If the problem with old OSes is fixable, then I think it's a good idea.

> > > I do have some EDKII/OVMF patches I hope to send out soon. I will Cc you on those.
> > > 
> > > -Sean
> > > 
> > > > 
> > > > 
> > > > > ---
> > > > >  hw/pci/Makefile.objs      |   2 +-
> > > > >  hw/pci/pci.c              |  96 ++++++++++++++++------
> > > > >  hw/pci/pci_ea.c           | 203 ++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  include/hw/pci/pci.h      |   7 ++
> > > > >  include/hw/pci/pci_ea.h   |  39 +++++++++
> > > > >  include/hw/pci/pci_regs.h |   4 +
> > > > >  include/qemu/typedefs.h   |   1 +
> > > > >  7 files changed, 328 insertions(+), 24 deletions(-)
> > > > >  create mode 100644 hw/pci/pci_ea.c
> > > > >  create mode 100644 include/hw/pci/pci_ea.h

Re: [Qemu-devel] [PATCH 1/1] Add support for PCI Enhanced Allocation "BARs"

[Sean O. Stalley]

On Wed, May 13, 2015 at 08:09:46AM +0200, Michael S. Tsirkin wrote:
> On Tue, May 12, 2015 at 02:23:07PM -0700, Sean O. Stalley wrote:
> > On Tue, May 12, 2015 at 11:33:49AM +0200, Michael S. Tsirkin wrote:
> > > On Mon, May 11, 2015 at 01:08:05PM -0700, Sean O. Stalley wrote:
> > > > On Mon, May 11, 2015 at 09:26:08PM +0200, Michael S. Tsirkin wrote:
> > > > > On Mon, May 11, 2015 at 11:56:44AM -0700, Sean O. Stalley wrote:
> > > > > > PCI Enhanced Allocation is a new method of allocating MMIO & IO
> > > > > > resources for PCI devices & bridges. It can be used instead
> > > > > > of the traditional PCI method of using BARs.
> > > > > > 
> > > > > > EA entries are hardware-initialized to a fixed address.
> > > > > > Unlike BARs, regions described by EA are cannot be moved.
> > > > > > Because of this, only devices which are perminately connected to
> > > > > > the PCI bus can use EA. A removeable PCI card must not use EA.
> > > > > > 
> > > > > > This patchset enables any existing QEMU PCI model to use EA in leiu of
> > > > > > BARs. It adds EA options to the PCI device paramaters.
> > > > > > 
> > > > > > The Enhanced Allocation ECN is publicly available here:
> > > > > > https://www.pcisig.com/specifications/conventional/ECN_Enhanced_Allocation_23_Oct_2014_Final.pdf
> > > > > > 
> > > > > > Signed-off-by: Sean O. Stalley <sean.stalley@intel.com>
> > > > > 
> > > > > 
> > > > > I will review this, thanks.  Will you also be sending a seabios patch to
> > > > > support these registers?  When you do, please Cc me.
> > > > 
> > > > Thanks Michael,
> > > > 
> > > > I wasn't planning on sending a seabios patch.
> > > 
> > > Won't this mean that bios might allocate PCI BARs conflicting with
> > > EA entries, causing guest to crash?
> > 
> > The short answer is yes, having EA hardware without an EA-aware bios could cause the guest to crash.
> > 
> > To make matters worse, EA may cause old OSes may crash as well.
> > If the OS supports hot-swap, and tries to remap the BARs, it could have the same problem.
> 
> Should _CRS in ACPI be updated to exclude the EA regions?
> If we do this, this problem will go away, won't it?
> 

I think that would work, assuming the OS uses the _CRS.
(https://lwn.net/Articles/373551/)

I wasn't quite sure how EA should affect ACPI.
This is why I pushed out the QEMU hardware changes before the EDKII changes.

> > 
> > This patch allows the user to add EA devices to an existing machine,
> > which is something that would never happen with real EA hardware.
> > 
> > EA devices will only be present on new platforms.
> > Because of this, I thought it made more sense to add EA support to EDKII than to seabios.
> > Will adding EA support to seabios be necessary as well?
> 
> If the problem with old OSes is fixable, then I think it's a good idea.
> 
> > > > I do have some EDKII/OVMF patches I hope to send out soon. I will Cc you on those.
> > > > 
> > > > -Sean
> > > > 
> > > > > 
> > > > > 
> > > > > > ---
> > > > > >  hw/pci/Makefile.objs      |   2 +-
> > > > > >  hw/pci/pci.c              |  96 ++++++++++++++++------
> > > > > >  hw/pci/pci_ea.c           | 203 ++++++++++++++++++++++++++++++++++++++++++++++
> > > > > >  include/hw/pci/pci.h      |   7 ++
> > > > > >  include/hw/pci/pci_ea.h   |  39 +++++++++
> > > > > >  include/hw/pci/pci_regs.h |   4 +
> > > > > >  include/qemu/typedefs.h   |   1 +
> > > > > >  7 files changed, 328 insertions(+), 24 deletions(-)
> > > > > >  create mode 100644 hw/pci/pci_ea.c
> > > > > >  create mode 100644 include/hw/pci/pci_ea.h

Re: [Qemu-devel] [PATCH 1/1] Add support for PCI Enhanced Allocation "BARs"

[Michael S. Tsirkin]

On Wed, May 13, 2015 at 10:29:42AM -0700, Sean O. Stalley wrote:
> On Wed, May 13, 2015 at 08:09:46AM +0200, Michael S. Tsirkin wrote:
> > On Tue, May 12, 2015 at 02:23:07PM -0700, Sean O. Stalley wrote:
> > > On Tue, May 12, 2015 at 11:33:49AM +0200, Michael S. Tsirkin wrote:
> > > > On Mon, May 11, 2015 at 01:08:05PM -0700, Sean O. Stalley wrote:
> > > > > On Mon, May 11, 2015 at 09:26:08PM +0200, Michael S. Tsirkin wrote:
> > > > > > On Mon, May 11, 2015 at 11:56:44AM -0700, Sean O. Stalley wrote:
> > > > > > > PCI Enhanced Allocation is a new method of allocating MMIO & IO
> > > > > > > resources for PCI devices & bridges. It can be used instead
> > > > > > > of the traditional PCI method of using BARs.
> > > > > > > 
> > > > > > > EA entries are hardware-initialized to a fixed address.
> > > > > > > Unlike BARs, regions described by EA are cannot be moved.
> > > > > > > Because of this, only devices which are perminately connected to
> > > > > > > the PCI bus can use EA. A removeable PCI card must not use EA.
> > > > > > > 
> > > > > > > This patchset enables any existing QEMU PCI model to use EA in leiu of
> > > > > > > BARs. It adds EA options to the PCI device paramaters.
> > > > > > > 
> > > > > > > The Enhanced Allocation ECN is publicly available here:
> > > > > > > https://www.pcisig.com/specifications/conventional/ECN_Enhanced_Allocation_23_Oct_2014_Final.pdf
> > > > > > > 
> > > > > > > Signed-off-by: Sean O. Stalley <sean.stalley@intel.com>
> > > > > > 
> > > > > > 
> > > > > > I will review this, thanks.  Will you also be sending a seabios patch to
> > > > > > support these registers?  When you do, please Cc me.
> > > > > 
> > > > > Thanks Michael,
> > > > > 
> > > > > I wasn't planning on sending a seabios patch.
> > > > 
> > > > Won't this mean that bios might allocate PCI BARs conflicting with
> > > > EA entries, causing guest to crash?
> > > 
> > > The short answer is yes, having EA hardware without an EA-aware bios could cause the guest to crash.
> > > 
> > > To make matters worse, EA may cause old OSes may crash as well.
> > > If the OS supports hot-swap, and tries to remap the BARs, it could have the same problem.
> > 
> > Should _CRS in ACPI be updated to exclude the EA regions?
> > If we do this, this problem will go away, won't it?
> > 
> 
> I think that would work, assuming the OS uses the _CRS.
> (https://lwn.net/Articles/373551/)

Well that's Linux, so just send patches to teach it about EA.

> I wasn't quite sure how EA should affect ACPI.
> This is why I pushed out the QEMU hardware changes before the EDKII changes.

OK, so now we have an idea, I'm just hoping it's the correct one :)

> > > 
> > > This patch allows the user to add EA devices to an existing machine,
> > > which is something that would never happen with real EA hardware.
> > > 
> > > EA devices will only be present on new platforms.
> > > Because of this, I thought it made more sense to add EA support to EDKII than to seabios.
> > > Will adding EA support to seabios be necessary as well?
> > 
> > If the problem with old OSes is fixable, then I think it's a good idea.
> > 
> > > > > I do have some EDKII/OVMF patches I hope to send out soon. I will Cc you on those.
> > > > > 
> > > > > -Sean
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > > ---
> > > > > > >  hw/pci/Makefile.objs      |   2 +-
> > > > > > >  hw/pci/pci.c              |  96 ++++++++++++++++------
> > > > > > >  hw/pci/pci_ea.c           | 203 ++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > >  include/hw/pci/pci.h      |   7 ++
> > > > > > >  include/hw/pci/pci_ea.h   |  39 +++++++++
> > > > > > >  include/hw/pci/pci_regs.h |   4 +
> > > > > > >  include/qemu/typedefs.h   |   1 +
> > > > > > >  7 files changed, 328 insertions(+), 24 deletions(-)
> > > > > > >  create mode 100644 hw/pci/pci_ea.c
> > > > > > >  create mode 100644 include/hw/pci/pci_ea.h

Re: [Qemu-devel] [PATCH 1/1] Add support for PCI Enhanced Allocation "BARs"

[Michael S. Tsirkin]

On Wed, May 13, 2015 at 10:29:42AM -0700, Sean O. Stalley wrote:
> On Wed, May 13, 2015 at 08:09:46AM +0200, Michael S. Tsirkin wrote:
> > On Tue, May 12, 2015 at 02:23:07PM -0700, Sean O. Stalley wrote:
> > > On Tue, May 12, 2015 at 11:33:49AM +0200, Michael S. Tsirkin wrote:
> > > > On Mon, May 11, 2015 at 01:08:05PM -0700, Sean O. Stalley wrote:
> > > > > On Mon, May 11, 2015 at 09:26:08PM +0200, Michael S. Tsirkin wrote:
> > > > > > On Mon, May 11, 2015 at 11:56:44AM -0700, Sean O. Stalley wrote:
> > > > > > > PCI Enhanced Allocation is a new method of allocating MMIO & IO
> > > > > > > resources for PCI devices & bridges. It can be used instead
> > > > > > > of the traditional PCI method of using BARs.
> > > > > > > 
> > > > > > > EA entries are hardware-initialized to a fixed address.
> > > > > > > Unlike BARs, regions described by EA are cannot be moved.
> > > > > > > Because of this, only devices which are perminately connected to
> > > > > > > the PCI bus can use EA. A removeable PCI card must not use EA.
> > > > > > > 
> > > > > > > This patchset enables any existing QEMU PCI model to use EA in leiu of
> > > > > > > BARs. It adds EA options to the PCI device paramaters.
> > > > > > > 
> > > > > > > The Enhanced Allocation ECN is publicly available here:
> > > > > > > https://www.pcisig.com/specifications/conventional/ECN_Enhanced_Allocation_23_Oct_2014_Final.pdf
> > > > > > > 
> > > > > > > Signed-off-by: Sean O. Stalley <sean.stalley@intel.com>
> > > > > > 
> > > > > > 
> > > > > > I will review this, thanks.  Will you also be sending a seabios patch to
> > > > > > support these registers?  When you do, please Cc me.
> > > > > 
> > > > > Thanks Michael,
> > > > > 
> > > > > I wasn't planning on sending a seabios patch.
> > > > 
> > > > Won't this mean that bios might allocate PCI BARs conflicting with
> > > > EA entries, causing guest to crash?
> > > 
> > > The short answer is yes, having EA hardware without an EA-aware bios could cause the guest to crash.
> > > 
> > > To make matters worse, EA may cause old OSes may crash as well.
> > > If the OS supports hot-swap, and tries to remap the BARs, it could have the same problem.
> > 
> > Should _CRS in ACPI be updated to exclude the EA regions?
> > If we do this, this problem will go away, won't it?
> > 
> 
> I think that would work, assuming the OS uses the _CRS.
> (https://lwn.net/Articles/373551/)
> 
> I wasn't quite sure how EA should affect ACPI.
> This is why I pushed out the QEMU hardware changes before the EDKII changes.


So I'm waiting for v2 of this?

> > > 
> > > This patch allows the user to add EA devices to an existing machine,
> > > which is something that would never happen with real EA hardware.
> > > 
> > > EA devices will only be present on new platforms.
> > > Because of this, I thought it made more sense to add EA support to EDKII than to seabios.
> > > Will adding EA support to seabios be necessary as well?
> > 
> > If the problem with old OSes is fixable, then I think it's a good idea.
> > 
> > > > > I do have some EDKII/OVMF patches I hope to send out soon. I will Cc you on those.
> > > > > 
> > > > > -Sean
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > > ---
> > > > > > >  hw/pci/Makefile.objs      |   2 +-
> > > > > > >  hw/pci/pci.c              |  96 ++++++++++++++++------
> > > > > > >  hw/pci/pci_ea.c           | 203 ++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > >  include/hw/pci/pci.h      |   7 ++
> > > > > > >  include/hw/pci/pci_ea.h   |  39 +++++++++
> > > > > > >  include/hw/pci/pci_regs.h |   4 +
> > > > > > >  include/qemu/typedefs.h   |   1 +
> > > > > > >  7 files changed, 328 insertions(+), 24 deletions(-)
> > > > > > >  create mode 100644 hw/pci/pci_ea.c
> > > > > > >  create mode 100644 include/hw/pci/pci_ea.h

Re: [Qemu-devel] [PATCH 1/1] Add support for PCI Enhanced Allocation "BARs"

[Sean O. Stalley]

On Sun, May 31, 2015 at 08:13:49PM +0200, Michael S. Tsirkin wrote:
> On Wed, May 13, 2015 at 10:29:42AM -0700, Sean O. Stalley wrote:
> > On Wed, May 13, 2015 at 08:09:46AM +0200, Michael S. Tsirkin wrote:
> > > On Tue, May 12, 2015 at 02:23:07PM -0700, Sean O. Stalley wrote:
> > > > On Tue, May 12, 2015 at 11:33:49AM +0200, Michael S. Tsirkin wrote:
> > > > > On Mon, May 11, 2015 at 01:08:05PM -0700, Sean O. Stalley wrote:
> > > > > > On Mon, May 11, 2015 at 09:26:08PM +0200, Michael S. Tsirkin wrote:
> > > > > > > On Mon, May 11, 2015 at 11:56:44AM -0700, Sean O. Stalley wrote:
> > > > > > > > PCI Enhanced Allocation is a new method of allocating MMIO & IO
> > > > > > > > resources for PCI devices & bridges. It can be used instead
> > > > > > > > of the traditional PCI method of using BARs.
> > > > > > > > 
> > > > > > > > EA entries are hardware-initialized to a fixed address.
> > > > > > > > Unlike BARs, regions described by EA are cannot be moved.
> > > > > > > > Because of this, only devices which are perminately connected to
> > > > > > > > the PCI bus can use EA. A removeable PCI card must not use EA.
> > > > > > > > 
> > > > > > > > This patchset enables any existing QEMU PCI model to use EA in leiu of
> > > > > > > > BARs. It adds EA options to the PCI device paramaters.
> > > > > > > > 
> > > > > > > > The Enhanced Allocation ECN is publicly available here:
> > > > > > > > https://www.pcisig.com/specifications/conventional/ECN_Enhanced_Allocation_23_Oct_2014_Final.pdf
> > > > > > > > 
> > > > > > > > Signed-off-by: Sean O. Stalley <sean.stalley@intel.com>
> > > > > > > 
> > > > > > > 
> > > > > > > I will review this, thanks.  Will you also be sending a seabios patch to
> > > > > > > support these registers?  When you do, please Cc me.
> > > > > > 
> > > > > > Thanks Michael,
> > > > > > 
> > > > > > I wasn't planning on sending a seabios patch.
> > > > > 
> > > > > Won't this mean that bios might allocate PCI BARs conflicting with
> > > > > EA entries, causing guest to crash?
> > > > 
> > > > The short answer is yes, having EA hardware without an EA-aware bios could cause the guest to crash.
> > > > 
> > > > To make matters worse, EA may cause old OSes may crash as well.
> > > > If the OS supports hot-swap, and tries to remap the BARs, it could have the same problem.
> > > 
> > > Should _CRS in ACPI be updated to exclude the EA regions?
> > > If we do this, this problem will go away, won't it?
> > > 
> > 
> > I think that would work, assuming the OS uses the _CRS.
> > (https://lwn.net/Articles/373551/)
> > 
> > I wasn't quite sure how EA should affect ACPI.
> > This is why I pushed out the QEMU hardware changes before the EDKII changes.
> 
> 
> So I'm waiting for v2 of this?
> 

I wasn't planning on changing the hardware models in this patch (at least not until they are reviewed).

Based on our discussion about ACPI, I was going to make changes to the firmware patches.
I don't think I need to change anything in this patchset to fix ACPI.

> > > > 
> > > > This patch allows the user to add EA devices to an existing machine,
> > > > which is something that would never happen with real EA hardware.
> > > > 
> > > > EA devices will only be present on new platforms.
> > > > Because of this, I thought it made more sense to add EA support to EDKII than to seabios.
> > > > Will adding EA support to seabios be necessary as well?
> > > 
> > > If the problem with old OSes is fixable, then I think it's a good idea.
> > > 
> > > > > > I do have some EDKII/OVMF patches I hope to send out soon. I will Cc you on those.
> > > > > > 
> > > > > > -Sean
> > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > > ---
> > > > > > > >  hw/pci/Makefile.objs      |   2 +-
> > > > > > > >  hw/pci/pci.c              |  96 ++++++++++++++++------
> > > > > > > >  hw/pci/pci_ea.c           | 203 ++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > >  include/hw/pci/pci.h      |   7 ++
> > > > > > > >  include/hw/pci/pci_ea.h   |  39 +++++++++
> > > > > > > >  include/hw/pci/pci_regs.h |   4 +
> > > > > > > >  include/qemu/typedefs.h   |   1 +
> > > > > > > >  7 files changed, 328 insertions(+), 24 deletions(-)
> > > > > > > >  create mode 100644 hw/pci/pci_ea.c
> > > > > > > >  create mode 100644 include/hw/pci/pci_ea.h

Re: [Qemu-devel] [PATCH 1/1] Add support for PCI Enhanced Allocation "BARs"

[Stalley, Sean]

Hi Michael,

Have you had a chance to look at this?
There's no hurry, im just curious.

Thanks,
Sean

> -----Original Message-----
> From: Sean O. Stalley [mailto:sean.stalley@intel.com]
> Sent: Monday, June 01, 2015 9:38 AM
> To: Michael S. Tsirkin
> Cc: qemu-devel@nongnu.org
> Subject: Re: [PATCH 1/1] Add support for PCI Enhanced Allocation "BARs"
> 
> On Sun, May 31, 2015 at 08:13:49PM +0200, Michael S. Tsirkin wrote:
> > On Wed, May 13, 2015 at 10:29:42AM -0700, Sean O. Stalley wrote:
> > > On Wed, May 13, 2015 at 08:09:46AM +0200, Michael S. Tsirkin wrote:
> > > > On Tue, May 12, 2015 at 02:23:07PM -0700, Sean O. Stalley wrote:
> > > > > On Tue, May 12, 2015 at 11:33:49AM +0200, Michael S. Tsirkin
> wrote:
> > > > > > On Mon, May 11, 2015 at 01:08:05PM -0700, Sean O. Stalley
> wrote:
> > > > > > > On Mon, May 11, 2015 at 09:26:08PM +0200, Michael S. Tsirkin
> wrote:
> > > > > > > > On Mon, May 11, 2015 at 11:56:44AM -0700, Sean O. Stalley
> wrote:
> > > > > > > > > PCI Enhanced Allocation is a new method of allocating
> > > > > > > > > MMIO & IO resources for PCI devices & bridges. It can be
> > > > > > > > > used instead of the traditional PCI method of using BARs.
> > > > > > > > >
> > > > > > > > > EA entries are hardware-initialized to a fixed address.
> > > > > > > > > Unlike BARs, regions described by EA are cannot be moved.
> > > > > > > > > Because of this, only devices which are perminately
> > > > > > > > > connected to the PCI bus can use EA. A removeable PCI card
> must not use EA.
> > > > > > > > >
> > > > > > > > > This patchset enables any existing QEMU PCI model to use
> > > > > > > > > EA in leiu of BARs. It adds EA options to the PCI device
> paramaters.
> > > > > > > > >
> > > > > > > > > The Enhanced Allocation ECN is publicly available here:
> > > > > > > > > https://www.pcisig.com/specifications/conventional/ECN_E
> > > > > > > > > nhanced_Allocation_23_Oct_2014_Final.pdf
> > > > > > > > >
> > > > > > > > > Signed-off-by: Sean O. Stalley <sean.stalley@intel.com>
> > > > > > > >
> > > > > > > >
> > > > > > > > I will review this, thanks.  Will you also be sending a
> > > > > > > > seabios patch to support these registers?  When you do,
> please Cc me.
> > > > > > >
> > > > > > > Thanks Michael,
> > > > > > >
> > > > > > > I wasn't planning on sending a seabios patch.
> > > > > >
> > > > > > Won't this mean that bios might allocate PCI BARs conflicting
> > > > > > with EA entries, causing guest to crash?
> > > > >
> > > > > The short answer is yes, having EA hardware without an EA-aware
> bios could cause the guest to crash.
> > > > >
> > > > > To make matters worse, EA may cause old OSes may crash as well.
> > > > > If the OS supports hot-swap, and tries to remap the BARs, it could
> have the same problem.
> > > >
> > > > Should _CRS in ACPI be updated to exclude the EA regions?
> > > > If we do this, this problem will go away, won't it?
> > > >
> > >
> > > I think that would work, assuming the OS uses the _CRS.
> > > (https://lwn.net/Articles/373551/)
> > >
> > > I wasn't quite sure how EA should affect ACPI.
> > > This is why I pushed out the QEMU hardware changes before the EDKII
> changes.
> >
> >
> > So I'm waiting for v2 of this?
> >
> 
> I wasn't planning on changing the hardware models in this patch (at least
> not until they are reviewed).
> 
> Based on our discussion about ACPI, I was going to make changes to the
> firmware patches.
> I don't think I need to change anything in this patchset to fix ACPI.
> 
> > > > >
> > > > > This patch allows the user to add EA devices to an existing
> > > > > machine, which is something that would never happen with real EA
> hardware.
> > > > >
> > > > > EA devices will only be present on new platforms.
> > > > > Because of this, I thought it made more sense to add EA support to
> EDKII than to seabios.
> > > > > Will adding EA support to seabios be necessary as well?
> > > >
> > > > If the problem with old OSes is fixable, then I think it's a good idea.
> > > >
> > > > > > > I do have some EDKII/OVMF patches I hope to send out soon. I
> will Cc you on those.
> > > > > > >
> > > > > > > -Sean
> > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > > ---
> > > > > > > > >  hw/pci/Makefile.objs      |   2 +-
> > > > > > > > >  hw/pci/pci.c              |  96 ++++++++++++++++------
> > > > > > > > >  hw/pci/pci_ea.c           | 203
> ++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > > >  include/hw/pci/pci.h      |   7 ++
> > > > > > > > >  include/hw/pci/pci_ea.h   |  39 +++++++++
> > > > > > > > >  include/hw/pci/pci_regs.h |   4 +
> > > > > > > > >  include/qemu/typedefs.h   |   1 +
> > > > > > > > >  7 files changed, 328 insertions(+), 24 deletions(-)
> > > > > > > > > create mode 100644 hw/pci/pci_ea.c  create mode 100644
> > > > > > > > > include/hw/pci/pci_ea.h

Re: [Qemu-devel] [PATCH 1/1] Add support for PCI Enhanced Allocation "BARs"

[Michael S. Tsirkin]

On Mon, May 11, 2015 at 11:56:44AM -0700, Sean O. Stalley wrote:
> PCI Enhanced Allocation is a new method of allocating MMIO & IO
> resources for PCI devices & bridges. It can be used instead
> of the traditional PCI method of using BARs.
> 
> EA entries are hardware-initialized to a fixed address.
> Unlike BARs, regions described by EA are cannot be moved.
> Because of this, only devices which are perminately connected to
> the PCI bus can use EA. A removeable PCI card must not use EA.
> 
> This patchset enables any existing QEMU PCI model to use EA in leiu of

s/in leiu/instead of/

if you can't spell it, don't use it :)

> BARs. It adds EA options to the PCI device paramaters.

parameters

> 
> The Enhanced Allocation ECN is publicly available here:
> https://www.pcisig.com/specifications/conventional/ECN_Enhanced_Allocation_23_Oct_2014_Final.pdf
> 
> Signed-off-by: Sean O. Stalley <sean.stalley@intel.com>
> ---
>  hw/pci/Makefile.objs      |   2 +-
>  hw/pci/pci.c              |  96 ++++++++++++++++------
>  hw/pci/pci_ea.c           | 203 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/pci/pci.h      |   7 ++
>  include/hw/pci/pci_ea.h   |  39 +++++++++
>  include/hw/pci/pci_regs.h |   4 +
>  include/qemu/typedefs.h   |   1 +
>  7 files changed, 328 insertions(+), 24 deletions(-)
>  create mode 100644 hw/pci/pci_ea.c
>  create mode 100644 include/hw/pci/pci_ea.h
> 
> diff --git a/hw/pci/Makefile.objs b/hw/pci/Makefile.objs
> index 9f905e6..e5d80cf 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 pci_ea.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 b3d5100..c7552ca 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -23,6 +23,7 @@
>   */
>  #include "hw/hw.h"
>  #include "hw/pci/pci.h"
> +#include "hw/pci/pci_ea.h"
>  #include "hw/pci/pci_bridge.h"
>  #include "hw/pci/pci_bus.h"
>  #include "hw/pci/pci_host.h"
> @@ -58,6 +59,10 @@ static Property pci_props[] = {
>                      QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false),
>      DEFINE_PROP_BIT("command_serr_enable", PCIDevice, cap_present,
>                      QEMU_PCI_CAP_SERR_BITNR, true),
> +    DEFINE_PROP_BOOL("enhanced_allocation", PCIDevice, enhanced, false),
> +    DEFINE_PROP_ARRAY("ea_addr", PCIDevice, ea_addr_len, ea_addr,
> +                      qdev_prop_uint64, hwaddr),
> +
>      DEFINE_PROP_END_OF_LIST()
>  };
>  

Hmm, why do we need the bool property? Can't we just check that
ea_addr is specified?

> @@ -882,6 +887,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>          config_read = pci_default_read_config;
>      if (!config_write)
>          config_write = pci_default_write_config;
> +
>      pci_dev->config_read = config_read;
>      pci_dev->config_write = config_write;
>      bus->devices[devfn] = pci_dev;

don't add random whitspace please.

> @@ -929,40 +935,72 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
>  
>      assert(region_num >= 0);
>      assert(region_num < PCI_NUM_REGIONS);
> -    if (size & (size-1)) {
> +
> +
> +    /* TODO: these checks should be done earlier */
> +    if (!pci_dev->enhanced && (size & (size-1))) {
>          fprintf(stderr, "ERROR: PCI region size must be pow2 "
>                      "type=0x%x, size=0x%"FMT_PCIBUS"\n", type, size);
>          exit(1);
>      }
>  
> +    if (pci_dev->enhanced) {
> +
> +        if (pci_dev->ea_addr_len <= region_num) {
> +            fprintf(stderr, "ERROR: Address for EA entry %i not given\n",
> +                    region_num);
> +            exit(1);
> +        }
> +
> +        if (pci_dev->ea_addr[region_num] == 0x0) {
> +            fprintf(stderr, "ERROR: Address for EA entry %i not set\n",
> +                    region_num);
> +            exit(1);
> +        }

So this means that the management tool needs to know the BAR number used
by the device. It also needs to know how much memory to request. Which
is all very low level, but at least more or less stable for emulated
devices, but we generally were free to change them for e.g. virtio
devices.

Isn't it sufficient to just enable ea?
Implement a global allocator, and have each device allocate space
according to BAR size.
We might need to use some tricks to make this stable when order
of initialization changes (sort by name?) but otherwise -
any problems with this?


> +
> +        if (pci_ea_invalid_addr(pci_dev->ea_addr[region_num])) {
> +            fprintf(stderr, "ERROR: Address for EA entry %i not valid\n",
> +                    region_num);
> +            exit(1);
> +        }
> +    }
> +
>      r = &pci_dev->io_regions[region_num];
>      r->addr = PCI_BAR_UNMAPPED;
>      r->size = size;
>      r->type = type;
> -    r->memory = NULL;
> -
> -    wmask = ~(size - 1);
> -    addr = pci_bar(pci_dev, region_num);
> -    if (region_num == PCI_ROM_SLOT) {
> -        /* ROM enable bit is writable */
> -        wmask |= PCI_ROM_ADDRESS_ENABLE;
> -    }
> -    pci_set_long(pci_dev->config + addr, type);
> -    if (!(r->type & PCI_BASE_ADDRESS_SPACE_IO) &&
> -        r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> -        pci_set_quad(pci_dev->wmask + addr, wmask);
> -        pci_set_quad(pci_dev->cmask + addr, ~0ULL);
> -    } else {
> -        pci_set_long(pci_dev->wmask + addr, wmask & 0xffffffff);
> -        pci_set_long(pci_dev->cmask + addr, 0xffffffff);
> +    r->enhanced = pci_dev->enhanced;
> +    r->memory = memory;
> +    r->address_space = type & PCI_BASE_ADDRESS_SPACE_IO ?
> +        pci_dev->bus->address_space_io : pci_dev->bus->address_space_mem;
> +
> +    if (!pci_dev->enhanced) {
> +
> +        wmask = ~(size - 1);
> +        addr = pci_bar(pci_dev, region_num);
> +        if (region_num == PCI_ROM_SLOT) {
> +            /* ROM enable bit is writable */
> +            wmask |= PCI_ROM_ADDRESS_ENABLE;
> +        }
> +        pci_set_long(pci_dev->config + addr, type);
> +        if (!(r->type & PCI_BASE_ADDRESS_SPACE_IO) &&
> +            r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> +            pci_set_quad(pci_dev->wmask + addr, wmask);
> +            pci_set_quad(pci_dev->cmask + addr, ~0ULL);
> +        } else {
> +            pci_set_long(pci_dev->wmask + addr, wmask & 0xffffffff);
> +            pci_set_long(pci_dev->cmask + addr, 0xffffffff);
> +        }
> +    } else { /* enhanced */
> +
> +        /* add the memory space now */
> +        r->addr = pci_dev->ea_addr[region_num];
> +        memory_region_add_subregion(r->address_space, r->addr, r->memory);
>      }
> -    pci_dev->io_regions[region_num].memory = memory;
> -    pci_dev->io_regions[region_num].address_space
> -        = type & PCI_BASE_ADDRESS_SPACE_IO
> -        ? pci_dev->bus->address_space_io
> -        : pci_dev->bus->address_space_mem;
>  }
>  
> +
> +
>  static void pci_update_vga(PCIDevice *pci_dev)
>  {
>      uint16_t cmd;
> @@ -1107,6 +1145,11 @@ static void pci_update_mappings(PCIDevice *d)
>  
>          new_addr = pci_bar_address(d, i, r->type, r->size);
>  
> +        /* EA BARs cannot be moved */
> +        if (r->enhanced) {
> +            continue;
> +        }
> +
>          /* This bar isn't changed */
>          if (new_addr == r->addr)
>              continue;
> @@ -1785,8 +1828,9 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>      pci_dev = do_pci_register_device(pci_dev, bus,
>                                       object_get_typename(OBJECT(qdev)),
>                                       pci_dev->devfn, errp);
> -    if (pci_dev == NULL)
> +    if (pci_dev == NULL) {
>          return;
> +    }
>  
>      if (pc->realize) {
>          pc->realize(pci_dev, &local_err);
> @@ -1797,6 +1841,12 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>          }
>      }
>  
> +    /* write the EA entry */
> +    if (pci_dev->enhanced) {
> +        pci_ea_cap_init(pci_dev);
> +    }
> +
> +
>      /* rom loading */
>      is_default_rom = false;
>      if (pci_dev->romfile == NULL && pc->romfile != NULL) {
> diff --git a/hw/pci/pci_ea.c b/hw/pci/pci_ea.c
> new file mode 100644
> index 0000000..a052519
> --- /dev/null
> +++ b/hw/pci/pci_ea.c
> @@ -0,0 +1,203 @@
> +/*
> + * PCI Enhanced Allocation (EA) Helper functions
> + * Copyright (c) 2015, Intel Corporation.
> + *
> + * Written by Sean O. Stalley <sean.stalley@intel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */
> +
> +/*
> + * Contians a lot of code taken from pcie.c
> + */
> +
> +#include "qemu-common.h"
> +#include "hw/pci/pci.h"
> +#include "hw/pci/pci_ea.h"
> +
> +bool pci_ea_invalid_addr(uint64_t addr)
> +{
> +    return (addr != (addr & PCI_EA_ADDR_MASK_64));
> +}
> +
> +/* determines if a 64 bit address is necessary for this address */
> +static bool pci_ea_addr_is_64(uint64_t addr)
> +{
> +    return (addr != (uint32_t)addr);
> +}

don't put () after return pls.

> +
> +/* Sets the first 32 bits of the Base or MaxOffset field
> + * Returns the length of the address set
> + */
> +static uint8_t pci_ea_set_addr(PCIDevice *dev, uint8_t pos, uint64_t val)
> +{
> +    uint8_t *current_reg = dev->config + pos;
> +
> +    val = val & PCI_EA_ADDR_MASK_64;
> +
> +    if (pci_ea_addr_is_64(val)) {
> +        val |= PCI_EA_FIELD_SIZE_FLAG;
> +    }
> +
> +    pci_set_long(current_reg, val);
> +    return sizeof(int32_t);
> +}
> +
> +/* Determines EA entry length for this IORegion in bytes
> + * (including the first DWORD) */
> +static int pci_ea_entry_length(PCIIORegion *r)
> +{
> +
> +    int len = 3 * sizeof(int32_t); /* First 32 bits of Base & Offset */
> +
> +    if (pci_ea_addr_is_64(r->addr)) {
> +        len += sizeof(int32_t); /* Base (if 64 bit) */
> +    }
> +
> +    if (pci_ea_addr_is_64(r->size - 1)) {
> +        len += sizeof(int32_t); /* Offset (if 64 bit) */
> +    }
> +
> +    return len;
> +
> +}
> +
> +/* determines the value to be set in the Entry Size field */
> +static int pci_ea_get_es(PCIIORegion *r)
> +{
> +
> +    int es = (pci_ea_entry_length(r) / sizeof(int32_t)) - 1;
> +
> +    assert(PCI_EA_MAX_ES >= es);
> +
> +    return es;
> +}
> +
> +/* Initialize an EA entry for the given PCIIORegion
> + *
> + * @dev: The PCI Device
> + * @bei: The BAR Equivalent Indicator
> + * @pos: The offset of this entry in PCI Configspace;
> + */
> +

no need for an empty line here.

> +static int pci_ea_fill_entry(PCIDevice *dev, int bei, int pos)
> +{
> +    PCIIORegion *r = &dev->io_regions[bei];
> +    uint32_t dw0 = 0; /* used for setting first DWORD */
> +    int len = 0;
> +
> +    assert(bei <= PCI_EA_MAX_BEI);
> +
> +    /* Check that the base and size are DWORD aligned */
> +    assert(!pci_ea_invalid_addr(r->addr));
> +    assert(!pci_ea_invalid_addr(r->size));
> +
> +    dw0 |= pci_ea_get_es(r);
> +    dw0 |= (bei << PCI_EA_BEI_OFFSET);
> +    dw0 |= (PCI_EA_ENABLE);
> +
> +    /* base Primary Properties off of type field */
> +    if (r->type & PCI_BASE_ADDRESS_SPACE_IO) {
> +        dw0 |= (PCI_EA_IO << PCI_EA_PP_OFFSET);
> +
> +    } else if (r->type & PCI_BASE_ADDRESS_MEM_PREFETCH) {
> +        dw0 |= (PCI_EA_MEM_PREFETCH << PCI_EA_PP_OFFSET);
> +
> +    } else {
> +        dw0 |= (PCI_EA_MEM << PCI_EA_PP_OFFSET);
> +    }
> +
> +    /* Secondary Properties should be ignored */
> +    dw0 |= (PCI_EA_UNAVAL << PCI_EA_SP_OFFSET);
> +
> +    pci_set_long(dev->config + pos, dw0);
> +
> +    len += sizeof(int32_t);
> +
> +    len += pci_ea_set_addr(dev, pos + len, r->addr);
> +    len += pci_ea_set_addr(dev, pos + len, r->size - 1);
> +
> +    /* Base (if 64 bit) */
> +    if (pci_ea_addr_is_64(r->addr)) {
> +        pci_set_long(dev->config + pos + len, (r->addr >> 32));
> +        len += sizeof(int32_t);
> +    }
> +
> +    /* Offset (if 64 bit) */
> +    if (pci_ea_addr_is_64(r->size - 4)) {
> +        pci_set_long(dev->config + pos + len, (r->size >> 32));
> +        len += sizeof(int32_t);
> +    }
> +
> +    assert(len == pci_ea_entry_length(r));
> +
> +    return len;
> +
> +}
> +
> +/*
> + * Initialize the EA capability descriptor
> + * must be done after the EA memory regions are initialized
> + * (or else the EA entries won't get written)
> + */
> +int pci_ea_cap_init(PCIDevice *dev)
> +{
> +    int pos;
> +    int ret;
> +    int i;
> +    int num_entries = 0;
> +    uint8_t length; /* length of this cap entry */
> +    PCIIORegion *r;
> +
> +    /* Determine the length of the capability entry */
> +
> +    length = sizeof(int32_t); /* DWORD 0: Cap ID, Next Pointer, Num Entries */
> +    /* TODO: DWORD 1 for Type 1 Functions */
> +
> +    /* add the length of every entry */
> +    for (i = 0; i < PCI_NUM_REGIONS; ++i) {
> +        r = &dev->io_regions[i];
> +
> +        if (r->enhanced) {
> +            num_entries++;
> +            length += pci_ea_entry_length(r);
> +        }
> +    }
> +
> +    /* add the EA CAP (sets DWORD 0) */
> +    pos = pci_add_capability(dev, PCI_CAP_ID_EA, 0, length);
> +    if (0 > pos) {
> +        return pos;
> +    }
> +
> +    /* DWORD 0: Cap ID, Next Pointer, Num Entries */
> +    assert(num_entries <= PCI_EA_MAX_NUM_ENTRIES); /* overflow */
> +    pci_set_byte(dev->config + pos + PCI_EA_NUM_ENTRIES_OFFSET, num_entries);
> +    pos += sizeof(int32_t);
> +
> +    for (i = 0; i < PCI_NUM_REGIONS; ++i) {
> +        r = &dev->io_regions[i];
> +
> +        if (r->enhanced) {
> +            ret = pci_ea_fill_entry(dev, i, pos);
> +
> +            if (0 > ret) {
> +                return ret;
> +            }
> +            pos += ret;
> +        }
> +
> +    }
> +
> +    assert(pos == pci_find_capability(dev, PCI_CAP_ID_EA) + length);
> +
> +    return pos;
> +
> +}
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index d4ffead..da64435 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -12,6 +12,7 @@
>  #include "hw/isa/isa.h"
>  
>  #include "hw/pci/pcie.h"
> +#include "hw/pci/pci_ea.h"
>  
>  /* PCI bus */
>  
> @@ -109,6 +110,7 @@ typedef struct PCIIORegion {
>      uint8_t type;
>      MemoryRegion *memory;
>      MemoryRegion *address_space;
> +    bool          enhanced;
>  } PCIIORegion;
>  
>  #define PCI_ROM_SLOT 6
> @@ -288,6 +290,11 @@ struct PCIDevice {
>      /* PCI Express */
>      PCIExpressDevice exp;
>  
> +    /* Enhanced Allocation */
> +    bool enhanced;
> +    uint32_t ea_addr_len;
> +    hwaddr *ea_addr;
> +
>      /* SHPC */
>      SHPCDevice *shpc;
>  
> diff --git a/include/hw/pci/pci_ea.h b/include/hw/pci/pci_ea.h
> new file mode 100644
> index 0000000..2b63012
> --- /dev/null
> +++ b/include/hw/pci/pci_ea.h
> @@ -0,0 +1,39 @@
> +/* Constants for pci enhanced allocation (EA) capability descriptor */
> +
> +#ifndef QEMU_PCI_EA_H
> +#define QEMU_PCI_EA_H
> +
> +#define PCI_EA_FIELD_SIZE_FLAG 0x00000002
> +
> +/* the address' must be DWORD aligned */
> +#define PCI_EA_ADDR_MASK_32 0xfffffffc
> +#define PCI_EA_ADDR_MASK_64 0xfffffffffffffffc
> +
> +#define PCI_EA_MAX_NUM_ENTRIES 0x3f
> +
> +#define PCI_EA_BEI_OFFSET 4 /* In bits from beginning of EA entry */
> +#define PCI_EA_ENABLE 0x80000000
> +#define PCI_EA_MAX_BEI 0x8
> +#define PCI_EA_MAX_ES  0x4
> +
> +/* Primary & Secondary Properties, per table 6-1 */
> +#define PCI_EA_PP_OFFSET 8
> +#define PCI_EA_SP_OFFSET 16
> +
> +/* Primary & Secondary Properties Fields, per table 6-1 */
> +#define PCI_EA_MEM               0x00 /* Non-Prefetchable */
> +#define PCI_EA_MEM_PREFETCH      0x01
> +#define PCI_EA_IO                0x02
> +#define PCI_EA_VIRT_MEM_PREFETCH 0x03
> +#define PCI_EA_VIRT_MEM          0x04 /* Non-Prefetchable */
> +#define PCI_EA_UNAVAL_MEM        0xFD
> +#define PCI_EA_UNAVAL_IO         0xFE
> +#define PCI_EA_UNAVAL            0xFF
> +
> +/* checks to see if ea_address is invalid */
> +bool pci_ea_invalid_addr(uint64_t addr);
> +
> +/* Initialize the ea capability descriptor */
> +int pci_ea_cap_init(PCIDevice *dev);
> +
> +#endif /* QEMU_PCI_EA_REG_H */
> diff --git a/include/hw/pci/pci_regs.h b/include/hw/pci/pci_regs.h
> index 56a404b..746ba1a 100644
> --- a/include/hw/pci/pci_regs.h
> +++ b/include/hw/pci/pci_regs.h
> @@ -213,6 +213,7 @@
>  #define  PCI_CAP_ID_MSIX	0x11	/* MSI-X */
>  #define  PCI_CAP_ID_SATA	0x12	/* Serial ATA */
>  #define  PCI_CAP_ID_AF		0x13	/* PCI Advanced Features */
> +#define  PCI_CAP_ID_EA		0x14	/* PCI Enhanced Allocation */
>  #define PCI_CAP_LIST_NEXT	1	/* Next capability in the list */
>  #define PCI_CAP_FLAGS		2	/* Capability defined flags (16 bits) */
>  #define PCI_CAP_SIZEOF		4
> @@ -340,6 +341,9 @@
>  #define PCI_AF_STATUS		5
>  #define  PCI_AF_STATUS_TP	0x01
>  
> +/* PCI Enhanced Allocation registers */
> +#define PCI_EA_NUM_ENTRIES_OFFSET 2
> +
>  /* PCI-X registers */
>  
>  #define PCI_X_CMD		2	/* Modes & Features */

This register is a copy of the linux header (in fact, we probably
should move it to where imported headers are).
Pls add entries not in linux in another header.

> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index cde3314..70a9f3f 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -48,6 +48,7 @@ typedef struct PcGuestInfo PcGuestInfo;
>  typedef struct PCIBridge PCIBridge;
>  typedef struct PCIBus PCIBus;
>  typedef struct PCIDevice PCIDevice;
> +typedef struct PCIEADevice PCIEADevice;
>  typedef struct PCIEAERErr PCIEAERErr;
>  typedef struct PCIEAERLog PCIEAERLog;
>  typedef struct PCIEAERMsg PCIEAERMsg;
> -- 
> 1.9.1

Re: [Qemu-devel] [PATCH 1/1] Add support for PCI Enhanced Allocation "BARs"

[Stalley, Sean]

Thanks for reviewing, my responses are inline.

-Sean
> -----Original Message-----
> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> Sent: Monday, June 22, 2015 2:15 PM
> To: Stalley, Sean
> Cc: qemu-devel@nongnu.org
> Subject: Re: [PATCH 1/1] Add support for PCI Enhanced Allocation "BARs"
> 
> On Mon, May 11, 2015 at 11:56:44AM -0700, Sean O. Stalley wrote:
> > PCI Enhanced Allocation is a new method of allocating MMIO & IO
> > resources for PCI devices & bridges. It can be used instead of the
> > traditional PCI method of using BARs.
> >
> > EA entries are hardware-initialized to a fixed address.
> > Unlike BARs, regions described by EA are cannot be moved.
> > Because of this, only devices which are perminately connected to the
> > PCI bus can use EA. A removeable PCI card must not use EA.
> >
> > This patchset enables any existing QEMU PCI model to use EA in leiu of
> 
> s/in leiu/instead of/
> 
> if you can't spell it, don't use it :)

Fair enough. I'll run the whole patch through a spell checker & see if I missed anything else.

> > BARs. It adds EA options to the PCI device paramaters.
> 
> parameters
> 
> >
> > The Enhanced Allocation ECN is publicly available here:
> >
> https://www.pcisig.com/specifications/conventional/ECN_Enhanced_Alloca
> > tion_23_Oct_2014_Final.pdf
> >
> > Signed-off-by: Sean O. Stalley <sean.stalley@intel.com>
> > ---
> >  hw/pci/Makefile.objs      |   2 +-
> >  hw/pci/pci.c              |  96 ++++++++++++++++------
> >  hw/pci/pci_ea.c           | 203
> ++++++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/pci/pci.h      |   7 ++
> >  include/hw/pci/pci_ea.h   |  39 +++++++++
> >  include/hw/pci/pci_regs.h |   4 +
> >  include/qemu/typedefs.h   |   1 +
> >  7 files changed, 328 insertions(+), 24 deletions(-)  create mode
> > 100644 hw/pci/pci_ea.c  create mode 100644 include/hw/pci/pci_ea.h
> >
> > diff --git a/hw/pci/Makefile.objs b/hw/pci/Makefile.objs index
> > 9f905e6..e5d80cf 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 pci_ea.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 b3d5100..c7552ca 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -23,6 +23,7 @@
> >   */
> >  #include "hw/hw.h"
> >  #include "hw/pci/pci.h"
> > +#include "hw/pci/pci_ea.h"
> >  #include "hw/pci/pci_bridge.h"
> >  #include "hw/pci/pci_bus.h"
> >  #include "hw/pci/pci_host.h"
> > @@ -58,6 +59,10 @@ static Property pci_props[] = {
> >                      QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false),
> >      DEFINE_PROP_BIT("command_serr_enable", PCIDevice, cap_present,
> >                      QEMU_PCI_CAP_SERR_BITNR, true),
> > +    DEFINE_PROP_BOOL("enhanced_allocation", PCIDevice, enhanced,
> false),
> > +    DEFINE_PROP_ARRAY("ea_addr", PCIDevice, ea_addr_len, ea_addr,
> > +                      qdev_prop_uint64, hwaddr),
> > +
> >      DEFINE_PROP_END_OF_LIST()
> >  };
> >
> 
> Hmm, why do we need the bool property? Can't we just check that ea_addr
> is specified?

Your right, It doesn't make a whole lot of sense to have a bool if we can just check for the array.
I'll pull the Bool property out of the next patch.
 
> > @@ -882,6 +887,7 @@ static PCIDevice
> *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
> >          config_read = pci_default_read_config;
> >      if (!config_write)
> >          config_write = pci_default_write_config;
> > +
> >      pci_dev->config_read = config_read;
> >      pci_dev->config_write = config_write;
> >      bus->devices[devfn] = pci_dev;
> 
> don't add random whitspace please.

Sorry. Will remove in next version.

> 
> > @@ -929,40 +935,72 @@ void pci_register_bar(PCIDevice *pci_dev, int
> > region_num,
> >
> >      assert(region_num >= 0);
> >      assert(region_num < PCI_NUM_REGIONS);
> > -    if (size & (size-1)) {
> > +
> > +
> > +    /* TODO: these checks should be done earlier */
> > +    if (!pci_dev->enhanced && (size & (size-1))) {
> >          fprintf(stderr, "ERROR: PCI region size must be pow2 "
> >                      "type=0x%x, size=0x%"FMT_PCIBUS"\n", type, size);
> >          exit(1);
> >      }
> >
> > +    if (pci_dev->enhanced) {
> > +
> > +        if (pci_dev->ea_addr_len <= region_num) {
> > +            fprintf(stderr, "ERROR: Address for EA entry %i not given\n",
> > +                    region_num);
> > +            exit(1);
> > +        }
> > +
> > +        if (pci_dev->ea_addr[region_num] == 0x0) {
> > +            fprintf(stderr, "ERROR: Address for EA entry %i not set\n",
> > +                    region_num);
> > +            exit(1);
> > +        }
> 
> So this means that the management tool needs to know the BAR number
> used by the device. It also needs to know how much memory to request.
> Which is all very low level, but at least more or less stable for emulated
> devices, but we generally were free to change them for e.g. virtio devices.
> 
> Isn't it sufficient to just enable ea?
> Implement a global allocator, and have each device allocate space according
> to BAR size.
> We might need to use some tricks to make this stable when order of
> initialization changes (sort by name?) but otherwise - any problems with
> this?

What you are suggesting would work, but would limit the amount of testing we could do.
Being able to move the EA regions around is useful for testing firmware & the OS.
For example, we can move around a region & make sure the ACPI table is set correctly.
We can also use it to test device drivers & see if they work when the MMIO isn't aligned like a BAR would be.

I'm assuming (and this may be a bad assumption) that if the user is enabling EA, they want to test EA.
If the user wants to use a global allocator, they can just disable EA & have firmware do it.

> 
> 
> > +
> > +        if (pci_ea_invalid_addr(pci_dev->ea_addr[region_num])) {
> > +            fprintf(stderr, "ERROR: Address for EA entry %i not valid\n",
> > +                    region_num);
> > +            exit(1);
> > +        }
> > +    }
> > +
> >      r = &pci_dev->io_regions[region_num];
> >      r->addr = PCI_BAR_UNMAPPED;
> >      r->size = size;
> >      r->type = type;
> > -    r->memory = NULL;
> > -
> > -    wmask = ~(size - 1);
> > -    addr = pci_bar(pci_dev, region_num);
> > -    if (region_num == PCI_ROM_SLOT) {
> > -        /* ROM enable bit is writable */
> > -        wmask |= PCI_ROM_ADDRESS_ENABLE;
> > -    }
> > -    pci_set_long(pci_dev->config + addr, type);
> > -    if (!(r->type & PCI_BASE_ADDRESS_SPACE_IO) &&
> > -        r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> > -        pci_set_quad(pci_dev->wmask + addr, wmask);
> > -        pci_set_quad(pci_dev->cmask + addr, ~0ULL);
> > -    } else {
> > -        pci_set_long(pci_dev->wmask + addr, wmask & 0xffffffff);
> > -        pci_set_long(pci_dev->cmask + addr, 0xffffffff);
> > +    r->enhanced = pci_dev->enhanced;
> > +    r->memory = memory;
> > +    r->address_space = type & PCI_BASE_ADDRESS_SPACE_IO ?
> > +        pci_dev->bus->address_space_io :
> > + pci_dev->bus->address_space_mem;
> > +
> > +    if (!pci_dev->enhanced) {
> > +
> > +        wmask = ~(size - 1);
> > +        addr = pci_bar(pci_dev, region_num);
> > +        if (region_num == PCI_ROM_SLOT) {
> > +            /* ROM enable bit is writable */
> > +            wmask |= PCI_ROM_ADDRESS_ENABLE;
> > +        }
> > +        pci_set_long(pci_dev->config + addr, type);
> > +        if (!(r->type & PCI_BASE_ADDRESS_SPACE_IO) &&
> > +            r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> > +            pci_set_quad(pci_dev->wmask + addr, wmask);
> > +            pci_set_quad(pci_dev->cmask + addr, ~0ULL);
> > +        } else {
> > +            pci_set_long(pci_dev->wmask + addr, wmask & 0xffffffff);
> > +            pci_set_long(pci_dev->cmask + addr, 0xffffffff);
> > +        }
> > +    } else { /* enhanced */
> > +
> > +        /* add the memory space now */
> > +        r->addr = pci_dev->ea_addr[region_num];
> > +        memory_region_add_subregion(r->address_space, r->addr,
> > + r->memory);
> >      }
> > -    pci_dev->io_regions[region_num].memory = memory;
> > -    pci_dev->io_regions[region_num].address_space
> > -        = type & PCI_BASE_ADDRESS_SPACE_IO
> > -        ? pci_dev->bus->address_space_io
> > -        : pci_dev->bus->address_space_mem;
> >  }
> >
> > +
> > +
> >  static void pci_update_vga(PCIDevice *pci_dev)  {
> >      uint16_t cmd;
> > @@ -1107,6 +1145,11 @@ static void pci_update_mappings(PCIDevice
> *d)
> >
> >          new_addr = pci_bar_address(d, i, r->type, r->size);
> >
> > +        /* EA BARs cannot be moved */
> > +        if (r->enhanced) {
> > +            continue;
> > +        }
> > +
> >          /* This bar isn't changed */
> >          if (new_addr == r->addr)
> >              continue;
> > @@ -1785,8 +1828,9 @@ static void pci_qdev_realize(DeviceState
> *qdev, Error **errp)
> >      pci_dev = do_pci_register_device(pci_dev, bus,
> >                                       object_get_typename(OBJECT(qdev)),
> >                                       pci_dev->devfn, errp);
> > -    if (pci_dev == NULL)
> > +    if (pci_dev == NULL) {
> >          return;
> > +    }
> >
> >      if (pc->realize) {
> >          pc->realize(pci_dev, &local_err); @@ -1797,6 +1841,12 @@
> > static void pci_qdev_realize(DeviceState *qdev, Error **errp)
> >          }
> >      }
> >
> > +    /* write the EA entry */
> > +    if (pci_dev->enhanced) {
> > +        pci_ea_cap_init(pci_dev);
> > +    }
> > +
> > +
> >      /* rom loading */
> >      is_default_rom = false;
> >      if (pci_dev->romfile == NULL && pc->romfile != NULL) { diff --git
> > a/hw/pci/pci_ea.c b/hw/pci/pci_ea.c new file mode 100644 index
> > 0000000..a052519
> > --- /dev/null
> > +++ b/hw/pci/pci_ea.c
> > @@ -0,0 +1,203 @@
> > +/*
> > + * PCI Enhanced Allocation (EA) Helper functions
> > + * Copyright (c) 2015, Intel Corporation.
> > + *
> > + * Written by Sean O. Stalley <sean.stalley@intel.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > +modify it
> > + * under the terms and conditions of the GNU General Public License,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope it will be useful, but
> > +WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY
> > +or
> > + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
> > +License for
> > + * more details.
> > + */
> > +
> > +/*
> > + * Contians a lot of code taken from pcie.c  */
> > +
> > +#include "qemu-common.h"
> > +#include "hw/pci/pci.h"
> > +#include "hw/pci/pci_ea.h"
> > +
> > +bool pci_ea_invalid_addr(uint64_t addr) {
> > +    return (addr != (addr & PCI_EA_ADDR_MASK_64)); }
> > +
> > +/* determines if a 64 bit address is necessary for this address */
> > +static bool pci_ea_addr_is_64(uint64_t addr) {
> > +    return (addr != (uint32_t)addr);
> > +}
> 
> don't put () after return pls.
> 

I'll remove the () them in these 2 functions.

> > +
> > +/* Sets the first 32 bits of the Base or MaxOffset field
> > + * Returns the length of the address set  */ static uint8_t
> > +pci_ea_set_addr(PCIDevice *dev, uint8_t pos, uint64_t val) {
> > +    uint8_t *current_reg = dev->config + pos;
> > +
> > +    val = val & PCI_EA_ADDR_MASK_64;
> > +
> > +    if (pci_ea_addr_is_64(val)) {
> > +        val |= PCI_EA_FIELD_SIZE_FLAG;
> > +    }
> > +
> > +    pci_set_long(current_reg, val);
> > +    return sizeof(int32_t);
> > +}
> > +
> > +/* Determines EA entry length for this IORegion in bytes
> > + * (including the first DWORD) */
> > +static int pci_ea_entry_length(PCIIORegion *r) {
> > +
> > +    int len = 3 * sizeof(int32_t); /* First 32 bits of Base & Offset
> > + */
> > +
> > +    if (pci_ea_addr_is_64(r->addr)) {
> > +        len += sizeof(int32_t); /* Base (if 64 bit) */
> > +    }
> > +
> > +    if (pci_ea_addr_is_64(r->size - 1)) {
> > +        len += sizeof(int32_t); /* Offset (if 64 bit) */
> > +    }
> > +
> > +    return len;
> > +
> > +}
> > +
> > +/* determines the value to be set in the Entry Size field */ static
> > +int pci_ea_get_es(PCIIORegion *r) {
> > +
> > +    int es = (pci_ea_entry_length(r) / sizeof(int32_t)) - 1;
> > +
> > +    assert(PCI_EA_MAX_ES >= es);
> > +
> > +    return es;
> > +}
> > +
> > +/* Initialize an EA entry for the given PCIIORegion
> > + *
> > + * @dev: The PCI Device
> > + * @bei: The BAR Equivalent Indicator
> > + * @pos: The offset of this entry in PCI Configspace;  */
> > +
> 
> no need for an empty line here.
> 

Will remove in next patch.

> > +static int pci_ea_fill_entry(PCIDevice *dev, int bei, int pos) {
> > +    PCIIORegion *r = &dev->io_regions[bei];
> > +    uint32_t dw0 = 0; /* used for setting first DWORD */
> > +    int len = 0;
> > +
> > +    assert(bei <= PCI_EA_MAX_BEI);
> > +
> > +    /* Check that the base and size are DWORD aligned */
> > +    assert(!pci_ea_invalid_addr(r->addr));
> > +    assert(!pci_ea_invalid_addr(r->size));
> > +
> > +    dw0 |= pci_ea_get_es(r);
> > +    dw0 |= (bei << PCI_EA_BEI_OFFSET);
> > +    dw0 |= (PCI_EA_ENABLE);
> > +
> > +    /* base Primary Properties off of type field */
> > +    if (r->type & PCI_BASE_ADDRESS_SPACE_IO) {
> > +        dw0 |= (PCI_EA_IO << PCI_EA_PP_OFFSET);
> > +
> > +    } else if (r->type & PCI_BASE_ADDRESS_MEM_PREFETCH) {
> > +        dw0 |= (PCI_EA_MEM_PREFETCH << PCI_EA_PP_OFFSET);
> > +
> > +    } else {
> > +        dw0 |= (PCI_EA_MEM << PCI_EA_PP_OFFSET);
> > +    }
> > +
> > +    /* Secondary Properties should be ignored */
> > +    dw0 |= (PCI_EA_UNAVAL << PCI_EA_SP_OFFSET);
> > +
> > +    pci_set_long(dev->config + pos, dw0);
> > +
> > +    len += sizeof(int32_t);
> > +
> > +    len += pci_ea_set_addr(dev, pos + len, r->addr);
> > +    len += pci_ea_set_addr(dev, pos + len, r->size - 1);
> > +
> > +    /* Base (if 64 bit) */
> > +    if (pci_ea_addr_is_64(r->addr)) {
> > +        pci_set_long(dev->config + pos + len, (r->addr >> 32));
> > +        len += sizeof(int32_t);
> > +    }
> > +
> > +    /* Offset (if 64 bit) */
> > +    if (pci_ea_addr_is_64(r->size - 4)) {
> > +        pci_set_long(dev->config + pos + len, (r->size >> 32));
> > +        len += sizeof(int32_t);
> > +    }
> > +
> > +    assert(len == pci_ea_entry_length(r));
> > +
> > +    return len;
> > +
> > +}
> > +
> > +/*
> > + * Initialize the EA capability descriptor
> > + * must be done after the EA memory regions are initialized
> > + * (or else the EA entries won't get written)  */ int
> > +pci_ea_cap_init(PCIDevice *dev) {
> > +    int pos;
> > +    int ret;
> > +    int i;
> > +    int num_entries = 0;
> > +    uint8_t length; /* length of this cap entry */
> > +    PCIIORegion *r;
> > +
> > +    /* Determine the length of the capability entry */
> > +
> > +    length = sizeof(int32_t); /* DWORD 0: Cap ID, Next Pointer, Num
> Entries */
> > +    /* TODO: DWORD 1 for Type 1 Functions */
> > +
> > +    /* add the length of every entry */
> > +    for (i = 0; i < PCI_NUM_REGIONS; ++i) {
> > +        r = &dev->io_regions[i];
> > +
> > +        if (r->enhanced) {
> > +            num_entries++;
> > +            length += pci_ea_entry_length(r);
> > +        }
> > +    }
> > +
> > +    /* add the EA CAP (sets DWORD 0) */
> > +    pos = pci_add_capability(dev, PCI_CAP_ID_EA, 0, length);
> > +    if (0 > pos) {
> > +        return pos;
> > +    }
> > +
> > +    /* DWORD 0: Cap ID, Next Pointer, Num Entries */
> > +    assert(num_entries <= PCI_EA_MAX_NUM_ENTRIES); /* overflow */
> > +    pci_set_byte(dev->config + pos + PCI_EA_NUM_ENTRIES_OFFSET,
> num_entries);
> > +    pos += sizeof(int32_t);
> > +
> > +    for (i = 0; i < PCI_NUM_REGIONS; ++i) {
> > +        r = &dev->io_regions[i];
> > +
> > +        if (r->enhanced) {
> > +            ret = pci_ea_fill_entry(dev, i, pos);
> > +
> > +            if (0 > ret) {
> > +                return ret;
> > +            }
> > +            pos += ret;
> > +        }
> > +
> > +    }
> > +
> > +    assert(pos == pci_find_capability(dev, PCI_CAP_ID_EA) + length);
> > +
> > +    return pos;
> > +
> > +}
> > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h index
> > d4ffead..da64435 100644
> > --- a/include/hw/pci/pci.h
> > +++ b/include/hw/pci/pci.h
> > @@ -12,6 +12,7 @@
> >  #include "hw/isa/isa.h"
> >
> >  #include "hw/pci/pcie.h"
> > +#include "hw/pci/pci_ea.h"
> >
> >  /* PCI bus */
> >
> > @@ -109,6 +110,7 @@ typedef struct PCIIORegion {
> >      uint8_t type;
> >      MemoryRegion *memory;
> >      MemoryRegion *address_space;
> > +    bool          enhanced;
> >  } PCIIORegion;
> >
> >  #define PCI_ROM_SLOT 6
> > @@ -288,6 +290,11 @@ struct PCIDevice {
> >      /* PCI Express */
> >      PCIExpressDevice exp;
> >
> > +    /* Enhanced Allocation */
> > +    bool enhanced;
> > +    uint32_t ea_addr_len;
> > +    hwaddr *ea_addr;
> > +
> >      /* SHPC */
> >      SHPCDevice *shpc;
> >
> > diff --git a/include/hw/pci/pci_ea.h b/include/hw/pci/pci_ea.h new
> > file mode 100644 index 0000000..2b63012
> > --- /dev/null
> > +++ b/include/hw/pci/pci_ea.h
> > @@ -0,0 +1,39 @@
> > +/* Constants for pci enhanced allocation (EA) capability descriptor
> > +*/
> > +
> > +#ifndef QEMU_PCI_EA_H
> > +#define QEMU_PCI_EA_H
> > +
> > +#define PCI_EA_FIELD_SIZE_FLAG 0x00000002
> > +
> > +/* the address' must be DWORD aligned */ #define
> PCI_EA_ADDR_MASK_32
> > +0xfffffffc #define PCI_EA_ADDR_MASK_64 0xfffffffffffffffc
> > +
> > +#define PCI_EA_MAX_NUM_ENTRIES 0x3f
> > +
> > +#define PCI_EA_BEI_OFFSET 4 /* In bits from beginning of EA entry */
> > +#define PCI_EA_ENABLE 0x80000000 #define PCI_EA_MAX_BEI 0x8
> #define
> > +PCI_EA_MAX_ES  0x4
> > +
> > +/* Primary & Secondary Properties, per table 6-1 */ #define
> > +PCI_EA_PP_OFFSET 8 #define PCI_EA_SP_OFFSET 16
> > +
> > +/* Primary & Secondary Properties Fields, per table 6-1 */
> > +#define PCI_EA_MEM               0x00 /* Non-Prefetchable */
> > +#define PCI_EA_MEM_PREFETCH      0x01
> > +#define PCI_EA_IO                0x02
> > +#define PCI_EA_VIRT_MEM_PREFETCH 0x03
> > +#define PCI_EA_VIRT_MEM          0x04 /* Non-Prefetchable */
> > +#define PCI_EA_UNAVAL_MEM        0xFD
> > +#define PCI_EA_UNAVAL_IO         0xFE
> > +#define PCI_EA_UNAVAL            0xFF
> > +
> > +/* checks to see if ea_address is invalid */ bool
> > +pci_ea_invalid_addr(uint64_t addr);
> > +
> > +/* Initialize the ea capability descriptor */ int
> > +pci_ea_cap_init(PCIDevice *dev);
> > +
> > +#endif /* QEMU_PCI_EA_REG_H */
> > diff --git a/include/hw/pci/pci_regs.h b/include/hw/pci/pci_regs.h
> > index 56a404b..746ba1a 100644
> > --- a/include/hw/pci/pci_regs.h
> > +++ b/include/hw/pci/pci_regs.h
> > @@ -213,6 +213,7 @@
> >  #define  PCI_CAP_ID_MSIX	0x11	/* MSI-X */
> >  #define  PCI_CAP_ID_SATA	0x12	/* Serial ATA */
> >  #define  PCI_CAP_ID_AF		0x13	/* PCI Advanced Features */
> > +#define  PCI_CAP_ID_EA		0x14	/* PCI Enhanced Allocation
> */
> >  #define PCI_CAP_LIST_NEXT	1	/* Next capability in the list */
> >  #define PCI_CAP_FLAGS		2	/* Capability defined flags
> (16 bits) */
> >  #define PCI_CAP_SIZEOF		4
> > @@ -340,6 +341,9 @@
> >  #define PCI_AF_STATUS		5
> >  #define  PCI_AF_STATUS_TP	0x01
> >
> > +/* PCI Enhanced Allocation registers */ #define
> > +PCI_EA_NUM_ENTRIES_OFFSET 2
> > +
> >  /* PCI-X registers */
> >
> >  #define PCI_X_CMD		2	/* Modes & Features */
> 
> This register is a copy of the linux header (in fact, we probably should move
> it to where imported headers are).
> Pls add entries not in linux in another header.

I thought this file looked familiar :)
I can put these definitions in pci_ea.h

> 
> > diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h index
> > cde3314..70a9f3f 100644
> > --- a/include/qemu/typedefs.h
> > +++ b/include/qemu/typedefs.h
> > @@ -48,6 +48,7 @@ typedef struct PcGuestInfo PcGuestInfo;  typedef
> > struct PCIBridge PCIBridge;  typedef struct PCIBus PCIBus;  typedef
> > struct PCIDevice PCIDevice;
> > +typedef struct PCIEADevice PCIEADevice;
> >  typedef struct PCIEAERErr PCIEAERErr;  typedef struct PCIEAERLog
> > PCIEAERLog;  typedef struct PCIEAERMsg PCIEAERMsg;
> > --
> > 1.9.1