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

[Isaku Yamahata]

On Thu, Oct 08, 2009 at 11:29:11AM +0200, Michael S. Tsirkin wrote:
> On Thu, Oct 08, 2009 at 09:08:07AM +0900, Isaku Yamahata wrote:
> > 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.
> 
> So maybe we should get away from both memcpy and range checks.  How
> about the following?  Note: it's untested, and we should see what this
> does to boot times. Quite likely nothing. I'd like to see some number on
> how many times does PCI header get written to during boot. I might check
> it myself but not anytime soon.

Looks good. I'll go for this approach.

thanks,

> Not-Quite-Signed-off-by: Michael S. Tsirkin <address@hidden>
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 1debdab..19b8cc6 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -583,18 +583,14 @@ 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];
>      int i;
>  
>      /* not efficient, but simple */
> -    memcpy(orig, d->config, PCI_CONFIG_SPACE_SIZE);
>      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);
>      }
> -    if (memcmp(orig + PCI_BASE_ADDRESS_0, d->config + PCI_BASE_ADDRESS_0, 24)
> -        || ((orig[PCI_COMMAND] ^ d->config[PCI_COMMAND])
> -            & (PCI_COMMAND_MEMORY | PCI_COMMAND_IO)))
> +    if (ranges_overlap(addr, l, PCI_COMMAND, PCI_ROM_ADDRESS + 3))
>          pci_update_mappings(d);
>  }
>  
> diff --git a/hw/pci.h b/hw/pci.h
> index 93f93fb..b9374d1 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -366,4 +366,27 @@ PCIBus *pci_apb_init(target_phys_addr_t special_base,
>  PCIBus *sh_pci_register_bus(pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
>                              void *pic, int devfn_min, int nirq);
>  
> +/* These are not pci specific. Should move into a separate header. */
> +
> +/* Check whether a given range covers a given byte. */
> +static inline int range_covers_byte(uint64_t first, uint64_t last,
> +                                 uint64_t byte)
> +{
> +     return first <= byte && last >= byte;
> +}
> +
> +/* Check whether 2 given ranges overlap. Undefined if last < first. */
> +static inline int ranges_overlap(uint64_t first1, uint64_t last1,
> +                              uint64_t first2, uint64_t last2)
> +{
> +     return first1 >= last2 && first2 >= last1;
> +}
> +
> +/* Get last byte of a range from offset + length.
> + * Undefined for ranges that wrap around 0. */
> +static inline uint64_t range_get_last(uint64_t offset, uint64_t len)
> +{
> +     return offset + len - 1;
> +}
> +
>  #endif
> 

-- 
yamahata