From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39287) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YUfN9-00045w-B4 for qemu-devel@nongnu.org; Sun, 08 Mar 2015 13:52:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YUfN8-0003j6-7N for qemu-devel@nongnu.org; Sun, 08 Mar 2015 13:52:15 -0400 Message-ID: <54FC8C2E.9000709@redhat.com> Date: Sun, 08 Mar 2015 19:51:42 +0200 From: Marcel Apfelbaum MIME-Version: 1.0 References: <1425813387-31231-1-git-send-email-marcel@redhat.com> <1425813387-31231-14-git-send-email-marcel@redhat.com> <20150308161340.GA11259@morn.localdomain> In-Reply-To: <20150308161340.GA11259@morn.localdomain> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 for-2.3 13/25] hw/acpi: remove from root bus 0 the crs resources used by other busses. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin O'Connor Cc: seabios@seabios.org, kraxel@redhat.com, mst@redhat.com, quintela@redhat.com, qemu-devel@nongnu.org, agraf@suse.de, alex.williamson@redhat.com, qemu-ppc@nongnu.org, hare@suse.de, imammedo@redhat.com, amit.shah@redhat.com, pbonzini@redhat.com, leon.alrae@imgtec.com, aurelien@aurel32.net, rth@twiddle.net On 03/08/2015 06:13 PM, Kevin O'Connor wrote: > On Sun, Mar 08, 2015 at 01:16:15PM +0200, Marcel Apfelbaum wrote: >> If multiple root busses are used, root bus 0 cannot use all the >> pci holes ranges. Remove the IO/mem ranges used by the other >> primary busses. > [...] >> - aml_append(crs, >> - aml_word_io(aml_min_fixed, aml_max_fixed, >> - aml_pos_decode, aml_entire_range, >> - 0x0000, 0x0D00, 0xFFFF, 0x0000, 0xF300)); >> + >> + /* prepare PCI IO ranges */ >> + range.base = 0x0D00; >> + range.limit = 0xFFFF; >> + if (QLIST_EMPTY(&io_ranges)) { >> + aml_append(crs, >> + aml_word_io(aml_min_fixed, aml_max_fixed, >> + aml_pos_decode, aml_entire_range, >> + 0x0000, range.base, range.limit, >> + 0x0000, range.limit - range.base + 1)); >> + } else { >> + QLIST_FOREACH(entry, &io_ranges, entry) { >> + if (range.base < entry->base) { >> + aml_append(crs, >> + aml_word_io(aml_min_fixed, aml_max_fixed, >> + aml_pos_decode, aml_entire_range, >> + 0x0000, range.base, entry->base - 1, >> + 0x0000, entry->base - range.base)); >> + } >> + range.base = entry->limit + 1; >> + if (!QLIST_NEXT(entry, entry)) { >> + aml_append(crs, >> + aml_word_io(aml_min_fixed, aml_max_fixed, >> + aml_pos_decode, aml_entire_range, >> + 0x0000, range.base, range.limit, >> + 0x0000, range.limit - range.base + 1)); >> + } >> + } >> + } > Hi Kevin, Thank you for your review! > If I read this correctly, it looks like a machine with two root buses > and 20 devices, each with one memory range and one io range, would end > up with 40 CRS ranges (ie, a CRS range for every resource). Correct. As Michael pointed out in another thread, the firmware is considered guest code and QEMU cannot assume anything on how the resources are assigned. This is why this solution was chosen. However we have two things that make the situation a little better. 1. The PXB implementation includes a pci-bridge and all devices are automatically attached to the secondary bus, in this way we have one IO/MEM range per extra root bus. 2. On top of this series we can add a merge algorithm that will bring together consecutive ranges. This series does not include this optimization and it focuses on the correctness. It also > looks like this furthers the requirement that the guest firmware > assign the PCI resources prior to QEMU being able to generate the ACPI > tables. > > Am I correct? If so, that doesn't sound ideal. You are correct, however is not that bad because we have the following sequence: - Early in the boot sequence the bios scans the PCI buses and assigns IO/MEM ranges - At this moment all resources needed by QEMU are present in the configuration space. - At the end of the boot sequence the BIOS queries the ACPI tables and *only then* the tables are computed. I think we use that implicitly for other features, anyway, it looks like an elegant solution with no real drawbacks. (Our assumptions are safe) Thanks, Marcel > > -Kevin >