From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 090D5C433EF for ; Tue, 4 Jan 2022 22:17:09 +0000 (UTC) Received: from localhost ([::1]:55782 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1n4s7A-0003wy-0V for qemu-devel@archiver.kernel.org; Tue, 04 Jan 2022 17:17:08 -0500 Received: from eggs.gnu.org ([209.51.188.92]:37658) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1n4s5Q-00028C-Iy for qemu-devel@nongnu.org; Tue, 04 Jan 2022 17:15:26 -0500 Received: from [2604:1380:4641:c500::1] (port=52114 helo=dfw.source.kernel.org) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1n4s5O-0000wj-3l for qemu-devel@nongnu.org; Tue, 04 Jan 2022 17:15:20 -0500 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 02EFC61477; Tue, 4 Jan 2022 22:15:08 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 6BF27C36AE0; Tue, 4 Jan 2022 22:15:07 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1641334507; bh=Kzxk3RkrfZPbqtVWNZWORHyjEKYI+rtH1xhVbWuMzOA=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=pwB/pe3aUdMravRuQYn0T8NZNVDW1z8daASldlC0mEeXgRK35sNzT5qwkYKVQed6w dbRUdFbLZS62AXFWZHBB/GyNHVMO2cLDezb/l4A4KxMtGGolTmYWUX4sQVCRdOuLu6 TtSk3Tk3igYcUp+ECfTHnS0+CkdaXql7E5waQP3kkt7KOYTultkV0KejXUDOE/aGut Oo2XxstZitkDhWkCNnrJr1nszmJ1eiygdKRVoblG0g+OUuCRowMpKe+p4eq6DOp+VK ZYiAIAopeXCR2JQRnP/mbcOm2xDTSNccLyqgHiNPax7NeZ2ZJFrQYYgFWVbaGZlWjo 7vNHAptC/AcgA== Received: from sofa.misterjones.org ([185.219.108.64] helo=wait-a-minute.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1n4s5B-00G18j-HJ; Tue, 04 Jan 2022 22:15:05 +0000 Date: Tue, 04 Jan 2022 22:15:04 +0000 Message-ID: <877dbfywpj.wl-maz@kernel.org> From: Marc Zyngier To: eric.auger@redhat.com Subject: Re: [PATCH v2 1/5] hw/arm/virt: Key enablement of highmem PCIe on highmem_ecam In-Reply-To: References: <20211003164605.3116450-1-maz@kernel.org> <20211003164605.3116450-2-maz@kernel.org> <87pmpiyrfw.wl-maz@kernel.org> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: eric.auger@redhat.com, qemu-devel@nongnu.org, drjones@redhat.com, peter.maydell@linaro.org, kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org, kernel-team@android.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-Host-Lookup-Failed: Reverse DNS lookup failed for 2604:1380:4641:c500::1 (failed) Received-SPF: pass client-ip=2604:1380:4641:c500::1; envelope-from=maz@kernel.org; helo=dfw.source.kernel.org X-Spam_score_int: 2 X-Spam_score: 0.2 X-Spam_bar: / X-Spam_report: (0.2 / 5.0 requ) DKIMWL_WL_HIGH=-0.37, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RDNS_NONE=0.793, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=no autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Peter Maydell , Andrew Jones , kvm@vger.kernel.org, qemu-devel@nongnu.org, kernel-team@android.com, kvmarm@lists.cs.columbia.edu Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" Hi Eric, On Tue, 04 Jan 2022 15:31:33 +0000, Eric Auger wrote: > > Hi Marc, > > On 12/27/21 4:53 PM, Marc Zyngier wrote: > > Hi Eric, > > > > Picking this up again after a stupidly long time... > > > > On Mon, 04 Oct 2021 13:00:21 +0100, > > Eric Auger wrote: > >> Hi Marc, > >> > >> On 10/3/21 6:46 PM, Marc Zyngier wrote: > >>> Currently, the highmem PCIe region is oddly keyed on the highmem > >>> attribute instead of highmem_ecam. Move the enablement of this PCIe > >>> region over to highmem_ecam. > >>> > >>> Signed-off-by: Marc Zyngier > >>> --- > >>> hw/arm/virt-acpi-build.c | 10 ++++------ > >>> hw/arm/virt.c | 4 ++-- > >>> 2 files changed, 6 insertions(+), 8 deletions(-) > >>> > >>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > >>> index 037cc1fd82..d7bef0e627 100644 > >>> --- a/hw/arm/virt-acpi-build.c > >>> +++ b/hw/arm/virt-acpi-build.c > >>> @@ -157,10 +157,9 @@ static void acpi_dsdt_add_virtio(Aml *scope, > >>> } > >>> > >>> static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, > >>> - uint32_t irq, bool use_highmem, bool highmem_ecam, > >>> - VirtMachineState *vms) > >>> + uint32_t irq, VirtMachineState *vms) > >>> { > >>> - int ecam_id = VIRT_ECAM_ID(highmem_ecam); > >>> + int ecam_id = VIRT_ECAM_ID(vms->highmem_ecam); > >>> struct GPEXConfig cfg = { > >>> .mmio32 = memmap[VIRT_PCIE_MMIO], > >>> .pio = memmap[VIRT_PCIE_PIO], > >>> @@ -169,7 +168,7 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, > >>> .bus = vms->bus, > >>> }; > >>> > >>> - if (use_highmem) { > >>> + if (vms->highmem_ecam) { > >> highmem_ecam is more restrictive than use_highmem: > >> vms->highmem_ecam &= vms->highmem && (!firmware_loaded || aarch64); > >> > >> If I remember correctly there was a problem using highmem ECAM with 32b > >> AAVMF FW. > >> > >> However 5125f9cd2532 ("hw/arm/virt: Add high MMIO PCI region, 512G in > >> size") introduced high MMIO PCI region without this constraint. > > Then I really don't understand the point of this highmem_ecam. We only > > register the highmem version if highmem_ecam is set (see the use of > > VIRT_ECAM_ID() to pick the right ECAM window). > > but aren't we talking about different regions? On one hand the [high] > MMIO region (512GB wide) and the [high] ECAM region (256MB large). > To me you can enable either independently. High MMIO region is used by > some devices likes ivshmem/video cards while high ECAM was introduced to > extend the number of supported buses: 601d626d148a (hw/arm/virt: Add a > new 256MB ECAM region). > > with the above change the high MMIO region won't be set with 32b > FW+kernel and LPAE whereas it is currently. > > high ECAM was not supported by 32b FW, hence the highmem_ecam. > > but maybe I miss your point? There are two issues. First, I have been conflating the ECAM and MMIO ranges, and you only made me realise that they were supposed to be independent. I still think the keying on highmem is wrong, but the main issue is that the highmem* flags don't quite describe the shape of the platform. All these booleans indicate is whether the feature they describe (the high MMIO range, the high ECAM range, and in one of my patches the high RD range) are *allowed* to live above 4GB, but do not express whether then are actually usable (i.e. fit in the PA range). Maybe we need to be more thorough in the way we describe the extended region in the VirtMachineState structure: - highmem: overall control for anything that *can* live above 4GB - highmem_ecam: Has a PCIe ECAM region above 256GB - highmem_mmio: Has a PCIe MMIO region above 256GB - highmem_redist: Has 512 RDs above 256GB Crucially, the last 3 items must fit in the PA range or be disabled. We have highmem_ecam which is keyed on highmem, but not on the PA range. highmem_mmio doesn't exist at all (we use highmem instead), and I'm only introducing highmem_redist. For these 3 ranges, we should have something like vms->highmem_xxx &= (vms->highmem && (vms->memmap[XXX].base + vms->vms->memmap[XXX].size) < vms->highest_gpa); and treat them as independent entities. Unless someone shouts, I'm going to go ahead and implement this logic. Thanks, M. -- Without deviation from the norm, progress is not possible.