[PATCH] WHPX: refactor load library

Sunil Muthuswamy posted 1 patch 28 weeks ago
Test asan passed
Test checkpatch passed
Test FreeBSD passed
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Test docker-quick@centos7 passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/MW2PR2101MB1116386CFE4628B6767D6CDBC07B0@MW2PR2101MB1116.namprd21.prod.outlook.com
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Eduardo Habkost <ehabkost@redhat.com>, Richard Henderson <rth@twiddle.net>
target/i386/whp-dispatch.h |  4 +++
target/i386/whpx-all.c     | 84 +++++++++++++++++++++++++++++++---------------
2 files changed, 61 insertions(+), 27 deletions(-)

[PATCH] WHPX: refactor load library

Posted by Sunil Muthuswamy 28 weeks ago
This refactors the load library of WHV libraries to make it more
modular. It makes a helper routine that can be called on demand.
This allows future expansion of load library/functions to support
functionality that is depenedent on some feature being available.

Signed-off-by: Sunil Muthuswamy <sunilmut@microsoft.com>
---
 target/i386/whp-dispatch.h |  4 +++
 target/i386/whpx-all.c     | 84 +++++++++++++++++++++++++++++++---------------
 2 files changed, 61 insertions(+), 27 deletions(-)

diff --git a/target/i386/whp-dispatch.h b/target/i386/whp-dispatch.h
index 23791fbb47..87d049ceab 100644
--- a/target/i386/whp-dispatch.h
+++ b/target/i386/whp-dispatch.h
@@ -50,5 +50,9 @@ extern struct WHPDispatch whp_dispatch;
 
 bool init_whp_dispatch(void);
 
+typedef enum WHPFunctionList {
+    WINHV_PLATFORM_FNS_DEFAULT,
+    WINHV_EMULATION_FNS_DEFAULT,
+} WHPFunctionList;
 
 #endif /* WHP_DISPATCH_H */
diff --git a/target/i386/whpx-all.c b/target/i386/whpx-all.c
index ed95105eae..4688f40a65 100644
--- a/target/i386/whpx-all.c
+++ b/target/i386/whpx-all.c
@@ -1356,6 +1356,57 @@ static void whpx_handle_interrupt(CPUState *cpu, int mask)
     }
 }
 
+/*
+ * Load the functions from the given library, using the given handle. If a
+ * handle is provided, it is used, otherwise the library is opened. The
+ * handle will be updated on return with the opened one.
+ */
+static bool load_whp_dipatch_fns(HMODULE *handle, WHPFunctionList function_list)
+{
+    HMODULE hLib = *handle;
+
+    #define WINHV_PLATFORM_DLL "WinHvPlatform.dll"
+    #define WINHV_EMULATION_DLL "WinHvEmulation.dll"
+    #define WHP_LOAD_FIELD(return_type, function_name, signature) \
+        whp_dispatch.function_name = \
+            (function_name ## _t)GetProcAddress(hLib, #function_name); \
+        if (!whp_dispatch.function_name) { \
+            error_report("Could not load function %s", #function_name); \
+            goto error; \
+        } \
+
+    #define WHP_LOAD_LIB(lib_name, handle_lib) \
+    if (!handle_lib) { \
+        handle_lib = LoadLibrary(lib_name); \
+        if (!handle_lib) { \
+            error_report("Could not load library %s.", lib_name); \
+            goto error; \
+        } \
+    } \
+
+    switch (function_list) {
+    case WINHV_PLATFORM_FNS_DEFAULT:
+        WHP_LOAD_LIB(WINHV_PLATFORM_DLL, hLib)
+        LIST_WINHVPLATFORM_FUNCTIONS(WHP_LOAD_FIELD)
+        break;
+
+    case WINHV_EMULATION_FNS_DEFAULT:
+        WHP_LOAD_LIB(WINHV_EMULATION_DLL, hLib)
+        LIST_WINHVEMULATION_FUNCTIONS(WHP_LOAD_FIELD)
+        break;
+    }
+
+    *handle = hLib;
+    return true;
+
+error:
+    if (hLib) {
+        FreeLibrary(hWinHvEmulation);
+    }
+
+    return false;
+}
+
 /*
  * Partition support
  */
@@ -1491,51 +1542,30 @@ static void whpx_type_init(void)
 
 bool init_whp_dispatch(void)
 {
-    const char *lib_name;
-    HMODULE hLib;
-
     if (whp_dispatch_initialized) {
         return true;
     }
 
-    #define WHP_LOAD_FIELD(return_type, function_name, signature) \
-        whp_dispatch.function_name = \
-            (function_name ## _t)GetProcAddress(hLib, #function_name); \
-        if (!whp_dispatch.function_name) { \
-            error_report("Could not load function %s from library %s.", \
-                         #function_name, lib_name); \
-            goto error; \
-        } \
-
-    lib_name = "WinHvPlatform.dll";
-    hWinHvPlatform = LoadLibrary(lib_name);
-    if (!hWinHvPlatform) {
-        error_report("Could not load library %s.", lib_name);
+    if (!load_whp_dipatch_fns(&hWinHvPlatform, WINHV_PLATFORM_FNS_DEFAULT)) {
         goto error;
     }
-    hLib = hWinHvPlatform;
-    LIST_WINHVPLATFORM_FUNCTIONS(WHP_LOAD_FIELD)
 
-    lib_name = "WinHvEmulation.dll";
-    hWinHvEmulation = LoadLibrary(lib_name);
-    if (!hWinHvEmulation) {
-        error_report("Could not load library %s.", lib_name);
+    if (!load_whp_dipatch_fns(&hWinHvEmulation, WINHV_EMULATION_FNS_DEFAULT)) {
         goto error;
     }
-    hLib = hWinHvEmulation;
-    LIST_WINHVEMULATION_FUNCTIONS(WHP_LOAD_FIELD)
 
     whp_dispatch_initialized = true;
-    return true;
-
-    error:
 
+    return true;
+error:
     if (hWinHvPlatform) {
         FreeLibrary(hWinHvPlatform);
     }
+
     if (hWinHvEmulation) {
         FreeLibrary(hWinHvEmulation);
     }
+
     return false;
 }
 
-- 
2.16.4


Re: [PATCH] WHPX: refactor load library

Posted by Paolo Bonzini 28 weeks ago
On 08/11/19 21:31, Sunil Muthuswamy wrote:
> 
> +typedef enum WHPFunctionList {
> +    WINHV_PLATFORM_FNS_DEFAULT,
> +    WINHV_EMULATION_FNS_DEFAULT,
> +} WHPFunctionList;
>  

What does "default" stand for?  I assume you have more changes to this
function in the future.

> + * Load the functions from the given library, using the given handle. If a
> + * handle is provided, it is used, otherwise the library is opened. The
> + * handle will be updated on return with the opened one.
> + */
> +static bool load_whp_dipatch_fns(HMODULE *handle, WHPFunctionList function_list)
> +{

Typo, "dipatch" instead of "dispatch".
> 
> +    if (hLib) {
> +        FreeLibrary(hWinHvEmulation);
> +    }

The argument to FreeLibrary should be hLib.

Paolo


RE: [PATCH] WHPX: refactor load library

Posted by Sunil Muthuswamy 28 weeks ago

> -----Original Message-----
> From: Paolo Bonzini <pbonzini@redhat.com>
> Sent: Wednesday, November 13, 2019 7:00 AM
> To: Sunil Muthuswamy <sunilmut@microsoft.com>; Richard Henderson <rth@twiddle.net>; Eduardo Habkost <ehabkost@redhat.com>;
> Stefan Weil <sw@weilnetz.de>
> Cc: qemu-devel@nongnu.org; Justin Terry (VM) <juterry@microsoft.com>
> Subject: Re: [PATCH] WHPX: refactor load library
> 
> On 08/11/19 21:31, Sunil Muthuswamy wrote:
> >
> > +typedef enum WHPFunctionList {
> > +    WINHV_PLATFORM_FNS_DEFAULT,
> > +    WINHV_EMULATION_FNS_DEFAULT,
> > +} WHPFunctionList;
> >
> 
> What does "default" stand for?  I assume you have more changes to this
> function in the future.
> 
Yes, there are more functions coming, such as for XSAVE. I used "default" to represent
whatever is there currently, for lack of a better term.

> > + * Load the functions from the given library, using the given handle. If a
> > + * handle is provided, it is used, otherwise the library is opened. The
> > + * handle will be updated on return with the opened one.
> > + */
> > +static bool load_whp_dipatch_fns(HMODULE *handle, WHPFunctionList function_list)
> > +{
> 
> Typo, "dipatch" instead of "dispatch".
> >
> > +    if (hLib) {
> > +        FreeLibrary(hWinHvEmulation);
> > +    }
> 
> The argument to FreeLibrary should be hLib.
> 

Thanks, will fix these in the next version.

RE: [PATCH] WHPX: refactor load library

Posted by Sunil Muthuswamy 28 weeks ago

> -----Original Message-----
> From: Sunil Muthuswamy
> Sent: Friday, November 8, 2019 12:32 PM
> To: 'Paolo Bonzini' <pbonzini@redhat.com>; 'Richard Henderson' <rth@twiddle.net>; 'Eduardo Habkost' <ehabkost@redhat.com>; 'Stefan
> Weil' <sw@weilnetz.de>
> Cc: 'qemu-devel@nongnu.org' <qemu-devel@nongnu.org>; Justin Terry (VM) <juterry@microsoft.com>
> Subject: [PATCH] WHPX: refactor load library
> 
> This refactors the load library of WHV libraries to make it more
> modular. It makes a helper routine that can be called on demand.
> This allows future expansion of load library/functions to support
> functionality that is depenedent on some feature being available.
> 
> Signed-off-by: Sunil Muthuswamy <sunilmut@microsoft.com>
> ---

Can I possibly get some eyes on this?


Re: [PATCH] WHPX: refactor load library

Posted by Eduardo Habkost 28 weeks ago
On Tue, Nov 12, 2019 at 06:42:00PM +0000, Sunil Muthuswamy wrote:
> 
> 
> > -----Original Message-----
> > From: Sunil Muthuswamy
> > Sent: Friday, November 8, 2019 12:32 PM
> > To: 'Paolo Bonzini' <pbonzini@redhat.com>; 'Richard Henderson' <rth@twiddle.net>; 'Eduardo Habkost' <ehabkost@redhat.com>; 'Stefan
> > Weil' <sw@weilnetz.de>
> > Cc: 'qemu-devel@nongnu.org' <qemu-devel@nongnu.org>; Justin Terry (VM) <juterry@microsoft.com>
> > Subject: [PATCH] WHPX: refactor load library
> > 
> > This refactors the load library of WHV libraries to make it more
> > modular. It makes a helper routine that can be called on demand.
> > This allows future expansion of load library/functions to support
> > functionality that is depenedent on some feature being available.
> > 
> > Signed-off-by: Sunil Muthuswamy <sunilmut@microsoft.com>
> > ---
> 
> Can I possibly get some eyes on this?

I'd be glad to queue the patch if we get a Reviewed-by line from
somebody who understands Windows and WHPX.  Maybe Justin?

Sunil, Justin, would you like to be listed as maintainers or
designated reviewers for the WHPX code in QEMU?

-- 
Eduardo


Re: [PATCH] WHPX: refactor load library

Posted by Philippe Mathieu-Daudé 28 weeks ago
On 11/12/19 8:47 PM, Eduardo Habkost wrote:
> On Tue, Nov 12, 2019 at 06:42:00PM +0000, Sunil Muthuswamy wrote:
>>
>>
>>> -----Original Message-----
>>> From: Sunil Muthuswamy
>>> Sent: Friday, November 8, 2019 12:32 PM
>>> To: 'Paolo Bonzini' <pbonzini@redhat.com>; 'Richard Henderson' <rth@twiddle.net>; 'Eduardo Habkost' <ehabkost@redhat.com>; 'Stefan
>>> Weil' <sw@weilnetz.de>
>>> Cc: 'qemu-devel@nongnu.org' <qemu-devel@nongnu.org>; Justin Terry (VM) <juterry@microsoft.com>
>>> Subject: [PATCH] WHPX: refactor load library
>>>
>>> This refactors the load library of WHV libraries to make it more
>>> modular. It makes a helper routine that can be called on demand.
>>> This allows future expansion of load library/functions to support
>>> functionality that is depenedent on some feature being available.
>>>
>>> Signed-off-by: Sunil Muthuswamy <sunilmut@microsoft.com>
>>> ---
>>
>> Can I possibly get some eyes on this?
> 
> I'd be glad to queue the patch if we get a Reviewed-by line from
> somebody who understands Windows and WHPX.  Maybe Justin?

Can we wait for approval from the Microsoft legal department first?
So we can start testing WHPX builds, and reduce the possibilities to 
introduce regressions.

Testing is ready, we are waiting for Microsoft to merge, see:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg646351.html

> 
> Sunil, Justin, would you like to be listed as maintainers or
> designated reviewers for the WHPX code in QEMU?

Great idea!


RE: [PATCH] WHPX: refactor load library

Posted by Sunil Muthuswamy 28 weeks ago

> Can we wait for approval from the Microsoft legal department first?
> So we can start testing WHPX builds, and reduce the possibilities to
> introduce regressions.
> 
> Testing is ready, we are waiting for Microsoft to merge, see:
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.mail-archive.com%2Fqemu-
> devel%40nongnu.org%2Fmsg646351.html&amp;data=02%7C01%7Csunilmut%40microsoft.com%7C41ce65aedecb47c7bd0d08d76857937d
> %7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637092597657121219&amp;sdata=tu0zZDIzlG%2F9lEU4SJi11%2B%2FX1JdHUt6PD
> 2teeYCMZ%2B8%3D&amp;reserved=0
> 

Yes, we have escalated this to the right set of teams, including the legal
team. We are working through the processes here internally and will
update once we have something. Meanwhile, it would be good to see if
we can get these patches in.

> >
> > Sunil, Justin, would you like to be listed as maintainers or
> > designated reviewers for the WHPX code in QEMU?
> 
> Great idea!
It's a valid and good point. I am discussing this internally here and
will get back.


Re: [PATCH] WHPX: refactor load library

Posted by Eduardo Habkost 28 weeks ago
On Wed, Nov 13, 2019 at 05:35:59PM +0100, Philippe Mathieu-Daudé wrote:
> On 11/12/19 8:47 PM, Eduardo Habkost wrote:
> > On Tue, Nov 12, 2019 at 06:42:00PM +0000, Sunil Muthuswamy wrote:
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: Sunil Muthuswamy
> > > > Sent: Friday, November 8, 2019 12:32 PM
> > > > To: 'Paolo Bonzini' <pbonzini@redhat.com>; 'Richard Henderson' <rth@twiddle.net>; 'Eduardo Habkost' <ehabkost@redhat.com>; 'Stefan
> > > > Weil' <sw@weilnetz.de>
> > > > Cc: 'qemu-devel@nongnu.org' <qemu-devel@nongnu.org>; Justin Terry (VM) <juterry@microsoft.com>
> > > > Subject: [PATCH] WHPX: refactor load library
> > > > 
> > > > This refactors the load library of WHV libraries to make it more
> > > > modular. It makes a helper routine that can be called on demand.
> > > > This allows future expansion of load library/functions to support
> > > > functionality that is depenedent on some feature being available.
> > > > 
> > > > Signed-off-by: Sunil Muthuswamy <sunilmut@microsoft.com>
> > > > ---
> > > 
> > > Can I possibly get some eyes on this?
> > 
> > I'd be glad to queue the patch if we get a Reviewed-by line from
> > somebody who understands Windows and WHPX.  Maybe Justin?
> 
> Can we wait for approval from the Microsoft legal department first?
> So we can start testing WHPX builds, and reduce the possibilities to
> introduce regressions.
> 
> Testing is ready, we are waiting for Microsoft to merge, see:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg646351.html

Making it easier for other people to test WHPX would be nice.
But in case this is not sorted out soon, I don't see a reason to
not merge WHPX changes if they are reviewed and approved by the
main author of that code (Justin).

-- 
Eduardo


RE: [PATCH] WHPX: refactor load library

Posted by Sunil Muthuswamy 28 weeks ago
> Making it easier for other people to test WHPX would be nice.

Yes, we understand the concerns and I generally agree here. I am
trying to connect the different teams involved here (legal, SDK here)
and connect the dots for them, to see what can be done here.

> But in case this is not sorted out soon, I don't see a reason to
> not merge WHPX changes if they are reviewed and approved by the
> main author of that code (Justin).
> 

Justin is ready to review it, but is out for another week. Will have him
review once he is back.