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 _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
