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

[Wei Liu <wei.liu2@citrix.com>]

On Tue, Feb 06, 2018 at 05:30:50PM +0000, Julien Grall wrote:
> 
> 
> 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@arm.com>:
> > > 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.

Yes.

> 
> > 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.
> 

Please move the check to libxl.

Wei.

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