Patchwork [V5,25/29] pci: add helper functions for pci config write function.

login
register
mail settings
Submitter Isaku Yamahata
Date Oct. 9, 2009, 6:28 a.m.
Message ID <1255069742-15724-26-git-send-email-yamahata@valinux.co.jp>
Download mbox | patch
Permalink /patch/35574/
State Under Review
Headers show

Comments

Isaku Yamahata - Oct. 9, 2009, 6:28 a.m.
add helper functions for pci config write function to check
if its configuration space is changed.
To detect the change in configuration space, memcpy the original
value and memcmp with updated value.
The original value is allocated in PCIDevice because its length
might be 4K for pci express which is a bit too large for stack.

Those functions will be used later and generic enough for specific
pci device to use.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci.c |    2 ++
 hw/pci.h |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 0 deletions(-)
Michael S. Tsirkin - Oct. 10, 2009, 7:58 p.m.
On Fri, Oct 09, 2009 at 03:28:58PM +0900, Isaku Yamahata wrote:
> add helper functions for pci config write function to check
> if its configuration space is changed.
> To detect the change in configuration space, memcpy the original
> value and memcmp with updated value.
> The original value is allocated in PCIDevice because its length
> might be 4K for pci express which is a bit too large for stack.
> 
> Those functions will be used later and generic enough for specific
> pci device to use.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  hw/pci.c |    2 ++
>  hw/pci.h |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 52 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 8e396b6..ece429f 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -446,6 +446,7 @@ static void pci_config_alloc(PCIDevice *pci_dev)
>      pci_dev->cmask = qemu_mallocz(config_size);
>      pci_dev->wmask = qemu_mallocz(config_size);
>      pci_dev->used = qemu_mallocz(config_size);
> +    pci_dev->orig = qemu_mallocz(config_size);
>  }
>  
>  static void pci_config_free(PCIDevice *pci_dev)
> @@ -454,6 +455,7 @@ static void pci_config_free(PCIDevice *pci_dev)
>      qemu_free(pci_dev->cmask);
>      qemu_free(pci_dev->wmask);
>      qemu_free(pci_dev->used);
> +    qemu_free(pci_dev->orig);
>  }
>  
>  /* -1 for devfn means auto assign */
> diff --git a/hw/pci.h b/hw/pci.h
> index bfa29c8..2896b0e 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -192,6 +192,9 @@ struct PCIDevice {
>      /* Used to allocate config space for capabilities. */
>      uint8_t *used;
>  
> +    /* Used to implement configuration space change */
> +    uint8_t *orig;
> +
>      /* the following fields are read only */
>      PCIBus *bus;
>      uint32_t devfn;
> @@ -388,6 +391,53 @@ static inline uint32_t pci_config_size(PCIDevice *d)
>      return pci_is_express(d) ? PCIE_CONFIG_SPACE_SIZE : PCI_CONFIG_SPACE_SIZE;
>  }
>  
> +struct pci_config_update {
> +    PCIDevice *d;
> +    uint32_t addr;
> +    uint32_t val;
> +    int len;
> +};
> +
> +static inline void pci_write_config_init(struct pci_config_update *update,
> +                                         PCIDevice *d,
> +                                         uint32_t addr, uint32_t val, int len)
> +{
> +    update->d = d;
> +    update->addr = addr;
> +    update->val = val;
> +    update->len = len;
> +    memcpy(d->orig, d->config, pci_config_size(d));
> +}
> +

I think this is a bad API, because it is not re-entrant:
If someone calls pci_write_config_init and then default_write_config
which calls pci_write_config_init internally, everything breaks.

What happens if default write onfig will just update regions
on each write into PCI header? That would simplify code very much.

> +static inline void pci_write_config_update(struct pci_config_update *update)
> +{
> +    PCIDevice *d = update->d;
> +    uint32_t addr = update->addr;
> +    uint32_t val = update->val;
> +    uint32_t config_size = pci_config_size(d);
> +    int i;
> +
> +    for(i = 0; i < update->len && addr < config_size; val >>= 8, ++i, ++addr) {
> +        uint8_t wmask = d->wmask[addr];
> +        d->config[addr] = (d->config[addr] & ~wmask) | (val & wmask);
> +    }
> +}
> +
> +/* check if [base, end) in configuration space is changed */
> +static inline int pci_config_changed(const struct pci_config_update *update,
> +                                     uint32_t base, uint32_t end)
> +{
> +    return memcmp(update->d->orig + base, update->d->config + base,
> +                  end - base);
> +}
> +
> +/* for convinience not to type symbol constant twice */
> +static inline int pci_config_changed_with_size(
> +    const struct pci_config_update *update, uint32_t base, uint32_t size)
> +{
> +    return pci_config_changed(update, base, base + size);
> +}
> +
>  /* lsi53c895a.c */
>  #define LSI_MAX_DEVS 7
>  
> -- 
> 1.6.0.2

Patch

diff --git a/hw/pci.c b/hw/pci.c
index 8e396b6..ece429f 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -446,6 +446,7 @@  static void pci_config_alloc(PCIDevice *pci_dev)
     pci_dev->cmask = qemu_mallocz(config_size);
     pci_dev->wmask = qemu_mallocz(config_size);
     pci_dev->used = qemu_mallocz(config_size);
+    pci_dev->orig = qemu_mallocz(config_size);
 }
 
 static void pci_config_free(PCIDevice *pci_dev)
@@ -454,6 +455,7 @@  static void pci_config_free(PCIDevice *pci_dev)
     qemu_free(pci_dev->cmask);
     qemu_free(pci_dev->wmask);
     qemu_free(pci_dev->used);
+    qemu_free(pci_dev->orig);
 }
 
 /* -1 for devfn means auto assign */
diff --git a/hw/pci.h b/hw/pci.h
index bfa29c8..2896b0e 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -192,6 +192,9 @@  struct PCIDevice {
     /* Used to allocate config space for capabilities. */
     uint8_t *used;
 
+    /* Used to implement configuration space change */
+    uint8_t *orig;
+
     /* the following fields are read only */
     PCIBus *bus;
     uint32_t devfn;
@@ -388,6 +391,53 @@  static inline uint32_t pci_config_size(PCIDevice *d)
     return pci_is_express(d) ? PCIE_CONFIG_SPACE_SIZE : PCI_CONFIG_SPACE_SIZE;
 }
 
+struct pci_config_update {
+    PCIDevice *d;
+    uint32_t addr;
+    uint32_t val;
+    int len;
+};
+
+static inline void pci_write_config_init(struct pci_config_update *update,
+                                         PCIDevice *d,
+                                         uint32_t addr, uint32_t val, int len)
+{
+    update->d = d;
+    update->addr = addr;
+    update->val = val;
+    update->len = len;
+    memcpy(d->orig, d->config, pci_config_size(d));
+}
+
+static inline void pci_write_config_update(struct pci_config_update *update)
+{
+    PCIDevice *d = update->d;
+    uint32_t addr = update->addr;
+    uint32_t val = update->val;
+    uint32_t config_size = pci_config_size(d);
+    int i;
+
+    for(i = 0; i < update->len && addr < config_size; val >>= 8, ++i, ++addr) {
+        uint8_t wmask = d->wmask[addr];
+        d->config[addr] = (d->config[addr] & ~wmask) | (val & wmask);
+    }
+}
+
+/* check if [base, end) in configuration space is changed */
+static inline int pci_config_changed(const struct pci_config_update *update,
+                                     uint32_t base, uint32_t end)
+{
+    return memcmp(update->d->orig + base, update->d->config + base,
+                  end - base);
+}
+
+/* for convinience not to type symbol constant twice */
+static inline int pci_config_changed_with_size(
+    const struct pci_config_update *update, uint32_t base, uint32_t size)
+{
+    return pci_config_changed(update, base, base + size);
+}
+
 /* lsi53c895a.c */
 #define LSI_MAX_DEVS 7