On Tue, Oct 14, 2025 at 06:48:23PM +0300, Grygorii Strashko wrote: > > > On 14.10.25 17:38, Roger Pau Monné wrote: > > On Tue, Oct 14, 2025 at 04:24:53PM +0300, Grygorii Strashko wrote: > > > On 13.10.25 15:17, Roger Pau Monné wrote: > > > > On Tue, Sep 30, 2025 at 12:52:16PM +0000, Grygorii Strashko wrote: > > > > > From: Sergiy Kibrik <[email protected]> > > > > > + > > > > > + If unsure, say Y. > > > > > + > > > > > config MEM_PAGING > > > > > bool "Xen memory paging support (UNSUPPORTED)" if UNSUPPORTED > > > > > depends on VM_EVENT > > > > > diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile > > > > > index 6ec2c8f2db56..736eb3f966e9 100644 > > > > > --- a/xen/arch/x86/hvm/Makefile > > > > > +++ b/xen/arch/x86/hvm/Makefile > > > > > @@ -1,6 +1,6 @@ > > > > > obj-$(CONFIG_AMD_SVM) += svm/ > > > > > obj-$(CONFIG_INTEL_VMX) += vmx/ > > > > > -obj-y += viridian/ > > > > > +obj-$(CONFIG_VIRIDIAN) += viridian/ > > > > > obj-y += asid.o > > > > > obj-y += dm.o > > > > > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > > > > > index 23bd7f078a1d..95a80369b9b8 100644 > > > > > --- a/xen/arch/x86/hvm/hvm.c > > > > > +++ b/xen/arch/x86/hvm/hvm.c > > > > > @@ -701,9 +701,12 @@ int hvm_domain_initialise(struct domain *d, > > > > > if ( hvm_tsc_scaling_supported ) > > > > > d->arch.hvm.tsc_scaling_ratio = > > > > > hvm_default_tsc_scaling_ratio; > > > > > - rc = viridian_domain_init(d); > > > > > - if ( rc ) > > > > > - goto fail2; > > > > > + if ( is_viridian_domain(d) ) > > > > > + { > > > > > + rc = viridian_domain_init(d); > > > > > + if ( rc ) > > > > > + goto fail2; > > > > > + } > > > > > > > > Are you sure this works as expected? > > > > > > > > The viridian_feature_mask() check is implemented using an HVM param, > > > > and hence can only be possibly set after the domain object is created. > > > > AFAICT is_viridian_domain(d) will unconditionally return false when > > > > called from domain_create() context, because the HVM params cannot > > > > possibly be set ahead of the domain being created. > > > > > > You are right. Thanks for the this catch. > > > > > > Taking above into account above, it seems Jan's proposal to convert below > > > viridian APIs into wrappers for VIRIDIAN=n case is right way to move > > > forward: > > > > > > int viridian_vcpu_init(struct vcpu *v); > > > int viridian_domain_init(struct domain *d); > > > void viridian_vcpu_deinit(struct vcpu *v); > > > void viridian_domain_deinit(struct domain *d); > > > > > > Right? > > > > Possibly. If you don't want to introduce a XEN_DOMCTL_createdomain > > flag you need to exclusively use the Kconfig option to decide whether > > the Viridian related structs must be allocated. IOW: you could also > > solve it by using IS_ENABLED(CONFIG_VIRIDIAN) instead of > > is_viridian_domain() for most of the calls here. > > > > The wrapper option might be better IMO, rather than adding > > IS_ENABLED(CONFIG_VIRIDIAN) around. > > I'll do wrappers - less if(s) in common HVM code. > > > > > > [1] https://patchwork.kernel.org/comment/26595213/ > > > > > > > > > > > If you want to do anything like this you will possibly need to > > > > introduce a new flag to XEN_DOMCTL_createdomain to signal whether the > > > > domain has Viridian extensions are enabled or not, so that it's know > > > > in the context where domain_create() gets called. > > > > > > In my opinion, it might be good not to go so far within this submission. > > > - It's not intended to change existing behavior of neither Xen nor > > > toolstack > > > for VIRIDIAN=y (default) > > > - just optout Viridian support when not needed. > > > > OK, that's fine. > > > > On further request though: if Viridian is build-time disabled in > > Kconfig, setting or fetching HVM_PARAM_VIRIDIAN should return -ENODEV > > or similar error. I don't think this is done as part of this patch.
Another bit I've noticed, you will need to adjust write_hvm_params() so it can tolerate xc_hvm_param_get() returning an error when HVM_PARAM_VIRIDIAN is not implemented by the hypervisor. Implementing the Viridian features using an HVM parameter was a bad approach probably. > Sure. Just have to ask for clarification what to return: > -EOPNOTSUPP (my choise) vs -EINVAL. Let me add Jan also to the To: field so we get consensus in one round. I won't use EINVAL, because that's returned for deprecated parameters also, and when the passed Viridian feature mask is invalid. EOPNOTSUPP is also returned for non-implemented hypercalls, so I'm not sure whether it could cause confusion here, as the hypercall is implemented, it's just the param that's not supported if build-disabled. Maybe ENODEV or ENXIO? Thanks, Roger.
