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

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Tuesday, December 7, 2021 5:28 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Bertrand Marquis
> <Bertrand.Marquis@xxxxxxx>; Michal Orzel <Michal.Orzel@xxxxxxx>;
> julien@xxxxxxx; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v3 01/10] xen: introduce
> XEN_DOMCTL_CDF_INTERNAL_directmap
> 
> On 07.12.2021 10:15, Penny Zheng wrote:
> > 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.
> 
> This was already outlined, but let me do so again: You do _not_ want to
> overlay with XEN_DOMCTL_CDF_*. domain_create() already has an internal-
> only parameter. That's a "bool" right now and wants extending to an
> "unsigned int" covering both the existing "is_priv" (step 1) and your new
> "directmap" (step 2). To make visible the relationship, naming the respective
> constants CDF_* (with no XEN_DOMCTL_ prefix to represent the
> difference) might be appopriate.

Oh,  I understand finally...
We shall create another new "unsigned int" CDF_* (with no XEN_DOMCTL_ prefix) to
cover all internal flags(priv and direct-map).

> Btw, as a result (if that's not the plan already anyway) you then probably 
> also
> want to decouple is_domain_direct_mapped() from is_hardware_domain(),
> and hence create Dom0 also with the new flag set.
> 
> Jan