[PATCH] ARM: KVM: drop arbitrary limitation to 4 CPU VMs

[PATCH] ARM: KVM: drop arbitrary limitation to 4 CPU VMs

[Marc Zyngier]

It appears we have an arbitrary limitation where we refuse to
create more than 4 virtual A15 in a single VM.

This limitation doesn't make much sense (the number 4 probably
comes from the maximum number of CPUs in a A15 cluster, but
KVM doesn't have any notion of cluster), and directly
contradicts CONFIG_MAX_VCPUS.

Just remove this code altogether.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/kvm/reset.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/arm/kvm/reset.c b/arch/arm/kvm/reset.c
index c02ba4a..8e259d2 100644
--- a/arch/arm/kvm/reset.c
+++ b/arch/arm/kvm/reset.c
@@ -33,8 +33,6 @@
  * Cortex-A15 Reset Values
  */
 
-static const int a15_max_cpu_idx = 3;
-
 static struct kvm_regs a15_regs_reset = {
 	.usr_regs.ARM_cpsr = SVC_MODE | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT,
 };
@@ -63,8 +61,6 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
 
 	switch (vcpu->arch.target) {
 	case KVM_ARM_TARGET_CORTEX_A15:
-		if (vcpu->vcpu_id > a15_max_cpu_idx)
-			return -EINVAL;
 		reset_regs = &a15_regs_reset;
 		vcpu->arch.midr = read_cpuid_id();
 		cpu_vtimer_irq = &a15_vtimer_irq;
-- 
1.8.2.3

[PATCH] ARM: KVM: drop arbitrary limitation to 4 CPU VMs

[Peter Maydell]

On 9 October 2013 02:17, Marc Zyngier <marc.zyngier@arm.com> wrote:
> It appears we have an arbitrary limitation where we refuse to
> create more than 4 virtual A15 in a single VM.
>
> This limitation doesn't make much sense (the number 4 probably
> comes from the maximum number of CPUs in a A15 cluster, but
> KVM doesn't have any notion of cluster)

"Don't allow configurations that can't exist in hardware" makes
sense as a restriction to me... QEMU will also enforce the 4-cpu
limit.

-- PMM

[PATCH] ARM: KVM: drop arbitrary limitation to 4 CPU VMs

[Marc Zyngier]

On 2013-10-09 03:15, Peter Maydell wrote:
> On 9 October 2013 02:17, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> It appears we have an arbitrary limitation where we refuse to
>> create more than 4 virtual A15 in a single VM.
>>
>> This limitation doesn't make much sense (the number 4 probably
>> comes from the maximum number of CPUs in a A15 cluster, but
>> KVM doesn't have any notion of cluster)
>
> "Don't allow configurations that can't exist in hardware" makes
> sense as a restriction to me... QEMU will also enforce the 4-cpu
> limit.

Nothing prevents anyone from building a multi-cluster A15 system:
http://www.lsi.com/products/mobile-communication-processors/pages/axxia-communication-processor-axm5500.aspx

If you want to limit QEMU to specific configurations, that's perfectly 
fine. The kernel doesn't need to, and kvmtool won't.

         M.
-- 
Fast, cheap, reliable. Pick two.

[PATCH] ARM: KVM: drop arbitrary limitation to 4 CPU VMs

[Christoffer Dall]

On Tue, Oct 08, 2013 at 06:17:08PM +0100, Marc Zyngier wrote:
> It appears we have an arbitrary limitation where we refuse to
> create more than 4 virtual A15 in a single VM.
> 
> This limitation doesn't make much sense (the number 4 probably
> comes from the maximum number of CPUs in a A15 cluster, but
> KVM doesn't have any notion of cluster), and directly
> contradicts CONFIG_MAX_VCPUS.

So this comes from the early days where I looked at the A15 TRM and the
MPIDR bit field for the CPU ID is limited to 2 bits.  Exactly because
I wasn't sure what remifications (if any) it would have to start
populating this register with cluster id = (vcpu_id / 4) and cpu id =
(vcpu_id % 4) I put this nice arbitrary restriction in there.

I think we need to fix how we show this register to the guest
otherwise... No?

-Christoffer

> 
> Just remove this code altogether.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/kvm/reset.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/arch/arm/kvm/reset.c b/arch/arm/kvm/reset.c
> index c02ba4a..8e259d2 100644
> --- a/arch/arm/kvm/reset.c
> +++ b/arch/arm/kvm/reset.c
> @@ -33,8 +33,6 @@
>   * Cortex-A15 Reset Values
>   */
>  
> -static const int a15_max_cpu_idx = 3;
> -
>  static struct kvm_regs a15_regs_reset = {
>  	.usr_regs.ARM_cpsr = SVC_MODE | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT,
>  };
> @@ -63,8 +61,6 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>  
>  	switch (vcpu->arch.target) {
>  	case KVM_ARM_TARGET_CORTEX_A15:
> -		if (vcpu->vcpu_id > a15_max_cpu_idx)
> -			return -EINVAL;
>  		reset_regs = &a15_regs_reset;
>  		vcpu->arch.midr = read_cpuid_id();
>  		cpu_vtimer_irq = &a15_vtimer_irq;
> -- 
> 1.8.2.3
> 
> 

[PATCH] ARM: KVM: drop arbitrary limitation to 4 CPU VMs

[Marc Zyngier]

On 13/10/13 02:09, Christoffer Dall wrote:
> On Tue, Oct 08, 2013 at 06:17:08PM +0100, Marc Zyngier wrote:
>> It appears we have an arbitrary limitation where we refuse to
>> create more than 4 virtual A15 in a single VM.
>>
>> This limitation doesn't make much sense (the number 4 probably
>> comes from the maximum number of CPUs in a A15 cluster, but
>> KVM doesn't have any notion of cluster), and directly
>> contradicts CONFIG_MAX_VCPUS.
> 
> So this comes from the early days where I looked at the A15 TRM and the
> MPIDR bit field for the CPU ID is limited to 2 bits.  Exactly because
> I wasn't sure what remifications (if any) it would have to start
> populating this register with cluster id = (vcpu_id / 4) and cpu id =
> (vcpu_id % 4) I put this nice arbitrary restriction in there.
> 
> I think we need to fix how we show this register to the guest
> otherwise... No?

I don't see this being an issue, but if we really want to be 100% true
to the A15/A7 TRM, we can always compute MPIDR that way, and adjust
L2CTLR as well.

That will require some userspace change in kvmtool though (need to
change the DT generator to cope with the cluster ID).

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH] ARM: KVM: drop arbitrary limitation to 4 CPU VMs

[Christoffer Dall]

On Mon, Oct 14, 2013 at 09:43:04AM +0100, Marc Zyngier wrote:
> On 13/10/13 02:09, Christoffer Dall wrote:
> > On Tue, Oct 08, 2013 at 06:17:08PM +0100, Marc Zyngier wrote:
> >> It appears we have an arbitrary limitation where we refuse to
> >> create more than 4 virtual A15 in a single VM.
> >>
> >> This limitation doesn't make much sense (the number 4 probably
> >> comes from the maximum number of CPUs in a A15 cluster, but
> >> KVM doesn't have any notion of cluster), and directly
> >> contradicts CONFIG_MAX_VCPUS.
> > 
> > So this comes from the early days where I looked at the A15 TRM and the
> > MPIDR bit field for the CPU ID is limited to 2 bits.  Exactly because
> > I wasn't sure what remifications (if any) it would have to start
> > populating this register with cluster id = (vcpu_id / 4) and cpu id =
> > (vcpu_id % 4) I put this nice arbitrary restriction in there.
> > 
> > I think we need to fix how we show this register to the guest
> > otherwise... No?
> 
> I don't see this being an issue, but if we really want to be 100% true
> to the A15/A7 TRM, we can always compute MPIDR that way, and adjust
> L2CTLR as well.

Even with mach-virt we're still pretending to be an A15/A7 right? I
think we should adhere to that.  Is there some other reason why people
shouldn't generally expect MPIDR to be correct (merging this code to KVM
and running in a VM notwithstanding)?

> 
> That will require some userspace change in kvmtool though (need to
> change the DT generator to cope with the cluster ID).
> 
That would be the less fun part...

-Christoffer

[PATCH] ARM: KVM: drop arbitrary limitation to 4 CPU VMs

[Marc Zyngier]

On 16/10/13 00:21, Christoffer Dall wrote:
> On Mon, Oct 14, 2013 at 09:43:04AM +0100, Marc Zyngier wrote:
>> On 13/10/13 02:09, Christoffer Dall wrote:
>>> On Tue, Oct 08, 2013 at 06:17:08PM +0100, Marc Zyngier wrote:
>>>> It appears we have an arbitrary limitation where we refuse to
>>>> create more than 4 virtual A15 in a single VM.
>>>>
>>>> This limitation doesn't make much sense (the number 4 probably
>>>> comes from the maximum number of CPUs in a A15 cluster, but
>>>> KVM doesn't have any notion of cluster), and directly
>>>> contradicts CONFIG_MAX_VCPUS.
>>>
>>> So this comes from the early days where I looked at the A15 TRM and the
>>> MPIDR bit field for the CPU ID is limited to 2 bits.  Exactly because
>>> I wasn't sure what remifications (if any) it would have to start
>>> populating this register with cluster id = (vcpu_id / 4) and cpu id =
>>> (vcpu_id % 4) I put this nice arbitrary restriction in there.
>>>
>>> I think we need to fix how we show this register to the guest
>>> otherwise... No?
>>
>> I don't see this being an issue, but if we really want to be 100% true
>> to the A15/A7 TRM, we can always compute MPIDR that way, and adjust
>> L2CTLR as well.
> 
> Even with mach-virt we're still pretending to be an A15/A7 right? I
> think we should adhere to that.  Is there some other reason why people
> shouldn't generally expect MPIDR to be correct (merging this code to KVM
> and running in a VM notwithstanding)?

Yes, it is probably better to adhere to the law of least surprise. I'm
reworking this patch and will repost (slightly longer) the series.

>>
>> That will require some userspace change in kvmtool though (need to
>> change the DT generator to cope with the cluster ID).
>>
> That would be the less fun part...

Actually, it is surprisingly easy once the kernel does the right thing.

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH] ARM: KVM: drop arbitrary limitation to 4 CPU VMs

[Christoffer Dall]

On 16 October 2013 02:49, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 16/10/13 00:21, Christoffer Dall wrote:
>> On Mon, Oct 14, 2013 at 09:43:04AM +0100, Marc Zyngier wrote:
>>> On 13/10/13 02:09, Christoffer Dall wrote:
>>>> On Tue, Oct 08, 2013 at 06:17:08PM +0100, Marc Zyngier wrote:
>>>>> It appears we have an arbitrary limitation where we refuse to
>>>>> create more than 4 virtual A15 in a single VM.
>>>>>
>>>>> This limitation doesn't make much sense (the number 4 probably
>>>>> comes from the maximum number of CPUs in a A15 cluster, but
>>>>> KVM doesn't have any notion of cluster), and directly
>>>>> contradicts CONFIG_MAX_VCPUS.
>>>>
>>>> So this comes from the early days where I looked at the A15 TRM and the
>>>> MPIDR bit field for the CPU ID is limited to 2 bits.  Exactly because
>>>> I wasn't sure what remifications (if any) it would have to start
>>>> populating this register with cluster id = (vcpu_id / 4) and cpu id =
>>>> (vcpu_id % 4) I put this nice arbitrary restriction in there.
>>>>
>>>> I think we need to fix how we show this register to the guest
>>>> otherwise... No?
>>>
>>> I don't see this being an issue, but if we really want to be 100% true
>>> to the A15/A7 TRM, we can always compute MPIDR that way, and adjust
>>> L2CTLR as well.
>>
>> Even with mach-virt we're still pretending to be an A15/A7 right? I
>> think we should adhere to that.  Is there some other reason why people
>> shouldn't generally expect MPIDR to be correct (merging this code to KVM
>> and running in a VM notwithstanding)?
>
> Yes, it is probably better to adhere to the law of least surprise. I'm
> reworking this patch and will repost (slightly longer) the series.
>
>>>
>>> That will require some userspace change in kvmtool though (need to
>>> change the DT generator to cope with the cluster ID).
>>>
>> That would be the less fun part...
>
> Actually, it is surprisingly easy once the kernel does the right thing.
>
Great ;)