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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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