On Fri, May 09 2014 at 3:06:03 pm BST, Christoffer Dall <christoffer.dall@xxxxxxxxxx> wrote:
> On Wed, Apr 16, 2014 at 02:39:39PM +0100, Marc Zyngier wrote:
>> Move the GICH_ELRSR access to its own function, and add it to the
>> vgic_ops structure.
>>
>> Acked-by: Catalin Marinas <catalin.marinas@xxxxxxx>
>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
>> ---
>> include/kvm/arm_vgic.h | 1 +
>> virt/kvm/arm/vgic.c | 20 ++++++++++++++++----
>> 2 files changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index 17bbe51..01013ec 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -84,6 +84,7 @@ struct vgic_lr {
>> struct vgic_ops {
>> struct vgic_lr (*get_lr)(const struct kvm_vcpu *, int);
>> void (*set_lr)(struct kvm_vcpu *, int, struct vgic_lr);
>> + u64 (*get_elrsr)(const struct kvm_vcpu *vcpu);
>> };
>>
>> struct vgic_dist {
>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>> index bac37b8..04206a8 100644
>> --- a/virt/kvm/arm/vgic.c
>> +++ b/virt/kvm/arm/vgic.c
>> @@ -1022,9 +1022,16 @@ static void vgic_v2_set_lr(struct kvm_vcpu *vcpu, int lr,
>> set_bit(lr, (unsigned long *)vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr);
>> }
>>
>> +static u64 vgic_v2_get_elrsr(const struct kvm_vcpu *vcpu)
>> +{
>> + const u32 *elrsr = vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr;
>> + return *(u64 *)elrsr;
>> +}
>> +
>> static const struct vgic_ops vgic_ops = {
>> .get_lr = vgic_v2_get_lr,
>> .set_lr = vgic_v2_set_lr,
>> + .get_elrsr = vgic_v2_get_elrsr,
>> };
>>
>> static inline struct vgic_lr vgic_get_lr(const struct kvm_vcpu *vcpu, int lr)
>> @@ -1038,6 +1045,11 @@ static inline void vgic_set_lr(struct kvm_vcpu *vcpu, int lr,
>> vgic_ops.set_lr(vcpu, lr, vlr);
>> }
>>
>> +static inline u64 vgic_get_elrsr(struct kvm_vcpu *vcpu)
>> +{
>> + return vgic_ops.get_elrsr(vcpu);
>> +}
>> +
>> static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu)
>> {
>> struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>> @@ -1278,14 +1290,15 @@ static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
>> {
>> struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>> struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>> + u64 elrsr = vgic_get_elrsr(vcpu);
>> + unsigned long *elrsr_ptr = (unsigned long *)&elrsr;
>
> looks to me like you're changing functionality here,
> vgic_process_maintenance() called below can modify the elrsr bitmap, but
> you're copying it into elrsr (local variable) here. Why du you even
> need the temporary elrsr variable here, and not just declare elrsr_ptr btw.?
Duh. Nice one. The whole reason for elrsr_ptr is to have a pointer to an
unsigned long which the bitmap ops can use... Don't say a word, I know.
>> int lr, pending;
>> bool level_pending;
>>
>> level_pending = vgic_process_maintenance(vcpu);
>>
>> /* Clear mappings for empty LRs */
>> - for_each_set_bit(lr, (unsigned long *)vgic_cpu->vgic_v2.vgic_elrsr,
>> - vgic_cpu->nr_lr) {
>> + for_each_set_bit(lr, elrsr_ptr, vgic_cpu->nr_lr) {
>> struct vgic_lr vlr;
>>
>> if (!test_and_clear_bit(lr, vgic_cpu->lr_used))
>> @@ -1298,8 +1311,7 @@ static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
>> }
>>
>> /* Check if we still have something up our sleeve... */
>> - pending = find_first_zero_bit((unsigned long *)vgic_cpu->vgic_v2.vgic_elrsr,
>> - vgic_cpu->nr_lr);
>> + pending = find_first_zero_bit(elrsr_ptr, vgic_cpu->nr_lr);
>> if (level_pending || pending < vgic_cpu->nr_lr)
>> set_bit(vcpu->vcpu_id, &dist->irq_pending_on_cpu);
>> }
>> --
>> 1.8.3.4
>>
> -Christoffer
>
--
Jazz is not dead. It just smells funny.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
