From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by mx.groups.io with SMTP id smtpd.web10.5899.1665486607120319054 for ; Tue, 11 Oct 2022 04:10:07 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=GsRpZKgk; spf=pass (domain: kernel.org, ip: 145.40.68.75, mailfrom: ardb@kernel.org) 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 ams.source.kernel.org (Postfix) with ESMTPS id 0CBDCB8124F for ; Tue, 11 Oct 2022 11:10:05 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id B7A95C433D6 for ; Tue, 11 Oct 2022 11:10:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1665486603; bh=b6jA4mLWSq9PVEQeKVuJRuEiQCpCztYmSFgYoqJgKgE=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=GsRpZKgkX6rjTkyDieNhhywGeNMbbckz8vSStJpknfFk+0bi48WhCD7rgFIJlqLiQ x1Die6MmI6ZIz/YCFzY1m9G1M2UUiO+uFiwkGaAO7+0Fgy2bRUIwwLZ08t37cXLjFi BOXQYNwDjnlOJC0OySFk7l1wZtodWdaP5wTslWyjpuE0ZHKd9vMorhb8hikr1hRfR7 kn4Hehh9DDXreCmIa3W/samgPAO58F57F3fBMBTMi/AVR9pTlZxk+dQ02LdKaYd5I7 WouglbkRhaTxeiJ2il1IYufs3nlWa6913Ldeqk5Zkr/Xq+yeSBUtZ4P7zydiYvkgsZ OSsMY5Q6RNoUw== Received: by mail-lf1-f43.google.com with SMTP id r14so20567022lfm.2 for ; Tue, 11 Oct 2022 04:10:03 -0700 (PDT) X-Gm-Message-State: ACrzQf0mZCPwz4gSVszHON++F99DMPg+f1mqUzQv0AMm8uL0ux+CbfXH 1dHJsekRq7GWL48lmzjuBd1OCHRJnbBFQsseTRo= X-Google-Smtp-Source: AMsMyM65H2BtgBI4y14CxH5mS3rTB+hhtRCQXVD1pxPped7nNqaQXqnBFaRO0XozriZ68X/bo8FEvNzvqzgj5gQbwOA= X-Received: by 2002:a19:c20b:0:b0:4a2:40e5:78b1 with SMTP id l11-20020a19c20b000000b004a240e578b1mr8371469lfc.228.1665486601746; Tue, 11 Oct 2022 04:10:01 -0700 (PDT) MIME-Version: 1.0 References: <20221010101202.1146624-1-sunilvl@ventanamicro.com> <20221010101202.1146624-27-sunilvl@ventanamicro.com> In-Reply-To: From: "Ard Biesheuvel" Date: Tue, 11 Oct 2022 13:09:50 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-staging/RiscV64QemuVirt PATCH 26/29] OvmfPkg: Add generic Qemu NOR flash DXE driver To: Sunil V L Cc: devel@edk2.groups.io, Ard Biesheuvel , Jiewen Yao , Jordan Justen , Gerd Hoffmann Content-Type: text/plain; charset="UTF-8" On Mon, 10 Oct 2022 at 19:20, Sunil V L wrote: > > On Mon, Oct 10, 2022 at 06:16:28PM +0200, Ard Biesheuvel wrote: > > On Mon, 10 Oct 2022 at 18:05, Sunil V L wrote: > > > > > > On Mon, Oct 10, 2022 at 05:29:15PM +0200, Ard Biesheuvel wrote: > > > > On Mon, 10 Oct 2022 at 17:19, Sunil V L wrote: > > > > > > > > > > On Mon, Oct 10, 2022 at 12:39:21PM +0200, Ard Biesheuvel wrote: > > > > > > On Mon, 10 Oct 2022 at 12:13, Sunil V L wrote: > > > > > > > > > > > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4076 > > > > > > > > > > > > > > RISC-V needs NorFlashDxe driver for qemu virt machine. The > > > > > > > ArmPlatformPkg has this driver but migrating it to generic > > > > > > > package like MdeModulePkg introduces circular dependencies. > > > > > > > So, add a simplified version of the NorFlashDxe driver in > > > > > > > OvmfPkg. > > > > > > > > > > > > > > > > > > > So what is the difference between this simplified version and the old > > > > > > version? And it is backed in QEMU by the same NOR flash emulation, > > > > > > shouldn't we use this driver for ArmVirtQemu as well? > > > > > > > > > > I agree. If we can break the dependency on EmbeddedPkg due to > > > > > NvVarStoreFormattedLib, then we can migrate to MdeModulePkg and all > > > > > consumers can use the same driver which would be the best solution IMO. > > > > > > > > > > Could you please let me know why NvVarStoreFormattedLib is added > > > > > in EmbeddedPkg instead of MdePkg or MdeModulePkg? Is it only for > > > > > non-server class platforms? I don't see it doing much so not sure > > > > > its use case. > > > > > > > > > > > > > I think that library as well as the definition of > > > > gEdkiiNvVarStoreFormattedGuid should be moved to MdeModulePkg. > > > > > > > > Then, we can look at moving NorFlashDxe in there as well. > > > > > > Great. Let me rework these patches then. > > > > > > > > But you haven't answered my question regarding how your version was simplified. > > > > > > > > > > Oh sorry. I forgot to modify the commit message but what I really meant > > > was removing the code which depends upon PcdNorFlashCheckBlockLocked for > > > virtual platforms. I thought it is useful only for real platforms and > > > qemu doesn't emulate it. Let me know if I am wrong. > > > Since the driver is copied for OVMF, I didn't want to add the PCD unnecessarily. > > > > > > > What I would like to do is the following: > > > > - move NvVarstoreFormattedLib into MdeModulePkg (as you proposed already) > > - create a new NorFlashDxe in Ovmf that is cleaned up, drops the > > handling of locked blocks (as you suggest) and minimizes the number of > > transitions between array mode and programming mode (i have some > > patches i can share as a starting point) > > - move ArmPlatformPkg's NorFlashDxe into its only remaining user, > > which is is Platform/ARM in edk2-platforms (there are two more users > > in edk2-platforms, but both are QEMU based on so they can switch to > > the OVMF one) > > Interesting. In that case, we can continue with current approach of copy in Ovmf > and add the optimization. I was assuming you want the same driver for > real and virtual platforms in MdeModulePkg :-). > On the one hand, I agree with Leif that having a single 'industry standard' driver in MdeModulePkg is the optimal approach. But otoh, we shouldn't kid ourselves and pretend that this driver can fulfil that role without some drastic changes. And given that nobody (including myself) seems to have time for that anyway, let's just be pragmatic here, and adopt the approach I suggested above. > Sure, please share those optimization changes. I will create patches as > you have outlined here. > There are two patches here. They seem to work but haven't seen any rigorous testing (or review). https://github.com/ardbiesheuvel/edk2/tree/norflash-for-ovmf > > > > > > > > Note that there is some room for improvement in that driver in > > > > relation to execution under KVM: switching between programming mode > > > > and array mode involves setting up/tearing down the KVM memslot, and > > > > currently, the driver is far from optimized when it comes to > > > > minimizing the number of transitions between read mode and write mode. > > > > > > I was not looking at optimizing it at this point of time. > > > > > > > I understand. And yet you are already proposing a different version of > > NorFlashDxe, and I don't want to clone the existing NorFlashDxe > > without cleaning it up first. > > Understood. Thanks for your help. > > Thanks > Sunil