| Message ID | 20170503104606.19342-15-marc.zyngier@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@arm.com> > --- > 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? > + *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. > + 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 > + 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@redhat.com> Thanks Eric >
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@arm.com> >> --- >> 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? >> + *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@redhat.com> Thanks! M.
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@arm.com> >>> --- >>> 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@redhat.com> > > Thanks! > > M. >
On Wed, 31 May 2017 08:33:05 +0200 Auger Eric <eric.auger@redhat.com> wrote: > 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@arm.com> > >>> --- > >>> 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. What part of the spec are you referring to? A virtual interrupt, even backed by a HW interrupt, does have an active state (unless it is an LPI). The only state we cannot observe in the LR when the HW bit is set is the Active+Pending state because the pending bit is then at the physical distributor level. For all intent and purposes, the active state in the LR behaves the same, irrespective of the HW state, until the guest issues a deactivation (either using EOI or DIR, depending on EOImode). Thanks, M.
Hi Marc, On 31/05/2017 08:46, Marc Zyngier wrote: > On Wed, 31 May 2017 08:33:05 +0200 > Auger Eric <eric.auger@redhat.com> wrote: > >> 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@arm.com> >>>>> --- >>>>> 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. > > What part of the spec are you referring to? > > A virtual interrupt, even backed by a HW interrupt, does have an active > state (unless it is an LPI). The only state we cannot observe in the LR > when the HW bit is set is the Active+Pending state because the pending > bit is then at the physical distributor level. > > For all intent and purposes, the active state in the LR behaves the > same, irrespective of the HW state, until the guest issues a > deactivation (either using EOI or DIR, depending on EOImode). Hum OK. I was referring to the note in table 5-9 of IHI0048B2 but effectively I added a "s" where there is none :-( " For hardware interrupts, the pending and active state is held in the physical Distributor rather than the virtual CPU interface. A hypervisor must only use the pending and active state for software originated interrupts associated with virtual devices, or SGIs. " Sorry for the noise. Eric > > Thanks, > > M. >
On 31/05/17 08:26, Auger Eric wrote: > Hi Marc, > > On 31/05/2017 08:46, Marc Zyngier wrote: >> On Wed, 31 May 2017 08:33:05 +0200 >> Auger Eric <eric.auger@redhat.com> wrote: >> >>> 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@arm.com> >>>>>> --- >>>>>> 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. >> >> What part of the spec are you referring to? >> >> A virtual interrupt, even backed by a HW interrupt, does have an active >> state (unless it is an LPI). The only state we cannot observe in the LR >> when the HW bit is set is the Active+Pending state because the pending >> bit is then at the physical distributor level. >> >> For all intent and purposes, the active state in the LR behaves the >> same, irrespective of the HW state, until the guest issues a >> deactivation (either using EOI or DIR, depending on EOImode). > > Hum OK. I was referring to the note in table 5-9 of IHI0048B2 but > effectively I added a "s" where there is none :-( > > " > For hardware interrupts, the pending and active state is held in the > physical Distributor rather than the virtual > CPU interface. A hypervisor must only use the pending and active state > for software originated interrupts > associated with virtual devices, or SGIs. > " > > Sorry for the noise. No worries. I've pestered against this verbiage multiple times because it is incredibly misleading for non native speakers (I misinterpreted this "pending and active state" a couple of times myself). This is why I wrote Active+Pending above, because it is less ambiguous. The GICv3 spec contains the same subtlety in multiple places. Man, how I hate this thing, it is unbelievable. Thanks, M.
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)) { + *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); + 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; + 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;
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@arm.com> --- include/linux/irqchip/arm-gic-v3.h | 2 + virt/kvm/arm/hyp/vgic-v3-sr.c | 119 +++++++++++++++++++++++++++++++++++++ 2 files changed, 121 insertions(+)