Re: [PATCH v2 1/6] xen: introduce XEN_DOMCTL_CDF_directmap

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.10.2021 05:09, Penny Zheng wrote:
> From: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> 
> This commit introduces a new arm-specific flag XEN_DOMCTL_CDF_directmap to
> specify that this domain should have its memory directly mapped
> (guest physical address == physical address) at domain creation.
> 
> Refine is_domain_direct_mapped to check whether the flag
> XEN_DOMCTL_CDF_directmap is set.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> ---
> CC: andrew.cooper3@xxxxxxxxxx
> CC: jbeulich@xxxxxxxx
> CC: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> CC: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> CC: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> ---

Please have here a brief log of changes in the new version, to aid
reviewers.

>  xen/arch/arm/domain.c        | 3 ++-
>  xen/arch/arm/domain_build.c  | 4 +++-
>  xen/common/domain.c          | 3 ++-
>  xen/include/asm-arm/domain.h | 4 ++--
>  xen/include/public/domctl.h  | 4 +++-
>  5 files changed, 12 insertions(+), 6 deletions(-)

You clearly had to re-base over the XEN_DOMCTL_CDF_vpmu addition. I think
just like that change (which I'd expect you to have looked at while doing
the re-base) you also need to at least fiddle with OCaml's
domain_create_flag, to keep the ABI check there happy.

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -72,9 +72,11 @@ struct xen_domctl_createdomain {
>  #define XEN_DOMCTL_CDF_nested_virt    (1U << _XEN_DOMCTL_CDF_nested_virt)
>  /* Should we expose the vPMU to the guest? */
>  #define XEN_DOMCTL_CDF_vpmu           (1U << 7)
> +/* If this domain has its memory directly mapped? (ARM only) */
> +#define XEN_DOMCTL_CDF_directmap      (1U << 8)

The comment doesn't read well; how about "Should domain memory be directly
mapped?" That's if a comment here is really needed in the first place. I
also don't think "Arm only" should be here - this may go stale. What I'm
missing in this regard is rejecting of the flag in x86'es
arch_sanitise_domain_config() (or by whichever other means).

Jan