Re: [edk2-devel] [PATCH] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery

["Laszlo Ersek" <lersek@redhat.com>]

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".)

Thank you very much!
Laszlo