[Qemu-devel] Re: [PATCH] qemu/pci: optimize pci config handling

[Isaku Yamahata]

On Wed, Oct 07, 2009 at 02:33:49PM +0200, Michael S. Tsirkin wrote:
> There's no need to save all of config space before each config cycle:
> just the 64 byte header is enough for our purposes.  This will become
> more important as we add pci express support, which has 4K config space.
> 
> Signed-off-by: Michael S. Tsirkin <address@hidden>
> Cc: Isaku Yamahata <address@hidden>
> ---
> 
> Isaku Yamahata, I think with this change, you can
> increase the size of config space to 4K without
> need for helper functions. Makes sense?

It is good that the patch makes the function header size
independent(256 or 4K).
However, your patch just makes the code special.
I think helper functions should be introduced eventually.
With helper function, the implementation detail/logic will be encapsulated
cleanly within them.
When I tried to introduce callback claiming ad-hoc range check is fragile,
you did suggest such helper functions would help.
My helper function is so generic that each pci device can use
and it will surely simplify them.
With your patch, each pci device doesn't get any benefit.

If your are opposing to range check claiming that we should stick
to memcpy/memcmp and aren't opposing to helper functions,
I'd like to rewrite the helper functions with memcpy/memcmp
adding new member, orig, in PCIDevice.
At least those helper functions should be generic enough such that
they can be used by not only pci_default_write_config(), but also other
pci devices. So whole configuration space would be copied.
i.e. while 256 bytes or 4Kbytes.

Although I don't see why range check is so bad as you claim,
since memcpy or range change isn't essential to me, I compromise.

thanks,

> 
>  hw/pci.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 41e99a9..5986937 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -541,11 +541,11 @@ uint32_t pci_default_read_config(PCIDevice *d,
>  
>  void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int 
> l)
>  {
> -    uint8_t orig[PCI_CONFIG_SPACE_SIZE];
> +    uint8_t orig[PCI_CONFIG_HEADER_SIZE];
>      int i;
>  
>      /* not efficient, but simple */
> -    memcpy(orig, d->config, PCI_CONFIG_SPACE_SIZE);
> +    memcpy(orig, d->config, sizeof orig);
>      for(i = 0; i < l && addr < PCI_CONFIG_SPACE_SIZE; val >>= 8, ++i, 
> ++addr) {
>          uint8_t wmask = d->wmask[addr];
>          d->config[addr] = (d->config[addr] & ~wmask) | (val & wmask);

-- 
yamahata