Re: [PATCH 2/2] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Clear C-bit when SEV is active

[Laszlo Ersek <lersek@redhat.com>]

Hi Brijesh,

you provided a lot of information (and it seems like your analysis was
advancing in parallel with your email -- I too do that sometimes :) ),
so it's not easy for me to write a concise response.

* Regarding the C-bit that covers the relocated save state area (esp.
considering that the area is not page aligned and/or contains executable
code):

I think (a) adding any code (under OvmfPkg, or under the core) that sets
the C-bit "correctly", and in turn, (b) lacking any code in edk2 that
actually *exercises* the "correct" C-bit setting, is counter-productive.
Unless the C-bit is actively exercised, we're just adding *dead* code,
which is a bad thing -- it's very easy to regress (without anyone
noticing), and it leads to developer confusion.

On the other hand, I wouldn't want your analysis to be lost (remember: I
asked for the explanation because I didn't understand the behavior). So
I'm thinking your analysis should be captured in both a commit message,
and a large comment *somewhere*. Possibly near the code (wherever it may
end up, AmdSevDxe or SmmCpuFeaturesLib) where you manage the C-bit for
the *initial* save state map.

I mean, wherever you manage the C-bit for the initial save state map
(which is required), you can also make a large comment on the *future*
location and behavior.

IMO it's OK if we don't guarantee the guest with functional access into
the relocated save state map -- there is no actual code relying on that!
-- as long as we document this gap.

Does this suggestion make sense to you?

* Regarding mapping all the NonExistent and MMIO GCD entries in the SMM
page table as plaintext: I think we should really be on par with
AmdSevDxe here. This is code that *will* be exercised, and if we cut
corners here, next time we add another MMIO range or device that needs
to be accessed from SMM too (for whatever reason), we'll go crazy otherwise.

* In general, regarding how and when SmmCpuFeaturesLib APIs are hooked
into PiSmmCpuDxeSmm: please ask questions -- and ask them to Mike :)
OVMF's instance of this lib class is Paolo's work (thanks again for
that!), so I always defer to Mike and Paolo whenever this lib class and
instance come up. SmmCpuFeaturesInitializeProcessor() looked suitable to
me, for implementing the previous bullet, but it's really just my
"shopping" for a good pre-existent hook point. If we need something
better, let's discuss it with Mike.

I'm sorry if the above is a bit too vague; please post some new patches,
even if only RFCs :)

Thanks!
Laszlo