| Message ID | 1477873703-22403-3-git-send-email-sjitindarsingh@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, Oct 31, 2016 at 11:28:23AM +1100, Suraj Jitindar Singh wrote: > The function kvmppc_set_arch_compat() is used to determine the value of the > processor compatibility register (PCR) for a guest running in a given > compatibility mode. There is currently no support for v3.00 of the ISA. > > Add support for v3.00 of the ISA which adds an ISA v2.07 compatilibity mode > to the PCR. > > We also add a check to ensure the processor we are running on is capable of > emulating the chosen processor (for example a POWER7 cannot emulate a > POWER8, similarly with a POWER8 and a POWER9). > > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com> > --- > arch/powerpc/kvm/book3s_hv.c | 32 +++++++++++++++++++++++--------- > 1 file changed, 23 insertions(+), 9 deletions(-) > > diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c > index 3686471..24681e7 100644 > --- a/arch/powerpc/kvm/book3s_hv.c > +++ b/arch/powerpc/kvm/book3s_hv.c > @@ -311,24 +311,38 @@ static int kvmppc_set_arch_compat(struct kvm_vcpu *vcpu, u32 arch_compat) > * If an arch bit is set in PCR, all the defined > * higher-order arch bits also have to be set. > */ > - pcr = PCR_ARCH_206 | PCR_ARCH_205; > + if (cpu_has_feature(CPU_FTR_ARCH_206)) > + pcr |= PCR_ARCH_205; > + if (cpu_has_feature(CPU_FTR_ARCH_207S)) > + pcr |= PCR_ARCH_206; > + if (cpu_has_feature(CPU_FTR_ARCH_300)) > + pcr |= PCR_ARCH_207; > break; > case PVR_ARCH_206: > case PVR_ARCH_206p: > - pcr = PCR_ARCH_206; > + /* Must be at least v2.06 to (emulate) it */ > + if (!cpu_has_feature(CPU_FTR_ARCH_206)) > + return -EINVAL; > + if (cpu_has_feature(CPU_FTR_ARCH_207S)) > + pcr |= PCR_ARCH_206; > + if (cpu_has_feature(CPU_FTR_ARCH_300)) > + pcr |= PCR_ARCH_207; > break; > case PVR_ARCH_207: > + /* Must be at least v2.07 to (emulate) it */ > + if (!cpu_has_feature(CPU_FTR_ARCH_207S)) > + return -EINVAL; > + if (cpu_has_feature(CPU_FTR_ARCH_300)) > + pcr |= PCR_ARCH_207; > + break; > + case PVR_ARCH_300: > + /* Must be at least v3.00 to (emulate) it */ > + if (!cpu_has_feature(CPU_FTR_ARCH_300)) > + return -EINVAL; > break; I can't help thinking that the repetitive structure of the lines you're adding must imply a regularity that could be expressed more concisely. If you defined a dummy PCR_ARCH_300 bit as 0x10, perhaps you could do something like this: if (cpu_has_feature(CPU_FTR_ARCH_300)) host_pcr_bit = PCR_ARCH_300; else if (cpu_has_feature(CPU_FTR_ARCH_207S)) host_pcr_bit = PCR_ARCH_207; else host_pcr_bit = PCR_ARCH_206; switch (arch_compat) { case PVR_ARCH_205: guest_pcr_bit = PCR_ARCH_205; break; case PVR_ARCH_206: guest_pcr_bit = PCR_ARCH_206; break; case PVR_ARCH_207: case PVR_ARCH_207S: guest_pcr_bit = PCR_ARCH_207; break; case PVR_ARCH_300: guest_pcr_bit = PCR_ARCH_300; break; default: return -EINVAL; } if (guest_pcr_bit > host_pcr_bit) return -EINVAL; pcr = host_pcr_bit - guest_pcr_bit; The translation from arch_compat to guest_pcr_bit might look neater as a table lookup on the low bits of arch_compat, after a bounds check. Paul. -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2016-10-31 at 16:44 +1100, Paul Mackerras wrote: > On Mon, Oct 31, 2016 at 11:28:23AM +1100, Suraj Jitindar Singh wrote: > > > > The function kvmppc_set_arch_compat() is used to determine the > > value of the > > processor compatibility register (PCR) for a guest running in a > > given > > compatibility mode. There is currently no support for v3.00 of the > > ISA. > > > > Add support for v3.00 of the ISA which adds an ISA v2.07 > > compatilibity mode > > to the PCR. > > > > We also add a check to ensure the processor we are running on is > > capable of > > emulating the chosen processor (for example a POWER7 cannot emulate > > a > > POWER8, similarly with a POWER8 and a POWER9). > > > > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com> > > --- > > arch/powerpc/kvm/book3s_hv.c | 32 +++++++++++++++++++++++--------- > > 1 file changed, 23 insertions(+), 9 deletions(-) > > > > diff --git a/arch/powerpc/kvm/book3s_hv.c > > b/arch/powerpc/kvm/book3s_hv.c > > index 3686471..24681e7 100644 > > --- a/arch/powerpc/kvm/book3s_hv.c > > +++ b/arch/powerpc/kvm/book3s_hv.c > > @@ -311,24 +311,38 @@ static int kvmppc_set_arch_compat(struct > > kvm_vcpu *vcpu, u32 arch_compat) > > * If an arch bit is set in PCR, all the > > defined > > * higher-order arch bits also have to be > > set. > > */ > > - pcr = PCR_ARCH_206 | PCR_ARCH_205; > > + if (cpu_has_feature(CPU_FTR_ARCH_206)) > > + pcr |= PCR_ARCH_205; > > + if (cpu_has_feature(CPU_FTR_ARCH_207S)) > > + pcr |= PCR_ARCH_206; > > + if (cpu_has_feature(CPU_FTR_ARCH_300)) > > + pcr |= PCR_ARCH_207; > > break; > > case PVR_ARCH_206: > > case PVR_ARCH_206p: > > - pcr = PCR_ARCH_206; > > + /* Must be at least v2.06 to (emulate) it > > */ > > + if (!cpu_has_feature(CPU_FTR_ARCH_206)) > > + return -EINVAL; > > + if (cpu_has_feature(CPU_FTR_ARCH_207S)) > > + pcr |= PCR_ARCH_206; > > + if (cpu_has_feature(CPU_FTR_ARCH_300)) > > + pcr |= PCR_ARCH_207; > > break; > > case PVR_ARCH_207: > > + /* Must be at least v2.07 to (emulate) it > > */ > > + if (!cpu_has_feature(CPU_FTR_ARCH_207S)) > > + return -EINVAL; > > + if (cpu_has_feature(CPU_FTR_ARCH_300)) > > + pcr |= PCR_ARCH_207; > > + break; > > + case PVR_ARCH_300: > > + /* Must be at least v3.00 to (emulate) it > > */ > > + if (!cpu_has_feature(CPU_FTR_ARCH_300)) > > + return -EINVAL; > > break; > I can't help thinking that the repetitive structure of the lines > you're adding must imply a regularity that could be expressed more > concisely. If you defined a dummy PCR_ARCH_300 bit as 0x10, perhaps > you could do something like this: > > if (cpu_has_feature(CPU_FTR_ARCH_300)) > host_pcr_bit = PCR_ARCH_300; > else if (cpu_has_feature(CPU_FTR_ARCH_207S)) > host_pcr_bit = PCR_ARCH_207; > else else if > host_pcr_bit = PCR_ARCH_206; else host_pcr_bit = PCR_ARCH_205; > > switch (arch_compat) { > case PVR_ARCH_205: > guest_pcr_bit = PCR_ARCH_205; > break; > case PVR_ARCH_206: > guest_pcr_bit = PCR_ARCH_206; > break; > case PVR_ARCH_207: > case PVR_ARCH_207S: > guest_pcr_bit = PCR_ARCH_207; > break; > case PVR_ARCH_300: > guest_pcr_bit = PCR_ARCH_300; > break; > default: > return -EINVAL; > } > > if (guest_pcr_bit > host_pcr_bit) > return -EINVAL; > > pcr = host_pcr_bit - guest_pcr_bit; That approach is simpler and more extensible, I guess I don't really like that it relies on the assumption that the PCR bits remain consecutive and breaks if that assumption becomes invalid, which may never be the case. I guess we can assume they will be for now and fix it in the event that ever changes. > > The translation from arch_compat to guest_pcr_bit might look neater > as > a table lookup on the low bits of arch_compat, after a bounds check. Is that really necessary? I don't see the benefit and the code is more readable in it's current form IMO. Something like this? unsigned long guest_pcr_bits = {0, /* 0 */ 0, /* 1 */ PCR_ARCH_205, /* 0x0F000002 */ PCR_ARCH_206, /* 0x0F000003 */ PCR_ARCH_207, /* 0x0F000004 */ PCR_ARCH_300 }; /* 0x0F000005 */ if (arch_compat <= PVR_ARCH_300 && arch_compat >= PVR_ARCH_205) guest_pcr_bit = guest_pcr_bits[arch_compat & 0xF]; else return -EINVAL; > > Paul. -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c index 3686471..24681e7 100644 --- a/arch/powerpc/kvm/book3s_hv.c +++ b/arch/powerpc/kvm/book3s_hv.c @@ -311,24 +311,38 @@ static int kvmppc_set_arch_compat(struct kvm_vcpu *vcpu, u32 arch_compat) * If an arch bit is set in PCR, all the defined * higher-order arch bits also have to be set. */ - pcr = PCR_ARCH_206 | PCR_ARCH_205; + if (cpu_has_feature(CPU_FTR_ARCH_206)) + pcr |= PCR_ARCH_205; + if (cpu_has_feature(CPU_FTR_ARCH_207S)) + pcr |= PCR_ARCH_206; + if (cpu_has_feature(CPU_FTR_ARCH_300)) + pcr |= PCR_ARCH_207; break; case PVR_ARCH_206: case PVR_ARCH_206p: - pcr = PCR_ARCH_206; + /* Must be at least v2.06 to (emulate) it */ + if (!cpu_has_feature(CPU_FTR_ARCH_206)) + return -EINVAL; + if (cpu_has_feature(CPU_FTR_ARCH_207S)) + pcr |= PCR_ARCH_206; + if (cpu_has_feature(CPU_FTR_ARCH_300)) + pcr |= PCR_ARCH_207; break; case PVR_ARCH_207: + /* Must be at least v2.07 to (emulate) it */ + if (!cpu_has_feature(CPU_FTR_ARCH_207S)) + return -EINVAL; + if (cpu_has_feature(CPU_FTR_ARCH_300)) + pcr |= PCR_ARCH_207; + break; + case PVR_ARCH_300: + /* Must be at least v3.00 to (emulate) it */ + if (!cpu_has_feature(CPU_FTR_ARCH_300)) + return -EINVAL; break; default: return -EINVAL; } - - if (!cpu_has_feature(CPU_FTR_ARCH_207S)) { - /* POWER7 can't emulate POWER8 */ - if (!(pcr & PCR_ARCH_206)) - return -EINVAL; - pcr &= ~PCR_ARCH_206; - } } spin_lock(&vc->lock);
The function kvmppc_set_arch_compat() is used to determine the value of the processor compatibility register (PCR) for a guest running in a given compatibility mode. There is currently no support for v3.00 of the ISA. Add support for v3.00 of the ISA which adds an ISA v2.07 compatilibity mode to the PCR. We also add a check to ensure the processor we are running on is capable of emulating the chosen processor (for example a POWER7 cannot emulate a POWER8, similarly with a POWER8 and a POWER9). Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com> --- arch/powerpc/kvm/book3s_hv.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-)