| Message ID | 20211215222115.6829-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/cpuid: Introduce dom0-cpuid= | expand |
On 15.12.2021 23:21, Andrew Cooper wrote: > To implement dom0-cpuid= support, the special cases would need extending. > However there is already a problem with late hwdom where the special cases > override toolstack settings, which is unintended and poor behaviour. > > Introduce a new init_dom0_cpuid_policy() for the purpose, moving the ITSC and > ARCH_CAPS logic. The is_hardware_domain() can be dropped, and for now there > is no need to rerun recalculate_cpuid_policy(); this is a relatively expensive > operation, and will become more-so over time. Would you mind leaving it there in a commented out form, hinting at when it may need re-enabling? > Rearrange the logic in create_dom0() to make room for a call to > init_dom0_cpuid_policy(). The AMX plans for having variable sized XSAVE > states require that modifications to the policy happen before vCPUs are > created. > > Additionally, factor out domid into a variable so we can be slightly more > correct in the case of a failure, and also print the error from > domain_create(). This will at least help distinguish -EINVAL from -ENOMEM. > > No practical change in behaviour. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Preferably with said comment added: Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On 16/12/2021 16:38, Jan Beulich wrote: > On 15.12.2021 23:21, Andrew Cooper wrote: >> To implement dom0-cpuid= support, the special cases would need extending. >> However there is already a problem with late hwdom where the special cases >> override toolstack settings, which is unintended and poor behaviour. >> >> Introduce a new init_dom0_cpuid_policy() for the purpose, moving the ITSC and >> ARCH_CAPS logic. The is_hardware_domain() can be dropped, and for now there >> is no need to rerun recalculate_cpuid_policy(); this is a relatively expensive >> operation, and will become more-so over time. > Would you mind leaving it there in a commented out form, hinting at when > it may need re-enabling? Leave what? The recalculate_cpuid_policy()? That comes back in later in the series. ~Andrew > >> Rearrange the logic in create_dom0() to make room for a call to >> init_dom0_cpuid_policy(). The AMX plans for having variable sized XSAVE >> states require that modifications to the policy happen before vCPUs are >> created. >> >> Additionally, factor out domid into a variable so we can be slightly more >> correct in the case of a failure, and also print the error from >> domain_create(). This will at least help distinguish -EINVAL from -ENOMEM. >> >> No practical change in behaviour. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Preferably with said comment added: > Reviewed-by: Jan Beulich <jbeulich@suse.com> > > Jan > >
On 16.12.2021 17:41, Andrew Cooper wrote: > On 16/12/2021 16:38, Jan Beulich wrote: >> On 15.12.2021 23:21, Andrew Cooper wrote: >>> To implement dom0-cpuid= support, the special cases would need extending. >>> However there is already a problem with late hwdom where the special cases >>> override toolstack settings, which is unintended and poor behaviour. >>> >>> Introduce a new init_dom0_cpuid_policy() for the purpose, moving the ITSC and >>> ARCH_CAPS logic. The is_hardware_domain() can be dropped, and for now there >>> is no need to rerun recalculate_cpuid_policy(); this is a relatively expensive >>> operation, and will become more-so over time. >> Would you mind leaving it there in a commented out form, hinting at when >> it may need re-enabling? > > Leave what? The recalculate_cpuid_policy()? That comes back in later > in the series. I've meanwhile spotted it, yes. Let's hope its conditional invocation there makes clear enough that with certain other changes it may also be needed. Jan
On 16/12/2021 16:51, Jan Beulich wrote: > On 16.12.2021 17:41, Andrew Cooper wrote: >> On 16/12/2021 16:38, Jan Beulich wrote: >>> On 15.12.2021 23:21, Andrew Cooper wrote: >>>> To implement dom0-cpuid= support, the special cases would need extending. >>>> However there is already a problem with late hwdom where the special cases >>>> override toolstack settings, which is unintended and poor behaviour. >>>> >>>> Introduce a new init_dom0_cpuid_policy() for the purpose, moving the ITSC and >>>> ARCH_CAPS logic. The is_hardware_domain() can be dropped, and for now there >>>> is no need to rerun recalculate_cpuid_policy(); this is a relatively expensive >>>> operation, and will become more-so over time. >>> Would you mind leaving it there in a commented out form, hinting at when >>> it may need re-enabling? >> Leave what? The recalculate_cpuid_policy()? That comes back in later >> in the series. > I've meanwhile spotted it, yes. Let's hope its conditional invocation > there makes clear enough that with certain other changes it may also > be needed. In reality, I expect ITSC never to need a recalc, and ARCH_CAPS is going to turn into not-a-special-case just as soon as I can possibly make it happen. ~Andrew
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c index 151944f65702..f63f5efc17f5 100644 --- a/xen/arch/x86/cpuid.c +++ b/xen/arch/x86/cpuid.c @@ -727,23 +727,28 @@ int init_domain_cpuid_policy(struct domain *d) if ( !p ) return -ENOMEM; - /* The hardware domain can't migrate. Give it ITSC if available. */ - if ( is_hardware_domain(d) ) - p->extd.itsc = cpu_has_itsc; + d->arch.cpuid = p; + + recalculate_cpuid_policy(d); + + return 0; +} + +void __init init_dom0_cpuid_policy(struct domain *d) +{ + struct cpuid_policy *p = d->arch.cpuid; + + /* dom0 can't migrate. Give it ITSC if available. */ + if ( cpu_has_itsc ) + p->extd.itsc = true; /* * Expose the "hardware speculation behaviour" bits of ARCH_CAPS to dom0, * so dom0 can turn off workarounds as appropriate. Temporary, until the * domain policy logic gains a better understanding of MSRs. */ - if ( is_hardware_domain(d) && cpu_has_arch_caps ) + if ( cpu_has_arch_caps ) p->feat.arch_caps = true; - - d->arch.cpuid = p; - - recalculate_cpuid_policy(d); - - return 0; } void guest_cpuid(const struct vcpu *v, uint32_t leaf, diff --git a/xen/arch/x86/include/asm/cpuid.h b/xen/arch/x86/include/asm/cpuid.h index 46904061d0ef..9c3637549a10 100644 --- a/xen/arch/x86/include/asm/cpuid.h +++ b/xen/arch/x86/include/asm/cpuid.h @@ -59,6 +59,9 @@ bool recheck_cpu_features(unsigned int cpu); /* Allocate and initialise a CPUID policy suitable for the domain. */ int init_domain_cpuid_policy(struct domain *d); +/* Apply dom0-specific tweaks to the CPUID policy. */ +void init_dom0_cpuid_policy(struct domain *d); + /* Clamp the CPUID policy to reality. */ void recalculate_cpuid_policy(struct domain *d); diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index f40a9fe5d351..e716005145d3 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -772,6 +772,7 @@ static struct domain *__init create_dom0(const module_t *image, }; struct domain *d; char *cmdline; + domid_t domid; if ( opt_dom0_pvh ) { @@ -786,10 +787,16 @@ static struct domain *__init create_dom0(const module_t *image, if ( iommu_enabled ) dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu; - /* Create initial domain 0. */ - d = domain_create(get_initial_domain_id(), &dom0_cfg, !pv_shim); - if ( IS_ERR(d) || (alloc_dom0_vcpu0(d) == NULL) ) - panic("Error creating domain 0\n"); + /* Create initial domain. Not d0 for pvshim. */ + domid = get_initial_domain_id(); + d = domain_create(domid, &dom0_cfg, !pv_shim); + if ( IS_ERR(d) ) + panic("Error creating d%u: %ld\n", domid, PTR_ERR(d)); + + init_dom0_cpuid_policy(d); + + if ( alloc_dom0_vcpu0(d) == NULL ) + panic("Error creating d%uv0\n", domid); /* Grab the DOM0 command line. */ cmdline = image->string ? __va(image->string) : NULL;
To implement dom0-cpuid= support, the special cases would need extending. However there is already a problem with late hwdom where the special cases override toolstack settings, which is unintended and poor behaviour. Introduce a new init_dom0_cpuid_policy() for the purpose, moving the ITSC and ARCH_CAPS logic. The is_hardware_domain() can be dropped, and for now there is no need to rerun recalculate_cpuid_policy(); this is a relatively expensive operation, and will become more-so over time. Rearrange the logic in create_dom0() to make room for a call to init_dom0_cpuid_policy(). The AMX plans for having variable sized XSAVE states require that modifications to the policy happen before vCPUs are created. Additionally, factor out domid into a variable so we can be slightly more correct in the case of a failure, and also print the error from domain_create(). This will at least help distinguish -EINVAL from -ENOMEM. No practical change in behaviour. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> v2: * New It is slightly weird now having CPUID split into two helpers, while the MSR side of things remains as a special case. It doesn't actually matter right now, and it is going to have to change anyway in due course, so I've decided to go with the simple option for now. --- xen/arch/x86/cpuid.c | 25 +++++++++++++++---------- xen/arch/x86/include/asm/cpuid.h | 3 +++ xen/arch/x86/setup.c | 15 +++++++++++---- 3 files changed, 29 insertions(+), 14 deletions(-)