Patchwork [08/50] KVM: PPC: Add support for explicit HIOR setting

login
register
mail settings
Submitter Alexander Graf
Date Jan. 4, 2012, 1:10 a.m.
Message ID <1325639448-9494-9-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/134168/
State New
Headers show

Comments

Alexander Graf - Jan. 4, 2012, 1:10 a.m.
Until now, we always set HIOR based on the PVR, but this is just wrong.
Instead, we should be setting HIOR explicitly, so user space can decide
what the initial HIOR value is - just like on real hardware.

We keep the old PVR based way around for backwards compatibility, but
once user space uses the SET_ONE_REG based method, we drop the PVR logic.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 Documentation/virtual/kvm/api.txt     |    1 +
 arch/powerpc/include/asm/kvm.h        |    2 ++
 arch/powerpc/include/asm/kvm_book3s.h |    2 ++
 arch/powerpc/kvm/book3s_pr.c          |    6 ++++--
 arch/powerpc/kvm/powerpc.c            |   14 ++++++++++++++
 include/linux/kvm.h                   |    1 +
 6 files changed, 24 insertions(+), 2 deletions(-)
Scott Wood - Jan. 4, 2012, 8:12 p.m.
On 01/03/2012 07:10 PM, Alexander Graf wrote:
> diff --git a/arch/powerpc/include/asm/kvm.h b/arch/powerpc/include/asm/kvm.h
> index 25964ee..fb3fddc 100644
> --- a/arch/powerpc/include/asm/kvm.h
> +++ b/arch/powerpc/include/asm/kvm.h
> @@ -327,4 +327,6 @@ struct kvm_book3e_206_tlb_params {
>  	__u32 reserved[8];
>  };
>  
> +#define KVM_ONE_REG_PPC_HIOR	KVM_ONE_REG_PPC | 0x100

Where does 0x100 come from?

There should probaly be some structure to the register space, with a
subarch field distinguishing between "common SPR", "book3s SPR", "book3e
SPR", along with others for non-SPR registers, KVM inventions, etc.

Can we shorten these to KVM_REG_PPC and KVM_REG_PPC_HIOR (especially if
we might use the same numbers for a future get/set many interface)?

-Scott

--
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
Alexander Graf - Jan. 5, 2012, 2:36 a.m.
On 04.01.2012, at 21:12, Scott Wood wrote:

> On 01/03/2012 07:10 PM, Alexander Graf wrote:
>> diff --git a/arch/powerpc/include/asm/kvm.h b/arch/powerpc/include/asm/kvm.h
>> index 25964ee..fb3fddc 100644
>> --- a/arch/powerpc/include/asm/kvm.h
>> +++ b/arch/powerpc/include/asm/kvm.h
>> @@ -327,4 +327,6 @@ struct kvm_book3e_206_tlb_params {
>> 	__u32 reserved[8];
>> };
>> 
>> +#define KVM_ONE_REG_PPC_HIOR	KVM_ONE_REG_PPC | 0x100
> 
> Where does 0x100 come from?

I quite frankly don't remember :). This could just as well be 0 or 1.

> There should probaly be some structure to the register space, with a
> subarch field distinguishing between "common SPR", "book3s SPR", "book3e
> SPR", along with others for non-SPR registers, KVM inventions, etc.

Not sure we really need this. I would very much prefer to just always use the textual representation. What would we do if something book3s specific suddenly becomes generic (like Altivec for example, or FPU)?

> Can we shorten these to KVM_REG_PPC and KVM_REG_PPC_HIOR (especially if
> we might use the same numbers for a future get/set many interface)?

Good idea. Will change.


Alex

--
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
Scott Wood - Jan. 5, 2012, 5:16 p.m.
On 01/04/2012 08:36 PM, Alexander Graf wrote:
> 
> On 04.01.2012, at 21:12, Scott Wood wrote:
> 
>> On 01/03/2012 07:10 PM, Alexander Graf wrote:
>>> diff --git a/arch/powerpc/include/asm/kvm.h b/arch/powerpc/include/asm/kvm.h
>>> index 25964ee..fb3fddc 100644
>>> --- a/arch/powerpc/include/asm/kvm.h
>>> +++ b/arch/powerpc/include/asm/kvm.h
>>> @@ -327,4 +327,6 @@ struct kvm_book3e_206_tlb_params {
>>> 	__u32 reserved[8];
>>> };
>>>
>>> +#define KVM_ONE_REG_PPC_HIOR	KVM_ONE_REG_PPC | 0x100
>>
>> Where does 0x100 come from?
> 
> I quite frankly don't remember :). This could just as well be 0 or 1.
> 
>> There should probaly be some structure to the register space, with a
>> subarch field distinguishing between "common SPR", "book3s SPR", "book3e
>> SPR", along with others for non-SPR registers, KVM inventions, etc.
> 
> Not sure we really need this. I would very much prefer to just always
> use the textual representation. What would we do if something book3s
> specific suddenly becomes generic (like Altivec for example, or
> FPU)?

Good point... A plain enumeration should be fine.  0x100 was strange
enough that it left me wondering what the value should be for the next
register to be added -- I didn't want it to turn into something like the
booke regs->trap mess, where some exceptions re-use the classic offsets,
and other exceptions have numbers that seem to be pulled from out of
nowhere.

-Scott

--
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
Alexander Graf - Jan. 6, 2012, 2:35 a.m.
On 05.01.2012, at 18:16, Scott Wood wrote:

> On 01/04/2012 08:36 PM, Alexander Graf wrote:
>> 
>> On 04.01.2012, at 21:12, Scott Wood wrote:
>> 
>>> On 01/03/2012 07:10 PM, Alexander Graf wrote:
>>>> diff --git a/arch/powerpc/include/asm/kvm.h b/arch/powerpc/include/asm/kvm.h
>>>> index 25964ee..fb3fddc 100644
>>>> --- a/arch/powerpc/include/asm/kvm.h
>>>> +++ b/arch/powerpc/include/asm/kvm.h
>>>> @@ -327,4 +327,6 @@ struct kvm_book3e_206_tlb_params {
>>>> 	__u32 reserved[8];
>>>> };
>>>> 
>>>> +#define KVM_ONE_REG_PPC_HIOR	KVM_ONE_REG_PPC | 0x100
>>> 
>>> Where does 0x100 come from?
>> 
>> I quite frankly don't remember :). This could just as well be 0 or 1.
>> 
>>> There should probaly be some structure to the register space, with a
>>> subarch field distinguishing between "common SPR", "book3s SPR", "book3e
>>> SPR", along with others for non-SPR registers, KVM inventions, etc.
>> 
>> Not sure we really need this. I would very much prefer to just always
>> use the textual representation. What would we do if something book3s
>> specific suddenly becomes generic (like Altivec for example, or
>> FPU)?
> 
> Good point... A plain enumeration should be fine.  0x100 was strange
> enough that it left me wondering what the value should be for the next
> register to be added -- I didn't want it to turn into something like the
> booke regs->trap mess, where some exceptions re-use the classic offsets,
> and other exceptions have numbers that seem to be pulled from out of
> nowhere.

Yup. I'll change it to something lower.

Also we're already using KVM_REG for MMIO register identifiers. But I guess we can just reuse the namespace as long as we're careful to not overlap them later.

#define KVM_REG_MASK            0x001f
#define KVM_REG_EXT_MASK        0xffe0
#define KVM_REG_GPR             0x0000
#define KVM_REG_FPR             0x0020
#define KVM_REG_QPR             0x0040
#define KVM_REG_FQPR            0x0060


Alex

--
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
Scott Wood - Jan. 6, 2012, 9:12 p.m.
On 01/05/2012 08:35 PM, Alexander Graf wrote:
> Also we're already using KVM_REG for MMIO register identifiers. But I guess we can just reuse the namespace as long as we're careful to not overlap them later.
> 
> #define KVM_REG_MASK            0x001f
> #define KVM_REG_EXT_MASK        0xffe0
> #define KVM_REG_GPR             0x0000
> #define KVM_REG_FPR             0x0020
> #define KVM_REG_QPR             0x0040
> #define KVM_REG_FQPR            0x0060

It looks like these are only used internally, despite being in the
public header -- so renaming these is an option as well if it'd be
confusing otherwise.

-Scott

--
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
Alexander Graf - Jan. 7, 2012, 1:14 a.m.
On 06.01.2012, at 22:12, Scott Wood wrote:

> On 01/05/2012 08:35 PM, Alexander Graf wrote:
>> Also we're already using KVM_REG for MMIO register identifiers. But I guess we can just reuse the namespace as long as we're careful to not overlap them later.
>> 
>> #define KVM_REG_MASK            0x001f
>> #define KVM_REG_EXT_MASK        0xffe0
>> #define KVM_REG_GPR             0x0000
>> #define KVM_REG_FPR             0x0020
>> #define KVM_REG_QPR             0x0040
>> #define KVM_REG_FQPR            0x0060
> 
> It looks like these are only used internally, despite being in the
> public header -- so renaming these is an option as well if it'd be
> confusing otherwise.

Yup, let's rename them while we can!


Alex

--
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
Scott Wood - Jan. 9, 2012, 7:52 p.m.
On 01/06/2012 07:14 PM, Alexander Graf wrote:
> 
> On 06.01.2012, at 22:12, Scott Wood wrote:
> 
>> On 01/05/2012 08:35 PM, Alexander Graf wrote:
>>> Also we're already using KVM_REG for MMIO register identifiers. But I guess we can just reuse the namespace as long as we're careful to not overlap them later.
>>>
>>> #define KVM_REG_MASK            0x001f
>>> #define KVM_REG_EXT_MASK        0xffe0
>>> #define KVM_REG_GPR             0x0000
>>> #define KVM_REG_FPR             0x0020
>>> #define KVM_REG_QPR             0x0040
>>> #define KVM_REG_FQPR            0x0060
>>
>> It looks like these are only used internally, despite being in the
>> public header -- so renaming these is an option as well if it'd be
>> confusing otherwise.
> 
> Yup, let's rename them while we can!

And move them out of the public header -- there's nothing userspace or a
guest can sanely do with these.

-Scott

--
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

Patch

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index b8c558d..b42f189 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -1553,6 +1553,7 @@  track of the implemented registers, find a list below:
 
   Arch  |       Register        | Width (bits)
         |                       |
+  PPC   | KVM_ONE_REG_PPC_HIOR  | 64
 
 4.66 KVM_GET_ONE_REG
 
diff --git a/arch/powerpc/include/asm/kvm.h b/arch/powerpc/include/asm/kvm.h
index 25964ee..fb3fddc 100644
--- a/arch/powerpc/include/asm/kvm.h
+++ b/arch/powerpc/include/asm/kvm.h
@@ -327,4 +327,6 @@  struct kvm_book3e_206_tlb_params {
 	__u32 reserved[8];
 };
 
+#define KVM_ONE_REG_PPC_HIOR	KVM_ONE_REG_PPC | 0x100
+
 #endif /* __LINUX_KVM_POWERPC_H */
diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/include/asm/kvm_book3s.h
index 69c7377..deb8a4e 100644
--- a/arch/powerpc/include/asm/kvm_book3s.h
+++ b/arch/powerpc/include/asm/kvm_book3s.h
@@ -90,6 +90,8 @@  struct kvmppc_vcpu_book3s {
 #endif
 	int context_id[SID_CONTEXTS];
 
+	bool hior_explicit;		/* HIOR is set by ioctl, not PVR */
+
 	struct hlist_head hpte_hash_pte[HPTEG_HASH_NUM_PTE];
 	struct hlist_head hpte_hash_pte_long[HPTEG_HASH_NUM_PTE_LONG];
 	struct hlist_head hpte_hash_vpte[HPTEG_HASH_NUM_VPTE];
diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
index e2cfb9e..aaefe19 100644
--- a/arch/powerpc/kvm/book3s_pr.c
+++ b/arch/powerpc/kvm/book3s_pr.c
@@ -151,14 +151,16 @@  void kvmppc_set_pvr(struct kvm_vcpu *vcpu, u32 pvr)
 #ifdef CONFIG_PPC_BOOK3S_64
 	if ((pvr >= 0x330000) && (pvr < 0x70330000)) {
 		kvmppc_mmu_book3s_64_init(vcpu);
-		to_book3s(vcpu)->hior = 0xfff00000;
+		if (!to_book3s(vcpu)->hior_explicit)
+			to_book3s(vcpu)->hior = 0xfff00000;
 		to_book3s(vcpu)->msr_mask = 0xffffffffffffffffULL;
 		vcpu->arch.cpu_type = KVM_CPU_3S_64;
 	} else
 #endif
 	{
 		kvmppc_mmu_book3s_32_init(vcpu);
-		to_book3s(vcpu)->hior = 0;
+		if (!to_book3s(vcpu)->hior_explicit)
+			to_book3s(vcpu)->hior = 0;
 		to_book3s(vcpu)->msr_mask = 0xffffffffULL;
 		vcpu->arch.cpu_type = KVM_CPU_3S_32;
 	}
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 4b01823..6d732b3 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -208,6 +208,7 @@  int kvm_dev_ioctl_check_extension(long ext)
 	case KVM_CAP_PPC_BOOKE_SREGS:
 #else
 	case KVM_CAP_PPC_SEGSTATE:
+	case KVM_CAP_PPC_HIOR:
 	case KVM_CAP_PPC_PAPR:
 #endif
 	case KVM_CAP_PPC_UNSET_IRQ:
@@ -633,6 +634,12 @@  static int kvm_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu,
 	int r = -EINVAL;
 
 	switch (reg->id) {
+#ifdef CONFIG_PPC_BOOK3S
+	case KVM_ONE_REG_PPC_HIOR:
+		reg->u.reg64 = to_book3s(vcpu)->hior;
+		r = 0;
+		break;
+#endif
 	default:
 		break;
 	}
@@ -646,6 +653,13 @@  static int kvm_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu,
 	int r = -EINVAL;
 
 	switch (reg->id) {
+#ifdef CONFIG_PPC_BOOK3S
+	case KVM_ONE_REG_PPC_HIOR:
+		to_book3s(vcpu)->hior = reg->u.reg64;
+		to_book3s(vcpu)->hior_explicit = true;
+		r = 0;
+		break;
+#endif
 	default:
 		break;
 	}
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 51ab25e..a20248e 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -555,6 +555,7 @@  struct kvm_ppc_pvinfo {
 #define KVM_CAP_PPC_SMT 64
 #define KVM_CAP_PPC_RMA	65
 #define KVM_CAP_MAX_VCPUS 66       /* returns max vcpus per vm */
+#define KVM_CAP_PPC_HIOR 67
 #define KVM_CAP_PPC_PAPR 68
 #define KVM_CAP_SW_TLB 69
 #define KVM_CAP_ONE_REG 70