[Top][All Lists]
[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: |
Isaku Yamahata |
Subject: |
[Qemu-devel] Re: [PATCH 38/61] pci: fix pci_default_write_config() |
Date: |
Wed, 30 Sep 2009 20:09:42 +0900 |
User-agent: |
Mutt/1.5.6i |
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 <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
- [Qemu-devel] [PATCH 36/61] pci: use QLIST_ macro instead of direct list manipulation., (continued)
- [Qemu-devel] [PATCH 36/61] pci: use QLIST_ macro instead of direct list manipulation., Isaku Yamahata, 2009/09/30
- [Qemu-devel] [PATCH 22/61] pci: use appropriate PRIs in PCI_DPRINTF()., Isaku Yamahata, 2009/09/30
- [Qemu-devel] [PATCH 43/61] pci: add helper function to initialize wmask., Isaku Yamahata, 2009/09/30
- [Qemu-devel] [PATCH 39/61] pci: factor out config update logic., Isaku Yamahata, 2009/09/30
- [Qemu-devel] [PATCH 55/61] ioapic: make ioapic_set_irq() static., Isaku Yamahata, 2009/09/30
- [Qemu-devel] [PATCH 53/61] pc q35 based chipset emulator, Isaku Yamahata, 2009/09/30
- [Qemu-devel] [PATCH 38/61] pci: fix pci_default_write_config(), Isaku Yamahata, 2009/09/30
- [Qemu-devel] [PATCH 12/61] pc: make pc_init1() not refer ferr_irq directly., Isaku Yamahata, 2009/09/30
- [Qemu-devel] [PATCH 44/61] pci: initialize wmask according to pci header type., Isaku Yamahata, 2009/09/30
- [Qemu-devel] [PATCH 41/61] pci: make bar update function aware of pci bridge., Isaku Yamahata, 2009/09/30
- [Qemu-devel] [PATCH 23/61] pci: use PCI_SLOT() and PCI_FUNC()., Isaku Yamahata, 2009/09/30
- [Qemu-devel] [PATCH 58/61] ioapic: make the number of pins configurable., Isaku Yamahata, 2009/09/30
- [Qemu-devel] [PATCH 47/61] pci.h: add more status constats., Isaku Yamahata, 2009/09/30
- [Qemu-devel] [PATCH 57/61] ioapic: add callback when entry is set or ioapic is reset, Isaku Yamahata, 2009/09/30
- [Qemu-devel] [PATCH 24/61] pci: define a constant to represent a unmapped bar and use it., Isaku Yamahata, 2009/09/30
- [Qemu-devel] [PATCH 18/61] pc: split out piix specific part from pc.c into pc_piix.c, Isaku Yamahata, 2009/09/30