[v2,05/15] KVM: arm/arm64: make GIC frame address initialization model specific
diff mbox

Message ID 1436538111-4294-6-git-send-email-andre.przywara@arm.com
State New
Headers show

Commit Message

Andre Przywara July 10, 2015, 2:21 p.m. UTC
Currently we initialize all the possible GIC frame addresses in one
function, without looking at the specific GIC model we instantiate
for the guest.
As this gets confusing when adding another VGIC model later, lets
move these initializations into the respective model's init functions.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 virt/kvm/arm/vgic-v2-emul.c | 3 +++
 virt/kvm/arm/vgic-v3-emul.c | 3 +++
 virt/kvm/arm/vgic.c         | 3 ---
 3 files changed, 6 insertions(+), 3 deletions(-)

Comments

Eric Auger Aug. 12, 2015, 1:02 p.m. UTC | #1
On 07/10/2015 04:21 PM, Andre Przywara wrote:
> Currently we initialize all the possible GIC frame addresses in one
> function, without looking at the specific GIC model we instantiate
> for the guest.
> As this gets confusing when adding another VGIC model later, lets
> move these initializations into the respective model's init 
nit: tobe more precise the init emulation function (not the
vgic_v2/v3_init_model model's init function). pfouh?! ;-)
functions.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  virt/kvm/arm/vgic-v2-emul.c | 3 +++
>  virt/kvm/arm/vgic-v3-emul.c | 3 +++
>  virt/kvm/arm/vgic.c         | 3 ---
>  3 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic-v2-emul.c b/virt/kvm/arm/vgic-v2-emul.c
> index 1390797..8faa28c 100644
> --- a/virt/kvm/arm/vgic-v2-emul.c
> +++ b/virt/kvm/arm/vgic-v2-emul.c
> @@ -567,6 +567,9 @@ void vgic_v2_init_emulation(struct kvm *kvm)
>  	dist->vm_ops.init_model = vgic_v2_init_model;
>  	dist->vm_ops.map_resources = vgic_v2_map_resources;
>  
> +	dist->vgic_cpu_base = VGIC_ADDR_UNDEF;
> +	dist->vgic_dist_base = VGIC_ADDR_UNDEF;
Looks strange to see the common dist_base here. Why don't you leave it
in common part, kvm_vgic_create; all the more so you left
kvm->arch.vgic.vctrl_base = vgic->vctrl_base in kvm_vgic_create.

Eric
> +
>  	kvm->arch.max_vcpus = VGIC_V2_MAX_CPUS;
>  }
>  
> diff --git a/virt/kvm/arm/vgic-v3-emul.c b/virt/kvm/arm/vgic-v3-emul.c
> index d2eeb20..1f42348 100644
> --- a/virt/kvm/arm/vgic-v3-emul.c
> +++ b/virt/kvm/arm/vgic-v3-emul.c
> @@ -885,6 +885,9 @@ void vgic_v3_init_emulation(struct kvm *kvm)
>  	dist->vm_ops.destroy_model = vgic_v3_destroy_model;
>  	dist->vm_ops.map_resources = vgic_v3_map_resources;
>  
> +	dist->vgic_dist_base = VGIC_ADDR_UNDEF;
> +	dist->vgic_redist_base = VGIC_ADDR_UNDEF;
> +
>  	kvm->arch.max_vcpus = KVM_MAX_VCPUS;
>  }
>  
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index cc8f5ed..59f1801 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1830,9 +1830,6 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
>  	kvm->arch.vgic.in_kernel = true;
>  	kvm->arch.vgic.vgic_model = type;
>  	kvm->arch.vgic.vctrl_base = vgic->vctrl_base;
> -	kvm->arch.vgic.vgic_dist_base = VGIC_ADDR_UNDEF;
> -	kvm->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF;
> -	kvm->arch.vgic.vgic_redist_base = VGIC_ADDR_UNDEF;
>  
>  out_unlock:
>  	for (; vcpu_lock_idx >= 0; vcpu_lock_idx--) {
> 

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andre Przywara Aug. 24, 2015, 5:24 p.m. UTC | #2
Hi,

On 12/08/15 14:02, Eric Auger wrote:
> On 07/10/2015 04:21 PM, Andre Przywara wrote:
>> Currently we initialize all the possible GIC frame addresses in one
>> function, without looking at the specific GIC model we instantiate
>> for the guest.
>> As this gets confusing when adding another VGIC model later, lets
>> move these initializations into the respective model's init 
> nit: tobe more precise the init emulation function (not the
> vgic_v2/v3_init_model model's init function). pfouh?! ;-)
> functions.

OK, will try to find a wording that is not completely confusing.

>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  virt/kvm/arm/vgic-v2-emul.c | 3 +++
>>  virt/kvm/arm/vgic-v3-emul.c | 3 +++
>>  virt/kvm/arm/vgic.c         | 3 ---
>>  3 files changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic-v2-emul.c b/virt/kvm/arm/vgic-v2-emul.c
>> index 1390797..8faa28c 100644
>> --- a/virt/kvm/arm/vgic-v2-emul.c
>> +++ b/virt/kvm/arm/vgic-v2-emul.c
>> @@ -567,6 +567,9 @@ void vgic_v2_init_emulation(struct kvm *kvm)
>>  	dist->vm_ops.init_model = vgic_v2_init_model;
>>  	dist->vm_ops.map_resources = vgic_v2_map_resources;
>>  
>> +	dist->vgic_cpu_base = VGIC_ADDR_UNDEF;
>> +	dist->vgic_dist_base = VGIC_ADDR_UNDEF;
> Looks strange to see the common dist_base here. Why don't you leave it
> in common part, kvm_vgic_create; all the more so you left
> kvm->arch.vgic.vctrl_base = vgic->vctrl_base in kvm_vgic_create.

The idea behind this is that dist_base refers to similar, but not
identical distributors (v2 vs. v3), so I found it a good idea to
initialize it in here. Also vctrl_base is host facing and not set by
userland, so this doesn't really compare here.

Cheers,
Andre.

>> +
>>  	kvm->arch.max_vcpus = VGIC_V2_MAX_CPUS;
>>  }
>>  
>> diff --git a/virt/kvm/arm/vgic-v3-emul.c b/virt/kvm/arm/vgic-v3-emul.c
>> index d2eeb20..1f42348 100644
>> --- a/virt/kvm/arm/vgic-v3-emul.c
>> +++ b/virt/kvm/arm/vgic-v3-emul.c
>> @@ -885,6 +885,9 @@ void vgic_v3_init_emulation(struct kvm *kvm)
>>  	dist->vm_ops.destroy_model = vgic_v3_destroy_model;
>>  	dist->vm_ops.map_resources = vgic_v3_map_resources;
>>  
>> +	dist->vgic_dist_base = VGIC_ADDR_UNDEF;
>> +	dist->vgic_redist_base = VGIC_ADDR_UNDEF;
>> +
>>  	kvm->arch.max_vcpus = KVM_MAX_VCPUS;
>>  }
>>  
>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>> index cc8f5ed..59f1801 100644
>> --- a/virt/kvm/arm/vgic.c
>> +++ b/virt/kvm/arm/vgic.c
>> @@ -1830,9 +1830,6 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
>>  	kvm->arch.vgic.in_kernel = true;
>>  	kvm->arch.vgic.vgic_model = type;
>>  	kvm->arch.vgic.vctrl_base = vgic->vctrl_base;
>> -	kvm->arch.vgic.vgic_dist_base = VGIC_ADDR_UNDEF;
>> -	kvm->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF;
>> -	kvm->arch.vgic.vgic_redist_base = VGIC_ADDR_UNDEF;
>>  
>>  out_unlock:
>>  	for (; vcpu_lock_idx >= 0; vcpu_lock_idx--) {
>>
> 
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Auger Aug. 31, 2015, 9:31 a.m. UTC | #3
On 08/24/2015 07:24 PM, Andre Przywara wrote:
> Hi,
> 
> On 12/08/15 14:02, Eric Auger wrote:
>> On 07/10/2015 04:21 PM, Andre Przywara wrote:
>>> Currently we initialize all the possible GIC frame addresses in one
>>> function, without looking at the specific GIC model we instantiate
>>> for the guest.
>>> As this gets confusing when adding another VGIC model later, lets
>>> move these initializations into the respective model's init 
>> nit: tobe more precise the init emulation function (not the
>> vgic_v2/v3_init_model model's init function). pfouh?! ;-)
>> functions.
> 
> OK, will try to find a wording that is not completely confusing.
> 
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> ---
>>>  virt/kvm/arm/vgic-v2-emul.c | 3 +++
>>>  virt/kvm/arm/vgic-v3-emul.c | 3 +++
>>>  virt/kvm/arm/vgic.c         | 3 ---
>>>  3 files changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/virt/kvm/arm/vgic-v2-emul.c b/virt/kvm/arm/vgic-v2-emul.c
>>> index 1390797..8faa28c 100644
>>> --- a/virt/kvm/arm/vgic-v2-emul.c
>>> +++ b/virt/kvm/arm/vgic-v2-emul.c
>>> @@ -567,6 +567,9 @@ void vgic_v2_init_emulation(struct kvm *kvm)
>>>  	dist->vm_ops.init_model = vgic_v2_init_model;
>>>  	dist->vm_ops.map_resources = vgic_v2_map_resources;
>>>  
>>> +	dist->vgic_cpu_base = VGIC_ADDR_UNDEF;
>>> +	dist->vgic_dist_base = VGIC_ADDR_UNDEF;
>> Looks strange to see the common dist_base here. Why don't you leave it
>> in common part, kvm_vgic_create; all the more so you left
>> kvm->arch.vgic.vctrl_base = vgic->vctrl_base in kvm_vgic_create.
> 
> The idea behind this is that dist_base refers to similar, but not
> identical distributors (v2 vs. v3), so I found it a good idea to
> initialize it in here. Also vctrl_base is host facing and not set by
> userland, so this doesn't really compare here.
ok

Eric
> 
> Cheers,
> Andre.
> 
>>> +
>>>  	kvm->arch.max_vcpus = VGIC_V2_MAX_CPUS;
>>>  }
>>>  
>>> diff --git a/virt/kvm/arm/vgic-v3-emul.c b/virt/kvm/arm/vgic-v3-emul.c
>>> index d2eeb20..1f42348 100644
>>> --- a/virt/kvm/arm/vgic-v3-emul.c
>>> +++ b/virt/kvm/arm/vgic-v3-emul.c
>>> @@ -885,6 +885,9 @@ void vgic_v3_init_emulation(struct kvm *kvm)
>>>  	dist->vm_ops.destroy_model = vgic_v3_destroy_model;
>>>  	dist->vm_ops.map_resources = vgic_v3_map_resources;
>>>  
>>> +	dist->vgic_dist_base = VGIC_ADDR_UNDEF;
>>> +	dist->vgic_redist_base = VGIC_ADDR_UNDEF;
>>> +
>>>  	kvm->arch.max_vcpus = KVM_MAX_VCPUS;
>>>  }
>>>  
>>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>>> index cc8f5ed..59f1801 100644
>>> --- a/virt/kvm/arm/vgic.c
>>> +++ b/virt/kvm/arm/vgic.c
>>> @@ -1830,9 +1830,6 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
>>>  	kvm->arch.vgic.in_kernel = true;
>>>  	kvm->arch.vgic.vgic_model = type;
>>>  	kvm->arch.vgic.vctrl_base = vgic->vctrl_base;
>>> -	kvm->arch.vgic.vgic_dist_base = VGIC_ADDR_UNDEF;
>>> -	kvm->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF;
>>> -	kvm->arch.vgic.vgic_redist_base = VGIC_ADDR_UNDEF;
>>>  
>>>  out_unlock:
>>>  	for (; vcpu_lock_idx >= 0; vcpu_lock_idx--) {
>>>
>>

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/virt/kvm/arm/vgic-v2-emul.c b/virt/kvm/arm/vgic-v2-emul.c
index 1390797..8faa28c 100644
--- a/virt/kvm/arm/vgic-v2-emul.c
+++ b/virt/kvm/arm/vgic-v2-emul.c
@@ -567,6 +567,9 @@  void vgic_v2_init_emulation(struct kvm *kvm)
 	dist->vm_ops.init_model = vgic_v2_init_model;
 	dist->vm_ops.map_resources = vgic_v2_map_resources;
 
+	dist->vgic_cpu_base = VGIC_ADDR_UNDEF;
+	dist->vgic_dist_base = VGIC_ADDR_UNDEF;
+
 	kvm->arch.max_vcpus = VGIC_V2_MAX_CPUS;
 }
 
diff --git a/virt/kvm/arm/vgic-v3-emul.c b/virt/kvm/arm/vgic-v3-emul.c
index d2eeb20..1f42348 100644
--- a/virt/kvm/arm/vgic-v3-emul.c
+++ b/virt/kvm/arm/vgic-v3-emul.c
@@ -885,6 +885,9 @@  void vgic_v3_init_emulation(struct kvm *kvm)
 	dist->vm_ops.destroy_model = vgic_v3_destroy_model;
 	dist->vm_ops.map_resources = vgic_v3_map_resources;
 
+	dist->vgic_dist_base = VGIC_ADDR_UNDEF;
+	dist->vgic_redist_base = VGIC_ADDR_UNDEF;
+
 	kvm->arch.max_vcpus = KVM_MAX_VCPUS;
 }
 
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index cc8f5ed..59f1801 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -1830,9 +1830,6 @@  int kvm_vgic_create(struct kvm *kvm, u32 type)
 	kvm->arch.vgic.in_kernel = true;
 	kvm->arch.vgic.vgic_model = type;
 	kvm->arch.vgic.vctrl_base = vgic->vctrl_base;
-	kvm->arch.vgic.vgic_dist_base = VGIC_ADDR_UNDEF;
-	kvm->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF;
-	kvm->arch.vgic.vgic_redist_base = VGIC_ADDR_UNDEF;
 
 out_unlock:
 	for (; vcpu_lock_idx >= 0; vcpu_lock_idx--) {