diff mbox

[04/10] Support for device capability

Message ID 1235982573-6932-5-git-send-email-sheng@linux.intel.com (mailing list archive)
State Accepted
Headers show

Commit Message

Sheng Yang March 2, 2009, 8:29 a.m. UTC 
This framework can be easily extended to support device capability, like
MSI/MSI-x.

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 qemu/hw/pci.c |   85 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu/hw/pci.h |   30 ++++++++++++++++++++
 2 files changed, 115 insertions(+), 0 deletions(-)

Comments

Marcelo Tosatti March 3, 2009, 12:42 p.m. UTC | #1 
Hi Sheng,

On Mon, Mar 02, 2009 at 04:29:27PM +0800, Sheng Yang wrote:
> This framework can be easily extended to support device capability, like
> MSI/MSI-x.
> 
> Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> ---
>  qemu/hw/pci.c |   85 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qemu/hw/pci.h |   30 ++++++++++++++++++++
>  2 files changed, 115 insertions(+), 0 deletions(-)
> 

> @@ -205,6 +215,15 @@ struct PCIDevice {
>  
>      /* Current IRQ levels.  Used internally by the generic PCI code.  */
>      int irq_state[4];
> +
> +    /* Device capability configuration space */
> +    struct {
> +        int supported;
> +        uint8_t config[PCI_CAPABILITY_CONFIG_MAX_LENGTH];
> +        unsigned int start, length;
> +        PCICapConfigReadFunc *config_read;
> +        PCICapConfigWriteFunc *config_write;
> +    } cap;
>  };

Why do you have a copy of the capabilities config space? Why not just
access PCIDevice->config directly?

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sheng Yang March 4, 2009, 2:56 a.m. UTC | #2 
On Tuesday 03 March 2009 20:42:07 Marcelo Tosatti wrote:
> Hi Sheng,
>
> On Mon, Mar 02, 2009 at 04:29:27PM +0800, Sheng Yang wrote:
> > This framework can be easily extended to support device capability, like
> > MSI/MSI-x.
> >
> > Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> > ---
> >  qemu/hw/pci.c |   85
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ qemu/hw/pci.h |
> >   30 ++++++++++++++++++++
> >  2 files changed, 115 insertions(+), 0 deletions(-)
> >
> >
> > @@ -205,6 +215,15 @@ struct PCIDevice {
> >
> >      /* Current IRQ levels.  Used internally by the generic PCI code.  */
> >      int irq_state[4];
> > +
> > +    /* Device capability configuration space */
> > +    struct {
> > +        int supported;
> > +        uint8_t config[PCI_CAPABILITY_CONFIG_MAX_LENGTH];
> > +        unsigned int start, length;
> > +        PCICapConfigReadFunc *config_read;
> > +        PCICapConfigWriteFunc *config_write;
> > +    } cap;
> >  };
>
> Why do you have a copy of the capabilities config space? Why not just
> access PCIDevice->config directly?

I am not sure upstream would accept which. Separate the logic seems more clear 
and easy to accept to me...
Marcelo Tosatti March 4, 2009, 1:57 p.m. UTC | #3 
On Wed, Mar 04, 2009 at 10:56:36AM +0800, Sheng Yang wrote:
> On Tuesday 03 March 2009 20:42:07 Marcelo Tosatti wrote:
> > Hi Sheng,
> >
> > On Mon, Mar 02, 2009 at 04:29:27PM +0800, Sheng Yang wrote:
> > > This framework can be easily extended to support device capability, like
> > > MSI/MSI-x.
> > >
> > > Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> > > ---
> > >  qemu/hw/pci.c |   85
> > > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ qemu/hw/pci.h |
> > >   30 ++++++++++++++++++++
> > >  2 files changed, 115 insertions(+), 0 deletions(-)
> > >
> > >
> > > @@ -205,6 +215,15 @@ struct PCIDevice {
> > >
> > >      /* Current IRQ levels.  Used internally by the generic PCI code.  */
> > >      int irq_state[4];
> > > +
> > > +    /* Device capability configuration space */
> > > +    struct {
> > > +        int supported;
> > > +        uint8_t config[PCI_CAPABILITY_CONFIG_MAX_LENGTH];
> > > +        unsigned int start, length;
> > > +        PCICapConfigReadFunc *config_read;
> > > +        PCICapConfigWriteFunc *config_write;
> > > +    } cap;
> > >  };
> >
> > Why do you have a copy of the capabilities config space? Why not just
> > access PCIDevice->config directly?
> 
> I am not sure upstream would accept which. Separate the logic seems more clear 
> and easy to accept to me...

You have to maintain two copies of the same data. Whats the point? The
logic can be separate with a unified config space, no? 


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/qemu/hw/pci.c b/qemu/hw/pci.c
index ccb4cfa..2225669 100644
--- a/qemu/hw/pci.c
+++ b/qemu/hw/pci.c
@@ -427,11 +427,65 @@  static void pci_update_mappings(PCIDevice *d)
     }
 }
 
+int pci_access_cap_config(PCIDevice *pci_dev, uint32_t address, int len)
+{
+    if (pci_dev->cap.supported && address >= pci_dev->cap.start &&
+            (address + len) < pci_dev->cap.start + pci_dev->cap.length)
+        return 1;
+    return 0;
+}
+
+uint32_t pci_default_cap_read_config(PCIDevice *pci_dev,
+                                     uint32_t address, int len)
+{
+    uint32_t val = 0;
+
+    if (pci_access_cap_config(pci_dev, address, len)) {
+        switch(len) {
+        default:
+        case 4:
+            if (address < pci_dev->cap.start + pci_dev->cap.length - 4) {
+                val = le32_to_cpu(*(uint32_t *)(pci_dev->cap.config
+                            + address - pci_dev->cap.start));
+                break;
+            }
+            /* fall through */
+        case 2:
+            if (address < pci_dev->cap.start + pci_dev->cap.length - 2) {
+                val = le16_to_cpu(*(uint16_t *)(pci_dev->cap.config
+                            + address - pci_dev->cap.start));
+                break;
+            }
+            /* fall through */
+        case 1:
+            val = pci_dev->cap.config[address - pci_dev->cap.start];
+            break;
+        }
+    }
+    return val;
+}
+
+void pci_default_cap_write_config(PCIDevice *pci_dev,
+                                  uint32_t address, uint32_t val, int len)
+{
+    if (pci_access_cap_config(pci_dev, address, len)) {
+        int i;
+        for (i = 0; i < len; i++) {
+            pci_dev->cap.config[address + i - pci_dev->cap.start] = val;
+            val >>= 8;
+        }
+        return;
+    }
+}
+
 uint32_t pci_default_read_config(PCIDevice *d,
                                  uint32_t address, int len)
 {
     uint32_t val;
 
+    if (pci_access_cap_config(d, address, len))
+        return d->cap.config_read(d, address, len);
+
     switch(len) {
     default:
     case 4:
@@ -485,6 +539,11 @@  void pci_default_write_config(PCIDevice *d,
         return;
     }
  default_config:
+    if (pci_access_cap_config(d, address, len)) {
+        d->cap.config_write(d, address, val, len);
+        return;
+    }
+
     /* not efficient, but simple */
     addr = address;
     for(i = 0; i < len; i++) {
@@ -903,3 +962,29 @@  PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did,
     s->bus = pci_register_secondary_bus(&s->dev, map_irq);
     return s->bus;
 }
+
+void pci_enable_capability_support(PCIDevice *pci_dev,
+                                   uint32_t config_start,
+                                   PCICapConfigReadFunc *config_read,
+                                   PCICapConfigWriteFunc *config_write,
+                                   PCICapConfigInitFunc *config_init)
+{
+    if (!pci_dev)
+        return;
+
+    if (config_start >= 0x40 && config_start < 0xff)
+        pci_dev->cap.start = config_start;
+    else
+        pci_dev->cap.start = PCI_CAPABILITY_CONFIG_DEFAULT_START_ADDR;
+    if (config_read)
+        pci_dev->cap.config_read = config_read;
+    else
+        pci_dev->cap.config_read = pci_default_cap_read_config;
+    if (config_write)
+        pci_dev->cap.config_write = config_write;
+    else
+        pci_dev->cap.config_write = pci_default_cap_write_config;
+    pci_dev->cap.supported = 1;
+    pci_dev->config[0x34] = pci_dev->cap.start;
+    config_init(pci_dev);
+}
diff --git a/qemu/hw/pci.h b/qemu/hw/pci.h
index 227da6e..7f3548b 100644
--- a/qemu/hw/pci.h
+++ b/qemu/hw/pci.h
@@ -133,6 +133,12 @@  typedef void PCIMapIORegionFunc(PCIDevice *pci_dev, int region_num,
                                 uint32_t addr, uint32_t size, int type);
 typedef int PCIUnregisterFunc(PCIDevice *pci_dev);
 
+typedef void PCICapConfigWriteFunc(PCIDevice *pci_dev,
+                                   uint32_t address, uint32_t val, int len);
+typedef uint32_t PCICapConfigReadFunc(PCIDevice *pci_dev,
+                                      uint32_t address, int len);
+typedef void PCICapConfigInitFunc(PCIDevice *pci_dev);
+
 #define PCI_ADDRESS_SPACE_MEM		0x00
 #define PCI_ADDRESS_SPACE_IO		0x01
 #define PCI_ADDRESS_SPACE_MEM_PREFETCH	0x08
@@ -183,6 +189,10 @@  typedef struct PCIIORegion {
 
 #define PCI_COMMAND_RESERVED_MASK_HI (PCI_COMMAND_RESERVED >> 8)
 
+#define PCI_CAPABILITY_CONFIG_MAX_LENGTH 0x60
+#define PCI_CAPABILITY_CONFIG_DEFAULT_START_ADDR 0x40
+#define PCI_CAPABILITY_CONFIG_MSI_LENGTH 0x10
+
 struct PCIDevice {
     /* PCI config space */
     uint8_t config[256];
@@ -205,6 +215,15 @@  struct PCIDevice {
 
     /* Current IRQ levels.  Used internally by the generic PCI code.  */
     int irq_state[4];
+
+    /* Device capability configuration space */
+    struct {
+        int supported;
+        uint8_t config[PCI_CAPABILITY_CONFIG_MAX_LENGTH];
+        unsigned int start, length;
+        PCICapConfigReadFunc *config_read;
+        PCICapConfigWriteFunc *config_write;
+    } cap;
 };
 
 PCIDevice *pci_register_device(PCIBus *bus, const char *name,
@@ -217,6 +236,12 @@  void pci_register_io_region(PCIDevice *pci_dev, int region_num,
                             uint32_t size, int type,
                             PCIMapIORegionFunc *map_func);
 
+void pci_enable_capability_support(PCIDevice *pci_dev,
+                                   uint32_t config_start,
+                                   PCICapConfigReadFunc *config_read,
+                                   PCICapConfigWriteFunc *config_write,
+                                   PCICapConfigInitFunc *config_init);
+
 int pci_map_irq(PCIDevice *pci_dev, int pin);
 uint32_t pci_default_read_config(PCIDevice *d,
                                  uint32_t address, int len);
@@ -224,6 +249,11 @@  void pci_default_write_config(PCIDevice *d,
                               uint32_t address, uint32_t val, int len);
 void pci_device_save(PCIDevice *s, QEMUFile *f);
 int pci_device_load(PCIDevice *s, QEMUFile *f);
+uint32_t pci_default_cap_read_config(PCIDevice *pci_dev,
+                                     uint32_t address, int len);
+void pci_default_cap_write_config(PCIDevice *pci_dev,
+                                  uint32_t address, uint32_t val, int len);
+int pci_access_cap_config(PCIDevice *pci_dev, uint32_t address, int len);
 
 typedef void (*pci_set_irq_fn)(qemu_irq *pic, int irq_num, int level);
 typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num);