RE: [PATCH v3 01/10] xen: introduce XEN_DOMCTL_CDF_INTERNAL_directmap

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi guys

> -----Original Message-----
> From: Penny Zheng <penny.zheng@xxxxxxx>
> Sent: Tuesday, November 16, 2021 1:25 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>
> Cc: nd <nd@xxxxxxx>
> Subject: [PATCH v3 01/10] xen: introduce
> XEN_DOMCTL_CDF_INTERNAL_directmap
> 
> From: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> 
> This commit introduces a new arm-specific flag
> XEN_DOMCTL_CDF_INTERNAL_directmap to specify that a domain should
> have its memory direct-map(guest physical address == physical address) at
> domain creation.
> 
> Since this flag is only available for domain created by XEN, not exposed to 
> the
> toolstack, we name it with extra "INTERNAL" to distinguish from other public
> XEN_DOMCTL_CDF_xxx flags, and add comments to warn developers not to
> accidently use its bitfield when introducing new XEN_DOMCTL_CDF_xxx flag.
> 
> Refine is_domain_direct_mapped to check whether the flag
> XEN_DOMCTL_CDF_INTERNAL_directmap is set.
> 
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> ---
> 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>
> ---
> v2 changes
> - remove the introduce of internal flag
> - remove flag direct_map since we already store this flag in d->options
> - Refine is_domain_direct_mapped to check whether the flag
> XEN_DOMCTL_CDF_directmap is set
> - reword "1:1 direct-map" to just "direct-map"
> ---
> v3 changes
> - move flag back to xen/include/xen/domain.h, to let it be only available for
> domain created by XEN.
> - name it with extra "INTERNAL" and add comments to warn developers not to
> accidently use its bitfield when introducing new XEN_DOMCTL_CDF_xxx flag.
> - reject this flag in x86'es arch_sanitise_domain_config()
> ---
>  xen/arch/arm/domain.c        | 3 ++-
>  xen/arch/arm/domain_build.c  | 4 +++-
>  xen/arch/x86/domain.c        | 6 ++++++
>  xen/common/domain.c          | 3 ++-
>  xen/include/asm-arm/domain.h | 4 ++--
>  xen/include/public/domctl.h  | 4 ++++
>  xen/include/xen/domain.h     | 3 +++
>  7 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index
> 96e1b23550..d77265c03f 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -629,7 +629,8 @@ int arch_sanitise_domain_config(struct
> xen_domctl_createdomain *config)  {
>      unsigned int max_vcpus;
>      unsigned int flags_required = (XEN_DOMCTL_CDF_hvm |
> XEN_DOMCTL_CDF_hap);
> -    unsigned int flags_optional = (XEN_DOMCTL_CDF_iommu |
> XEN_DOMCTL_CDF_vpmu);
> +    unsigned int flags_optional = (XEN_DOMCTL_CDF_iommu |
> XEN_DOMCTL_CDF_vpmu |
> +                                   XEN_DOMCTL_CDF_INTERNAL_directmap);
> 
>      if ( (config->flags & ~flags_optional) != flags_required )
>      {
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 19487c79da..664c88ebe4 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -3089,8 +3089,10 @@ static int __init construct_dom0(struct domain *d)
> void __init create_dom0(void)  {
>      struct domain *dom0;
> +    /* DOM0 has always its memory direct-map. */
>      struct xen_domctl_createdomain dom0_cfg = {
> -        .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
> +        .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap |
> +                 XEN_DOMCTL_CDF_INTERNAL_directmap,
>          .max_evtchn_port = -1,
>          .max_grant_frames = gnttab_dom0_frames(),
>          .max_maptrack_frames = -1,
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index
> ef1812dc14..eba6502218 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -692,6 +692,12 @@ int arch_sanitise_domain_config(struct
> xen_domctl_createdomain *config)
>          return -EINVAL;
>      }
> 
> +    if ( config->flags & XEN_DOMCTL_CDF_INTERNAL_directmap )
> +    {
> +        dprintk(XENLOG_INFO, "direct-map cannot be enabled yet\n");
> +        return -EINVAL;
> +    }
> +
>      return 0;
>  }
> 
> diff --git a/xen/common/domain.c b/xen/common/domain.c index
> 56d47dd664..13ac5950bc 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -486,7 +486,8 @@ static int sanitise_domain_config(struct
> xen_domctl_createdomain *config)
>           ~(XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap |
>             XEN_DOMCTL_CDF_s3_integrity | XEN_DOMCTL_CDF_oos_off |
>             XEN_DOMCTL_CDF_xs_domain | XEN_DOMCTL_CDF_iommu |
> -           XEN_DOMCTL_CDF_nested_virt | XEN_DOMCTL_CDF_vpmu) )
> +           XEN_DOMCTL_CDF_nested_virt | XEN_DOMCTL_CDF_vpmu |
> +           XEN_DOMCTL_CDF_INTERNAL_directmap) )
>      {
>          dprintk(XENLOG_INFO, "Unknown CDF flags %#x\n", config->flags);
>          return -EINVAL;
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 9b3647587a..4f2c3f09d4 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -29,8 +29,8 @@ enum domain_type {
>  #define is_64bit_domain(d) (0)
>  #endif
> 
> -/* The hardware domain has always its memory direct mapped. */ -#define
> is_domain_direct_mapped(d) is_hardware_domain(d)
> +#define is_domain_direct_mapped(d) \
> +        (d->options & XEN_DOMCTL_CDF_INTERNAL_directmap)
> 
>  struct vtimer {
>      struct vcpu *v;
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index
> 1c21d4dc75..054e545c97 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -72,6 +72,10 @@ 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)
> +/*
> + * Be aware that bit 8 has already been occupied by flag
> + * XEN_DOMCTL_CDF_INTERNAL_directmap, defined in
> xen/include/xen/domain.h.
> + */
> 
>  /* Max XEN_DOMCTL_CDF_* constant.  Used for ABI checking. */  #define
> XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_vpmu diff --git
> a/xen/include/xen/domain.h b/xen/include/xen/domain.h index
> 160c8dbdab..2b9edfdcee 100644
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -28,6 +28,9 @@ void getdomaininfo(struct domain *d, struct
> xen_domctl_getdomaininfo *info);  void arch_get_domain_info(const struct
> domain *d,
>                            struct xen_domctl_getdomaininfo *info);
> 
> +/* Should domain memory be directly mapped? */
> +#define XEN_DOMCTL_CDF_INTERNAL_directmap      (1U << 8)
> +

I run into some trouble with defining this flag internal in the new serie.

Let me explain in details here:

1. Currently XEN_DOMCTL_CDF_MAX is set to XEN_DOMCTL_CDF_vpmu.
So we can say that XEN_DOMCTL_CDF_MAX knows that there are 8 CDF flags(0 to 7).
The corresponding ocaml tool has a list of CDF flags and currently it knows 
that there are 8 CDF flags:
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=tools/ocaml/libs/xc/xenctrl.ml;h=7503031d8f61c2dbcd4aa803738c83e10dfb7bb8;hb=HEAD#l64
 
This tool performs a check to see if the XEN_DOMCTL_CDF_MAX is equal to the 
number of entries in domain_create_flag.

2. Here we are reserving bit 8 for internal flag 
XEN_DOMCTL_CDF_INTERNAL_directmap. As this is internal flag,
I do not want to modify XEN_DOMCTL_CDF_MAX.

3. Everything is perfect until someone tries to add another global CDF flag:

#define XEN_DOMCTL_CDF_next_flag  (1<<9)
#define XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_next_flag

XEN_DOMCTL_CDF_MAX shows right now that there are 10 flags but ocaml tool sees 
only 9.
then we are getting build error.

Hmm, would you please help me find a way to fix this dilemma, thx.

>  /*
>   * Arch-specifics.
>   */
> --
> 2.25.1

Cheers,
Penny Zheng