Re: [Xen-devel] [PATCH v7 2/8] libxl: introduce a new structure to represent static shared memory regions

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 28 Aug 2018, Julien Grall wrote:
> Hi,
> 
> On 11/08/18 01:00, Stefano Stabellini wrote:
> > From: Zhongze Liu <blackskygg@xxxxxxxxx>
> > 
> > Author: Zhongze Liu <blackskygg@xxxxxxxxx>
> > 
> > Add a new structure to the IDL family to represent static shared memory
> > regions
> > as proposed in the proposal "Allow setting up shared memory areas between
> > VMs
> > from xl config file" (see [1]).
> > 
> > And deleted some trailing white spaces.
> > 
> > [1] https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.html
> > 
> > Signed-off-by: Zhongze Liu <blackskygg@xxxxxxxxx>
> > Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > Acked-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > 
> > Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> > Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > Cc: Julien Grall <julien.grall@xxxxxxx>
> > Cc: xen-devel@xxxxxxxxxxxxx
> > ---
> > Changes in v5:
> > - fix typos
> > - add LIBXL_HAVE_SSHM
> > - replace end with size
> > ---
> >   tools/libxl/libxl.h         |  6 ++++++
> >   tools/libxl/libxl_types.idl | 32 ++++++++++++++++++++++++++++++--
> >   2 files changed, 36 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > index ae2d63d..a9a523e 100644
> > --- a/tools/libxl/libxl.h
> > +++ b/tools/libxl/libxl.h
> > @@ -2455,6 +2455,12 @@ int libxl_fd_set_nonblock(libxl_ctx *ctx, int fd, int
> > nonblock);
> >   int libxl_qemu_monitor_command(libxl_ctx *ctx, uint32_t domid,
> >                                  const char *command_line, char **output);
> >   +#define LIBXL_HAVE_SSHM 1
> 
> Can you move this closer to the other LIBXL_HAVE_*? Also, you want to add a
> comment on top as we do for the others.

Yes, I'll do


> > +
> > +/* Constants for libxl_static_shm */
> > +#define LIBXL_SSHM_RANGE_UNKNOWN UINT64_MAX
> > +#define LIBXL_SSHM_ID_MAXLEN    128
> 
> Why do you need to bound the size of the string?

It is passed by the user, it is good to be clear about the max size?


> > +
> >   #include <libxl_event.h>
> >     #endif /* LIBXL_H */
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index 4a38580..e1fb975 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -565,10 +565,10 @@ libxl_domain_build_info = Struct("domain_build_info",[
> >                                          ("keymap",           string),
> >                                          ("sdl",
> > libxl_sdl_info),
> >                                          ("spice",
> > libxl_spice_info),
> > -
> > +
> >                                          ("gfx_passthru",
> > libxl_defbool),
> >                                          ("gfx_passthru_kind",
> > libxl_gfx_passthru_kind),
> > -
> > +
> >                                          ("serial",           string),
> >                                          ("boot",             string),
> >                                          ("usb",
> > libxl_defbool),
> > @@ -903,6 +903,33 @@ libxl_device_vsnd = Struct("device_vsnd", [
> >       ("pcms", Array(libxl_vsnd_pcm, "num_vsnd_pcms"))
> >       ])
> >   +libxl_sshm_cachepolicy = Enumeration("sshm_cachepolicy", [
> > +    (-1, "UNKNOWN"),
> > +    (0,  "ARM_NORMAL"),  # ARM policies should be < 32
> > +    (32,  "X86_NORMAL"), # X86 policies should be >= 32
> > +    ], init_val = "LIBXL_SSHM_CHCHE_POLICY_UNKNOWN")
> 
> Shouldn't this be LIBXL_SSHM_CACHEPOLICY_UNKNOWN?

well spotted, I'll fix

> > +
> > +libxl_sshm_prot = Enumeration("sshm_prot", [
> > +    (-1, "UNKNOWN"),
> > +    (3,  "RW"),
> > +    ], init_val = "LIBXL_SSHM_PROT_UNKNOWN")
> > +
> > +libxl_sshm_role = Enumeration("sshm_role", [
> > +    (-1, "UNKNOWN"),
> > +    (0,  "MASTER"),
> > +    (1,  "SLAVE"),
> > +    ], init_val = "LIBXL_SSHM_ROLE_UNKNOWN")
> > +
> > +libxl_static_shm = Struct("static_shm", [
> > +    ("id", string),
> > +    ("offset", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),
> > +    ("begin", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),
> > +    ("size", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),
> > +    ("prot", libxl_sshm_prot, {'init_val': 'LIBXL_SSHM_PROT_UNKNOWN'}),
> > +    ("cache_policy", libxl_sshm_cachepolicy, {'init_val':
> > 'LIBXL_SSHM_CACHEPOLICY_UNKNOWN'}),
> > +    ("role", libxl_sshm_role, {'init_val': 'LIBXL_SSHM_ROLE_UNKNOWN'}),
> > +])
> > +
> >   libxl_domain_config = Struct("domain_config", [
> >       ("c_info", libxl_domain_create_info),
> >       ("b_info", libxl_domain_build_info),
> > @@ -924,6 +951,7 @@ libxl_domain_config = Struct("domain_config", [
> >       ("channels", Array(libxl_device_channel, "num_channels")),
> >       ("usbctrls", Array(libxl_device_usbctrl, "num_usbctrls")),
> >       ("usbdevs", Array(libxl_device_usbdev, "num_usbdevs")),
> > +    ("sshms", Array(libxl_static_shm, "num_sshms")),
> >         ("on_poweroff", libxl_action_on_shutdown),
> >       ("on_reboot", libxl_action_on_shutdown),
> > 
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel