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

[Vikram Garhwal <vikram.garhwal@xxxxxxx>]

Hi Oleksandr,

On 10/28/22 1:26 PM, Oleksandr Tyshchenko wrote:
> On Thu, Oct 27, 2022 at 12:24 PM Alex Bennée <alex.bennee@xxxxxxxxxx> 
> wrote:
> Hello all
>
>
>
>     Vikram Garhwal <vikram.garhwal@xxxxxxx> writes:
>
>     > xenstore_record_dm_state() will also be used in aarch64 xenpv
>     machine.
>     >
>     > Signed-off-by: Vikram Garhwal <vikram.garhwal@xxxxxxx>
>     > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>
>     > ---
>     >  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