| Message ID | 1318911341-10890-1-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
On 17.10.2011, at 21:15, David Gibson wrote: > Currently, when KVM is enabled, the pseries machine checks if the host > CPU supports VMX, VSX and/or DFP instructions and advertises > accordingly in the guest device tree. It does this regardless of what > CPU is selected on the command line. On the other hand, when in TCG > mode, it never advertises any of these facilities, even basic VMX > (Altivec) which is supported in TCG. > > Now that we have a -cpu host option for ppc, it is fairly > straightforward to fix both problems. This patch changes the -cpu > host code to override the basic cpu spec derived from the PVR with > information queried from the host avout VMX, VSX and DFP capability. > The pseries code then uses the instruction availability advertised in > the cpu state to set the guest device tree correctly for both the KVM > and TCG cases. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/spapr.c | 10 +++++----- > target-ppc/cpu.h | 20 ++++++++++++++++++++ > target-ppc/kvm.c | 23 ++++++++++++++++++++++- > target-ppc/translate_init.c | 18 ++---------------- > 4 files changed, 49 insertions(+), 22 deletions(-) > > diff --git a/hw/spapr.c b/hw/spapr.c > index fecfa4a..26333ac 100644 > --- a/hw/spapr.c > +++ b/hw/spapr.c > @@ -186,8 +186,6 @@ static void *spapr_create_fdt_skel(const char *cpu_model, > 0xffffffff, 0xffffffff}; > uint32_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq() : TIMEBASE_FREQ; > uint32_t cpufreq = kvm_enabled() ? kvmppc_get_clockfreq() : 1000000000; > - uint32_t vmx = kvm_enabled() ? kvmppc_get_vmx() : 0; > - uint32_t dfp = kvm_enabled() ? kvmppc_get_dfp() : 0; > > if ((index % smt) != 0) { > continue; > @@ -239,15 +237,17 @@ static void *spapr_create_fdt_skel(const char *cpu_model, > * 0 / no property == no vector extensions > * 1 == VMX / Altivec available > * 2 == VSX available */ > - if (vmx) { > + if (env->insns_flags & PPC_ALTIVEC) { > + uint32_t vmx = (env->insns_flags2 & PPC2_VSX) ? 2 : 1; > + > _FDT((fdt_property_cell(fdt, "ibm,vmx", vmx))); > } > > /* Advertise DFP (Decimal Floating Point) if available > * 0 / no property == no DFP > * 1 == DFP available */ > - if (dfp) { > - _FDT((fdt_property_cell(fdt, "ibm,dfp", dfp))); > + if (env->insns_flags2 & PPC2_DFP) { > + _FDT((fdt_property_cell(fdt, "ibm,dfp", 1))); > } > > _FDT((fdt_end_node(fdt))); > diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h > index 8e5c85c..d177bd4 100644 > --- a/target-ppc/cpu.h > +++ b/target-ppc/cpu.h > @@ -858,6 +858,22 @@ enum { > /* The whole PowerPC CPU context */ > #define NB_MMU_MODES 3 > > +struct ppc_def_t { > + const char *name; > + uint32_t pvr; > + uint32_t svr; > + uint64_t insns_flags; > + uint64_t insns_flags2; > + uint64_t msr_mask; > + powerpc_mmu_t mmu_model; > + powerpc_excp_t excp_model; > + powerpc_input_t bus_model; > + uint32_t flags; > + int bfd_mach; > + void (*init_proc)(CPUPPCState *env); > + int (*check_pow)(CPUPPCState *env); > +}; > + > struct CPUPPCState { > /* First are the most commonly used resources > * during translated code execution > @@ -1844,6 +1860,10 @@ enum { > > /* BookE 2.06 PowerPC specification */ > PPC2_BOOKE206 = 0x0000000000000001ULL, > + /* VSX (extensions to Altivec / VMX) */ > + PPC2_VSX = 0x0000000000000002ULL, > + /* Decimal Floating Point (DFP) */ > + PPC2_DFP = 0x0000000000000004ULL, > }; > > /*****************************************************************************/ > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c > index 430558b..3d12c5c 100644 > --- a/target-ppc/kvm.c > +++ b/target-ppc/kvm.c > @@ -887,14 +887,35 @@ static inline uint32_t mfpvr(void) > return pvr; > } > > +static void alter_insns(uint64_t *word, uint64_t flags, bool on) > +{ > + if (on) { > + *word |= flags; > + } else { > + *word &= ~flags; > + } > +} > + > const ppc_def_t *kvmppc_host_cpu_def(void) > { > uint32_t host_pvr = mfpvr(); > const ppc_def_t *base_spec; > + ppc_def_t *spec; > + uint32_t vmx = kvmppc_get_vmx(); > + uint32_t dfp = kvmppc_get_dfp(); > > base_spec = ppc_find_by_pvr(host_pvr); > > - return base_spec; > + spec = g_malloc0(sizeof(*spec)); > + memcpy(spec, base_spec, sizeof(*spec)); > + > + /* Now fix up the spec with information we can query from the host */ > + > + alter_insns(&spec->insns_flags, PPC_ALTIVEC, vmx > 0); > + alter_insns(&spec->insns_flags2, PPC2_VSX, vmx > 1); > + alter_insns(&spec->insns_flags2, PPC2_DFP, dfp); The logic is reversed. We always want the CPU description to contain all the feature bits that really describes what the CPU looks like. Then we can mask it for KVM and TCG to make sure we only expose to the guest what the host is actually capable of. > + > + return spec; > } > > bool kvm_arch_stop_on_emulation_error(CPUState *env) > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c > index 7de097d..c954192 100644 > --- a/target-ppc/translate_init.c > +++ b/target-ppc/translate_init.c > @@ -34,22 +34,6 @@ > #define TODO_USER_ONLY 1 > #endif > > -struct ppc_def_t { > - const char *name; > - uint32_t pvr; > - uint32_t svr; > - uint64_t insns_flags; > - uint64_t insns_flags2; > - uint64_t msr_mask; > - powerpc_mmu_t mmu_model; > - powerpc_excp_t excp_model; > - powerpc_input_t bus_model; > - uint32_t flags; > - int bfd_mach; > - void (*init_proc)(CPUPPCState *env); > - int (*check_pow)(CPUPPCState *env); > -}; > - > /* For user-mode emulation, we don't emulate any IRQ controller */ > #if defined(CONFIG_USER_ONLY) > #define PPC_IRQ_INIT_FN(name) \ > @@ -6535,6 +6519,8 @@ static void init_proc_970MP (CPUPPCState *env) > PPC_64B | PPC_ALTIVEC | \ > PPC_SEGMENT_64B | PPC_SLBI | \ > PPC_POPCNTB | PPC_POPCNTWD) > +/* FIXME: Should also have PPC2_VSX and PPC2_DFP, but we don't > + * implement those in TCG yet */ > #define POWERPC_INSNS2_POWER7 (PPC_NONE) So down here, we would have the flags set, but then mask them out in the TCG case because our TCG backend doesn't support them yet. Alex
On Thu, Oct 20, 2011 at 10:12:51AM -0700, Alexander Graf wrote: > On 17.10.2011, at 21:15, David Gibson wrote: [snip] > > const ppc_def_t *kvmppc_host_cpu_def(void) > > { > > uint32_t host_pvr = mfpvr(); > > const ppc_def_t *base_spec; > > + ppc_def_t *spec; > > + uint32_t vmx = kvmppc_get_vmx(); > > + uint32_t dfp = kvmppc_get_dfp(); > > > > base_spec = ppc_find_by_pvr(host_pvr); > > > > - return base_spec; > > + spec = g_malloc0(sizeof(*spec)); > > + memcpy(spec, base_spec, sizeof(*spec)); > > + > > + /* Now fix up the spec with information we can query from the host */ > > + > > + alter_insns(&spec->insns_flags, PPC_ALTIVEC, vmx > 0); > > + alter_insns(&spec->insns_flags2, PPC2_VSX, vmx > 1); > > + alter_insns(&spec->insns_flags2, PPC2_DFP, dfp); > > The logic is reversed. We always want the CPU description to contain > all the feature bits that really describes what the CPU looks > like. Then we can mask it for KVM and TCG to make sure we only > expose to the guest what the host is actually capable of. Well, this is executed only in the -cpu host case, and only modifies the special "host" copy of the cpu spec. The reasoning is that querying the host directly is a _better_ source of definitive information about the cpu in this case, than what we think based on the PVR. For example the host may have a CPU that is capable of VSX or DFP, but where it is disabled in the firmware for some reason. This would become even more important if we implement approximate PVR matching again for -cpu host (in a not so roken way this time, of course). > > + > > + return spec; > > } > > > > bool kvm_arch_stop_on_emulation_error(CPUState *env) > > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c > > index 7de097d..c954192 100644 > > --- a/target-ppc/translate_init.c > > +++ b/target-ppc/translate_init.c > > @@ -34,22 +34,6 @@ > > #define TODO_USER_ONLY 1 > > #endif > > > > -struct ppc_def_t { > > - const char *name; > > - uint32_t pvr; > > - uint32_t svr; > > - uint64_t insns_flags; > > - uint64_t insns_flags2; > > - uint64_t msr_mask; > > - powerpc_mmu_t mmu_model; > > - powerpc_excp_t excp_model; > > - powerpc_input_t bus_model; > > - uint32_t flags; > > - int bfd_mach; > > - void (*init_proc)(CPUPPCState *env); > > - int (*check_pow)(CPUPPCState *env); > > -}; > > - > > /* For user-mode emulation, we don't emulate any IRQ controller */ > > #if defined(CONFIG_USER_ONLY) > > #define PPC_IRQ_INIT_FN(name) \ > > @@ -6535,6 +6519,8 @@ static void init_proc_970MP (CPUPPCState *env) > > PPC_64B | PPC_ALTIVEC | \ > > PPC_SEGMENT_64B | PPC_SLBI | \ > > PPC_POPCNTB | PPC_POPCNTWD) > > +/* FIXME: Should also have PPC2_VSX and PPC2_DFP, but we don't > > + * implement those in TCG yet */ > > #define POWERPC_INSNS2_POWER7 (PPC_NONE) > > So down here, we would have the flags set, but then mask them out in > the TCG case because our TCG backend doesn't support them yet. So, I really don't follow what the logic you want is. It sounds more like what I have already, so I'm not sure how -cpu host comes into this.
On 20.10.2011, at 17:41, David Gibson <david@gibson.dropbear.id.au> wrote: > On Thu, Oct 20, 2011 at 10:12:51AM -0700, Alexander Graf wrote: >> On 17.10.2011, at 21:15, David Gibson wrote: > [snip] >>> const ppc_def_t *kvmppc_host_cpu_def(void) >>> { >>> uint32_t host_pvr = mfpvr(); >>> const ppc_def_t *base_spec; >>> + ppc_def_t *spec; >>> + uint32_t vmx = kvmppc_get_vmx(); >>> + uint32_t dfp = kvmppc_get_dfp(); >>> >>> base_spec = ppc_find_by_pvr(host_pvr); >>> >>> - return base_spec; >>> + spec = g_malloc0(sizeof(*spec)); >>> + memcpy(spec, base_spec, sizeof(*spec)); >>> + >>> + /* Now fix up the spec with information we can query from the host */ >>> + >>> + alter_insns(&spec->insns_flags, PPC_ALTIVEC, vmx > 0); >>> + alter_insns(&spec->insns_flags2, PPC2_VSX, vmx > 1); >>> + alter_insns(&spec->insns_flags2, PPC2_DFP, dfp); >> >> The logic is reversed. We always want the CPU description to contain >> all the feature bits that really describes what the CPU looks >> like. Then we can mask it for KVM and TCG to make sure we only >> expose to the guest what the host is actually capable of. > > Well, this is executed only in the -cpu host case, and only modifies > the special "host" copy of the cpu spec. The reasoning is that > querying the host directly is a _better_ source of definitive > information about the cpu in this case, than what we think based on > the PVR. For example the host may have a CPU that is capable of VSX > or DFP, but where it is disabled in the firmware for some reason. > This would become even more important if we implement approximate PVR > matching again for -cpu host (in a not so roken way this time, of > course). > >>> + >>> + return spec; >>> } >>> >>> bool kvm_arch_stop_on_emulation_error(CPUState *env) >>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c >>> index 7de097d..c954192 100644 >>> --- a/target-ppc/translate_init.c >>> +++ b/target-ppc/translate_init.c >>> @@ -34,22 +34,6 @@ >>> #define TODO_USER_ONLY 1 >>> #endif >>> >>> -struct ppc_def_t { >>> - const char *name; >>> - uint32_t pvr; >>> - uint32_t svr; >>> - uint64_t insns_flags; >>> - uint64_t insns_flags2; >>> - uint64_t msr_mask; >>> - powerpc_mmu_t mmu_model; >>> - powerpc_excp_t excp_model; >>> - powerpc_input_t bus_model; >>> - uint32_t flags; >>> - int bfd_mach; >>> - void (*init_proc)(CPUPPCState *env); >>> - int (*check_pow)(CPUPPCState *env); >>> -}; >>> - >>> /* For user-mode emulation, we don't emulate any IRQ controller */ >>> #if defined(CONFIG_USER_ONLY) >>> #define PPC_IRQ_INIT_FN(name) \ >>> @@ -6535,6 +6519,8 @@ static void init_proc_970MP (CPUPPCState *env) >>> PPC_64B | PPC_ALTIVEC | \ >>> PPC_SEGMENT_64B | PPC_SLBI | \ >>> PPC_POPCNTB | PPC_POPCNTWD) >>> +/* FIXME: Should also have PPC2_VSX and PPC2_DFP, but we don't >>> + * implement those in TCG yet */ >>> #define POWERPC_INSNS2_POWER7 (PPC_NONE) >> >> So down here, we would have the flags set, but then mask them out in >> the TCG case because our TCG backend doesn't support them yet. > > So, I really don't follow what the logic you want is. It sounds more > like what I have already, so I'm not sure how -cpu host comes into > this. Well, I want something very simple, layered: -cpu host only searches for pvr matches and selects a different CPU type based on this We have 2 masks of available flags: TCG emulatable flags and KVM virtualizable flags. The KVM flags need to be generated dynamically, from the host dt for now. TCG flags are constant. Then we always AND the inst feature bits with the mask. This tells every other layer what features are available. That way even running -cpu G5 on a p7 works properly by not exposing DFP for example. Based on the inst feature bits we now expose features in the guest dt. Simple, eh? :) Alex >
On Thu, Oct 20, 2011 at 07:40:00PM -0700, Alexander Graf wrote: > On 20.10.2011, at 17:41, David Gibson <david@gibson.dropbear.id.au> wrote: > > On Thu, Oct 20, 2011 at 10:12:51AM -0700, Alexander Graf wrote: > >> On 17.10.2011, at 21:15, David Gibson wrote: [snip] > > So, I really don't follow what the logic you want is. It sounds more > > like what I have already, so I'm not sure how -cpu host comes into > > this. > > Well, I want something very simple, layered: > > -cpu host only searches for pvr matches and selects a different CPU > -type based on this Hrm, ok, well I can do this if you like, but note that this is quite different from how -cpu host behaves on x86. There it builds the CPU spec from scratch based on querying the host cpuid, rather than selecting from an existing list of cpus. I selected from the existing table based on host PVR because that was the easiest source for some of the info in the cpu_spec, but my intention was that anything we _can_ query directly from the host would override the table. It seems to be your approach is giving up on the possibility of allowing -cpu host to work (and give you full access to the host features) when qemu doesn't recognize the precise PVR of the host cpu. This gets further complicated in the case of the w-i-p patch I have to properly advertise page sizes, where it's not just presence or absence of a feature, but the specific SLB and HPTE encodings must be advertised to the guest. > We have 2 masks of available flags: TCG emulatable flags and KVM > virtualizable flags. The KVM flags need to be generated dynamically, > from the host dt for now. TCG flags are constant. > > Then we always AND the inst feature bits with the mask. This tells > every other layer what features are available. That way even running > -cpu G5 on a p7 works properly by not exposing DFP for example. That case was already fine. Are you suggesting doing the AND in the per-machine code (so, as we build the guest dt in the spapr case) or when we build the env->insn_flags from the spec->insn_flags? > > Based on the inst feature bits we now expose features in the guest dt. > > > Simple, eh? :) > > Alex > > > >
On 20.10.2011, at 22:06, David Gibson wrote: > On Thu, Oct 20, 2011 at 07:40:00PM -0700, Alexander Graf wrote: >> On 20.10.2011, at 17:41, David Gibson <david@gibson.dropbear.id.au> wrote: >>> On Thu, Oct 20, 2011 at 10:12:51AM -0700, Alexander Graf wrote: >>>> On 17.10.2011, at 21:15, David Gibson wrote: > [snip] >>> So, I really don't follow what the logic you want is. It sounds more >>> like what I have already, so I'm not sure how -cpu host comes into >>> this. >> >> Well, I want something very simple, layered: >> >> -cpu host only searches for pvr matches and selects a different CPU >> -type based on this > > Hrm, ok, well I can do this if you like, but note that this is quite > different from how -cpu host behaves on x86. There it builds the CPU > spec from scratch based on querying the host cpuid, rather than > selecting from an existing list of cpus. I selected from the existing > table based on host PVR because that was the easiest source for some > of the info in the cpu_spec, but my intention was that anything we > _can_ query directly from the host would override the table. > > It seems to be your approach is giving up on the possibility of > allowing -cpu host to work (and give you full access to the host > features) when qemu doesn't recognize the precise PVR of the host cpu. I disagree :). This is what x86 does: * -cpu host fetches CPUID info from host, puts it into vcpu * vcpu CPUID info gets ANDed with KVM capability CPUIDs I want basically the same thing. I want to have 2 different layers for 2 different semantics. One for what the host CPU would be able to do and one for what we can emulate, and two different steps to ensure control over them. The thing I think I'm apparently not bringing over yet is that I'm more than happy to get rid of the PVR searching step for -cpu host and instead use a full host capability inquiry mechanism. But that inquiry should indicate what the host CPU can do. It has nothing to do with KVM yet. The masking with KVM capabilities should be the next separate step. My goal is really to separate different layers into actual different layers :). > This gets further complicated in the case of the w-i-p patch I have to > properly advertise page sizes, where it's not just presence or absence > of a feature, but the specific SLB and HPTE encodings must be > advertised to the guest. Yup, so we'd read out the host dt to find the host possible encodings (probably a bad idea, but that's a different story) and then ask KVM what encodings it supports and expose the ANDed product of them to the guest. > >> We have 2 masks of available flags: TCG emulatable flags and KVM >> virtualizable flags. The KVM flags need to be generated dynamically, >> from the host dt for now. TCG flags are constant. >> >> Then we always AND the inst feature bits with the mask. This tells >> every other layer what features are available. That way even running >> -cpu G5 on a p7 works properly by not exposing DFP for example. > > That case was already fine. > > Are you suggesting doing the AND in the per-machine code (so, as we > build the guest dt in the spapr case) or when we build the env->insn_flags > from the spec->insn_flags? I suggest doing that in translate_init.c where we actually build the env->insn_flags from the spec. Alex
On Thu, Oct 20, 2011 at 11:49:40PM -0700, Alexander Graf wrote: > > On 20.10.2011, at 22:06, David Gibson wrote: > > > On Thu, Oct 20, 2011 at 07:40:00PM -0700, Alexander Graf wrote: > >> On 20.10.2011, at 17:41, David Gibson <david@gibson.dropbear.id.au> wrote: > >>> On Thu, Oct 20, 2011 at 10:12:51AM -0700, Alexander Graf wrote: > >>>> On 17.10.2011, at 21:15, David Gibson wrote: > > [snip] > >>> So, I really don't follow what the logic you want is. It sounds more > >>> like what I have already, so I'm not sure how -cpu host comes into > >>> this. > >> > >> Well, I want something very simple, layered: > >> > >> -cpu host only searches for pvr matches and selects a different CPU > >> -type based on this > > > > Hrm, ok, well I can do this if you like, but note that this is quite > > different from how -cpu host behaves on x86. There it builds the CPU > > spec from scratch based on querying the host cpuid, rather than > > selecting from an existing list of cpus. I selected from the existing > > table based on host PVR because that was the easiest source for some > > of the info in the cpu_spec, but my intention was that anything we > > _can_ query directly from the host would override the table. > > > > It seems to be your approach is giving up on the possibility of > > allowing -cpu host to work (and give you full access to the host > > features) when qemu doesn't recognize the precise PVR of the host cpu. > > I disagree :). This is what x86 does: > > * -cpu host fetches CPUID info from host, puts it into vcpu > * vcpu CPUID info gets ANDed with KVM capability CPUIDs > > I want basically the same thing. I want to have 2 different layers > for 2 different semantics. One for what the host CPU would be able > to do and one for what we can emulate, and two different steps to > ensure control over them. > > The thing I think I'm apparently not bringing over yet is that I'm > more than happy to get rid of the PVR searching step for -cpu host > and instead use a full host capability inquiry mechanism. But that > inquiry should indicate what the host CPU can do. It has nothing to > do with KVM yet. The masking with KVM capabilities should be the > next separate step. > > My goal is really to separate different layers into actual different > layers :). Hrm. I think I see what you're getting at. Although nothing in that patch is about kvm capabilities - it's all about working out what the host's cpu can do. > > This gets further complicated in the case of the w-i-p patch I have to > > properly advertise page sizes, where it's not just presence or absence > > of a feature, but the specific SLB and HPTE encodings must be > > advertised to the guest. > > Yup, so we'd read out the host dt to find the host possible > encodings (probably a bad idea, but that's a different story) Um, a different story perhaps, but one I kind of need an answer to in the near future... I can query the host cpu's page sizes easily enough, but I'm really not sure where this should be stashed before filtering as suggested below. > and > then ask KVM what encodings it supports and expose the ANDed product > of them to the guest. > > > > >> We have 2 masks of available flags: TCG emulatable flags and KVM > >> virtualizable flags. The KVM flags need to be generated dynamically, > >> from the host dt for now. TCG flags are constant. > >> > >> Then we always AND the inst feature bits with the mask. This tells > >> every other layer what features are available. That way even running > >> -cpu G5 on a p7 works properly by not exposing DFP for example. > > > > That case was already fine. > > > > Are you suggesting doing the AND in the per-machine code (so, as we > > build the guest dt in the spapr case) or when we build the env->insn_flags > > from the spec->insn_flags? > > I suggest doing that in translate_init.c where we actually build the > env->insn_flags from the spec. Ok, that makes sense.
On 23.10.2011, at 22:29, David Gibson wrote: > On Thu, Oct 20, 2011 at 11:49:40PM -0700, Alexander Graf wrote: >> >> On 20.10.2011, at 22:06, David Gibson wrote: >> >>> On Thu, Oct 20, 2011 at 07:40:00PM -0700, Alexander Graf wrote: >>>> On 20.10.2011, at 17:41, David Gibson <david@gibson.dropbear.id.au> wrote: >>>>> On Thu, Oct 20, 2011 at 10:12:51AM -0700, Alexander Graf wrote: >>>>>> On 17.10.2011, at 21:15, David Gibson wrote: >>> [snip] >>>>> So, I really don't follow what the logic you want is. It sounds more >>>>> like what I have already, so I'm not sure how -cpu host comes into >>>>> this. >>>> >>>> Well, I want something very simple, layered: >>>> >>>> -cpu host only searches for pvr matches and selects a different CPU >>>> -type based on this >>> >>> Hrm, ok, well I can do this if you like, but note that this is quite >>> different from how -cpu host behaves on x86. There it builds the CPU >>> spec from scratch based on querying the host cpuid, rather than >>> selecting from an existing list of cpus. I selected from the existing >>> table based on host PVR because that was the easiest source for some >>> of the info in the cpu_spec, but my intention was that anything we >>> _can_ query directly from the host would override the table. >>> >>> It seems to be your approach is giving up on the possibility of >>> allowing -cpu host to work (and give you full access to the host >>> features) when qemu doesn't recognize the precise PVR of the host cpu. >> >> I disagree :). This is what x86 does: >> >> * -cpu host fetches CPUID info from host, puts it into vcpu >> * vcpu CPUID info gets ANDed with KVM capability CPUIDs >> >> I want basically the same thing. I want to have 2 different layers >> for 2 different semantics. One for what the host CPU would be able >> to do and one for what we can emulate, and two different steps to >> ensure control over them. >> >> The thing I think I'm apparently not bringing over yet is that I'm >> more than happy to get rid of the PVR searching step for -cpu host >> and instead use a full host capability inquiry mechanism. But that >> inquiry should indicate what the host CPU can do. It has nothing to >> do with KVM yet. The masking with KVM capabilities should be the >> next separate step. >> >> My goal is really to separate different layers into actual different >> layers :). > > Hrm. I think I see what you're getting at. Although nothing in that > patch is about kvm capabilities - it's all about working out what the > host's cpu can do. Reading through the patch again I think I see your point now :). Yes, the kvmppc_host_cpu_def function only tries to fetch the host CPU capabilities. So yes, there is basically only the masking part with what we can actually virtualize missing. But for now we can just assume that every feature the host CPU supports is available. I'll apply your patch for now, as it certainly is better than what we had before. > >>> This gets further complicated in the case of the w-i-p patch I have to >>> properly advertise page sizes, where it's not just presence or absence >>> of a feature, but the specific SLB and HPTE encodings must be >>> advertised to the guest. >> >> Yup, so we'd read out the host dt to find the host possible >> encodings (probably a bad idea, but that's a different story) > > Um, a different story perhaps, but one I kind of need an answer to in > the near future... I can query the host cpu's page sizes easily > enough, but I'm really not sure where this should be stashed before > filtering as suggested below. Page sizes are usually powers of 2, so we should be ok with just having a bitmap there with each bit meaning 1 << (n + 12). Alex
On 24.10.2011, at 10:25, Alexander Graf wrote: > > On 23.10.2011, at 22:29, David Gibson wrote: > >> On Thu, Oct 20, 2011 at 11:49:40PM -0700, Alexander Graf wrote: >>> >>> On 20.10.2011, at 22:06, David Gibson wrote: >>> >>>> On Thu, Oct 20, 2011 at 07:40:00PM -0700, Alexander Graf wrote: >>>>> On 20.10.2011, at 17:41, David Gibson <david@gibson.dropbear.id.au> wrote: >>>>>> On Thu, Oct 20, 2011 at 10:12:51AM -0700, Alexander Graf wrote: >>>>>>> On 17.10.2011, at 21:15, David Gibson wrote: >>>> [snip] >>>>>> So, I really don't follow what the logic you want is. It sounds more >>>>>> like what I have already, so I'm not sure how -cpu host comes into >>>>>> this. >>>>> >>>>> Well, I want something very simple, layered: >>>>> >>>>> -cpu host only searches for pvr matches and selects a different CPU >>>>> -type based on this >>>> >>>> Hrm, ok, well I can do this if you like, but note that this is quite >>>> different from how -cpu host behaves on x86. There it builds the CPU >>>> spec from scratch based on querying the host cpuid, rather than >>>> selecting from an existing list of cpus. I selected from the existing >>>> table based on host PVR because that was the easiest source for some >>>> of the info in the cpu_spec, but my intention was that anything we >>>> _can_ query directly from the host would override the table. >>>> >>>> It seems to be your approach is giving up on the possibility of >>>> allowing -cpu host to work (and give you full access to the host >>>> features) when qemu doesn't recognize the precise PVR of the host cpu. >>> >>> I disagree :). This is what x86 does: >>> >>> * -cpu host fetches CPUID info from host, puts it into vcpu >>> * vcpu CPUID info gets ANDed with KVM capability CPUIDs >>> >>> I want basically the same thing. I want to have 2 different layers >>> for 2 different semantics. One for what the host CPU would be able >>> to do and one for what we can emulate, and two different steps to >>> ensure control over them. >>> >>> The thing I think I'm apparently not bringing over yet is that I'm >>> more than happy to get rid of the PVR searching step for -cpu host >>> and instead use a full host capability inquiry mechanism. But that >>> inquiry should indicate what the host CPU can do. It has nothing to >>> do with KVM yet. The masking with KVM capabilities should be the >>> next separate step. >>> >>> My goal is really to separate different layers into actual different >>> layers :). >> >> Hrm. I think I see what you're getting at. Although nothing in that >> patch is about kvm capabilities - it's all about working out what the >> host's cpu can do. > > Reading through the patch again I think I see your point now :). Yes, the kvmppc_host_cpu_def function only tries to fetch the host CPU capabilities. > > So yes, there is basically only the masking part with what we can actually virtualize missing. But for now we can just assume that every feature the host CPU supports is available. > > I'll apply your patch for now, as it certainly is better than what we had before. This breaks on 970mp (PowerStation). kvmppc_get_vmx returns -1 because ibm,vmx doesn't exist in the host dt, but the CPU still supports Altivec. Any alternative way to enumerate VMX availability? Alex
On 24.10.2011, at 10:55, Alexander Graf wrote: > > On 24.10.2011, at 10:25, Alexander Graf wrote: > >> >> On 23.10.2011, at 22:29, David Gibson wrote: >> >>> On Thu, Oct 20, 2011 at 11:49:40PM -0700, Alexander Graf wrote: >>>> >>>> On 20.10.2011, at 22:06, David Gibson wrote: >>>> >>>>> On Thu, Oct 20, 2011 at 07:40:00PM -0700, Alexander Graf wrote: >>>>>> On 20.10.2011, at 17:41, David Gibson <david@gibson.dropbear.id.au> wrote: >>>>>>> On Thu, Oct 20, 2011 at 10:12:51AM -0700, Alexander Graf wrote: >>>>>>>> On 17.10.2011, at 21:15, David Gibson wrote: >>>>> [snip] >>>>>>> So, I really don't follow what the logic you want is. It sounds more >>>>>>> like what I have already, so I'm not sure how -cpu host comes into >>>>>>> this. >>>>>> >>>>>> Well, I want something very simple, layered: >>>>>> >>>>>> -cpu host only searches for pvr matches and selects a different CPU >>>>>> -type based on this >>>>> >>>>> Hrm, ok, well I can do this if you like, but note that this is quite >>>>> different from how -cpu host behaves on x86. There it builds the CPU >>>>> spec from scratch based on querying the host cpuid, rather than >>>>> selecting from an existing list of cpus. I selected from the existing >>>>> table based on host PVR because that was the easiest source for some >>>>> of the info in the cpu_spec, but my intention was that anything we >>>>> _can_ query directly from the host would override the table. >>>>> >>>>> It seems to be your approach is giving up on the possibility of >>>>> allowing -cpu host to work (and give you full access to the host >>>>> features) when qemu doesn't recognize the precise PVR of the host cpu. >>>> >>>> I disagree :). This is what x86 does: >>>> >>>> * -cpu host fetches CPUID info from host, puts it into vcpu >>>> * vcpu CPUID info gets ANDed with KVM capability CPUIDs >>>> >>>> I want basically the same thing. I want to have 2 different layers >>>> for 2 different semantics. One for what the host CPU would be able >>>> to do and one for what we can emulate, and two different steps to >>>> ensure control over them. >>>> >>>> The thing I think I'm apparently not bringing over yet is that I'm >>>> more than happy to get rid of the PVR searching step for -cpu host >>>> and instead use a full host capability inquiry mechanism. But that >>>> inquiry should indicate what the host CPU can do. It has nothing to >>>> do with KVM yet. The masking with KVM capabilities should be the >>>> next separate step. >>>> >>>> My goal is really to separate different layers into actual different >>>> layers :). >>> >>> Hrm. I think I see what you're getting at. Although nothing in that >>> patch is about kvm capabilities - it's all about working out what the >>> host's cpu can do. >> >> Reading through the patch again I think I see your point now :). Yes, the kvmppc_host_cpu_def function only tries to fetch the host CPU capabilities. >> >> So yes, there is basically only the masking part with what we can actually virtualize missing. But for now we can just assume that every feature the host CPU supports is available. >> >> I'll apply your patch for now, as it certainly is better than what we had before. > > This breaks on 970mp (PowerStation). kvmppc_get_vmx returns -1 because ibm,vmx doesn't exist in the host dt, but the CPU still supports Altivec. > > Any alternative way to enumerate VMX availability? Thinking about it a bit more ... Why do we need to check the host's capability to do VMX/VSX/DFP? Shouldn't the PVR already tell us everything we need to know? We're still missing some way for KVM to tell us what it can virtualize to the guest, but for now we assume that anything we throw at it works anyways. Alex
[snip] > >> Reading through the patch again I think I see your point now :). Yes, the kvmppc_host_cpu_def function only tries to fetch the host CPU capabilities. > >> > >> So yes, there is basically only the masking part with what we can actually virtualize missing. But for now we can just assume that every feature the host CPU supports is available. > >> > >> I'll apply your patch for now, as it certainly is better than what we had before. > > > > This breaks on 970mp (PowerStation). kvmppc_get_vmx returns -1 because ibm,vmx doesn't exist in the host dt, but the CPU still supports Altivec. > > > > Any alternative way to enumerate VMX availability? > > Thinking about it a bit more ... Why do we need to check the host's > capability to do VMX/VSX/DFP? Shouldn't the PVR already tell us > everything we need to know? Well.. not necessarily. First there's the possibility of a CPU that's theoretically capable of VSX or DFP, but where the administrator has disabled it in firmware. Second, if we add approximate PVR matching (which I'd like to do), then we should trust the host information over the table, because we could actually be dealing with a diffferent revision to the one we got from the table. > We're still missing some way for KVM to tell us what it can > virtualize to the guest, but for now we assume that anything we > throw at it works anyways. Right. I think we'll hneed to do that on a feature by feature basis as we discover things that can't be KVM virtualized. I will send a patch that deals with the masking for features that TCG can't emulate.
On Mon, Oct 24, 2011 at 10:25:26AM -0700, Alexander Graf wrote: > On 23.10.2011, at 22:29, David Gibson wrote: > > On Thu, Oct 20, 2011 at 11:49:40PM -0700, Alexander Graf wrote: [snip] > >>> This gets further complicated in the case of the w-i-p patch I have to > >>> properly advertise page sizes, where it's not just presence or absence > >>> of a feature, but the specific SLB and HPTE encodings must be > >>> advertised to the guest. > >> > >> Yup, so we'd read out the host dt to find the host possible > >> encodings (probably a bad idea, but that's a different story) > > > > Um, a different story perhaps, but one I kind of need an answer to in > > the near future... I can query the host cpu's page sizes easily > > enough, but I'm really not sure where this should be stashed before > > filtering as suggested below. > > Page sizes are usually powers of 2, so we should be ok with just > having a bitmap there with each bit meaning 1 << (n + 12). Not sufficient. Again, it's not just the presence/absence of page sizes I need, but the SLB and HPTE bit encodings. And even if it weren't for that, we need which base page size versus actual page sizes combinations are supported, not just whether a given page size is supported somehow. I did have a draft patch adding more generalized multiple page size support to TCG, which would have provided a solution except that a) I don't really want to finish tha off - still a fair bit of work - just in order to pass through host page sizes and b) I lost the draft in an unfortunate encfs+git corruption incident :(.
On 24.10.2011, at 16:08, David Gibson wrote: > [snip] >>>> Reading through the patch again I think I see your point now :). Yes, the kvmppc_host_cpu_def function only tries to fetch the host CPU capabilities. >>>> >>>> So yes, there is basically only the masking part with what we can actually virtualize missing. But for now we can just assume that every feature the host CPU supports is available. >>>> >>>> I'll apply your patch for now, as it certainly is better than what we had before. >>> >>> This breaks on 970mp (PowerStation). kvmppc_get_vmx returns -1 because ibm,vmx doesn't exist in the host dt, but the CPU still supports Altivec. >>> >>> Any alternative way to enumerate VMX availability? >> >> Thinking about it a bit more ... Why do we need to check the host's >> capability to do VMX/VSX/DFP? Shouldn't the PVR already tell us >> everything we need to know? > > Well.. not necessarily. First there's the possibility of a CPU that's > theoretically capable of VSX or DFP, but where the administrator has > disabled it in firmware. Oh you can disable it in firmware? Then we should take it from the dt if available, yes. > Second, if we add approximate PVR matching > (which I'd like to do), then we should trust the host information over > the table, because we could actually be dealing with a diffferent > revision to the one we got from the table. Yeah, for fuzzy matching we want it. I agree. > >> We're still missing some way for KVM to tell us what it can >> virtualize to the guest, but for now we assume that anything we >> throw at it works anyways. > > Right. I think we'll hneed to do that on a feature by feature basis > as we discover things that can't be KVM virtualized. I will send a > patch that deals with the masking for features that TCG can't emulate. Thanks :). Alex
On Mon, Oct 24, 2011 at 04:43:18PM -0700, Alexander Graf wrote: > > On 24.10.2011, at 16:08, David Gibson wrote: > > > [snip] > >>>> Reading through the patch again I think I see your point now :). Yes, the kvmppc_host_cpu_def function only tries to fetch the host CPU capabilities. > >>>> > >>>> So yes, there is basically only the masking part with what we can actually virtualize missing. But for now we can just assume that every feature the host CPU supports is available. > >>>> > >>>> I'll apply your patch for now, as it certainly is better than what we had before. > >>> > >>> This breaks on 970mp (PowerStation). kvmppc_get_vmx returns -1 because ibm,vmx doesn't exist in the host dt, but the CPU still supports Altivec. > >>> > >>> Any alternative way to enumerate VMX availability? > >> > >> Thinking about it a bit more ... Why do we need to check the host's > >> capability to do VMX/VSX/DFP? Shouldn't the PVR already tell us > >> everything we need to know? > > > > Well.. not necessarily. First there's the possibility of a CPU that's > > theoretically capable of VSX or DFP, but where the administrator has > > disabled it in firmware. > > Oh you can disable it in firmware? Then we should take it from the > dt if available, yes. I think so. I'm not 100% sure on the details. But I believe there's a thing designed for partition migration which essentially goes "don't use this processor feature, because I want to be able to migrate you to an earlier processor sometime". > > Second, if we add approximate PVR matching > > (which I'd like to do), then we should trust the host information over > > the table, because we could actually be dealing with a diffferent > > revision to the one we got from the table. > > Yeah, for fuzzy matching we want it. I agree. > > >> We're still missing some way for KVM to tell us what it can > >> virtualize to the guest, but for now we assume that anything we > >> throw at it works anyways. > > > > Right. I think we'll hneed to do that on a feature by feature basis > > as we discover things that can't be KVM virtualized. I will send a > > patch that deals with the masking for features that TCG can't emulate. > > Thanks :). > > > Alex > >
On 24.10.2011, at 17:09, David Gibson <david@gibson.dropbear.id.au> wrote: > On Mon, Oct 24, 2011 at 04:43:18PM -0700, Alexander Graf wrote: >> >> On 24.10.2011, at 16:08, David Gibson wrote: >> >>> [snip] >>>>>> Reading through the patch again I think I see your point now :). Yes, the kvmppc_host_cpu_def function only tries to fetch the host CPU capabilities. >>>>>> >>>>>> So yes, there is basically only the masking part with what we can actually virtualize missing. But for now we can just assume that every feature the host CPU supports is available. >>>>>> >>>>>> I'll apply your patch for now, as it certainly is better than what we had before. >>>>> >>>>> This breaks on 970mp (PowerStation). kvmppc_get_vmx returns -1 because ibm,vmx doesn't exist in the host dt, but the CPU still supports Altivec. >>>>> >>>>> Any alternative way to enumerate VMX availability? >>>> >>>> Thinking about it a bit more ... Why do we need to check the host's >>>> capability to do VMX/VSX/DFP? Shouldn't the PVR already tell us >>>> everything we need to know? >>> >>> Well.. not necessarily. First there's the possibility of a CPU that's >>> theoretically capable of VSX or DFP, but where the administrator has >>> disabled it in firmware. >> >> Oh you can disable it in firmware? Then we should take it from the >> dt if available, yes. > > I think so. I'm not 100% sure on the details. But I believe there's > a thing designed for partition migration which essentially goes "don't > use this processor feature, because I want to be able to migrate you > to an earlier processor sometime". Good ;). If that one was to simply omit the vmx property, Linux would take the vmx availability from pvr today as well, so we're aligned with what the host OS does now. Alex > >>> Second, if we add approximate PVR matching >>> (which I'd like to do), then we should trust the host information over >>> the table, because we could actually be dealing with a diffferent >>> revision to the one we got from the table. >> >> Yeah, for fuzzy matching we want it. I agree. >> >>>> We're still missing some way for KVM to tell us what it can >>>> virtualize to the guest, but for now we assume that anything we >>>> throw at it works anyways. >>> >>> Right. I think we'll hneed to do that on a feature by feature basis >>> as we discover things that can't be KVM virtualized. I will send a >>> patch that deals with the masking for features that TCG can't emulate. >> >> Thanks :). >> >> >> Alex >> >> > > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson
diff --git a/hw/spapr.c b/hw/spapr.c index fecfa4a..26333ac 100644 --- a/hw/spapr.c +++ b/hw/spapr.c @@ -186,8 +186,6 @@ static void *spapr_create_fdt_skel(const char *cpu_model, 0xffffffff, 0xffffffff}; uint32_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq() : TIMEBASE_FREQ; uint32_t cpufreq = kvm_enabled() ? kvmppc_get_clockfreq() : 1000000000; - uint32_t vmx = kvm_enabled() ? kvmppc_get_vmx() : 0; - uint32_t dfp = kvm_enabled() ? kvmppc_get_dfp() : 0; if ((index % smt) != 0) { continue; @@ -239,15 +237,17 @@ static void *spapr_create_fdt_skel(const char *cpu_model, * 0 / no property == no vector extensions * 1 == VMX / Altivec available * 2 == VSX available */ - if (vmx) { + if (env->insns_flags & PPC_ALTIVEC) { + uint32_t vmx = (env->insns_flags2 & PPC2_VSX) ? 2 : 1; + _FDT((fdt_property_cell(fdt, "ibm,vmx", vmx))); } /* Advertise DFP (Decimal Floating Point) if available * 0 / no property == no DFP * 1 == DFP available */ - if (dfp) { - _FDT((fdt_property_cell(fdt, "ibm,dfp", dfp))); + if (env->insns_flags2 & PPC2_DFP) { + _FDT((fdt_property_cell(fdt, "ibm,dfp", 1))); } _FDT((fdt_end_node(fdt))); diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h index 8e5c85c..d177bd4 100644 --- a/target-ppc/cpu.h +++ b/target-ppc/cpu.h @@ -858,6 +858,22 @@ enum { /* The whole PowerPC CPU context */ #define NB_MMU_MODES 3 +struct ppc_def_t { + const char *name; + uint32_t pvr; + uint32_t svr; + uint64_t insns_flags; + uint64_t insns_flags2; + uint64_t msr_mask; + powerpc_mmu_t mmu_model; + powerpc_excp_t excp_model; + powerpc_input_t bus_model; + uint32_t flags; + int bfd_mach; + void (*init_proc)(CPUPPCState *env); + int (*check_pow)(CPUPPCState *env); +}; + struct CPUPPCState { /* First are the most commonly used resources * during translated code execution @@ -1844,6 +1860,10 @@ enum { /* BookE 2.06 PowerPC specification */ PPC2_BOOKE206 = 0x0000000000000001ULL, + /* VSX (extensions to Altivec / VMX) */ + PPC2_VSX = 0x0000000000000002ULL, + /* Decimal Floating Point (DFP) */ + PPC2_DFP = 0x0000000000000004ULL, }; /*****************************************************************************/ diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c index 430558b..3d12c5c 100644 --- a/target-ppc/kvm.c +++ b/target-ppc/kvm.c @@ -887,14 +887,35 @@ static inline uint32_t mfpvr(void) return pvr; } +static void alter_insns(uint64_t *word, uint64_t flags, bool on) +{ + if (on) { + *word |= flags; + } else { + *word &= ~flags; + } +} + const ppc_def_t *kvmppc_host_cpu_def(void) { uint32_t host_pvr = mfpvr(); const ppc_def_t *base_spec; + ppc_def_t *spec; + uint32_t vmx = kvmppc_get_vmx(); + uint32_t dfp = kvmppc_get_dfp(); base_spec = ppc_find_by_pvr(host_pvr); - return base_spec; + spec = g_malloc0(sizeof(*spec)); + memcpy(spec, base_spec, sizeof(*spec)); + + /* Now fix up the spec with information we can query from the host */ + + alter_insns(&spec->insns_flags, PPC_ALTIVEC, vmx > 0); + alter_insns(&spec->insns_flags2, PPC2_VSX, vmx > 1); + alter_insns(&spec->insns_flags2, PPC2_DFP, dfp); + + return spec; } bool kvm_arch_stop_on_emulation_error(CPUState *env) diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c index 7de097d..c954192 100644 --- a/target-ppc/translate_init.c +++ b/target-ppc/translate_init.c @@ -34,22 +34,6 @@ #define TODO_USER_ONLY 1 #endif -struct ppc_def_t { - const char *name; - uint32_t pvr; - uint32_t svr; - uint64_t insns_flags; - uint64_t insns_flags2; - uint64_t msr_mask; - powerpc_mmu_t mmu_model; - powerpc_excp_t excp_model; - powerpc_input_t bus_model; - uint32_t flags; - int bfd_mach; - void (*init_proc)(CPUPPCState *env); - int (*check_pow)(CPUPPCState *env); -}; - /* For user-mode emulation, we don't emulate any IRQ controller */ #if defined(CONFIG_USER_ONLY) #define PPC_IRQ_INIT_FN(name) \ @@ -6535,6 +6519,8 @@ static void init_proc_970MP (CPUPPCState *env) PPC_64B | PPC_ALTIVEC | \ PPC_SEGMENT_64B | PPC_SLBI | \ PPC_POPCNTB | PPC_POPCNTWD) +/* FIXME: Should also have PPC2_VSX and PPC2_DFP, but we don't + * implement those in TCG yet */ #define POWERPC_INSNS2_POWER7 (PPC_NONE) #define POWERPC_MSRM_POWER7 (0x800000000204FF36ULL) #define POWERPC_MMU_POWER7 (POWERPC_MMU_2_06)
Currently, when KVM is enabled, the pseries machine checks if the host CPU supports VMX, VSX and/or DFP instructions and advertises accordingly in the guest device tree. It does this regardless of what CPU is selected on the command line. On the other hand, when in TCG mode, it never advertises any of these facilities, even basic VMX (Altivec) which is supported in TCG. Now that we have a -cpu host option for ppc, it is fairly straightforward to fix both problems. This patch changes the -cpu host code to override the basic cpu spec derived from the PVR with information queried from the host avout VMX, VSX and DFP capability. The pseries code then uses the instruction availability advertised in the cpu state to set the guest device tree correctly for both the KVM and TCG cases. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/spapr.c | 10 +++++----- target-ppc/cpu.h | 20 ++++++++++++++++++++ target-ppc/kvm.c | 23 ++++++++++++++++++++++- target-ppc/translate_init.c | 18 ++---------------- 4 files changed, 49 insertions(+), 22 deletions(-)