| Message ID | 1254305917-14784-39-git-send-email-yamahata@valinux.co.jp |
|---|---|
| State | Superseded |
| Headers | show |
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. > 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. 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. > Cc: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > --- > 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 > -- > 1.6.0.2
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. > 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? And pci_update_mapppings() takes care of such cases. > > Cc: Michael S. Tsirkin <mst@redhat.com> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > > --- > > 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 >
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 <mst@redhat.com> > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > > > --- > > > 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
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) || + 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
When updated ROM expantion address of header type 0, it missed to update mappings. By using helper functions this patch also avoids memcpy() and memcmp(). Cc: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> --- hw/pci.c | 11 +++++------ hw/pci.h | 9 ++++++++- 2 files changed, 13 insertions(+), 7 deletions(-)