Hi Marc,
On 30/05/2017 16:24, Marc Zyngier wrote:
> On 30/05/17 08:48, Auger Eric wrote:
>> Hi Marc
>>
>> On 03/05/2017 12:45, Marc Zyngier wrote:
>>> Add a handler for writing the guest's view of the ICC_EOIR1_EL1
>>> register. This involves dropping the priority of the interrupt,
>>> and deactivating it if required (EOImode == 0).
>>>
>>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
>>> ---
>>> include/linux/irqchip/arm-gic-v3.h | 2 +
>>> virt/kvm/arm/hyp/vgic-v3-sr.c | 119 +++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 121 insertions(+)
>>>
>>> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
>>> index 7610ea4e8337..c56d9bc2c904 100644
>>> --- a/include/linux/irqchip/arm-gic-v3.h
>>> +++ b/include/linux/irqchip/arm-gic-v3.h
>>> @@ -403,6 +403,8 @@
>>>
>>> #define ICH_HCR_EN (1 << 0)
>>> #define ICH_HCR_UIE (1 << 1)
>>> +#define ICH_HCR_EOIcount_SHIFT 27
>>> +#define ICH_HCR_EOIcount_MASK (0x1f << ICH_HCR_EOIcount_SHIFT)
>>>
>>> #define ICH_VMCR_CBPR_SHIFT 4
>>> #define ICH_VMCR_CBPR_MASK (1 << ICH_VMCR_CBPR_SHIFT)
>>> diff --git a/virt/kvm/arm/hyp/vgic-v3-sr.c b/virt/kvm/arm/hyp/vgic-v3-sr.c
>>> index 49aad1de3ac8..a76351b3ad66 100644
>>> --- a/virt/kvm/arm/hyp/vgic-v3-sr.c
>>> +++ b/virt/kvm/arm/hyp/vgic-v3-sr.c
>>> @@ -425,6 +425,26 @@ static int __hyp_text __vgic_v3_highest_priority_lr(struct kvm_vcpu *vcpu,
>>> return lr;
>>> }
>>>
>>> +static int __hyp_text __vgic_v3_find_active_lr(struct kvm_vcpu *vcpu,
>>> + int intid, u64 *lr_val)
>>> +{
>>> + unsigned int used_lrs = vcpu->arch.vgic_cpu.used_lrs;
>>> + int i;
>>> +
>>> + for (i = 0; i < used_lrs; i++) {
>>> + u64 val = __gic_v3_get_lr(i);
>>> +
>>> + if ((val & ICH_LR_VIRTUAL_ID_MASK) == intid &&
>>> + (val & ICH_LR_ACTIVE_BIT)) {
>> I guess it is safe because we don't have yet virtual interrupts directly
>> mapped to phys IRQs, besides timer one?
>
> What would that change? I don't see how having a HW interrupt here would
> be unsafe... Am I missing something?
I was thinking about the case of an HW mapped interrupt whose active
state must be observed at distributor level - as I understood the spec -
and not at LR level.
Thanks
Eric
>
>>> + *lr_val = val;
>>> + return i;
>>> + }
>>> + }
>>> +
>>> + *lr_val = ICC_IAR1_EL1_SPURIOUS;
>>> + return -1;
>>> +}
>>> +
>>> static int __hyp_text __vgic_v3_get_highest_active_priority(void)
>>> {
>>> u8 nr_pre_bits = vtr_to_nr_pre_bits(read_gicreg(ICH_VTR_EL2));
>>> @@ -490,6 +510,44 @@ static void __hyp_text __vgic_v3_set_active_priority(u8 pre)
>>> __vgic_v3_write_ap1rn(val | bit, apr);
>>> }
>>>
>>> +static int __hyp_text __vgic_v3_clear_highest_active_priority(void)
>>> +{
>>> + u8 nr_pre_bits = vtr_to_nr_pre_bits(read_gicreg(ICH_VTR_EL2));
>>> + u8 nr_aprs = 1 << (nr_pre_bits - 5);
>> may be worth to introduce a macro to compute the number of APRn regs.
>> This may be more understandable.
>
> Sure, will do.
>
>>> + u32 hap = 0;
>>> + int i;
>>> +
>>> + for (i = 0; i < nr_aprs; i++) {
>>> + u32 ap0, ap1;
>>> + int c0, c1;
>>> +
>>> + ap0 = __vgic_v3_read_ap0rn(i);
>>> + ap1 = __vgic_v3_read_ap1rn(i);
>>> + if (!ap0 && !ap1) {
>>> + hap += 32;
>>> + continue;
>>> + }
>>> +
>>> + c0 = ap0 ? __ffs(ap0) : 32;
>>> + c1 = ap1 ? __ffs(ap1) : 32;
>>> +
>>> + /* Always clear the LSB, which is the highest priority */
>>> + if (c0 < c1) {
>>> + ap0 &= ap0 - 1;
>>> + __vgic_v3_write_ap0rn(ap0, i);
>>> + hap += c0;
>>> + } else {
>>> + ap1 &= ap1 - 1;
>>> + __vgic_v3_write_ap1rn(ap1, i);
>>> + hap += c1;
>>> + }
>>> +
>>> + return hap << (8 - nr_pre_bits);
>>> + }
>>> +
>>> + return GICv3_IDLE_PRIORITY;
>>> +}
>>> +
>>> static void __hyp_text __vgic_v3_read_iar(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
>>> {
>>> u64 lr_val;
>>> @@ -526,6 +584,64 @@ static void __hyp_text __vgic_v3_read_iar(struct kvm_vcpu *vcpu, u32 vmcr, int r
>>> vcpu_set_reg(vcpu, rt, ICC_IAR1_EL1_SPURIOUS);
>>> }
>>>
>>> +static void __hyp_text __vgic_v3_clear_active_lr(int lr, u64 lr_val)
>>> +{
>>> + lr_val &= ~ICH_LR_ACTIVE_BIT;
>>> + if (lr_val & ICH_LR_HW) {
>>> + u32 pid;
>> nit: insert a line
>
> OK.
>
>>> + pid = (lr_val & ICH_LR_PHYS_ID_MASK) >> ICH_LR_PHYS_ID_SHIFT;
>>> + gic_write_dir(pid);
>>> + }
>>> +
>>> + __gic_v3_set_lr(lr_val, lr);
>>> +}
>>> +
>>> +static void __hyp_text __vgic_v3_bump_eoicount(void)
>>> +{
>>> + u32 hcr;
>>> +
>>> + hcr = read_gicreg(ICH_HCR_EL2);
>>> + hcr += 1 << ICH_HCR_EOIcount_SHIFT;
>>> + write_gicreg(hcr, ICH_HCR_EL2);
>>> +}
>>> +
>>> +static void __hyp_text __vgic_v3_write_eoir(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
>>> +{
>>> + u32 vid = vcpu_get_reg(vcpu, rt);
>>> + u64 lr_val;
>>> + u8 lr_prio, act_prio;
>>> + int lr, grp;
>>> +
>>> + grp = __vgic_v3_get_group(vcpu);
>>> +
>>> + /* Drop priority in any case */
>>> + act_prio = __vgic_v3_clear_highest_active_priority();
>>> +
>>> + /* If EOIing an LPI, no deactivate to be performed */
>>> + if (vid >= VGIC_MIN_LPI)
>>> + return;
>>> +
>>> + /* EOImode == 1, nothing to be done here */
>>> + if (vmcr & ICH_VMCR_EOIM_MASK)
>>> + return;
>>> +
>>> + lr = __vgic_v3_find_active_lr(vcpu, vid, &lr_val);
>>> + if (lr == -1) {
>>> + __vgic_v3_bump_eoicount();
>>> + return;
>>> + }
>>> +
>>> + lr_prio = (lr_val & ICH_LR_PRIORITY_MASK) >> ICH_LR_PRIORITY_SHIFT;
>>> +
>>> + /* If priorities or group do not match, the guest has fscked-up. */
>>> + if (grp != !!(lr_val & ICH_LR_GROUP) ||
>>> + __vgic_v3_pri_to_pre(lr_prio, vmcr, grp) != act_prio)
>>> + return;
>>> +
>>> + /* Let's now perform the deactivation */
>>> + __vgic_v3_clear_active_lr(lr, lr_val);
>>> +}
>>> +
>>> static void __hyp_text __vgic_v3_read_igrpen1(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
>>> {
>>> vcpu_set_reg(vcpu, rt, !!(vmcr & ICH_VMCR_ENG1_MASK));
>>> @@ -591,6 +707,9 @@ int __hyp_text __vgic_v3_perform_cpuif_access(struct kvm_vcpu *vcpu)
>>> case SYS_ICC_IAR1_EL1:
>>> fn = __vgic_v3_read_iar;
>>> break;
>>> + case SYS_ICC_EOIR1_EL1:
>>> + fn = __vgic_v3_write_eoir;
>>> + break;
>>> case SYS_ICC_GRPEN1_EL1:
>>> if (is_read)
>>> fn = __vgic_v3_read_igrpen1;
>>
>> Looks good to me
>>
>> Reviewed-by: Eric Auger <eric.auger@xxxxxxxxxx>
>
> Thanks!
>
> M.
>
