From: Penny Zheng <penny.zheng@arm.com>
This commit introduces process_shm to cope with static shared memory in
domain construction.
DOMID_IO will be the default owner of memory pre-shared among multiple domains
at boot time, when no explicit owner is specified.
This commit only considers allocating static shared memory to dom_io
when owner domain is not explicitly defined in device tree, all the left,
including the "borrower" code path, the "explicit owner" code path, shall
be introduced later in the following patches.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
v5 change:
- refine in-code comment
---
v4 change:
- no changes
---
v3 change:
- refine in-code comment
---
v2 change:
- instead of introducing a new system domain, reuse the existing dom_io
- make dom_io a non-auto-translated domain, then no need to create P2M
for it
- change dom_io definition and make it wider to support static shm here too
- introduce is_shm_allocated_to_domio to check whether static shm is
allocated yet, instead of using shm_mask bitmap
- add in-code comment
---
xen/arch/arm/domain_build.c | 132 +++++++++++++++++++++++++++++++++++-
xen/common/domain.c | 3 +
2 files changed, 134 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 7ddd16c26d..91a5ace851 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -527,6 +527,10 @@ static bool __init append_static_memory_to_bank(struct domain *d,
return true;
}
+/*
+ * If cell is NULL, pbase and psize should hold valid values.
+ * Otherwise, cell will be populated together with pbase and psize.
+ */
static mfn_t __init acquire_static_memory_bank(struct domain *d,
const __be32 **cell,
u32 addr_cells, u32 size_cells,
@@ -535,7 +539,8 @@ static mfn_t __init acquire_static_memory_bank(struct domain *d,
mfn_t smfn;
int res;
- device_tree_get_reg(cell, addr_cells, size_cells, pbase, psize);
+ if ( cell )
+ device_tree_get_reg(cell, addr_cells, size_cells, pbase, psize);
ASSERT(IS_ALIGNED(*pbase, PAGE_SIZE) && IS_ALIGNED(*psize, PAGE_SIZE));
if ( PFN_DOWN(*psize) > UINT_MAX )
{
@@ -759,6 +764,125 @@ static void __init assign_static_memory_11(struct domain *d,
panic("Failed to assign requested static memory for direct-map domain %pd.",
d);
}
+
+#ifdef CONFIG_STATIC_SHM
+/*
+ * This function checks whether the static shared memory region is
+ * already allocated to dom_io.
+ */
+static bool __init is_shm_allocated_to_domio(paddr_t pbase)
+{
+ struct page_info *page;
+
+ page = maddr_to_page(pbase);
+ ASSERT(page);
+
+ if ( page_get_owner(page) == NULL )
+ return false;
+
+ ASSERT(page_get_owner(page) == dom_io);
+ return true;
+}
+
+static mfn_t __init acquire_shared_memory_bank(struct domain *d,
+ u32 addr_cells, u32 size_cells,
+ paddr_t *pbase, paddr_t *psize)
+{
+ /*
+ * Pages of statically shared memory shall be included
+ * in domain_tot_pages().
+ */
+ d->max_pages += PFN_DOWN(*psize);
+
+ return acquire_static_memory_bank(d, NULL, addr_cells, size_cells,
+ pbase, psize);
+
+}
+
+/*
+ * Func allocate_shared_memory is supposed to be only called
+ * from the owner.
+ */
+static int __init allocate_shared_memory(struct domain *d,
+ u32 addr_cells, u32 size_cells,
+ paddr_t pbase, paddr_t psize)
+{
+ mfn_t smfn;
+
+ dprintk(XENLOG_INFO,
+ "Allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
+ pbase, pbase + psize);
+
+ smfn = acquire_shared_memory_bank(d, addr_cells, size_cells, &pbase,
+ &psize);
+ if ( mfn_eq(smfn, INVALID_MFN) )
+ return -EINVAL;
+
+ /*
+ * DOMID_IO is the domain, like DOMID_XEN, that is not auto-translated.
+ * It sees RAM 1:1 and we do not need to create P2M mapping for it
+ */
+ ASSERT(d == dom_io);
+ return 0;
+}
+
+static int __init process_shm(struct domain *d,
+ const struct dt_device_node *node)
+{
+ struct dt_device_node *shm_node;
+ int ret = 0;
+ const struct dt_property *prop;
+ const __be32 *cells;
+ u32 shm_id;
+ u32 addr_cells, size_cells;
+ paddr_t gbase, pbase, psize;
+
+ dt_for_each_child_node(node, shm_node)
+ {
+ if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") )
+ continue;
+
+ if ( !dt_property_read_u32(shm_node, "xen,shm-id", &shm_id) )
+ {
+ printk("Shared memory node does not provide \"xen,shm-id\" property.\n");
+ return -ENOENT;
+ }
+
+ addr_cells = dt_n_addr_cells(shm_node);
+ size_cells = dt_n_size_cells(shm_node);
+ prop = dt_find_property(shm_node, "xen,shared-mem", NULL);
+ if ( !prop )
+ {
+ printk("Shared memory node does not provide \"xen,shared-mem\" property.\n");
+ return -ENOENT;
+ }
+ cells = (const __be32 *)prop->value;
+ /* xen,shared-mem = <pbase, psize, gbase>; */
+ device_tree_get_reg(&cells, addr_cells, size_cells, &pbase, &psize);
+ ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize, PAGE_SIZE));
+ gbase = dt_read_number(cells, addr_cells);
+
+ /* TODO: Consider owner domain is not the default dom_io. */
+ /*
+ * Per static shared memory region could be shared between multiple
+ * domains.
+ * In case re-allocating the same shared memory region, we check
+ * if it is already allocated to the default owner dom_io before
+ * the actual allocation.
+ */
+ if ( !is_shm_allocated_to_domio(pbase) )
+ {
+ /* Allocate statically shared pages to the default owner dom_io. */
+ ret = allocate_shared_memory(dom_io, addr_cells, size_cells,
+ pbase, psize);
+ if ( ret )
+ return ret;
+ }
+ }
+
+ return 0;
+}
+#endif /* CONFIG_STATIC_SHM */
#else
static void __init allocate_static_memory(struct domain *d,
struct kernel_info *kinfo,
@@ -3236,6 +3360,12 @@ static int __init construct_domU(struct domain *d,
else
assign_static_memory_11(d, &kinfo, node);
+#ifdef CONFIG_STATIC_SHM
+ rc = process_shm(d, node);
+ if ( rc < 0 )
+ return rc;
+#endif
+
/*
* Base address and irq number are needed when creating vpl011 device
* tree node in prepare_dtb_domU, so initialization on related variables
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 7570eae91a..7070f5a9b9 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -780,6 +780,9 @@ void __init setup_system_domains(void)
* This domain owns I/O pages that are within the range of the page_info
* array. Mappings occur at the priv of the caller.
* Quarantined PCI devices will be associated with this domain.
+ *
+ * DOMID_IO is also the default owner of memory pre-shared among multiple
+ * domains at boot time.
*/
dom_io = domain_create(DOMID_IO, NULL, 0);
if ( IS_ERR(dom_io) )
--
2.25.1Hi Penny,
On 20/06/2022 06:11, Penny Zheng wrote:
> From: Penny Zheng <penny.zheng@arm.com>
>
> This commit introduces process_shm to cope with static shared memory in
> domain construction.
>
> DOMID_IO will be the default owner of memory pre-shared among multiple domains
> at boot time, when no explicit owner is specified.
The document in patch #1 suggest the page will be shared with
dom_shared. But here you say "DOMID_IO".
Which one is correct?
>
> This commit only considers allocating static shared memory to dom_io
> when owner domain is not explicitly defined in device tree, all the left,
> including the "borrower" code path, the "explicit owner" code path, shall
> be introduced later in the following patches.
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> v5 change:
> - refine in-code comment
> ---
> v4 change:
> - no changes
> ---
> v3 change:
> - refine in-code comment
> ---
> v2 change:
> - instead of introducing a new system domain, reuse the existing dom_io
> - make dom_io a non-auto-translated domain, then no need to create P2M
> for it
> - change dom_io definition and make it wider to support static shm here too
> - introduce is_shm_allocated_to_domio to check whether static shm is
> allocated yet, instead of using shm_mask bitmap
> - add in-code comment
> ---
> xen/arch/arm/domain_build.c | 132 +++++++++++++++++++++++++++++++++++-
> xen/common/domain.c | 3 +
> 2 files changed, 134 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 7ddd16c26d..91a5ace851 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -527,6 +527,10 @@ static bool __init append_static_memory_to_bank(struct domain *d,
> return true;
> }
>
> +/*
> + * If cell is NULL, pbase and psize should hold valid values.
> + * Otherwise, cell will be populated together with pbase and psize.
> + */
> static mfn_t __init acquire_static_memory_bank(struct domain *d,
> const __be32 **cell,
> u32 addr_cells, u32 size_cells,
> @@ -535,7 +539,8 @@ static mfn_t __init acquire_static_memory_bank(struct domain *d,
> mfn_t smfn;
> int res;
>
> - device_tree_get_reg(cell, addr_cells, size_cells, pbase, psize);
> + if ( cell )
> + device_tree_get_reg(cell, addr_cells, size_cells, pbase, psize);
I think this is a bit of a hack. To me it sounds like this should be
moved out to a separate helper. This will also make the interface of
acquire_shared_memory_bank() less questionable (see below).
As this is v5, I would be OK with a follow-up for this split. But this
interface of acuiqre_shared_memory_bank() needs to change.
> ASSERT(IS_ALIGNED(*pbase, PAGE_SIZE) && IS_ALIGNED(*psize, PAGE_SIZE));
In the context of your series, who is checking that both psize and pbase
are suitably aligned?
> if ( PFN_DOWN(*psize) > UINT_MAX )
> {
> @@ -759,6 +764,125 @@ static void __init assign_static_memory_11(struct domain *d,
> panic("Failed to assign requested static memory for direct-map domain %pd.",
> d);
> }
> +
> +#ifdef CONFIG_STATIC_SHM
> +/*
> + * This function checks whether the static shared memory region is
> + * already allocated to dom_io.
> + */
> +static bool __init is_shm_allocated_to_domio(paddr_t pbase)
> +{
> + struct page_info *page;
> +
> + page = maddr_to_page(pbase);
> + ASSERT(page);
maddr_to_page() can never return NULL. If you want to check a page will
be valid, then you should use mfn_valid().
However, the ASSERT() implies that the address was suitably checked
before. But I can't find such check.
> +
> + if ( page_get_owner(page) == NULL )
> + return false;
> +
> + ASSERT(page_get_owner(page) == dom_io);
Could this be hit because of a wrong device-tree? If yes, then this
should not be an ASSERT() (they are not suitable to check user input).
> + return true;
> +}
> +
> +static mfn_t __init acquire_shared_memory_bank(struct domain *d,
> + u32 addr_cells, u32 size_cells,
> + paddr_t *pbase, paddr_t *psize)
There is something that doesn't add-up in this interface. The use of
pointer implies that pbase and psize may be modified by the function. So...
> +{
> + /*
> + * Pages of statically shared memory shall be included
> + * in domain_tot_pages().
> + */
> + d->max_pages += PFN_DOWN(*psize);
... it sounds a bit strange to use psize here. If psize, can't be
modified than it should probably not be a pointer.
Also, where do you check that d->max_pages will not overflow?
> +
> + return acquire_static_memory_bank(d, NULL, addr_cells, size_cells,
> + pbase, psize);
> +
> +}
> +
> +/*
> + * Func allocate_shared_memory is supposed to be only called
I am a bit concerned with the word "supposed". Are you implying that it
may be called by someone that is not the owner? If not, then it should
be "should".
Also NIT: Spell out completely "func". I.e "The function".
> + * from the owner.
I read from as "current should be the owner". But I guess this is not
what you mean here. Instead it looks like you mean "d" is the owner. So
I would write "d should be the owner of the shared area".
It would be good to have a check/ASSERT confirm this (assuming this is
easy to write).
> + */
> +static int __init allocate_shared_memory(struct domain *d,
> + u32 addr_cells, u32 size_cells,
> + paddr_t pbase, paddr_t psize)
> +{
> + mfn_t smfn;
> +
> + dprintk(XENLOG_INFO,
> + "Allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
> + pbase, pbase + psize);
NIT: I would suggest to also print the domain. This could help to easily
figure out that 'd' wasn't the owner.
> +
> + smfn = acquire_shared_memory_bank(d, addr_cells, size_cells, &pbase,
> + &psize);
> + if ( mfn_eq(smfn, INVALID_MFN) )
> + return -EINVAL;
> +
> + /*
> + * DOMID_IO is the domain, like DOMID_XEN, that is not auto-translated.
> + * It sees RAM 1:1 and we do not need to create P2M mapping for it
> + */
> + ASSERT(d == dom_io);
> + return 0;
> +}
> +
> +static int __init process_shm(struct domain *d,
> + const struct dt_device_node *node)
> +{
> + struct dt_device_node *shm_node;
> + int ret = 0;
> + const struct dt_property *prop;
> + const __be32 *cells;
> + u32 shm_id;
> + u32 addr_cells, size_cells;
> + paddr_t gbase, pbase, psize;
> +
> + dt_for_each_child_node(node, shm_node)
> + {
> + if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") )
> + continue;
> +
> + if ( !dt_property_read_u32(shm_node, "xen,shm-id", &shm_id) )
> + {
> + printk("Shared memory node does not provide \"xen,shm-id\" property.\n");
> + return -ENOENT;
> + }
> +
> + addr_cells = dt_n_addr_cells(shm_node);
> + size_cells = dt_n_size_cells(shm_node);
> + prop = dt_find_property(shm_node, "xen,shared-mem", NULL);
> + if ( !prop )
> + {
> + printk("Shared memory node does not provide \"xen,shared-mem\" property.\n");
> + return -ENOENT;
> + }
> + cells = (const __be32 *)prop->value;
> + /* xen,shared-mem = <pbase, psize, gbase>; */
> + device_tree_get_reg(&cells, addr_cells, size_cells, &pbase, &psize);
> + ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize, PAGE_SIZE));
See above about what ASSERT()s are for.
> + gbase = dt_read_number(cells, addr_cells);
> +
> + /* TODO: Consider owner domain is not the default dom_io. */
> + /*
> + * Per static shared memory region could be shared between multiple
> + * domains.
> + * In case re-allocating the same shared memory region, we check
> + * if it is already allocated to the default owner dom_io before
> + * the actual allocation.
> + */
> + if ( !is_shm_allocated_to_domio(pbase) )
> + {
> + /* Allocate statically shared pages to the default owner dom_io. */
> + ret = allocate_shared_memory(dom_io, addr_cells, size_cells,
> + pbase, psize);
> + if ( ret )
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +#endif /* CONFIG_STATIC_SHM */
> #else
> static void __init allocate_static_memory(struct domain *d,
> struct kernel_info *kinfo,
> @@ -3236,6 +3360,12 @@ static int __init construct_domU(struct domain *d,
> else
> assign_static_memory_11(d, &kinfo, node);
>
> +#ifdef CONFIG_STATIC_SHM
> + rc = process_shm(d, node);
> + if ( rc < 0 )
> + return rc;
> +#endif
> +
> /*
> * Base address and irq number are needed when creating vpl011 device
> * tree node in prepare_dtb_domU, so initialization on related variables
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 7570eae91a..7070f5a9b9 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -780,6 +780,9 @@ void __init setup_system_domains(void)
> * This domain owns I/O pages that are within the range of the page_info
> * array. Mappings occur at the priv of the caller.
> * Quarantined PCI devices will be associated with this domain.
> + *
> + * DOMID_IO is also the default owner of memory pre-shared among multiple
> + * domains at boot time.
> */
> dom_io = domain_create(DOMID_IO, NULL, 0);
> if ( IS_ERR(dom_io) )
Cheers,
--
Julien Grall
Hi Julien
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Saturday, June 25, 2022 2:22 AM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>;
> Jan Beulich <jbeulich@suse.com>; Wei Liu <wl@xen.org>
> Subject: Re: [PATCH v5 2/8] xen/arm: allocate static shared memory to the
> default owner dom_io
>
> Hi Penny,
>
> On 20/06/2022 06:11, Penny Zheng wrote:
> > From: Penny Zheng <penny.zheng@arm.com>
> >
> > This commit introduces process_shm to cope with static shared memory
> > in domain construction.
> >
> > DOMID_IO will be the default owner of memory pre-shared among
> multiple
> > domains at boot time, when no explicit owner is specified.
>
> The document in patch #1 suggest the page will be shared with dom_shared.
> But here you say "DOMID_IO".
>
> Which one is correct?
>
I’ll fix the documentation, DOM_IO is the last call.
> >
> > This commit only considers allocating static shared memory to dom_io
> > when owner domain is not explicitly defined in device tree, all the
> > left, including the "borrower" code path, the "explicit owner" code
> > path, shall be introduced later in the following patches.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> > ---
> > v5 change:
> > - refine in-code comment
> > ---
> > v4 change:
> > - no changes
> > ---
> > v3 change:
> > - refine in-code comment
> > ---
> > v2 change:
> > - instead of introducing a new system domain, reuse the existing
> > dom_io
> > - make dom_io a non-auto-translated domain, then no need to create P2M
> > for it
> > - change dom_io definition and make it wider to support static shm
> > here too
> > - introduce is_shm_allocated_to_domio to check whether static shm is
> > allocated yet, instead of using shm_mask bitmap
> > - add in-code comment
> > ---
> > xen/arch/arm/domain_build.c | 132
> +++++++++++++++++++++++++++++++++++-
> > xen/common/domain.c | 3 +
> > 2 files changed, 134 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 7ddd16c26d..91a5ace851 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -527,6 +527,10 @@ static bool __init
> append_static_memory_to_bank(struct domain *d,
> > return true;
> > }
> >
> > +/*
> > + * If cell is NULL, pbase and psize should hold valid values.
> > + * Otherwise, cell will be populated together with pbase and psize.
> > + */
> > static mfn_t __init acquire_static_memory_bank(struct domain *d,
> > const __be32 **cell,
> > u32 addr_cells, u32
> > size_cells, @@ -535,7 +539,8 @@ static mfn_t __init
> acquire_static_memory_bank(struct domain *d,
> > mfn_t smfn;
> > int res;
> >
> > - device_tree_get_reg(cell, addr_cells, size_cells, pbase, psize);
> > + if ( cell )
> > + device_tree_get_reg(cell, addr_cells, size_cells, pbase,
> > + psize);
>
> I think this is a bit of a hack. To me it sounds like this should be moved out to
> a separate helper. This will also make the interface of
> acquire_shared_memory_bank() less questionable (see below).
>
Ok, I'll try to not reuse acquire_static_memory_bank in
acquire_shared_memory_bank.
> As this is v5, I would be OK with a follow-up for this split. But this interface of
> acuiqre_shared_memory_bank() needs to change.
>
I'll try to fix it in next version.
> > ASSERT(IS_ALIGNED(*pbase, PAGE_SIZE) && IS_ALIGNED(*psize,
> > PAGE_SIZE));
>
> In the context of your series, who is checking that both psize and pbase are
> suitably aligned?
>
Actually, the whole parsing process is redundant for the static shared memory.
I've already parsed it and checked it before in process_shm.
> > if ( PFN_DOWN(*psize) > UINT_MAX )
> > {
> > @@ -759,6 +764,125 @@ static void __init assign_static_memory_11(struct
> domain *d,
> > panic("Failed to assign requested static memory for direct-map
> domain %pd.",
> > d);
> > }
> > +
> > +#ifdef CONFIG_STATIC_SHM
> > +/*
> > + * This function checks whether the static shared memory region is
> > + * already allocated to dom_io.
> > + */
> > +static bool __init is_shm_allocated_to_domio(paddr_t pbase) {
> > + struct page_info *page;
> > +
> > + page = maddr_to_page(pbase);
> > + ASSERT(page);
>
> maddr_to_page() can never return NULL. If you want to check a page will be
> valid, then you should use mfn_valid().
>
> However, the ASSERT() implies that the address was suitably checked before.
> But I can't find such check.
>
> > +
> > + if ( page_get_owner(page) == NULL )
> > + return false;
> > +
> > + ASSERT(page_get_owner(page) == dom_io);
> Could this be hit because of a wrong device-tree? If yes, then this should not
> be an ASSERT() (they are not suitable to check user input).
>
Yes, it can happen, I'll change it to if-array to output the error.
> > + return true;
> > +}
> > +
> > +static mfn_t __init acquire_shared_memory_bank(struct domain *d,
> > + u32 addr_cells, u32 size_cells,
> > + paddr_t *pbase,
> > +paddr_t *psize)
>
> There is something that doesn't add-up in this interface. The use of pointer
> implies that pbase and psize may be modified by the function. So...
>
Just like you points out before, it's a bit hacky to use acquire_static_memory_bank,
and the pointer used here is also due to it. Internal parsing process of acquire_static_memory_bank
needs pointer to deliver the result.
I’ll rewrite acquire_shared_memory, and it will be like:
"
static mfn_t __init acquire_shared_memory_bank(struct domain *d,
paddr_t pbase, paddr_t psize)
{
mfn_t smfn;
unsigned long nr_pfns;
int res;
/*
* Pages of statically shared memory shall be included
* in domain_tot_pages().
*/
nr_pfns = PFN_DOWN(psize);
if ( d->max_page + nr_pfns > UINT_MAX )
{
printk(XENLOG_ERR "%pd: Over-allocation for d->max_pages: %lu.\n",
d, psize);
return INVALID_MFN;
}
d->max_pages += nr_pfns;
smfn = maddr_to_mfn(pbase);
res = acquire_domstatic_pages(d, smfn, nr_pfns, 0);
if ( res )
{
printk(XENLOG_ERR
"%pd: failed to acquire static memory: %d.\n", d, res);
return INVALID_MFN;
}
return smfn
}
"
> > +{
> > + /*
> > + * Pages of statically shared memory shall be included
> > + * in domain_tot_pages().
> > + */
> > + d->max_pages += PFN_DOWN(*psize);
>
> ... it sounds a bit strange to use psize here. If psize, can't be modified than it
> should probably not be a pointer.
>
> Also, where do you check that d->max_pages will not overflow?
>
I'll check the overflow as follows:
"
nr_pfns = PFN_DOWN(psize);
if ( d->max_page + nr_pfns > UINT_MAX )
{
printk(XENLOG_ERR "%pd: Over-allocation for d->max_pages: %lu.\n",
d, psize);
return INVALID_MFN;
}
d->max_pages += nr_pfns;
"
> > +
> > + return acquire_static_memory_bank(d, NULL, addr_cells, size_cells,
> > + pbase, psize);
> > +
> > +}
> > +
> > +/*
> > + * Func allocate_shared_memory is supposed to be only called
>
> I am a bit concerned with the word "supposed". Are you implying that it may
> be called by someone that is not the owner? If not, then it should be
> "should".
>
> Also NIT: Spell out completely "func". I.e "The function".
>
> > + * from the owner.
>
> I read from as "current should be the owner". But I guess this is not what you
> mean here. Instead it looks like you mean "d" is the owner. So I would write
> "d should be the owner of the shared area".
>
> It would be good to have a check/ASSERT confirm this (assuming this is easy
> to write).
>
The check is already in the calling path, I guess...
Only under certain circumstances, we could call allocate_shared_memory
> > + */
> > +static int __init allocate_shared_memory(struct domain *d,
> > + u32 addr_cells, u32 size_cells,
> > + paddr_t pbase, paddr_t
> > +psize) {
> > + mfn_t smfn;
> > +
> > + dprintk(XENLOG_INFO,
> > + "Allocate static shared memory BANK %#"PRIpaddr"-
> %#"PRIpaddr".\n",
> > + pbase, pbase + psize);
>
> NIT: I would suggest to also print the domain. This could help to easily figure
> out that 'd' wasn't the owner.
>
Sure
> > +
> > + smfn = acquire_shared_memory_bank(d, addr_cells, size_cells, &pbase,
> > + &psize);
> > + if ( mfn_eq(smfn, INVALID_MFN) )
> > + return -EINVAL;
> > +
> > + /*
> > + * DOMID_IO is the domain, like DOMID_XEN, that is not auto-
> translated.
> > + * It sees RAM 1:1 and we do not need to create P2M mapping for it
> > + */
> > + ASSERT(d == dom_io);
> > + return 0;
> > +}
> > +
> > +static int __init process_shm(struct domain *d,
> > + const struct dt_device_node *node) {
> > + struct dt_device_node *shm_node;
> > + int ret = 0;
> > + const struct dt_property *prop;
> > + const __be32 *cells;
> > + u32 shm_id;
> > + u32 addr_cells, size_cells;
> > + paddr_t gbase, pbase, psize;
> > +
> > + dt_for_each_child_node(node, shm_node)
> > + {
> > + if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-
> memory-v1") )
> > + continue;
> > +
> > + if ( !dt_property_read_u32(shm_node, "xen,shm-id", &shm_id) )
> > + {
> > + printk("Shared memory node does not provide \"xen,shm-id\"
> property.\n");
> > + return -ENOENT;
> > + }
> > +
> > + addr_cells = dt_n_addr_cells(shm_node);
> > + size_cells = dt_n_size_cells(shm_node);
> > + prop = dt_find_property(shm_node, "xen,shared-mem", NULL);
> > + if ( !prop )
> > + {
> > + printk("Shared memory node does not provide \"xen,shared-
> mem\" property.\n");
> > + return -ENOENT;
> > + }
> > + cells = (const __be32 *)prop->value;
> > + /* xen,shared-mem = <pbase, psize, gbase>; */
> > + device_tree_get_reg(&cells, addr_cells, size_cells, &pbase, &psize);
> > + ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize,
> > + PAGE_SIZE));
>
> See above about what ASSERT()s are for.
>
Do you think address was suitably checked here, is it enough?
If it is enough, I'll modify above ASSERT() to mfn_valid()
> > + gbase = dt_read_number(cells, addr_cells);
> > +
> > + /* TODO: Consider owner domain is not the default dom_io. */
> > + /*
> > + * Per static shared memory region could be shared between multiple
> > + * domains.
> > + * In case re-allocating the same shared memory region, we check
> > + * if it is already allocated to the default owner dom_io before
> > + * the actual allocation.
> > + */
> > + if ( !is_shm_allocated_to_domio(pbase) )
> > + {
> > + /* Allocate statically shared pages to the default owner dom_io. */
> > + ret = allocate_shared_memory(dom_io, addr_cells, size_cells,
> > + pbase, psize);
> > + if ( ret )
> > + return ret;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +#endif /* CONFIG_STATIC_SHM */
> > #else
> > static void __init allocate_static_memory(struct domain *d,
> > struct kernel_info *kinfo,
> > @@ -3236,6 +3360,12 @@ static int __init construct_domU(struct domain
> *d,
> > else
> > assign_static_memory_11(d, &kinfo, node);
> >
> > +#ifdef CONFIG_STATIC_SHM
> > + rc = process_shm(d, node);
> > + if ( rc < 0 )
> > + return rc;
> > +#endif
> > +
> > /*
> > * Base address and irq number are needed when creating vpl011
> device
> > * tree node in prepare_dtb_domU, so initialization on related
> > variables diff --git a/xen/common/domain.c b/xen/common/domain.c
> index
> > 7570eae91a..7070f5a9b9 100644
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -780,6 +780,9 @@ void __init setup_system_domains(void)
> > * This domain owns I/O pages that are within the range of the
> page_info
> > * array. Mappings occur at the priv of the caller.
> > * Quarantined PCI devices will be associated with this domain.
> > + *
> > + * DOMID_IO is also the default owner of memory pre-shared among
> multiple
> > + * domains at boot time.
> > */
> > dom_io = domain_create(DOMID_IO, NULL, 0);
> > if ( IS_ERR(dom_io) )
>
> Cheers,
>
> --
> Julien Grall
On 29/06/2022 08:13, Penny Zheng wrote:
> Hi Julien
Hi Penny,
>
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: Saturday, June 25, 2022 2:22 AM
>> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
>> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
>> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
>> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Andrew Cooper
>> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>;
>> Jan Beulich <jbeulich@suse.com>; Wei Liu <wl@xen.org>
>> Subject: Re: [PATCH v5 2/8] xen/arm: allocate static shared memory to the
>> default owner dom_io
>>
>> Hi Penny,
>>
>> On 20/06/2022 06:11, Penny Zheng wrote:
>>> From: Penny Zheng <penny.zheng@arm.com>
>>>
>>> This commit introduces process_shm to cope with static shared memory
>>> in domain construction.
>>>
>>> DOMID_IO will be the default owner of memory pre-shared among
>> multiple
>>> domains at boot time, when no explicit owner is specified.
>>
>> The document in patch #1 suggest the page will be shared with dom_shared.
>> But here you say "DOMID_IO".
>>
>> Which one is correct?
>>
>
> I’ll fix the documentation, DOM_IO is the last call.
>
>>>
>>> This commit only considers allocating static shared memory to dom_io
>>> when owner domain is not explicitly defined in device tree, all the
>>> left, including the "borrower" code path, the "explicit owner" code
>>> path, shall be introduced later in the following patches.
>>>
>>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>> ---
>>> v5 change:
>>> - refine in-code comment
>>> ---
>>> v4 change:
>>> - no changes
>>> ---
>>> v3 change:
>>> - refine in-code comment
>>> ---
>>> v2 change:
>>> - instead of introducing a new system domain, reuse the existing
>>> dom_io
>>> - make dom_io a non-auto-translated domain, then no need to create P2M
>>> for it
>>> - change dom_io definition and make it wider to support static shm
>>> here too
>>> - introduce is_shm_allocated_to_domio to check whether static shm is
>>> allocated yet, instead of using shm_mask bitmap
>>> - add in-code comment
>>> ---
>>> xen/arch/arm/domain_build.c | 132
>> +++++++++++++++++++++++++++++++++++-
>>> xen/common/domain.c | 3 +
>>> 2 files changed, 134 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index 7ddd16c26d..91a5ace851 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -527,6 +527,10 @@ static bool __init
>> append_static_memory_to_bank(struct domain *d,
>>> return true;
>>> }
>>>
>>> +/*
>>> + * If cell is NULL, pbase and psize should hold valid values.
>>> + * Otherwise, cell will be populated together with pbase and psize.
>>> + */
>>> static mfn_t __init acquire_static_memory_bank(struct domain *d,
>>> const __be32 **cell,
>>> u32 addr_cells, u32
>>> size_cells, @@ -535,7 +539,8 @@ static mfn_t __init
>> acquire_static_memory_bank(struct domain *d,
>>> mfn_t smfn;
>>> int res;
>>>
>>> - device_tree_get_reg(cell, addr_cells, size_cells, pbase, psize);
>>> + if ( cell )
>>> + device_tree_get_reg(cell, addr_cells, size_cells, pbase,
>>> + psize);
>>
>> I think this is a bit of a hack. To me it sounds like this should be moved out to
>> a separate helper. This will also make the interface of
>> acquire_shared_memory_bank() less questionable (see below).
>>
>
> Ok, I'll try to not reuse acquire_static_memory_bank in
> acquire_shared_memory_bank.
I am OK with that so long it doesn't involve too much duplication.
>>> ASSERT(IS_ALIGNED(*pbase, PAGE_SIZE) && IS_ALIGNED(*psize,
>>> PAGE_SIZE));
>>
>> In the context of your series, who is checking that both psize and pbase are
>> suitably aligned?
>>
>
> Actually, the whole parsing process is redundant for the static shared memory.
> I've already parsed it and checked it before in process_shm.
I looked at process_shm() and couldn't find any code that would check
pbase and psize are suitable aligned (ASSERT() doesn't count).
>
>>> + return true;
>>> +}
>>> +
>>> +static mfn_t __init acquire_shared_memory_bank(struct domain *d,
>>> + u32 addr_cells, u32 size_cells,
>>> + paddr_t *pbase,
>>> +paddr_t *psize)
>>
>> There is something that doesn't add-up in this interface. The use of pointer
>> implies that pbase and psize may be modified by the function. So...
>>
>
> Just like you points out before, it's a bit hacky to use acquire_static_memory_bank,
> and the pointer used here is also due to it. Internal parsing process of acquire_static_memory_bank
> needs pointer to deliver the result.
>
> I’ll rewrite acquire_shared_memory, and it will be like:
> "
> static mfn_t __init acquire_shared_memory_bank(struct domain *d,
> paddr_t pbase, paddr_t psize)
> {
> mfn_t smfn;
> unsigned long nr_pfns;
> int res;
>
> /*
> * Pages of statically shared memory shall be included
> * in domain_tot_pages().
> */
> nr_pfns = PFN_DOWN(psize);
> if ( d->max_page + nr_pfns > UINT_MAX )
On Arm32, this check would always be true a 32-bit unsigned value is
always below UINT_MAX. On arm64, you might get away because nr_pfns is
unsigned long (so I think the type promotion works). But this is fragile.
I would suggest to use the following check:
(UINT_MAX - d->max_page) < nr_pfns
> {
> printk(XENLOG_ERR "%pd: Over-allocation for d->max_pages: %lu.\n",
> d, psize);
> return INVALID_MFN;
> }
> d->max_pages += nr_pfns;
>
> smfn = maddr_to_mfn(pbase);
> res = acquire_domstatic_pages(d, smfn, nr_pfns, 0);
> if ( res )
> {
> printk(XENLOG_ERR
> "%pd: failed to acquire static memory: %d.\n", d, res);
> return INVALID_MFN;
> }
>
> return smfn
> }
> "
>
>>> +{
>>> + /*
>>> + * Pages of statically shared memory shall be included
>>> + * in domain_tot_pages().
>>> + */
>>> + d->max_pages += PFN_DOWN(*psize);
>>
>> ... it sounds a bit strange to use psize here. If psize, can't be modified than it
>> should probably not be a pointer.
>>
>> Also, where do you check that d->max_pages will not overflow?
>>
>
> I'll check the overflow as follows:
See above about the check.
> "
> nr_pfns = PFN_DOWN(psize);
> if ( d->max_page + nr_pfns > UINT_MAX )
> {
> printk(XENLOG_ERR "%pd: Over-allocation for d->max_pages: %lu.\n",
> d, psize);
> return INVALID_MFN;
> }
> d->max_pages += nr_pfns;
> "
>
>>> +
>>> + return acquire_static_memory_bank(d, NULL, addr_cells, size_cells,
>>> + pbase, psize);
>>> +
>>> +}
>>> +
>>> +/*
>>> + * Func allocate_shared_memory is supposed to be only called
>>
>> I am a bit concerned with the word "supposed". Are you implying that it may
>> be called by someone that is not the owner? If not, then it should be
>> "should".
>>
>> Also NIT: Spell out completely "func". I.e "The function".
>>
>>> + * from the owner.
>>
>> I read from as "current should be the owner". But I guess this is not what you
>> mean here. Instead it looks like you mean "d" is the owner. So I would write
>> "d should be the owner of the shared area".
>>
>> It would be good to have a check/ASSERT confirm this (assuming this is easy
>> to write).
>>
>
> The check is already in the calling path, I guess...
Can you please confirm it?
[...]
>>> + prop = dt_find_property(shm_node, "xen,shared-mem", NULL);
>>> + if ( !prop )
>>> + {
>>> + printk("Shared memory node does not provide \"xen,shared-
>> mem\" property.\n");
>>> + return -ENOENT;
>>> + }
>>> + cells = (const __be32 *)prop->value;
>>> + /* xen,shared-mem = <pbase, psize, gbase>; */
>>> + device_tree_get_reg(&cells, addr_cells, size_cells, &pbase, &psize);
>>> + ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize,
>>> + PAGE_SIZE));
>>
>> See above about what ASSERT()s are for.
>>
>
> Do you think address was suitably checked here, is it enough?
As I wrote before, ASSERT() should not be used to check user inputs.
They need to happen in both debug and production build.
> If it is enough, I'll modify above ASSERT() to mfn_valid()
It is not clear what ASSERT() you are referring to.
Cheers,
--
Julien Grall
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Wednesday, June 29, 2022 6:35 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>;
> Jan Beulich <jbeulich@suse.com>; Wei Liu <wl@xen.org>
> Subject: Re: [PATCH v5 2/8] xen/arm: allocate static shared memory to the
> default owner dom_io
>
>
>
> On 29/06/2022 08:13, Penny Zheng wrote:
> > Hi Julien
>
> Hi Penny,
>
Hi Julien
> >
> >> -----Original Message-----
> >> From: Julien Grall <julien@xen.org>
> >> Sent: Saturday, June 25, 2022 2:22 AM
> >> To: Penny Zheng <Penny.Zheng@arm.com>; xen-
> devel@lists.xenproject.org
> >> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> >> <sstabellini@kernel.org>; Bertrand Marquis
> >> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> >> <Volodymyr_Babchuk@epam.com>; Andrew Cooper
> >> <andrew.cooper3@citrix.com>; George Dunlap
> >> <george.dunlap@citrix.com>; Jan Beulich <jbeulich@suse.com>; Wei Liu
> >> <wl@xen.org>
> >> Subject: Re: [PATCH v5 2/8] xen/arm: allocate static shared memory to
> >> the default owner dom_io
> >>
> >> Hi Penny,
> >>
> >> On 20/06/2022 06:11, Penny Zheng wrote:
> >>> From: Penny Zheng <penny.zheng@arm.com>
> >>>
> >>> This commit introduces process_shm to cope with static shared memory
> >>> in domain construction.
> >>>
> >>> DOMID_IO will be the default owner of memory pre-shared among
> >> multiple
> >>> domains at boot time, when no explicit owner is specified.
> >>
> >> The document in patch #1 suggest the page will be shared with
> dom_shared.
> >> But here you say "DOMID_IO".
> >>
> >> Which one is correct?
> >>
> >
> > I’ll fix the documentation, DOM_IO is the last call.
> >
> >>>
> >>> This commit only considers allocating static shared memory to dom_io
> >>> when owner domain is not explicitly defined in device tree, all the
> >>> left, including the "borrower" code path, the "explicit owner" code
> >>> path, shall be introduced later in the following patches.
> >>>
> >>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> >>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> >>> ---
> >>> v5 change:
> >>> - refine in-code comment
> >>> ---
> >>> v4 change:
> >>> - no changes
> >>> ---
> >>> v3 change:
> >>> - refine in-code comment
> >>> ---
> >>> v2 change:
> >>> - instead of introducing a new system domain, reuse the existing
> >>> dom_io
> >>> - make dom_io a non-auto-translated domain, then no need to create
> >>> P2M for it
> >>> - change dom_io definition and make it wider to support static shm
> >>> here too
> >>> - introduce is_shm_allocated_to_domio to check whether static shm is
> >>> allocated yet, instead of using shm_mask bitmap
> >>> - add in-code comment
> >>> ---
> >>> xen/arch/arm/domain_build.c | 132
> >> +++++++++++++++++++++++++++++++++++-
> >>> xen/common/domain.c | 3 +
> >>> 2 files changed, 134 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/xen/arch/arm/domain_build.c
> >>> b/xen/arch/arm/domain_build.c index 7ddd16c26d..91a5ace851 100644
> >>> --- a/xen/arch/arm/domain_build.c
> >>> +++ b/xen/arch/arm/domain_build.c
> >>> @@ -527,6 +527,10 @@ static bool __init
> >> append_static_memory_to_bank(struct domain *d,
> >>> return true;
> >>> }
> >>>
> >>> +/*
> >>> + * If cell is NULL, pbase and psize should hold valid values.
> >>> + * Otherwise, cell will be populated together with pbase and psize.
> >>> + */
> >>> static mfn_t __init acquire_static_memory_bank(struct domain *d,
> >>> const __be32 **cell,
> >>> u32 addr_cells,
> >>> u32 size_cells, @@ -535,7 +539,8 @@ static mfn_t __init
> >> acquire_static_memory_bank(struct domain *d,
> >>> mfn_t smfn;
> >>> int res;
> >>>
> >>> - device_tree_get_reg(cell, addr_cells, size_cells, pbase, psize);
> >>> + if ( cell )
> >>> + device_tree_get_reg(cell, addr_cells, size_cells, pbase,
> >>> + psize);
> >>
> >> I think this is a bit of a hack. To me it sounds like this should be
> >> moved out to a separate helper. This will also make the interface of
> >> acquire_shared_memory_bank() less questionable (see below).
> >>
> >
> > Ok, I'll try to not reuse acquire_static_memory_bank in
> > acquire_shared_memory_bank.
>
> I am OK with that so long it doesn't involve too much duplication.
>
> >>> ASSERT(IS_ALIGNED(*pbase, PAGE_SIZE) && IS_ALIGNED(*psize,
> >>> PAGE_SIZE));
> >>
> >> In the context of your series, who is checking that both psize and
> >> pbase are suitably aligned?
> >>
> >
> > Actually, the whole parsing process is redundant for the static shared
> memory.
> > I've already parsed it and checked it before in process_shm.
>
> I looked at process_shm() and couldn't find any code that would check pbase
> and psize are suitable aligned (ASSERT() doesn't count).
>
> >
> >>> + return true;
> >>> +}
> >>> +
> >>> +static mfn_t __init acquire_shared_memory_bank(struct domain *d,
> >>> + u32 addr_cells, u32 size_cells,
> >>> + paddr_t *pbase,
> >>> +paddr_t *psize)
> >>
> >> There is something that doesn't add-up in this interface. The use of
> >> pointer implies that pbase and psize may be modified by the function. So...
> >>
> >
> > Just like you points out before, it's a bit hacky to use
> > acquire_static_memory_bank, and the pointer used here is also due to
> > it. Internal parsing process of acquire_static_memory_bank needs pointer
> to deliver the result.
> >
> > I’ll rewrite acquire_shared_memory, and it will be like:
> > "
> > static mfn_t __init acquire_shared_memory_bank(struct domain *d,
> > paddr_t pbase, paddr_t
> > psize) {
> > mfn_t smfn;
> > unsigned long nr_pfns;
> > int res;
> >
> > /*
> > * Pages of statically shared memory shall be included
> > * in domain_tot_pages().
> > */
> > nr_pfns = PFN_DOWN(psize);
> > if ( d->max_page + nr_pfns > UINT_MAX )
>
> On Arm32, this check would always be true a 32-bit unsigned value is always
> below UINT_MAX. On arm64, you might get away because nr_pfns is
> unsigned long (so I think the type promotion works). But this is fragile.
>
> I would suggest to use the following check:
>
> (UINT_MAX - d->max_page) < nr_pfns
>
> > {
> > printk(XENLOG_ERR "%pd: Over-allocation for d->max_pages: %lu.\n",
> > d, psize);
> > return INVALID_MFN;
> > }
> > d->max_pages += nr_pfns;
> >
> > smfn = maddr_to_mfn(pbase);
> > res = acquire_domstatic_pages(d, smfn, nr_pfns, 0);
> > if ( res )
> > {
> > printk(XENLOG_ERR
> > "%pd: failed to acquire static memory: %d.\n", d, res);
> > return INVALID_MFN;
> > }
> >
> > return smfn
> > }
> > "
> >
> >>> +{
> >>> + /*
> >>> + * Pages of statically shared memory shall be included
> >>> + * in domain_tot_pages().
> >>> + */
> >>> + d->max_pages += PFN_DOWN(*psize);
> >>
> >> ... it sounds a bit strange to use psize here. If psize, can't be
> >> modified than it should probably not be a pointer.
> >>
> >> Also, where do you check that d->max_pages will not overflow?
> >>
> >
> > I'll check the overflow as follows:
>
> See above about the check.
>
> > "
> > nr_pfns = PFN_DOWN(psize);
> > if ( d->max_page + nr_pfns > UINT_MAX )
> > {
> > printk(XENLOG_ERR "%pd: Over-allocation for d->max_pages: %lu.\n",
> > d, psize);
> > return INVALID_MFN;
> > }
> > d->max_pages += nr_pfns;
> > "
> >
> >>> +
> >>> + return acquire_static_memory_bank(d, NULL, addr_cells, size_cells,
> >>> + pbase, psize);
> >>> +
> >>> +}
> >>> +
> >>> +/*
> >>> + * Func allocate_shared_memory is supposed to be only called
> >>
> >> I am a bit concerned with the word "supposed". Are you implying that
> >> it may be called by someone that is not the owner? If not, then it
> >> should be "should".
> >>
> >> Also NIT: Spell out completely "func". I.e "The function".
> >>
> >>> + * from the owner.
> >>
> >> I read from as "current should be the owner". But I guess this is not
> >> what you mean here. Instead it looks like you mean "d" is the owner.
> >> So I would write "d should be the owner of the shared area".
> >>
> >> It would be good to have a check/ASSERT confirm this (assuming this
> >> is easy to write).
> >>
> >
> > The check is already in the calling path, I guess...
>
> Can you please confirm it?
>
I mean that allocate_shared_memory is only called under the following condition, and
it confirms it is the right owner.
"
if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) ||
(!owner_dom_io && strcmp(role_str, "owner") == 0) )
{
/* Allocate statically shared pages to the owner domain. */
ret = allocate_shared_memory(owner_dom_io ? dom_io : d,
addr_cells, size_cells,
pbase, psize, gbase);
"
TBH, apart from that, I don't know how to check if the input d is the right owner, or am I
misunderstanding some your suggestion here?
> [...]
>
> >>> + prop = dt_find_property(shm_node, "xen,shared-mem", NULL);
> >>> + if ( !prop )
> >>> + {
> >>> + printk("Shared memory node does not provide
> >>> + \"xen,shared-
> >> mem\" property.\n");
> >>> + return -ENOENT;
> >>> + }
> >>> + cells = (const __be32 *)prop->value;
> >>> + /* xen,shared-mem = <pbase, psize, gbase>; */
> >>> + device_tree_get_reg(&cells, addr_cells, size_cells, &pbase, &psize);
> >>> + ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize,
> >>> + PAGE_SIZE));
> >>
> >> See above about what ASSERT()s are for.
> >>
> >
> > Do you think address was suitably checked here, is it enough?
>
> As I wrote before, ASSERT() should not be used to check user inputs.
> They need to happen in both debug and production build.
>
> > If it is enough, I'll modify above ASSERT() to mfn_valid()
>
> It is not clear what ASSERT() you are referring to.
>
For whether page is aligned, I will add the below check:
"
if ( !IS_ALIGNED(pbase, PAGE_SIZE) || !IS_ALIGNED(psize, PAGE_SIZE) ||
!IS_ALIGNED(gbase, PAGE_SIZE) )
{
printk("%pd: physical address %lu, size %lu or guest address %lu is not suitably aligned.\n",
d, pbase, psize, gbase);
return -EINVAL;
}
"
For whether the whole address range is valid, I will add the below check:
"
for ( i = 0; i < PFN_DOWN(psize); i++ )
if ( !mfn_valid(mfn_add(maddr_to_mfn(pbase), i)) )
{
printk("%pd: invalid physical address %"PRI_paddr" or size %"PRI_paddr"\n",
d, pbase, psize);
return -EINVAL;
}
"
> Cheers,
>
> --
> Julien Grall
Hi Penny,
On 04/07/2022 08:20, Penny Zheng wrote:
>>>>> +
>>>>> + return acquire_static_memory_bank(d, NULL, addr_cells, size_cells,
>>>>> + pbase, psize);
>>>>> +
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * Func allocate_shared_memory is supposed to be only called
>>>>
>>>> I am a bit concerned with the word "supposed". Are you implying that
>>>> it may be called by someone that is not the owner? If not, then it
>>>> should be "should".
>>>>
>>>> Also NIT: Spell out completely "func". I.e "The function".
>>>>
>>>>> + * from the owner.
>>>>
>>>> I read from as "current should be the owner". But I guess this is not
>>>> what you mean here. Instead it looks like you mean "d" is the owner.
>>>> So I would write "d should be the owner of the shared area".
>>>>
>>>> It would be good to have a check/ASSERT confirm this (assuming this
>>>> is easy to write).
>>>>
>>>
>>> The check is already in the calling path, I guess...
>>
>> Can you please confirm it?
>>
>
> I mean that allocate_shared_memory is only called under the following condition, and
> it confirms it is the right owner.
> "
> if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) ||
> (!owner_dom_io && strcmp(role_str, "owner") == 0) )
> {
> /* Allocate statically shared pages to the owner domain. */
> ret = allocate_shared_memory(owner_dom_io ? dom_io : d,
> addr_cells, size_cells,
> pbase, psize, gbase);
> "
I agree that this looks to be the case today. But error can slip easily,
so if we can add some ASSERT() in function then you could catch issues
during development. Hence why I suggested to add an ASSERT() if possible.
>
> TBH, apart from that, I don't know how to check if the input d is the right owner, or am I
> misunderstanding some your suggestion here?
Is page_get_owner() already properly set? If yes, you could ASSERT() the
first page of the range belongs to 'd'.
This is more an hardening exercise, so it is not critical if it is
difficult (or not possible) to have an ASSERT().
>
>> [...]
>>
>>>>> + prop = dt_find_property(shm_node, "xen,shared-mem", NULL);
>>>>> + if ( !prop )
>>>>> + {
>>>>> + printk("Shared memory node does not provide
>>>>> + \"xen,shared-
>>>> mem\" property.\n");
>>>>> + return -ENOENT;
>>>>> + }
>>>>> + cells = (const __be32 *)prop->value;
>>>>> + /* xen,shared-mem = <pbase, psize, gbase>; */
>>>>> + device_tree_get_reg(&cells, addr_cells, size_cells, &pbase, &psize);
>>>>> + ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize,
>>>>> + PAGE_SIZE));
>>>>
>>>> See above about what ASSERT()s are for.
>>>>
>>>
>>> Do you think address was suitably checked here, is it enough?
>>
>> As I wrote before, ASSERT() should not be used to check user inputs.
>> They need to happen in both debug and production build.
>>
>>> If it is enough, I'll modify above ASSERT() to mfn_valid()
>>
>> It is not clear what ASSERT() you are referring to.
>>
>
> For whether page is aligned, I will add the below check:
> "
> if ( !IS_ALIGNED(pbase, PAGE_SIZE) || !IS_ALIGNED(psize, PAGE_SIZE) ||
> !IS_ALIGNED(gbase, PAGE_SIZE) )
> {
> printk("%pd: physical address %lu, size %lu or guest address %lu is not suitably aligned.\n",
AFAICT, the type for the 3 variables is paddr_t. So you want to use
0x%"PRIpaddr" instead of %lu.
BTW, in general, for address it is preferable to use hexadecimal over
decimal.
> d, pbase, psize, gbase);
> return -EINVAL;
> }
> "
> For whether the whole address range is valid, I will add the below check:
> "
> for ( i = 0; i < PFN_DOWN(psize); i++ )
> if ( !mfn_valid(mfn_add(maddr_to_mfn(pbase), i)) )
> {
> printk("%pd: invalid physical address %"PRI_paddr" or size %"PRI_paddr"\n",
s/PRI_paddr/PRIpaddr/
I am also not sure why you want to print the size here?
> d, pbase, psize);
> return -EINVAL;
> }
> "
Cheers,
--
Julien Grall
© 2016 - 2026 Red Hat, Inc.