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

[Julien Grall <julien.grall@arm.com>]

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.

> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel