Hi Dhaval,
Yes, that is what I’m suggesting. But I’m not telling you that your current
implementation needs to be adjusted, but I am providing my feedback, that IMHO
continued /use and evolution/ of the added PCD may to be messy because it’s
untyped. I already gave you a Reviewed-by, so you’re welcome to do whatever you
want with the additional feedback.
As far as an example goes… see:
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialPciDeviceInfo|{0xFF}|VOID*|0x00010067
…
DeviceInfo = (PCI_UART_DEVICE_INFO *)PcdGetPtr (PcdSerialPciDeviceInfo);
So a PCD can be of type VOID *.
A
From: Dhaval Sharma <dha...@rivosinc.com>
Sent: Wednesday, November 1, 2023 12:05 PM
To: Warkentin, Andrei <andrei.warken...@intel.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH v7 5/5] OvmfPkg/RiscVVirt: Override for RV CPU
Features
Hi Andrei,
Are you suggesting:
1. We have a generic PCD to store the address of RVFeatures bitfield.
2. It gets populated at some point during initialization with let's say some
kind of global variable address which keeps this bitfield.
3. SEC/DXE phases deref this address and use where needed?
Is there a reference I can take a look at?
Assuming my above understanding is correct, the whole idea was to keep
implementation based on simple PCD such that it can be used easily as an
override mechanism. The reason to keep it "enabled" by default in MDE is that
if m-mode decides to keep it disabled it is anyways going to remain that way.
So practically this PCD is going to be useful only in cases where the user
wants to "Override Disable". LMK if you still think we should modify the
implementation.
=D
On Tue, Oct 31, 2023 at 10:31 PM Warkentin, Andrei
<andrei.warken...@intel.com<mailto:andrei.warken...@intel.com>> wrote:
I think I misunderstood the intent. Reviewing the full patchset, it seems this
is necessary to avoid using the new CMO path in the Virt platform (since the
default value is all FFs). Shouldn’t the default Pcd value here be all 0’s –
i.e. CMO or any other feature use becomes “opt in” instead of “opt out”?
It also seems that encoding the meaning inside the bit positions is a bit…
obscure. Have you considered storing a pointer to a struct with bitfields
instead? You could then change the logic to be something like “If PcdPtrValue
!= NULL && ((struct cast *) PcdPtrValue)->LegibleFieldName”. I think this would
do wonders for code maintainability. The cost of course is in having to
initialize the Pcd now at runtime, and the additional dereference, but that
seems like a low cost all things considered.
From: Dhaval Sharma <dha...@rivosinc.com<mailto:dha...@rivosinc.com>>
Sent: Tuesday, October 31, 2023 1:13 AM
To: Warkentin, Andrei
<andrei.warken...@intel.com<mailto:andrei.warken...@intel.com>>;
devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Subject: Re: [edk2-devel] [PATCH v7 5/5] OvmfPkg/RiscVVirt: Override for RV CPU
Features
Thanks. This PCD is for Virt platform only. Or maybe I am missing the point.
--
Thanks!
=D
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110491): https://edk2.groups.io/g/devel/message/110491
Mute This Topic: https://groups.io/mt/102256471/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
