| Message ID | 1474991845-27962-18-git-send-email-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 27.09.16 at 17:57, <roger.pau@citrix.com> wrote: > The logic used to setup the CPUID leaves is extremely simplistic (and > probably wrong for hardware different than mine). I'm not sure what's the > best way to deal with this, the code that currently sets the CPUID leaves > for HVM guests lives in libxc, maybe moving it xen/common would be better? Yeah, a pre-populated array of leaves certainly won't do. > + rc = arch_set_info_hvm_guest(v, &cpu_ctx); > + if ( rc ) > + { > + printk("Unable to setup Dom0 BSP context: %d\n", rc); > + return rc; > + } > + clear_bit(_VPF_down, &v->pause_flags); Even if it may not matter right away, I think you want to clear this flag later, after having completed all setup. > + for ( i = 0; i < ARRAY_SIZE(cpuid_leaves); i++ ) > + { > + d->arch.cpuids[i].input[0] = cpuid_leaves[i].index; > + d->arch.cpuids[i].input[1] = cpuid_leaves[i].count; > + if ( d->arch.cpuids[i].input[1] == XEN_CPUID_INPUT_UNUSED ) > + cpuid(d->arch.cpuids[i].input[0], &d->arch.cpuids[i].eax, > + &d->arch.cpuids[i].ebx, &d->arch.cpuids[i].ecx, > + &d->arch.cpuids[i].edx); > + else > + cpuid_count(d->arch.cpuids[i].input[0], d->arch.cpuids[i].input[1], > + &d->arch.cpuids[i].eax, &d->arch.cpuids[i].ebx, > + &d->arch.cpuids[i].ecx, &d->arch.cpuids[i].edx); Why this if/else? It is always fine to use cpuid_count(). Jan
On Thu, Oct 06, 2016 at 09:20:07AM -0600, Jan Beulich wrote: > >>> On 27.09.16 at 17:57, <roger.pau@citrix.com> wrote: > > The logic used to setup the CPUID leaves is extremely simplistic (and > > probably wrong for hardware different than mine). I'm not sure what's the > > best way to deal with this, the code that currently sets the CPUID leaves > > for HVM guests lives in libxc, maybe moving it xen/common would be better? > > Yeah, a pre-populated array of leaves certainly won't do. This is what current HVM guests use, and TBH, I would prefer to don't diverge from HVM. Would it make sense to leave this as-is, until all this cpuid stuff is fixed? (IIRC Andrew is still working on this). > > + rc = arch_set_info_hvm_guest(v, &cpu_ctx); > > + if ( rc ) > > + { > > + printk("Unable to setup Dom0 BSP context: %d\n", rc); > > + return rc; > > + } > > + clear_bit(_VPF_down, &v->pause_flags); > > Even if it may not matter right away, I think you want to clear this > flag later, after having completed all setup. Right, I've now moved the clear_bit to the end of construct_dom0_hvm. > > + for ( i = 0; i < ARRAY_SIZE(cpuid_leaves); i++ ) > > + { > > + d->arch.cpuids[i].input[0] = cpuid_leaves[i].index; > > + d->arch.cpuids[i].input[1] = cpuid_leaves[i].count; > > + if ( d->arch.cpuids[i].input[1] == XEN_CPUID_INPUT_UNUSED ) > > + cpuid(d->arch.cpuids[i].input[0], &d->arch.cpuids[i].eax, > > + &d->arch.cpuids[i].ebx, &d->arch.cpuids[i].ecx, > > + &d->arch.cpuids[i].edx); > > + else > > + cpuid_count(d->arch.cpuids[i].input[0], d->arch.cpuids[i].input[1], > > + &d->arch.cpuids[i].eax, &d->arch.cpuids[i].ebx, > > + &d->arch.cpuids[i].ecx, &d->arch.cpuids[i].edx); > > Why this if/else? It is always fine to use cpuid_count(). Done, now it's cpuid_count. Roger.
On 12/10/16 12:06, Roger Pau Monne wrote: > On Thu, Oct 06, 2016 at 09:20:07AM -0600, Jan Beulich wrote: >>>>> On 27.09.16 at 17:57, <roger.pau@citrix.com> wrote: >>> The logic used to setup the CPUID leaves is extremely simplistic (and >>> probably wrong for hardware different than mine). I'm not sure what's the >>> best way to deal with this, the code that currently sets the CPUID leaves >>> for HVM guests lives in libxc, maybe moving it xen/common would be better? >> Yeah, a pre-populated array of leaves certainly won't do. > This is what current HVM guests use, and TBH, I would prefer to don't > diverge from HVM. Would it make sense to leave this as-is, until all this > cpuid stuff is fixed? (IIRC Andrew is still working on this). My CPUID work will remove the need for any of this, (and indeed, is a prerequisite for an HVM Control domain to build further HVM domains). At boot, where we currently calculate the featuresets, we will also calculate maximum full policies. domain_create() will clone the appropriate default policy (pv or hvm) as a starting point. A regular domU will have the toolstack optionally reduce the policy via the domctl interface, but in the absence of any changes, the domain will get the maximum supported featureset available on the hardware. ~Andrew
>>> On 12.10.16 at 13:06, <roger.pau@citrix.com> wrote: > On Thu, Oct 06, 2016 at 09:20:07AM -0600, Jan Beulich wrote: >> >>> On 27.09.16 at 17:57, <roger.pau@citrix.com> wrote: >> > The logic used to setup the CPUID leaves is extremely simplistic (and >> > probably wrong for hardware different than mine). I'm not sure what's the >> > best way to deal with this, the code that currently sets the CPUID leaves >> > for HVM guests lives in libxc, maybe moving it xen/common would be better? >> >> Yeah, a pre-populated array of leaves certainly won't do. > > This is what current HVM guests use, and TBH, I would prefer to don't > diverge from HVM. Would it make sense to leave this as-is, until all this > cpuid stuff is fixed? (IIRC Andrew is still working on this). Leaving it as is makes little sense to me. What would make sense is to make Andrew's work a prereq for this patch. Jan
diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c index effebf1..8ea54ae 100644 --- a/xen/arch/x86/domain_build.c +++ b/xen/arch/x86/domain_build.c @@ -40,6 +40,7 @@ #include <public/version.h> #include <public/arch-x86/hvm/start_info.h> +#include <public/hvm/hvm_vcpu.h> static long __initdata dom0_nrpages; static long __initdata dom0_min_nrpages; @@ -2015,6 +2016,101 @@ out: return rc; } +static int __init hvm_setup_cpus(struct domain *d, paddr_t entry, + paddr_t start_info) +{ + vcpu_hvm_context_t cpu_ctx; + struct vcpu *v = d->vcpu[0]; + int cpu, i, rc; + struct { + uint32_t index; + uint32_t count; + } cpuid_leaves[] = { + {0, XEN_CPUID_INPUT_UNUSED}, + {1, XEN_CPUID_INPUT_UNUSED}, + {2, XEN_CPUID_INPUT_UNUSED}, + {4, 0}, + {4, 1}, + {4, 2}, + {4, 3}, + {4, 4}, + {7, 0}, + {0xa, XEN_CPUID_INPUT_UNUSED}, + {0xd, 0}, + {0x80000000, XEN_CPUID_INPUT_UNUSED}, + {0x80000001, XEN_CPUID_INPUT_UNUSED}, + {0x80000002, XEN_CPUID_INPUT_UNUSED}, + {0x80000003, XEN_CPUID_INPUT_UNUSED}, + {0x80000004, XEN_CPUID_INPUT_UNUSED}, + {0x80000005, XEN_CPUID_INPUT_UNUSED}, + {0x80000006, XEN_CPUID_INPUT_UNUSED}, + {0x80000007, XEN_CPUID_INPUT_UNUSED}, + {0x80000008, XEN_CPUID_INPUT_UNUSED}, + }; + + printk("** Setting up BSP/APs **\n"); + + cpu = v->processor; + for ( i = 1; i < d->max_vcpus; i++ ) + { + cpu = cpumask_cycle(cpu, &dom0_cpus); + setup_dom0_vcpu(d, i, cpu); + } + + memset(&cpu_ctx, 0, sizeof(cpu_ctx)); + + cpu_ctx.mode = VCPU_HVM_MODE_32B; + + cpu_ctx.cpu_regs.x86_32.ebx = start_info; + cpu_ctx.cpu_regs.x86_32.eip = entry; + cpu_ctx.cpu_regs.x86_32.cr0 = X86_CR0_PE | X86_CR0_ET; + + cpu_ctx.cpu_regs.x86_32.cs_limit = ~0u; + cpu_ctx.cpu_regs.x86_32.ds_limit = ~0u; + cpu_ctx.cpu_regs.x86_32.ss_limit = ~0u; + cpu_ctx.cpu_regs.x86_32.tr_limit = 0x67; + cpu_ctx.cpu_regs.x86_32.cs_ar = 0xc9b; + cpu_ctx.cpu_regs.x86_32.ds_ar = 0xc93; + cpu_ctx.cpu_regs.x86_32.ss_ar = 0xc93; + cpu_ctx.cpu_regs.x86_32.tr_ar = 0x8b; + + rc = arch_set_info_hvm_guest(v, &cpu_ctx); + if ( rc ) + { + printk("Unable to setup Dom0 BSP context: %d\n", rc); + return rc; + } + clear_bit(_VPF_down, &v->pause_flags); + + for ( i = 0; i < ARRAY_SIZE(cpuid_leaves); i++ ) + { + d->arch.cpuids[i].input[0] = cpuid_leaves[i].index; + d->arch.cpuids[i].input[1] = cpuid_leaves[i].count; + if ( d->arch.cpuids[i].input[1] == XEN_CPUID_INPUT_UNUSED ) + cpuid(d->arch.cpuids[i].input[0], &d->arch.cpuids[i].eax, + &d->arch.cpuids[i].ebx, &d->arch.cpuids[i].ecx, + &d->arch.cpuids[i].edx); + else + cpuid_count(d->arch.cpuids[i].input[0], d->arch.cpuids[i].input[1], + &d->arch.cpuids[i].eax, &d->arch.cpuids[i].ebx, + &d->arch.cpuids[i].ecx, &d->arch.cpuids[i].edx); + /* XXX: we need to do much more filtering here. */ + if ( d->arch.cpuids[i].input[0] == 1 ) + d->arch.cpuids[i].ecx &= ~X86_FEATURE_VMX; + } + + rc = setup_permissions(d); + if ( rc ) + { + panic("Unable to setup Dom0 permissions: %d\n", rc); + return rc; + } + + update_domain_wallclock_time(d); + + return 0; +} + static int __init construct_dom0_hvm(struct domain *d, const module_t *image, unsigned long image_headroom, module_t *initrd, @@ -2049,6 +2145,13 @@ static int __init construct_dom0_hvm(struct domain *d, const module_t *image, return rc; } + rc = hvm_setup_cpus(d, entry, start_info); + if ( rc ) + { + printk("Failed to setup Dom0 CPUs: %d\n", rc); + return rc; + } + return 0; }
Initialize Dom0 BSP/APs and setup the memory and IO permissions. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> --- The logic used to setup the CPUID leaves is extremely simplistic (and probably wrong for hardware different than mine). I'm not sure what's the best way to deal with this, the code that currently sets the CPUID leaves for HVM guests lives in libxc, maybe moving it xen/common would be better? --- xen/arch/x86/domain_build.c | 103 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+)