| Message ID | 20211015030945.2082898-2-penny.zheng@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | direct-map memory map | expand |
On 15.10.2021 05:09, Penny Zheng wrote: > From: Stefano Stabellini <stefano.stabellini@xilinx.com> > > 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@xilinx.com> > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > --- > CC: andrew.cooper3@citrix.com > CC: jbeulich@suse.com > CC: George Dunlap <George.Dunlap@eu.citrix.com> > CC: Ian Jackson <ian.jackson@eu.citrix.com> > CC: Wei Liu <wl@xen.org> > CC: "Roger Pau Monné" <roger.pau@citrix.com> > --- 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
Hi Penny, On 15/10/2021 04:09, Penny Zheng wrote: > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 0167731ab0..37e2d62d47 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -3069,8 +3069,10 @@ static int __init construct_dom0(struct domain *d) > void __init create_dom0(void) > { > struct domain *dom0; > + /* DOM0 has always its memory directly mapped. */ > 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_directmap, > .max_evtchn_port = -1, > .max_grant_frames = gnttab_dom0_frames(), > .max_maptrack_frames = -1, > diff --git a/xen/common/domain.c b/xen/common/domain.c > index 8b53c49d1e..7a6131db74 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_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 14e575288f..fc42c6a310 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) > +/* Check if domain is direct-map memory map. */ > +#define is_domain_direct_mapped(d) (d->options & XEN_DOMCTL_CDF_directmap) > > struct vtimer { > struct vcpu *v; > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h > index 238384b5ae..b505a0db51 100644 > --- 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) > > /* Max XEN_DOMCTL_CDF_* constant. Used for ABI checking. */ > -#define XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_vpmu > +#define XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_directmap In the previous version, this flag was only settable for domain created by Xen. Now this is also settable by the toolstack. I don't think the toolstack can sensibly use this flag (at least in the current state). So can you explain why this flag is exposed to the toolstack? Cheers,
Hi Jan > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: Friday, October 15, 2021 4:47 PM > To: Penny Zheng <Penny.Zheng@arm.com> > Cc: Wei Chen <Wei.Chen@arm.com>; Bertrand Marquis > <Bertrand.Marquis@arm.com>; xen-devel@lists.xenproject.org; > sstabellini@kernel.org; julien@xen.org > 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@xilinx.com> > > > > 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@xilinx.com> > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > > --- > > CC: andrew.cooper3@citrix.com > > CC: jbeulich@suse.com > > CC: George Dunlap <George.Dunlap@eu.citrix.com> > > CC: Ian Jackson <ian.jackson@eu.citrix.com> > > CC: Wei Liu <wl@xen.org> > > CC: "Roger Pau Monné" <roger.pau@citrix.com> > > --- > > 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
Hi julien > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: Friday, October 15, 2021 4:57 PM > To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org; > sstabellini@kernel.org > Cc: Wei Chen <Wei.Chen@arm.com>; Bertrand Marquis > <Bertrand.Marquis@arm.com> > Subject: Re: [PATCH v2 1/6] xen: introduce XEN_DOMCTL_CDF_directmap > > Hi Penny, > > On 15/10/2021 04:09, Penny Zheng wrote: > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > > index 0167731ab0..37e2d62d47 100644 > > --- a/xen/arch/arm/domain_build.c > > +++ b/xen/arch/arm/domain_build.c > > @@ -3069,8 +3069,10 @@ static int __init construct_dom0(struct domain *d) > > void __init create_dom0(void) > > { > > struct domain *dom0; > > + /* DOM0 has always its memory directly mapped. */ > > 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_directmap, > > .max_evtchn_port = -1, > > .max_grant_frames = gnttab_dom0_frames(), > > .max_maptrack_frames = -1, > > diff --git a/xen/common/domain.c b/xen/common/domain.c index > > 8b53c49d1e..7a6131db74 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_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 14e575288f..fc42c6a310 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) > > +/* Check if domain is direct-map memory map. */ #define > > +is_domain_direct_mapped(d) (d->options & XEN_DOMCTL_CDF_directmap) > > > > struct vtimer { > > struct vcpu *v; > > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h > > index 238384b5ae..b505a0db51 100644 > > --- 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) > > > > /* Max XEN_DOMCTL_CDF_* constant. Used for ABI checking. */ > > -#define XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_vpmu > > +#define XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_directmap > > In the previous version, this flag was only settable for domain created by Xen. > Now this is also settable by the toolstack. I don't think the toolstack can > sensibly use this flag (at least in the current state). > > So can you explain why this flag is exposed to the toolstack? Oh, I misunderstood the previous discussion on internal usage a bit, sorry. And I will make it back to xen/include/xen/domain.h > > Cheers, > > -- > Julien Grall
On 15.10.2021 11:59, Penny Zheng wrote: >> From: Jan Beulich <jbeulich@suse.com> >> Sent: Friday, October 15, 2021 4:47 PM >> >> On 15.10.2021 05:09, Penny Zheng wrote: >>> 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) I was assuming so, and hence I didn't also refer you to the vPCI patch, which you could also have used for reference. I guess my primary problem here is that I can't see what you're trying to tell me. Jan
Hi Julien > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: Friday, October 15, 2021 4:57 PM > To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org; > sstabellini@kernel.org > Cc: Wei Chen <Wei.Chen@arm.com>; Bertrand Marquis > <Bertrand.Marquis@arm.com> > Subject: Re: [PATCH v2 1/6] xen: introduce XEN_DOMCTL_CDF_directmap > > Hi Penny, > > On 15/10/2021 04:09, Penny Zheng wrote: > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > > index 0167731ab0..37e2d62d47 100644 > > --- a/xen/arch/arm/domain_build.c > > +++ b/xen/arch/arm/domain_build.c > > @@ -3069,8 +3069,10 @@ static int __init construct_dom0(struct domain *d) > > void __init create_dom0(void) > > { > > struct domain *dom0; > > + /* DOM0 has always its memory directly mapped. */ > > 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_directmap, > > .max_evtchn_port = -1, > > .max_grant_frames = gnttab_dom0_frames(), > > .max_maptrack_frames = -1, > > diff --git a/xen/common/domain.c b/xen/common/domain.c index > > 8b53c49d1e..7a6131db74 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_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 14e575288f..fc42c6a310 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) > > +/* Check if domain is direct-map memory map. */ #define > > +is_domain_direct_mapped(d) (d->options & XEN_DOMCTL_CDF_directmap) > > > > struct vtimer { > > struct vcpu *v; > > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h > > index 238384b5ae..b505a0db51 100644 > > --- 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) > > > > /* Max XEN_DOMCTL_CDF_* constant. Used for ABI checking. */ > > -#define XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_vpmu > > +#define XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_directmap > > In the previous version, this flag was only settable for domain created by Xen. > Now this is also settable by the toolstack. I don't think the toolstack can > sensibly use this flag (at least in the current state). > > So can you explain why this flag is exposed to the toolstack? > if I moved XEN_DOMCTL_CDF_directmap to the domain.h, and let it hold the bit 8, in case later another developer wants to introduce a new flag to accidently hold 8 bit too, I would like to add some explanatory comments here and maybe rename the XEN_DOMCTL_CDF_directmap to XEN_DOMCTL_CDF_Internal_directmap, wdyt? > Cheers, > > -- Cheers Penny Zheng > Julien Grall
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index eef0661beb..8cee1c6349 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -628,7 +628,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_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 0167731ab0..37e2d62d47 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -3069,8 +3069,10 @@ static int __init construct_dom0(struct domain *d) void __init create_dom0(void) { struct domain *dom0; + /* DOM0 has always its memory directly mapped. */ 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_directmap, .max_evtchn_port = -1, .max_grant_frames = gnttab_dom0_frames(), .max_maptrack_frames = -1, diff --git a/xen/common/domain.c b/xen/common/domain.c index 8b53c49d1e..7a6131db74 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_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 14e575288f..fc42c6a310 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) +/* Check if domain is direct-map memory map. */ +#define is_domain_direct_mapped(d) (d->options & XEN_DOMCTL_CDF_directmap) struct vtimer { struct vcpu *v; diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index 238384b5ae..b505a0db51 100644 --- 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) /* Max XEN_DOMCTL_CDF_* constant. Used for ABI checking. */ -#define XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_vpmu +#define XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_directmap uint32_t flags;