Re: [PATCH v2 2/4] xen/arm: Add new HVM_PARAM_HV_RSRV_{BASE_PFN,SIZE} keys in HVMOP

[Julien Grall <julien@xxxxxxx>]

Hi Henry,

On 11/05/2024 01:56, 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 new HVMOP keys HVM_PARAM_HV_RSRV_{BASE_PFN,SIZE} for storing the
> hypervisor reserved pages region base PFN and size.
>
> Currently, the hypervisor reserved pages region is used as the Arm
> Dom0less DomU guest magic pages region. Therefore protect the HVMOP
> keys with "#if defined(__arm__) || defined(__aarch64__)". The values
> will be set at Dom0less DomU construction time after Dom0less DomU's
> magic pages region has been allocated.
>
> Reported-by: Alec Kwapis <alec.kwapis@xxxxxxxxxxxxx>
> Signed-off-by: Henry Wang <xin.wang2@xxxxxxx>
> ---
> v2:
> - Rename the HVMOP keys to HVM_PARAM_HV_RSRV_{BASE_PFN,SIZE}. (Daniel)
> - Add comment on top of HVM_PARAM_HV_RSRV_{BASE_PFN,SIZE} to describe
>    its usage. Protect them with #ifdef. (Daniel, Jan)
> ---
>   xen/arch/arm/dom0less-build.c   |  3 +++
>   xen/arch/arm/hvm.c              |  2 ++
>   xen/include/public/hvm/params.h | 11 ++++++++++-
>   3 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> index 4b96ddd9ce..5bb53ebb47 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -764,6 +764,9 @@ static int __init alloc_magic_pages(struct domain *d)
>           return rc;
>       }
>   
> +    d->arch.hvm.params[HVM_PARAM_HV_RSRV_BASE_PFN] = gfn_x(gfn);
> +    d->arch.hvm.params[HVM_PARAM_HV_RSRV_SIZE] = NR_MAGIC_PAGES;
> +
>       return 0;
>   }
>   
> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
> index 0989309fea..949d804f8b 100644
> --- a/xen/arch/arm/hvm.c
> +++ b/xen/arch/arm/hvm.c
> @@ -55,6 +55,8 @@ 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_HV_RSRV_BASE_PFN:
> +    case HVM_PARAM_HV_RSRV_SIZE:
We should not allow the guest to read HVM_PARAM_HV_*. So this would need 
to be handled like HVM_PARAM_HV_*.
But I have some reservation with using HVM params. It means we are 
setting in stone how we communicate/allocate the reserved pages.
I can see a few issues that may bite back in the future.

We always pre-allocate the reserved pages. But most of the feature may 
not be used which ends up to be a waste of memory. For instance, you 
only seem to one page for dom0less, but allocate 4. I understand that 
libxenguest is doing the same today, however we have the flexibility to 
change it by just not allocating the page.
Similarly, we expect the hypervisor to provide enough reserved pages for 
the toolstack to work. Yet, I believe some of the features (e.g. 
memaccess) doesn't need to be known in advance.
The number of pages are tiny at the moment (4) on Xen. But given the 
name, I could see someone asking for setting aside more reserved spaces 
and a single range may start to be a problem for direct mapped domain 
(unless we don't need to map the magic pages 1:1).
Overall, if we want to make it part of the stable ABI. Then it would be 
better to provide a (large) reserved space that will be allocated on 
demand (IOW there is no associated physical memory). If it doesn't work 
for direct mapped domain, then another solution is to have a toolstack 
hypercall to allocate a GFN (they don't need to be contiguous) and the 
backing memory.
Cheers,

--
Julien Grall