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

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Friday, October 15, 2021 4:47 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Bertrand Marquis
> <Bertrand.Marquis@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> sstabellini@xxxxxxxxxx; julien@xxxxxxx
> Subject: Re: [PATCH v2 1/6] xen: introduce XEN_DOMCTL_CDF_directmap
> 
> 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.
> 

Sure.

> >  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.
> 

The patch serie is based on the staging branch with an extra commit "
Revert "xen/domctl: Introduce XEN_DOMCTL_CDF_vpci flag", which
Is already been pushed to community for review.(
https://lists.xenproject.org/archives/html/xen-devel/2021-10/msg00822.html)

> > --- 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