| Message ID | 20231006091353.96367-3-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | domain: followup for phys address mapping series | expand |
Hi Roger, > On Oct 6, 2023, at 17:13, Roger Pau Monne <roger.pau@citrix.com> wrote: > > XENFEAT_runstate_phys_area is exposed to all architectures, while > XENFEAT_vcpu_time_phys_area is currnelty only implemented for x86, and hence > the feature flag is also only exposed on x86. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Release-acked-by: Henry Wang <Henry.Wang@arm.com> Kind regards, Henry > --- > CHANGELOG.md | 2 ++ > xen/common/kernel.c | 6 +++++- > xen/include/public/features.h | 9 +++++++++ > 3 files changed, 16 insertions(+), 1 deletion(-) > > diff --git a/CHANGELOG.md b/CHANGELOG.md > index e33cf4e1b113..41da710426f6 100644 > --- a/CHANGELOG.md > +++ b/CHANGELOG.md > @@ -31,6 +31,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) > - Add Intel Hardware P-States (HWP) cpufreq driver. > - On Arm, experimental support for dynamic addition/removal of Xen device tree > nodes using a device tree overlay binary (.dtbo). > + - Introduce two new hypercalls to map the vCPU runstate and time areas by > + physical rather than linear addresses. > > ### Removed > - On x86, the "pku" command line option has been removed. It has never > diff --git a/xen/common/kernel.c b/xen/common/kernel.c > index 52aa28762782..b6302e44b34e 100644 > --- a/xen/common/kernel.c > +++ b/xen/common/kernel.c > @@ -607,7 +607,11 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > switch ( fi.submap_idx ) > { > case 0: > - fi.submap = (1U << XENFEAT_memory_op_vnode_supported); > + fi.submap = (1U << XENFEAT_memory_op_vnode_supported) | > +#ifdef CONFIG_X86 > + (1U << XENFEAT_vcpu_time_phys_area) | > +#endif > + (1U << XENFEAT_runstate_phys_area); > if ( VM_ASSIST(d, pae_extended_cr3) ) > fi.submap |= (1U << XENFEAT_pae_pgdir_above_4gb); > if ( paging_mode_translate(d) ) > diff --git a/xen/include/public/features.h b/xen/include/public/features.h > index d2a9175aae67..cffb2f14a562 100644 > --- a/xen/include/public/features.h > +++ b/xen/include/public/features.h > @@ -111,6 +111,15 @@ > #define XENFEAT_not_direct_mapped 16 > #define XENFEAT_direct_mapped 17 > > +/* > + * Signal whether the hypervisor implements the following hypercalls: > + * > + * VCPUOP_register_runstate_phys_area > + * VCPUOP_register_vcpu_time_phys_area > + */ > +#define XENFEAT_runstate_phys_area 18 > +#define XENFEAT_vcpu_time_phys_area 19 > + > #define XENFEAT_NR_SUBMAPS 1 > > #endif /* __XEN_PUBLIC_FEATURES_H__ */ > -- > 2.42.0 >
On 06/10/2023 10:13 am, Roger Pau Monne wrote: > XENFEAT_runstate_phys_area is exposed to all architectures, while > XENFEAT_vcpu_time_phys_area is currnelty only implemented for x86, and hence > the feature flag is also only exposed on x86. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Thanks for doing this. > --- > CHANGELOG.md | 2 ++ > xen/common/kernel.c | 6 +++++- > xen/include/public/features.h | 9 +++++++++ > 3 files changed, 16 insertions(+), 1 deletion(-) > > diff --git a/CHANGELOG.md b/CHANGELOG.md > index e33cf4e1b113..41da710426f6 100644 > --- a/CHANGELOG.md > +++ b/CHANGELOG.md > @@ -31,6 +31,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) > - Add Intel Hardware P-States (HWP) cpufreq driver. > - On Arm, experimental support for dynamic addition/removal of Xen device tree > nodes using a device tree overlay binary (.dtbo). > + - Introduce two new hypercalls to map the vCPU runstate and time areas by > + physical rather than linear addresses. I'd suggest linear/virtual here. Linear is the correct term in x86, but virtual is the correct term in pretty much every other architecture. > diff --git a/xen/include/public/features.h b/xen/include/public/features.h > index d2a9175aae67..cffb2f14a562 100644 > --- a/xen/include/public/features.h > +++ b/xen/include/public/features.h > @@ -111,6 +111,15 @@ > #define XENFEAT_not_direct_mapped 16 > #define XENFEAT_direct_mapped 17 > > +/* > + * Signal whether the hypervisor implements the following hypercalls: This is not what the hypervisor implements. It's what the domain is permitted to use. There also needs to be a matching patch to public/vcpu.h to require implementations to check for these feature bits before using the new hypercalls. Also, diff --git a/xen/common/domain.c b/xen/common/domain.c index b8281d7cff9d..df994bd30fd2 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -1998,6 +1998,10 @@ long common_vcpu_op(int cmd, struct vcpu *v { struct vcpu_register_runstate_memory_area area; + rc = -ENOSYS; + if ( 0 /* TODO: Dom's XENFEAT_runstate_phys_area setting */ ) + break; + rc = -EFAULT; if ( copy_from_guest(&area.addr.p, arg, 1) ) break; and a matching one for XENFEAT_vcpu_time_phys_area because I'm even more serious about this becoming a domain controllable setting following what OSSTest had to say overnight. ~Andrew
On Fri, Oct 06, 2023 at 11:47:48AM +0100, Andrew Cooper wrote: > On 06/10/2023 10:13 am, Roger Pau Monne wrote: > > XENFEAT_runstate_phys_area is exposed to all architectures, while > > XENFEAT_vcpu_time_phys_area is currnelty only implemented for x86, and hence > > the feature flag is also only exposed on x86. > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > Thanks for doing this. > > > --- > > CHANGELOG.md | 2 ++ > > xen/common/kernel.c | 6 +++++- > > xen/include/public/features.h | 9 +++++++++ > > 3 files changed, 16 insertions(+), 1 deletion(-) > > > > diff --git a/CHANGELOG.md b/CHANGELOG.md > > index e33cf4e1b113..41da710426f6 100644 > > --- a/CHANGELOG.md > > +++ b/CHANGELOG.md > > @@ -31,6 +31,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) > > - Add Intel Hardware P-States (HWP) cpufreq driver. > > - On Arm, experimental support for dynamic addition/removal of Xen device tree > > nodes using a device tree overlay binary (.dtbo). > > + - Introduce two new hypercalls to map the vCPU runstate and time areas by > > + physical rather than linear addresses. > > I'd suggest linear/virtual here. Linear is the correct term in x86, but > virtual is the correct term in pretty much every other architecture. > > > diff --git a/xen/include/public/features.h b/xen/include/public/features.h > > index d2a9175aae67..cffb2f14a562 100644 > > --- a/xen/include/public/features.h > > +++ b/xen/include/public/features.h > > @@ -111,6 +111,15 @@ > > #define XENFEAT_not_direct_mapped 16 > > #define XENFEAT_direct_mapped 17 > > > > +/* > > + * Signal whether the hypervisor implements the following hypercalls: > > This is not what the hypervisor implements. It's what the domain is > permitted to use. > > There also needs to be a matching patch to public/vcpu.h to require > implementations to check for these feature bits before using the new > hypercalls. > > Also, > > diff --git a/xen/common/domain.c b/xen/common/domain.c > index b8281d7cff9d..df994bd30fd2 100644 > --- a/xen/common/domain.c > +++ b/xen/common/domain.c > @@ -1998,6 +1998,10 @@ long common_vcpu_op(int cmd, struct vcpu *v > { > struct vcpu_register_runstate_memory_area area; > > + rc = -ENOSYS; > + if ( 0 /* TODO: Dom's XENFEAT_runstate_phys_area setting */ ) > + break; > + > rc = -EFAULT; > if ( copy_from_guest(&area.addr.p, arg, 1) ) > break; > > and a matching one for XENFEAT_vcpu_time_phys_area because I'm even more > serious about this becoming a domain controllable setting following what > OSSTest had to say overnight. While this is all fine, please note that the newly added code {,un}map_guest_area() is also used by the existing VCPUOP_register_vcpu_info hypercall, and that one can't be disabled. IOW: even if we add knobs to make the new hypercalls selectable, most of the newly added code could still be reached from VCPUOP_register_vcpu_info. Thanks, Roger.
On 06/10/2023 12:02 pm, Roger Pau Monné wrote: > On Fri, Oct 06, 2023 at 11:47:48AM +0100, Andrew Cooper wrote: >> diff --git a/xen/common/domain.c b/xen/common/domain.c >> index b8281d7cff9d..df994bd30fd2 100644 >> --- a/xen/common/domain.c >> +++ b/xen/common/domain.c >> @@ -1998,6 +1998,10 @@ long common_vcpu_op(int cmd, struct vcpu *v >> { >> struct vcpu_register_runstate_memory_area area; >> >> + rc = -ENOSYS; >> + if ( 0 /* TODO: Dom's XENFEAT_runstate_phys_area setting */ ) >> + break; >> + >> rc = -EFAULT; >> if ( copy_from_guest(&area.addr.p, arg, 1) ) >> break; >> >> and a matching one for XENFEAT_vcpu_time_phys_area because I'm even more >> serious about this becoming a domain controllable setting following what >> OSSTest had to say overnight. > While this is all fine, please note that the newly added code > {,un}map_guest_area() is also used by the existing > VCPUOP_register_vcpu_info hypercall, and that one can't be disabled. Yeah, I'm aware we're stuck there, but a crap interface from the past is not an excuse not to do new interfaces properly. ~Andrew
On Fri, Oct 06, 2023 at 12:19:29PM +0100, Andrew Cooper wrote: > On 06/10/2023 12:02 pm, Roger Pau Monné wrote: > > On Fri, Oct 06, 2023 at 11:47:48AM +0100, Andrew Cooper wrote: > >> diff --git a/xen/common/domain.c b/xen/common/domain.c > >> index b8281d7cff9d..df994bd30fd2 100644 > >> --- a/xen/common/domain.c > >> +++ b/xen/common/domain.c > >> @@ -1998,6 +1998,10 @@ long common_vcpu_op(int cmd, struct vcpu *v > >> { > >> struct vcpu_register_runstate_memory_area area; > >> > >> + rc = -ENOSYS; > >> + if ( 0 /* TODO: Dom's XENFEAT_runstate_phys_area setting */ ) > >> + break; > >> + > >> rc = -EFAULT; > >> if ( copy_from_guest(&area.addr.p, arg, 1) ) > >> break; > >> > >> and a matching one for XENFEAT_vcpu_time_phys_area because I'm even more > >> serious about this becoming a domain controllable setting following what > >> OSSTest had to say overnight. > > While this is all fine, please note that the newly added code > > {,un}map_guest_area() is also used by the existing > > VCPUOP_register_vcpu_info hypercall, and that one can't be disabled. > > Yeah, I'm aware we're stuck there, but a crap interface from the past is > not an excuse not to do new interfaces properly. Right, want I intended to say is that if we are worried the new {,un}map_guest_area() might be buggy, and would like to have a way to disable it, just preventing usage of VCPUOP_register_{runstate,vcpu_info}_phys_area won't be enough as the newly introduced function is also used by the existing VCPUOP_register_vcpu_info. Not that we shouldn't add the checks, just that those won't cover all usages of {,un}map_guest_area(). Thanks, Roger.
diff --git a/CHANGELOG.md b/CHANGELOG.md index e33cf4e1b113..41da710426f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) - Add Intel Hardware P-States (HWP) cpufreq driver. - On Arm, experimental support for dynamic addition/removal of Xen device tree nodes using a device tree overlay binary (.dtbo). + - Introduce two new hypercalls to map the vCPU runstate and time areas by + physical rather than linear addresses. ### Removed - On x86, the "pku" command line option has been removed. It has never diff --git a/xen/common/kernel.c b/xen/common/kernel.c index 52aa28762782..b6302e44b34e 100644 --- a/xen/common/kernel.c +++ b/xen/common/kernel.c @@ -607,7 +607,11 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) switch ( fi.submap_idx ) { case 0: - fi.submap = (1U << XENFEAT_memory_op_vnode_supported); + fi.submap = (1U << XENFEAT_memory_op_vnode_supported) | +#ifdef CONFIG_X86 + (1U << XENFEAT_vcpu_time_phys_area) | +#endif + (1U << XENFEAT_runstate_phys_area); if ( VM_ASSIST(d, pae_extended_cr3) ) fi.submap |= (1U << XENFEAT_pae_pgdir_above_4gb); if ( paging_mode_translate(d) ) diff --git a/xen/include/public/features.h b/xen/include/public/features.h index d2a9175aae67..cffb2f14a562 100644 --- a/xen/include/public/features.h +++ b/xen/include/public/features.h @@ -111,6 +111,15 @@ #define XENFEAT_not_direct_mapped 16 #define XENFEAT_direct_mapped 17 +/* + * Signal whether the hypervisor implements the following hypercalls: + * + * VCPUOP_register_runstate_phys_area + * VCPUOP_register_vcpu_time_phys_area + */ +#define XENFEAT_runstate_phys_area 18 +#define XENFEAT_vcpu_time_phys_area 19 + #define XENFEAT_NR_SUBMAPS 1 #endif /* __XEN_PUBLIC_FEATURES_H__ */
XENFEAT_runstate_phys_area is exposed to all architectures, while XENFEAT_vcpu_time_phys_area is currnelty only implemented for x86, and hence the feature flag is also only exposed on x86. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- CHANGELOG.md | 2 ++ xen/common/kernel.c | 6 +++++- xen/include/public/features.h | 9 +++++++++ 3 files changed, 16 insertions(+), 1 deletion(-)