On Thu, Nov 17, 2016 at 12:22 AM, Christoffer Dall
<christoffer.dall@xxxxxxxxxx> wrote:
> On Fri, Nov 04, 2016 at 04:43:31PM +0530, vijay.kilari@xxxxxxxxx wrote:
>> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
>>
>> ICC_VMCR_EL2 supports virtual access to ICC_IGRPEN1_EL1.Enable
>> and ICC_IGRPEN0_EL1.Enable fields. Add grpen0 and grpen1 member
>> variables to struct vmcr to support read and write of these fields.
>>
>> Also refactor vgic_set_vmcr and vgic_get_vmcr() code.
>> Drop ICH_VMCR_CTLR_SHIFT and ICH_VMCR_CTLR_MASK macros and instead
>> use ICH_VMCR_EOI* and ICH_VMCR_CBPR* macros
>> .
>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
>> ---
>> include/linux/irqchip/arm-gic-v3.h | 2 --
>> virt/kvm/arm/vgic/vgic-mmio-v2.c | 16 ----------------
>> virt/kvm/arm/vgic/vgic-mmio.c | 16 ++++++++++++++++
>> virt/kvm/arm/vgic/vgic-v3.c | 10 ++++++++--
>> virt/kvm/arm/vgic/vgic.h | 5 +++++
>> 5 files changed, 29 insertions(+), 20 deletions(-)
>>
>> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
>> index d48d886..61646aa 100644
>> --- a/include/linux/irqchip/arm-gic-v3.h
>> +++ b/include/linux/irqchip/arm-gic-v3.h
>> @@ -404,8 +404,6 @@
>> #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-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> index 2cb04b7..ad353b5 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> @@ -212,22 +212,6 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu *vcpu,
>> }
>> }
>>
>> -static void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
>> -{
>> - if (kvm_vgic_global_state.type == VGIC_V2)
>> - vgic_v2_set_vmcr(vcpu, vmcr);
>> - else
>> - vgic_v3_set_vmcr(vcpu, vmcr);
>> -}
>> -
>> -static void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
>> -{
>> - if (kvm_vgic_global_state.type == VGIC_V2)
>> - vgic_v2_get_vmcr(vcpu, vmcr);
>> - else
>> - vgic_v3_get_vmcr(vcpu, vmcr);
>> -}
>> -
>> #define GICC_ARCH_VERSION_V2 0x2
>>
>> /* These are for userland accesses only, there is no guest-facing emulation. */
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
>> index 9939d1d..173d6f0 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
>> @@ -416,6 +416,22 @@ int vgic_validate_mmio_region_addr(struct kvm_device *dev,
>> return -ENXIO;
>> }
>>
>> +void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
>> +{
>> + if (kvm_vgic_global_state.type == VGIC_V2)
>> + vgic_v2_set_vmcr(vcpu, vmcr);
>> + else
>> + vgic_v3_set_vmcr(vcpu, vmcr);
>> +}
>> +
>> +void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
>> +{
>> + if (kvm_vgic_global_state.type == VGIC_V2)
>> + vgic_v2_get_vmcr(vcpu, vmcr);
>> + else
>> + vgic_v3_get_vmcr(vcpu, vmcr);
>> +}
>> +
>> /*
>> * kvm_mmio_read_buf() returns a value in a format where it can be converted
>> * to a byte array and be directly observed as the guest wanted it to appear
>> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
>> index 9f0dae3..967c295 100644
>> --- a/virt/kvm/arm/vgic/vgic-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-v3.c
>> @@ -175,10 +175,13 @@ void vgic_v3_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp)
>> {
>> u32 vmcr;
>>
>> - vmcr = (vmcrp->ctlr << ICH_VMCR_CTLR_SHIFT) & ICH_VMCR_CTLR_MASK;
>> + vmcr = (vmcrp->ctlr << ICH_VMCR_CBPR_SHIFT) & ICH_VMCR_CBPR_MASK;
>> + vmcr |= (vmcrp->ctlr << ICH_VMCR_EOIM_SHIFT) & ICH_VMCR_EOIM_MASK;
>
> This looks weird: The EOImode field is bit[2] in the CTLR, and VEOIM is
> bit[9] in the ICH_VMCR, but you're just shifting the ctlr field left by
> 9 and then masking off everything by bit 9, so you'll end with never
> being able to set VEOIM I think...
>
OK
> Also, we do we now forget about VFIQEn and VAckCtl? The latter I can
> understand because it's deprecated, but why the first? This particular
> piece of information would be very nice to have in the commit message.
I understand that group 0 interrupts are not handled. So vFIQEn can be ignored.
Spec says, if SRE=1 (non-secure) this bit is RES1 also it is alias to
ICC_CTLR_EL1
if SRE is 1. However there is no bit in ICC_CTLR_EL1 for FIQen. It is defined
only in GICV_CTLR which is used when SRE=0.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
