Re: [PATCH 01/10] xen/arm: introduce domain on Static Allocation

[Julien Grall <julien@xen.org>]

On 19/05/2021 03:22, Penny Zheng wrote:
> Hi Julien

Hi Penny,

>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: Tuesday, May 18, 2021 4:58 PM
>> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
>> sstabellini@kernel.org
>> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
>> <Wei.Chen@arm.com>; nd <nd@arm.com>
>> Subject: Re: [PATCH 01/10] xen/arm: introduce domain on Static Allocation
>>> +beginning, shall never go to heap allocator or boot allocator for any use.
>>> +
>>> +Domains on Static Allocation is supported through device tree
>>> +property `xen,static-mem` specifying reserved RAM banks as this domain's
>> guest RAM.
>>
>> I would suggest to use "physical RAM" when you refer to the host memory.
>>
>>> +By default, they shall be mapped to the fixed guest RAM address
>>> +`GUEST_RAM0_BASE`, `GUEST_RAM1_BASE`.
>>
>> There are a few bits that needs to clarified or part of the description:
>>     1) "By default" suggests there is an alternative possibility.
>> However, I don't see any.
>>     2) Will the first region of xen,static-mem be mapped to GUEST_RAM0_BASE
>> and the second to GUEST_RAM1_BASE? What if a third region is specificed?
>>     3) We don't guarantee the base address and the size of the banks.
>> Wouldn't it be better to let the admin select the region he/she wants?
>>     4) How do you determine the number of cells for the address and the size?
>>
> 
> The specific implementation on this part could be traced to the last commit
> https://patchew.org/Xen/20210518052113.725808-1-penny.zheng@arm.com/20210518052113.725808-11-penny.zheng@arm.com/

Right. My point is an admin should not have to read the code in order to 
figure out how the allocation works.

> 
> It will exhaust the GUEST_RAM0_SIZE, then seek to the GUEST_RAM1_BASE.
> GUEST_RAM0 may take up several regions.

Can this be clarified in the commit message.

> Yes, I may add the 1:1 direct-map scenario here to explain the alternative possibility

Ok. So I would suggest to remove "by default" until the implementation 
for direct-map is added.

> For the third point, are you suggesting that we could provide an option that let user
> also define guest memory base address/size?

When reading the document, I originally thought that the first region 
would be mapped to bank1, and then the second region mapped to bank2...

But from your reply, this is not the expected behavior. Therefore, 
please ignore my suggestion here.

> I'm confused on the fourth point, you mean the address cell and size cell for xen,static-mem = <...>?

Yes. This should be clarified in the document. See more below why?

> It will be consistent with the ones defined in the parent node, domUx.
Hmmm... To take the example you provided, the parent would be chosen. 
However, from the example, I would expect the property #{address, 
size}-cells in domU1 to be used. What did I miss?

>>> +Static Allocation is only supported on AArch64 for now.
>>
>> The code doesn't seem to be AArch64 specific. So why can't this be used for
>> 32-bit Arm?
>>
> 
> True, we have plans to make it also workable in AArch32 in the future.
> Because we considered XEN on cortex-R52.

All the code seems to be implemented in arm generic code. So isn't it 
already working?

>>>    static int __init early_scan_node(const void *fdt,
>>>                                      int node, const char *name, int depth,
>>>                                      u32 address_cells, u32 size_cells,
>>> @@ -345,6 +394,9 @@ static int __init early_scan_node(const void *fdt,
>>>            process_multiboot_node(fdt, node, name, address_cells, size_cells);
>>>        else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen") )
>>>            process_chosen_node(fdt, node, name, address_cells,
>>> size_cells);
>>> +    else if ( depth == 2 && fdt_get_property(fdt, node, "xen,static-mem",
>> NULL) )
>>> +        process_static_memory(fdt, node, "xen,static-mem", address_cells,
>>> +                              size_cells, &bootinfo.static_mem);
>>
>> I am a bit concerned to add yet another method to parse the DT and all the
>> extra code it will add like in patch #2.
>>
>>   From the host PoV, they are memory reserved for a specific purpose.
>> Would it be possible to consider the reserve-memory binding for that
>> purpose? This will happen outside of chosen, but we could use a phandle to
>> refer the region.
>>
> 
> Correct me if I understand wrongly, do you mean what this device tree snippet looks like:

Yes, this is what I had in mind. Although I have one small remark below.


> reserved-memory {
>     #address-cells = <2>;
>     #size-cells = <2>;
>     ranges;
>   
>     static-mem-domU1: static-mem@0x30000000{

I think the node would need to contain a compatible (name to be defined).

>        reg = <0x0 0x30000000 0x0 0x20000000>;
>     };
> };
> 
> Chosen {
>   ...
> domU1 {
>     xen,static-mem = <&static-mem-domU1>;
> };
> ...
> };
> 
>>>
>>>        if ( rc < 0 )
>>>            printk("fdt: node `%s': parsing failed\n", name); diff --git
>>> a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h index
>>> 5283244015..5e9f296760 100644
>>> --- a/xen/include/asm-arm/setup.h
>>> +++ b/xen/include/asm-arm/setup.h
>>> @@ -74,6 +74,8 @@ struct bootinfo {
>>>    #ifdef CONFIG_ACPI
>>>        struct meminfo acpi;
>>>    #endif
>>> +    /* Static Memory */
>>> +    struct meminfo static_mem;
>>>    };
>>>
>>>    extern struct bootinfo bootinfo;
>>>
>>
>> Cheers,
>>
>> --
>> Julien Grall

Cheers,

-- 
Julien Grall