[Qemu-devel] Re: [PATCH V5 25/29] pci: add helper functions for pci config write function.

["Michael S. Tsirkin" <mst@redhat.com>]

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