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

[Zhongze Liu <blackskygg@xxxxxxxxx>]

Hi Wei and Julien,

2018-02-07 1:47 GMT+08:00 Wei Liu <wei.liu2@xxxxxxxxxx>:
> 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@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.
>
> 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.

I agree that we should move the alignment check to libxl.

But I still think that re-specification checks could only be done in
libxlu, because
this could only be spotted in the parsing phase -- it shouldn't be
libxl's job. For
libxl, an *_UNKNOWN values just indicates that the user of libxl
didn't explicitly
assign a value to the option upon calling the constructor of the sshm struct,
and the code in libxl will set the option to its default value.
However, I do think
I failed to  handle the possibilities where an option value is not
not *_UNKNOWN and
not any valid values, which might occur if the user of libxl didn't
use the constructor
to initialize the sshm struct.

Cheers,

Zhongze Liu.

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