[v1,09/12] accel/xen/xen-all: export xenstore_record_dm_state

Commit Message

Vikram Garhwal Oct. 15, 2022, 5:07 a.m. UTC

xenstore_record_dm_state() will also be used in aarch64 xenpv machine.

Signed-off-by: Vikram Garhwal <[e-mail]>
Signed-off-by: Stefano Stabellini <[e-mail]>
---
 accel/xen/xen-all.c  | 2 +-
 include/hw/xen/xen.h | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

Comments

Paul Durrant Oct. 19, 2022, 4:20 p.m. UTC | #1

On 15/10/2022 06:07, Vikram Garhwal wrote:
> xenstore_record_dm_state() will also be used in aarch64 xenpv machine.
> 
> Signed-off-by: Vikram Garhwal <[e-mail]>
> Signed-off-by: Stefano Stabellini <[e-mail]>

Reviewed-by: Paul Durrant <[e-mail]>

Alex Bennée Oct. 27, 2022, 9:14 a.m. UTC | #2

Vikram Garhwal <[e-mail]> writes:

> xenstore_record_dm_state() will also be used in aarch64 xenpv machine.
>
> Signed-off-by: Vikram Garhwal <[e-mail]>
> Signed-off-by: Stefano Stabellini <[e-mail]>
> ---
>  accel/xen/xen-all.c  | 2 +-
>  include/hw/xen/xen.h | 2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
> index 69aa7d018b..276625b78b 100644
> --- a/accel/xen/xen-all.c
> +++ b/accel/xen/xen-all.c
> @@ -100,7 +100,7 @@ void xenstore_store_pv_console_info(int i, Chardev *chr)
>  }
>  
>  
> -static void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
> +void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
>  {
>      char path[50];
>  
> diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
> index afdf9c436a..31e9538a5c 100644
> --- a/include/hw/xen/xen.h
> +++ b/include/hw/xen/xen.h
> @@ -9,6 +9,7 @@
>   */
>  
>  #include "exec/cpu-common.h"
> +#include <xenstore.h>

This is breaking a bunch of the builds and generally we try and avoid
adding system includes in headers (apart from osdep.h) for this reason.
In fact there is a comment just above to that fact.

I think you can just add struct xs_handle to typedefs.h (or maybe just
xen.h) and directly include xenstore.h in xen-all.c following the usual
rules:

  https://qemu.readthedocs.io/en/latest/devel/style.html#include-directives

It might be worth doing an audit to see what else is including xen.h
needlessly or should be using sysemu/xen.h. 

>  
>  /* xen-machine.c */
>  enum xen_mode {
> @@ -31,5 +32,6 @@ qemu_irq *xen_interrupt_controller_init(void);
>  void xenstore_store_pv_console_info(int i, Chardev *chr);
>  
>  void xen_register_framebuffer(struct MemoryRegion *mr);
> +void xenstore_record_dm_state(struct xs_handle *xs, const char *state);
>  
>  #endif /* QEMU_HW_XEN_H */

Oleksandr Tyshchenko Oct. 28, 2022, 8:26 p.m. UTC | #3

On Thu, Oct 27, 2022 at 12:24 PM Alex Bennée <[e-mail]> wrote:

Hello all



> Vikram Garhwal <[e-mail]> writes:
>
> > xenstore_record_dm_state() will also be used in aarch64 xenpv machine.
> >
> > Signed-off-by: Vikram Garhwal <[e-mail]>
> > Signed-off-by: Stefano Stabellini <[e-mail]>
> > ---
> >  accel/xen/xen-all.c  | 2 +-
> >  include/hw/xen/xen.h | 2 ++
> >  2 files changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
> > index 69aa7d018b..276625b78b 100644
> > --- a/accel/xen/xen-all.c
> > +++ b/accel/xen/xen-all.c
> > @@ -100,7 +100,7 @@ void xenstore_store_pv_console_info(int i, Chardev
> *chr)
> >  }
> >
> >
> > -static void xenstore_record_dm_state(struct xs_handle *xs, const char
> *state)
> > +void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
> >  {
> >      char path[50];
> >
> > diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
> > index afdf9c436a..31e9538a5c 100644
> > --- a/include/hw/xen/xen.h
> > +++ b/include/hw/xen/xen.h
> > @@ -9,6 +9,7 @@
> >   */
> >
> >  #include "exec/cpu-common.h"
> > +#include <xenstore.h>
>
> This is breaking a bunch of the builds and generally we try and avoid
> adding system includes in headers (apart from osdep.h) for this reason.
> In fact there is a comment just above to that fact.
>
> I think you can just add struct xs_handle to typedefs.h (or maybe just
> xen.h) and directly include xenstore.h in xen-all.c following the usual
> rules:
>
>
> https://qemu.readthedocs.io/en/latest/devel/style.html#include-directives
>
> It might be worth doing an audit to see what else is including xen.h
> needlessly or should be using sysemu/xen.h.
>
> >
> >  /* xen-machine.c */
> >  enum xen_mode {
> > @@ -31,5 +32,6 @@ qemu_irq *xen_interrupt_controller_init(void);
> >  void xenstore_store_pv_console_info(int i, Chardev *chr);
> >
> >  void xen_register_framebuffer(struct MemoryRegion *mr);
> > +void xenstore_record_dm_state(struct xs_handle *xs, const char *state);
> >
> >  #endif /* QEMU_HW_XEN_H */
>
>
> --
> Alex Bennée
>
>

For considering:
I think this patch and some other changes done in "[PATCH v1 10/12] hw/arm:
introduce xenpv machine" (the opening of Xen interfaces and
calling xenstore_record_dm_state() in hw/arm/xen_arm.c:xen_init_ioreq())
could be avoided if we enable the Xen accelerator (either by passing "-M
xenpv,accel=xen" or by adding mc->default_machine_opts = "accel=xen";
to hw/arm/xen_arm.c:xen_arm_machine_class_init() or by some other method).
These actions are already done in accel/xen/xen-all.c:xen_init(). Please
note, that I am not too familiar with that code, so there might be nuances.

Besides that, Xen accelerator will be needed for the xen-mapcache to be in
use (this is needed for mapping guest memory), there are a few
xen_enabled() checks spreading around that code to perform Xen specific
actions.

Vikram Garhwal Oct. 29, 2022, 5:22 a.m. UTC | #4

Thanks, Alex, for reviewing this one. I built for all the archs and it was fine. Can you please share more about what environment builds are breaking? So, I can test the changes for v2.

Regards,
Vikram

From: Alex Bennée <[e-mail]>
Date: Thursday, October 27, 2022 at 2:24 AM
To: Garhwal, Vikram <[e-mail]>
Cc: [e-mail] <[e-mail]>, Stabellini, Stefano <[e-mail]>, Stefano Stabellini <[e-mail]>, Anthony Perard <[e-mail]>, Paul Durrant <[e-mail]>, [e-mail] <[e-mail]>
Subject: Re: [PATCH v1 09/12] accel/xen/xen-all: export xenstore_record_dm_state

Vikram Garhwal <[e-mail]> writes:

> xenstore_record_dm_state() will also be used in aarch64 xenpv machine.
>
> Signed-off-by: Vikram Garhwal <[e-mail]>
> Signed-off-by: Stefano Stabellini <[e-mail]>
> ---
>  accel/xen/xen-all.c  | 2 +-
>  include/hw/xen/xen.h | 2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
> index 69aa7d018b..276625b78b 100644
> --- a/accel/xen/xen-all.c
> +++ b/accel/xen/xen-all.c
> @@ -100,7 +100,7 @@ void xenstore_store_pv_console_info(int i, Chardev *chr)
>  }
>
>
> -static void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
> +void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
>  {
>      char path[50];
>
> diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
> index afdf9c436a..31e9538a5c 100644
> --- a/include/hw/xen/xen.h
> +++ b/include/hw/xen/xen.h
> @@ -9,6 +9,7 @@
>   */
>
>  #include "exec/cpu-common.h"
> +#include <xenstore.h>

This is breaking a bunch of the builds and generally we try and avoid
adding system includes in headers (apart from osdep.h) for this reason.
In fact there is a comment just above to that fact.

I think you can just add struct xs_handle to typedefs.h (or maybe just
xen.h) and directly include xenstore.h in xen-all.c following the usual
rules:

  https://qemu.readthedocs.io/en/latest/devel/style.html#include-directives

It might be worth doing an audit to see what else is including xen.h
needlessly or should be using sysemu/xen.h.

>
>  /* xen-machine.c */
>  enum xen_mode {
> @@ -31,5 +32,6 @@ qemu_irq *xen_interrupt_controller_init(void);
>  void xenstore_store_pv_console_info(int i, Chardev *chr);
>
>  void xen_register_framebuffer(struct MemoryRegion *mr);
> +void xenstore_record_dm_state(struct xs_handle *xs, const char *state);
>
>  #endif /* QEMU_HW_XEN_H */


--
Alex Bennée

Vikram Garhwal Oct. 29, 2022, 5:36 a.m. UTC | #5

Hi Oleksandr,

On 10/28/22 1:26 PM, Oleksandr Tyshchenko wrote:
>
>
> On Thu, Oct 27, 2022 at 12:24 PM Alex Bennée <[e-mail]> 
> wrote:
>
> Hello all
>
>
>
>     Vikram Garhwal <[e-mail]> writes:
>
>     > xenstore_record_dm_state() will also be used in aarch64 xenpv
>     machine.
>     >
>     > Signed-off-by: Vikram Garhwal <[e-mail]>
>     > Signed-off-by: Stefano Stabellini <[e-mail]>
>     > ---
>     >  accel/xen/xen-all.c  | 2 +-
>     >  include/hw/xen/xen.h | 2 ++
>     >  2 files changed, 3 insertions(+), 1 deletion(-)
>     >
>     > diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
>     > index 69aa7d018b..276625b78b 100644
>     > --- a/accel/xen/xen-all.c
>     > +++ b/accel/xen/xen-all.c
>     > @@ -100,7 +100,7 @@ void xenstore_store_pv_console_info(int i,
>     Chardev *chr)
>     >  }
>     >
>     >
>     > -static void xenstore_record_dm_state(struct xs_handle *xs,
>     const char *state)
>     > +void xenstore_record_dm_state(struct xs_handle *xs, const char
>     *state)
>     >  {
>     >      char path[50];
>     >
>     > diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
>     > index afdf9c436a..31e9538a5c 100644
>     > --- a/include/hw/xen/xen.h
>     > +++ b/include/hw/xen/xen.h
>     > @@ -9,6 +9,7 @@
>     >   */
>     >
>     >  #include "exec/cpu-common.h"
>     > +#include <xenstore.h>
>
>     This is breaking a bunch of the builds and generally we try and avoid
>     adding system includes in headers (apart from osdep.h) for this
>     reason.
>     In fact there is a comment just above to that fact.
>
>     I think you can just add struct xs_handle to typedefs.h (or maybe just
>     xen.h) and directly include xenstore.h in xen-all.c following the
>     usual
>     rules:
>
>     https://qemu.readthedocs.io/en/latest/devel/style.html#include-directives
>
>     It might be worth doing an audit to see what else is including xen.h
>     needlessly or should be using sysemu/xen.h.
>
>     >
>     >  /* xen-machine.c */
>     >  enum xen_mode {
>     > @@ -31,5 +32,6 @@ qemu_irq *xen_interrupt_controller_init(void);
>     >  void xenstore_store_pv_console_info(int i, Chardev *chr);
>     >
>     >  void xen_register_framebuffer(struct MemoryRegion *mr);
>     > +void xenstore_record_dm_state(struct xs_handle *xs, const char
>     *state);
>     >
>     >  #endif /* QEMU_HW_XEN_H */
>
>
>     -- 
>     Alex Bennée
>
>
>
> For considering:
> I think this patch and some other changes done in "[PATCH v1 10/12] 
> hw/arm: introduce xenpv machine" (the opening of Xen interfaces and 
> calling xenstore_record_dm_state() in hw/arm/xen_arm.c:xen_init_ioreq())
> could be avoided if we enable the Xen accelerator (either by passing 
> "-M xenpv,accel=xen" or by adding mc->default_machine_opts = 
> "accel=xen"; to hw/arm/xen_arm.c:xen_arm_machine_class_init() or by 
> some other method).
> These actions are already done in accel/xen/xen-all.c:xen_init(). 
> Please note, that I am not too familiar with that code, so there might 
> be nuances.
>
> Besides that, Xen accelerator will be needed for the xen-mapcache to 
> be in use (this is needed for mapping guest memory), there are a few 
> xen_enabled() checks spreading around that code to perform Xen 
> specific actions.
>
Unfortunately, I am not that familiar with xen as accelerator function. 
Let me check and get back to you.
> -- 
> Regards,
>
> Oleksandr Tyshchenko

Alex Bennée Nov. 1, 2022, 11:29 a.m. UTC | #6

"Garhwal, Vikram" <[e-mail]> writes:

> Thanks, Alex, for reviewing this one. I built for all the archs and it was fine. Can you please share more about what
> environment builds are breaking? So, I can test the changes for v2.

My cross build environment failed:

  ../../configure' '--disable-docs' '--disable-tools' '--cross-prefix=aarch64-linux-gnu-' '--enable-xen' '--target-list=i386-softmmu,x86_64-softmmu,arm-softmmu,aarch64-softmmu' '--disable-tpm'

On a Debian Bullseye with:

  11:30:20 [root@zen:~] # dpkg -l libxen\*
  Desired=Unknown/Install/Remove/Purge/Hold
  | Status=Not/Inst/Conf-files/Unpacked/halF-conf/Half-inst/trig-aWait/Trig-pend
  |/ Err?=(none)/Reinst-required (Status,Err: uppercase=bad)
  ||/ Name                       Version                 Architecture Description
  +++-==========================-=======================-============-====================================================
  ii  libxen-dev:arm64           4.14.5+24-g87d90d511c-1 arm64        Public headers and libs for Xen
  ii  libxencall1:amd64          4.14.5+24-g87d90d511c-1 amd64        Xen runtime library - libxencall
  ii  libxencall1:arm64          4.14.5+24-g87d90d511c-1 arm64        Xen runtime library - libxencall
  ii  libxendevicemodel1:amd64   4.14.5+24-g87d90d511c-1 amd64        Xen runtime libraries - libxendevicemodel
  ii  libxendevicemodel1:arm64   4.14.5+24-g87d90d511c-1 arm64        Xen runtime libraries - libxendevicemodel
  ii  libxenevtchn1:amd64        4.14.5+24-g87d90d511c-1 amd64        Xen runtime libraries - libxenevtchn
  ii  libxenevtchn1:arm64        4.14.5+24-g87d90d511c-1 arm64        Xen runtime libraries - libxenevtchn
  ii  libxenforeignmemory1:amd64 4.14.5+24-g87d90d511c-1 amd64        Xen runtime libraries - libxenforeignmemory
  ii  libxenforeignmemory1:arm64 4.14.5+24-g87d90d511c-1 arm64        Xen runtime libraries - libxenforeignmemory
  ii  libxengnttab1:amd64        4.14.5+24-g87d90d511c-1 amd64        Xen runtime libraries - libxengnttab
  ii  libxengnttab1:arm64        4.14.5+24-g87d90d511c-1 arm64        Xen runtime libraries - libxengnttab
  ii  libxenhypfs1:amd64         4.14.5+24-g87d90d511c-1 amd64        Xen runtime library - libxenhypfs
  ii  libxenhypfs1:arm64         4.14.5+24-g87d90d511c-1 arm64        Xen runtime library - libxenhypfs
  ii  libxenmisc4.14:amd64       4.14.5+24-g87d90d511c-1 amd64        Xen runtime libraries - miscellaneous, versioned ABI
  ii  libxenmisc4.14:arm64       4.14.5+24-g87d90d511c-1 arm64        Xen runtime libraries - miscellaneous, versioned ABI
  ii  libxenstore3.0:amd64       4.14.5+24-g87d90d511c-1 amd64        Xen runtime libraries - libxenstore
  ii  libxenstore3.0:arm64       4.14.5+24-g87d90d511c-1 arm64        Xen runtime libraries - libxenstore
  ii  libxentoolcore1:amd64      4.14.5+24-g87d90d511c-1 amd64        Xen runtime libraries - libxentoolcore
  ii  libxentoolcore1:arm64      4.14.5+24-g87d90d511c-1 arm64        Xen runtime libraries - libxentoolcore
  ii  libxentoollog1:amd64       4.14.5+24-g87d90d511c-1 amd64        Xen runtime libraries - libxentoollog
  ii  libxentoollog1:arm64       4.14.5+24-g87d90d511c-1 arm64        Xen runtime libraries - libxentoollog

But also a bunch of cross builds on the CI system:

  https://gitlab.com/stsquad/qemu/-/pipelines/677956972/failures

>
>  
>
> Regards,
>
> Vikram
>
>  
>
> From: Alex Bennée <[e-mail]>
> Date: Thursday, October 27, 2022 at 2:24 AM
> To: Garhwal, Vikram <[e-mail]>
> Cc: [e-mail] <[e-mail]>, Stabellini, Stefano <[e-mail]>, Stefano
> Stabellini <[e-mail]>, Anthony Perard <[e-mail]>, Paul Durrant <[e-mail]>,
> [e-mail] <[e-mail]>
> Subject: Re: [PATCH v1 09/12] accel/xen/xen-all: export xenstore_record_dm_state
>
> Vikram Garhwal <[e-mail]> writes:
>
>> xenstore_record_dm_state() will also be used in aarch64 xenpv machine.
>>
>> Signed-off-by: Vikram Garhwal <[e-mail]>
>> Signed-off-by: Stefano Stabellini <[e-mail]>
>> ---
>>  accel/xen/xen-all.c  | 2 +-
>>  include/hw/xen/xen.h | 2 ++
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
>> index 69aa7d018b..276625b78b 100644
>> --- a/accel/xen/xen-all.c
>> +++ b/accel/xen/xen-all.c
>> @@ -100,7 +100,7 @@ void xenstore_store_pv_console_info(int i, Chardev *chr)
>>  }
>>  
>>  
>> -static void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
>> +void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
>>  {
>>      char path[50];
>>  
>> diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
>> index afdf9c436a..31e9538a5c 100644
>> --- a/include/hw/xen/xen.h
>> +++ b/include/hw/xen/xen.h
>> @@ -9,6 +9,7 @@
>>   */
>>  
>>  #include "exec/cpu-common.h"
>> +#include <xenstore.h>
>
> This is breaking a bunch of the builds and generally we try and avoid
> adding system includes in headers (apart from osdep.h) for this reason.
> In fact there is a comment just above to that fact.
>
> I think you can just add struct xs_handle to typedefs.h (or maybe just
> xen.h) and directly include xenstore.h in xen-all.c following the usual
> rules:
>
>   https://qemu.readthedocs.io/en/latest/devel/style.html#include-directives
>
> It might be worth doing an audit to see what else is including xen.h
> needlessly or should be using sysemu/xen.h. 
>
>>  
>>  /* xen-machine.c */
>>  enum xen_mode {
>> @@ -31,5 +32,6 @@ qemu_irq *xen_interrupt_controller_init(void);
>>  void xenstore_store_pv_console_info(int i, Chardev *chr);
>>  
>>  void xen_register_framebuffer(struct MemoryRegion *mr);
>> +void xenstore_record_dm_state(struct xs_handle *xs, const char *state);
>>  
>>  #endif /* QEMU_HW_XEN_H */

Patch

diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
index 69aa7d018b..276625b78b 100644
--- a/accel/xen/xen-all.c
+++ b/accel/xen/xen-all.c
@@ -100,7 +100,7 @@  void xenstore_store_pv_console_info(int i, Chardev *chr)
 }
 
 
-static void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
+void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
 {
     char path[50];
 
diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index afdf9c436a..31e9538a5c 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -9,6 +9,7 @@ 
  */
 
 #include "exec/cpu-common.h"
+#include <xenstore.h>
 
 /* xen-machine.c */
 enum xen_mode {
@@ -31,5 +32,6 @@  qemu_irq *xen_interrupt_controller_init(void);
 void xenstore_store_pv_console_info(int i, Chardev *chr);
 
 void xen_register_framebuffer(struct MemoryRegion *mr);
+void xenstore_record_dm_state(struct xs_handle *xs, const char *state);
 
 #endif /* QEMU_HW_XEN_H */