On 03/02/15 18:35, Jordan Justen wrote: > On 2015-03-02 09:13:22, Ard Biesheuvel wrote: >> This refactors the XenHypercallLib implementation for ARM and x86, >> and their callers, so that the init code for the XenHypercallLib >> is only called in places where we have already established that we >> are running as a Xen guest, or (in the XenConsoleSerialPortLib case) >> where ARM is the only user of the library, and its init code is a nop. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org> >> --- >> OvmfPkg/Include/Library/XenHypercallLib.h | 12 +++++++++ >> .../XenConsoleSerialPortLib.c | 7 +++++ >> OvmfPkg/Library/XenHypercallLib/XenHypercallArm.c | 30 >> ++++++++++++++++++++++ >> .../Library/XenHypercallLib/XenHypercallIntel.c | 8 +++--- > > How about following the X86Foo.c file naming convention I see used in > MdePkg/Library/BaseLib? I think sometimes Ia32/Foo.c will also be used > for items used on both IA32 and X64. > >> .../Library/XenHypercallLib/XenHypercallLibArm.inf | 1 + >> .../XenHypercallLib/XenHypercallLibIntel.inf | 1 - >> OvmfPkg/XenBusDxe/XenBusDxe.c | 7 +++++ >> 7 files changed, 61 insertions(+), 5 deletions(-) >> create mode 100644 OvmfPkg/Library/XenHypercallLib/XenHypercallArm.c >> >> diff --git a/OvmfPkg/Include/Library/XenHypercallLib.h >> b/OvmfPkg/Include/Library/XenHypercallLib.h >> index 1a468ea7dcc5..0e4b13d5397a 100644 >> --- a/OvmfPkg/Include/Library/XenHypercallLib.h >> +++ b/OvmfPkg/Include/Library/XenHypercallLib.h >> @@ -17,6 +17,18 @@ >> #define __XEN_HYPERCALL_LIB_H__ >> >> /** >> + Initialize the Xen hypercall library >> + >> + @return EFI_SUCCESS on success, or an EFI_xxx return code appropriate >> + for the failure mode on failure. >> +**/ >> +EFI_STATUS >> +EFIAPI >> +XenHypercallLibInit ( >> + VOID >> + ); >> + >> +/** > > In QemuFwCfgLib, we have QemuFwCfgIsAvailable, rather than directly > calling the constructor. > > I kind of prefer libraries to automatically run the constructor, but > maybe it doesn't work here? It sounded like maybe there was an > ordering issue?
No, not an ordering issue. The library constructor in this case can genuinely fail (when you run XenBusDxe and other stuff including the library on a non-Xen guest). In this case no ASSERT() should be tripped, instead the dependent module (in particular, XenBusDxe) should exit cleanly, as early as possible. I don't mind if the patch is reworked the way you suggest. Ie. the library constructor for the Intel flavor always runs, while the ARM flavor gets no constructor. Plus, a new XenHypercallIsAvailable() function is added to the library class. The ARM flavor would always return success; the Intel flavor dependent on the non-nullity of the HyperPage static variable. XenBusDxe would call XenHypercallIsAvailable() in its entry point, and exit cleanly with EFI_ABORTED if Xen were absent. The other library user, XenConsoleSerialPortLib, already has a constructor-level dependency on XenHypercallLib (which ensures construction order), so the SerialPortInitialize() constructor can also call XenHypercallIsAvailable(). Thanks Laszlo ------------------------------------------------------------------------------ Dive into the World of Parallel Programming The Go Parallel Website, sponsored by Intel and developed in partnership with Slashdot Media, is your hub for all things parallel software development, from weekly thought leadership blogs to news, videos, case studies, tutorials and more. Take a look and join the conversation now. http://goparallel.sourceforge.net/ _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
