[PATCH 0/3] Guest magic region allocation for 11 Dom0less domUs - Take two

[PATCH 0/3] Guest magic region allocation for 11 Dom0less domUs - Take two

[Henry Wang]

Hi all,

This series is trying to fix the reported guest magic region allocation
issue for 11 Dom0less domUs, an error message can seen from the
init-dom0less application on 1:1 direct-mapped Dom0less DomUs:
```
Allocating magic pages
memory.c:238:d0v0 mfn 0x39000 doesn't belong to d1
Error on alloc magic pages
```

This is because populate_physmap() automatically assumes gfn == mfn
for direct mapped domains. This cannot be true for the magic pages
that are allocated later for 1:1 Dom0less DomUs from the init-dom0less
helper application executed in Dom0. For domain using statically
allocated memory but not 1:1 direct-mapped, similar error "failed to
retrieve a reserved page" can be seen as the reserved memory list
is empty at that time.

In [1] I've tried to fix this issue by the domctl approach, and
discussions in [2] and [3] indicates that a domctl is not really
necessary, as we can simplify the issue to "allocate the Dom0less
guest magic regions at the Dom0less DomU build time and pass the
region base PFN to init-dom0less application". Therefore, the first
patch in this series will allocate magic pages for Dom0less DomUs,
the second patch will store the allocated region base PFN to HVMOP
params like HVM_PARAM_CALLBACK_IRQ, and the third patch uses the
HVMOP to get the stored guest magic region base PFN to avoid hardcoding
GUEST_MAGIC_BASE.

Gitlab CI for this series can be found in [4].

[1] https://lore.kernel.org/xen-devel/20240409045357.236802-1-xin.wang2@amd.com/
[2] https://lore.kernel.org/xen-devel/c7857223-eab8-409a-b618-6ec70f6165aa@apertussolutions.com/
[3] https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2404251508470.3940@ubuntu-linux-20-04-desktop/
[4] https://gitlab.com/xen-project/people/henryw/xen/-/pipelines/1268643360

Henry Wang (3):
  xen/arm/dom0less-build: Alloc magic pages for Dom0less DomUs from
    hypervisor
  xen/arm, tools: Add a new HVM_PARAM_MAGIC_BASE_PFN key in HVMOP
  tools/init-dom0less: Avoid hardcoding GUEST_MAGIC_BASE

 tools/helpers/init-dom0less.c   | 38 ++++++++++++++-------------------
 tools/libs/guest/xg_dom_arm.c   |  3 ++-
 xen/arch/arm/dom0less-build.c   | 24 +++++++++++++++++++++
 xen/arch/arm/hvm.c              |  1 +
 xen/include/public/arch-arm.h   |  1 +
 xen/include/public/hvm/params.h |  1 +
 6 files changed, 45 insertions(+), 23 deletions(-)

-- 
2.34.1

[PATCH 1/3] xen/arm/dom0less-build: Alloc magic pages for Dom0less DomUs from hypervisor

[Henry Wang]

There are use cases (for example using the PV driver) in Dom0less
setup that require Dom0less DomUs start immediately with Dom0, but
initialize XenStore later after Dom0's successful boot and call to
the init-dom0less application.

An error message can seen from the init-dom0less application on
1:1 direct-mapped domains:
```
Allocating magic pages
memory.c:238:d0v0 mfn 0x39000 doesn't belong to d1
Error on alloc magic pages
```
This is because currently the magic pages for Dom0less DomUs are
populated by the init-dom0less app through populate_physmap(), and
populate_physmap() automatically assumes gfn == mfn for 1:1 direct
mapped domains. This cannot be true for the magic pages that are
allocated later from the init-dom0less application executed in Dom0.
For domain using statically allocated memory but not 1:1 direct-mapped,
similar error "failed to retrieve a reserved page" can be seen as the
reserved memory list is empty at that time.

To solve above issue, this commit allocates the magic pages for
Dom0less DomUs at the domain construction time. The base address/PFN
of the magic page region will be noted and communicated to the
init-dom0less application in Dom0.

Reported-by: Alec Kwapis <alec.kwapis@medtronic.com>
Suggested-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Signed-off-by: Henry Wang <xin.wang2@amd.com>
---
 tools/libs/guest/xg_dom_arm.c |  1 -
 xen/arch/arm/dom0less-build.c | 22 ++++++++++++++++++++++
 xen/include/public/arch-arm.h |  1 +
 3 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/tools/libs/guest/xg_dom_arm.c b/tools/libs/guest/xg_dom_arm.c
index 2fd8ee7ad4..8cc7f27dbb 100644
--- a/tools/libs/guest/xg_dom_arm.c
+++ b/tools/libs/guest/xg_dom_arm.c
@@ -25,7 +25,6 @@
 
 #include "xg_private.h"
 
-#define NR_MAGIC_PAGES 4
 #define CONSOLE_PFN_OFFSET 0
 #define XENSTORE_PFN_OFFSET 1
 #define MEMACCESS_PFN_OFFSET 2
diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
index fb63ec6fd1..40dc85c759 100644
--- a/xen/arch/arm/dom0less-build.c
+++ b/xen/arch/arm/dom0less-build.c
@@ -834,11 +834,33 @@ static int __init construct_domU(struct domain *d,
 
     if ( kinfo.dom0less_feature & DOM0LESS_XENSTORE )
     {
+        struct page_info *magic_pg;
+        mfn_t mfn;
+        gfn_t gfn;
+
         ASSERT(hardware_domain);
         rc = alloc_xenstore_evtchn(d);
         if ( rc < 0 )
             return rc;
         d->arch.hvm.params[HVM_PARAM_STORE_PFN] = ~0ULL;
+
+        d->max_pages += NR_MAGIC_PAGES;
+        magic_pg = alloc_domheap_pages(d, get_order_from_pages(NR_MAGIC_PAGES), 0);
+        if ( magic_pg == NULL )
+            return -ENOMEM;
+
+        mfn = page_to_mfn(magic_pg);
+        if ( !is_domain_direct_mapped(d) )
+            gfn = gaddr_to_gfn(GUEST_MAGIC_BASE);
+        else
+            gfn = gaddr_to_gfn(mfn_to_maddr(mfn));
+
+        rc = guest_physmap_add_pages(d, gfn, mfn, NR_MAGIC_PAGES);
+        if ( rc )
+        {
+            free_domheap_pages(magic_pg, get_order_from_pages(NR_MAGIC_PAGES));
+            return rc;
+        }
     }
 
     return rc;
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index e167e14f8d..f24e7bbe37 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -475,6 +475,7 @@ typedef uint64_t xen_callback_t;
 
 #define GUEST_MAGIC_BASE  xen_mk_ullong(0x39000000)
 #define GUEST_MAGIC_SIZE  xen_mk_ullong(0x01000000)
+#define NR_MAGIC_PAGES 4
 
 #define GUEST_RAM_BANKS   2
 
-- 
2.34.1

[PATCH 2/3] xen/arm, tools: Add a new HVM_PARAM_MAGIC_BASE_PFN key in HVMOP

[Henry Wang]

For use cases such as Dom0less PV drivers, a mechanism to communicate
Dom0less DomU's static data with the runtime control plane (Dom0) is
needed. Since on Arm HVMOP is already the existing approach to address
such use cases (for example the allocation of HVM_PARAM_CALLBACK_IRQ),
add a new HVMOP key HVM_PARAM_MAGIC_BASE_PFN for storing the magic
page region base PFN. The value will be set at Dom0less DomU
construction time after Dom0less DomU's magic page region has been
allocated.

To keep consistent, also set the value for HVM_PARAM_MAGIC_BASE_PFN
for libxl guests in alloc_magic_pages().

Reported-by: Alec Kwapis <alec.kwapis@medtronic.com>
Signed-off-by: Henry Wang <xin.wang2@amd.com>
---
 tools/libs/guest/xg_dom_arm.c   | 2 ++
 xen/arch/arm/dom0less-build.c   | 2 ++
 xen/arch/arm/hvm.c              | 1 +
 xen/include/public/hvm/params.h | 1 +
 4 files changed, 6 insertions(+)

diff --git a/tools/libs/guest/xg_dom_arm.c b/tools/libs/guest/xg_dom_arm.c
index 8cc7f27dbb..3c08782d1d 100644
--- a/tools/libs/guest/xg_dom_arm.c
+++ b/tools/libs/guest/xg_dom_arm.c
@@ -74,6 +74,8 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
     xc_clear_domain_page(dom->xch, dom->guest_domid, base + MEMACCESS_PFN_OFFSET);
     xc_clear_domain_page(dom->xch, dom->guest_domid, dom->vuart_gfn);
 
+    xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_MAGIC_BASE_PFN,
+            base);
     xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_PFN,
             dom->console_pfn);
     xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_PFN,
diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
index 40dc85c759..72187c167d 100644
--- a/xen/arch/arm/dom0less-build.c
+++ b/xen/arch/arm/dom0less-build.c
@@ -861,6 +861,8 @@ static int __init construct_domU(struct domain *d,
             free_domheap_pages(magic_pg, get_order_from_pages(NR_MAGIC_PAGES));
             return rc;
         }
+
+        d->arch.hvm.params[HVM_PARAM_MAGIC_BASE_PFN] = gfn_x(gfn);
     }
 
     return rc;
diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
index 0989309fea..fa6141e30c 100644
--- a/xen/arch/arm/hvm.c
+++ b/xen/arch/arm/hvm.c
@@ -55,6 +55,7 @@ static int hvm_allow_get_param(const struct domain *d, unsigned int param)
     case HVM_PARAM_STORE_EVTCHN:
     case HVM_PARAM_CONSOLE_PFN:
     case HVM_PARAM_CONSOLE_EVTCHN:
+    case HVM_PARAM_MAGIC_BASE_PFN:
         return 0;
 
         /*
diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
index a22b4ed45d..c1720b33b9 100644
--- a/xen/include/public/hvm/params.h
+++ b/xen/include/public/hvm/params.h
@@ -76,6 +76,7 @@
  */
 #define HVM_PARAM_STORE_PFN    1
 #define HVM_PARAM_STORE_EVTCHN 2
+#define HVM_PARAM_MAGIC_BASE_PFN    3
 
 #define HVM_PARAM_IOREQ_PFN    5
 
-- 
2.34.1

[PATCH 3/3] tools/init-dom0less: Avoid hardcoding GUEST_MAGIC_BASE

[Henry Wang]

Currently the GUEST_MAGIC_BASE in the init-dom0less application is
hardcoded, which will lead to failures for 1:1 direct-mapped Dom0less
DomUs.

Since the guest magic region is now allocated from the hypervisor,
instead of hardcoding the guest magic pages region, use
xc_hvm_param_get() to get the guest magic region PFN, and based on
that the XenStore PFN can be calculated. Also, we don't need to set
the max mem anymore, so drop the call to xc_domain_setmaxmem(). Rename
the alloc_xs_page() to get_xs_page() to reflect the changes.

Take the opportunity to do some coding style improvements when possible.

Reported-by: Alec Kwapis <alec.kwapis@medtronic.com>
Signed-off-by: Henry Wang <xin.wang2@amd.com>
---
 tools/helpers/init-dom0less.c | 38 +++++++++++++++--------------------
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/tools/helpers/init-dom0less.c b/tools/helpers/init-dom0less.c
index fee93459c4..7f6953a818 100644
--- a/tools/helpers/init-dom0less.c
+++ b/tools/helpers/init-dom0less.c
@@ -19,24 +19,20 @@
 #define XENSTORE_PFN_OFFSET 1
 #define STR_MAX_LENGTH 128
 
-static int alloc_xs_page(struct xc_interface_core *xch,
-                         libxl_dominfo *info,
-                         uint64_t *xenstore_pfn)
+static int get_xs_page(struct xc_interface_core *xch, libxl_dominfo *info,
+                       uint64_t *xenstore_pfn)
 {
     int rc;
-    const xen_pfn_t base = GUEST_MAGIC_BASE >> XC_PAGE_SHIFT;
-    xen_pfn_t p2m = (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET;
+    xen_pfn_t magic_base_pfn;
 
-    rc = xc_domain_setmaxmem(xch, info->domid,
-                             info->max_memkb + (XC_PAGE_SIZE/1024));
-    if (rc < 0)
-        return rc;
-
-    rc = xc_domain_populate_physmap_exact(xch, info->domid, 1, 0, 0, &p2m);
-    if (rc < 0)
-        return rc;
+    rc = xc_hvm_param_get(xch, info->domid, HVM_PARAM_MAGIC_BASE_PFN,
+                          &magic_base_pfn);
+    if (rc < 0) {
+        printf("Failed to get HVM_PARAM_MAGIC_BASE_PFN\n");
+        return 1;
+    }
 
-    *xenstore_pfn = base + XENSTORE_PFN_OFFSET;
+    *xenstore_pfn = magic_base_pfn + XENSTORE_PFN_OFFSET;
     rc = xc_clear_domain_page(xch, info->domid, *xenstore_pfn);
     if (rc < 0)
         return rc;
@@ -100,6 +96,7 @@ static bool do_xs_write_vm(struct xs_handle *xsh, xs_transaction_t t,
  */
 static int create_xenstore(struct xs_handle *xsh,
                            libxl_dominfo *info, libxl_uuid uuid,
+                           xen_pfn_t xenstore_pfn,
                            evtchn_port_t xenstore_port)
 {
     domid_t domid;
@@ -145,8 +142,7 @@ static int create_xenstore(struct xs_handle *xsh,
     rc = snprintf(target_memkb_str, STR_MAX_LENGTH, "%"PRIu64, info->current_memkb);
     if (rc < 0 || rc >= STR_MAX_LENGTH)
         return rc;
-    rc = snprintf(ring_ref_str, STR_MAX_LENGTH, "%lld",
-                  (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET);
+    rc = snprintf(ring_ref_str, STR_MAX_LENGTH, "%"PRIu_xen_pfn, xenstore_pfn);
     if (rc < 0 || rc >= STR_MAX_LENGTH)
         return rc;
     rc = snprintf(xenstore_port_str, STR_MAX_LENGTH, "%u", xenstore_port);
@@ -245,8 +241,8 @@ static int init_domain(struct xs_handle *xsh,
     if (!xenstore_evtchn)
         return 0;
 
-    /* Alloc xenstore page */
-    if (alloc_xs_page(xch, info, &xenstore_pfn) != 0) {
+    /* Get xenstore page */
+    if (get_xs_page(xch, info, &xenstore_pfn) != 0) {
         printf("Error on alloc magic pages\n");
         return 1;
     }
@@ -278,13 +274,11 @@ static int init_domain(struct xs_handle *xsh,
     if (rc < 0)
         return rc;
 
-    rc = create_xenstore(xsh, info, uuid, xenstore_evtchn);
+    rc = create_xenstore(xsh, info, uuid, xenstore_pfn, xenstore_evtchn);
     if (rc)
         err(1, "writing to xenstore");
 
-    rc = xs_introduce_domain(xsh, info->domid,
-            (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET,
-            xenstore_evtchn);
+    rc = xs_introduce_domain(xsh, info->domid, xenstore_pfn, xenstore_evtchn);
     if (!rc)
         err(1, "xs_introduce_domain");
     return 0;
-- 
2.34.1

Re: [PATCH 2/3] xen/arm, tools: Add a new HVM_PARAM_MAGIC_BASE_PFN key in HVMOP

[Jan Beulich]

On 26.04.2024 05:14, Henry Wang wrote:
> --- a/xen/include/public/hvm/params.h
> +++ b/xen/include/public/hvm/params.h
> @@ -76,6 +76,7 @@
>   */
>  #define HVM_PARAM_STORE_PFN    1
>  #define HVM_PARAM_STORE_EVTCHN 2
> +#define HVM_PARAM_MAGIC_BASE_PFN    3
>  
>  #define HVM_PARAM_IOREQ_PFN    5

Considering all adjacent values are used, it is overwhelmingly likely that
3 was once used, too. Such re-use needs to be done carefully. Since you
need this for Arm only, that's likely okay, but doesn't go without (a)
saying and (b) considering the possible future case of dom0less becoming
arch-agnostic, or hyperlaunch wanting to extend the scope. Plus (c) imo
this also needs at least a comment, maybe even an #ifdef, seeing how x86-
focused most of the rest of this header is.

Jan

Re: [PATCH 2/3] xen/arm, tools: Add a new HVM_PARAM_MAGIC_BASE_PFN key in HVMOP

[Henry Wang]

Hi Jan,

On 4/26/2024 2:21 PM, Jan Beulich wrote:
> On 26.04.2024 05:14, Henry Wang wrote:
>> --- a/xen/include/public/hvm/params.h
>> +++ b/xen/include/public/hvm/params.h
>> @@ -76,6 +76,7 @@
>>    */
>>   #define HVM_PARAM_STORE_PFN    1
>>   #define HVM_PARAM_STORE_EVTCHN 2
>> +#define HVM_PARAM_MAGIC_BASE_PFN    3
>>   
>>   #define HVM_PARAM_IOREQ_PFN    5
> Considering all adjacent values are used, it is overwhelmingly likely that
> 3 was once used, too. Such re-use needs to be done carefully. Since you
> need this for Arm only, that's likely okay, but doesn't go without (a)
> saying and (b) considering the possible future case of dom0less becoming
> arch-agnostic, or hyperlaunch wanting to extend the scope. Plus (c) imo
> this also needs at least a comment, maybe even an #ifdef, seeing how x86-
> focused most of the rest of this header is.

Thanks for the feedback. These make sense. I think probably 
dom0less/hyperlaunch will have similar use cases so the number 3 can be 
reused at that time. Therefore, in v2, I will add more description in 
commit message, a comment on top of this macro and protect it with 
#ifdef. Hope this will address your concern. Thanks.

Kind regards,
Henry

> Jan

Re: [PATCH 2/3] xen/arm, tools: Add a new HVM_PARAM_MAGIC_BASE_PFN key in HVMOP

[Jan Beulich]

On 26.04.2024 08:30, Henry Wang wrote:
> On 4/26/2024 2:21 PM, Jan Beulich wrote:
>> On 26.04.2024 05:14, Henry Wang wrote:
>>> --- a/xen/include/public/hvm/params.h
>>> +++ b/xen/include/public/hvm/params.h
>>> @@ -76,6 +76,7 @@
>>>    */
>>>   #define HVM_PARAM_STORE_PFN    1
>>>   #define HVM_PARAM_STORE_EVTCHN 2
>>> +#define HVM_PARAM_MAGIC_BASE_PFN    3
>>>   
>>>   #define HVM_PARAM_IOREQ_PFN    5
>> Considering all adjacent values are used, it is overwhelmingly likely that
>> 3 was once used, too. Such re-use needs to be done carefully. Since you
>> need this for Arm only, that's likely okay, but doesn't go without (a)
>> saying and (b) considering the possible future case of dom0less becoming
>> arch-agnostic, or hyperlaunch wanting to extend the scope. Plus (c) imo
>> this also needs at least a comment, maybe even an #ifdef, seeing how x86-
>> focused most of the rest of this header is.
> 
> Thanks for the feedback. These make sense. I think probably 
> dom0less/hyperlaunch will have similar use cases so the number 3 can be 
> reused at that time. Therefore, in v2, I will add more description in 
> commit message, a comment on top of this macro and protect it with 
> #ifdef. Hope this will address your concern. Thanks.

FTAOD: If you foresee re-use by hyperlaunch, re-using a previously used
number may need re-considering. Which isn't to say that number re-use is
excluded here, but it would need at least figuring out (and then stating)
what exactly the number was used for and until when.

Jan

Re: [PATCH 2/3] xen/arm, tools: Add a new HVM_PARAM_MAGIC_BASE_PFN key in HVMOP

[Henry Wang]

Hi Jan,

On 4/26/2024 2:50 PM, Jan Beulich wrote:
> On 26.04.2024 08:30, Henry Wang wrote:
>> On 4/26/2024 2:21 PM, Jan Beulich wrote:
>>> On 26.04.2024 05:14, Henry Wang wrote:
>>>> --- a/xen/include/public/hvm/params.h
>>>> +++ b/xen/include/public/hvm/params.h
>>>> @@ -76,6 +76,7 @@
>>>>     */
>>>>    #define HVM_PARAM_STORE_PFN    1
>>>>    #define HVM_PARAM_STORE_EVTCHN 2
>>>> +#define HVM_PARAM_MAGIC_BASE_PFN    3
>>>>    
>>>>    #define HVM_PARAM_IOREQ_PFN    5
>>> Considering all adjacent values are used, it is overwhelmingly likely that
>>> 3 was once used, too. Such re-use needs to be done carefully. Since you
>>> need this for Arm only, that's likely okay, but doesn't go without (a)
>>> saying and (b) considering the possible future case of dom0less becoming
>>> arch-agnostic, or hyperlaunch wanting to extend the scope. Plus (c) imo
>>> this also needs at least a comment, maybe even an #ifdef, seeing how x86-
>>> focused most of the rest of this header is.
>> Thanks for the feedback. These make sense. I think probably
>> dom0less/hyperlaunch will have similar use cases so the number 3 can be
>> reused at that time. Therefore, in v2, I will add more description in
>> commit message, a comment on top of this macro and protect it with
>> #ifdef. Hope this will address your concern. Thanks.
> FTAOD: If you foresee re-use by hyperlaunch, re-using a previously used
> number may need re-considering. Which isn't to say that number re-use is
> excluded here, but it would need at least figuring out (and then stating)
> what exactly the number was used for and until when.

I just did a bit search and noticed that the number 3 was used to be
#define HVM_PARAM_APIC_ENABLED 3

and it was removed 18 years ago in commit: 
6bc01e4efd50e1986a9391f75980d45691f42b74

So I think we are likely to be ok if reuse 3 on Arm with proper #ifdef.

Kind regards,
Henry

> Jan

Re: [PATCH 1/3] xen/arm/dom0less-build: Alloc magic pages for Dom0less DomUs from hypervisor

[Daniel P. Smith]

On 4/25/24 23:14, Henry Wang wrote:
> There are use cases (for example using the PV driver) in Dom0less
> setup that require Dom0less DomUs start immediately with Dom0, but
> initialize XenStore later after Dom0's successful boot and call to
> the init-dom0less application.
> 
> An error message can seen from the init-dom0less application on
> 1:1 direct-mapped domains:
> ```
> Allocating magic pages
> memory.c:238:d0v0 mfn 0x39000 doesn't belong to d1
> Error on alloc magic pages
> ```
> This is because currently the magic pages for Dom0less DomUs are
> populated by the init-dom0less app through populate_physmap(), and
> populate_physmap() automatically assumes gfn == mfn for 1:1 direct
> mapped domains. This cannot be true for the magic pages that are
> allocated later from the init-dom0less application executed in Dom0.
> For domain using statically allocated memory but not 1:1 direct-mapped,
> similar error "failed to retrieve a reserved page" can be seen as the
> reserved memory list is empty at that time.
> 
> To solve above issue, this commit allocates the magic pages for
> Dom0less DomUs at the domain construction time. The base address/PFN
> of the magic page region will be noted and communicated to the
> init-dom0less application in Dom0.

Might I suggest we not refer to these as magic pages? I would consider 
them as hypervisor reserved pages for the VM to have access to virtual 
platform capabilities. We may see this expand in the future for some 
unforeseen, new capability.

Re: [PATCH 2/3] xen/arm, tools: Add a new HVM_PARAM_MAGIC_BASE_PFN key in HVMOP

[Daniel P. Smith]

On 4/26/24 02:21, Jan Beulich wrote:
> On 26.04.2024 05:14, Henry Wang wrote:
>> --- a/xen/include/public/hvm/params.h
>> +++ b/xen/include/public/hvm/params.h
>> @@ -76,6 +76,7 @@
>>    */
>>   #define HVM_PARAM_STORE_PFN    1
>>   #define HVM_PARAM_STORE_EVTCHN 2
>> +#define HVM_PARAM_MAGIC_BASE_PFN    3
>>   
>>   #define HVM_PARAM_IOREQ_PFN    5
> 
> Considering all adjacent values are used, it is overwhelmingly likely that
> 3 was once used, too. Such re-use needs to be done carefully. Since you
> need this for Arm only, that's likely okay, but doesn't go without (a)
> saying and (b) considering the possible future case of dom0less becoming
> arch-agnostic, or hyperlaunch wanting to extend the scope. Plus (c) imo
> this also needs at least a comment, maybe even an #ifdef, seeing how x86-
> focused most of the rest of this header is.

I would recommend having two new params,

#define HVM_PARAM_HV_RSRV_BASE_PVH 3
#define HVM_PARAM_HV_RSRV_SIZE 4

This will communicate how many pages have been reserved and where those 
pages are located.

v/r,
dps

Re: [PATCH 2/3] xen/arm, tools: Add a new HVM_PARAM_MAGIC_BASE_PFN key in HVMOP

[Daniel P. Smith]

On 4/25/24 23:14, Henry Wang wrote:
> For use cases such as Dom0less PV drivers, a mechanism to communicate
> Dom0less DomU's static data with the runtime control plane (Dom0) is
> needed. Since on Arm HVMOP is already the existing approach to address
> such use cases (for example the allocation of HVM_PARAM_CALLBACK_IRQ),
> add a new HVMOP key HVM_PARAM_MAGIC_BASE_PFN for storing the magic
> page region base PFN. The value will be set at Dom0less DomU
> construction time after Dom0less DomU's magic page region has been
> allocated.
> 
> To keep consistent, also set the value for HVM_PARAM_MAGIC_BASE_PFN
> for libxl guests in alloc_magic_pages().
> 
> Reported-by: Alec Kwapis <alec.kwapis@medtronic.com>
> Signed-off-by: Henry Wang <xin.wang2@amd.com>
> ---
>   tools/libs/guest/xg_dom_arm.c   | 2 ++
>   xen/arch/arm/dom0less-build.c   | 2 ++
>   xen/arch/arm/hvm.c              | 1 +
>   xen/include/public/hvm/params.h | 1 +
>   4 files changed, 6 insertions(+)
> 
> diff --git a/tools/libs/guest/xg_dom_arm.c b/tools/libs/guest/xg_dom_arm.c
> index 8cc7f27dbb..3c08782d1d 100644
> --- a/tools/libs/guest/xg_dom_arm.c
> +++ b/tools/libs/guest/xg_dom_arm.c
> @@ -74,6 +74,8 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
>       xc_clear_domain_page(dom->xch, dom->guest_domid, base + MEMACCESS_PFN_OFFSET);
>       xc_clear_domain_page(dom->xch, dom->guest_domid, dom->vuart_gfn);
>   
> +    xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_MAGIC_BASE_PFN,
> +            base);
>       xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_PFN,
>               dom->console_pfn);
>       xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_PFN,
> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> index 40dc85c759..72187c167d 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -861,6 +861,8 @@ static int __init construct_domU(struct domain *d,
>               free_domheap_pages(magic_pg, get_order_from_pages(NR_MAGIC_PAGES));
>               return rc;
>           }
> +
> +        d->arch.hvm.params[HVM_PARAM_MAGIC_BASE_PFN] = gfn_x(gfn);
>       }
>   
>       return rc;
> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
> index 0989309fea..fa6141e30c 100644
> --- a/xen/arch/arm/hvm.c
> +++ b/xen/arch/arm/hvm.c
> @@ -55,6 +55,7 @@ static int hvm_allow_get_param(const struct domain *d, unsigned int param)
>       case HVM_PARAM_STORE_EVTCHN:
>       case HVM_PARAM_CONSOLE_PFN:
>       case HVM_PARAM_CONSOLE_EVTCHN:
> +    case HVM_PARAM_MAGIC_BASE_PFN:
>           return 0;

I know you are just adding, so more of a question for the Arm maintainers:

Why does Arm have a pair of private access control functions subverting XSM?

v/r,
dps

Re: [PATCH 2/3] xen/arm, tools: Add a new HVM_PARAM_MAGIC_BASE_PFN key in HVMOP

[Daniel P. Smith]

On 4/29/24 20:35, Daniel P. Smith wrote:
> On 4/25/24 23:14, Henry Wang wrote:
>> For use cases such as Dom0less PV drivers, a mechanism to communicate
>> Dom0less DomU's static data with the runtime control plane (Dom0) is
>> needed. Since on Arm HVMOP is already the existing approach to address
>> such use cases (for example the allocation of HVM_PARAM_CALLBACK_IRQ),
>> add a new HVMOP key HVM_PARAM_MAGIC_BASE_PFN for storing the magic
>> page region base PFN. The value will be set at Dom0less DomU
>> construction time after Dom0less DomU's magic page region has been
>> allocated.
>>
>> To keep consistent, also set the value for HVM_PARAM_MAGIC_BASE_PFN
>> for libxl guests in alloc_magic_pages().
>>
>> Reported-by: Alec Kwapis <alec.kwapis@medtronic.com>
>> Signed-off-by: Henry Wang <xin.wang2@amd.com>
>> ---
>>   tools/libs/guest/xg_dom_arm.c   | 2 ++
>>   xen/arch/arm/dom0less-build.c   | 2 ++
>>   xen/arch/arm/hvm.c              | 1 +
>>   xen/include/public/hvm/params.h | 1 +
>>   4 files changed, 6 insertions(+)
>>
>> diff --git a/tools/libs/guest/xg_dom_arm.c 
>> b/tools/libs/guest/xg_dom_arm.c
>> index 8cc7f27dbb..3c08782d1d 100644
>> --- a/tools/libs/guest/xg_dom_arm.c
>> +++ b/tools/libs/guest/xg_dom_arm.c
>> @@ -74,6 +74,8 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
>>       xc_clear_domain_page(dom->xch, dom->guest_domid, base + 
>> MEMACCESS_PFN_OFFSET);
>>       xc_clear_domain_page(dom->xch, dom->guest_domid, dom->vuart_gfn);
>> +    xc_hvm_param_set(dom->xch, dom->guest_domid, 
>> HVM_PARAM_MAGIC_BASE_PFN,
>> +            base);
>>       xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_PFN,
>>               dom->console_pfn);
>>       xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_PFN,
>> diff --git a/xen/arch/arm/dom0less-build.c 
>> b/xen/arch/arm/dom0less-build.c
>> index 40dc85c759..72187c167d 100644
>> --- a/xen/arch/arm/dom0less-build.c
>> +++ b/xen/arch/arm/dom0less-build.c
>> @@ -861,6 +861,8 @@ static int __init construct_domU(struct domain *d,
>>               free_domheap_pages(magic_pg, 
>> get_order_from_pages(NR_MAGIC_PAGES));
>>               return rc;
>>           }
>> +
>> +        d->arch.hvm.params[HVM_PARAM_MAGIC_BASE_PFN] = gfn_x(gfn);
>>       }
>>       return rc;
>> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
>> index 0989309fea..fa6141e30c 100644
>> --- a/xen/arch/arm/hvm.c
>> +++ b/xen/arch/arm/hvm.c
>> @@ -55,6 +55,7 @@ static int hvm_allow_get_param(const struct domain 
>> *d, unsigned int param)
>>       case HVM_PARAM_STORE_EVTCHN:
>>       case HVM_PARAM_CONSOLE_PFN:
>>       case HVM_PARAM_CONSOLE_EVTCHN:
>> +    case HVM_PARAM_MAGIC_BASE_PFN:
>>           return 0;
> 
> I know you are just adding, so more of a question for the Arm maintainers:
> 
> Why does Arm have a pair of private access control functions subverting 
> XSM?

On closer look, I see x86 has done the same. While the way this is set 
up bothers me, reviewing the history it was clearly decided that only 
controlling use of the op by a src domain against a target domain was 
sufficient. Ultimately, it is seeing a mini access control policy being 
codified in the access code is what is bothering me here. Fixing this 
would require a complete rework of xsm_hvm_param, and while it would 
correct the access control from a purist perspective, the security 
benefit would be very low.

v/r,
dps

Re: [PATCH 2/3] xen/arm, tools: Add a new HVM_PARAM_MAGIC_BASE_PFN key in HVMOP

[Henry Wang]

Hi Daniel,

On 4/30/2024 8:31 AM, Daniel P. Smith wrote:
>
> On 4/26/24 02:21, Jan Beulich wrote:
>> On 26.04.2024 05:14, Henry Wang wrote:
>>> --- a/xen/include/public/hvm/params.h
>>> +++ b/xen/include/public/hvm/params.h
>>> @@ -76,6 +76,7 @@
>>>    */
>>>   #define HVM_PARAM_STORE_PFN    1
>>>   #define HVM_PARAM_STORE_EVTCHN 2
>>> +#define HVM_PARAM_MAGIC_BASE_PFN    3
>>>     #define HVM_PARAM_IOREQ_PFN    5
>>
>> Considering all adjacent values are used, it is overwhelmingly likely 
>> that
>> 3 was once used, too. Such re-use needs to be done carefully. Since you
>> need this for Arm only, that's likely okay, but doesn't go without (a)
>> saying and (b) considering the possible future case of dom0less becoming
>> arch-agnostic, or hyperlaunch wanting to extend the scope. Plus (c) imo
>> this also needs at least a comment, maybe even an #ifdef, seeing how 
>> x86-
>> focused most of the rest of this header is.
>
> I would recommend having two new params,

Sounds good. I can do the suggestion in v2.

>
> #define HVM_PARAM_HV_RSRV_BASE_PVH 3
> #define HVM_PARAM_HV_RSRV_SIZE 4

I think 4 is currently in use, so I think I will find another couple of 
numbers in the end for both of them. Instead of reusing 3 and 4.

Kind regards,
Henry

>
> This will communicate how many pages have been reserved and where 
> those pages are located.
>
> v/r,
> dps

Re: [PATCH 1/3] xen/arm/dom0less-build: Alloc magic pages for Dom0less DomUs from hypervisor

[Henry Wang]

Hi Daniel,

On 4/30/2024 8:27 AM, Daniel P. Smith wrote:
> On 4/25/24 23:14, Henry Wang wrote:
>> There are use cases (for example using the PV driver) in Dom0less
>> setup that require Dom0less DomUs start immediately with Dom0, but
>> initialize XenStore later after Dom0's successful boot and call to
>> the init-dom0less application.
>>
>> An error message can seen from the init-dom0less application on
>> 1:1 direct-mapped domains:
>> ```
>> Allocating magic pages
>> memory.c:238:d0v0 mfn 0x39000 doesn't belong to d1
>> Error on alloc magic pages
>> ```
>> This is because currently the magic pages for Dom0less DomUs are
>> populated by the init-dom0less app through populate_physmap(), and
>> populate_physmap() automatically assumes gfn == mfn for 1:1 direct
>> mapped domains. This cannot be true for the magic pages that are
>> allocated later from the init-dom0less application executed in Dom0.
>> For domain using statically allocated memory but not 1:1 direct-mapped,
>> similar error "failed to retrieve a reserved page" can be seen as the
>> reserved memory list is empty at that time.
>>
>> To solve above issue, this commit allocates the magic pages for
>> Dom0less DomUs at the domain construction time. The base address/PFN
>> of the magic page region will be noted and communicated to the
>> init-dom0less application in Dom0.
>
> Might I suggest we not refer to these as magic pages? I would consider 
> them as hypervisor reserved pages for the VM to have access to virtual 
> platform capabilities. We may see this expand in the future for some 
> unforeseen, new capability.

I think magic page is a specific terminology to refer to these pages, 
see alloc_magic_pages() for both x86 and Arm. I will reword the last 
paragraph of the commit message to refer them as "hypervisor reserved 
pages (currently used as magic pages on Arm)" if this sounds good to you.

Kind regards,
Henry

Re: [PATCH 2/3] xen/arm, tools: Add a new HVM_PARAM_MAGIC_BASE_PFN key in HVMOP

[Jan Beulich]

On 30.04.2024 04:51, Henry Wang wrote:
> On 4/30/2024 8:31 AM, Daniel P. Smith wrote:
>> On 4/26/24 02:21, Jan Beulich wrote:
>>> On 26.04.2024 05:14, Henry Wang wrote:
>>>> --- a/xen/include/public/hvm/params.h
>>>> +++ b/xen/include/public/hvm/params.h
>>>> @@ -76,6 +76,7 @@
>>>>    */
>>>>   #define HVM_PARAM_STORE_PFN    1
>>>>   #define HVM_PARAM_STORE_EVTCHN 2
>>>> +#define HVM_PARAM_MAGIC_BASE_PFN    3
>>>>     #define HVM_PARAM_IOREQ_PFN    5
>>>
>>> Considering all adjacent values are used, it is overwhelmingly likely 
>>> that
>>> 3 was once used, too. Such re-use needs to be done carefully. Since you
>>> need this for Arm only, that's likely okay, but doesn't go without (a)
>>> saying and (b) considering the possible future case of dom0less becoming
>>> arch-agnostic, or hyperlaunch wanting to extend the scope. Plus (c) imo
>>> this also needs at least a comment, maybe even an #ifdef, seeing how 
>>> x86-
>>> focused most of the rest of this header is.
>>
>> I would recommend having two new params,
> 
> Sounds good. I can do the suggestion in v2.
> 
>>
>> #define HVM_PARAM_HV_RSRV_BASE_PVH 3
>> #define HVM_PARAM_HV_RSRV_SIZE 4
> 
> I think 4 is currently in use, so I think I will find another couple of 
> numbers in the end for both of them. Instead of reusing 3 and 4.

Right. There are ample gaps, but any use of values within a gap will need
appropriate care. FTAOD using such a gap looks indeed preferable, to avoid
further growing the (sparse) array. Alternatively, if we're firm on this
never going to be used on x86, some clearly x86-specific indexes (e.g. 36
and 37) could be given non-x86 purpose.

Jan

Re: [PATCH 2/3] xen/arm, tools: Add a new HVM_PARAM_MAGIC_BASE_PFN key in HVMOP

[Henry Wang]

Hi Jan,

On 4/30/2024 2:11 PM, Jan Beulich wrote:
> On 30.04.2024 04:51, Henry Wang wrote:
>> On 4/30/2024 8:31 AM, Daniel P. Smith wrote:
>>> On 4/26/24 02:21, Jan Beulich wrote:
>>>> On 26.04.2024 05:14, Henry Wang wrote:
>>>>> --- a/xen/include/public/hvm/params.h
>>>>> +++ b/xen/include/public/hvm/params.h
>>>>> @@ -76,6 +76,7 @@
>>>>>     */
>>>>>    #define HVM_PARAM_STORE_PFN    1
>>>>>    #define HVM_PARAM_STORE_EVTCHN 2
>>>>> +#define HVM_PARAM_MAGIC_BASE_PFN    3
>>>>>      #define HVM_PARAM_IOREQ_PFN    5
>>>> Considering all adjacent values are used, it is overwhelmingly likely
>>>> that
>>>> 3 was once used, too. Such re-use needs to be done carefully. Since you
>>>> need this for Arm only, that's likely okay, but doesn't go without (a)
>>>> saying and (b) considering the possible future case of dom0less becoming
>>>> arch-agnostic, or hyperlaunch wanting to extend the scope. Plus (c) imo
>>>> this also needs at least a comment, maybe even an #ifdef, seeing how
>>>> x86-
>>>> focused most of the rest of this header is.
>>> I would recommend having two new params,
>> Sounds good. I can do the suggestion in v2.
>>
>>> #define HVM_PARAM_HV_RSRV_BASE_PVH 3
>>> #define HVM_PARAM_HV_RSRV_SIZE 4
>> I think 4 is currently in use, so I think I will find another couple of
>> numbers in the end for both of them. Instead of reusing 3 and 4.
> Right. There are ample gaps, but any use of values within a gap will need
> appropriate care. FTAOD using such a gap looks indeed preferable, to avoid
> further growing the (sparse) array. Alternatively, if we're firm on this
> never going to be used on x86, some clearly x86-specific indexes (e.g. 36
> and 37) could be given non-x86 purpose.

Sorry, I am a bit confused. I take Daniel's comment as to add two new 
params, which is currently only used for Arm, but eventually will be 
used for hyperlaunch on x86 (as the name indicated). So I think I will 
use the name that he suggested, but the number changed to 39 and 40.

Kind regards,
Henry

>
> Jan

Re: [PATCH 2/3] xen/arm, tools: Add a new HVM_PARAM_MAGIC_BASE_PFN key in HVMOP

[Jan Beulich]

On 30.04.2024 10:12, Henry Wang wrote:
> Hi Jan,
> 
> On 4/30/2024 2:11 PM, Jan Beulich wrote:
>> On 30.04.2024 04:51, Henry Wang wrote:
>>> On 4/30/2024 8:31 AM, Daniel P. Smith wrote:
>>>> On 4/26/24 02:21, Jan Beulich wrote:
>>>>> On 26.04.2024 05:14, Henry Wang wrote:
>>>>>> --- a/xen/include/public/hvm/params.h
>>>>>> +++ b/xen/include/public/hvm/params.h
>>>>>> @@ -76,6 +76,7 @@
>>>>>>     */
>>>>>>    #define HVM_PARAM_STORE_PFN    1
>>>>>>    #define HVM_PARAM_STORE_EVTCHN 2
>>>>>> +#define HVM_PARAM_MAGIC_BASE_PFN    3
>>>>>>      #define HVM_PARAM_IOREQ_PFN    5
>>>>> Considering all adjacent values are used, it is overwhelmingly likely
>>>>> that
>>>>> 3 was once used, too. Such re-use needs to be done carefully. Since you
>>>>> need this for Arm only, that's likely okay, but doesn't go without (a)
>>>>> saying and (b) considering the possible future case of dom0less becoming
>>>>> arch-agnostic, or hyperlaunch wanting to extend the scope. Plus (c) imo
>>>>> this also needs at least a comment, maybe even an #ifdef, seeing how
>>>>> x86-
>>>>> focused most of the rest of this header is.
>>>> I would recommend having two new params,
>>> Sounds good. I can do the suggestion in v2.
>>>
>>>> #define HVM_PARAM_HV_RSRV_BASE_PVH 3
>>>> #define HVM_PARAM_HV_RSRV_SIZE 4
>>> I think 4 is currently in use, so I think I will find another couple of
>>> numbers in the end for both of them. Instead of reusing 3 and 4.
>> Right. There are ample gaps, but any use of values within a gap will need
>> appropriate care. FTAOD using such a gap looks indeed preferable, to avoid
>> further growing the (sparse) array. Alternatively, if we're firm on this
>> never going to be used on x86, some clearly x86-specific indexes (e.g. 36
>> and 37) could be given non-x86 purpose.
> 
> Sorry, I am a bit confused. I take Daniel's comment as to add two new 
> params, which is currently only used for Arm, but eventually will be 
> used for hyperlaunch on x86 (as the name indicated). So I think I will 
> use the name that he suggested, but the number changed to 39 and 40.

Well, yes, if re-use for x86 is intended, then unused slots need taking.
Question then still is whether the array bounds indeed need moving up,
or whether instead one of the gaps can be (re)used.

Jan

Re: [PATCH 1/3] xen/arm/dom0less-build: Alloc magic pages for Dom0less DomUs from hypervisor

[Daniel P. Smith]

On 4/29/24 22:55, Henry Wang wrote:
> Hi Daniel,
> 
> On 4/30/2024 8:27 AM, Daniel P. Smith wrote:
>> On 4/25/24 23:14, Henry Wang wrote:
>>> There are use cases (for example using the PV driver) in Dom0less
>>> setup that require Dom0less DomUs start immediately with Dom0, but
>>> initialize XenStore later after Dom0's successful boot and call to
>>> the init-dom0less application.
>>>
>>> An error message can seen from the init-dom0less application on
>>> 1:1 direct-mapped domains:
>>> ```
>>> Allocating magic pages
>>> memory.c:238:d0v0 mfn 0x39000 doesn't belong to d1
>>> Error on alloc magic pages
>>> ```
>>> This is because currently the magic pages for Dom0less DomUs are
>>> populated by the init-dom0less app through populate_physmap(), and
>>> populate_physmap() automatically assumes gfn == mfn for 1:1 direct
>>> mapped domains. This cannot be true for the magic pages that are
>>> allocated later from the init-dom0less application executed in Dom0.
>>> For domain using statically allocated memory but not 1:1 direct-mapped,
>>> similar error "failed to retrieve a reserved page" can be seen as the
>>> reserved memory list is empty at that time.
>>>
>>> To solve above issue, this commit allocates the magic pages for
>>> Dom0less DomUs at the domain construction time. The base address/PFN
>>> of the magic page region will be noted and communicated to the
>>> init-dom0less application in Dom0.
>>
>> Might I suggest we not refer to these as magic pages? I would consider 
>> them as hypervisor reserved pages for the VM to have access to virtual 
>> platform capabilities. We may see this expand in the future for some 
>> unforeseen, new capability.
> 
> I think magic page is a specific terminology to refer to these pages, 
> see alloc_magic_pages() for both x86 and Arm. I will reword the last 
> paragraph of the commit message to refer them as "hypervisor reserved 
> pages (currently used as magic pages on Arm)" if this sounds good to you.

I would highlight that is a term used in the toolstack, while is 
probably not the best, there is no reason to change in there, but the 
hypervisor does not carry that terminology. IMHO we should not introduce 
it there and be explicit about why the pages are getting reserved.

v/r,
dps

Re: [PATCH 2/3] xen/arm, tools: Add a new HVM_PARAM_MAGIC_BASE_PFN key in HVMOP

[Stefano Stabellini]

On Fri, 26 Apr 2024, Henry Wang wrote:
> For use cases such as Dom0less PV drivers, a mechanism to communicate
> Dom0less DomU's static data with the runtime control plane (Dom0) is
> needed. Since on Arm HVMOP is already the existing approach to address
> such use cases (for example the allocation of HVM_PARAM_CALLBACK_IRQ),
> add a new HVMOP key HVM_PARAM_MAGIC_BASE_PFN for storing the magic
> page region base PFN. The value will be set at Dom0less DomU
> construction time after Dom0less DomU's magic page region has been
> allocated.
> 
> To keep consistent, also set the value for HVM_PARAM_MAGIC_BASE_PFN
> for libxl guests in alloc_magic_pages().
> 
> Reported-by: Alec Kwapis <alec.kwapis@medtronic.com>
> Signed-off-by: Henry Wang <xin.wang2@amd.com>
> ---
>  tools/libs/guest/xg_dom_arm.c   | 2 ++
>  xen/arch/arm/dom0less-build.c   | 2 ++
>  xen/arch/arm/hvm.c              | 1 +
>  xen/include/public/hvm/params.h | 1 +
>  4 files changed, 6 insertions(+)
> 
> diff --git a/tools/libs/guest/xg_dom_arm.c b/tools/libs/guest/xg_dom_arm.c
> index 8cc7f27dbb..3c08782d1d 100644
> --- a/tools/libs/guest/xg_dom_arm.c
> +++ b/tools/libs/guest/xg_dom_arm.c
> @@ -74,6 +74,8 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
>      xc_clear_domain_page(dom->xch, dom->guest_domid, base + MEMACCESS_PFN_OFFSET);
>      xc_clear_domain_page(dom->xch, dom->guest_domid, dom->vuart_gfn);
>  
> +    xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_MAGIC_BASE_PFN,
> +            base);
>      xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_PFN,
>              dom->console_pfn);
>      xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_PFN,
> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> index 40dc85c759..72187c167d 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -861,6 +861,8 @@ static int __init construct_domU(struct domain *d,
>              free_domheap_pages(magic_pg, get_order_from_pages(NR_MAGIC_PAGES));
>              return rc;
>          }
> +
> +        d->arch.hvm.params[HVM_PARAM_MAGIC_BASE_PFN] = gfn_x(gfn);

I apologize as I have not read the whole email thread in reply to this
patch.

Why do we need to introduce a new hvm param instead of just setting
HVM_PARAM_CONSOLE_PFN and HVM_PARAM_STORE_PFN directly here?

Re: [PATCH 2/3] xen/arm, tools: Add a new HVM_PARAM_MAGIC_BASE_PFN key in HVMOP

[Henry Wang]

Hi Stefano,

On 5/3/2024 2:08 AM, Stefano Stabellini wrote:
> On Fri, 26 Apr 2024, Henry Wang wrote:
>> For use cases such as Dom0less PV drivers, a mechanism to communicate
>> Dom0less DomU's static data with the runtime control plane (Dom0) is
>> needed. Since on Arm HVMOP is already the existing approach to address
>> such use cases (for example the allocation of HVM_PARAM_CALLBACK_IRQ),
>> add a new HVMOP key HVM_PARAM_MAGIC_BASE_PFN for storing the magic
>> page region base PFN. The value will be set at Dom0less DomU
>> construction time after Dom0less DomU's magic page region has been
>> allocated.
>>
>> To keep consistent, also set the value for HVM_PARAM_MAGIC_BASE_PFN
>> for libxl guests in alloc_magic_pages().
>>
>> Reported-by: Alec Kwapis <alec.kwapis@medtronic.com>
>> Signed-off-by: Henry Wang <xin.wang2@amd.com>
>> ---
>>   tools/libs/guest/xg_dom_arm.c   | 2 ++
>>   xen/arch/arm/dom0less-build.c   | 2 ++
>>   xen/arch/arm/hvm.c              | 1 +
>>   xen/include/public/hvm/params.h | 1 +
>>   4 files changed, 6 insertions(+)
>>
>> diff --git a/tools/libs/guest/xg_dom_arm.c b/tools/libs/guest/xg_dom_arm.c
>> index 8cc7f27dbb..3c08782d1d 100644
>> --- a/tools/libs/guest/xg_dom_arm.c
>> +++ b/tools/libs/guest/xg_dom_arm.c
>> @@ -74,6 +74,8 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
>>       xc_clear_domain_page(dom->xch, dom->guest_domid, base + MEMACCESS_PFN_OFFSET);
>>       xc_clear_domain_page(dom->xch, dom->guest_domid, dom->vuart_gfn);
>>   
>> +    xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_MAGIC_BASE_PFN,
>> +            base);
>>       xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_PFN,
>>               dom->console_pfn);
>>       xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_PFN,
>> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
>> index 40dc85c759..72187c167d 100644
>> --- a/xen/arch/arm/dom0less-build.c
>> +++ b/xen/arch/arm/dom0less-build.c
>> @@ -861,6 +861,8 @@ static int __init construct_domU(struct domain *d,
>>               free_domheap_pages(magic_pg, get_order_from_pages(NR_MAGIC_PAGES));
>>               return rc;
>>           }
>> +
>> +        d->arch.hvm.params[HVM_PARAM_MAGIC_BASE_PFN] = gfn_x(gfn);
> I apologize as I have not read the whole email thread in reply to this
> patch.
>
> Why do we need to introduce a new hvm param instead of just setting
> HVM_PARAM_CONSOLE_PFN and HVM_PARAM_STORE_PFN directly here?
>

Yeah this is a good question, I aIso thought about this but in the end 
didn't do that directly because I don't really want to break the current 
protocol between Linux, Xen and toolstack.
In docs/features/dom0less.pandoc, section "PV Drivers", there is a 
communication protocol saying that Xen should keep the 
HVM_PARAM_STORE_PFN to ~0ULL until the toolstack sets the 
HVM_PARAM_STORE_PFN.

I am open to change the protocol (changes might be needed in the Linux 
side too), if it is ok to do that, I can set the HVM params here 
directly and change the doc accordingly.

Kind regards,
Henry

Re: [PATCH 1/3] xen/arm/dom0less-build: Alloc magic pages for Dom0less DomUs from hypervisor

[Henry Wang]

Hi Daniel,

On 4/30/2024 6:22 PM, Daniel P. Smith wrote:
> On 4/29/24 22:55, Henry Wang wrote:
>> Hi Daniel,
>>
>> On 4/30/2024 8:27 AM, Daniel P. Smith wrote:
>>> On 4/25/24 23:14, Henry Wang wrote:
>>>> There are use cases (for example using the PV driver) in Dom0less
>>>> setup that require Dom0less DomUs start immediately with Dom0, but
>>>> initialize XenStore later after Dom0's successful boot and call to
>>>> the init-dom0less application.
>>>>
>>>> An error message can seen from the init-dom0less application on
>>>> 1:1 direct-mapped domains:
>>>> ```
>>>> Allocating magic pages
>>>> memory.c:238:d0v0 mfn 0x39000 doesn't belong to d1
>>>> Error on alloc magic pages
>>>> ```
>>>> This is because currently the magic pages for Dom0less DomUs are
>>>> populated by the init-dom0less app through populate_physmap(), and
>>>> populate_physmap() automatically assumes gfn == mfn for 1:1 direct
>>>> mapped domains. This cannot be true for the magic pages that are
>>>> allocated later from the init-dom0less application executed in Dom0.
>>>> For domain using statically allocated memory but not 1:1 
>>>> direct-mapped,
>>>> similar error "failed to retrieve a reserved page" can be seen as the
>>>> reserved memory list is empty at that time.
>>>>
>>>> To solve above issue, this commit allocates the magic pages for
>>>> Dom0less DomUs at the domain construction time. The base address/PFN
>>>> of the magic page region will be noted and communicated to the
>>>> init-dom0less application in Dom0.
>>>
>>> Might I suggest we not refer to these as magic pages? I would 
>>> consider them as hypervisor reserved pages for the VM to have access 
>>> to virtual platform capabilities. We may see this expand in the 
>>> future for some unforeseen, new capability.
>>
>> I think magic page is a specific terminology to refer to these pages, 
>> see alloc_magic_pages() for both x86 and Arm. I will reword the last 
>> paragraph of the commit message to refer them as "hypervisor reserved 
>> pages (currently used as magic pages on Arm)" if this sounds good to 
>> you.
>
> I would highlight that is a term used in the toolstack, while is 
> probably not the best, there is no reason to change in there, but the 
> hypervisor does not carry that terminology. IMHO we should not 
> introduce it there and be explicit about why the pages are getting 
> reserved.

Thanks for the suggestion. I will rework the commit message.

Kind regards,
Henry

>
> v/r,
> dps
>