From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hannes Reinecke Subject: Re: Indefinite recursion in pci_default_read_config Date: Tue, 15 Dec 2009 12:26:15 +0100 Message-ID: <4B277257.9040405@suse.de> References: <4B276B81.4030709@suse.de> <4B276C1D.5060909@redhat.com> <20091215111101.GA13110@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Avi Kivity , kvm@vger.kernel.org To: "Michael S. Tsirkin" Return-path: Received: from cantor.suse.de ([195.135.220.2]:55653 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759955AbZLOL0R (ORCPT ); Tue, 15 Dec 2009 06:26:17 -0500 In-Reply-To: <20091215111101.GA13110@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: Michael S. Tsirkin wrote: > On Tue, Dec 15, 2009 at 12:59:41PM +0200, Avi Kivity wrote: >> On 12/15/2009 12:57 PM, Hannes Reinecke wrote: >>> Hi all, >>> >>> I just triggered a nasty indefinite recursion in pci_default_read_c= onfig: >>> >>> uint32_t pci_default_read_config(PCIDevice *d, >>> uint32_t address, int len) >>> { >>> uint32_t val =3D 0; >>> assert(len =3D=3D 1 || len =3D=3D 2 || len =3D=3D 4); >>> >>> if (pci_access_cap_config(d, address, len)) { >>> return d->cap.config_read(d, address, len); >>> } >>> >>> len =3D MIN(len, pci_config_size(d) - address); >>> memcpy(&val, d->config + address, len); >>> return le32_to_cpu(val); >>> } >>> >>> And d->cap.config_read is pointing to pci_default_read_config: >>> >>> (gdb) print *d >>> $3 =3D {qdev =3D {id =3D 0xc99b10 "01:10.0", state =3D DEV_STATE_IN= ITIALIZED, >>> opts =3D 0xc99ad0, hotplugged =3D 0, info =3D 0x837e60, parent= _bus =3D 0xc71710, >>> num_gpio_out =3D 0, gpio_out =3D 0x0, num_gpio_in =3D 0, gpio_= in =3D 0x0, >>> child_bus =3D {lh_first =3D 0x0}, num_child_bus =3D 0, sibling= =3D { >>> le_next =3D 0xc99c30, le_prev =3D 0xc71730}}, >>> config =3D 0xca3010 "\206\200\312\020\003", >>> cmask =3D 0xca3120 "\377\377\377\377", wmask =3D 0xca3230 "", >>> used =3D 0xca3340 "", bus =3D 0xc71710, devfn =3D 32, >>> name =3D "pci-assign", '\000', io_regions =3D = {{ >>> addr =3D 4060102656, size =3D 16384, filtered_size =3D 16384= , type =3D 0 '\000', >>> map_func =3D 0x46a5f0}, {addr =3D 0,= size =3D 0, >>> filtered_size =3D 0, type =3D 0 '\000', map_func =3D 0}, {ad= dr =3D 0, size =3D 0, >>> filtered_size =3D 0, type =3D 0 '\000', map_func =3D 0}, {ad= dr =3D 4060119040, >>> size =3D 16384, filtered_size =3D 16384, type =3D 0 '\000', >>> map_func =3D 0x46a5f0}, {addr =3D 0,= size =3D 0, >>> filtered_size =3D 0, type =3D 0 '\000', map_func =3D 0}, {ad= dr =3D 0, size =3D 0, >>> filtered_size =3D 0, type =3D 0 '\000', map_func =3D 0}, {ad= dr =3D 0, size =3D 0, >>> filtered_size =3D 0, type =3D 0 '\000', map_func =3D 0}}, >>> config_read =3D 0x46a050, >>> config_write =3D 0x469f30, irq =3D= 0xca3450, >>> irq_state =3D 0 '\000', cap_present =3D 0, msix_cap =3D 0 '\000'= , >>> msix_entries_nr =3D 0, msix_table_page =3D 0x0, msix_mmio_index = =3D 0, >>> msix_entry_used =3D 0x0, msix_bar_size =3D 0, version_id =3D 2, >>> msix_page_size =3D 0, msix_irq_entries =3D 0x0, cap =3D {support= ed =3D 1, >>> start =3D 64, length =3D 16, >>> config_read =3D 0x416770, >>> config_write =3D 0x46b750}} >>> =20 >> Michael? This is likely a bad merge on my part. Can you help? >> >> --=20 >> error compiling committee.c: too many arguments to function >=20 >=20 > Um, yes. I think the following is the right way to do this. > As a side note, we really should work to remove all these > hacks and make assignment use capability support > in upstream qemu. >=20 > -- >=20 > diff --git a/hw/pci.c b/hw/pci.c > index 110a5fc..a74d3d4 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -1016,19 +1016,26 @@ static void pci_update_irq_disabled(PCIDevice= *d, int was_irq_disabled) > } > } > =20 > +uint32_t pci_read_config(PCIDevice *d, > + uint32_t address, int len) > +{ > + uint32_t val =3D 0; > + > + len =3D MIN(len, pci_config_size(d) - address); > + memcpy(&val, d->config + address, len); > + return le32_to_cpu(val); > +} > + > uint32_t pci_default_read_config(PCIDevice *d, > uint32_t address, int len) > { > - uint32_t val =3D 0; > assert(len =3D=3D 1 || len =3D=3D 2 || len =3D=3D 4); > =20 > if (pci_access_cap_config(d, address, len)) { > return d->cap.config_read(d, address, len); > } > =20 > - len =3D MIN(len, pci_config_size(d) - address); > - memcpy(&val, d->config + address, len); > - return le32_to_cpu(val); > + return pci_read_config(d, address, len); > } > =20 > static void pci_write_config(PCIDevice *pci_dev, > @@ -1052,7 +1059,7 @@ int pci_access_cap_config(PCIDevice *pci_dev, u= int32_t address, int len) > uint32_t pci_default_cap_read_config(PCIDevice *pci_dev, > uint32_t address, int len) > { > - return pci_default_read_config(pci_dev, address, len); > + return pci_read_config(pci_dev, address, len); > } > =20 > void pci_default_cap_write_config(PCIDevice *pci_dev, Ok, works. Except for a missing prototype in hw/pci.h :-) Cheers, Hannes --=20 Dr. Hannes Reinecke zSeries & Storage hare@suse.de +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg GF: Markus Rex, HRB 16746 (AG N=FCrnberg)