From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web10.1810.1593024395131289035 for ; Wed, 24 Jun 2020 11:46:35 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Aq/+2ec0; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1593024394; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=8yjBzwQu/WhCsWi9a08RKcd+T9AHKF5e1PbApuP0TI8=; b=Aq/+2ec07e0YXN4+bS9h5EpnbDiGs/pqEl4ID1eYriJhTGMe5RMfcy/z0gLtxO6GRyVeB6 9/1WEcrUEGhvO/lcfSQdWJZeOWvTkGaJr/jiIXBA0gi3EQyM3t2uG8i5pE9R2yjf25K6qx VuxtkkVIK139mHKj86SUgX9PkTounQM= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-14-ZYvMQqxvP9ywvCK-4v3njA-1; Wed, 24 Jun 2020 14:46:32 -0400 X-MC-Unique: ZYvMQqxvP9ywvCK-4v3njA-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 6F9067BBA; Wed, 24 Jun 2020 18:46:31 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-115.ams2.redhat.com [10.36.112.115]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1FF2879318; Wed, 24 Jun 2020 18:46:27 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery From: "Laszlo Ersek" To: Andrew Jones , Ard Biesheuvel Cc: =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , devel@edk2.groups.io, Eric Auger References: <20200623175700.1564281-1-ard.biesheuvel@arm.com> <85a63fc4-f3d1-1e17-bf1d-dace59bb02a8@arm.com> <2a43b904-5172-0fb3-6d40-e1fd3b652fe7@redhat.com> <20200624134154.isbcqvscs7eidbd4@kamzik.brq.redhat.com> <20200624143709.odjhl5xc7ehxe2xl@kamzik.brq.redhat.com> Message-ID: <41900866-b572-296a-2343-aa3ecc4612f6@redhat.com> Date: Wed, 24 Jun 2020 20:46:26 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit On 06/24/20 20:43, Laszlo Ersek wrote: > On 06/24/20 16:37, Andrew Jones wrote: >> On Wed, Jun 24, 2020 at 03:48:46PM +0200, Ard Biesheuvel wrote: >>> On 6/24/20 3:41 PM, Andrew Jones wrote: >>>> On Wed, Jun 24, 2020 at 03:02:49PM +0200, Philippe Mathieu-Daudé wrote: >>>>> On 6/24/20 1:43 PM, Ard Biesheuvel wrote: >>> ... >>>>>> I wasn't aware that we even expose the flash in the DSDT. In any case, >>>>>> no driver exists in Linux today that claims the LNRO0015 _HID, and so I >>>>>> agree we should simply remove it entirely. >>>>>> >>>>>> However, I am no longer able to contribute to QEMU, so I was hoping you >>>>>> or Phil could take the action? >>>>> >>>>> I try to stay as far as possible from ACPI, but here it seems >>>>> fair I assign myself to this (except if Drew/Eric prefer to >>>>> do it, of course!). >>>> >>>> I don't mind doing it. IIUC, all we need to do is remove the flash device >>>> from the DSDT to "hide" it from the guest. Of course we'll need some >>>> compat code too in order to only do this for machine types 5.1 and later, >>>> and that means that running guest kernels which want to bind the flash on >>>> 5.0 and older machine types will still have the problem. >>>> >>> >>> Do you think that is really necessary? LNRO0015 never had a driver in Linux >>> to begin with, and I doubt other ACPI capable arm64 OSes would be any >>> different. >> >> I'd rather not add/remove hardware in older machine types. While it's >> unlikely anybody would notice, we can't be sure that there's nothing >> out there which would break. > > I agree. > >>> >>>> Also, it seems a bit odd to hide hardware from the guest OS. Wouldn't it >>>> be better to somehow flag that the hardare may be in use by firmware, >>>> and therefore is only safe to use if runtime services are not used? I'm >>>> not sure ACPI supports that for table entries like these, but maybe some >>>> _STA value indicates something like it. I'll take a look at the spec. >>>> >>> >>> We could do either, but a _STA indicating that the device is not present or >>> not ready amounts to the same afaik. Ultimately, the OS could still access >>> the physical range if it wanted to (e.g., via /dev/mem), so not exposing it >>> in the first place seems more robust to me. >>> >> >> If there's no _STA state that says the device is here and works, but it's >> not available, then I agree removing it is the same. And, thinking about >> it some more, this flash device is really only for our host-controlled >> firmware. Since we don't give the guest any control over the firmware, >> then the device the firmware lives on should probably not even exist as >> far as the guest is concerned. > > I think it's safest to remove the object from the DSDT; at least x86 > Windows used to be really picky about _STA in Device Manager. Best to > avoid yellow triangles (or whatever) there (assuming Device Manager is a > thing on aarch64 Windows -- I don't know). > > Drew, when you remove the flash addition function call, please replace > it with a comment that's similar to the one we have about the RTC. That > way, we can run "git blame" on the comment. (Pure deletions tend to > impede "git blame", as no code lines remain on which git-blame could > report a "latest commit".) oops, sorry, dumb request -- this isn't going to be a pure removal; the flash addition will remain, it will only be made conditional on a new machine type property. So there's going to be *something* to run git-blame on. Apologies for my mistake. Laszlo