Re: [Xen-devel] [PATCH v4 4/7] libxl: support mapping static shared memory areas during domain creation

[Julien Grall <julien.grall@xxxxxxx>]

On 02/06/2018 03:59 PM, Zhongze Liu wrote:
> Hi Julien,
Hi,


> 2018-02-06 21:07 GMT+08:00 Julien Grall <julien.grall@xxxxxxx>:
> > Hi,
> >
> > On 01/30/2018 05:50 PM, Zhongze Liu wrote:
> > > Add libxl__sshm_add to map shared pages from one DomU to another, The
> > > mapping
> > > process involves the follwing steps:
> [...]
>
> > > +
> > > +/* Set default values for libxl_static_shm */
> > > +int libxl__sshm_setdefault(libxl__gc *gc, uint32_t domid,
> > > +                           libxl_static_shm *sshm)
> > > +{
> > > +    int rc;
> > > +
> > > +    if (sshm->role == LIBXL_SSHM_ROLE_UNKNOWN)
> > > +        sshm->role = LIBXL_SSHM_ROLE_SLAVE;
> > > +    if (sshm->prot == LIBXL_SSHM_PROT_UNKNOWN)
> > > +        sshm->prot = LIBXL_SSHM_PROT_RW;
> >
> > What is the purpose of {ROLE,PROT}_UNKNOWN if you default it resp. to
> > ROLE_SLAVE and PROT_RW.  Would not it be easier to just drop them?
> The *_UNKNOWN values are used by the libxlu code to check whether a specific
> option was set more than once.
AFAIK, a toolstack is free to not use libxlu. Someone may implement 
their own toolstack on top of libxl and may use ROLE_UNKNOWN by mistake.
> Without the default *_UNKNOWN value, I will not
> be able to judge if, say, role is set to 'slave' by the user or not,
> and therefore, if I
> see the user setting role to 'master', I won't be able to tell if role
> is specified twice
> or not.
>
> I think treating re-specification of options as errors will be good
> for the users.
In that case, you should treat that as an error for everyone and not 
only xl. This would avoid confusion on other toolstack.
> [...]
>
> > > +
> > > +/*   libxl__sshm_do_map -- map pages into slave's physmap
> > > + *
> > > + *   This functions maps
> > > + *     master gfn: [@msshm->begin + @sshm->offset, @msshm->end +
> > > @sshm->offset)
> > > + *   into
> > > + *     slave gfn: [@sshm->begin, @sshm->end)
> > > + *
> > > + *   The gfns of the pages that are successfully mapped will be stored
> > > + *   in @mapped, and the number of the gfns will be stored in @nmapped.
> > > + *
> > > + *   The caller have to guarentee that sshm->begin < sshm->end and all
> > > the
> >
> > s/have to/has to/ I think.
> > s/guarentee/guarantee/
> >
> > > + *   values are page-aligned.
> >
> > Hmmm, I don't see the alignement check in libxl. So do you rely on the
> > toolstack to do it?
> Yes, This was done in libxlu_sshm.c.
Same remark as above regarding libxlu. Note that I am maintaining the 
tools. Ian and Wei may have a different opinion here.
Cheers,

--
Julien Grall

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