Re: [PATCH v3 0/8] Follow-up static shared memory PART I

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Penny,

On 22/08/2023 07:32, Penny Zheng wrote:
> 
> 
> Hi, michal
> 
> On 2023/8/21 18:49, Michal Orzel wrote:
>> Hi Penny,
>>
>> On 21/08/2023 06:00, Penny Zheng wrote:
>>>
>>>
>>> There are some unsolving issues on current 4.17 static shared memory
>>> feature[1], including:
>>> - In order to avoid keeping growing 'membank', having the shared memory
>>> info in separate structures is preferred.
>>> - Missing implementation on having the host address optional in
>>> "xen,shared-mem" property
>>> - Removing static shared memory from extended regions
>>> - Missing reference release on foreign superpage
>>> - Missing "xen,offset" feature, which is introduced in Linux DOC[2]
>>>
>>> All above objects have been divided into two parts to complete. And this
>>> patch serie is PART I.
>>>
>>> [1] 
>>> https://lore.kernel.org/all/20220908135513.1800511-1-Penny.Zheng@xxxxxxx/
>>> [2] 
>>> https://www.kernel.org/doc/Documentation/devicetree/bindings/reserved-memory/xen%2Cshared-memory.txt
>>
>> It looks like there is a problem with the changes introduced in this series.
>> The gitlab static shared memory tests failed:
>> https://gitlab.com/xen-project/patchew/xen/-/pipelines/973985190
>> No Xen logs meaning the failure occurred before serial console 
>> initialization.
>>
>> Now, I would like to share some observations after playing around with the 
>> current static shared mem code today.
>> 1) Static shared memory region is advertised to a domain by creating a child 
>> node under reserved-memory.
>> /reserved-memory is nothing but a way to carve out a region from the normal 
>> memory specified in /memory node.
>> For me, such regions should be described in domain's /memory node as well. 
>> This is not the case at the moment
>> for static shm unlike to other sub-nodes of /reserved-memory (present in 
>> host dtb) for which Xen creates separate
>> /memory nodes.
>>
> 
> Hmm, correct me if I'm wrong,
> If we describe twice in domain's /memory node too, it will be treated as
> normal memory, then any application could use it. The reason why we put
> static shm under /reserved-memory is that we only hope special driver,
> like static shm linux driver, could access it.
> 
> If you track down in make_memory_node(), only memory range that is
> reserved for device (or firmware) will be described twice as normal
> memory in Dom0. Memory like static shm, will get passed.

Reserved memory is a region of RAM (not MMIO) carved out for a special purpose 
which can be used by a driver for e.g. shared dma pool.
Therefore, such region shall be described both under /memory (used to present 
the total RAM and reserved memory is in RAM)
and under /reserved-memory. The OS parses /memory and then parses 
/reserved-memory to exclude the regions from normal usage
(there is also no-map property to tell OS not to create virtual mapping). So 
you do not need to worry about OS making use of something we marked as reserved.
This is exactly what Xen does if there are regions described as reserved in the 
host dtb:
1. Xen parses host dtb, adds reserved regions into bootinfo.reserved_mem so 
that it will not be used e.g. for allocator
2. While copying nodes from host dtb, Xen copies reserved memory nodes to dom0 
dtb and only maps the regions in p2m without permitting iomem access
3. Xen creates another /memory node to contain the reserved memory ranges

I guess static shm is not exception to this flow. It is part of RAM suited for 
memory sharing.

> 
>> 2) Domain dtb parsing issue with two /reserved-memory nodes present.
>> In case there is a /reserved-memory node already present in the host dtb, 
>> Xen would create yet another /reserved-memory
>> node for the static shm (to be observed in case of dom0). This is a bug as 
>> there can be only one /reserved-memory node.
>> This leads to an error when dumping with dtc and leads to a shm node not 
>> being visible to a domain (guest OS relies on
>> a presence of a single /reserved-memory node). The issue is because in 
>> make_resv_memory_node(), you are not checking if
>> such node already exists.
> 
> Yes, you're true.
> In Dom0, we could see two /reserved-memory nodes. I think, if there is a
> /reserved-memory node already present in the host dtb, we shall reserve
> it in kinfo for make_resv_memory_node().
This way you will only take the reference of a region but what about all the 
properties, node names, etc. that you need to copy?
This is why Xen first copies the reserved memory nodes from host dtb and then 
adds ranges to /memory.
In our shm case, we would need to insert the shm node into existing reserved 
memory node. This is a bit tricky as you can no longer
make use of fdt_{begin,end}_node and instead use the helpers operating on 
offsets.

~Michal