From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: Indefinite recursion in pci_default_read_config Date: Tue, 15 Dec 2009 13:11:01 +0200 Message-ID: <20091215111101.GA13110@redhat.com> References: <4B276B81.4030709@suse.de> <4B276C1D.5060909@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Hannes Reinecke , kvm@vger.kernel.org To: Avi Kivity Return-path: Received: from mx1.redhat.com ([209.132.183.28]:40244 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759887AbZLOLNq (ORCPT ); Tue, 15 Dec 2009 06:13:46 -0500 Content-Disposition: inline In-Reply-To: <4B276C1D.5060909@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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_config: >> >> uint32_t pci_default_read_config(PCIDevice *d, >> uint32_t address, int len) >> { >> uint32_t val = 0; >> assert(len == 1 || len == 2 || len == 4); >> >> if (pci_access_cap_config(d, address, len)) { >> return d->cap.config_read(d, address, len); >> } >> >> len = 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 = {qdev = {id = 0xc99b10 "01:10.0", state = DEV_STATE_INITIALIZED, >> opts = 0xc99ad0, hotplugged = 0, info = 0x837e60, parent_bus = 0xc71710, >> num_gpio_out = 0, gpio_out = 0x0, num_gpio_in = 0, gpio_in = 0x0, >> child_bus = {lh_first = 0x0}, num_child_bus = 0, sibling = { >> le_next = 0xc99c30, le_prev = 0xc71730}}, >> config = 0xca3010 "\206\200\312\020\003", >> cmask = 0xca3120 "\377\377\377\377", wmask = 0xca3230 "", >> used = 0xca3340 "", bus = 0xc71710, devfn = 32, >> name = "pci-assign", '\000', io_regions = {{ >> addr = 4060102656, size = 16384, filtered_size = 16384, type = 0 '\000', >> map_func = 0x46a5f0}, {addr = 0, size = 0, >> filtered_size = 0, type = 0 '\000', map_func = 0}, {addr = 0, size = 0, >> filtered_size = 0, type = 0 '\000', map_func = 0}, {addr = 4060119040, >> size = 16384, filtered_size = 16384, type = 0 '\000', >> map_func = 0x46a5f0}, {addr = 0, size = 0, >> filtered_size = 0, type = 0 '\000', map_func = 0}, {addr = 0, size = 0, >> filtered_size = 0, type = 0 '\000', map_func = 0}, {addr = 0, size = 0, >> filtered_size = 0, type = 0 '\000', map_func = 0}}, >> config_read = 0x46a050, >> config_write = 0x469f30, irq = 0xca3450, >> irq_state = 0 '\000', cap_present = 0, msix_cap = 0 '\000', >> msix_entries_nr = 0, msix_table_page = 0x0, msix_mmio_index = 0, >> msix_entry_used = 0x0, msix_bar_size = 0, version_id = 2, >> msix_page_size = 0, msix_irq_entries = 0x0, cap = {supported = 1, >> start = 64, length = 16, >> config_read = 0x416770, >> config_write = 0x46b750}} >> > > Michael? This is likely a bad merge on my part. Can you help? > > -- > error compiling committee.c: too many arguments to function 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. -- 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) } } +uint32_t pci_read_config(PCIDevice *d, + uint32_t address, int len) +{ + uint32_t val = 0; + + len = 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 = 0; assert(len == 1 || len == 2 || len == 4); if (pci_access_cap_config(d, address, len)) { return d->cap.config_read(d, address, len); } - len = 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); } static void pci_write_config(PCIDevice *pci_dev, @@ -1052,7 +1059,7 @@ int pci_access_cap_config(PCIDevice *pci_dev, uint32_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); } void pci_default_cap_write_config(PCIDevice *pci_dev,