From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Mswi4-00050s-O3 for qemu-devel@nongnu.org; Wed, 30 Sep 2009 06:47:00 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Mswhz-0004xN-IC for qemu-devel@nongnu.org; Wed, 30 Sep 2009 06:46:59 -0400 Received: from [199.232.76.173] (port=58568 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Mswhz-0004ww-4g for qemu-devel@nongnu.org; Wed, 30 Sep 2009 06:46:55 -0400 Received: from mx1.redhat.com ([209.132.183.28]:15062) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Mswhy-0000sT-IA for qemu-devel@nongnu.org; Wed, 30 Sep 2009 06:46:54 -0400 Date: Wed, 30 Sep 2009 12:44:46 +0200 From: "Michael S. Tsirkin" Message-ID: <20090930104445.GA18802@redhat.com> References: <1254305917-14784-1-git-send-email-yamahata@valinux.co.jp> <1254305917-14784-39-git-send-email-yamahata@valinux.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1254305917-14784-39-git-send-email-yamahata@valinux.co.jp> Subject: [Qemu-devel] Re: [PATCH 38/61] pci: fix pci_default_write_config() List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Isaku Yamahata Cc: qemu-devel@nongnu.org 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 > Signed-off-by: Isaku Yamahata > --- > 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