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

[Penny Zheng <penny.zheng@xxxxxxx>]

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.
> 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().
> I haven't looked closely at this series yet. It might be that these issues are 
> fixed. If not, I would definitely
> suggest to fix them in the first place.
>
> ~Michal