Re: [edk2-devel] [PATCH v2] ArmVirt: Allow memory attributes protocol to be disabled on first boot

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

On 12/8/23 16:34, Ard Biesheuvel wrote:
> On Fri, 8 Dec 2023 at 15:34, Laszlo Ersek <lersek@redhat.com> wrote:

>>> diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
>>> index 0f2d7873279f..c55978f75c19 100644
>>> --- a/ArmVirtPkg/ArmVirtPkg.dec
>>> +++ b/ArmVirtPkg/ArmVirtPkg.dec
>>> @@ -68,3 +68,9 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
>>>    # Cloud Hypervisor has no other way to pass Rsdp address to the guest except use a PCD.
>>>
>>>    #
>>>
>>>    gArmVirtTokenSpaceGuid.PcdCloudHvAcpiRsdpBaseAddress|0x0|UINT64|0x00000005
>>>
>>> +
>>>
>>> +  ##
>>>
>>> +  # Whether the EFI memory attribus protocol should be uninstalled before
>>>
>>> +  # invoking the OS loader on the first boot. This may be needed to work around
>>>
>>> +  # problematic builds of shim that use the protocol incorrectly.
>>>
>>> +  gArmVirtTokenSpaceGuid.PcdUninstallMemAttrProtocolOnFirstBoot|FALSE|BOOLEAN|0x00000006
>>>
>>
>> (1) could be a feature PCD (although it couldn't be patchable-in-module
>> then, and perhaps we don't consider this a "feature")
>>
> 
> Is this a general remark on the similarity between feature PCDs and
> boolean PCDs?

Yes, just a general remark; I generally don't have a surefire handle on
when to use a feature PCD versus a fixed-at-build (and/or
patchable-in-module) BOOLEAN PCD.

>> (4) Why the change from an explicit call from AfterConsole to a
>> constructor? Was AfterConsole too late somehow?
>>
> 
> Yes. Checking for the existence of "BootOrder" needs to occur earlier,
> or it will have been created by the BDS.
> 
>> I think constructors should be the last resort.
>>
> 
> Not disagreeing with that.
> 
>> (5) Is the depex really necessary? BDS is supposed to start when all
>> drivers have been dispatched, and so by that time, all of the UEFI
>> architectural protocols should be available. (BDS will launch UEFI
>> drivers, and all the UEFI drivers have an implicit depex on all the
>> architectural protocols.)
>>
> 
> The BDS arch protocol will be invoked at that point. but the BdsDxe
> itself could be dispatched much earlier, at which point the
> constructor of this library will be invoked.
> 
> And I'll need to include the CPU arch protocol as well here, as this
> is installed at the same time as the EFI memory attributes protocol by
> the CPU dxe driver.

Ah, so the idea is to inject code between the memory attributes
protocol's installation and BdsDxe launching.


>> (7) Tying back to my point (4) -- I understand this is a hack anyway,
>> but I'm still uncomfortable with platform BDS uninstalling a protocol
>> that is owned by / provided by the CPU driver. Feels like a significant
>> layering violation.
>>
> 
> It is.
> 
>> Can we modify the CPU driver instead, to listen to a new event group,
>> upon which being signaled, the CPU driver would uninstall the protocol
>> (and close the listening event)?
>>
>> This PlatformBootManagerLib instance would act more or less the same
>> (I'd suggest signaling the event group from within AfterConsole, in case
>> the PCD default and/or the fw_cfg knob dictated that), but the protocol
>> uninstallation would occur in "ArmPkg/Drivers/CpuDxe".
>>
>> In more technical terms, the layering violation IMO is that we mess with
>> CpuDxe's "mCpuHandle" and "mMemoryAttribute" static variables from
>> within BDS. Adding the new event group requires more boiler-plate code
>> for sure, but there's a small code-size benefit as well: we'd not have
>> to look up either the handle (with LocateHandle) or the protocol
>> interface (with HandleProtocol), as CpuDxe inherently knows those
>> (mCpuHandle, mMemoryAttribute).
>>
> 
> I agree with your analysis here. But I am reluctant to introduce
> elaborate infrastructure across drivers to implement a feature that
> should not exist in the first place.
> 
> As I mentioned a couple of times, I am rather unhappy with the
> complete lack of involvement of the people who created this mess in
> the first place, and what I am after is really a minimal, local hack
> that unblocks the actual end users (people running LIMA on ARM based
> Macs) without creating building blocks that will be used by the distro
> forks to erode the original functionality even further,

Understood. I agree to keep this contained, location-wise.

(I notice Gerd reports downthread though that limiting the protocol's
masking time-wise as well (i.e. to first boot) is not helpful, because
even rhel-9.3 grubaa64 has problems when the protocol is exposed. So a
simpler but broader approach could be better.)

Thanks!
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112300): https://edk2.groups.io/g/devel/message/112300
Mute This Topic: https://groups.io/mt/103031504/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-