qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Qemu-devel] Re: [PATCH 38/61] pci: fix pci_default_write_config()


From: Michael S. Tsirkin
Subject: [Qemu-devel] Re: [PATCH 38/61] pci: fix pci_default_write_config()
Date: Wed, 30 Sep 2009 14:50:48 +0200
User-agent: Mutt/1.5.19 (2009-01-05)

On Wed, Sep 30, 2009 at 08:09:42PM +0900, Isaku Yamahata wrote:
> On Wed, Sep 30, 2009 at 12:44:46PM +0200, Michael S. Tsirkin wrote:
> > On Wed, Sep 30, 2009 at 07:18:14PM +0900, Isaku Yamahata wrote:
> > > When updated ROM expantion address of header type 0,
> > > it missed to update mappings.
> > 
> > Can the bugfix be separated from proposed optimization please?
> > It should be a one-liner.
> 
> Will do.
> 
> 
> > > By using helper functions this patch also avoids memcpy() and memcmp().
> > 
> > I expect this is all slow-path done during boot, so an extra memcpy
> > operation can not hurt.
> > I actually think keeping the original copy around is a good thing,
> > we probably should put it in PCI structure, so that devices can easily
> > check whether some value was changed.
> 
> Yes, I agree that it's slow path.
> In fact I did it having PCI express support in mind.
> PCI express has 4K bytes config space instead of 256 bytes.
> I suppose 4K is a bit large for stack/memcpy/memcmp to detect
> at most 4 bytes change.
> So Optimization to remember 4 bytes (+ alignment?) and
> compare is what you want.

I don't know if the concern is real. Isn't this premature optimization?
Assuming it is not - the way I would do this, is keep the copy of the
array inside PCI device structure (no stack), and only memcpy the
relevant bytes to keep them in sync. It should be as simple as:
        memcpy(pdev->origin + addr, pdev->config + addr, len)
memcpy is done with a small length, so it is cheap.

> 
> > For example, in your patch pci_update_mappings is called even if none of
> > the values are changed, or if memory is disabled, and
> > you also don't seem to update mappings when memory is enabled/disabled.
> 
> It's slow path, isn't it?

Yes, but you seem add code to optimize it.  FWIW, with a lot of devices,
extra scan will be much slower than a 4K memcpy which might be a single
asm instruction.

> And pci_update_mapppings() takes care of such cases.

Yes but you don't seem to call it e.g. on command write.

> 
> > > Cc: Michael S. Tsirkin <address@hidden>
> > > Signed-off-by: Isaku Yamahata <address@hidden>
> > > ---
> > >  hw/pci.c |   11 +++++------
> > >  hw/pci.h |    9 ++++++++-
> > >  2 files changed, 13 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/hw/pci.c b/hw/pci.c
> > > index 757fe7b..2a59667 100644
> > > --- a/hw/pci.c
> > > +++ b/hw/pci.c
> > > @@ -627,20 +627,19 @@ 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;
> > >      uint32_t config_size = pcie_config_size(d);
> > >  
> > > -    /* not efficient, but simple */
> > > -    memcpy(orig, d->config, PCI_CONFIG_SPACE_SIZE);
> > >      for(i = 0; i < l && addr < config_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 (pci_config_changed(addr, l,
> > > +                           PCI_BASE_ADDRESS_0, PCI_BASE_ADDRESS_5 + 4) ||
> > 
> > How can pci_config_changed know whether some value was
> > actually changed without looking at original and new value?
> > IMO it's better to keep the original array around,
> > and use simple memcmp.
> > 
> > > +        pci_config_changed_with_size(addr, l, PCI_ROM_ADDRESS, 4)) {
> > >          pci_update_mappings(d);
> > > +    }
> > >  }
> > >  
> > >  static PCIBus *pci_find_bus_from(PCIBus *from, int bus_num)
> > > diff --git a/hw/pci.h b/hw/pci.h
> > > index 26c15c5..3327905 100644
> > > --- a/hw/pci.h
> > > +++ b/hw/pci.h
> > > @@ -121,7 +121,12 @@ static inline int pci_bar_is_64bit(const PCIIORegion 
> > > *r)
> > >  #define  PCI_HEADER_TYPE_BRIDGE          1
> > >  #define  PCI_HEADER_TYPE_CARDBUS 2
> > >  #define  PCI_HEADER_TYPE_MULTI_FUNCTION 0x80
> > > -#define PCI_BASE_ADDRESS_0       0x10    /* 32 bits */
> > > +#define PCI_BASE_ADDRESS_0      0x10    /* 32 bits */
> > > +#define PCI_BASE_ADDRESS_1      0x14    /* 32 bits [htype 0,1 only] */
> > > +#define PCI_BASE_ADDRESS_2      0x18    /* 32 bits [htype 0 only] */
> > > +#define PCI_BASE_ADDRESS_3      0x1c    /* 32 bits */
> > > +#define PCI_BASE_ADDRESS_4      0x20    /* 32 bits */
> > > +#define PCI_BASE_ADDRESS_5      0x24    /* 32 bits */
> > >  #define PCI_PRIMARY_BUS          0x18    /* Primary bus number */
> > >  #define PCI_SECONDARY_BUS        0x19    /* Secondary bus number */
> > >  #define PCI_SEC_STATUS           0x1e    /* Secondary status register, 
> > > only bit 14 used */
> > > @@ -141,7 +146,9 @@ static inline int pci_bar_is_64bit(const PCIIORegion 
> > > *r)
> > >  #define PCI_SUBVENDOR_ID        0x2c    /* obsolete, use 
> > > PCI_SUBSYSTEM_VENDOR_ID */
> > >  #define PCI_SUBDEVICE_ID        0x2e    /* obsolete, use 
> > > PCI_SUBSYSTEM_ID */
> > >  
> > > +#define PCI_ROM_ADDRESS         0x30    /* Bits 31..11 are address, 
> > > 10..1 reserved */
> > >  #define  PCI_ROM_ADDRESS_ENABLE 0x01
> > > +#define PCI_ROM_ADDRESS_MASK    (~0x7ffUL)
> > >  
> > >  /* Bits in the PCI Status Register (PCI 2.3 spec) */
> > >  #define PCI_STATUS_RESERVED1     0x007
> > 
> 
> -- 
> yamahata




reply via email to

[Prev in Thread] Current Thread [Next in Thread]