whpx: fix compilation

[PATCH] whpx: fix compilation

Posted by marcandre.lureau@redhat.com 3 years, 4 months ago

From: Marc-André Lureau <marcandre.lureau@redhat.com>

When compiling WHPX (on msys2)

FAILED: libqemu-x86_64-softmmu.fa.p/target_i386_whpx_whpx-all.c.obj
../target/i386/whpx/whpx-all.c:29:10: fatal error: whp-dispatch.h: No such file or directory
   29 | #include "whp-dispatch.h"
      |          ^~~~~~~~~~~~~~~~

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/sysemu/whpx.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/sysemu/whpx.h b/include/sysemu/whpx.h
index 9346fd92e9..79ab3d73cf 100644
--- a/include/sysemu/whpx.h
+++ b/include/sysemu/whpx.h
@@ -15,7 +15,9 @@
 
 #ifdef CONFIG_WHPX
 
-#include "whp-dispatch.h"
+#include <windows.h>
+#include <WinHvPlatform.h>
+#include <WinHvEmulation.h>
 
 struct whpx_state {
     uint64_t mem_quota;
-- 
2.29.0

Re: [PATCH] whpx: fix compilation

Posted by Claudio Fontana 3 years, 4 months ago

On 12/18/20 9:46 AM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> When compiling WHPX (on msys2)
> 
> FAILED: libqemu-x86_64-softmmu.fa.p/target_i386_whpx_whpx-all.c.obj
> ../target/i386/whpx/whpx-all.c:29:10: fatal error: whp-dispatch.h: No such file or directory
>    29 | #include "whp-dispatch.h"
>       |          ^~~~~~~~~~~~~~~~
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/sysemu/whpx.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/sysemu/whpx.h b/include/sysemu/whpx.h
> index 9346fd92e9..79ab3d73cf 100644
> --- a/include/sysemu/whpx.h
> +++ b/include/sysemu/whpx.h
> @@ -15,7 +15,9 @@
>  
>  #ifdef CONFIG_WHPX
>  
> -#include "whp-dispatch.h"
> +#include <windows.h>
> +#include <WinHvPlatform.h>
> +#include <WinHvEmulation.h>
>  
>  struct whpx_state {
>      uint64_t mem_quota;
> 

Hi Marc, interesting factoid: cirrus-ci shows "GREEN", screaming all-ok for msys2, while the error is present,
and visible only on manual inspection of "Run main".

https://cirrus-ci.com/task/6573590369796096

Maybe something that needs attention CI-wise? Not sure who knows the details there, and how to fix it..
Maybe it's my move of whpx code to a subdirectory of target/i386/ that caused the breakage?

Ciao,

Claudio

Re: [PATCH] whpx: fix compilation

Posted by Paolo Bonzini 3 years, 4 months ago

On 18/12/20 09:46, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> When compiling WHPX (on msys2)
> 
> FAILED: libqemu-x86_64-softmmu.fa.p/target_i386_whpx_whpx-all.c.obj
> ../target/i386/whpx/whpx-all.c:29:10: fatal error: whp-dispatch.h: No such file or directory
>     29 | #include "whp-dispatch.h"
>        |          ^~~~~~~~~~~~~~~~
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   include/sysemu/whpx.h | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/sysemu/whpx.h b/include/sysemu/whpx.h
> index 9346fd92e9..79ab3d73cf 100644
> --- a/include/sysemu/whpx.h
> +++ b/include/sysemu/whpx.h
> @@ -15,7 +15,9 @@
>   
>   #ifdef CONFIG_WHPX
>   
> -#include "whp-dispatch.h"
> +#include <windows.h>
> +#include <WinHvPlatform.h>
> +#include <WinHvEmulation.h>
>   
>   struct whpx_state {
>       uint64_t mem_quota;
> 

This is wrong, just "git mv target/i386/whpx/whp-dispatch.h 
include/sysemu" instead (and possibly change the #include line to 
sysemu/whp-dispatch.h).

But I wonder if whp-dispatch.h is needed at all in this file.  If we can 
just include it in whpx-all.c and whpx-apic.c, that would be much better.

Paolo

Re: [PATCH] whpx: fix compilation

Posted by Marc-André Lureau 3 years, 4 months ago

Hi

On Fri, Dec 18, 2020 at 1:51 PM Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 18/12/20 09:46, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > When compiling WHPX (on msys2)
> >
> > FAILED: libqemu-x86_64-softmmu.fa.p/target_i386_whpx_whpx-all.c.obj
> > ../target/i386/whpx/whpx-all.c:29:10: fatal error: whp-dispatch.h: No
> such file or directory
> >     29 | #include "whp-dispatch.h"
> >        |          ^~~~~~~~~~~~~~~~
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >   include/sysemu/whpx.h | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/sysemu/whpx.h b/include/sysemu/whpx.h
> > index 9346fd92e9..79ab3d73cf 100644
> > --- a/include/sysemu/whpx.h
> > +++ b/include/sysemu/whpx.h
> > @@ -15,7 +15,9 @@
> >
> >   #ifdef CONFIG_WHPX
> >
> > -#include "whp-dispatch.h"
> > +#include <windows.h>
> > +#include <WinHvPlatform.h>
> > +#include <WinHvEmulation.h>
> >
> >   struct whpx_state {
> >       uint64_t mem_quota;
> >
>
> This is wrong, just "git mv target/i386/whpx/whp-dispatch.h
> include/sysemu" instead (and possibly change the #include line to
> sysemu/whp-dispatch.h).
>

Wrong? It fixes the build. So whatever is in whp-dispatch, it's not exactly
necessary. Why should it be included?

Furthermore, I tried your suggestion first. But it fails with other include
issues. I can probably solve them differently, but why should whp-dispatch
be in include/sysemu?


> But I wonder if whp-dispatch.h is needed at all in this file.  If we can
> just include it in whpx-all.c and whpx-apic.c, that would be much better.
>

May be, but how does this solve the issue with include/sysemu/whpx.h ?

Re: [PATCH] whpx: fix compilation

Posted by Paolo Bonzini 3 years, 4 months ago

On 18/12/20 11:08, Marc-André Lureau wrote:
>      >   #ifdef CONFIG_WHPX
>      >
>      > -#include "whp-dispatch.h"
>      > +#include <windows.h>
>      > +#include <WinHvPlatform.h>
>      > +#include <WinHvEmulation.h>
>      >
>      >   struct whpx_state {
>      >       uint64_t mem_quota;
>      >
> 
>     This is wrong, just "git mv target/i386/whpx/whp-dispatch.h
>     include/sysemu" instead (and possibly change the #include line to
>     sysemu/whp-dispatch.h).
> 
> 
> Wrong? It fixes the build. So whatever is in whp-dispatch, it's not 
> exactly necessary. Why should it be included?

Did you read the code that initializes whp-dispatch, or even easier try 
to find the patch the introduced it ("git log --follow 
target/i386/whpx/whp-dispatch.h")?  Marc-André, I expect you to know 
better than "it fixes the build, ergo it is correct"...

> Furthermore, I tried your suggestion first. But it fails with other 
> include issues. I can probably solve them differently, but why should 
> whp-dispatch be in include/sysemu?

It shouldn't indeed, hence the second suggestion:

>     But I wonder if whp-dispatch.h is needed at all in this file.  If we
>     can just include it in whpx-all.c and whpx-apic.c, that would be much
>     better.
> 
> May be, but how does this solve the issue with include/sysemu/whpx.h ?

Because "just" including it in those two .c files implies removing it 
from include/sysemu/whpx.h.  I was a bit concise, I admit.

Thanks,

Paolo

Re: [PATCH] whpx: fix compilation

Posted by Marc-André Lureau 3 years, 4 months ago

Hi

On Fri, Dec 18, 2020 at 2:28 PM Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 18/12/20 11:08, Marc-André Lureau wrote:
> >      >   #ifdef CONFIG_WHPX
> >      >
> >      > -#include "whp-dispatch.h"
> >      > +#include <windows.h>
> >      > +#include <WinHvPlatform.h>
> >      > +#include <WinHvEmulation.h>
> >      >
> >      >   struct whpx_state {
> >      >       uint64_t mem_quota;
> >      >
> >
> >     This is wrong, just "git mv target/i386/whpx/whp-dispatch.h
> >     include/sysemu" instead (and possibly change the #include line to
> >     sysemu/whp-dispatch.h).
> >
> >
> > Wrong? It fixes the build. So whatever is in whp-dispatch, it's not
> > exactly necessary. Why should it be included?
>
> Did you read the code that initializes whp-dispatch, or even easier try
> to find the patch the introduced it ("git log --follow
> target/i386/whpx/whp-dispatch.h")?  Marc-André, I expect you to know
> better than "it fixes the build, ergo it is correct"...
>
>
I did try to follow. And it ends up with:
commit 93d1499c8119989e3eb9a6936c5a18aaaaca6330
Author: Paolo Bonzini <pbonzini@redhat.com>
Date:   Wed Jun 6 15:41:58 2018 +0200

    whpx: commit missing file

    Not included by mistake in commit
327fccb288976f95808efa968082fc9d4a9ced84.

    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

then that commit didn't exactly help me to understand why this header
should be global in include/...



> > Furthermore, I tried your suggestion first. But it fails with other
> > include issues. I can probably solve them differently, but why should
> > whp-dispatch be in include/sysemu?
>
> It shouldn't indeed, hence the second suggestion:
>
> >     But I wonder if whp-dispatch.h is needed at all in this file.  If we
> >     can just include it in whpx-all.c and whpx-apic.c, that would be much
> >     better.
> >
> > May be, but how does this solve the issue with include/sysemu/whpx.h ?
>
> Because "just" including it in those two .c files implies removing it
> from include/sysemu/whpx.h.  I was a bit concise, I admit.
>
>
Perhaps, but whpx.h still is around with at least  WHV_PARTITION_HANDLE (if
not more from indirect includes with types from the windows headers)

So I still stand behind my suggestion. It merely reduces the amount of
includes to the windows system headers. We can try to further cleanup and
reduce the amount of includes or header later on. This is not my goal (why
do I always end up in side-track tasks that take days..)

Re: [PATCH] whpx: fix compilation

Posted by Paolo Bonzini 3 years, 4 months ago

You're right that the headers _are_ included in whp-dispatch.h, so yeah 
they do provide WHV_PARTITION_HANDLE which is the only thing that needs 
API includes.

But in turn whpx_state is only needed for whpx_apic_in_platform(), so 
the main issue is that sysemu/whpx.h is exposing unnecessary application 
details.  We can make two changes.  First, make whpx_apic_in_platform() 
a function instead of a macro, so that whpx_state can be moved to a new 
header whpx-internal.h.

In fact, whp-dispatch.h and the new header can be merged, too.  I'll 
send two patches.

Paolo