[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: |
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
- [Qemu-devel] Re: [PATCH 36/61] pci: use QLIST_ macro instead of direct list manipulation., (continued)
- [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
- [Qemu-devel] [PATCH 42/61] pci/brdige: qdevfy and initialize secondary bus and subordinate bus., Isaku Yamahata, 2009/09/30