From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39073) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aIe6o-0003Yi-Q1 for qemu-devel@nongnu.org; Mon, 11 Jan 2016 10:10:15 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aIe6l-0007GS-W5 for qemu-devel@nongnu.org; Mon, 11 Jan 2016 10:10:14 -0500 Received: from mx1.redhat.com ([209.132.183.28]:43795) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aIe6l-0007GI-OV for qemu-devel@nongnu.org; Mon, 11 Jan 2016 10:10:11 -0500 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id 7F4C3A37FA for ; Mon, 11 Jan 2016 15:10:11 +0000 (UTC) References: <1452515063-5615-1-git-send-email-marcel@redhat.com> <20160111150753.324bd37b@nial.brq.redhat.com> From: Marcel Apfelbaum Message-ID: <5693C5D0.8050107@redhat.com> Date: Mon, 11 Jan 2016 17:10:08 +0200 MIME-Version: 1.0 In-Reply-To: <20160111150753.324bd37b@nial.brq.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] hw/pci: do not update the PCI mappings while Decode (I/O or memory) bit is not set in the Command register List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: lersek@redhat.com, qemu-devel@nongnu.org, mst@redhat.com On 01/11/2016 04:07 PM, Igor Mammedov wrote: > On Mon, 11 Jan 2016 14:24:23 +0200 > Marcel Apfelbaum wrote: > >> Two reasons: >> - PCI Spec indicates that while the bit is not set >> the memory sizing is not finished. >> - pci_bar_address will return PCI_BAR_UNMAPPED >> and a previous value can be accidentally overridden >> if the command register is modified (and not the BAR). >> >> Signed-off-by: Marcel Apfelbaum >> --- >> >> Hi, >> >> I found this when trying to use multiple root complexes with OVMF. >> >> When trying to attach a device to the pxb-pcie device as Integrated >> Device it did not receive the IO/MEM resources. >> >> The reason is that OVMF is working like that: >> 1. It disables the Decode (I/O or memory) bit in the Command register >> 2. It configures the device BARS >> 3. Makes some tests on the Command register >> 4. ... >> 5. Enables the Decode (I/O or memory) at some point. >> >> On step 3 all the BARS are overridden to 0xffffffff by QEMU. >> >> Since QEMU uses the device BARs to compute the new host bridge resources >> it now gets garbage. >> >> Laszlo, this also solves the SHPC problem for the pci-2-pci bridge inside the pxb. >> Now we can enable the SHPC for it too. > Hi Igor, Thanks for the review. > What about migration case? > Shouldn't mappings be updated to match source even if bit isn't set? I don't know much about migration, but I think it would be just OK because the regions itself are migrated (I think...) and the migration target can't use the IO or MEM (until the bit will be set), so the mappings don't matter. Once set, the mappings would be updated on the target. Another thing, in the source the mappings are not updated either until the bit is is set. Thanks, Marcel > >> >> Thanks, >> Marcel >> >> hw/pci/pci.c | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) >> >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >> index 168b9cc..f9127dc 100644 >> --- a/hw/pci/pci.c >> +++ b/hw/pci/pci.c >> @@ -1148,6 +1148,7 @@ static void pci_update_mappings(PCIDevice *d) >> PCIIORegion *r; >> int i; >> pcibus_t new_addr; >> + uint16_t cmd = pci_get_word(d->config + PCI_COMMAND); >> >> for(i = 0; i < PCI_NUM_REGIONS; i++) { >> r = &d->io_regions[i]; >> @@ -1156,6 +1157,22 @@ static void pci_update_mappings(PCIDevice *d) >> if (!r->size) >> continue; >> >> + /* >> + * Do not update the mappings until the command register's >> + * Decode (I/O or memory) bit is not set. Two reasons: >> + * - PCI Spec indicates that while the bit is not set >> + * the memory sizing is not finished. >> + * - pci_bar_address will return PCI_BAR_UNMAPPED >> + * and a previous value can be accidentally overridden >> + * if the command register is modified (and not the BAR). >> + * */ >> + if (((r->type & PCI_BASE_ADDRESS_SPACE_IO) && >> + !(cmd & PCI_COMMAND_IO)) || >> + ((r->type != PCI_BASE_ADDRESS_SPACE_IO) && >> + !(cmd & PCI_COMMAND_MEMORY))) { >> + continue; >> + } >> + >> new_addr = pci_bar_address(d, i, r->type, r->size); >> >> /* This bar isn't changed */ >