Re: [PATCH 2/3] OvmfPkg: introduce FD_SIZE_4MB (mainly) for Windows HCK

[Laszlo Ersek <lersek@redhat.com>]

On 05/01/17 19:23, Jordan Justen wrote:
> On 2017-05-01 03:51:42, Laszlo Ersek wrote:
>> On 04/30/17 23:16, Jordan Justen wrote:
>>> On 2017-04-30 07:42:36, Laszlo Ersek wrote:
>>>
>>>> $ build \
>>>>   -b DEBUG \
>>>>   -a IA32 -a X64 \
>>>>   -p OvmfPkg/OvmfPkgIa32X64.dsc \
>>>>   -t GCC48 \
>>>>   -D SMM_REQUIRE \
>>>>   -D SECURE_BOOT_ENABLE \
>>>>   -D HTTP_BOOT_ENABLE \
>>>>   -D NETWORK_IP6_ENABLE \
>>>>   -D TLS_ENABLE
>>>
>>> Do you enable the last 3 in your production builds? I didn't think it
>>> was the case, but it would change things...
>>
>> That's a very good question, and I expected it.
>>
>> Any sane person being responsible for supporting a package will strive
>> very hard to minimize the features enabled in the package, in order to
>> minimize the problem surface / support burden. I tend to consider myself
>> a sane person, so no, HTTP_BOOT_ENABLE, NETWORK_IP6_ENABLE, and
>> TLS_ENABLE are not turned on.
>>
>> (TLS_ENABLE carries even more weight, because it increases the security
>> attack surface, so turning *that* off is very desirable.)
>>
>> *But*, I certainly want to keep the *ability* to turn these features on
>> (and maybe later features, in 2-3 years' time) if a customer or a
>> partner requests it.
> 
> It sounds like you don't expect to 'support' this. At least not to the
> same level as the rest of the firmware.

I hope never to have to support these, but at some point into the future
of RHEL7, I might have no choice.

> I think it is fine to say, if you want to enable these, you may have
> to disable debug on some other features,

As I explained earlier, universal DEBUG coverage is a requirement for
supportability.

(

Example: in parallel to this discussion, our virt QE reported an
independent issue, namely an out-of-SMRAM condition with 240+ VCPUs.

https://bugzilla.redhat.com/show_bug.cgi?id=1447027

The out-of-SMRAM condition is caught by an ASSERT() only. It is in
"UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c", function SetStaticPageTable():

    212           PageDirectoryEntry = AllocatePageTableMemory (1);
    213           ASSERT(PageDirectoryEntry != NULL);
    214           ZeroMem (PageDirectoryEntry, EFI_PAGES_TO_SIZE(1));

I find it plausible that if this memory allocation fails, then the
firmware has really no way to continue -- that's okay. But, the ASSERT()
disappears in a RELEASE build -- not okay. Memory allocation failures
should never be handled with *just* an ASSERT().

I hope you appreciate my insistence on DEBUG a bit more, through this
real-life example.

)

> or remove some other features.

Removing features on purpose can be called "offensive regression" in an
enterprise distro, and it is the antithesis of why people decide to pay
for enterprise support.

> In other words, at this point I don't think the size of these should
> be added into the equation for how 'full' the 2MB image is.

I think at this point it is clear that these patches are not appropriate
for upstream OvmfPkg. That's an acceptable closure for me -- while I
would have preferred to get the patches in, the real moral imperative
for me is to *try* the upstreaming at least, whenever I think the
patches are applicable to upstream. I think we've had an honest and
thorough discussion on this -- thank you very much for taking the time!
(Especially over the weekend!)

The current upstream 2MB build should continue working for everyday
purposes.

Cheers
Laszlo