| Message ID | 20210518052113.725808-2-penny.zheng@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Domain on Static Allocation | expand |
Hi Penny, On 18/05/2021 06:21, Penny Zheng wrote: > Static Allocation refers to system or sub-system(domains) for which memory > areas are pre-defined by configuration using physical address ranges. > Those pre-defined memory, -- Static Momery, as parts of RAM reserved in the s/Momery/Memory/ > 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. > By default, they shall be mapped to the fixed guest RAM address > `GUEST_RAM0_BASE`, `GUEST_RAM1_BASE`. > > This patch introduces this new `xen,static-mem` property to define static > memory nodes in device tree file. > This patch also documents and parses this new attribute at boot time and > stores related info in static_mem for later initialization. > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > --- > docs/misc/arm/device-tree/booting.txt | 33 +++++++++++++++++ > xen/arch/arm/bootfdt.c | 52 +++++++++++++++++++++++++++ > xen/include/asm-arm/setup.h | 2 ++ > 3 files changed, 87 insertions(+) > > diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt > index 5243bc7fd3..d209149d71 100644 > --- a/docs/misc/arm/device-tree/booting.txt > +++ b/docs/misc/arm/device-tree/booting.txt > @@ -268,3 +268,36 @@ The DTB fragment is loaded at 0xc000000 in the example above. It should > follow the convention explained in docs/misc/arm/passthrough.txt. The > DTB fragment will be added to the guest device tree, so that the guest > kernel will be able to discover the device. > + > + > +Static Allocation > +============= > + > +Static Allocation refers to system or sub-system(domains) for which memory > +areas are pre-defined by configuration using physical address ranges. > +Those pre-defined memory, -- Static Momery, as parts of RAM reserved in the s/Momery/Memory/ > +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? > +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? > + > +The dtb property should look like as follows: > + > + chosen { > + domU1 { > + compatible = "xen,domain"; > + #address-cells = <0x2>; > + #size-cells = <0x2>; > + cpus = <2>; > + xen,static-mem = <0x0 0x30000000 0x0 0x20000000>; > + > + ... > + }; > + }; > + > +DOMU1 on Static Allocation has reserved RAM bank at 0x30000000 of 512MB size Do you mean "DomU1 will have a static memory of 512MB reserved from the physical address..."? > +as guest RAM. > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > index dcff512648..e9f14e6a44 100644 > --- a/xen/arch/arm/bootfdt.c > +++ b/xen/arch/arm/bootfdt.c > @@ -327,6 +327,55 @@ static void __init process_chosen_node(const void *fdt, int node, > add_boot_module(BOOTMOD_RAMDISK, start, end-start, false); > } > > +static int __init process_static_memory(const void *fdt, int node, > + const char *name, > + u32 address_cells, u32 size_cells, > + void *data) > +{ > + int i; > + int banks; > + const __be32 *cell; > + paddr_t start, size; > + u32 reg_cells = address_cells + size_cells; > + struct meminfo *mem = data; > + const struct fdt_property *prop; > + > + if ( address_cells < 1 || size_cells < 1 ) > + { > + printk("fdt: invalid #address-cells or #size-cells for static memory"); > + return -EINVAL; > + } > + > + /* > + * Check if static memory property belongs to a specific domain, that is, > + * its node `domUx` has compatible string "xen,domain". > + */ > + if ( fdt_node_check_compatible(fdt, node, "xen,domain") != 0 ) > + printk("xen,static-mem property can only locate under /domUx node.\n"); > + > + prop = fdt_get_property(fdt, node, name, NULL); > + if ( !prop ) > + return -ENOENT; > + > + cell = (const __be32 *)prop->data; > + banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32)); > + > + for ( i = 0; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ ) > + { > + device_tree_get_reg(&cell, address_cells, size_cells, &start, &size); > + /* Some DT may describe empty bank, ignore them */ > + if ( !size ) > + continue; > + mem->bank[mem->nr_banks].start = start; > + mem->bank[mem->nr_banks].size = size; > + mem->nr_banks++; > + } > + > + if ( i < banks ) > + return -ENOSPC; > + return 0; > +} > + > 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. > > 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,
Hi Julien > -----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 > > Hi Penny, > > On 18/05/2021 06:21, Penny Zheng wrote: > > Static Allocation refers to system or sub-system(domains) for which > > memory areas are pre-defined by configuration using physical address > ranges. > > Those pre-defined memory, -- Static Momery, as parts of RAM reserved > > in the > > s/Momery/Memory/ Oh, thx! > > > 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. > > By default, they shall be mapped to the fixed guest RAM address > > `GUEST_RAM0_BASE`, `GUEST_RAM1_BASE`. > > > > This patch introduces this new `xen,static-mem` property to define > > static memory nodes in device tree file. > > This patch also documents and parses this new attribute at boot time > > and stores related info in static_mem for later initialization. > > > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > > --- > > docs/misc/arm/device-tree/booting.txt | 33 +++++++++++++++++ > > xen/arch/arm/bootfdt.c | 52 +++++++++++++++++++++++++++ > > xen/include/asm-arm/setup.h | 2 ++ > > 3 files changed, 87 insertions(+) > > > > diff --git a/docs/misc/arm/device-tree/booting.txt > > b/docs/misc/arm/device-tree/booting.txt > > index 5243bc7fd3..d209149d71 100644 > > --- a/docs/misc/arm/device-tree/booting.txt > > +++ b/docs/misc/arm/device-tree/booting.txt > > @@ -268,3 +268,36 @@ The DTB fragment is loaded at 0xc000000 in the > example above. It should > > follow the convention explained in docs/misc/arm/passthrough.txt. The > > DTB fragment will be added to the guest device tree, so that the guest > > kernel will be able to discover the device. > > + > > + > > +Static Allocation > > +============= > > + > > +Static Allocation refers to system or sub-system(domains) for which > > +memory areas are pre-defined by configuration using physical address > ranges. > > +Those pre-defined memory, -- Static Momery, as parts of RAM reserved > > +in the > > s/Momery/Memory/ > Oh, thx > > +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/ It will exhaust the GUEST_RAM0_SIZE, then seek to the GUEST_RAM1_BASE. GUEST_RAM0 may take up several regions. Yes, I may add the 1:1 direct-map scenario here to explain the alternative possibility For the third point, are you suggesting that we could provide an option that let user also define guest memory base address/size? I'm confused on the fourth point, you mean the address cell and size cell for xen,static-mem = <...>? It will be consistent with the ones defined in the parent node, domUx. > > +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. > > + > > +The dtb property should look like as follows: > > + > > + chosen { > > + domU1 { > > + compatible = "xen,domain"; > > + #address-cells = <0x2>; > > + #size-cells = <0x2>; > > + cpus = <2>; > > + xen,static-mem = <0x0 0x30000000 0x0 0x20000000>; > > + > > + ... > > + }; > > + }; > > + > > +DOMU1 on Static Allocation has reserved RAM bank at 0x30000000 of > > +512MB size > > Do you mean "DomU1 will have a static memory of 512MB reserved from the > physical address..."? > Yes, yes. You phrase it more clearly, thx > > +as guest RAM. > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index > > dcff512648..e9f14e6a44 100644 > > --- a/xen/arch/arm/bootfdt.c > > +++ b/xen/arch/arm/bootfdt.c > > @@ -327,6 +327,55 @@ static void __init process_chosen_node(const void > *fdt, int node, > > add_boot_module(BOOTMOD_RAMDISK, start, end-start, false); > > } > > > > +static int __init process_static_memory(const void *fdt, int node, > > + const char *name, > > + u32 address_cells, u32 size_cells, > > + void *data) { > > + int i; > > + int banks; > > + const __be32 *cell; > > + paddr_t start, size; > > + u32 reg_cells = address_cells + size_cells; > > + struct meminfo *mem = data; > > + const struct fdt_property *prop; > > + > > + if ( address_cells < 1 || size_cells < 1 ) > > + { > > + printk("fdt: invalid #address-cells or #size-cells for static memory"); > > + return -EINVAL; > > + } > > + > > + /* > > + * Check if static memory property belongs to a specific domain, that is, > > + * its node `domUx` has compatible string "xen,domain". > > + */ > > + if ( fdt_node_check_compatible(fdt, node, "xen,domain") != 0 ) > > + printk("xen,static-mem property can only locate under /domUx > > + node.\n"); > > + > > + prop = fdt_get_property(fdt, node, name, NULL); > > + if ( !prop ) > > + return -ENOENT; > > + > > + cell = (const __be32 *)prop->data; > > + banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32)); > > + > > + for ( i = 0; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ ) > > + { > > + device_tree_get_reg(&cell, address_cells, size_cells, &start, &size); > > + /* Some DT may describe empty bank, ignore them */ > > + if ( !size ) > > + continue; > > + mem->bank[mem->nr_banks].start = start; > > + mem->bank[mem->nr_banks].size = size; > > + mem->nr_banks++; > > + } > > + > > + if ( i < banks ) > > + return -ENOSPC; > > + return 0; > > +} > > + > > 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: reserved-memory { #address-cells = <2>; #size-cells = <2>; ranges; static-mem-domU1: static-mem@0x30000000{ 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
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,
Hi Julien > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: Thursday, May 20, 2021 2:27 AM > 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 > > 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/20 > > 210518052113.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. > Ok, I will expand commit to let it be clarified more clearly. > > 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. > Ok, will do. Thx. > > 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? > Yeah, the property #{address, size}-cells in domU1 will be used. And the parent node will be domU1. The dtb property should look like as follows: chosen { domU1 { compatible = "xen,domain"; #address-cells = <0x2>; #size-cells = <0x2>; cpus = <2>; xen,static-mem = <0x0 0x30000000 0x0 0x20000000>; ... }; }; > > +DOMU1 on Static Allocation has reserved RAM bank at 0x30000000 of 512MB size > >>> +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). > Ok, maybe, hmmm, how about "xen,static-memory"? > > 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 Cheers Penny Zheng
Hi, On 20/05/2021 07:07, Penny Zheng wrote: >>> 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? >> > > Yeah, the property #{address, size}-cells in domU1 will be used. And the parent > node will be domU1. You may have misunderstood what I meant. "domU1" is the node that contains the property "xen,static-mem". The parent node would be the one above (in our case "chosen"). > > The dtb property should look like as follows: > > chosen { > domU1 { > compatible = "xen,domain"; > #address-cells = <0x2>; > #size-cells = <0x2>; > cpus = <2>; > xen,static-mem = <0x0 0x30000000 0x0 0x20000000>; > > ... > }; > }; > >>> +DOMU1 on Static Allocation has reserved RAM bank at 0x30000000 of 512MB size > >>>>> +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). >> > > Ok, maybe, hmmm, how about "xen,static-memory"? I would possibly add "domain" in the name to make clear this is domain memory. Stefano, what do you think? Cheers,
Hi Julien > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: Thursday, May 20, 2021 4:51 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 > > Hi, > > On 20/05/2021 07:07, Penny Zheng wrote: > >>> 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? > >> > > > > Yeah, the property #{address, size}-cells in domU1 will be used. And > > the parent node will be domU1. > > You may have misunderstood what I meant. "domU1" is the node that > contains the property "xen,static-mem". The parent node would be the one > above (in our case "chosen"). > I re-re-reconsider what you meant here, hope this time I get what you mean, correct me if I'm wrong, List an example here: / { reserved-memory { #address-cells = <2>; #size-cells = <2>; staticmemdomU1: static-memory@0x30000000 { compatible = "xen,static-memory-domain"; reg = <0x0 0x30000000 0x0 0x20000000>; }; }; chosen { domU1 { compatible = "xen,domain"; #address-cells = <0x1>; #size-cells = <0x1>; cpus = <2>; xen,static-mem = <&staticmemdomU1>; ... }; }; }; If user gives two different #address-cells and #size-cells in reserved-memory and domU1, Then when parsing it through `xen,static-mem`, it may have unexpected answers. I could not think a way to fix it properly in codes, do you have any suggestion? Or we just put a warning in doc/commits. > > > > The dtb property should look like as follows: > > > > chosen { > > domU1 { > > compatible = "xen,domain"; > > #address-cells = <0x2>; > > #size-cells = <0x2>; > > cpus = <2>; > > xen,static-mem = <0x0 0x30000000 0x0 0x20000000>; > > > > ... > > }; > > }; > > > >>> +DOMU1 on Static Allocation has reserved RAM bank at 0x30000000 of > >>> +512MB size > > > >>>>> +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). > >> > > > > Ok, maybe, hmmm, how about "xen,static-memory"? > > I would possibly add "domain" in the name to make clear this is domain > memory. Stefano, what do you think? > > Cheers, > > > Julien Grall Cheers, Penny Zheng
Hi, On 02/06/2021 11:09, Penny Zheng wrote: > Hi Julien > >> -----Original Message----- >> From: Julien Grall <julien@xen.org> >> Sent: Thursday, May 20, 2021 4:51 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 >> >> Hi, >> >> On 20/05/2021 07:07, Penny Zheng wrote: >>>>> 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? >>>> >>> >>> Yeah, the property #{address, size}-cells in domU1 will be used. And >>> the parent node will be domU1. >> >> You may have misunderstood what I meant. "domU1" is the node that >> contains the property "xen,static-mem". The parent node would be the one >> above (in our case "chosen"). >> > > I re-re-reconsider what you meant here, hope this time I get what you mean, correct me if I'm wrong, > List an example here: > > / { > reserved-memory { > #address-cells = <2>; > #size-cells = <2>; > > staticmemdomU1: static-memory@0x30000000 { > compatible = "xen,static-memory-domain"; > reg = <0x0 0x30000000 0x0 0x20000000>; > }; > }; > > chosen { > domU1 { > compatible = "xen,domain"; > #address-cells = <0x1>; > #size-cells = <0x1>; > cpus = <2>; > xen,static-mem = <&staticmemdomU1>; > > ... > }; > }; > }; > > If user gives two different #address-cells and #size-cells in reserved-memory and domU1, Then when parsing it > through `xen,static-mem`, it may have unexpected answers. Why are you using the #address-cells and #size-cells from the node domU1 to parse staticmemdomU1? > I could not think a way to fix it properly in codes, do you have any suggestion? Or we just put a warning in doc/commits. The correct approach is to find the parent of staticmemdomU1 (i.e. reserved-memory) and use the #address-cells and #size-cells from there. Cheers,
I have not read most emails in this thread (sorry!) but I spotted this discussion about device tree and I would like to reply to that as we have discussed something very similar in the context of system device tree. On Thu, 3 Jun 2021, Julien Grall wrote: > On 02/06/2021 11:09, Penny Zheng wrote: > > Hi Julien > > > > > -----Original Message----- > > > From: Julien Grall <julien@xen.org> > > > Sent: Thursday, May 20, 2021 4:51 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 > > > > > > Hi, > > > > > > On 20/05/2021 07:07, Penny Zheng wrote: > > > > > > 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? > > > > > > > > > > > > > Yeah, the property #{address, size}-cells in domU1 will be used. And > > > > the parent node will be domU1. > > > > > > You may have misunderstood what I meant. "domU1" is the node that > > > contains the property "xen,static-mem". The parent node would be the one > > > above (in our case "chosen"). > > > > > > > I re-re-reconsider what you meant here, hope this time I get what you mean, > > correct me if I'm wrong, > > List an example here: > > > > / { > > reserved-memory { > > #address-cells = <2>; > > #size-cells = <2>; > > > > staticmemdomU1: static-memory@0x30000000 { > > compatible = "xen,static-memory-domain"; > > reg = <0x0 0x30000000 0x0 0x20000000>; > > }; > > }; > > > > chosen { > > domU1 { > > compatible = "xen,domain"; > > #address-cells = <0x1>; > > #size-cells = <0x1>; > > cpus = <2>; > > xen,static-mem = <&staticmemdomU1>; > > > > ... > > }; > > }; > > }; > > > > If user gives two different #address-cells and #size-cells in > > reserved-memory and domU1, Then when parsing it > > through `xen,static-mem`, it may have unexpected answers. > > Why are you using the #address-cells and #size-cells from the node domU1 to > parse staticmemdomU1? > > > I could not think a way to fix it properly in codes, do you have any > > suggestion? Or we just put a warning in doc/commits. > > The correct approach is to find the parent of staticmemdomU1 (i.e. > reserved-memory) and use the #address-cells and #size-cells from there. Julien is right about how to parse the static-memory. But I have a suggestion on the new binding. The /reserved-memory node is a weird node: it is one of the very few node (the only node aside from /chosen) which is about software configurations rather than hardware description. For this reason, in a device tree with multiple domains /reserved-memory doesn't make a lot of sense: for which domain is the memory reserved? This was one of the first points raised by Rob Herring in reviewing system device tree. So the solution we went for is the following: if there is a default domain /reserved-memory applies to the default domain. Otherwise, each domain is going to have its own reserved-memory. Example: domU1 { compatible = "xen,domain"; #address-cells = <0x1>; #size-cells = <0x1>; cpus = <2>; reserved-memory { #address-cells = <2>; #size-cells = <2>; static-memory@0x30000000 { compatible = "xen,static-memory-domain"; reg = <0x0 0x30000000 0x0 0x20000000>; }; }; }; So I don't think we want to use reserved-memory for this, either /reserved-memory or /chosen/domU1/reserved-memory. Instead it would be good to align it with system device tree and define it as a new property under domU1. In system device tree we would use a property called "memory" to specify one or more ranges, e.g.: domU1 { memory = <0x0 0x500000 0x0 0x7fb00000>; Unfortunately for xen,domains we have already defined "memory" to specify an amount, rather than a range. That's too bad because the most natural way to do this would be: domU1 { size = <amount>; memory = <ranges>; When we'll introduce native system device tree support in Xen we'll be able to do that. For now, we need to come up with a different property. For instance: "static-memory" (other names are welcome if you have a better suggestion). We use a new property called "static-memory" together with #static-memory-address-cells and #static-memory-size-cells to define how many cells to use for address and size. Example: domU1 { #static-memory-address-cells = <2>; #static-memory-size-cells = <2>; static-memory = <0x0 0x500000 0x0 0x7fb00000>; Another alternative would be to extend the definition of the existing "memory" property to potentially handle not just sizes but also address and size pairs. There are a couple of ways to do that, including using #memory-address-cells = <0>; to specify when memory only has a size, or changing compatible string to "xen,domain-v2". But actually I would avoid it. I would keep it simple and just introduce a new property like "static-memory" (we can come up with a better name).
Hi, On Thu, 3 Jun 2021 at 22:33, Stefano Stabellini <sstabellini@kernel.org> wrote: > On Thu, 3 Jun 2021, Julien Grall wrote: > > On 02/06/2021 11:09, Penny Zheng wrote: > > > I could not think a way to fix it properly in codes, do you have any > > > suggestion? Or we just put a warning in doc/commits. > > > > The correct approach is to find the parent of staticmemdomU1 (i.e. > > reserved-memory) and use the #address-cells and #size-cells from there. > > Julien is right about how to parse the static-memory. > > But I have a suggestion on the new binding. The /reserved-memory node is > a weird node: it is one of the very few node (the only node aside from > /chosen) which is about software configurations rather than hardware > description. > > For this reason, in a device tree with multiple domains /reserved-memory > doesn't make a lot of sense: for which domain is the memory reserved? IHMO, /reserved-memory refers to the memory that the hypervisor should not touch. It is just a coincidence that most of the domains are then passed through to dom0. This also matches the fact that the GIC, /memory is consumed by the hypervisor directly and not the domain.. > > This was one of the first points raised by Rob Herring in reviewing > system device tree. > > So the solution we went for is the following: if there is a default > domain /reserved-memory applies to the default domain. Otherwise, each > domain is going to have its own reserved-memory. Example: > > domU1 { > compatible = "xen,domain"; > #address-cells = <0x1>; > #size-cells = <0x1>; > cpus = <2>; > > reserved-memory { > #address-cells = <2>; > #size-cells = <2>; > > static-memory@0x30000000 { > compatible = "xen,static-memory-domain"; > reg = <0x0 0x30000000 0x0 0x20000000>; > }; > }; > }; This sounds wrong to me because the memory is reserved from the hypervisor PoV not from the domain. IOW, when I read this, I think the memory will be reserved in the domain. > > So I don't think we want to use reserved-memory for this, either > /reserved-memory or /chosen/domU1/reserved-memory. Instead it would be > good to align it with system device tree and define it as a new property > under domU1. Do you have any formal documentation of the system device-tree? > > In system device tree we would use a property called "memory" to specify > one or more ranges, e.g.: > > domU1 { > memory = <0x0 0x500000 0x0 0x7fb00000>; > > Unfortunately for xen,domains we have already defined "memory" to > specify an amount, rather than a range. That's too bad because the most > natural way to do this would be: > > domU1 { > size = <amount>; > memory = <ranges>; > > When we'll introduce native system device tree support in Xen we'll be > able to do that. For now, we need to come up with a different property. > For instance: "static-memory" (other names are welcome if you have a > better suggestion). > > We use a new property called "static-memory" together with > #static-memory-address-cells and #static-memory-size-cells to define how > many cells to use for address and size. > > Example: > > domU1 { > #static-memory-address-cells = <2>; > #static-memory-size-cells = <2>; > static-memory = <0x0 0x500000 0x0 0x7fb00000>; This is pretty similar to what Penny suggested. But I dislike it because of the amount of code that needs to be duplicated with the reserved memory. Cheers,
On Thu, 3 Jun 2021, Julien Grall wrote: > On Thu, 3 Jun 2021 at 22:33, Stefano Stabellini <sstabellini@kernel.org> wrote: > > On Thu, 3 Jun 2021, Julien Grall wrote: > > > On 02/06/2021 11:09, Penny Zheng wrote: > > > > I could not think a way to fix it properly in codes, do you have any > > > > suggestion? Or we just put a warning in doc/commits. > > > > > > The correct approach is to find the parent of staticmemdomU1 (i.e. > > > reserved-memory) and use the #address-cells and #size-cells from there. > > > > Julien is right about how to parse the static-memory. > > > > But I have a suggestion on the new binding. The /reserved-memory node is > > a weird node: it is one of the very few node (the only node aside from > > /chosen) which is about software configurations rather than hardware > > description. > > > > For this reason, in a device tree with multiple domains /reserved-memory > > doesn't make a lot of sense: for which domain is the memory reserved? > > IHMO, /reserved-memory refers to the memory that the hypervisor should > not touch. It is just a coincidence that most of the domains are then > passed through to dom0. > > This also matches the fact that the GIC, /memory is consumed by the > hypervisor directly and not the domain.. In system device tree one of the key principles is to distinguish between hardware description and domains configuration. The domains configuration is under /domains (originally it was under /chosen then the DT maintainers requested to move it to its own top-level node), while everything else is for hardware description. /chosen and /reserved-memory are exceptions. They are top-level nodes but they are for software configurations. In system device tree configurations go under the domain node. This makes sense: Xen, dom0 and domU can all have different reserved-memory and chosen configurations. /domains/domU1/reserved-memory gives us a clear way to express reserved-memory configurations for domU1. Which leaves us with /reserved-memory. Who is that for? It is for the default domain. The default domain is the one receiving all devices by default. In a Xen setting, it is probably Dom0. In this case, we don't want to add reserved-memory regions for DomUs to Dom0's list. Dom0's reserved-memory list is for its own drivers. We could also make an argument that the default domain is Xen itself. From a spec perspective, that would be fine too. In this case, /reserved-memory is a list of memory regions reserved for Xen drivers. Either way, I don't think is a great fit for domains memory allocations. > > This was one of the first points raised by Rob Herring in reviewing > > system device tree. > > > > So the solution we went for is the following: if there is a default > > domain /reserved-memory applies to the default domain. Otherwise, each > > domain is going to have its own reserved-memory. Example: > > > > domU1 { > > compatible = "xen,domain"; > > #address-cells = <0x1>; > > #size-cells = <0x1>; > > cpus = <2>; > > > > reserved-memory { > > #address-cells = <2>; > > #size-cells = <2>; > > > > static-memory@0x30000000 { > > compatible = "xen,static-memory-domain"; > > reg = <0x0 0x30000000 0x0 0x20000000>; > > }; > > }; > > }; > > This sounds wrong to me because the memory is reserved from the > hypervisor PoV not from the domain. IOW, when I read this, I think the > memory will be reserved in the domain. It is definitely very wrong to place the static-memory allocation under /chosen/domU1/reserved-memory. Sorry if I caused confusion. I only meant it as an example of how reserved-memory (actual reserved-memory list driver-specific memory ranges) is used. > > > > So I don't think we want to use reserved-memory for this, either > > /reserved-memory or /chosen/domU1/reserved-memory. Instead it would be > > good to align it with system device tree and define it as a new property > > under domU1. > > Do you have any formal documentation of the system device-tree? It lives here: https://github.com/devicetree-org/lopper/tree/master/specification Start from specification.md. It is the oldest part of the spec, so it is not yet written with a formal specification language. FYI there are a number of things in-flight in regards to domains that we discussed in the last call but they are not yet settled, thus, they are not yet committed (access flags definitions and hierarchical domains). However, they don't affect domains memory allocations so from that perspective nothing has changed. > > In system device tree we would use a property called "memory" to specify > > one or more ranges, e.g.: > > > > domU1 { > > memory = <0x0 0x500000 0x0 0x7fb00000>; > > > > Unfortunately for xen,domains we have already defined "memory" to > > specify an amount, rather than a range. That's too bad because the most > > natural way to do this would be: > > > > domU1 { > > size = <amount>; > > memory = <ranges>; > > > > When we'll introduce native system device tree support in Xen we'll be > > able to do that. For now, we need to come up with a different property. > > For instance: "static-memory" (other names are welcome if you have a > > better suggestion). > > > > We use a new property called "static-memory" together with > > #static-memory-address-cells and #static-memory-size-cells to define how > > many cells to use for address and size. > > > > Example: > > > > domU1 { > > #static-memory-address-cells = <2>; > > #static-memory-size-cells = <2>; > > static-memory = <0x0 0x500000 0x0 0x7fb00000>; > > This is pretty similar to what Penny suggested. But I dislike it > because of the amount of code that needs to be duplicated with the > reserved memory. Where is the code duplication? In the parsing itself? If there is code duplication, can we find a way to share some of the code to avoid the duplication?
Hi stefano and julien > -----Original Message----- > From: Stefano Stabellini <sstabellini@kernel.org> > Sent: Friday, June 4, 2021 7:56 AM > To: Julien Grall <julien.grall.oss@gmail.com> > Cc: Stefano Stabellini <sstabellini@kernel.org>; Penny Zheng > <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org; 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 > > On Thu, 3 Jun 2021, Julien Grall wrote: > > On Thu, 3 Jun 2021 at 22:33, Stefano Stabellini <sstabellini@kernel.org> > wrote: > > > On Thu, 3 Jun 2021, Julien Grall wrote: > > > > On 02/06/2021 11:09, Penny Zheng wrote: > > > > > I could not think a way to fix it properly in codes, do you have > > > > > any suggestion? Or we just put a warning in doc/commits. > > > > > > > > The correct approach is to find the parent of staticmemdomU1 (i.e. > > > > reserved-memory) and use the #address-cells and #size-cells from there. > > > > > > Julien is right about how to parse the static-memory. > > > > > > But I have a suggestion on the new binding. The /reserved-memory > > > node is a weird node: it is one of the very few node (the only node > > > aside from > > > /chosen) which is about software configurations rather than hardware > > > description. > > > > > > For this reason, in a device tree with multiple domains > > > /reserved-memory doesn't make a lot of sense: for which domain is the > memory reserved? > > > > IHMO, /reserved-memory refers to the memory that the hypervisor should > > not touch. It is just a coincidence that most of the domains are then > > passed through to dom0. > > > > This also matches the fact that the GIC, /memory is consumed by the > > hypervisor directly and not the domain.. > > In system device tree one of the key principles is to distinguish between > hardware description and domains configuration. The domains configuration > is under /domains (originally it was under /chosen then the DT maintainers > requested to move it to its own top-level node), while everything else is for > hardware description. > > /chosen and /reserved-memory are exceptions. They are top-level nodes but > they are for software configurations. In system device tree configurations go > under the domain node. This makes sense: Xen, dom0 and domU can all have > different reserved-memory and chosen configurations. > > /domains/domU1/reserved-memory gives us a clear way to express reserved- > memory configurations for domU1. > > Which leaves us with /reserved-memory. Who is that for? It is for the default > domain. > > The default domain is the one receiving all devices by default. In a Xen setting, > it is probably Dom0. In this case, we don't want to add reserved-memory > regions for DomUs to Dom0's list. Dom0's reserved-memory list is for its own > drivers. We could also make an argument that the default domain is Xen itself. > From a spec perspective, that would be fine too. In this case, /reserved- > memory is a list of memory regions reserved for Xen drivers. Either way, I don't > think is a great fit for domains memory allocations. > > > > > This was one of the first points raised by Rob Herring in reviewing > > > system device tree. > > > > > > So the solution we went for is the following: if there is a default > > > domain /reserved-memory applies to the default domain. Otherwise, > > > each domain is going to have its own reserved-memory. Example: > > > > > > domU1 { > > > compatible = "xen,domain"; > > > #address-cells = <0x1>; > > > #size-cells = <0x1>; > > > cpus = <2>; > > > > > > reserved-memory { > > > #address-cells = <2>; > > > #size-cells = <2>; > > > > > > static-memory@0x30000000 { > > > compatible = "xen,static-memory-domain"; > > > reg = <0x0 0x30000000 0x0 0x20000000>; > > > }; > > > }; > > > }; > > > > This sounds wrong to me because the memory is reserved from the > > hypervisor PoV not from the domain. IOW, when I read this, I think the > > memory will be reserved in the domain. > > It is definitely very wrong to place the static-memory allocation under > /chosen/domU1/reserved-memory. Sorry if I caused confusion. I only meant it > as an example of how reserved-memory (actual reserved-memory list driver- > specific memory ranges) is used. > > > > > > > > So I don't think we want to use reserved-memory for this, either > > > /reserved-memory or /chosen/domU1/reserved-memory. Instead it would > > > be good to align it with system device tree and define it as a new > > > property under domU1. > > > > Do you have any formal documentation of the system device-tree? > > It lives here: > https://github.com/devicetree-org/lopper/tree/master/specification > > Start from specification.md. It is the oldest part of the spec, so it is not yet > written with a formal specification language. > > FYI there are a number of things in-flight in regards to domains that we > discussed in the last call but they are not yet settled, thus, they are not yet > committed (access flags definitions and hierarchical domains). However, they > don't affect domains memory allocations so from that perspective nothing has > changed. > > > > > In system device tree we would use a property called "memory" to > > > specify one or more ranges, e.g.: > > > > > > domU1 { > > > memory = <0x0 0x500000 0x0 0x7fb00000>; > > > > > > Unfortunately for xen,domains we have already defined "memory" to > > > specify an amount, rather than a range. That's too bad because the > > > most natural way to do this would be: > > > > > > domU1 { > > > size = <amount>; > > > memory = <ranges>; > > > > > > When we'll introduce native system device tree support in Xen we'll > > > be able to do that. For now, we need to come up with a different property. > > > For instance: "static-memory" (other names are welcome if you have a > > > better suggestion). > > > > > > We use a new property called "static-memory" together with > > > #static-memory-address-cells and #static-memory-size-cells to define > > > how many cells to use for address and size. > > > > > > Example: > > > > > > domU1 { > > > #static-memory-address-cells = <2>; > > > #static-memory-size-cells = <2>; > > > static-memory = <0x0 0x500000 0x0 0x7fb00000>; > > > > This is pretty similar to what Penny suggested. But I dislike it > > because of the amount of code that needs to be duplicated with the > > reserved memory. > > Where is the code duplication? In the parsing itself? > > If there is code duplication, can we find a way to share some of the code to > avoid the duplication? Both your opinions are so convincing... :/ Correct me if I am wrong: I think the duplication which Julien means are here, See commit https://patchew.org/Xen/20210518052113.725808-1-penny.zheng@arm.com/20210518052113.725808-3-penny.zheng@arm.com/ I added a another similar loop in dt_unreserved_regions to unreserve static memory. For this part, I could try to extract common codes. But another part I think is just this commit, where I added another check for static memory in early_scan_node: + 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); TBH, I don't know how to fix here.... I've already finished Patch v2, we could continue discussing on how to define it in Device tree here, and it will be included in Patch v3~~~
On Fri, 4 Jun 2021, Penny Zheng wrote: > > > > In system device tree we would use a property called "memory" to > > > > specify one or more ranges, e.g.: > > > > > > > > domU1 { > > > > memory = <0x0 0x500000 0x0 0x7fb00000>; > > > > > > > > Unfortunately for xen,domains we have already defined "memory" to > > > > specify an amount, rather than a range. That's too bad because the > > > > most natural way to do this would be: > > > > > > > > domU1 { > > > > size = <amount>; > > > > memory = <ranges>; > > > > > > > > When we'll introduce native system device tree support in Xen we'll > > > > be able to do that. For now, we need to come up with a different property. > > > > For instance: "static-memory" (other names are welcome if you have a > > > > better suggestion). > > > > > > > > We use a new property called "static-memory" together with > > > > #static-memory-address-cells and #static-memory-size-cells to define > > > > how many cells to use for address and size. > > > > > > > > Example: > > > > > > > > domU1 { > > > > #static-memory-address-cells = <2>; > > > > #static-memory-size-cells = <2>; > > > > static-memory = <0x0 0x500000 0x0 0x7fb00000>; > > > > > > This is pretty similar to what Penny suggested. But I dislike it > > > because of the amount of code that needs to be duplicated with the > > > reserved memory. > > > > Where is the code duplication? In the parsing itself? > > > > If there is code duplication, can we find a way to share some of the code to > > avoid the duplication? > > Both your opinions are so convincing... :/ > > Correct me if I am wrong: > I think the duplication which Julien means are here, See commit > https://patchew.org/Xen/20210518052113.725808-1-penny.zheng@arm.com/20210518052113.725808-3-penny.zheng@arm.com/ > I added a another similar loop in dt_unreserved_regions to unreserve static memory. > For this part, I could try to extract common codes. > > But another part I think is just this commit, where I added another check for static memory > in early_scan_node: > > + 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); > > TBH, I don't know how to fix here.... Is it only one loop in dt_unreserved_regions and another call to process_static_memory? If you can make the code common in dt_unreserved_regions there wouldn't be much duplication left. To explain my point of view a bit better, I think we have a lot more freedom in the Xen implementation compared to the device tree specification. For the sake of an example, let's say that we wanted Xen to reuse bootinfo.reserved_mem for both reserved-memory and static-memory. I don't know if it is even a good idea (I haven't looked into it, it is just an example) but I think it would be OK, we could decide to do that. We have less room for flexibility in the device tree specification. /reserved-memory is for special configurations of 1 domain only. I don't think we could add domain static memory allocations to it.
Hi Stefano, On 04/06/2021 00:55, Stefano Stabellini wrote: > On Thu, 3 Jun 2021, Julien Grall wrote: >> On Thu, 3 Jun 2021 at 22:33, Stefano Stabellini <sstabellini@kernel.org> wrote: >>> On Thu, 3 Jun 2021, Julien Grall wrote: >>>> On 02/06/2021 11:09, Penny Zheng wrote: >>>>> I could not think a way to fix it properly in codes, do you have any >>>>> suggestion? Or we just put a warning in doc/commits. >>>> >>>> The correct approach is to find the parent of staticmemdomU1 (i.e. >>>> reserved-memory) and use the #address-cells and #size-cells from there. >>> >>> Julien is right about how to parse the static-memory. >>> >>> But I have a suggestion on the new binding. The /reserved-memory node is >>> a weird node: it is one of the very few node (the only node aside from >>> /chosen) which is about software configurations rather than hardware >>> description. >>> >>> For this reason, in a device tree with multiple domains /reserved-memory >>> doesn't make a lot of sense: for which domain is the memory reserved? >> >> IHMO, /reserved-memory refers to the memory that the hypervisor should >> not touch. It is just a coincidence that most of the domains are then >> passed through to dom0. >> >> This also matches the fact that the GIC, /memory is consumed by the >> hypervisor directly and not the domain.. > > In system device tree one of the key principles is to distinguish between > hardware description and domains configuration. The domains > configuration is under /domains (originally it was under /chosen then > the DT maintainers requested to move it to its own top-level node), while > everything else is for hardware description. > > /chosen and /reserved-memory are exceptions. They are top-level nodes > but they are for software configurations. In system device tree > configurations go under the domain node. This makes sense: Xen, dom0 and > domU can all have different reserved-memory and chosen configurations. > > /domains/domU1/reserved-memory gives us a clear way to express > reserved-memory configurations for domU1. > > Which leaves us with /reserved-memory. Who is that for? It is for the > default domain. > > The default domain is the one receiving all devices by default. In a Xen > setting, it is probably Dom0. Let's take an example, let say in the future someone wants to allocate a specific region for the memory used by the GICv3 ITS. From what you said above, /reserved-memory would be used by dom0. So how would you be able to tell the hypervisor that the region is reserved for itself? > In this case, we don't want to add > reserved-memory regions for DomUs to Dom0's list. Dom0's reserved-memory > list is for its own drivers. We could also make an argument that the > default domain is Xen itself. From a spec perspective, that would be > fine too. In this case, /reserved-memory is a list of memory regions > reserved for Xen drivers. We seem to have a different way to read the binding description in [1]. For convenience, I will copy it here: "Reserved memory is specified as a node under the /reserved-memory node. The operating system shall exclude reserved memory from normal usage one can create child nodes describing particular reserved (excluded from normal use) memory regions. Such memory regions are usually designed for the special usage by various device drivers. " I read it as this can be used to exclude any memory from the allocator for a specific purpose. They give the example of device drivers, but they don't exclude other purpose. So... > Either way, I don't think is a great fit for > domains memory allocations. ... I don't really understand why this is not a great fit. The regions have been *reserved* for a purpose. >>> >>> So I don't think we want to use reserved-memory for this, either >>> /reserved-memory or /chosen/domU1/reserved-memory. Instead it would be >>> good to align it with system device tree and define it as a new property >>> under domU1. >> >> Do you have any formal documentation of the system device-tree? > > It lives here: > https://github.com/devicetree-org/lopper/tree/master/specification > > Start from specification.md. It is the oldest part of the spec, so it is > not yet written with a formal specification language. > > FYI there are a number of things in-flight in regards to domains that > we discussed in the last call but they are not yet settled, thus, they > are not yet committed (access flags definitions and hierarchical > domains). However, they don't affect domains memory allocations so from > that perspective nothing has changed. Thanks! > > >>> In system device tree we would use a property called "memory" to specify >>> one or more ranges, e.g.: >>> >>> domU1 { >>> memory = <0x0 0x500000 0x0 0x7fb00000>; >>> >>> Unfortunately for xen,domains we have already defined "memory" to >>> specify an amount, rather than a range. That's too bad because the most >>> natural way to do this would be: >>> >>> domU1 { >>> size = <amount>; >>> memory = <ranges>; >>> >>> When we'll introduce native system device tree support in Xen we'll be >>> able to do that. For now, we need to come up with a different property. >>> For instance: "static-memory" (other names are welcome if you have a >>> better suggestion). >>> >>> We use a new property called "static-memory" together with >>> #static-memory-address-cells and #static-memory-size-cells to define how >>> many cells to use for address and size. >>> >>> Example: >>> >>> domU1 { >>> #static-memory-address-cells = <2>; >>> #static-memory-size-cells = <2>; >>> static-memory = <0x0 0x500000 0x0 0x7fb00000>; >> >> This is pretty similar to what Penny suggested. But I dislike it >> because of the amount of code that needs to be duplicated with the >> reserved memory. > > Where is the code duplication? In the parsing itself? So the problem is we need an entire new way to parse and walk yet another binding that will describe memory excluded from normal allocator hypervisor. The code is pretty much the same as parsing /reserved-memory except it will be on a different address cells, size cells, property. > > If there is code duplication, can we find a way to share some of the > code to avoid the duplication? Feel free to propose one. I suggested to use /reserved-memory because this is the approach that makes the most sense to me (see my reply above). TBH, even after your explanation, I am still a bit puzzled into why /reserved-memory cannot be leveraged to exclude domain region from the hypervisor allocator. Cheers, [1] https://www.kernel.org/doc/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
Hi All,
On 7 Jun 2021, at 19:09, Julien Grall <julien@xen.org<mailto:julien@xen.org>> wrote:
Hi Stefano,
On 04/06/2021 00:55, Stefano Stabellini wrote:
On Thu, 3 Jun 2021, Julien Grall wrote:
On Thu, 3 Jun 2021 at 22:33, Stefano Stabellini <sstabellini@kernel.org<mailto:sstabellini@kernel.org>> wrote:
On Thu, 3 Jun 2021, Julien Grall wrote:
On 02/06/2021 11:09, Penny Zheng wrote:
I could not think a way to fix it properly in codes, do you have any
suggestion? Or we just put a warning in doc/commits.
The correct approach is to find the parent of staticmemdomU1 (i.e.
reserved-memory) and use the #address-cells and #size-cells from there.
Julien is right about how to parse the static-memory.
But I have a suggestion on the new binding. The /reserved-memory node is
a weird node: it is one of the very few node (the only node aside from
/chosen) which is about software configurations rather than hardware
description.
For this reason, in a device tree with multiple domains /reserved-memory
doesn't make a lot of sense: for which domain is the memory reserved?
IHMO, /reserved-memory refers to the memory that the hypervisor should
not touch. It is just a coincidence that most of the domains are then
passed through to dom0.
This also matches the fact that the GIC, /memory is consumed by the
hypervisor directly and not the domain..
In system device tree one of the key principles is to distinguish between
hardware description and domains configuration. The domains
configuration is under /domains (originally it was under /chosen then
the DT maintainers requested to move it to its own top-level node), while
everything else is for hardware description.
/chosen and /reserved-memory are exceptions. They are top-level nodes
but they are for software configurations. In system device tree
configurations go under the domain node. This makes sense: Xen, dom0 and
domU can all have different reserved-memory and chosen configurations.
/domains/domU1/reserved-memory gives us a clear way to express
reserved-memory configurations for domU1.
Which leaves us with /reserved-memory. Who is that for? It is for the
default domain.
The default domain is the one receiving all devices by default. In a Xen
setting, it is probably Dom0.
Let's take an example, let say in the future someone wants to allocate a specific region for the memory used by the GICv3 ITS.
From what you said above, /reserved-memory would be used by dom0. So how would you be able to tell the hypervisor that the region is reserved for itself?
In this case, we don't want to add
reserved-memory regions for DomUs to Dom0's list. Dom0's reserved-memory
list is for its own drivers. We could also make an argument that the
default domain is Xen itself. From a spec perspective, that would be
fine too. In this case, /reserved-memory is a list of memory regions
reserved for Xen drivers.
We seem to have a different way to read the binding description in [1]. For convenience, I will copy it here:
"Reserved memory is specified as a node under the /reserved-memory node.
The operating system shall exclude reserved memory from normal usage
one can create child nodes describing particular reserved (excluded from
normal use) memory regions. Such memory regions are usually designed for
the special usage by various device drivers.
"
I read it as this can be used to exclude any memory from the allocator for a specific purpose. They give the example of device drivers, but they don't exclude other purpose. So...
Either way, I don't think is a great fit for
domains memory allocations.
... I don't really understand why this is not a great fit. The regions have been *reserved* for a purpose.
So I don't think we want to use reserved-memory for this, either
/reserved-memory or /chosen/domU1/reserved-memory. Instead it would be
good to align it with system device tree and define it as a new property
under domU1.
Do you have any formal documentation of the system device-tree?
It lives here:
https://github.com/devicetree-org/lopper/tree/master/specification
Start from specification.md. It is the oldest part of the spec, so it is
not yet written with a formal specification language.
FYI there are a number of things in-flight in regards to domains that
we discussed in the last call but they are not yet settled, thus, they
are not yet committed (access flags definitions and hierarchical
domains). However, they don't affect domains memory allocations so from
that perspective nothing has changed.
Thanks!
In system device tree we would use a property called "memory" to specify
one or more ranges, e.g.:
domU1 {
memory = <0x0 0x500000 0x0 0x7fb00000>;
Unfortunately for xen,domains we have already defined "memory" to
specify an amount, rather than a range. That's too bad because the most
natural way to do this would be:
domU1 {
size = <amount>;
memory = <ranges>;
When we'll introduce native system device tree support in Xen we'll be
able to do that. For now, we need to come up with a different property.
For instance: "static-memory" (other names are welcome if you have a
better suggestion).
We use a new property called "static-memory" together with
#static-memory-address-cells and #static-memory-size-cells to define how
many cells to use for address and size.
Example:
domU1 {
#static-memory-address-cells = <2>;
#static-memory-size-cells = <2>;
static-memory = <0x0 0x500000 0x0 0x7fb00000>;
This is pretty similar to what Penny suggested. But I dislike it
because of the amount of code that needs to be duplicated with the
reserved memory.
Where is the code duplication? In the parsing itself?
So the problem is we need an entire new way to parse and walk yet another binding that will describe memory excluded from normal allocator hypervisor.
The code is pretty much the same as parsing /reserved-memory except it will be on a different address cells, size cells, property.
If there is code duplication, can we find a way to share some of the
code to avoid the duplication?
Feel free to propose one. I suggested to use /reserved-memory because this is the approach that makes the most sense to me (see my reply above).
TBH, even after your explanation, I am still a bit puzzled into why /reserved-memory cannot be leveraged to exclude domain region from the hypervisor allocator.
I really tend to think that the original solution from Penny is for now the easiest and simplest to document.
In the long term, using directly memory and giving in it the address range directly is the most natural solution but that would clash with the current usage for it.
I would like to suggest the following approach:
- keep original solution from Penny
- start to discuss a domain v2 so that we could solve current issues we have including the passthrough devices which are not really easy to define.
As a user I would just expect to put a device tree or links in a domain definition to define its characteristic and devices and using the standard names (memory for example).
Also I must admit I need to read more the system device tree spec to check if we could just use it directly (and be compliant to a standard).
Would that approach be acceptable ?
I am more then happy to drive a working group on rethinking the device tree together with Penny.
Cheers
Bertrand
Cheers,
[1] https://www.kernel.org/doc/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
--
Julien Grall
On 09/06/2021 10:56, Bertrand Marquis wrote: > Hi All, Hi, >> On 7 Jun 2021, at 19:09, Julien Grall <julien@xen.org >> <mailto:julien@xen.org>> wrote: >> Feel free to propose one. I suggested to use /reserved-memory because >> this is the approach that makes the most sense to me (see my reply above). >> >> TBH, even after your explanation, I am still a bit puzzled into why >> /reserved-memory cannot be leveraged to exclude domain region from the >> hypervisor allocator. > > I really tend to think that the original solution from Penny is for now > the easiest and simplest to document. I can live with Penny's solution so long we don't duplicate the parsing and we don't create new datastructure in Xen for the new type of reserved memory. However... > > In the long term, using directly memory and giving in it the address > range directly is the most natural solution but that would clash with > the current usage for it. ... we are already going to have quite some churn to support the system device-tree. So I don't want yet another binding to be invented in a few months time. IOW, the new binding should be a long term solution rather than a temporary one to fill the gap until we agree on what you call "domain v2". Cheers,
Hi julien > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: Wednesday, June 9, 2021 6:47 PM > To: Bertrand Marquis <Bertrand.Marquis@arm.com> > Cc: Stefano Stabellini <sstabellini@kernel.org>; Julien Grall > <julien.grall.oss@gmail.com>; Penny Zheng <Penny.Zheng@arm.com>; xen- > devel@lists.xenproject.org; Wei Chen <Wei.Chen@arm.com>; nd > <nd@arm.com> > Subject: Re: [PATCH 01/10] xen/arm: introduce domain on Static Allocation > > > > On 09/06/2021 10:56, Bertrand Marquis wrote: > > Hi All, > > Hi, > > >> On 7 Jun 2021, at 19:09, Julien Grall <julien@xen.org > >> <mailto:julien@xen.org>> wrote: > >> Feel free to propose one. I suggested to use /reserved-memory because > >> this is the approach that makes the most sense to me (see my reply above). > >> > >> TBH, even after your explanation, I am still a bit puzzled into why > >> /reserved-memory cannot be leveraged to exclude domain region from > >> the hypervisor allocator. > > > > I really tend to think that the original solution from Penny is for > > now the easiest and simplest to document. > > I can live with Penny's solution so long we don't duplicate the parsing and we > don't create new datastructure in Xen for the new type of reserved memory. > However... > Just to confirm what I understand here, you are not only worrying about the duplication code imported in dt_unreserved_regions, but also must introducing another path in func early_scan_node to parse my first implementation "xen,static-mem = <...>", right? On duplication code part, I could think of a way to extract common codes to fix, but for introducing another new path to parse, FWIT, it is inevitable if not re-using reserved-memory. ;/ > > In the long term, using directly memory and giving in it the address > > range directly is the most natural solution but that would clash with > > the current usage for it. > > ... we are already going to have quite some churn to support the system > device-tree. So I don't want yet another binding to be invented in a few > months time. > > IOW, the new binding should be a long term solution rather than a temporary > one to fill the gap until we agree on what you call "domain v2". > > Cheers, > > -- Cheers Penny > Julien Grall
On 15/06/2021 08:08, Penny Zheng wrote: > Hi julien Hi Penny, >> -----Original Message----- >> From: Julien Grall <julien@xen.org> >> Sent: Wednesday, June 9, 2021 6:47 PM >> To: Bertrand Marquis <Bertrand.Marquis@arm.com> >> Cc: Stefano Stabellini <sstabellini@kernel.org>; Julien Grall >> <julien.grall.oss@gmail.com>; Penny Zheng <Penny.Zheng@arm.com>; xen- >> devel@lists.xenproject.org; Wei Chen <Wei.Chen@arm.com>; nd >> <nd@arm.com> >> Subject: Re: [PATCH 01/10] xen/arm: introduce domain on Static Allocation >> >> >> >> On 09/06/2021 10:56, Bertrand Marquis wrote: >>> Hi All, >> >> Hi, >> >>>> On 7 Jun 2021, at 19:09, Julien Grall <julien@xen.org >>>> <mailto:julien@xen.org>> wrote: >>>> Feel free to propose one. I suggested to use /reserved-memory because >>>> this is the approach that makes the most sense to me (see my reply above). >>>> >>>> TBH, even after your explanation, I am still a bit puzzled into why >>>> /reserved-memory cannot be leveraged to exclude domain region from >>>> the hypervisor allocator. >>> >>> I really tend to think that the original solution from Penny is for >>> now the easiest and simplest to document. >> >> I can live with Penny's solution so long we don't duplicate the parsing and we >> don't create new datastructure in Xen for the new type of reserved memory. >> However... >> > > Just to confirm what I understand here, you are not only worrying about the duplication code imported in > dt_unreserved_regions, but also must introducing another path in func early_scan_node to parse my first implementation > "xen,static-mem = <...>", right? That's correct. > > On duplication code part, I could think of a way to extract common codes to fix, but for introducing another new path to parse, > FWIT, it is inevitable if not re-using reserved-memory. ;/ I don't think this is inevitable. If you look at the code, we already share the parsing between reserved-memory and memory. AFAICT, the main difference now is the property to parse. Other than that, the content is exactly the same. So we could pass the name of the property to parse. Cheers,
diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt index 5243bc7fd3..d209149d71 100644 --- a/docs/misc/arm/device-tree/booting.txt +++ b/docs/misc/arm/device-tree/booting.txt @@ -268,3 +268,36 @@ The DTB fragment is loaded at 0xc000000 in the example above. It should follow the convention explained in docs/misc/arm/passthrough.txt. The DTB fragment will be added to the guest device tree, so that the guest kernel will be able to discover the device. + + +Static Allocation +============= + +Static Allocation refers to system or sub-system(domains) for which memory +areas are pre-defined by configuration using physical address ranges. +Those pre-defined memory, -- Static Momery, as parts of RAM reserved in the +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. +By default, they shall be mapped to the fixed guest RAM address +`GUEST_RAM0_BASE`, `GUEST_RAM1_BASE`. + +Static Allocation is only supported on AArch64 for now. + +The dtb property should look like as follows: + + chosen { + domU1 { + compatible = "xen,domain"; + #address-cells = <0x2>; + #size-cells = <0x2>; + cpus = <2>; + xen,static-mem = <0x0 0x30000000 0x0 0x20000000>; + + ... + }; + }; + +DOMU1 on Static Allocation has reserved RAM bank at 0x30000000 of 512MB size +as guest RAM. diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index dcff512648..e9f14e6a44 100644 --- a/xen/arch/arm/bootfdt.c +++ b/xen/arch/arm/bootfdt.c @@ -327,6 +327,55 @@ static void __init process_chosen_node(const void *fdt, int node, add_boot_module(BOOTMOD_RAMDISK, start, end-start, false); } +static int __init process_static_memory(const void *fdt, int node, + const char *name, + u32 address_cells, u32 size_cells, + void *data) +{ + int i; + int banks; + const __be32 *cell; + paddr_t start, size; + u32 reg_cells = address_cells + size_cells; + struct meminfo *mem = data; + const struct fdt_property *prop; + + if ( address_cells < 1 || size_cells < 1 ) + { + printk("fdt: invalid #address-cells or #size-cells for static memory"); + return -EINVAL; + } + + /* + * Check if static memory property belongs to a specific domain, that is, + * its node `domUx` has compatible string "xen,domain". + */ + if ( fdt_node_check_compatible(fdt, node, "xen,domain") != 0 ) + printk("xen,static-mem property can only locate under /domUx node.\n"); + + prop = fdt_get_property(fdt, node, name, NULL); + if ( !prop ) + return -ENOENT; + + cell = (const __be32 *)prop->data; + banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32)); + + for ( i = 0; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ ) + { + device_tree_get_reg(&cell, address_cells, size_cells, &start, &size); + /* Some DT may describe empty bank, ignore them */ + if ( !size ) + continue; + mem->bank[mem->nr_banks].start = start; + mem->bank[mem->nr_banks].size = size; + mem->nr_banks++; + } + + if ( i < banks ) + return -ENOSPC; + return 0; +} + 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); 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;
Static Allocation refers to system or sub-system(domains) for which memory areas are pre-defined by configuration using physical address ranges. Those pre-defined memory, -- Static Momery, as parts of RAM reserved in the 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. By default, they shall be mapped to the fixed guest RAM address `GUEST_RAM0_BASE`, `GUEST_RAM1_BASE`. This patch introduces this new `xen,static-mem` property to define static memory nodes in device tree file. This patch also documents and parses this new attribute at boot time and stores related info in static_mem for later initialization. Signed-off-by: Penny Zheng <penny.zheng@arm.com> --- docs/misc/arm/device-tree/booting.txt | 33 +++++++++++++++++ xen/arch/arm/bootfdt.c | 52 +++++++++++++++++++++++++++ xen/include/asm-arm/setup.h | 2 ++ 3 files changed, 87 insertions(+)