Re: [Xen-devel] [PATCH v8 6/8] docs: documentation about static shared memory regions

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Stefano Stabellini writes ("[PATCH v8 6/8] docs: documentation about static 
shared memory regions"):
> Author: Zhongze Liu <blackskygg@xxxxxxxxx>
> 
> Add docs to document the motivation, usage, use cases and other
> relevant information about the static shared memory feature.
> 
> This is for the proposal "Allow setting up shared memory areas between VMs
> from xl config file". See:

Thanks.  I'm reviewing this series and I obviously needed to start
with this patch so I could see what was going on.

> diff --git a/docs/man/xl-static-shm-configuration.pod.5 
> b/docs/man/xl-static-shm-configuration.pod.5
> new file mode 100644
> index 0000000..81ff3f1
> --- /dev/null
> +++ b/docs/man/xl-static-shm-configuration.pod.5
...
> +* Uniquely identified by a string that is no longer than 128 characters, 
> which
> +is called an B<identifier> in this document.
> +
> +* Backed by exactly one domain, which is called a B<master> domain, and all
> +the other domains who are also sharing this region are called B<slave>s.

Would it be possible to rename these concepts ?  Nowadays it is
generally preferable to avoid master/slave terminology.  Perhaps
`owning' and `borrowing' domains ?  I'm not sure exactly what the
precise relationship is.  It would be good to explain that by
explaining what the semantic difference is between the roles of the
owning and borrowing domains.

> +More formally, the string is a series of comma-separated keyword/value
> +pairs. Each parameter may be specified at most once. Default values apply if
> +the parameter is not specified.

Thanks for the formal syntax specification.

> +=item Description
> +
> +The unique identifier of the shared memory region.
> +
> +Every identifier could appear only once in each xl config file.

You need to specify in what scope this identifier is unique.
Obviously it is unique with an owning domain; but is is unique within
the host ?

Looking at the config syntax I think it is unique within the host,
because the borrowing domain config syntax does not specify the owning
domain.  So the statement
  Every identifier could appear only once in each xl config file.
is rather more limited than it could be.

Err, wait.  I have just noticed something.  You are saying a domain
cannot take on the slave role multiple times for the same ID
(presumably, with different offsets).

Why this limitation ?  It seems artificial (and is perhaps due to the
libxl xenstore control data structure).  I guess it's liveable with if
we don't bake it into the guest ABI or the config syntax.

> +=item B<begin>/B<size>
> +=item Description
> +The boundaries of the shared memory area.
> +=item Supported values
> +
> +Same with B<offset>.

Units, please.

Also, which memory namespace (address space) is this in ?  I think
guest [pseudo]physical.  (Does that mean that this feature is not ever
going to be available for PV such as x86 PV?)  Please state the
address space, anyway - ideally with an appropriate reference to some
architectural document which defines the various address spaces,
although I suspect there is no such document :-(.

The effect of this facility on the guest ABI needs to be stated.  I
think that specifying this stuff provides (as far as the guests are
concerned) a static area of guest pseudophysical space which is shared
between the various guests.

And there is no built-in way provided for the guest to discover that
this has happened ?  The guest is just supposed to know ?  If so that
seems unfortunate.  It would be nice to advertise this somehow.

> +=item Supported values
> +
> +Decimals or hexadecimals with a prefix "0x", and should be the multiple of 
> the
> +hypervisor page granularity (currently 4K on both ARM and x86).

Is it not also necessary to avoid certain guest pseudophysical areas
because they are reserved, or used by Xen or the tools for RAM, or
something ?

> +=item Description
> +
> +The backing area would be taken from one domain, which we will mark as
> +the "master domain", and this domain should be created prior to any
> +other slave domains that depend on it. The master's shared memory range
> +is NOT allocated in addition to its regular memory. Hence, it is usually
> +a good idea to choose a subrange of the regular guest memory allocation,
> +which starts at GUEST_RAM0_BASE, see xen/include/public/arch-arm.h.

This is not arch-neutral of course.

It is an unfortunate that this setup requires the system administrator
and guest to have intimate knowledge of Xen's approach to memory
layout.  It would be nice to have a way to improve that but I guess
the result would be much more complicated.

(Please forgive me if this has already been extensively discussed.)

> +The "slave domain" maps the memory of the master. The address of said
> +mapping should not be overlapping with the normal memory allocation of
> +the slave domain.

Ah, here is the text which I looked for in begin/end.  Maybe this
discussion about memory selection would benefit from being moved out
into its own part of the document.

In general this document seems to be structured around just the config
options and might benefit from a more explicit and longer `principles
of operation' or `description' where some of this text could go.

(I don't consider such a reorganisation critical to getting an ack for
this series though.)

> diff --git a/docs/misc/xenstore-paths.markdown 
> b/docs/misc/xenstore-paths.markdown
> index 33d2819..6ebc324 100644
> --- a/docs/misc/xenstore-paths.markdown
> +++ b/docs/misc/xenstore-paths.markdown
> @@ -174,6 +174,14 @@ than this amount of RAM.
>  
>  The size of the video RAM this domain is configured with.
>  
> +#### ~/static_shm/[_a-zA-Z0-9]+/role = ("master"|"slave") []
> +
> +(Note: Currently, this will only appear on ARM guests.)
> +
> +The role of this domain in the static shared memory region whose id matches
> +the `[_a-zA-Z0-9]+` part in the path. (Described in the manpage
> +**xl-static-shm-configuration(5)**).

This implies that the scope of the `unique id' is at least any group
of domains which do any of this static memory sharing, but leaves open
the possibility of multiple groups using the same id.  But I think
that this is excluded by the config syntax.

Would it make any kind of semantic sense for the same region to be
borrowed multiple times, or for a domain to borrow a region from
itself ?

These may seem silly but this kind of generality often turns out to be
useful.  See for example the use of the Xen vbd protocol back to dom0
for running bootloaders.

> +* master: the domid of the backing domain.
> +* slaves: information about the slaves that are sharing the region, see
> +  ** /libxl/static_shm/[_a-zA-Z0-9]+/slaves/$DOMID/* ** below.
> +* usercnt: An integer. This is the reference count of the backing memory 
> region,
> +  including the master domain itself. When this value reachies 0, the backing
> +  memory region will be freed.

Why is usercnt not equal to the count of
/libxl/static_shm/[_a-zA-Z0-9]+/slaves ?

If it is there is no need to store it separately, and doing so is
undesirable because it needlessly introduced the possibility of
illegal combinations of state.

> +#### /libxl/staitc_shm/[_a-zA-Z0-9]+/slaves/$DOMID/* []

Typo `staitc'.

Thanks,
Ian.

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