Hi Andrei,

Are you suggesting:

We have a generic PCD to store the address of RVFeatures bitfield.
It gets populated at some point during initialization with let's say some kind of global variable address which keeps this bitfield.
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.

On Tue, Oct 31, 2023 at 10:31 PM Warkentin, Andrei <andrei.warkentin@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 <dhaval@rivosinc.com>
Sent: Tuesday, October 31, 2023 1:13 AM
To: Warkentin, Andrei <andrei.warkentin@intel.com>; 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.