* [RFC PATCH] kvm: arm: vgic-v3: add the emulate GICC_CTLR layout support for vmcr ctlr field. @ 2017-05-16 1:19 wanghaibin 2017-05-19 9:16 ` wanghaibin 2017-05-20 12:17 ` Christoffer Dall 0 siblings, 2 replies; 6+ messages in thread From: wanghaibin @ 2017-05-16 1:19 UTC (permalink / raw) To: zhaoshenglong, wu.wubin, christoffer.dall, marc.zyngier; +Cc: kvmarm Boot a virtual machine with the emulated GICv2 on the GICv3 hardware. Migrate the virtual machine will be successful, but the virtual machine will hang at the destination. The GICC_CTLR and ICC_CTLR_EL1 have the different layout. Currently, the set/get the VMCR interface just take vmcr ctlr field as the ICC_CTLR_EL1 layout. Should we consider the GICC_CTLR layout to avoid this problem? Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com> --- include/linux/irqchip/arm-gic-v3.h | 2 ++ virt/kvm/arm/vgic/vgic-v3.c | 48 ++++++++++++++++++++++++-------------- 2 files changed, 32 insertions(+), 18 deletions(-) diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h index 97cbca1..e40c711 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_VMCR_CTLR_SHIFT 0 +#define ICH_VMCR_CTLR_MASK (0x21f << ICH_VMCR_CTLR_SHIFT) #define ICH_VMCR_CBPR_SHIFT 4 #define ICH_VMCR_CBPR_MASK (1 << ICH_VMCR_CBPR_SHIFT) #define ICH_VMCR_EOIM_SHIFT 9 diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c index be0f4c3..66defae 100644 --- a/virt/kvm/arm/vgic/vgic-v3.c +++ b/virt/kvm/arm/vgic/vgic-v3.c @@ -174,19 +174,25 @@ void vgic_v3_clear_lr(struct kvm_vcpu *vcpu, int lr) void vgic_v3_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) { u32 vmcr; + u32 model = vcpu->kvm->arch.vgic.vgic_model; + + if (model == KVM_DEV_TYPE_ARM_VGIC_V2) + vmcr = (vmcrp->ctlr << ICH_VMCR_CTLR_SHIFT) & ICH_VMCR_CTLR_MASK; + else { + /* + * Ignore the FIQen bit, because GIC emulation always implies + * SRE=1 which means the vFIQEn bit is also RES1. + */ + vmcr = ((vmcrp->ctlr >> ICC_CTLR_EL1_EOImode_SHIFT) << + ICH_VMCR_EOIM_SHIFT) & ICH_VMCR_EOIM_MASK; + vmcr |= (vmcrp->ctlr << ICH_VMCR_CBPR_SHIFT) & ICH_VMCR_CBPR_MASK; + vmcr |= (vmcrp->grpen0 << ICH_VMCR_ENG0_SHIFT) & ICH_VMCR_ENG0_MASK; + vmcr |= (vmcrp->grpen1 << ICH_VMCR_ENG1_SHIFT) & ICH_VMCR_ENG1_MASK; + } - /* - * Ignore the FIQen bit, because GIC emulation always implies - * SRE=1 which means the vFIQEn bit is also RES1. - */ - vmcr = ((vmcrp->ctlr >> ICC_CTLR_EL1_EOImode_SHIFT) << - ICH_VMCR_EOIM_SHIFT) & ICH_VMCR_EOIM_MASK; - vmcr |= (vmcrp->ctlr << ICH_VMCR_CBPR_SHIFT) & ICH_VMCR_CBPR_MASK; vmcr |= (vmcrp->abpr << ICH_VMCR_BPR1_SHIFT) & ICH_VMCR_BPR1_MASK; vmcr |= (vmcrp->bpr << ICH_VMCR_BPR0_SHIFT) & ICH_VMCR_BPR0_MASK; vmcr |= (vmcrp->pmr << ICH_VMCR_PMR_SHIFT) & ICH_VMCR_PMR_MASK; - vmcr |= (vmcrp->grpen0 << ICH_VMCR_ENG0_SHIFT) & ICH_VMCR_ENG0_MASK; - vmcr |= (vmcrp->grpen1 << ICH_VMCR_ENG1_SHIFT) & ICH_VMCR_ENG1_MASK; vcpu->arch.vgic_cpu.vgic_v3.vgic_vmcr = vmcr; } @@ -194,19 +200,25 @@ void vgic_v3_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) void vgic_v3_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) { u32 vmcr = vcpu->arch.vgic_cpu.vgic_v3.vgic_vmcr; + u32 model = vcpu->kvm->arch.vgic.vgic_model; + + if (model == KVM_DEV_TYPE_ARM_VGIC_V2) + vmcrp->ctlr = (vmcr & ICH_VMCR_CTLR_MASK) >> ICH_VMCR_CTLR_SHIFT; + else { + /* + * Ignore the FIQen bit, because GIC emulation always implies + * SRE=1 which means the vFIQEn bit is also RES1. + */ + vmcrp->ctlr = ((vmcr >> ICH_VMCR_EOIM_SHIFT) << + ICC_CTLR_EL1_EOImode_SHIFT) & ICC_CTLR_EL1_EOImode_MASK; + vmcrp->ctlr |= (vmcr & ICH_VMCR_CBPR_MASK) >> ICH_VMCR_CBPR_SHIFT; + vmcrp->grpen0 = (vmcr & ICH_VMCR_ENG0_MASK) >> ICH_VMCR_ENG0_SHIFT; + vmcrp->grpen1 = (vmcr & ICH_VMCR_ENG1_MASK) >> ICH_VMCR_ENG1_SHIFT; + } - /* - * Ignore the FIQen bit, because GIC emulation always implies - * SRE=1 which means the vFIQEn bit is also RES1. - */ - vmcrp->ctlr = ((vmcr >> ICH_VMCR_EOIM_SHIFT) << - ICC_CTLR_EL1_EOImode_SHIFT) & ICC_CTLR_EL1_EOImode_MASK; - vmcrp->ctlr |= (vmcr & ICH_VMCR_CBPR_MASK) >> ICH_VMCR_CBPR_SHIFT; vmcrp->abpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT; vmcrp->bpr = (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT; vmcrp->pmr = (vmcr & ICH_VMCR_PMR_MASK) >> ICH_VMCR_PMR_SHIFT; - vmcrp->grpen0 = (vmcr & ICH_VMCR_ENG0_MASK) >> ICH_VMCR_ENG0_SHIFT; - vmcrp->grpen1 = (vmcr & ICH_VMCR_ENG1_MASK) >> ICH_VMCR_ENG1_SHIFT; } #define INITIAL_PENDBASER_VALUE \ -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] kvm: arm: vgic-v3: add the emulate GICC_CTLR layout support for vmcr ctlr field. 2017-05-16 1:19 [RFC PATCH] kvm: arm: vgic-v3: add the emulate GICC_CTLR layout support for vmcr ctlr field wanghaibin @ 2017-05-19 9:16 ` wanghaibin 2017-05-20 12:17 ` Christoffer Dall 1 sibling, 0 replies; 6+ messages in thread From: wanghaibin @ 2017-05-19 9:16 UTC (permalink / raw) To: wanghaibin; +Cc: marc.zyngier, kvmarm, wu.wubin On 2017/5/16 9:19, wanghaibin wrote: > Boot a virtual machine with the emulated GICv2 on the GICv3 hardware. > Migrate the virtual machine will be successful, but the virtual machine will > hang at the destination. > > The GICC_CTLR and ICC_CTLR_EL1 have the different layout. Currently, the set/get > the VMCR interface just take vmcr ctlr field as the ICC_CTLR_EL1 layout. > Should we consider the GICC_CTLR layout to avoid this problem? Ping .. BTW: I test this case on hisilicon D03 board, and it's like the commit (5fb247d79c04240dce86c842976cde1edde7f7ed) introduced this problem which doesn't consider the compatible of GICC_CTLR layoyut. + the patch author Vijaya Thanks. > > Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com> > --- > include/linux/irqchip/arm-gic-v3.h | 2 ++ > virt/kvm/arm/vgic/vgic-v3.c | 48 ++++++++++++++++++++++++-------------- > 2 files changed, 32 insertions(+), 18 deletions(-) > > diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h > index 97cbca1..e40c711 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_VMCR_CTLR_SHIFT 0 > +#define ICH_VMCR_CTLR_MASK (0x21f << ICH_VMCR_CTLR_SHIFT) > #define ICH_VMCR_CBPR_SHIFT 4 > #define ICH_VMCR_CBPR_MASK (1 << ICH_VMCR_CBPR_SHIFT) > #define ICH_VMCR_EOIM_SHIFT 9 > diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c > index be0f4c3..66defae 100644 > --- a/virt/kvm/arm/vgic/vgic-v3.c > +++ b/virt/kvm/arm/vgic/vgic-v3.c > @@ -174,19 +174,25 @@ void vgic_v3_clear_lr(struct kvm_vcpu *vcpu, int lr) > void vgic_v3_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) > { > u32 vmcr; > + u32 model = vcpu->kvm->arch.vgic.vgic_model; > + > + if (model == KVM_DEV_TYPE_ARM_VGIC_V2) > + vmcr = (vmcrp->ctlr << ICH_VMCR_CTLR_SHIFT) & ICH_VMCR_CTLR_MASK; > + else { > + /* > + * Ignore the FIQen bit, because GIC emulation always implies > + * SRE=1 which means the vFIQEn bit is also RES1. > + */ > + vmcr = ((vmcrp->ctlr >> ICC_CTLR_EL1_EOImode_SHIFT) << > + ICH_VMCR_EOIM_SHIFT) & ICH_VMCR_EOIM_MASK; > + vmcr |= (vmcrp->ctlr << ICH_VMCR_CBPR_SHIFT) & ICH_VMCR_CBPR_MASK; > + vmcr |= (vmcrp->grpen0 << ICH_VMCR_ENG0_SHIFT) & ICH_VMCR_ENG0_MASK; > + vmcr |= (vmcrp->grpen1 << ICH_VMCR_ENG1_SHIFT) & ICH_VMCR_ENG1_MASK; > + } > > - /* > - * Ignore the FIQen bit, because GIC emulation always implies > - * SRE=1 which means the vFIQEn bit is also RES1. > - */ > - vmcr = ((vmcrp->ctlr >> ICC_CTLR_EL1_EOImode_SHIFT) << > - ICH_VMCR_EOIM_SHIFT) & ICH_VMCR_EOIM_MASK; > - vmcr |= (vmcrp->ctlr << ICH_VMCR_CBPR_SHIFT) & ICH_VMCR_CBPR_MASK; > vmcr |= (vmcrp->abpr << ICH_VMCR_BPR1_SHIFT) & ICH_VMCR_BPR1_MASK; > vmcr |= (vmcrp->bpr << ICH_VMCR_BPR0_SHIFT) & ICH_VMCR_BPR0_MASK; > vmcr |= (vmcrp->pmr << ICH_VMCR_PMR_SHIFT) & ICH_VMCR_PMR_MASK; > - vmcr |= (vmcrp->grpen0 << ICH_VMCR_ENG0_SHIFT) & ICH_VMCR_ENG0_MASK; > - vmcr |= (vmcrp->grpen1 << ICH_VMCR_ENG1_SHIFT) & ICH_VMCR_ENG1_MASK; > > vcpu->arch.vgic_cpu.vgic_v3.vgic_vmcr = vmcr; > } > @@ -194,19 +200,25 @@ void vgic_v3_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) > void vgic_v3_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) > { > u32 vmcr = vcpu->arch.vgic_cpu.vgic_v3.vgic_vmcr; > + u32 model = vcpu->kvm->arch.vgic.vgic_model; > + > + if (model == KVM_DEV_TYPE_ARM_VGIC_V2) > + vmcrp->ctlr = (vmcr & ICH_VMCR_CTLR_MASK) >> ICH_VMCR_CTLR_SHIFT; > + else { > + /* > + * Ignore the FIQen bit, because GIC emulation always implies > + * SRE=1 which means the vFIQEn bit is also RES1. > + */ > + vmcrp->ctlr = ((vmcr >> ICH_VMCR_EOIM_SHIFT) << > + ICC_CTLR_EL1_EOImode_SHIFT) & ICC_CTLR_EL1_EOImode_MASK; > + vmcrp->ctlr |= (vmcr & ICH_VMCR_CBPR_MASK) >> ICH_VMCR_CBPR_SHIFT; > + vmcrp->grpen0 = (vmcr & ICH_VMCR_ENG0_MASK) >> ICH_VMCR_ENG0_SHIFT; > + vmcrp->grpen1 = (vmcr & ICH_VMCR_ENG1_MASK) >> ICH_VMCR_ENG1_SHIFT; > + } > > - /* > - * Ignore the FIQen bit, because GIC emulation always implies > - * SRE=1 which means the vFIQEn bit is also RES1. > - */ > - vmcrp->ctlr = ((vmcr >> ICH_VMCR_EOIM_SHIFT) << > - ICC_CTLR_EL1_EOImode_SHIFT) & ICC_CTLR_EL1_EOImode_MASK; > - vmcrp->ctlr |= (vmcr & ICH_VMCR_CBPR_MASK) >> ICH_VMCR_CBPR_SHIFT; > vmcrp->abpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT; > vmcrp->bpr = (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT; > vmcrp->pmr = (vmcr & ICH_VMCR_PMR_MASK) >> ICH_VMCR_PMR_SHIFT; > - vmcrp->grpen0 = (vmcr & ICH_VMCR_ENG0_MASK) >> ICH_VMCR_ENG0_SHIFT; > - vmcrp->grpen1 = (vmcr & ICH_VMCR_ENG1_MASK) >> ICH_VMCR_ENG1_SHIFT; > } > > #define INITIAL_PENDBASER_VALUE \ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] kvm: arm: vgic-v3: add the emulate GICC_CTLR layout support for vmcr ctlr field. 2017-05-16 1:19 [RFC PATCH] kvm: arm: vgic-v3: add the emulate GICC_CTLR layout support for vmcr ctlr field wanghaibin 2017-05-19 9:16 ` wanghaibin @ 2017-05-20 12:17 ` Christoffer Dall 2017-05-23 16:33 ` Marc Zyngier 1 sibling, 1 reply; 6+ messages in thread From: Christoffer Dall @ 2017-05-20 12:17 UTC (permalink / raw) To: wanghaibin; +Cc: marc.zyngier, kvmarm, wu.wubin Hi Wanghaibin, On Tue, May 16, 2017 at 09:19:26AM +0800, wanghaibin wrote: > Boot a virtual machine with the emulated GICv2 on the GICv3 hardware. > Migrate the virtual machine will be successful, but the virtual machine will > hang at the destination. > > The GICC_CTLR and ICC_CTLR_EL1 have the different layout. Currently, the set/get > the VMCR interface just take vmcr ctlr field as the ICC_CTLR_EL1 layout. > Should we consider the GICC_CTLR layout to avoid this problem? > > Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com> Nice find. I agree with the problem, but I don't agree with your solution, because it's really hard to figure out the mappings, I think. How about this (untested) patch instead? Could you test the migration on your setup? commit 7c94545b6801840364647d5deb81f47da5e1f547 (HEAD -> vmcr-gicv2on3-cleanup, kernelorg/vmcr-gicv2on3-cleanup) Author: Christoffer Dall <cdall@linaro.org> Date: Sat May 20 14:12:34 2017 +0200 KVM: arm/arm64: Fix isues with GICv2 on GICv3 migration We have been a little loose with our intermediate VMCR representation where we had a 'ctlr' field, but we failed to differentiate between the GICv2 GICC_CTLR and ICC_CTLR_EL1 layouts, and therefore ended up mapping the wrong bits into the individual fields of the ICH_VMCR_EL2 when emulating a GICv2 on a GICv3 system. Fix this by using explicit fields for the VMCR bits instead. Cc: Marc Zyngier <marc.zyngier@arm.com> Cc: Eric Auger <eric.auger@redhat.com> Reported-by: wanghaibin <wanghaibin.wang@huawei.com> Signed-off-by: Christoffer Dall <cdall@linaro.org> diff --git a/arch/arm64/kvm/vgic-sys-reg-v3.c b/arch/arm64/kvm/vgic-sys-reg-v3.c index 79f37e3..6260b69 100644 --- a/arch/arm64/kvm/vgic-sys-reg-v3.c +++ b/arch/arm64/kvm/vgic-sys-reg-v3.c @@ -65,8 +65,8 @@ static bool access_gic_ctlr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, * Here set VMCR.CTLR in ICC_CTLR_EL1 layout. * The vgic_set_vmcr() will convert to ICH_VMCR layout. */ - vmcr.ctlr = val & ICC_CTLR_EL1_CBPR_MASK; - vmcr.ctlr |= val & ICC_CTLR_EL1_EOImode_MASK; + vmcr.cbpr = (val & ICC_CTLR_EL1_CBPR_MASK) >> ICC_CTLR_EL1_CBPR_SHIFT; + vmcr.eoim = (val & ICC_CTLR_EL1_EOImode_MASK) >> ICC_CTLR_EL1_EOImode_SHIFT; vgic_set_vmcr(vcpu, &vmcr); } else { val = 0; @@ -83,8 +83,8 @@ static bool access_gic_ctlr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, * The VMCR.CTLR value is in ICC_CTLR_EL1 layout. * Extract it directly using ICC_CTLR_EL1 reg definitions. */ - val |= vmcr.ctlr & ICC_CTLR_EL1_CBPR_MASK; - val |= vmcr.ctlr & ICC_CTLR_EL1_EOImode_MASK; + val |= (vmcr.cbpr << ICC_CTLR_EL1_CBPR_SHIFT) & ICC_CTLR_EL1_CBPR_MASK; + val |= (vmcr.eoim << ICC_CTLR_EL1_EOImode_SHIFT) & ICC_CTLR_EL1_EOImode_MASK; p->regval = val; } @@ -135,7 +135,7 @@ static bool access_gic_bpr1(struct kvm_vcpu *vcpu, struct sys_reg_params *p, p->regval = 0; vgic_get_vmcr(vcpu, &vmcr); - if (!((vmcr.ctlr & ICH_VMCR_CBPR_MASK) >> ICH_VMCR_CBPR_SHIFT)) { + if (!vmcr.cbpr) { if (p->is_write) { vmcr.abpr = (p->regval & ICC_BPR1_EL1_MASK) >> ICC_BPR1_EL1_SHIFT; diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h index fffb912..1fa293a 100644 --- a/include/linux/irqchip/arm-gic-v3.h +++ b/include/linux/irqchip/arm-gic-v3.h @@ -417,6 +417,10 @@ #define ICH_HCR_EN (1 << 0) #define ICH_HCR_UIE (1 << 1) +#define ICH_VMCR_ACK_CTL_SHIFT 2 +#define ICH_VMCR_ACK_CTL_MASK (1 << ICH_VMCR_ACK_CTL_SHIFT) +#define ICH_VMCR_FIQ_EN_SHIFT 3 +#define ICH_VMCR_FIQ_EN_MASK (1 << ICH_VMCR_FIQ_EN_SHIFT) #define ICH_VMCR_CBPR_SHIFT 4 #define ICH_VMCR_CBPR_MASK (1 << ICH_VMCR_CBPR_SHIFT) #define ICH_VMCR_EOIM_SHIFT 9 diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h index dc30f3d..0baebcb 100644 --- a/include/linux/irqchip/arm-gic.h +++ b/include/linux/irqchip/arm-gic.h @@ -25,6 +25,11 @@ #define GICC_ENABLE 0x1 #define GICC_INT_PRI_THRESHOLD 0xf0 +#define GIC_CPU_CTRL_EnableGrp0 (1 << 0) +#define GIC_CPU_CTRL_EnableGrp1 (1 << 1) +#define GIC_CPU_CTRL_AckCtl (1 << 2) +#define GIC_CPU_CTRL_FIQEn (1 << 3) +#define GIC_CPU_CTRL_CBPR (1 << 4) #define GIC_CPU_CTRL_EOImodeNS (1 << 9) #define GICC_IAR_INT_ID_MASK 0x3ff @@ -84,8 +89,19 @@ #define GICH_LR_EOI (1 << 19) #define GICH_LR_HW (1 << 31) -#define GICH_VMCR_CTRL_SHIFT 0 -#define GICH_VMCR_CTRL_MASK (0x21f << GICH_VMCR_CTRL_SHIFT) +#define GICH_VMCR_ENABLE_GRP0_SHIFT 0 +#define GICH_VMCR_ENABLE_GRP0_MASK (1 << GICH_VMCR_ENABLE_GRP0_SHIFT) +#define GICH_VMCR_ENABLE_GRP1_SHIFT 1 +#define GICH_VMCR_ENABLE_GRP1_MASK (1 << GICH_VMCR_ENABLE_GRP1_SHIFT) +#define GICH_VMCR_ACK_CTL_SHIFT 2 +#define GICH_VMCR_ACK_CTL_MASK (1 << GICH_VMCR_ACK_CTL_SHIFT) +#define GICH_VMCR_FIQ_EN_SHIFT 3 +#define GICH_VMCR_FIQ_EN_MASK (1 << GICH_VMCR_FIQ_EN_SHIFT) +#define GICH_VMCR_CBPR_SHIFT 4 +#define GICH_VMCR_CBPR_MASK (1 << GICH_VMCR_CBPR_SHIFT) +#define GICH_VMCR_EOI_MODE_SHIFT 9 +#define GICH_VMCR_EOI_MODE_MASK (1 << GICH_VMCR_EOI_MODE_SHIFT) + #define GICH_VMCR_PRIMASK_SHIFT 27 #define GICH_VMCR_PRIMASK_MASK (0x1f << GICH_VMCR_PRIMASK_SHIFT) #define GICH_VMCR_BINPOINT_SHIFT 21 diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c index 0a4283e..0b71a46 100644 --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c @@ -226,7 +226,13 @@ static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu, switch (addr & 0xff) { case GIC_CPU_CTRL: - val = vmcr.ctlr; + val = vmcr.grpen0 << __ffs(GIC_CPU_CTRL_EnableGrp0); + val |= vmcr.grpen1 << __ffs(GIC_CPU_CTRL_EnableGrp1); + val |= vmcr.ackctl << __ffs(GIC_CPU_CTRL_AckCtl); + val |= vmcr.fiqen << __ffs(GIC_CPU_CTRL_FIQEn); + val |= vmcr.cbpr << __ffs(GIC_CPU_CTRL_CBPR); + val |= vmcr.eoim << __ffs(GIC_CPU_CTRL_EOImodeNS); + break; case GIC_CPU_PRIMASK: /* @@ -267,7 +273,13 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu, switch (addr & 0xff) { case GIC_CPU_CTRL: - vmcr.ctlr = val; + vmcr.grpen0 = !!(val & GIC_CPU_CTRL_EnableGrp0); + vmcr.grpen1 = !!(val & GIC_CPU_CTRL_EnableGrp1); + vmcr.ackctl = !!(val & GIC_CPU_CTRL_AckCtl); + vmcr.fiqen = !!(val & GIC_CPU_CTRL_FIQEn); + vmcr.cbpr = !!(val & GIC_CPU_CTRL_CBPR); + vmcr.eoim = !!(val & GIC_CPU_CTRL_EOImodeNS); + break; case GIC_CPU_PRIMASK: /* diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c index 504b4bd..e4187e5 100644 --- a/virt/kvm/arm/vgic/vgic-v2.c +++ b/virt/kvm/arm/vgic/vgic-v2.c @@ -177,7 +177,18 @@ void vgic_v2_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) struct vgic_v2_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v2; u32 vmcr; - vmcr = (vmcrp->ctlr << GICH_VMCR_CTRL_SHIFT) & GICH_VMCR_CTRL_MASK; + vmcr = (vmcrp->grpen0 << GICH_VMCR_ENABLE_GRP0_SHIFT) & + GICH_VMCR_ENABLE_GRP0_MASK; + vmcr |= (vmcrp->grpen1 << GICH_VMCR_ENABLE_GRP1_SHIFT) & + GICH_VMCR_ENABLE_GRP1_MASK; + vmcr |= (vmcrp->ackctl << GICH_VMCR_ACK_CTL_SHIFT) & + GICH_VMCR_ACK_CTL_MASK; + vmcr |= (vmcrp->fiqen << GICH_VMCR_FIQ_EN_SHIFT) & + GICH_VMCR_FIQ_EN_MASK; + vmcr |= (vmcrp->cbpr << GICH_VMCR_CBPR_SHIFT) & + GICH_VMCR_CBPR_MASK; + vmcr |= (vmcrp->eoim << GICH_VMCR_EOI_MODE_SHIFT) & + GICH_VMCR_EOI_MODE_MASK; vmcr |= (vmcrp->abpr << GICH_VMCR_ALIAS_BINPOINT_SHIFT) & GICH_VMCR_ALIAS_BINPOINT_MASK; vmcr |= (vmcrp->bpr << GICH_VMCR_BINPOINT_SHIFT) & @@ -195,8 +206,19 @@ void vgic_v2_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) vmcr = cpu_if->vgic_vmcr; - vmcrp->ctlr = (vmcr & GICH_VMCR_CTRL_MASK) >> - GICH_VMCR_CTRL_SHIFT; + vmcrp->grpen0 = (vmcr & GICH_VMCR_ENABLE_GRP0_MASK) >> + GICH_VMCR_ENABLE_GRP0_SHIFT; + vmcrp->grpen1 = (vmcr & GICH_VMCR_ENABLE_GRP1_MASK) >> + GICH_VMCR_ENABLE_GRP1_SHIFT; + vmcrp->ackctl = (vmcr & GICH_VMCR_ACK_CTL_MASK) >> + GICH_VMCR_ACK_CTL_SHIFT; + vmcrp->fiqen = (vmcr & GICH_VMCR_FIQ_EN_MASK) >> + GICH_VMCR_FIQ_EN_SHIFT; + vmcrp->cbpr = (vmcr & GICH_VMCR_CBPR_MASK) >> + GICH_VMCR_CBPR_SHIFT; + vmcrp->eoim = (vmcr & GICH_VMCR_EOI_MODE_MASK) >> + GICH_VMCR_EOI_MODE_SHIFT; + vmcrp->abpr = (vmcr & GICH_VMCR_ALIAS_BINPOINT_MASK) >> GICH_VMCR_ALIAS_BINPOINT_SHIFT; vmcrp->bpr = (vmcr & GICH_VMCR_BINPOINT_MASK) >> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c index 6fe3f00..051cbde 100644 --- a/virt/kvm/arm/vgic/vgic-v3.c +++ b/virt/kvm/arm/vgic/vgic-v3.c @@ -159,15 +159,24 @@ void vgic_v3_clear_lr(struct kvm_vcpu *vcpu, int lr) void vgic_v3_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) { struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3; - u32 vmcr; + u32 model = vcpu->kvm->arch.vgic.vgic_model; + u32 vmcr = 0; - /* - * Ignore the FIQen bit, because GIC emulation always implies - * SRE=1 which means the vFIQEn bit is also RES1. - */ - vmcr = ((vmcrp->ctlr >> ICC_CTLR_EL1_EOImode_SHIFT) << - ICH_VMCR_EOIM_SHIFT) & ICH_VMCR_EOIM_MASK; - vmcr |= (vmcrp->ctlr << ICH_VMCR_CBPR_SHIFT) & ICH_VMCR_CBPR_MASK; + if (model == KVM_DEV_TYPE_ARM_VGIC_V2) { + vmcr |= (vmcrp->ackctl << ICH_VMCR_ACK_CTL_SHIFT) & + ICH_VMCR_ACK_CTL_MASK; + vmcr |= (vmcrp->fiqen << ICH_VMCR_FIQ_EN_SHIFT) & + ICH_VMCR_FIQ_EN_MASK; + } else { + /* + * When emulating GICv3 on GICv3 with SRE=1 on the + * VFIQEn bit is RES1 and the VAckCtl bit is RES0. + */ + vmcr = ICH_VMCR_FIQ_EN_MASK; + } + + vmcr |= (vmcrp->cbpr << ICH_VMCR_CBPR_SHIFT) & ICH_VMCR_CBPR_MASK; + vmcr |= (vmcrp->eoim << ICH_VMCR_EOIM_SHIFT) & ICH_VMCR_EOIM_MASK; vmcr |= (vmcrp->abpr << ICH_VMCR_BPR1_SHIFT) & ICH_VMCR_BPR1_MASK; vmcr |= (vmcrp->bpr << ICH_VMCR_BPR0_SHIFT) & ICH_VMCR_BPR0_MASK; vmcr |= (vmcrp->pmr << ICH_VMCR_PMR_SHIFT) & ICH_VMCR_PMR_MASK; @@ -180,17 +189,27 @@ void vgic_v3_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) void vgic_v3_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) { struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3; + u32 model = vcpu->kvm->arch.vgic.vgic_model; u32 vmcr; vmcr = cpu_if->vgic_vmcr; - /* - * Ignore the FIQen bit, because GIC emulation always implies - * SRE=1 which means the vFIQEn bit is also RES1. - */ - vmcrp->ctlr = ((vmcr >> ICH_VMCR_EOIM_SHIFT) << - ICC_CTLR_EL1_EOImode_SHIFT) & ICC_CTLR_EL1_EOImode_MASK; - vmcrp->ctlr |= (vmcr & ICH_VMCR_CBPR_MASK) >> ICH_VMCR_CBPR_SHIFT; + if (model == KVM_DEV_TYPE_ARM_VGIC_V2) { + vmcrp->ackctl = (vmcr & ICH_VMCR_ACK_CTL_MASK) >> + ICH_VMCR_ACK_CTL_SHIFT; + vmcrp->fiqen = (vmcr & ICH_VMCR_FIQ_EN_MASK) >> + ICH_VMCR_FIQ_EN_SHIFT; + } else { + /* + * When emulating GICv3 on GICv3 with SRE=1 on the + * VFIQEn bit is RES1 and the VAckCtl bit is RES0. + */ + vmcrp->fiqen = 1; + vmcrp->ackctl = 0; + } + + vmcrp->cbpr = (vmcr & ICH_VMCR_CBPR_MASK) >> ICH_VMCR_CBPR_SHIFT; + vmcrp->eoim = (vmcr & ICH_VMCR_EOIM_MASK) >> ICH_VMCR_EOIM_SHIFT; vmcrp->abpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT; vmcrp->bpr = (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT; vmcrp->pmr = (vmcr & ICH_VMCR_PMR_MASK) >> ICH_VMCR_PMR_SHIFT; diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h index da83e4c..bba7fa2 100644 --- a/virt/kvm/arm/vgic/vgic.h +++ b/virt/kvm/arm/vgic/vgic.h @@ -111,14 +111,18 @@ static inline bool irq_is_pending(struct vgic_irq *irq) * registers regardless of the hardware backed GIC used. */ struct vgic_vmcr { - u32 ctlr; + u32 grpen0; + u32 grpen1; + + u32 ackctl; + u32 fiqen; + u32 cbpr; + u32 eoim; + u32 abpr; u32 bpr; u32 pmr; /* Priority mask field in the GICC_PMR and * ICC_PMR_EL1 priority field format */ - /* Below member variable are valid only for GICv3 */ - u32 grpen0; - u32 grpen1; }; struct vgic_reg_attr { Thanks, -Christoffer ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] kvm: arm: vgic-v3: add the emulate GICC_CTLR layout support for vmcr ctlr field. 2017-05-20 12:17 ` Christoffer Dall @ 2017-05-23 16:33 ` Marc Zyngier 2017-05-23 16:55 ` Christoffer Dall 0 siblings, 1 reply; 6+ messages in thread From: Marc Zyngier @ 2017-05-23 16:33 UTC (permalink / raw) To: Christoffer Dall, wanghaibin; +Cc: kvmarm, wu.wubin On 20/05/17 13:17, Christoffer Dall wrote: > Hi Wanghaibin, > > On Tue, May 16, 2017 at 09:19:26AM +0800, wanghaibin wrote: >> Boot a virtual machine with the emulated GICv2 on the GICv3 hardware. >> Migrate the virtual machine will be successful, but the virtual machine will >> hang at the destination. >> >> The GICC_CTLR and ICC_CTLR_EL1 have the different layout. Currently, the set/get >> the VMCR interface just take vmcr ctlr field as the ICC_CTLR_EL1 layout. >> Should we consider the GICC_CTLR layout to avoid this problem? >> >> Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com> > > Nice find. > > I agree with the problem, but I don't agree with your solution, because > it's really hard to figure out the mappings, I think. > > How about this (untested) patch instead? Could you test the migration > on your setup? > > commit 7c94545b6801840364647d5deb81f47da5e1f547 (HEAD -> vmcr-gicv2on3-cleanup, kernelorg/vmcr-gicv2on3-cleanup) > Author: Christoffer Dall <cdall@linaro.org> > Date: Sat May 20 14:12:34 2017 +0200 > > KVM: arm/arm64: Fix isues with GICv2 on GICv3 migration > > We have been a little loose with our intermediate VMCR representation > where we had a 'ctlr' field, but we failed to differentiate between the > GICv2 GICC_CTLR and ICC_CTLR_EL1 layouts, and therefore ended up mapping > the wrong bits into the individual fields of the ICH_VMCR_EL2 when > emulating a GICv2 on a GICv3 system. > > Fix this by using explicit fields for the VMCR bits instead. > > Cc: Marc Zyngier <marc.zyngier@arm.com> > Cc: Eric Auger <eric.auger@redhat.com> > Reported-by: wanghaibin <wanghaibin.wang@huawei.com> > Signed-off-by: Christoffer Dall <cdall@linaro.org> > > diff --git a/arch/arm64/kvm/vgic-sys-reg-v3.c b/arch/arm64/kvm/vgic-sys-reg-v3.c > index 79f37e3..6260b69 100644 > --- a/arch/arm64/kvm/vgic-sys-reg-v3.c > +++ b/arch/arm64/kvm/vgic-sys-reg-v3.c > @@ -65,8 +65,8 @@ static bool access_gic_ctlr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, > * Here set VMCR.CTLR in ICC_CTLR_EL1 layout. > * The vgic_set_vmcr() will convert to ICH_VMCR layout. > */ > - vmcr.ctlr = val & ICC_CTLR_EL1_CBPR_MASK; > - vmcr.ctlr |= val & ICC_CTLR_EL1_EOImode_MASK; > + vmcr.cbpr = (val & ICC_CTLR_EL1_CBPR_MASK) >> ICC_CTLR_EL1_CBPR_SHIFT; > + vmcr.eoim = (val & ICC_CTLR_EL1_EOImode_MASK) >> ICC_CTLR_EL1_EOImode_SHIFT; > vgic_set_vmcr(vcpu, &vmcr); > } else { > val = 0; > @@ -83,8 +83,8 @@ static bool access_gic_ctlr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, > * The VMCR.CTLR value is in ICC_CTLR_EL1 layout. > * Extract it directly using ICC_CTLR_EL1 reg definitions. > */ > - val |= vmcr.ctlr & ICC_CTLR_EL1_CBPR_MASK; > - val |= vmcr.ctlr & ICC_CTLR_EL1_EOImode_MASK; > + val |= (vmcr.cbpr << ICC_CTLR_EL1_CBPR_SHIFT) & ICC_CTLR_EL1_CBPR_MASK; > + val |= (vmcr.eoim << ICC_CTLR_EL1_EOImode_SHIFT) & ICC_CTLR_EL1_EOImode_MASK; > > p->regval = val; > } > @@ -135,7 +135,7 @@ static bool access_gic_bpr1(struct kvm_vcpu *vcpu, struct sys_reg_params *p, > p->regval = 0; > > vgic_get_vmcr(vcpu, &vmcr); > - if (!((vmcr.ctlr & ICH_VMCR_CBPR_MASK) >> ICH_VMCR_CBPR_SHIFT)) { > + if (!vmcr.cbpr) { > if (p->is_write) { > vmcr.abpr = (p->regval & ICC_BPR1_EL1_MASK) >> > ICC_BPR1_EL1_SHIFT; > diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h > index fffb912..1fa293a 100644 > --- a/include/linux/irqchip/arm-gic-v3.h > +++ b/include/linux/irqchip/arm-gic-v3.h > @@ -417,6 +417,10 @@ > #define ICH_HCR_EN (1 << 0) > #define ICH_HCR_UIE (1 << 1) > > +#define ICH_VMCR_ACK_CTL_SHIFT 2 > +#define ICH_VMCR_ACK_CTL_MASK (1 << ICH_VMCR_ACK_CTL_SHIFT) > +#define ICH_VMCR_FIQ_EN_SHIFT 3 > +#define ICH_VMCR_FIQ_EN_MASK (1 << ICH_VMCR_FIQ_EN_SHIFT) > #define ICH_VMCR_CBPR_SHIFT 4 > #define ICH_VMCR_CBPR_MASK (1 << ICH_VMCR_CBPR_SHIFT) > #define ICH_VMCR_EOIM_SHIFT 9 > diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h > index dc30f3d..0baebcb 100644 > --- a/include/linux/irqchip/arm-gic.h > +++ b/include/linux/irqchip/arm-gic.h > @@ -25,6 +25,11 @@ > #define GICC_ENABLE 0x1 > #define GICC_INT_PRI_THRESHOLD 0xf0 > > +#define GIC_CPU_CTRL_EnableGrp0 (1 << 0) > +#define GIC_CPU_CTRL_EnableGrp1 (1 << 1) > +#define GIC_CPU_CTRL_AckCtl (1 << 2) > +#define GIC_CPU_CTRL_FIQEn (1 << 3) > +#define GIC_CPU_CTRL_CBPR (1 << 4) > #define GIC_CPU_CTRL_EOImodeNS (1 << 9) > > #define GICC_IAR_INT_ID_MASK 0x3ff > @@ -84,8 +89,19 @@ > #define GICH_LR_EOI (1 << 19) > #define GICH_LR_HW (1 << 31) > > -#define GICH_VMCR_CTRL_SHIFT 0 > -#define GICH_VMCR_CTRL_MASK (0x21f << GICH_VMCR_CTRL_SHIFT) > +#define GICH_VMCR_ENABLE_GRP0_SHIFT 0 > +#define GICH_VMCR_ENABLE_GRP0_MASK (1 << GICH_VMCR_ENABLE_GRP0_SHIFT) > +#define GICH_VMCR_ENABLE_GRP1_SHIFT 1 > +#define GICH_VMCR_ENABLE_GRP1_MASK (1 << GICH_VMCR_ENABLE_GRP1_SHIFT) > +#define GICH_VMCR_ACK_CTL_SHIFT 2 > +#define GICH_VMCR_ACK_CTL_MASK (1 << GICH_VMCR_ACK_CTL_SHIFT) > +#define GICH_VMCR_FIQ_EN_SHIFT 3 > +#define GICH_VMCR_FIQ_EN_MASK (1 << GICH_VMCR_FIQ_EN_SHIFT) > +#define GICH_VMCR_CBPR_SHIFT 4 > +#define GICH_VMCR_CBPR_MASK (1 << GICH_VMCR_CBPR_SHIFT) > +#define GICH_VMCR_EOI_MODE_SHIFT 9 > +#define GICH_VMCR_EOI_MODE_MASK (1 << GICH_VMCR_EOI_MODE_SHIFT) > + > #define GICH_VMCR_PRIMASK_SHIFT 27 > #define GICH_VMCR_PRIMASK_MASK (0x1f << GICH_VMCR_PRIMASK_SHIFT) > #define GICH_VMCR_BINPOINT_SHIFT 21 > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c > index 0a4283e..0b71a46 100644 > --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c > +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c > @@ -226,7 +226,13 @@ static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu, > > switch (addr & 0xff) { > case GIC_CPU_CTRL: > - val = vmcr.ctlr; > + val = vmcr.grpen0 << __ffs(GIC_CPU_CTRL_EnableGrp0); > + val |= vmcr.grpen1 << __ffs(GIC_CPU_CTRL_EnableGrp1); > + val |= vmcr.ackctl << __ffs(GIC_CPU_CTRL_AckCtl); > + val |= vmcr.fiqen << __ffs(GIC_CPU_CTRL_FIQEn); > + val |= vmcr.cbpr << __ffs(GIC_CPU_CTRL_CBPR); > + val |= vmcr.eoim << __ffs(GIC_CPU_CTRL_EOImodeNS); Funky ;-) Should we try to standardize on one way or another of expressing the shift? > + > break; > case GIC_CPU_PRIMASK: > /* > @@ -267,7 +273,13 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu, > > switch (addr & 0xff) { > case GIC_CPU_CTRL: > - vmcr.ctlr = val; > + vmcr.grpen0 = !!(val & GIC_CPU_CTRL_EnableGrp0); > + vmcr.grpen1 = !!(val & GIC_CPU_CTRL_EnableGrp1); > + vmcr.ackctl = !!(val & GIC_CPU_CTRL_AckCtl); > + vmcr.fiqen = !!(val & GIC_CPU_CTRL_FIQEn); > + vmcr.cbpr = !!(val & GIC_CPU_CTRL_CBPR); > + vmcr.eoim = !!(val & GIC_CPU_CTRL_EOImodeNS); > + > break; > case GIC_CPU_PRIMASK: > /* > diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c > index 504b4bd..e4187e5 100644 > --- a/virt/kvm/arm/vgic/vgic-v2.c > +++ b/virt/kvm/arm/vgic/vgic-v2.c > @@ -177,7 +177,18 @@ void vgic_v2_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) > struct vgic_v2_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v2; > u32 vmcr; > > - vmcr = (vmcrp->ctlr << GICH_VMCR_CTRL_SHIFT) & GICH_VMCR_CTRL_MASK; > + vmcr = (vmcrp->grpen0 << GICH_VMCR_ENABLE_GRP0_SHIFT) & > + GICH_VMCR_ENABLE_GRP0_MASK; > + vmcr |= (vmcrp->grpen1 << GICH_VMCR_ENABLE_GRP1_SHIFT) & > + GICH_VMCR_ENABLE_GRP1_MASK; > + vmcr |= (vmcrp->ackctl << GICH_VMCR_ACK_CTL_SHIFT) & > + GICH_VMCR_ACK_CTL_MASK; > + vmcr |= (vmcrp->fiqen << GICH_VMCR_FIQ_EN_SHIFT) & > + GICH_VMCR_FIQ_EN_MASK; > + vmcr |= (vmcrp->cbpr << GICH_VMCR_CBPR_SHIFT) & > + GICH_VMCR_CBPR_MASK; > + vmcr |= (vmcrp->eoim << GICH_VMCR_EOI_MODE_SHIFT) & > + GICH_VMCR_EOI_MODE_MASK; > vmcr |= (vmcrp->abpr << GICH_VMCR_ALIAS_BINPOINT_SHIFT) & > GICH_VMCR_ALIAS_BINPOINT_MASK; > vmcr |= (vmcrp->bpr << GICH_VMCR_BINPOINT_SHIFT) & > @@ -195,8 +206,19 @@ void vgic_v2_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) > > vmcr = cpu_if->vgic_vmcr; > > - vmcrp->ctlr = (vmcr & GICH_VMCR_CTRL_MASK) >> > - GICH_VMCR_CTRL_SHIFT; > + vmcrp->grpen0 = (vmcr & GICH_VMCR_ENABLE_GRP0_MASK) >> > + GICH_VMCR_ENABLE_GRP0_SHIFT; > + vmcrp->grpen1 = (vmcr & GICH_VMCR_ENABLE_GRP1_MASK) >> > + GICH_VMCR_ENABLE_GRP1_SHIFT; > + vmcrp->ackctl = (vmcr & GICH_VMCR_ACK_CTL_MASK) >> > + GICH_VMCR_ACK_CTL_SHIFT; > + vmcrp->fiqen = (vmcr & GICH_VMCR_FIQ_EN_MASK) >> > + GICH_VMCR_FIQ_EN_SHIFT; > + vmcrp->cbpr = (vmcr & GICH_VMCR_CBPR_MASK) >> > + GICH_VMCR_CBPR_SHIFT; > + vmcrp->eoim = (vmcr & GICH_VMCR_EOI_MODE_MASK) >> > + GICH_VMCR_EOI_MODE_SHIFT; > + > vmcrp->abpr = (vmcr & GICH_VMCR_ALIAS_BINPOINT_MASK) >> > GICH_VMCR_ALIAS_BINPOINT_SHIFT; > vmcrp->bpr = (vmcr & GICH_VMCR_BINPOINT_MASK) >> > diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c > index 6fe3f00..051cbde 100644 > --- a/virt/kvm/arm/vgic/vgic-v3.c > +++ b/virt/kvm/arm/vgic/vgic-v3.c > @@ -159,15 +159,24 @@ void vgic_v3_clear_lr(struct kvm_vcpu *vcpu, int lr) > void vgic_v3_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) > { > struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3; > - u32 vmcr; > + u32 model = vcpu->kvm->arch.vgic.vgic_model; > + u32 vmcr = 0; nit: drop the initialization... > > - /* > - * Ignore the FIQen bit, because GIC emulation always implies > - * SRE=1 which means the vFIQEn bit is also RES1. > - */ > - vmcr = ((vmcrp->ctlr >> ICC_CTLR_EL1_EOImode_SHIFT) << > - ICH_VMCR_EOIM_SHIFT) & ICH_VMCR_EOIM_MASK; > - vmcr |= (vmcrp->ctlr << ICH_VMCR_CBPR_SHIFT) & ICH_VMCR_CBPR_MASK; > + if (model == KVM_DEV_TYPE_ARM_VGIC_V2) { > + vmcr |= (vmcrp->ackctl << ICH_VMCR_ACK_CTL_SHIFT) & > + ICH_VMCR_ACK_CTL_MASK; ... and make this a strict assignment instead of an or? > + vmcr |= (vmcrp->fiqen << ICH_VMCR_FIQ_EN_SHIFT) & > + ICH_VMCR_FIQ_EN_MASK; > + } else { > + /* > + * When emulating GICv3 on GICv3 with SRE=1 on the > + * VFIQEn bit is RES1 and the VAckCtl bit is RES0. > + */ > + vmcr = ICH_VMCR_FIQ_EN_MASK; > + } > + > + vmcr |= (vmcrp->cbpr << ICH_VMCR_CBPR_SHIFT) & ICH_VMCR_CBPR_MASK; > + vmcr |= (vmcrp->eoim << ICH_VMCR_EOIM_SHIFT) & ICH_VMCR_EOIM_MASK; > vmcr |= (vmcrp->abpr << ICH_VMCR_BPR1_SHIFT) & ICH_VMCR_BPR1_MASK; > vmcr |= (vmcrp->bpr << ICH_VMCR_BPR0_SHIFT) & ICH_VMCR_BPR0_MASK; > vmcr |= (vmcrp->pmr << ICH_VMCR_PMR_SHIFT) & ICH_VMCR_PMR_MASK; > @@ -180,17 +189,27 @@ void vgic_v3_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) > void vgic_v3_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) > { > struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3; > + u32 model = vcpu->kvm->arch.vgic.vgic_model; > u32 vmcr; > > vmcr = cpu_if->vgic_vmcr; > > - /* > - * Ignore the FIQen bit, because GIC emulation always implies > - * SRE=1 which means the vFIQEn bit is also RES1. > - */ > - vmcrp->ctlr = ((vmcr >> ICH_VMCR_EOIM_SHIFT) << > - ICC_CTLR_EL1_EOImode_SHIFT) & ICC_CTLR_EL1_EOImode_MASK; > - vmcrp->ctlr |= (vmcr & ICH_VMCR_CBPR_MASK) >> ICH_VMCR_CBPR_SHIFT; > + if (model == KVM_DEV_TYPE_ARM_VGIC_V2) { > + vmcrp->ackctl = (vmcr & ICH_VMCR_ACK_CTL_MASK) >> > + ICH_VMCR_ACK_CTL_SHIFT; > + vmcrp->fiqen = (vmcr & ICH_VMCR_FIQ_EN_MASK) >> > + ICH_VMCR_FIQ_EN_SHIFT; > + } else { > + /* > + * When emulating GICv3 on GICv3 with SRE=1 on the > + * VFIQEn bit is RES1 and the VAckCtl bit is RES0. > + */ > + vmcrp->fiqen = 1; > + vmcrp->ackctl = 0; > + } > + > + vmcrp->cbpr = (vmcr & ICH_VMCR_CBPR_MASK) >> ICH_VMCR_CBPR_SHIFT; > + vmcrp->eoim = (vmcr & ICH_VMCR_EOIM_MASK) >> ICH_VMCR_EOIM_SHIFT; > vmcrp->abpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT; > vmcrp->bpr = (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT; > vmcrp->pmr = (vmcr & ICH_VMCR_PMR_MASK) >> ICH_VMCR_PMR_SHIFT; > diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h > index da83e4c..bba7fa2 100644 > --- a/virt/kvm/arm/vgic/vgic.h > +++ b/virt/kvm/arm/vgic/vgic.h > @@ -111,14 +111,18 @@ static inline bool irq_is_pending(struct vgic_irq *irq) > * registers regardless of the hardware backed GIC used. > */ > struct vgic_vmcr { > - u32 ctlr; > + u32 grpen0; > + u32 grpen1; > + > + u32 ackctl; > + u32 fiqen; > + u32 cbpr; > + u32 eoim; > + > u32 abpr; > u32 bpr; > u32 pmr; /* Priority mask field in the GICC_PMR and > * ICC_PMR_EL1 priority field format */ > - /* Below member variable are valid only for GICv3 */ > - u32 grpen0; > - u32 grpen1; > }; > > struct vgic_reg_attr { Other than the couple of nits above, this looks pretty good. I've given it a go on my LS2085 with both v2 and v3 guests, save/restoring in a loop, and it worked nicely enough. Worth backporting to 4.11-stable. Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> Tested-by: Marc Zyngier <marc.zyngier@arm.com> M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] kvm: arm: vgic-v3: add the emulate GICC_CTLR layout support for vmcr ctlr field. 2017-05-23 16:33 ` Marc Zyngier @ 2017-05-23 16:55 ` Christoffer Dall 2017-05-23 18:13 ` Marc Zyngier 0 siblings, 1 reply; 6+ messages in thread From: Christoffer Dall @ 2017-05-23 16:55 UTC (permalink / raw) To: Marc Zyngier; +Cc: kvmarm, wu.wubin On Tue, May 23, 2017 at 05:33:46PM +0100, Marc Zyngier wrote: > On 20/05/17 13:17, Christoffer Dall wrote: > > Hi Wanghaibin, > > > > On Tue, May 16, 2017 at 09:19:26AM +0800, wanghaibin wrote: > >> Boot a virtual machine with the emulated GICv2 on the GICv3 hardware. > >> Migrate the virtual machine will be successful, but the virtual machine will > >> hang at the destination. > >> > >> The GICC_CTLR and ICC_CTLR_EL1 have the different layout. Currently, the set/get > >> the VMCR interface just take vmcr ctlr field as the ICC_CTLR_EL1 layout. > >> Should we consider the GICC_CTLR layout to avoid this problem? > >> > >> Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com> > > > > Nice find. > > > > I agree with the problem, but I don't agree with your solution, because > > it's really hard to figure out the mappings, I think. > > > > How about this (untested) patch instead? Could you test the migration > > on your setup? > > > > commit 7c94545b6801840364647d5deb81f47da5e1f547 (HEAD -> vmcr-gicv2on3-cleanup, kernelorg/vmcr-gicv2on3-cleanup) > > Author: Christoffer Dall <cdall@linaro.org> > > Date: Sat May 20 14:12:34 2017 +0200 > > > > KVM: arm/arm64: Fix isues with GICv2 on GICv3 migration > > > > We have been a little loose with our intermediate VMCR representation > > where we had a 'ctlr' field, but we failed to differentiate between the > > GICv2 GICC_CTLR and ICC_CTLR_EL1 layouts, and therefore ended up mapping > > the wrong bits into the individual fields of the ICH_VMCR_EL2 when > > emulating a GICv2 on a GICv3 system. > > > > Fix this by using explicit fields for the VMCR bits instead. > > > > Cc: Marc Zyngier <marc.zyngier@arm.com> > > Cc: Eric Auger <eric.auger@redhat.com> > > Reported-by: wanghaibin <wanghaibin.wang@huawei.com> > > Signed-off-by: Christoffer Dall <cdall@linaro.org> > > > > diff --git a/arch/arm64/kvm/vgic-sys-reg-v3.c b/arch/arm64/kvm/vgic-sys-reg-v3.c > > index 79f37e3..6260b69 100644 > > --- a/arch/arm64/kvm/vgic-sys-reg-v3.c > > +++ b/arch/arm64/kvm/vgic-sys-reg-v3.c > > @@ -65,8 +65,8 @@ static bool access_gic_ctlr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, > > * Here set VMCR.CTLR in ICC_CTLR_EL1 layout. > > * The vgic_set_vmcr() will convert to ICH_VMCR layout. > > */ > > - vmcr.ctlr = val & ICC_CTLR_EL1_CBPR_MASK; > > - vmcr.ctlr |= val & ICC_CTLR_EL1_EOImode_MASK; > > + vmcr.cbpr = (val & ICC_CTLR_EL1_CBPR_MASK) >> ICC_CTLR_EL1_CBPR_SHIFT; > > + vmcr.eoim = (val & ICC_CTLR_EL1_EOImode_MASK) >> ICC_CTLR_EL1_EOImode_SHIFT; > > vgic_set_vmcr(vcpu, &vmcr); > > } else { > > val = 0; > > @@ -83,8 +83,8 @@ static bool access_gic_ctlr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, > > * The VMCR.CTLR value is in ICC_CTLR_EL1 layout. > > * Extract it directly using ICC_CTLR_EL1 reg definitions. > > */ > > - val |= vmcr.ctlr & ICC_CTLR_EL1_CBPR_MASK; > > - val |= vmcr.ctlr & ICC_CTLR_EL1_EOImode_MASK; > > + val |= (vmcr.cbpr << ICC_CTLR_EL1_CBPR_SHIFT) & ICC_CTLR_EL1_CBPR_MASK; > > + val |= (vmcr.eoim << ICC_CTLR_EL1_EOImode_SHIFT) & ICC_CTLR_EL1_EOImode_MASK; > > > > p->regval = val; > > } > > @@ -135,7 +135,7 @@ static bool access_gic_bpr1(struct kvm_vcpu *vcpu, struct sys_reg_params *p, > > p->regval = 0; > > > > vgic_get_vmcr(vcpu, &vmcr); > > - if (!((vmcr.ctlr & ICH_VMCR_CBPR_MASK) >> ICH_VMCR_CBPR_SHIFT)) { > > + if (!vmcr.cbpr) { > > if (p->is_write) { > > vmcr.abpr = (p->regval & ICC_BPR1_EL1_MASK) >> > > ICC_BPR1_EL1_SHIFT; > > diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h > > index fffb912..1fa293a 100644 > > --- a/include/linux/irqchip/arm-gic-v3.h > > +++ b/include/linux/irqchip/arm-gic-v3.h > > @@ -417,6 +417,10 @@ > > #define ICH_HCR_EN (1 << 0) > > #define ICH_HCR_UIE (1 << 1) > > > > +#define ICH_VMCR_ACK_CTL_SHIFT 2 > > +#define ICH_VMCR_ACK_CTL_MASK (1 << ICH_VMCR_ACK_CTL_SHIFT) > > +#define ICH_VMCR_FIQ_EN_SHIFT 3 > > +#define ICH_VMCR_FIQ_EN_MASK (1 << ICH_VMCR_FIQ_EN_SHIFT) > > #define ICH_VMCR_CBPR_SHIFT 4 > > #define ICH_VMCR_CBPR_MASK (1 << ICH_VMCR_CBPR_SHIFT) > > #define ICH_VMCR_EOIM_SHIFT 9 > > diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h > > index dc30f3d..0baebcb 100644 > > --- a/include/linux/irqchip/arm-gic.h > > +++ b/include/linux/irqchip/arm-gic.h > > @@ -25,6 +25,11 @@ > > #define GICC_ENABLE 0x1 > > #define GICC_INT_PRI_THRESHOLD 0xf0 > > > > +#define GIC_CPU_CTRL_EnableGrp0 (1 << 0) > > +#define GIC_CPU_CTRL_EnableGrp1 (1 << 1) > > +#define GIC_CPU_CTRL_AckCtl (1 << 2) > > +#define GIC_CPU_CTRL_FIQEn (1 << 3) > > +#define GIC_CPU_CTRL_CBPR (1 << 4) > > #define GIC_CPU_CTRL_EOImodeNS (1 << 9) > > > > #define GICC_IAR_INT_ID_MASK 0x3ff > > @@ -84,8 +89,19 @@ > > #define GICH_LR_EOI (1 << 19) > > #define GICH_LR_HW (1 << 31) > > > > -#define GICH_VMCR_CTRL_SHIFT 0 > > -#define GICH_VMCR_CTRL_MASK (0x21f << GICH_VMCR_CTRL_SHIFT) > > +#define GICH_VMCR_ENABLE_GRP0_SHIFT 0 > > +#define GICH_VMCR_ENABLE_GRP0_MASK (1 << GICH_VMCR_ENABLE_GRP0_SHIFT) > > +#define GICH_VMCR_ENABLE_GRP1_SHIFT 1 > > +#define GICH_VMCR_ENABLE_GRP1_MASK (1 << GICH_VMCR_ENABLE_GRP1_SHIFT) > > +#define GICH_VMCR_ACK_CTL_SHIFT 2 > > +#define GICH_VMCR_ACK_CTL_MASK (1 << GICH_VMCR_ACK_CTL_SHIFT) > > +#define GICH_VMCR_FIQ_EN_SHIFT 3 > > +#define GICH_VMCR_FIQ_EN_MASK (1 << GICH_VMCR_FIQ_EN_SHIFT) > > +#define GICH_VMCR_CBPR_SHIFT 4 > > +#define GICH_VMCR_CBPR_MASK (1 << GICH_VMCR_CBPR_SHIFT) > > +#define GICH_VMCR_EOI_MODE_SHIFT 9 > > +#define GICH_VMCR_EOI_MODE_MASK (1 << GICH_VMCR_EOI_MODE_SHIFT) > > + > > #define GICH_VMCR_PRIMASK_SHIFT 27 > > #define GICH_VMCR_PRIMASK_MASK (0x1f << GICH_VMCR_PRIMASK_SHIFT) > > #define GICH_VMCR_BINPOINT_SHIFT 21 > > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c > > index 0a4283e..0b71a46 100644 > > --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c > > +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c > > @@ -226,7 +226,13 @@ static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu, > > > > switch (addr & 0xff) { > > case GIC_CPU_CTRL: > > - val = vmcr.ctlr; > > + val = vmcr.grpen0 << __ffs(GIC_CPU_CTRL_EnableGrp0); > > + val |= vmcr.grpen1 << __ffs(GIC_CPU_CTRL_EnableGrp1); > > + val |= vmcr.ackctl << __ffs(GIC_CPU_CTRL_AckCtl); > > + val |= vmcr.fiqen << __ffs(GIC_CPU_CTRL_FIQEn); > > + val |= vmcr.cbpr << __ffs(GIC_CPU_CTRL_CBPR); > > + val |= vmcr.eoim << __ffs(GIC_CPU_CTRL_EOImodeNS); > > Funky ;-) Should we try to standardize on one way or another of > expressing the shift? > I wouldn't mind that, but because the existing v2 definition for EOImodeNS was defined differently from the existing v3 definitions, and this is fixing a real problem, I decdied to keep the consistency in the definitions and work around it in the code that uses it. But as always, I'm open to suggestions. Do you think we should change the way the GICv2 define is done? > > + > > break; > > case GIC_CPU_PRIMASK: > > /* > > @@ -267,7 +273,13 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu, > > > > switch (addr & 0xff) { > > case GIC_CPU_CTRL: > > - vmcr.ctlr = val; > > + vmcr.grpen0 = !!(val & GIC_CPU_CTRL_EnableGrp0); > > + vmcr.grpen1 = !!(val & GIC_CPU_CTRL_EnableGrp1); > > + vmcr.ackctl = !!(val & GIC_CPU_CTRL_AckCtl); > > + vmcr.fiqen = !!(val & GIC_CPU_CTRL_FIQEn); > > + vmcr.cbpr = !!(val & GIC_CPU_CTRL_CBPR); > > + vmcr.eoim = !!(val & GIC_CPU_CTRL_EOImodeNS); > > + > > break; > > case GIC_CPU_PRIMASK: > > /* > > diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c > > index 504b4bd..e4187e5 100644 > > --- a/virt/kvm/arm/vgic/vgic-v2.c > > +++ b/virt/kvm/arm/vgic/vgic-v2.c > > @@ -177,7 +177,18 @@ void vgic_v2_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) > > struct vgic_v2_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v2; > > u32 vmcr; > > > > - vmcr = (vmcrp->ctlr << GICH_VMCR_CTRL_SHIFT) & GICH_VMCR_CTRL_MASK; > > + vmcr = (vmcrp->grpen0 << GICH_VMCR_ENABLE_GRP0_SHIFT) & > > + GICH_VMCR_ENABLE_GRP0_MASK; > > + vmcr |= (vmcrp->grpen1 << GICH_VMCR_ENABLE_GRP1_SHIFT) & > > + GICH_VMCR_ENABLE_GRP1_MASK; > > + vmcr |= (vmcrp->ackctl << GICH_VMCR_ACK_CTL_SHIFT) & > > + GICH_VMCR_ACK_CTL_MASK; > > + vmcr |= (vmcrp->fiqen << GICH_VMCR_FIQ_EN_SHIFT) & > > + GICH_VMCR_FIQ_EN_MASK; > > + vmcr |= (vmcrp->cbpr << GICH_VMCR_CBPR_SHIFT) & > > + GICH_VMCR_CBPR_MASK; > > + vmcr |= (vmcrp->eoim << GICH_VMCR_EOI_MODE_SHIFT) & > > + GICH_VMCR_EOI_MODE_MASK; > > vmcr |= (vmcrp->abpr << GICH_VMCR_ALIAS_BINPOINT_SHIFT) & > > GICH_VMCR_ALIAS_BINPOINT_MASK; > > vmcr |= (vmcrp->bpr << GICH_VMCR_BINPOINT_SHIFT) & > > @@ -195,8 +206,19 @@ void vgic_v2_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) > > > > vmcr = cpu_if->vgic_vmcr; > > > > - vmcrp->ctlr = (vmcr & GICH_VMCR_CTRL_MASK) >> > > - GICH_VMCR_CTRL_SHIFT; > > + vmcrp->grpen0 = (vmcr & GICH_VMCR_ENABLE_GRP0_MASK) >> > > + GICH_VMCR_ENABLE_GRP0_SHIFT; > > + vmcrp->grpen1 = (vmcr & GICH_VMCR_ENABLE_GRP1_MASK) >> > > + GICH_VMCR_ENABLE_GRP1_SHIFT; > > + vmcrp->ackctl = (vmcr & GICH_VMCR_ACK_CTL_MASK) >> > > + GICH_VMCR_ACK_CTL_SHIFT; > > + vmcrp->fiqen = (vmcr & GICH_VMCR_FIQ_EN_MASK) >> > > + GICH_VMCR_FIQ_EN_SHIFT; > > + vmcrp->cbpr = (vmcr & GICH_VMCR_CBPR_MASK) >> > > + GICH_VMCR_CBPR_SHIFT; > > + vmcrp->eoim = (vmcr & GICH_VMCR_EOI_MODE_MASK) >> > > + GICH_VMCR_EOI_MODE_SHIFT; > > + > > vmcrp->abpr = (vmcr & GICH_VMCR_ALIAS_BINPOINT_MASK) >> > > GICH_VMCR_ALIAS_BINPOINT_SHIFT; > > vmcrp->bpr = (vmcr & GICH_VMCR_BINPOINT_MASK) >> > > diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c > > index 6fe3f00..051cbde 100644 > > --- a/virt/kvm/arm/vgic/vgic-v3.c > > +++ b/virt/kvm/arm/vgic/vgic-v3.c > > @@ -159,15 +159,24 @@ void vgic_v3_clear_lr(struct kvm_vcpu *vcpu, int lr) > > void vgic_v3_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) > > { > > struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3; > > - u32 vmcr; > > + u32 model = vcpu->kvm->arch.vgic.vgic_model; > > + u32 vmcr = 0; > > nit: drop the initialization... > right > > > > - /* > > - * Ignore the FIQen bit, because GIC emulation always implies > > - * SRE=1 which means the vFIQEn bit is also RES1. > > - */ > > - vmcr = ((vmcrp->ctlr >> ICC_CTLR_EL1_EOImode_SHIFT) << > > - ICH_VMCR_EOIM_SHIFT) & ICH_VMCR_EOIM_MASK; > > - vmcr |= (vmcrp->ctlr << ICH_VMCR_CBPR_SHIFT) & ICH_VMCR_CBPR_MASK; > > + if (model == KVM_DEV_TYPE_ARM_VGIC_V2) { > > + vmcr |= (vmcrp->ackctl << ICH_VMCR_ACK_CTL_SHIFT) & > > + ICH_VMCR_ACK_CTL_MASK; > > ... and make this a strict assignment instead of an or? > yes > > + vmcr |= (vmcrp->fiqen << ICH_VMCR_FIQ_EN_SHIFT) & > > + ICH_VMCR_FIQ_EN_MASK; > > + } else { > > + /* > > + * When emulating GICv3 on GICv3 with SRE=1 on the > > + * VFIQEn bit is RES1 and the VAckCtl bit is RES0. > > + */ > > + vmcr = ICH_VMCR_FIQ_EN_MASK; > > + } > > + > > + vmcr |= (vmcrp->cbpr << ICH_VMCR_CBPR_SHIFT) & ICH_VMCR_CBPR_MASK; > > + vmcr |= (vmcrp->eoim << ICH_VMCR_EOIM_SHIFT) & ICH_VMCR_EOIM_MASK; > > vmcr |= (vmcrp->abpr << ICH_VMCR_BPR1_SHIFT) & ICH_VMCR_BPR1_MASK; > > vmcr |= (vmcrp->bpr << ICH_VMCR_BPR0_SHIFT) & ICH_VMCR_BPR0_MASK; > > vmcr |= (vmcrp->pmr << ICH_VMCR_PMR_SHIFT) & ICH_VMCR_PMR_MASK; > > @@ -180,17 +189,27 @@ void vgic_v3_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) > > void vgic_v3_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) > > { > > struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3; > > + u32 model = vcpu->kvm->arch.vgic.vgic_model; > > u32 vmcr; > > > > vmcr = cpu_if->vgic_vmcr; > > > > - /* > > - * Ignore the FIQen bit, because GIC emulation always implies > > - * SRE=1 which means the vFIQEn bit is also RES1. > > - */ > > - vmcrp->ctlr = ((vmcr >> ICH_VMCR_EOIM_SHIFT) << > > - ICC_CTLR_EL1_EOImode_SHIFT) & ICC_CTLR_EL1_EOImode_MASK; > > - vmcrp->ctlr |= (vmcr & ICH_VMCR_CBPR_MASK) >> ICH_VMCR_CBPR_SHIFT; > > + if (model == KVM_DEV_TYPE_ARM_VGIC_V2) { > > + vmcrp->ackctl = (vmcr & ICH_VMCR_ACK_CTL_MASK) >> > > + ICH_VMCR_ACK_CTL_SHIFT; > > + vmcrp->fiqen = (vmcr & ICH_VMCR_FIQ_EN_MASK) >> > > + ICH_VMCR_FIQ_EN_SHIFT; > > + } else { > > + /* > > + * When emulating GICv3 on GICv3 with SRE=1 on the > > + * VFIQEn bit is RES1 and the VAckCtl bit is RES0. > > + */ > > + vmcrp->fiqen = 1; > > + vmcrp->ackctl = 0; > > + } > > + > > + vmcrp->cbpr = (vmcr & ICH_VMCR_CBPR_MASK) >> ICH_VMCR_CBPR_SHIFT; > > + vmcrp->eoim = (vmcr & ICH_VMCR_EOIM_MASK) >> ICH_VMCR_EOIM_SHIFT; > > vmcrp->abpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT; > > vmcrp->bpr = (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT; > > vmcrp->pmr = (vmcr & ICH_VMCR_PMR_MASK) >> ICH_VMCR_PMR_SHIFT; > > diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h > > index da83e4c..bba7fa2 100644 > > --- a/virt/kvm/arm/vgic/vgic.h > > +++ b/virt/kvm/arm/vgic/vgic.h > > @@ -111,14 +111,18 @@ static inline bool irq_is_pending(struct vgic_irq *irq) > > * registers regardless of the hardware backed GIC used. > > */ > > struct vgic_vmcr { > > - u32 ctlr; > > + u32 grpen0; > > + u32 grpen1; > > + > > + u32 ackctl; > > + u32 fiqen; > > + u32 cbpr; > > + u32 eoim; > > + > > u32 abpr; > > u32 bpr; > > u32 pmr; /* Priority mask field in the GICC_PMR and > > * ICC_PMR_EL1 priority field format */ > > - /* Below member variable are valid only for GICv3 */ > > - u32 grpen0; > > - u32 grpen1; > > }; > > > > struct vgic_reg_attr { > > Other than the couple of nits above, this looks pretty good. I've given > it a go on my LS2085 with both v2 and v3 guests, save/restoring in a > loop, and it worked nicely enough. Worth backporting to 4.11-stable. > > Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> > Tested-by: Marc Zyngier <marc.zyngier@arm.com> > Thanks! Let me know if you'd like me to fix up the definitions. I did remember you talking about making all this (naming etc.) consistent some time, and I figured we may be able to fix these up then as well? -Christoffer ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] kvm: arm: vgic-v3: add the emulate GICC_CTLR layout support for vmcr ctlr field. 2017-05-23 16:55 ` Christoffer Dall @ 2017-05-23 18:13 ` Marc Zyngier 0 siblings, 0 replies; 6+ messages in thread From: Marc Zyngier @ 2017-05-23 18:13 UTC (permalink / raw) To: Christoffer Dall; +Cc: kvmarm, wu.wubin On 23/05/17 17:55, Christoffer Dall wrote: > On Tue, May 23, 2017 at 05:33:46PM +0100, Marc Zyngier wrote: >> On 20/05/17 13:17, Christoffer Dall wrote: >>> Hi Wanghaibin, >>> >>> On Tue, May 16, 2017 at 09:19:26AM +0800, wanghaibin wrote: >>>> Boot a virtual machine with the emulated GICv2 on the GICv3 hardware. >>>> Migrate the virtual machine will be successful, but the virtual machine will >>>> hang at the destination. >>>> >>>> The GICC_CTLR and ICC_CTLR_EL1 have the different layout. Currently, the set/get >>>> the VMCR interface just take vmcr ctlr field as the ICC_CTLR_EL1 layout. >>>> Should we consider the GICC_CTLR layout to avoid this problem? >>>> >>>> Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com> >>> >>> Nice find. >>> >>> I agree with the problem, but I don't agree with your solution, because >>> it's really hard to figure out the mappings, I think. >>> >>> How about this (untested) patch instead? Could you test the migration >>> on your setup? >>> >>> commit 7c94545b6801840364647d5deb81f47da5e1f547 (HEAD -> vmcr-gicv2on3-cleanup, kernelorg/vmcr-gicv2on3-cleanup) >>> Author: Christoffer Dall <cdall@linaro.org> >>> Date: Sat May 20 14:12:34 2017 +0200 >>> >>> KVM: arm/arm64: Fix isues with GICv2 on GICv3 migration >>> >>> We have been a little loose with our intermediate VMCR representation >>> where we had a 'ctlr' field, but we failed to differentiate between the >>> GICv2 GICC_CTLR and ICC_CTLR_EL1 layouts, and therefore ended up mapping >>> the wrong bits into the individual fields of the ICH_VMCR_EL2 when >>> emulating a GICv2 on a GICv3 system. >>> >>> Fix this by using explicit fields for the VMCR bits instead. >>> >>> Cc: Marc Zyngier <marc.zyngier@arm.com> >>> Cc: Eric Auger <eric.auger@redhat.com> >>> Reported-by: wanghaibin <wanghaibin.wang@huawei.com> >>> Signed-off-by: Christoffer Dall <cdall@linaro.org> >>> >>> diff --git a/arch/arm64/kvm/vgic-sys-reg-v3.c b/arch/arm64/kvm/vgic-sys-reg-v3.c >>> index 79f37e3..6260b69 100644 >>> --- a/arch/arm64/kvm/vgic-sys-reg-v3.c >>> +++ b/arch/arm64/kvm/vgic-sys-reg-v3.c >>> @@ -65,8 +65,8 @@ static bool access_gic_ctlr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, >>> * Here set VMCR.CTLR in ICC_CTLR_EL1 layout. >>> * The vgic_set_vmcr() will convert to ICH_VMCR layout. >>> */ >>> - vmcr.ctlr = val & ICC_CTLR_EL1_CBPR_MASK; >>> - vmcr.ctlr |= val & ICC_CTLR_EL1_EOImode_MASK; >>> + vmcr.cbpr = (val & ICC_CTLR_EL1_CBPR_MASK) >> ICC_CTLR_EL1_CBPR_SHIFT; >>> + vmcr.eoim = (val & ICC_CTLR_EL1_EOImode_MASK) >> ICC_CTLR_EL1_EOImode_SHIFT; >>> vgic_set_vmcr(vcpu, &vmcr); >>> } else { >>> val = 0; >>> @@ -83,8 +83,8 @@ static bool access_gic_ctlr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, >>> * The VMCR.CTLR value is in ICC_CTLR_EL1 layout. >>> * Extract it directly using ICC_CTLR_EL1 reg definitions. >>> */ >>> - val |= vmcr.ctlr & ICC_CTLR_EL1_CBPR_MASK; >>> - val |= vmcr.ctlr & ICC_CTLR_EL1_EOImode_MASK; >>> + val |= (vmcr.cbpr << ICC_CTLR_EL1_CBPR_SHIFT) & ICC_CTLR_EL1_CBPR_MASK; >>> + val |= (vmcr.eoim << ICC_CTLR_EL1_EOImode_SHIFT) & ICC_CTLR_EL1_EOImode_MASK; >>> >>> p->regval = val; >>> } >>> @@ -135,7 +135,7 @@ static bool access_gic_bpr1(struct kvm_vcpu *vcpu, struct sys_reg_params *p, >>> p->regval = 0; >>> >>> vgic_get_vmcr(vcpu, &vmcr); >>> - if (!((vmcr.ctlr & ICH_VMCR_CBPR_MASK) >> ICH_VMCR_CBPR_SHIFT)) { >>> + if (!vmcr.cbpr) { >>> if (p->is_write) { >>> vmcr.abpr = (p->regval & ICC_BPR1_EL1_MASK) >> >>> ICC_BPR1_EL1_SHIFT; >>> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h >>> index fffb912..1fa293a 100644 >>> --- a/include/linux/irqchip/arm-gic-v3.h >>> +++ b/include/linux/irqchip/arm-gic-v3.h >>> @@ -417,6 +417,10 @@ >>> #define ICH_HCR_EN (1 << 0) >>> #define ICH_HCR_UIE (1 << 1) >>> >>> +#define ICH_VMCR_ACK_CTL_SHIFT 2 >>> +#define ICH_VMCR_ACK_CTL_MASK (1 << ICH_VMCR_ACK_CTL_SHIFT) >>> +#define ICH_VMCR_FIQ_EN_SHIFT 3 >>> +#define ICH_VMCR_FIQ_EN_MASK (1 << ICH_VMCR_FIQ_EN_SHIFT) >>> #define ICH_VMCR_CBPR_SHIFT 4 >>> #define ICH_VMCR_CBPR_MASK (1 << ICH_VMCR_CBPR_SHIFT) >>> #define ICH_VMCR_EOIM_SHIFT 9 >>> diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h >>> index dc30f3d..0baebcb 100644 >>> --- a/include/linux/irqchip/arm-gic.h >>> +++ b/include/linux/irqchip/arm-gic.h >>> @@ -25,6 +25,11 @@ >>> #define GICC_ENABLE 0x1 >>> #define GICC_INT_PRI_THRESHOLD 0xf0 >>> >>> +#define GIC_CPU_CTRL_EnableGrp0 (1 << 0) >>> +#define GIC_CPU_CTRL_EnableGrp1 (1 << 1) >>> +#define GIC_CPU_CTRL_AckCtl (1 << 2) >>> +#define GIC_CPU_CTRL_FIQEn (1 << 3) >>> +#define GIC_CPU_CTRL_CBPR (1 << 4) >>> #define GIC_CPU_CTRL_EOImodeNS (1 << 9) >>> >>> #define GICC_IAR_INT_ID_MASK 0x3ff >>> @@ -84,8 +89,19 @@ >>> #define GICH_LR_EOI (1 << 19) >>> #define GICH_LR_HW (1 << 31) >>> >>> -#define GICH_VMCR_CTRL_SHIFT 0 >>> -#define GICH_VMCR_CTRL_MASK (0x21f << GICH_VMCR_CTRL_SHIFT) >>> +#define GICH_VMCR_ENABLE_GRP0_SHIFT 0 >>> +#define GICH_VMCR_ENABLE_GRP0_MASK (1 << GICH_VMCR_ENABLE_GRP0_SHIFT) >>> +#define GICH_VMCR_ENABLE_GRP1_SHIFT 1 >>> +#define GICH_VMCR_ENABLE_GRP1_MASK (1 << GICH_VMCR_ENABLE_GRP1_SHIFT) >>> +#define GICH_VMCR_ACK_CTL_SHIFT 2 >>> +#define GICH_VMCR_ACK_CTL_MASK (1 << GICH_VMCR_ACK_CTL_SHIFT) >>> +#define GICH_VMCR_FIQ_EN_SHIFT 3 >>> +#define GICH_VMCR_FIQ_EN_MASK (1 << GICH_VMCR_FIQ_EN_SHIFT) >>> +#define GICH_VMCR_CBPR_SHIFT 4 >>> +#define GICH_VMCR_CBPR_MASK (1 << GICH_VMCR_CBPR_SHIFT) >>> +#define GICH_VMCR_EOI_MODE_SHIFT 9 >>> +#define GICH_VMCR_EOI_MODE_MASK (1 << GICH_VMCR_EOI_MODE_SHIFT) >>> + >>> #define GICH_VMCR_PRIMASK_SHIFT 27 >>> #define GICH_VMCR_PRIMASK_MASK (0x1f << GICH_VMCR_PRIMASK_SHIFT) >>> #define GICH_VMCR_BINPOINT_SHIFT 21 >>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c >>> index 0a4283e..0b71a46 100644 >>> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c >>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c >>> @@ -226,7 +226,13 @@ static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu, >>> >>> switch (addr & 0xff) { >>> case GIC_CPU_CTRL: >>> - val = vmcr.ctlr; >>> + val = vmcr.grpen0 << __ffs(GIC_CPU_CTRL_EnableGrp0); >>> + val |= vmcr.grpen1 << __ffs(GIC_CPU_CTRL_EnableGrp1); >>> + val |= vmcr.ackctl << __ffs(GIC_CPU_CTRL_AckCtl); >>> + val |= vmcr.fiqen << __ffs(GIC_CPU_CTRL_FIQEn); >>> + val |= vmcr.cbpr << __ffs(GIC_CPU_CTRL_CBPR); >>> + val |= vmcr.eoim << __ffs(GIC_CPU_CTRL_EOImodeNS); >> >> Funky ;-) Should we try to standardize on one way or another of >> expressing the shift? >> > > I wouldn't mind that, but because the existing v2 definition for > EOImodeNS was defined differently from the existing v3 definitions, and > this is fixing a real problem, I decdied to keep the consistency in the > definitions and work around it in the code that uses it. > > But as always, I'm open to suggestions. Do you think we should change > the way the GICv2 define is done? I think we might as well fix it, since there is only one single user of EOImodeNS so far. > >>> + >>> break; >>> case GIC_CPU_PRIMASK: >>> /* >>> @@ -267,7 +273,13 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu, >>> >>> switch (addr & 0xff) { >>> case GIC_CPU_CTRL: >>> - vmcr.ctlr = val; >>> + vmcr.grpen0 = !!(val & GIC_CPU_CTRL_EnableGrp0); >>> + vmcr.grpen1 = !!(val & GIC_CPU_CTRL_EnableGrp1); >>> + vmcr.ackctl = !!(val & GIC_CPU_CTRL_AckCtl); >>> + vmcr.fiqen = !!(val & GIC_CPU_CTRL_FIQEn); >>> + vmcr.cbpr = !!(val & GIC_CPU_CTRL_CBPR); >>> + vmcr.eoim = !!(val & GIC_CPU_CTRL_EOImodeNS); >>> + >>> break; >>> case GIC_CPU_PRIMASK: >>> /* >>> diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c >>> index 504b4bd..e4187e5 100644 >>> --- a/virt/kvm/arm/vgic/vgic-v2.c >>> +++ b/virt/kvm/arm/vgic/vgic-v2.c >>> @@ -177,7 +177,18 @@ void vgic_v2_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) >>> struct vgic_v2_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v2; >>> u32 vmcr; >>> >>> - vmcr = (vmcrp->ctlr << GICH_VMCR_CTRL_SHIFT) & GICH_VMCR_CTRL_MASK; >>> + vmcr = (vmcrp->grpen0 << GICH_VMCR_ENABLE_GRP0_SHIFT) & >>> + GICH_VMCR_ENABLE_GRP0_MASK; >>> + vmcr |= (vmcrp->grpen1 << GICH_VMCR_ENABLE_GRP1_SHIFT) & >>> + GICH_VMCR_ENABLE_GRP1_MASK; >>> + vmcr |= (vmcrp->ackctl << GICH_VMCR_ACK_CTL_SHIFT) & >>> + GICH_VMCR_ACK_CTL_MASK; >>> + vmcr |= (vmcrp->fiqen << GICH_VMCR_FIQ_EN_SHIFT) & >>> + GICH_VMCR_FIQ_EN_MASK; >>> + vmcr |= (vmcrp->cbpr << GICH_VMCR_CBPR_SHIFT) & >>> + GICH_VMCR_CBPR_MASK; >>> + vmcr |= (vmcrp->eoim << GICH_VMCR_EOI_MODE_SHIFT) & >>> + GICH_VMCR_EOI_MODE_MASK; >>> vmcr |= (vmcrp->abpr << GICH_VMCR_ALIAS_BINPOINT_SHIFT) & >>> GICH_VMCR_ALIAS_BINPOINT_MASK; >>> vmcr |= (vmcrp->bpr << GICH_VMCR_BINPOINT_SHIFT) & >>> @@ -195,8 +206,19 @@ void vgic_v2_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) >>> >>> vmcr = cpu_if->vgic_vmcr; >>> >>> - vmcrp->ctlr = (vmcr & GICH_VMCR_CTRL_MASK) >> >>> - GICH_VMCR_CTRL_SHIFT; >>> + vmcrp->grpen0 = (vmcr & GICH_VMCR_ENABLE_GRP0_MASK) >> >>> + GICH_VMCR_ENABLE_GRP0_SHIFT; >>> + vmcrp->grpen1 = (vmcr & GICH_VMCR_ENABLE_GRP1_MASK) >> >>> + GICH_VMCR_ENABLE_GRP1_SHIFT; >>> + vmcrp->ackctl = (vmcr & GICH_VMCR_ACK_CTL_MASK) >> >>> + GICH_VMCR_ACK_CTL_SHIFT; >>> + vmcrp->fiqen = (vmcr & GICH_VMCR_FIQ_EN_MASK) >> >>> + GICH_VMCR_FIQ_EN_SHIFT; >>> + vmcrp->cbpr = (vmcr & GICH_VMCR_CBPR_MASK) >> >>> + GICH_VMCR_CBPR_SHIFT; >>> + vmcrp->eoim = (vmcr & GICH_VMCR_EOI_MODE_MASK) >> >>> + GICH_VMCR_EOI_MODE_SHIFT; >>> + >>> vmcrp->abpr = (vmcr & GICH_VMCR_ALIAS_BINPOINT_MASK) >> >>> GICH_VMCR_ALIAS_BINPOINT_SHIFT; >>> vmcrp->bpr = (vmcr & GICH_VMCR_BINPOINT_MASK) >> >>> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c >>> index 6fe3f00..051cbde 100644 >>> --- a/virt/kvm/arm/vgic/vgic-v3.c >>> +++ b/virt/kvm/arm/vgic/vgic-v3.c >>> @@ -159,15 +159,24 @@ void vgic_v3_clear_lr(struct kvm_vcpu *vcpu, int lr) >>> void vgic_v3_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) >>> { >>> struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3; >>> - u32 vmcr; >>> + u32 model = vcpu->kvm->arch.vgic.vgic_model; >>> + u32 vmcr = 0; >> >> nit: drop the initialization... >> > > right > >>> >>> - /* >>> - * Ignore the FIQen bit, because GIC emulation always implies >>> - * SRE=1 which means the vFIQEn bit is also RES1. >>> - */ >>> - vmcr = ((vmcrp->ctlr >> ICC_CTLR_EL1_EOImode_SHIFT) << >>> - ICH_VMCR_EOIM_SHIFT) & ICH_VMCR_EOIM_MASK; >>> - vmcr |= (vmcrp->ctlr << ICH_VMCR_CBPR_SHIFT) & ICH_VMCR_CBPR_MASK; >>> + if (model == KVM_DEV_TYPE_ARM_VGIC_V2) { >>> + vmcr |= (vmcrp->ackctl << ICH_VMCR_ACK_CTL_SHIFT) & >>> + ICH_VMCR_ACK_CTL_MASK; >> >> ... and make this a strict assignment instead of an or? >> > > yes > >>> + vmcr |= (vmcrp->fiqen << ICH_VMCR_FIQ_EN_SHIFT) & >>> + ICH_VMCR_FIQ_EN_MASK; >>> + } else { >>> + /* >>> + * When emulating GICv3 on GICv3 with SRE=1 on the >>> + * VFIQEn bit is RES1 and the VAckCtl bit is RES0. >>> + */ >>> + vmcr = ICH_VMCR_FIQ_EN_MASK; >>> + } >>> + >>> + vmcr |= (vmcrp->cbpr << ICH_VMCR_CBPR_SHIFT) & ICH_VMCR_CBPR_MASK; >>> + vmcr |= (vmcrp->eoim << ICH_VMCR_EOIM_SHIFT) & ICH_VMCR_EOIM_MASK; >>> vmcr |= (vmcrp->abpr << ICH_VMCR_BPR1_SHIFT) & ICH_VMCR_BPR1_MASK; >>> vmcr |= (vmcrp->bpr << ICH_VMCR_BPR0_SHIFT) & ICH_VMCR_BPR0_MASK; >>> vmcr |= (vmcrp->pmr << ICH_VMCR_PMR_SHIFT) & ICH_VMCR_PMR_MASK; >>> @@ -180,17 +189,27 @@ void vgic_v3_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) >>> void vgic_v3_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) >>> { >>> struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3; >>> + u32 model = vcpu->kvm->arch.vgic.vgic_model; >>> u32 vmcr; >>> >>> vmcr = cpu_if->vgic_vmcr; >>> >>> - /* >>> - * Ignore the FIQen bit, because GIC emulation always implies >>> - * SRE=1 which means the vFIQEn bit is also RES1. >>> - */ >>> - vmcrp->ctlr = ((vmcr >> ICH_VMCR_EOIM_SHIFT) << >>> - ICC_CTLR_EL1_EOImode_SHIFT) & ICC_CTLR_EL1_EOImode_MASK; >>> - vmcrp->ctlr |= (vmcr & ICH_VMCR_CBPR_MASK) >> ICH_VMCR_CBPR_SHIFT; >>> + if (model == KVM_DEV_TYPE_ARM_VGIC_V2) { >>> + vmcrp->ackctl = (vmcr & ICH_VMCR_ACK_CTL_MASK) >> >>> + ICH_VMCR_ACK_CTL_SHIFT; >>> + vmcrp->fiqen = (vmcr & ICH_VMCR_FIQ_EN_MASK) >> >>> + ICH_VMCR_FIQ_EN_SHIFT; >>> + } else { >>> + /* >>> + * When emulating GICv3 on GICv3 with SRE=1 on the >>> + * VFIQEn bit is RES1 and the VAckCtl bit is RES0. >>> + */ >>> + vmcrp->fiqen = 1; >>> + vmcrp->ackctl = 0; >>> + } >>> + >>> + vmcrp->cbpr = (vmcr & ICH_VMCR_CBPR_MASK) >> ICH_VMCR_CBPR_SHIFT; >>> + vmcrp->eoim = (vmcr & ICH_VMCR_EOIM_MASK) >> ICH_VMCR_EOIM_SHIFT; >>> vmcrp->abpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT; >>> vmcrp->bpr = (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT; >>> vmcrp->pmr = (vmcr & ICH_VMCR_PMR_MASK) >> ICH_VMCR_PMR_SHIFT; >>> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h >>> index da83e4c..bba7fa2 100644 >>> --- a/virt/kvm/arm/vgic/vgic.h >>> +++ b/virt/kvm/arm/vgic/vgic.h >>> @@ -111,14 +111,18 @@ static inline bool irq_is_pending(struct vgic_irq *irq) >>> * registers regardless of the hardware backed GIC used. >>> */ >>> struct vgic_vmcr { >>> - u32 ctlr; >>> + u32 grpen0; >>> + u32 grpen1; >>> + >>> + u32 ackctl; >>> + u32 fiqen; >>> + u32 cbpr; >>> + u32 eoim; >>> + >>> u32 abpr; >>> u32 bpr; >>> u32 pmr; /* Priority mask field in the GICC_PMR and >>> * ICC_PMR_EL1 priority field format */ >>> - /* Below member variable are valid only for GICv3 */ >>> - u32 grpen0; >>> - u32 grpen1; >>> }; >>> >>> struct vgic_reg_attr { >> >> Other than the couple of nits above, this looks pretty good. I've given >> it a go on my LS2085 with both v2 and v3 guests, save/restoring in a >> loop, and it worked nicely enough. Worth backporting to 4.11-stable. >> >> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> >> Tested-by: Marc Zyngier <marc.zyngier@arm.com> >> > > Thanks! > > Let me know if you'd like me to fix up the definitions. I did remember > you talking about making all this (naming etc.) consistent some time, > and I figured we may be able to fix these up then as well? Indeed, though we can do it independently of this patch. My idea is to introduce the GICv2 compliant naming, keeping the old naming during a series of patches that performs a tree-wide renaming, and then drop the old definitions. I don't think this should gate this patch though. Just fix EOImodeNS to have a proper _SHIFT companion. Something along the line of: diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h index 0baebcb3acb3..d3453ee072fc 100644 --- a/include/linux/irqchip/arm-gic.h +++ b/include/linux/irqchip/arm-gic.h @@ -25,12 +25,18 @@ #define GICC_ENABLE 0x1 #define GICC_INT_PRI_THRESHOLD 0xf0 -#define GIC_CPU_CTRL_EnableGrp0 (1 << 0) -#define GIC_CPU_CTRL_EnableGrp1 (1 << 1) -#define GIC_CPU_CTRL_AckCtl (1 << 2) -#define GIC_CPU_CTRL_FIQEn (1 << 3) -#define GIC_CPU_CTRL_CBPR (1 << 4) -#define GIC_CPU_CTRL_EOImodeNS (1 << 9) +#define GIC_CPU_CTRL_EnableGrp0_SHIFT 0 +#define GIC_CPU_CTRL_EnableGrp0 (1 << GIC_CPU_CTRL_EnableGrp0_SHIFT) +#define GIC_CPU_CTRL_EnableGrp1_SHIFT 1 +#define GIC_CPU_CTRL_EnableGrp1 (1 << GIC_CPU_CTRL_EnableGrp1_SHIFT) +#define GIC_CPU_CTRL_AckCtl_SHIFT 2 +#define GIC_CPU_CTRL_AckCtl (1 << GIC_CPU_CTRL_AckCtl_SHIFT) +#define GIC_CPU_CTRL_FIQEn_SHIFT 3 +#define GIC_CPU_CTRL_FIQEn (1 << GIC_CPU_CTRL_FIQEn_SHIFT) +#define GIC_CPU_CTRL_CBPR_SHIFT 4 +#define GIC_CPU_CTRL_CBPR (1 << GIC_CPU_CTRL_CBPR_SHIFT) +#define GIC_CPU_CTRL_EOImodeNS_SHIFT 9 +#define GIC_CPU_CTRL_EOImodeNS (1 << GIC_CPU_CTRL_EOImodeNS_SHIFT) #define GICC_IAR_INT_ID_MASK 0x3ff #define GICC_INT_SPURIOUS 1023 Would that work for you? Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-05-23 18:09 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-05-16 1:19 [RFC PATCH] kvm: arm: vgic-v3: add the emulate GICC_CTLR layout support for vmcr ctlr field wanghaibin 2017-05-19 9:16 ` wanghaibin 2017-05-20 12:17 ` Christoffer Dall 2017-05-23 16:33 ` Marc Zyngier 2017-05-23 16:55 ` Christoffer Dall 2017-05-23 18:13 ` Marc Zyngier
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.