On 11/30/15 01:50, Jordan Justen wrote: > On 2015-11-26 16:26:57, Laszlo Ersek wrote: >> On 11/26/15 17:17, Laszlo Ersek wrote: >>> On 11/25/15 22:29, Jordan Justen wrote: >>>> On 2015-11-25 10:06:03, Laszlo Ersek wrote: >>>>> On 11/25/15 18:46, Jordan Justen wrote: >>>>>> On 2015-11-03 13:01:17, Laszlo Ersek wrote: >>>>>> >>>>>>> + qemu-system-i386 -cpu coreduo,-nx \ >>>>>> >>>>>> Sometimes we put '$' before a command prompt. I know there is no >>>>>> standard prompt, but that is the one that I usually use in examples. >>>>> >>>>> Hm, I'm not really consistent on that. When I paste a command line >>>>> mainly for illustration, I do use $. When I expect people to actually >>>>> paste the example / snippet into a script, or at the shell prompt >>>>> directly, then $ just gets in the way of easy line selection. I think in >>>>> this case I'd prefer to omit the dollar sign. But, you can convince me. >>>> >>>> It can get in the way if you just want to copy/paste several commands >>>> to run right away. I don't think that is the case here. It is just one >>>> command, and it is going to at least take two copy/pastes regardless. >>>> I'm not real concerned about this either way. >>> >>> Okay, I'll add the dollar sign to the lines where the binaries are entered. >>> >>>> >>>>>> >>>>>>> + >>>>>>> +Dependent on the development status of the >>>>>>> +"UefiCpuPkg/Universal/Acpi/S3Resume2Pei" module, S3 resume may not >>>>>>> work in >>>>>>> +OvmfPkg/OvmfPkgX64.dsc builds. In such cases, >>>>>>> OvmfPkg/OvmfPkgIa32X64.dsc is >>>>>>> +recommended for running X64 guests. >>>>>> >>>>>> Is this paragraph needed? >>>>> >>>>> I do think so. People (me included) tend to build the X64 DSC, unless >>>>> advised otherwie. >>>> >>>> Yeah. The order of relavence is definitely X64, then IA32, then >>>> IA32+X64. >>>> >>>>> >>>>>> I don't think we should have to say that >>>>>> UefiCpuPkg/S3Resume2Pei might be broken. Is it still broken? >>>>> >>>>> I believe so, yes. Jiewen outlined a number of changes required for the >>>>> PEIM: >>>>> >>>>> http://thread.gmane.org/gmane.comp.bios.edk2.devel/3509/focus=3523 >>>>> >>>> >>>> It sounds like we should have the X64 dsc generate a build error with >>>> SMM_REQUIRED until this is fixed. Maybe that can take the place of >>>> this comment in the README? >>> >>> I disagree with the build error. The X64 SMM_REQUIRE build can work >>> perfectly fine if you disable S3 in QEMU (that is: dynamically, on the >>> command line), or if you do enable S3, but never suspend & resume the guest. >>> >>> This README patch names the option separately that controls S3 ("-global >>> ICH9-LPC.disable_s3=[01]"). >>> >>> In fact, we (Red Hat) don't support the S3 enabled case (speaking >>> unofficially -- this has varied over time). It tends to expose bugs in >>> various components (like guest OS-level drivers). >>> >>> The protection that SMM gives (... is supposed to give) to the variable >>> driver stack is still worthwhile, with S3 disabled. >>> >>>> Jiewen, Mike, do you have a plan for solving the X64 PEI >>>> incompatibility? >>> >>> That would be nice, yes, but I don't think it should delay this series >>> even longer. As far as I understand, no Intel firmware that is released >>> for physical machines *and* uses SMM builds the module for 64-bit. So >>> that would be a first. I'm not sure what testing could be done outside >>> of OVMF, which might delay this even longer. >>> >>> Why are you against this paragraph in the README? > > I don't think it is strong enough of an action. We are already hanging > the system SMM is enabled when the VM doesn't support it. Why not just > hang in PEI if SMM & S3 is enabled while running in X64 PEI? > > Noting it here seems like we've buried in some fine print of the > README. :)
Fine by me. Can you please elaborate on your preference? (End of last week I submitted v5 (with your R-b on the updated README patch). Only three patches in that series need reviews for committing it, and I would like to commit it by Wednesday this week or so. The patches that need the reviews are listed in the v5 blurb.) Would you like me to post an incremental patch, or post a v6 with this update? Also, where exactly do you want me to hang the fw under such a (mis)config? I think SmmAccessPei is not appropriate. How about the InitializePlatform() function in "OvmfPkg/PlatformPei/Platform.c"? Actually, I would prefer if you could submit this patch, either as a separate / independent one, on top of my series, or else such that I can include it as-is at some point in an SMM v6 series. I'm fairly sure I won't get it right for your liking for the first time, and I'd like to avoid a v7 just because of this. (In truth I'd like to avoid even v6 because of this, but I'll let you decide if you wish to contribute that patch separately, or for inclusion in v6.) Thanks Laszlo > > -Jordan > >> >> Anyway, since you gave your R-b for this patch, I'm going to pick it up, >> and address all of your remarks, except the one about this paragraph -- >> I think it is prudent to keep it. It is very easy to remove it when the >> time comes, and I will very likely notice any changes to S3Resume2Pei in >> a timely manner (because I always skim fresh commits). >> >> Thanks >> Laszlo >> >>> >>> Thanks >>> Laszlo >>> >>>> >>>> -Jordan >>>> >>>>> This was on Oct 28th. >>>>> >>>>> And, whenever I fetch new commits from SVN, I always skim them quickly, >>>>> before I rebase my local branches on top, so I know what to expect and >>>>> what patches to edit (possibly) during the rebase. I don't recall any >>>>> related changes for S3Resume2Pei. >>>>> >>>>> But, it's not hard to check at once: >>>>> >>>>> $ git log -1 UefiCpuPkg/Universal/Acpi/S3Resume2Pei >>>>> >>>>> commit c1fd37cd6bcb98143bd4a44f427735a748058ad8 >>>>> Author: Hao Wu <hao.a...@intel.com> >>>>> Date: Mon Jul 13 01:24:24 2015 +0000 >>>>> >>>>> UefiCpuPkg S3Resume2Pei: Fix ASSERT in WriteToOsS3PerformanceData >>>>> >>>>> That is, July 2015. >>>>> >>>>> So yeah, we do need this hint in the README. We can remove it later. >>>>> >>>>>> Reviewed-by: Jordan Justen <jordan.l.jus...@intel.com> >>>>> >>>>> I'll await your response before picking this up (for just the >>>>> modifications that I agreed to above). >>>>> >>>>> Thanks! >>>>> Laszlo >>>>> >>>>>> >>>>>>> + >>>>>>> === Network Support === >>>>>>> >>>>>>> OVMF provides a UEFI network stack by default. Its lowest level driver >>>>>>> is the >>>>>>> -- >>>>>>> 1.8.3.1 >>>>>>> >>>>>>> _______________________________________________ >>>>>>> edk2-devel mailing list >>>>>>> edk2-devel@lists.01.org >>>>>>> https://lists.01.org/mailman/listinfo/edk2-devel >>>>> >>> >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org >>> https://lists.01.org/mailman/listinfo/edk2-devel >>> >> _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
