On 08/08/17 14:01, wanghaibin wrote:
> On 2017/7/25 19:25, Marc Zyngier wrote:
>
>> On Mon, Jul 17 2017 at 6:23:31 pm BST, wanghaibin <wanghaibin.wang@xxxxxxxxxx> wrote:
>>> This patch is used for GICv2 on GICv3.
>>>
>>> About GICV_APRn hardware register access,the SPEC says:
>>> When System register access is enabled for EL2, these registers access
>>> ICH_AP1Rn_EL2, and all active priorities for virtual machines are held
>>> in ICH_AP1Rn_EL2 regardless of interrupt group.
>>>
>>> For GICv3 hardware, we access the active priorities from ICH_AP1Rn_EL2
>>> in this scene.
>>>
>>> Signed-off-by: wanghaibin <wanghaibin.wang@xxxxxxxxxx>
>>> ---
>>> virt/kvm/arm/vgic/vgic-mmio.c | 12 ++++++++++++
>>> virt/kvm/arm/vgic/vgic-v3.c | 20 ++++++++++++++++++++
>>> virt/kvm/arm/vgic/vgic.h | 2 ++
>>> 3 files changed, 34 insertions(+)
>>>
>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
>>> index f6f3681..3b648a7 100644
>>> --- a/virt/kvm/arm/vgic/vgic-mmio.c
>>> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
>>> @@ -456,14 +456,26 @@ static int match_region(const void *key, const void *elt)
>>>
>>> void vgic_set_apr(struct kvm_vcpu *vcpu, u32 idx, u32 val)
>>> {
>>> + u32 vgic_model = vcpu->kvm->arch.vgic.vgic_model;
>>> +
>>> if (kvm_vgic_global_state.type == VGIC_V2)
>>> vgic_v2_set_apr(vcpu, idx, val);
>>> + else {
>>
>> Coding style.
>
>
> Will fix.
>
> Thanks
>
>>
>>> + if (vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
>>> + vgic_v3_set_apr(vcpu, 1, idx, val);
>>> + }
>>> }
>>>
>>> u32 vgic_get_apr(struct kvm_vcpu *vcpu, u32 idx)
>>> {
>>> + u32 vgic_model = vcpu->kvm->arch.vgic.vgic_model;
>>> +
>>> if (kvm_vgic_global_state.type == VGIC_V2)
>>> return vgic_v2_get_apr(vcpu, idx);
>>> + else {
>>
>> Same here.
>>
>>> + if (vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
>>> + return vgic_v3_get_apr(vcpu, 1, idx);
>>> + }
>>>
>>> return 0;
>>> }
>>> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
>>> index 030248e..da40681 100644
>>> --- a/virt/kvm/arm/vgic/vgic-v3.c
>>> +++ b/virt/kvm/arm/vgic/vgic-v3.c
>>> @@ -156,6 +156,26 @@ void vgic_v3_clear_lr(struct kvm_vcpu *vcpu, int lr)
>>> vcpu->arch.vgic_cpu.vgic_v3.vgic_lr[lr] = 0;
>>> }
>>>
>>> +void vgic_v3_set_apr(struct kvm_vcpu *vcpu, u8 group, u32 idx, u32 val)
>>> +{
>>> + struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3;
>>> +
>>> + if (group)
>>> + cpu_if->vgic_ap1r[idx] = val;
>>> + else
>>> + cpu_if->vgic_ap0r[idx] = val;
>>> +}
>>> +
>>> +u32 vgic_v3_get_apr(struct kvm_vcpu *vcpu, u8 group, u32 idx)
>>> +{
>>> + struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3;
>>> +
>>> + if (group)
>>> + return cpu_if->vgic_ap1r[idx];
>>> + else
>>> + return cpu_if->vgic_ap0r[idx];
>>> +}
>>> +
>>
>> How do we ensure that these APRs are even valid? We can have anything
>> from 5 to 7 bits of preemption, and thus 1 to 4 APRs. The rest is
>> UNDEFined.
>
>
> Sorry, Marc, I can't follow you.
>
> Do you suspect that an undefined APR register(implemented by ICH_VTR_EL2.PRIbits) will be accessed here?
Two implementations may not implement the same number of PRIbits. But
you seem to allow userspace to save/restore them, even if that doesn't
make sense.
> This code just access the soft vgic_ap[0,1]r structure, and I notice that, this structure saved the hardware
> APRn info under the constraint of ICH_VTR_EL2.PREbits implemented when save the vcpu content. the soft vgic_ap[0,1]r
> structure will keep 0 value when the hw APRn UNDEFined.
Yes, but userspace should be aware of this constraint, and access
register that cannot be used on this implementation.
To summarize, you need to be consistent. Something that cannot be used
for the guest shouldn't be available to userspace either.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
