Hi Michal,
On 3/11/2024 5:10 PM, Michal Orzel wrote:
Hi Henry,
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 5e7a7f3e7e..54f3601ab0 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -696,6 +696,7 @@ int arch_domain_create(struct domain *d,
{
unsigned int count = 0;
int rc;
+ struct mem_map_domain *mem_map = &d->arch.mem_map;
BUILD_BUG_ON(GUEST_MAX_VCPUS < MAX_VIRT_CPUS);
@@ -785,6 +786,11 @@ int arch_domain_create(struct domain *d,
d->arch.sve_vl = config->arch.sve_vl;
#endif
+ mem_map->regions[mem_map->nr_mem_regions].start = GUEST_MAGIC_BASE;
You don't check for exceeding max number of regions. Is the expectation that
nr_mem_regions
is 0 at this stage? Maybe add an ASSERT here.
Sure, I will add the checking.
+ mem_map->regions[mem_map->nr_mem_regions].size = GUEST_MAGIC_SIZE;
+ mem_map->regions[mem_map->nr_mem_regions].type = GUEST_MEM_REGION_MAGIC;
+ mem_map->nr_mem_regions++;
+
return 0;
fail:
diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
index ad56efb0f5..92024bcaa0 100644
--- a/xen/arch/arm/domctl.c
+++ b/xen/arch/arm/domctl.c
@@ -148,7 +148,6 @@ long arch_do_domctl(struct xen_domctl *domctl, struct
domain *d,
return 0;
}
-
case XEN_DOMCTL_vuart_op:
{
int rc;
@@ -176,6 +175,24 @@ long arch_do_domctl(struct xen_domctl *domctl, struct
domain *d,
return rc;
}
+ case XEN_DOMCTL_get_mem_map:
+ {
+ int rc;
Without initialization, what will be the rc value on success?
Thanks for catching this (and the copy back issue below). I made a silly
mistake here and didn't catch it as I also missed checking the rc in the
toolstack side...I will fix both side.
+ /*
+ * Cap the number of regions to the minimum value between toolstack and
+ * hypervisor to avoid overflowing the buffer.
+ */
+ uint32_t nr_regions = min(d->arch.mem_map.nr_mem_regions,
+ domctl->u.mem_map.nr_mem_regions);
+
+ if ( copy_to_guest(domctl->u.mem_map.buffer,
+ d->arch.mem_map.regions,
+ nr_regions) ||
+ __copy_to_guest(u_domctl, domctl, 1) )
In domctl.h, you wrote that nr_regions is IN/OUT but you don't seem to write
back the actual number
of regions.
Thanks. Added "domctl->u.mem_map.nr_mem_regions = nr_regions;" locally.
+/* Guest memory region types */
+#define GUEST_MEM_REGION_DEFAULT 0
What's the purpose of this default type? It seems unusued.
I added it because struct arch_domain (or we should say struct domain)
is zalloc-ed. So the default type field in struct xen_mem_region is 0.
Otherwise we may (mistakenly) define a region type as 0 and lead to
mistakes.
+#define GUEST_MEM_REGION_MAGIC 1
+
/* Physical Address Space */
/* Virtio MMIO mappings */
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index a33f9ec32b..77bf999651 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -946,6 +946,25 @@ struct xen_domctl_paging_mempool {
uint64_aligned_t size; /* Size in bytes. */
};
+#define XEN_MAX_MEM_REGIONS 1
The max number of regions can differ between arches. How are you going to
handle it?
I think we can add
```
#ifndef XEN_MAX_MEM_REGIONS
#define XEN_MAX_MEM_REGIONS 1
#endif
```
here and define the arch specific XEN_MAX_MEM_REGIONS in
public/arch-*.h. I will fix this in v3.
Kind regards,
Henry
~Michal
