From: vijay.kilari at gmail.com <hidden> Date: 2016-11-23 13:01:47
From: Vijaya Kumar K <redacted>
This patchset adds API for saving and restoring
of VGICv3 registers to support live migration with new vgic feature.
This API definition is as per version of VGICv3 specification
Documentation/virtual/kvm/devices/arm-vgic-v3.txt
The patch 3 & 4 are picked from the Pavel's previous implementation.
http://www.spinics.net/lists/kvm/msg122040.html
NOTE: Only compilation tested for AArch32. No hardware to test.
v8 => v9:
- Rebased to kvmarm/next branch
- Introduce support for save and restore of CPU interface
registers for AArch32 mode (9,10 and 11 patches).
Only compilation tested.
- Fixed vmcr.ctlr format
- Updated error code for invalid CPU REG value in Documentation
- Updated commit messages and added comments required
- Queued IRQ when irq_line is set.
- Compatibility check on ICC_CTLR_EL1.SEIS and A3V
v7 => v8:
- Rebased to 4.9-rc3
- Fixed wrong parameter to VGIC_TO_MPIDR
v6 => v7:
- Rename all patches heading from vgic-new to vgic
- Moved caching of priority and ID bits from vgic global struct
to vgic_cpu struct.
v5 => v6:
- Collated all register definitions to single patch (4)
- Introduce macro to convert userspace MPIDR format to MPIDR reg format
- Check on ICC_CTLR_EL1.CBPR value is made while accessing ICC_BPR1_EL1
- Cached ich_vtr_el2 and guests priority and ID bits
- Check on number of priority and ID bits when ICC_CTRL_EL1 write is made
- Check is made on SRE bit for ICC_SRE_EL1 write
v4 => v5:
- ICC_CTLR_EL1 access is updated to reflect HW values
- Updated ICC reg access mask and shift macros
- Introduced patch 4 for VMCR changes
- Other minor fixes.
v3 => v4:
- Rebased to latest code base
- Moved vgic_uaccess() from vgic-mmio-v2.c to vgic-mmio.c
- Dropped macro REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED_UACCESS
- Dropped LE conversion for userspace access
- Introduced vgic_uaccess_write_pending() for ISPENDR write
- Change macro KVM_DEV_ARM_VGIC_V3_CPUID_MASK to KVM_DEV_ARM_VGIC_V3_MIDR_MASK
- Refactored some code as common code.
- Changed handing of ICC_* registers
- Allowed ICC_SRE_EL1 read by userspace
- Fixed KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_* macros
v2 => v3:
- Implemented separate API for ISPENDR and ICPENDR to
read soft_pending instead of pending for level triggerred interrupts
- Implemented ioctl KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO to access line level
- Rebased on top of Christoffer's patch set
http://www.spinics.net/lists/kvm/msg136840.html
NOTE: GICD_STATUSR and GICR_STATUSR are implemented as RAZ/WI.
v1 => v2:
- The init sequence change patch is no more required.
Fixed in patch 2 by using static vgic_io_dev regions structure instead
of using dynamic allocation pointer.
- Updated commit message of patch 4.
- Dropped usage of union to manage 32-bit and 64-bit access in patch 1.
Used local variable for 32-bit access.
- Updated macro __ARM64_SYS_REG and ARM64_SYS_REG in
arch/arm64/include/uapi/asm/kvm.h as per qemu requirements.
Vijaya Kumar K (11):
arm/arm64: vgic: Implement support for userspace access
arm/arm64: vgic: Add distributor and redistributor access
arm/arm64: vgic: Introduce find_reg_by_id()
irqchip/gic-v3: Add missing system register definitions
arm/arm64: vgic: Introduce VENG0 and VENG1 fields to vmcr struct
arm/arm64: vgic: Implement VGICv3 CPU interface access
arm/arm64: vgic: Implement KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO ioctl
arm/arm64: Documentation: Update arm-vgic-v3.txt
arm: coproc: Drop const from coproc reg access function
arm: coproc: Introduce find_coproc_reg_by_id()
arm: vgic: Save and restore GICv3 CPU interface regs for AArch32
Documentation/virtual/kvm/devices/arm-vgic-v3.txt | 2 +-
arch/arm/include/uapi/asm/kvm.h | 13 ++
arch/arm/kvm/Makefile | 2 +
arch/arm/kvm/coproc.c | 42 ++--
arch/arm/kvm/coproc.h | 8 +-
arch/arm64/include/uapi/asm/kvm.h | 13 ++
arch/arm64/kvm/Makefile | 2 +
arch/arm64/kvm/sys_regs.c | 22 +-
arch/arm64/kvm/sys_regs.h | 4 +
include/kvm/arm_vgic.h | 9 +
include/linux/irqchip/arm-gic-v3.h | 45 +++-
virt/kvm/arm/vgic/vgic-coproc-reg-v3.c | 155 +++++++++++++
virt/kvm/arm/vgic/vgic-kvm-device.c | 219 +++++++++++++++++-
virt/kvm/arm/vgic/vgic-mmio-v2.c | 57 +----
virt/kvm/arm/vgic/vgic-mmio-v3.c | 203 +++++++++++++++--
virt/kvm/arm/vgic/vgic-mmio.c | 154 ++++++++++++-
virt/kvm/arm/vgic/vgic-mmio.h | 28 +++
virt/kvm/arm/vgic/vgic-sys-reg-common.c | 258 ++++++++++++++++++++++
virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 142 ++++++++++++
virt/kvm/arm/vgic/vgic-v3.c | 30 ++-
virt/kvm/arm/vgic/vgic.h | 78 ++++++-
21 files changed, 1365 insertions(+), 121 deletions(-)
create mode 100644 virt/kvm/arm/vgic/vgic-coproc-reg-v3.c
create mode 100644 virt/kvm/arm/vgic/vgic-sys-reg-common.c
create mode 100644 virt/kvm/arm/vgic/vgic-sys-reg-v3.c
--
1.9.1
From: vijay.kilari at gmail.com <hidden> Date: 2016-11-23 13:01:48
From: Vijaya Kumar K <redacted>
Read and write of some registers like ISPENDR and ICPENDR
from userspace requires special handling when compared to
guest access for these registers.
Refer to Documentation/virtual/kvm/devices/arm-vgic-v3.txt
for handling of ISPENDR, ICPENDR registers handling.
Add infrastructure to support guest and userspace read
and write for the required registers
Also moved vgic_uaccess from vgic-mmio-v2.c to vgic-mmio.c
Signed-off-by: Vijaya Kumar K <redacted>
---
virt/kvm/arm/vgic/vgic-mmio-v2.c | 25 ----------
virt/kvm/arm/vgic/vgic-mmio-v3.c | 102 ++++++++++++++++++++++++++++++++-------
virt/kvm/arm/vgic/vgic-mmio.c | 78 +++++++++++++++++++++++++++---
virt/kvm/arm/vgic/vgic-mmio.h | 19 ++++++++
4 files changed, 175 insertions(+), 49 deletions(-)
@@ -207,6 +207,66 @@ static unsigned long vgic_mmio_read_v3_idregs(struct kvm_vcpu *vcpu,return0;}+staticunsignedlongvgic_v3_uaccess_read_pending(structkvm_vcpu*vcpu,+gpa_taddr,unsignedintlen)+{+u32intid=VGIC_ADDR_TO_INTID(addr,1);+u32value=0;+inti;++/*+*Aleveltriggerredinterruptpendingstateislatchedinboth+*"soft_pending"and"line_level"variables.Userspacewillsave+*andrestoresoft_pendingandline_levelseparately.+*RefertoDocumentation/virtual/kvm/devices/arm-vgic-v3.txt+*handlingofISPENDRandICPENDR.+*/+for(i=0;i<len*8;i++){+structvgic_irq*irq=vgic_get_irq(vcpu->kvm,vcpu,intid+i);++if(irq->config==VGIC_CONFIG_LEVEL&&irq->soft_pending)+value|=(1U<<i);+if(irq->config==VGIC_CONFIG_EDGE&&irq->pending)+value|=(1U<<i);++vgic_put_irq(vcpu->kvm,irq);+}++returnvalue;+}++staticvoidvgic_v3_uaccess_write_pending(structkvm_vcpu*vcpu,+gpa_taddr,unsignedintlen,+unsignedlongval)+{+u32intid=VGIC_ADDR_TO_INTID(addr,1);+inti;++for(i=0;i<len*8;i++){+structvgic_irq*irq=vgic_get_irq(vcpu->kvm,vcpu,intid+i);++spin_lock(&irq->irq_lock);+if(test_bit(i,&val)){+/* soft_pending is set irrespective of irq type+*(leveloredge)toavoiddependencythatVMshould+*restoreirqconfigbeforependinginfo.+*/+irq->pending=true;+irq->soft_pending=true;+vgic_queue_irq_unlock(vcpu->kvm,irq);+}else{+irq->soft_pending=false;+if(irq->config==VGIC_CONFIG_EDGE||+(irq->config==VGIC_CONFIG_LEVEL&&+!irq->line_level))+irq->pending=false;+spin_unlock(&irq->irq_lock);+}++vgic_put_irq(vcpu->kvm,irq);+}+}+/* We want to avoid outer shareable. */u64vgic_sanitise_shareability(u64field){
From: vijay.kilari at gmail.com <hidden> Date: 2016-11-23 13:01:49
From: Vijaya Kumar K <redacted>
VGICv3 Distributor and Redistributor registers are accessed using
KVM_DEV_ARM_VGIC_GRP_DIST_REGS and KVM_DEV_ARM_VGIC_GRP_REDIST_REGS
with KVM_SET_DEVICE_ATTR and KVM_GET_DEVICE_ATTR ioctls.
These registers are accessed as 32-bit and cpu mpidr
value passed along with register offset is used to identify the
cpu for redistributor registers access.
The version of VGIC v3 specification is define here
Documentation/virtual/kvm/devices/arm-vgic-v3.txt
Also update arch/arm/include/uapi/asm/kvm.h to compile for
AArch32 mode.
Signed-off-by: Vijaya Kumar K <redacted>
---
arch/arm/include/uapi/asm/kvm.h | 4 +
arch/arm64/include/uapi/asm/kvm.h | 4 +
virt/kvm/arm/vgic/vgic-kvm-device.c | 144 ++++++++++++++++++++++++++++++++++--
virt/kvm/arm/vgic/vgic-mmio-v2.c | 16 +---
virt/kvm/arm/vgic/vgic-mmio-v3.c | 72 ++++++++++++++++++
virt/kvm/arm/vgic/vgic-mmio.c | 22 ++++++
virt/kvm/arm/vgic/vgic-mmio.h | 4 +
virt/kvm/arm/vgic/vgic.h | 49 +++++++++++-
8 files changed, 292 insertions(+), 23 deletions(-)
@@ -612,6 +623,34 @@ int vgic_register_redist_iodevs(struct kvm *kvm, gpa_t redist_base_address)returnret;}+intvgic_v3_has_attr_regs(structkvm_device*dev,structkvm_device_attr*attr)+{+conststructvgic_register_region*regions;+gpa_taddr;+intnr_regions;++addr=attr->attr&KVM_DEV_ARM_VGIC_OFFSET_MASK;++switch(attr->group){+caseKVM_DEV_ARM_VGIC_GRP_DIST_REGS:+regions=vgic_v3_dist_registers;+nr_regions=ARRAY_SIZE(vgic_v3_dist_registers);+break;+caseKVM_DEV_ARM_VGIC_GRP_REDIST_REGS:{+regions=vgic_v3_rdbase_registers;+nr_regions=ARRAY_SIZE(vgic_v3_rdbase_registers);+break;+}+default:+return-ENXIO;+}++/* We only support aligned 32-bit accesses. */+if(addr&3)+return-ENXIO;++returnvgic_validate_mmio_region_addr(dev,regions,nr_regions,addr);+}/**Compareagivenaffinity(level1-3andalevel0mask,fromtheSGI*generationregisterICC_SGI1R_EL1)withagivenVCPU.
@@ -718,3 +757,36 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)vgic_put_irq(vcpu->kvm,irq);}}++intvgic_v3_dist_uaccess(structkvm_vcpu*vcpu,boolis_write,+intoffset,u32*val)+{+structvgic_io_devicedev={+.regions=vgic_v3_dist_registers,+.nr_regions=ARRAY_SIZE(vgic_v3_dist_registers),+};++returnvgic_uaccess(vcpu,&dev,is_write,offset,val);+}++intvgic_v3_redist_uaccess(structkvm_vcpu*vcpu,boolis_write,+intoffset,u32*val)+{+structvgic_io_devicerd_dev={+.regions=vgic_v3_rdbase_registers,+.nr_regions=ARRAY_SIZE(vgic_v3_rdbase_registers),+};++structvgic_io_devicesgi_dev={+.regions=vgic_v3_sgibase_registers,+.nr_regions=ARRAY_SIZE(vgic_v3_sgibase_registers),+};++/* SGI_base is the next 64K frame after RD_base */+if(offset>=SZ_64K)+returnvgic_uaccess(vcpu,&sgi_dev,is_write,+offset-SZ_64K,val);+else+returnvgic_uaccess(vcpu,&rd_dev,is_write,+offset,val);+}
@@ -394,6 +394,28 @@ static int match_region(const void *key, const void *elt)sizeof(region[0]),match_region);}+/* Check if address falls within the region */+intvgic_validate_mmio_region_addr(structkvm_device*dev,+conststructvgic_register_region*regions,+intnr_regions,gpa_taddr)+{+inti,len;+intnr_irqs=dev->kvm->arch.vgic.nr_spis+VGIC_NR_PRIVATE_IRQS;++for(i=0;i<nr_regions;i++){+if(regions[i].bits_per_irq)+len=(regions[i].bits_per_irq*nr_irqs)/8;+else+len=regions[i].len;++if(regions[i].reg_offset<=addr&&+regions[i].reg_offset+len>addr)+return0;+}++return-ENXIO;+}+/**kvm_mmio_read_buf()returnsavalueinaformatwhereitcanbeconverted*toabytearrayandbedirectlyobservedastheguestwantedittoappear
From: vijay.kilari at gmail.com <hidden> Date: 2016-11-23 13:01:50
From: Vijaya Kumar K <redacted>
In order to implement vGICv3 CPU interface access, we will need to perform
table lookup of system registers. We would need both index_to_params() and
find_reg() exported for that purpose, but instead we export a single
function which combines them both.
Signed-off-by: Pavel Fedin <redacted>
Signed-off-by: Vijaya Kumar K <redacted>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Acked-by: Christoffer Dall <redacted>
---
arch/arm64/kvm/sys_regs.c | 22 +++++++++++++++-------
arch/arm64/kvm/sys_regs.h | 4 ++++
2 files changed, 19 insertions(+), 7 deletions(-)
@@ -1929,9 +1938,8 @@ static int set_invariant_sys_reg(u64 id, void __user *uaddr)interr;u64val=0;/* Make sure high bits are 0 for 32-bit regs */-if(!index_to_params(id,¶ms))-return-ENOENT;-r=find_reg(¶ms,invariant_sys_regs,ARRAY_SIZE(invariant_sys_regs));+r=find_reg_by_id(id,¶ms,invariant_sys_regs,+ARRAY_SIZE(invariant_sys_regs));if(!r)return-ENOENT;
From: vijay.kilari at gmail.com <hidden> Date: 2016-11-23 13:01:52
From: Vijaya Kumar K <redacted>
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 <redacted>
---
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 | 22 ++++++++++++++++++++--
virt/kvm/arm/vgic/vgic.h | 5 +++++
5 files changed, 41 insertions(+), 20 deletions(-)
@@ -212,22 +212,6 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu *vcpu,}}-staticvoidvgic_set_vmcr(structkvm_vcpu*vcpu,structvgic_vmcr*vmcr)-{-if(kvm_vgic_global_state.type==VGIC_V2)-vgic_v2_set_vmcr(vcpu,vmcr);-else-vgic_v3_set_vmcr(vcpu,vmcr);-}--staticvoidvgic_get_vmcr(structkvm_vcpu*vcpu,structvgic_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. */
@@ -78,6 +78,9 @@ struct vgic_vmcr {u32abpr;u32bpr;u32pmr;+/* Below member variable are valid only for GICv3 */+u32grpen0;+u32grpen1;};structvgic_irq*vgic_get_irq(structkvm*kvm,structkvm_vcpu*vcpu,
@@ -138,6 +141,8 @@ int vgic_v3_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write,intvgic_v3_redist_uaccess(structkvm_vcpu*vcpu,boolis_write,intoffset,u32*val);intkvm_register_vgic_device(unsignedlongtype);+voidvgic_set_vmcr(structkvm_vcpu*vcpu,structvgic_vmcr*vmcr);+voidvgic_get_vmcr(structkvm_vcpu*vcpu,structvgic_vmcr*vmcr);intvgic_lazy_init(structkvm*kvm);intvgic_init(structkvm*kvm);
From: vijay.kilari at gmail.com <hidden> Date: 2016-11-23 13:01:53
From: Vijaya Kumar K <redacted>
VGICv3 CPU interface registers are accessed using
KVM_DEV_ARM_VGIC_CPU_SYSREGS ioctl. These registers are accessed
as 64-bit. The cpu MPIDR value is passed along with register id.
is used to identify the cpu for registers access.
The VM that supports SEIs expect it on destination machine to handle
guest aborts and hence checked for ICC_CTLR_EL1.SEIS compatibility.
Similarly, VM that supports Affinity Level 3 that is required for AArch64
mode, is required to be supported on destination machine. Hence checked
for ICC_CTLR_EL1.A3V compatibility.
The CPU system register handling is spitted into two files
vgic-sys-reg-common.c and vgic-sys-reg-v3.c.
The vgic-sys-reg-common.c handles read and write of VGIC CPU registers
for both AArch64 and AArch32 mode. The vgic-sys-reg-v3.c handles AArch64
mode and is compiled only for AArch64 mode.
Updated arch/arm/include/uapi/asm/kvm.h with new definitions
required to compile for AArch32.
The version of VGIC v3 specification is define here
Documentation/virtual/kvm/devices/arm-vgic-v3.txt
Signed-off-by: Pavel Fedin <redacted>
Signed-off-by: Vijaya Kumar K <redacted>
---
arch/arm/include/uapi/asm/kvm.h | 2 +
arch/arm64/include/uapi/asm/kvm.h | 3 +
arch/arm64/kvm/Makefile | 2 +
include/kvm/arm_vgic.h | 9 ++
virt/kvm/arm/vgic/vgic-kvm-device.c | 28 ++++
virt/kvm/arm/vgic/vgic-mmio-v3.c | 18 +++
virt/kvm/arm/vgic/vgic-sys-reg-common.c | 258 ++++++++++++++++++++++++++++++++
virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 142 ++++++++++++++++++
virt/kvm/arm/vgic/vgic-v3.c | 8 +
virt/kvm/arm/vgic/vgic.h | 22 +++
10 files changed, 492 insertions(+)
@@ -641,6 +641,24 @@ int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)nr_regions=ARRAY_SIZE(vgic_v3_rdbase_registers);break;}+caseKVM_DEV_ARM_VGIC_CPU_SYSREGS:{+u64reg,id;+unsignedlongvgic_mpidr,mpidr_reg;+structkvm_vcpu*vcpu;++vgic_mpidr=(attr->attr&KVM_DEV_ARM_VGIC_V3_MPIDR_MASK)>>+KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT;++/* Convert plain mpidr value to MPIDR reg format */+mpidr_reg=VGIC_TO_MPIDR(vgic_mpidr);++vcpu=kvm_mpidr_to_vcpu(dev->kvm,mpidr_reg);+if(!vcpu)+return-EINVAL;++id=(attr->attr&KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK);+returnvgic_v3_has_cpu_sysregs_attr(vcpu,0,id,®);+}default:return-ENXIO;}
@@ -0,0 +1,258 @@+/*+*VGICsystemregistershandlingfunctions+*+*Thisprogramisfreesoftware;youcanredistributeitand/ormodify+*itunderthetermsoftheGNUGeneralPublicLicenseversion2as+*publishedbytheFreeSoftwareFoundation.+*+*Thisprogramisdistributedinthehopethatitwillbeuseful,+*butWITHOUTANYWARRANTY;withouteventheimpliedwarrantyof+*MERCHANTABILITYorFITNESSFORAPARTICULARPURPOSE.Seethe+*GNUGeneralPublicLicenseformoredetails.+*/++#include<linux/irqchip/arm-gic-v3.h>+#include<linux/kvm.h>+#include<linux/kvm_host.h>+#include<asm/kvm_emulate.h>+#include"vgic.h"++boolaccess_gic_ctlr_reg(structkvm_vcpu*vcpu,boolis_write,+unsignedlong*reg)+{+structvgic_cpu*vgic_v3_cpu=&vcpu->arch.vgic_cpu;+structvgic_vmcrvmcr;+u64val;+u32valid_bits,seis,a3v;++vgic_get_vmcr(vcpu,&vmcr);+if(is_write){+val=*reg;++/*+*DisallowrestoringVMstateifnotsupportedbythis+*hardware.+*/+valid_bits=((val&ICC_CTLR_EL1_PRI_BITS_MASK)>>+ICC_CTLR_EL1_PRI_BITS_SHIFT)+1;+if(valid_bits>vgic_v3_cpu->num_pri_bits)+returnfalse;++vgic_v3_cpu->num_pri_bits=valid_bits;++valid_bits=(val&ICC_CTLR_EL1_ID_BITS_MASK)>>+ICC_CTLR_EL1_ID_BITS_SHIFT;+if(valid_bits>vgic_v3_cpu->num_id_bits)+returnfalse;++vgic_v3_cpu->num_id_bits=valid_bits;++valid_bits=((kvm_vgic_global_state.ich_vtr_el2&+ICH_VTR_SEIS_MASK)>>ICH_VTR_SEIS_SHIFT);+seis=(val&ICC_CTLR_EL1_SEIS_MASK)>>+ICC_CTLR_EL1_SEIS_SHIFT;+if(valid_bits!=seis)+returnfalse;++valid_bits=((kvm_vgic_global_state.ich_vtr_el2&+ICH_VTR_A3V_MASK)>>ICH_VTR_A3V_SHIFT);+a3v=(val&ICC_CTLR_EL1_A3V_MASK)>>+ICC_CTLR_EL1_A3V_SHIFT;+if(valid_bits!=a3v)+returnfalse;++vmcr.ctlr=(val&ICC_CTLR_EL1_CBPR_MASK);+vmcr.ctlr|=(val&ICC_CTLR_EL1_EOImode_MASK);+vgic_set_vmcr(vcpu,&vmcr);+}else{+val=0;+val|=(vgic_v3_cpu->num_pri_bits-1)<<+ICC_CTLR_EL1_PRI_BITS_SHIFT;+val|=vgic_v3_cpu->num_id_bits<<+ICC_CTLR_EL1_ID_BITS_SHIFT;+val|=((kvm_vgic_global_state.ich_vtr_el2&+ICH_VTR_SEIS_MASK)>>ICH_VTR_SEIS_SHIFT)<<+ICC_CTLR_EL1_SEIS_SHIFT;+val|=((kvm_vgic_global_state.ich_vtr_el2&+ICH_VTR_A3V_MASK)>>ICH_VTR_A3V_SHIFT)<<+ICC_CTLR_EL1_A3V_SHIFT;+val|=(vmcr.ctlr&ICC_CTLR_EL1_CBPR_MASK);+val|=(vmcr.ctlr&ICC_CTLR_EL1_EOImode_MASK);++*reg=val;+}++returntrue;+}++boolaccess_gic_pmr_reg(structkvm_vcpu*vcpu,boolis_write,+unsignedlong*reg)+{+structvgic_vmcrvmcr;++vgic_get_vmcr(vcpu,&vmcr);+if(is_write){+vmcr.pmr=(*reg&ICC_PMR_EL1_MASK)>>ICC_PMR_EL1_SHIFT;+vgic_set_vmcr(vcpu,&vmcr);+}else{+*reg=(vmcr.pmr<<ICC_PMR_EL1_SHIFT)&ICC_PMR_EL1_MASK;+}++returntrue;+}++boolaccess_gic_bpr0_reg(structkvm_vcpu*vcpu,boolis_write,+unsignedlong*reg)+{+structvgic_vmcrvmcr;++vgic_get_vmcr(vcpu,&vmcr);+if(is_write){+vmcr.bpr=(*reg&ICC_BPR0_EL1_MASK)>>+ICC_BPR0_EL1_SHIFT;+vgic_set_vmcr(vcpu,&vmcr);+}else{+*reg=(vmcr.bpr<<ICC_BPR0_EL1_SHIFT)&ICC_BPR0_EL1_MASK;+}++returntrue;+}++boolaccess_gic_bpr1_reg(structkvm_vcpu*vcpu,boolis_write,+unsignedlong*reg)+{+structvgic_vmcrvmcr;++if(!is_write)+*reg=0;++vgic_get_vmcr(vcpu,&vmcr);+if(!((vmcr.ctlr&ICH_VMCR_CBPR_MASK)>>ICH_VMCR_CBPR_SHIFT)){+if(is_write){+vmcr.abpr=(*reg&ICC_BPR1_EL1_MASK)>>+ICC_BPR1_EL1_SHIFT;+vgic_set_vmcr(vcpu,&vmcr);+}else{+*reg=(vmcr.abpr<<ICC_BPR1_EL1_SHIFT)&+ICC_BPR1_EL1_MASK;+}+}else{+if(!is_write)+*reg=min((vmcr.bpr+1),7U);+}++returntrue;+}++boolaccess_gic_grpen0_reg(structkvm_vcpu*vcpu,boolis_write,+unsignedlong*reg)+{+structvgic_vmcrvmcr;++vgic_get_vmcr(vcpu,&vmcr);+if(is_write){+vmcr.grpen0=(*reg&ICC_IGRPEN0_EL1_MASK)>>+ICC_IGRPEN0_EL1_SHIFT;+vgic_set_vmcr(vcpu,&vmcr);+}else{+*reg=(vmcr.grpen0<<ICC_IGRPEN0_EL1_SHIFT)&+ICC_IGRPEN0_EL1_MASK;+}++returntrue;+}++boolaccess_gic_grpen1_reg(structkvm_vcpu*vcpu,boolis_write,+unsignedlong*reg)+{+structvgic_vmcrvmcr;++vgic_get_vmcr(vcpu,&vmcr);+if(is_write){+vmcr.grpen1=(*reg&ICC_IGRPEN1_EL1_MASK)>>+ICC_IGRPEN1_EL1_SHIFT;+vgic_set_vmcr(vcpu,&vmcr);+}else{+*reg=(vmcr.grpen1<<ICC_IGRPEN1_EL1_SHIFT)&+ICC_IGRPEN1_EL1_MASK;+}++returntrue;+}++staticvoidvgic_v3_access_apr_reg(structkvm_vcpu*vcpu,boolis_write,+u8apr,u8idx,unsignedlong*reg)+{+structvgic_v3_cpu_if*vgicv3=&vcpu->arch.vgic_cpu.vgic_v3;+uint32_t*ap_reg;++if(apr)+ap_reg=&vgicv3->vgic_ap1r[idx];+else+ap_reg=&vgicv3->vgic_ap0r[idx];++if(is_write)+*ap_reg=*reg;+else+*reg=*ap_reg;+}++staticboolaccess_gic_aprn(structkvm_vcpu*vcpu,boolis_write,u8apr,+u8idx,unsignedlong*reg)+{+structvgic_cpu*vgic_v3_cpu=&vcpu->arch.vgic_cpu;++/* num_pri_bits are initialized with HW supported values.+*Wecanrelysafelyonnum_pri_bitsevenifVMhasnot+*restoredICC_CTLR_EL1beforerestoringAPnRregisters.+*/+switch(vgic_v3_cpu->num_pri_bits){+case7:+vgic_v3_access_apr_reg(vcpu,is_write,apr,idx,reg);+break;+case6:+if(idx>1)+gotoerr;+vgic_v3_access_apr_reg(vcpu,is_write,apr,idx,reg);+break;+default:+if(idx>0)+gotoerr;+vgic_v3_access_apr_reg(vcpu,is_write,apr,idx,reg);+}++returntrue;+err:+if(!is_write)+*reg=0;++returnfalse;+}++boolaccess_gic_ap0r_reg(structkvm_vcpu*vcpu,boolis_write,u8idx,+unsignedlong*reg)+{+returnaccess_gic_aprn(vcpu,is_write,0,idx,reg);+}++boolaccess_gic_ap1r_reg(structkvm_vcpu*vcpu,boolis_write,u8idx,+unsignedlong*reg)+{+returnaccess_gic_aprn(vcpu,is_write,1,idx,reg);+}++boolaccess_gic_sre_reg(structkvm_vcpu*vcpu,boolis_write,+unsignedlong*reg)+{+structvgic_v3_cpu_if*vgicv3=&vcpu->arch.vgic_cpu.vgic_v3;++/* Validate SRE bit */+if(is_write){+if(!(*reg&ICC_SRE_EL1_SRE))+returnfalse;+}else{+*reg=vgicv3->vgic_sre;+}++returntrue;+}
@@ -240,6 +240,13 @@ void vgic_v3_enable(struct kvm_vcpu *vcpu)vgic_v3->vgic_sre=0;}+vcpu->arch.vgic_cpu.num_id_bits=(kvm_vgic_global_state.ich_vtr_el2&+ICH_VTR_ID_BITS_MASK)>>+ICH_VTR_ID_BITS_SHIFT;+vcpu->arch.vgic_cpu.num_pri_bits=((kvm_vgic_global_state.ich_vtr_el2&+ICH_VTR_PRI_BITS_MASK)>>+ICH_VTR_PRI_BITS_SHIFT)+1;+/* Get the show on the road... */vgic_v3->vgic_hcr=ICH_HCR_EN;}
@@ -340,6 +347,7 @@ int vgic_v3_probe(const struct gic_kvm_info *info)*/kvm_vgic_global_state.nr_lr=(ich_vtr_el2&0xf)+1;kvm_vgic_global_state.can_emulate_gicv2=false;+kvm_vgic_global_state.ich_vtr_el2=ich_vtr_el2;if(!info->vcpu.start){kvm_info("GICv3: no GICV resource entry\n");
@@ -145,7 +145,7 @@ Groups: Errors: -ENXIO: Getting or setting this register is not yet supported -EBUSY: VCPU is running- -EINVAL: Invalid mpidr supplied+ -EINVAL: Invalid mpidr or register value supplied KVM_DEV_ARM_VGIC_GRP_NR_IRQS
From: vijay.kilari at gmail.com <hidden> Date: 2016-11-23 13:01:56
From: Vijaya Kumar K <redacted>
In order to read or write to coproc regiter, the
access function is should allow the coproc_param to be non
constant.
Signed-off-by: Vijaya Kumar K <redacted>
---
arch/arm/kvm/coproc.c | 20 ++++++++++----------
arch/arm/kvm/coproc.h | 4 ++--
2 files changed, 12 insertions(+), 12 deletions(-)
From: vijay.kilari at gmail.com <hidden> Date: 2016-11-23 13:01:57
From: Vijaya Kumar K <redacted>
To access CPU interface access in AArch32 mode, we need to
perform coproc_reg table lookup. For this introduce
find_coproc_reg_by_id() and export.
Also use this function internally in coproc.c wherever
required.
Signed-off-by: Vijaya Kumar K <redacted>
---
arch/arm/kvm/coproc.c | 22 +++++++++++++++-------
arch/arm/kvm/coproc.h | 4 ++++
2 files changed, 19 insertions(+), 7 deletions(-)
From: vijay.kilari at gmail.com <hidden> Date: 2016-11-23 13:01:58
From: Vijaya Kumar K <redacted>
Use coproc_reg infrastructure to save and restore CPU
interface register of GICv3 for AArch32 host.
Signed-off-by: Vijaya Kumar K <redacted>
---
arch/arm/kvm/Makefile | 2 +
virt/kvm/arm/vgic/vgic-coproc-reg-v3.c | 155 +++++++++++++++++++++++++++++++++
2 files changed, 157 insertions(+)
From: Christoffer Dall <hidden> Date: 2016-11-28 13:05:12
On Wed, Nov 23, 2016 at 06:31:48PM +0530, vijay.kilari at gmail.com wrote:
quoted hunk
From: Vijaya Kumar K <redacted>
Read and write of some registers like ISPENDR and ICPENDR
from userspace requires special handling when compared to
guest access for these registers.
Refer to Documentation/virtual/kvm/devices/arm-vgic-v3.txt
for handling of ISPENDR, ICPENDR registers handling.
Add infrastructure to support guest and userspace read
and write for the required registers
Also moved vgic_uaccess from vgic-mmio-v2.c to vgic-mmio.c
Signed-off-by: Vijaya Kumar K <redacted>
---
virt/kvm/arm/vgic/vgic-mmio-v2.c | 25 ----------
virt/kvm/arm/vgic/vgic-mmio-v3.c | 102 ++++++++++++++++++++++++++++++++-------
virt/kvm/arm/vgic/vgic-mmio.c | 78 +++++++++++++++++++++++++++---
virt/kvm/arm/vgic/vgic-mmio.h | 19 ++++++++
4 files changed, 175 insertions(+), 49 deletions(-)
@@ -207,6 +207,66 @@ static unsigned long vgic_mmio_read_v3_idregs(struct kvm_vcpu *vcpu,return0;}+staticunsignedlongvgic_v3_uaccess_read_pending(structkvm_vcpu*vcpu,+gpa_taddr,unsignedintlen)+{+u32intid=VGIC_ADDR_TO_INTID(addr,1);+u32value=0;+inti;++/*+*Aleveltriggerredinterruptpendingstateislatchedinboth+*"soft_pending"and"line_level"variables.Userspacewillsave+*andrestoresoft_pendingandline_levelseparately.+*RefertoDocumentation/virtual/kvm/devices/arm-vgic-v3.txt+*handlingofISPENDRandICPENDR.+*/+for(i=0;i<len*8;i++){+structvgic_irq*irq=vgic_get_irq(vcpu->kvm,vcpu,intid+i);++if(irq->config==VGIC_CONFIG_LEVEL&&irq->soft_pending)+value|=(1U<<i);+if(irq->config==VGIC_CONFIG_EDGE&&irq->pending)+value|=(1U<<i);++vgic_put_irq(vcpu->kvm,irq);+}++returnvalue;+}++staticvoidvgic_v3_uaccess_write_pending(structkvm_vcpu*vcpu,+gpa_taddr,unsignedintlen,+unsignedlongval)+{+u32intid=VGIC_ADDR_TO_INTID(addr,1);+inti;++for(i=0;i<len*8;i++){+structvgic_irq*irq=vgic_get_irq(vcpu->kvm,vcpu,intid+i);++spin_lock(&irq->irq_lock);+if(test_bit(i,&val)){+/* soft_pending is set irrespective of irq type+*(leveloredge)toavoiddependencythatVMshould+*restoreirqconfigbeforependinginfo.+*/
@@ -356,7 +416,7 @@ static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu, * We take some special care here to fix the calculation of the register * offset. */-#define REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(off, rd, wr, bpi, acc) \+#define REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(off, rd, wr, ur, uw, bpi, acc) \ { \ .reg_offset = off, \ .bits_per_irq = bpi, \
From: Christoffer Dall <hidden> Date: 2016-11-28 13:08:25
On Wed, Nov 23, 2016 at 06:31:49PM +0530, vijay.kilari at gmail.com wrote:
quoted hunk
From: Vijaya Kumar K <redacted>
VGICv3 Distributor and Redistributor registers are accessed using
KVM_DEV_ARM_VGIC_GRP_DIST_REGS and KVM_DEV_ARM_VGIC_GRP_REDIST_REGS
with KVM_SET_DEVICE_ATTR and KVM_GET_DEVICE_ATTR ioctls.
These registers are accessed as 32-bit and cpu mpidr
value passed along with register offset is used to identify the
cpu for redistributor registers access.
The version of VGIC v3 specification is define here
Documentation/virtual/kvm/devices/arm-vgic-v3.txt
Also update arch/arm/include/uapi/asm/kvm.h to compile for
AArch32 mode.
Signed-off-by: Vijaya Kumar K <redacted>
---
arch/arm/include/uapi/asm/kvm.h | 4 +
arch/arm64/include/uapi/asm/kvm.h | 4 +
virt/kvm/arm/vgic/vgic-kvm-device.c | 144 ++++++++++++++++++++++++++++++++++--
virt/kvm/arm/vgic/vgic-mmio-v2.c | 16 +---
virt/kvm/arm/vgic/vgic-mmio-v3.c | 72 ++++++++++++++++++
virt/kvm/arm/vgic/vgic-mmio.c | 22 ++++++
virt/kvm/arm/vgic/vgic-mmio.h | 4 +
virt/kvm/arm/vgic/vgic.h | 49 +++++++++++-
8 files changed, 292 insertions(+), 23 deletions(-)
@@ -612,6 +623,34 @@ int vgic_register_redist_iodevs(struct kvm *kvm, gpa_t redist_base_address)returnret;}+intvgic_v3_has_attr_regs(structkvm_device*dev,structkvm_device_attr*attr)+{+conststructvgic_register_region*regions;+gpa_taddr;+intnr_regions;++addr=attr->attr&KVM_DEV_ARM_VGIC_OFFSET_MASK;++switch(attr->group){+caseKVM_DEV_ARM_VGIC_GRP_DIST_REGS:+regions=vgic_v3_dist_registers;+nr_regions=ARRAY_SIZE(vgic_v3_dist_registers);+break;+caseKVM_DEV_ARM_VGIC_GRP_REDIST_REGS:{+regions=vgic_v3_rdbase_registers;+nr_regions=ARRAY_SIZE(vgic_v3_rdbase_registers);+break;+}+default:+return-ENXIO;+}++/* We only support aligned 32-bit accesses. */+if(addr&3)+return-ENXIO;++returnvgic_validate_mmio_region_addr(dev,regions,nr_regions,addr);+}/**Compareagivenaffinity(level1-3andalevel0mask,fromtheSGI*generationregisterICC_SGI1R_EL1)withagivenVCPU.
@@ -718,3 +757,36 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)vgic_put_irq(vcpu->kvm,irq);}}++intvgic_v3_dist_uaccess(structkvm_vcpu*vcpu,boolis_write,+intoffset,u32*val)+{+structvgic_io_devicedev={+.regions=vgic_v3_dist_registers,+.nr_regions=ARRAY_SIZE(vgic_v3_dist_registers),+};++returnvgic_uaccess(vcpu,&dev,is_write,offset,val);+}++intvgic_v3_redist_uaccess(structkvm_vcpu*vcpu,boolis_write,+intoffset,u32*val)+{+structvgic_io_devicerd_dev={+.regions=vgic_v3_rdbase_registers,+.nr_regions=ARRAY_SIZE(vgic_v3_rdbase_registers),+};++structvgic_io_devicesgi_dev={+.regions=vgic_v3_sgibase_registers,+.nr_regions=ARRAY_SIZE(vgic_v3_sgibase_registers),+};++/* SGI_base is the next 64K frame after RD_base */+if(offset>=SZ_64K)+returnvgic_uaccess(vcpu,&sgi_dev,is_write,+offset-SZ_64K,val);+else+returnvgic_uaccess(vcpu,&rd_dev,is_write,+offset,val);+}
@@ -394,6 +394,28 @@ static int match_region(const void *key, const void *elt)sizeof(region[0]),match_region);}+/* Check if address falls within the region */+intvgic_validate_mmio_region_addr(structkvm_device*dev,+conststructvgic_register_region*regions,+intnr_regions,gpa_taddr)+{+inti,len;+intnr_irqs=dev->kvm->arch.vgic.nr_spis+VGIC_NR_PRIVATE_IRQS;++for(i=0;i<nr_regions;i++){+if(regions[i].bits_per_irq)+len=(regions[i].bits_per_irq*nr_irqs)/8;+else+len=regions[i].len;++if(regions[i].reg_offset<=addr&&+regions[i].reg_offset+len>addr)+return0;+}++return-ENXIO;+}+/**kvm_mmio_read_buf()returnsavalueinaformatwhereitcanbeconverted*toabytearrayandbedirectlyobservedastheguestwantedittoappear
From: Christoffer Dall <hidden> Date: 2016-11-28 14:28:36
On Wed, Nov 23, 2016 at 06:31:52PM +0530, vijay.kilari at gmail.com wrote:
quoted hunk
From: Vijaya Kumar K <redacted>
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 <redacted>
---
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 | 22 ++++++++++++++++++++--
virt/kvm/arm/vgic/vgic.h | 5 +++++
5 files changed, 41 insertions(+), 20 deletions(-)
@@ -212,22 +212,6 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu *vcpu,}}-staticvoidvgic_set_vmcr(structkvm_vcpu*vcpu,structvgic_vmcr*vmcr)-{-if(kvm_vgic_global_state.type==VGIC_V2)-vgic_v2_set_vmcr(vcpu,vmcr);-else-vgic_v3_set_vmcr(vcpu,vmcr);-}--staticvoidvgic_get_vmcr(structkvm_vcpu*vcpu,structvgic_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. */
@@ -78,6 +78,9 @@ struct vgic_vmcr {u32abpr;u32bpr;u32pmr;+/* Below member variable are valid only for GICv3 */+u32grpen0;+u32grpen1;};structvgic_irq*vgic_get_irq(structkvm*kvm,structkvm_vcpu*vcpu,
@@ -138,6 +141,8 @@ int vgic_v3_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write,intvgic_v3_redist_uaccess(structkvm_vcpu*vcpu,boolis_write,intoffset,u32*val);intkvm_register_vgic_device(unsignedlongtype);+voidvgic_set_vmcr(structkvm_vcpu*vcpu,structvgic_vmcr*vmcr);+voidvgic_get_vmcr(structkvm_vcpu*vcpu,structvgic_vmcr*vmcr);intvgic_lazy_init(structkvm*kvm);intvgic_init(structkvm*kvm);
--
1.9.1
My comments on style above notwithstanding:
Reviewed-by: Christoffer Dall <redacted>
From: Christoffer Dall <hidden> Date: 2016-11-28 19:39:38
On Wed, Nov 23, 2016 at 06:31:53PM +0530, vijay.kilari at gmail.com wrote:
From: Vijaya Kumar K <redacted>
VGICv3 CPU interface registers are accessed using
KVM_DEV_ARM_VGIC_CPU_SYSREGS ioctl. These registers are accessed
as 64-bit. The cpu MPIDR value is passed along with register id.
is used to identify the cpu for registers access.
The VM that supports SEIs expect it on destination machine to handle
guest aborts and hence checked for ICC_CTLR_EL1.SEIS compatibility.
Similarly, VM that supports Affinity Level 3 that is required for AArch64
mode, is required to be supported on destination machine. Hence checked
for ICC_CTLR_EL1.A3V compatibility.
The CPU system register handling is spitted into two files
spitted? Did you mean 'split into' ?
vgic-sys-reg-common.c and vgic-sys-reg-v3.c.
The vgic-sys-reg-common.c handles read and write of VGIC CPU registers
So this is weird because everything in virt/kvm/arm/ is exactly supposed
to be common between arm and arm64 already.
I would rather that you had a copy of vgic-sys-reg-v3.c in arch/arm/kvm/
and in arch/arm64/kvm/ each taking care of its own architecture.
But note that I didn't actually require that you implemented support for
GICv3 migration on AArch32 hosts for these patches, I just didn't want
thigns to silently break.
If we cannot test the AArch32 implementation, we should potentially just
make sure that is not supported yet, return a proper error to userspace
and get the AArch64 host implementation correct.
I suggest you move your:
virt/kvm/arm/vgic/vgic-sys-reg-v3.c to
arch/arm64/kvm/vgic-sys-reg-v3.c
and rename
virt/kvm/arm/vgic/vgic-sys-reg-common.c to
virt/kvm/arm/vgic/vgic-sys-reg-v3.c
And then wait with the AArch32 host side for now, but just make sure it
compiles and returns an error as opposed to crashing the system if
someone tries to excercise this interface on an AArch32 host.
quoted hunk
for both AArch64 and AArch32 mode. The vgic-sys-reg-v3.c handles AArch64
mode and is compiled only for AArch64 mode.
Updated arch/arm/include/uapi/asm/kvm.h with new definitions
required to compile for AArch32.
The version of VGIC v3 specification is define here
Documentation/virtual/kvm/devices/arm-vgic-v3.txt
Signed-off-by: Pavel Fedin <redacted>
Signed-off-by: Vijaya Kumar K <redacted>
---
arch/arm/include/uapi/asm/kvm.h | 2 +
arch/arm64/include/uapi/asm/kvm.h | 3 +
arch/arm64/kvm/Makefile | 2 +
include/kvm/arm_vgic.h | 9 ++
virt/kvm/arm/vgic/vgic-kvm-device.c | 28 ++++
virt/kvm/arm/vgic/vgic-mmio-v3.c | 18 +++
virt/kvm/arm/vgic/vgic-sys-reg-common.c | 258 ++++++++++++++++++++++++++++++++
virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 142 ++++++++++++++++++
virt/kvm/arm/vgic/vgic-v3.c | 8 +
virt/kvm/arm/vgic/vgic.h | 22 +++
10 files changed, 492 insertions(+)
@@ -641,6 +641,24 @@ int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)nr_regions=ARRAY_SIZE(vgic_v3_rdbase_registers);break;}+caseKVM_DEV_ARM_VGIC_CPU_SYSREGS:{+u64reg,id;+unsignedlongvgic_mpidr,mpidr_reg;+structkvm_vcpu*vcpu;++vgic_mpidr=(attr->attr&KVM_DEV_ARM_VGIC_V3_MPIDR_MASK)>>+KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT;++/* Convert plain mpidr value to MPIDR reg format */+mpidr_reg=VGIC_TO_MPIDR(vgic_mpidr);++vcpu=kvm_mpidr_to_vcpu(dev->kvm,mpidr_reg);+if(!vcpu)+return-EINVAL;++id=(attr->attr&KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK);+returnvgic_v3_has_cpu_sysregs_attr(vcpu,0,id,®);+}default:return-ENXIO;}
It looks to me like userspace can then program active priorities with
higher numbers than what it will program num_pri_bits to later. Is that
not weird, or am I missing something?
@@ -240,6 +240,13 @@ void vgic_v3_enable(struct kvm_vcpu *vcpu)vgic_v3->vgic_sre=0;}+vcpu->arch.vgic_cpu.num_id_bits=(kvm_vgic_global_state.ich_vtr_el2&+ICH_VTR_ID_BITS_MASK)>>+ICH_VTR_ID_BITS_SHIFT;+vcpu->arch.vgic_cpu.num_pri_bits=((kvm_vgic_global_state.ich_vtr_el2&+ICH_VTR_PRI_BITS_MASK)>>+ICH_VTR_PRI_BITS_SHIFT)+1;+/* Get the show on the road... */vgic_v3->vgic_hcr=ICH_HCR_EN;}
@@ -340,6 +347,7 @@ int vgic_v3_probe(const struct gic_kvm_info *info)*/kvm_vgic_global_state.nr_lr=(ich_vtr_el2&0xf)+1;kvm_vgic_global_state.can_emulate_gicv2=false;+kvm_vgic_global_state.ich_vtr_el2=ich_vtr_el2;if(!info->vcpu.start){kvm_info("GICv3: no GICV resource entry\n");
@@ -510,6 +510,25 @@ static int vgic_v3_attr_regs_access(struct kvm_device *dev,regid,reg);break;}+caseKVM_DEV_ARM_VGIC_GRP_LEVEL_INFO:{+unsignedintinfo,intid;++info=(attr->attr&KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK)>>+KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT;+if(info==VGIC_LEVEL_INFO_LINE_LEVEL){+if(is_write)+tmp32=*reg;+intid=attr->attr&+KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK;+ret=vgic_v3_line_level_info_uaccess(vcpu,is_write,+intid,&tmp32);+if(!is_write)+*reg=tmp32;
I had a comment here about not having to use the tmp32 by modifying the
line_level_info function, that you seem to have missed.
Hint: The level info is not called from an MMIO path so you should be
able to just write it in a natural way.
quoted hunk
+ } else {
+ ret = -EINVAL;
+ }
+ break;
+ }
default:
ret = -EINVAL;
break;
I think you also missed my comment about having to keep the pending
state in sync with the level state.
Which means you have to set the pending state when the line_level goes
up, and lower it when it goes down unless soft_pending is also set,
assuming it's configured as a level triggered interrupt.
If it's an edge-triggered interrupt, I think you only need to set the
pending state on a line being asserted and the rest should be adjusted
in case the user restores the configuration state to level triggered
later.
Thanks,
-Christoffer
From: Christoffer Dall <hidden> Date: 2016-11-28 19:51:36
Hi Vijaya,
On Wed, Nov 23, 2016 at 06:31:47PM +0530, vijay.kilari at gmail.com wrote:
From: Vijaya Kumar K <redacted>
This patchset adds API for saving and restoring
of VGICv3 registers to support live migration with new vgic feature.
This API definition is as per version of VGICv3 specification
Documentation/virtual/kvm/devices/arm-vgic-v3.txt
The patch 3 & 4 are picked from the Pavel's previous implementation.
http://www.spinics.net/lists/kvm/msg122040.html
NOTE: Only compilation tested for AArch32. No hardware to test.
I did not review the 32-bit part, because if you cannot test it, I don't
think we should merge it.
I just want clarity on what happens if someone tries to use this on
32-bit, and I want to make sure it fails gracefully.
Thanks,
-Christoffer
On Tue, Nov 29, 2016 at 1:09 AM, Christoffer Dall
[off-list ref] wrote:
On Wed, Nov 23, 2016 at 06:31:53PM +0530, vijay.kilari at gmail.com wrote:
quoted
From: Vijaya Kumar K <redacted>
VGICv3 CPU interface registers are accessed using
KVM_DEV_ARM_VGIC_CPU_SYSREGS ioctl. These registers are accessed
as 64-bit. The cpu MPIDR value is passed along with register id.
is used to identify the cpu for registers access.
The VM that supports SEIs expect it on destination machine to handle
guest aborts and hence checked for ICC_CTLR_EL1.SEIS compatibility.
Similarly, VM that supports Affinity Level 3 that is required for AArch64
mode, is required to be supported on destination machine. Hence checked
for ICC_CTLR_EL1.A3V compatibility.
The CPU system register handling is spitted into two files
spitted? Did you mean 'split into' ?
quoted
vgic-sys-reg-common.c and vgic-sys-reg-v3.c.
The vgic-sys-reg-common.c handles read and write of VGIC CPU registers
So this is weird because everything in virt/kvm/arm/ is exactly supposed
to be common between arm and arm64 already.
I would rather that you had a copy of vgic-sys-reg-v3.c in arch/arm/kvm/
and in arch/arm64/kvm/ each taking care of its own architecture.
But note that I didn't actually require that you implemented support for
GICv3 migration on AArch32 hosts for these patches, I just didn't want
thigns to silently break.
If we cannot test the AArch32 implementation, we should potentially just
make sure that is not supported yet, return a proper error to userspace
and get the AArch64 host implementation correct.
I suggest you move your:
virt/kvm/arm/vgic/vgic-sys-reg-v3.c to
arch/arm64/kvm/vgic-sys-reg-v3.c
and rename
virt/kvm/arm/vgic/vgic-sys-reg-common.c to
virt/kvm/arm/vgic/vgic-sys-reg-v3.c
And then wait with the AArch32 host side for now, but just make sure it
compiles and returns an error as opposed to crashing the system if
someone tries to excercise this interface on an AArch32 host.
I will add arch/arm/kvm/vgic-coproc-v3.c (pls check if file name is ok or not?)
and return -ENXIO as shown below and update document accordingly.
int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, bool is_write, u64 id,
u64 *reg)
{
/*
* TODO: Implement for AArch32
*/
return -ENXIO;
}
int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu, bool is_write, u64 id,
u64 *reg)
{
/*
* TODO: Implement for AArch32
*/
return -ENXIO;
}
quoted
for both AArch64 and AArch32 mode. The vgic-sys-reg-v3.c handles AArch64
mode and is compiled only for AArch64 mode.
Updated arch/arm/include/uapi/asm/kvm.h with new definitions
required to compile for AArch32.
The version of VGIC v3 specification is define here
Documentation/virtual/kvm/devices/arm-vgic-v3.txt
Signed-off-by: Pavel Fedin <redacted>
Signed-off-by: Vijaya Kumar K <redacted>
---
[...]
quoted
+static bool access_gic_aprn(struct kvm_vcpu *vcpu, bool is_write, u8 apr,
+ u8 idx, unsigned long *reg)
+{
+ struct vgic_cpu *vgic_v3_cpu = &vcpu->arch.vgic_cpu;
+
+ /* num_pri_bits are initialized with HW supported values.
+ * We can rely safely on num_pri_bits even if VM has not
+ * restored ICC_CTLR_EL1 before restoring APnR registers.
+ */
It looks to me like userspace can then program active priorities with
higher numbers than what it will program num_pri_bits to later. Is that
not weird, or am I missing something?
As long as it is within HW supported priorities it is safe.
@@ -240,6 +240,13 @@ void vgic_v3_enable(struct kvm_vcpu *vcpu)vgic_v3->vgic_sre=0;}+vcpu->arch.vgic_cpu.num_id_bits=(kvm_vgic_global_state.ich_vtr_el2&+ICH_VTR_ID_BITS_MASK)>>+ICH_VTR_ID_BITS_SHIFT;+vcpu->arch.vgic_cpu.num_pri_bits=((kvm_vgic_global_state.ich_vtr_el2&+ICH_VTR_PRI_BITS_MASK)>>+ICH_VTR_PRI_BITS_SHIFT)+1;+/* Get the show on the road... */vgic_v3->vgic_hcr=ICH_HCR_EN;}
@@ -340,6 +347,7 @@ int vgic_v3_probe(const struct gic_kvm_info *info)*/kvm_vgic_global_state.nr_lr=(ich_vtr_el2&0xf)+1;kvm_vgic_global_state.can_emulate_gicv2=false;+kvm_vgic_global_state.ich_vtr_el2=ich_vtr_el2;if(!info->vcpu.start){kvm_info("GICv3: no GICV resource entry\n");
From: Christoffer Dall <hidden> Date: 2016-11-29 08:37:47
On Tue, Nov 29, 2016 at 01:08:26PM +0530, Vijay Kilari wrote:
On Tue, Nov 29, 2016 at 1:09 AM, Christoffer Dall
[off-list ref] wrote:
quoted
On Wed, Nov 23, 2016 at 06:31:53PM +0530, vijay.kilari at gmail.com wrote:
quoted
From: Vijaya Kumar K <redacted>
VGICv3 CPU interface registers are accessed using
KVM_DEV_ARM_VGIC_CPU_SYSREGS ioctl. These registers are accessed
as 64-bit. The cpu MPIDR value is passed along with register id.
is used to identify the cpu for registers access.
The VM that supports SEIs expect it on destination machine to handle
guest aborts and hence checked for ICC_CTLR_EL1.SEIS compatibility.
Similarly, VM that supports Affinity Level 3 that is required for AArch64
mode, is required to be supported on destination machine. Hence checked
for ICC_CTLR_EL1.A3V compatibility.
The CPU system register handling is spitted into two files
spitted? Did you mean 'split into' ?
quoted
vgic-sys-reg-common.c and vgic-sys-reg-v3.c.
The vgic-sys-reg-common.c handles read and write of VGIC CPU registers
So this is weird because everything in virt/kvm/arm/ is exactly supposed
to be common between arm and arm64 already.
I would rather that you had a copy of vgic-sys-reg-v3.c in arch/arm/kvm/
and in arch/arm64/kvm/ each taking care of its own architecture.
But note that I didn't actually require that you implemented support for
GICv3 migration on AArch32 hosts for these patches, I just didn't want
thigns to silently break.
If we cannot test the AArch32 implementation, we should potentially just
make sure that is not supported yet, return a proper error to userspace
and get the AArch64 host implementation correct.
I suggest you move your:
virt/kvm/arm/vgic/vgic-sys-reg-v3.c to
arch/arm64/kvm/vgic-sys-reg-v3.c
and rename
virt/kvm/arm/vgic/vgic-sys-reg-common.c to
virt/kvm/arm/vgic/vgic-sys-reg-v3.c
And then wait with the AArch32 host side for now, but just make sure it
compiles and returns an error as opposed to crashing the system if
someone tries to excercise this interface on an AArch32 host.
I will add arch/arm/kvm/vgic-coproc-v3.c (pls check if file name is ok or not?)
I would call it vgic-v3-coproc.c
and return -ENXIO as shown below and update document accordingly.
int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, bool is_write, u64 id,
u64 *reg)
{
/*
* TODO: Implement for AArch32
*/
return -ENXIO;
}
int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu, bool is_write, u64 id,
u64 *reg)
{
/*
* TODO: Implement for AArch32
*/
return -ENXIO;
}
quoted
quoted
for both AArch64 and AArch32 mode. The vgic-sys-reg-v3.c handles AArch64
mode and is compiled only for AArch64 mode.
Updated arch/arm/include/uapi/asm/kvm.h with new definitions
required to compile for AArch32.
The version of VGIC v3 specification is define here
Documentation/virtual/kvm/devices/arm-vgic-v3.txt
Signed-off-by: Pavel Fedin <redacted>
Signed-off-by: Vijaya Kumar K <redacted>
---
[...]
quoted
quoted
+static bool access_gic_aprn(struct kvm_vcpu *vcpu, bool is_write, u8 apr,
+ u8 idx, unsigned long *reg)
+{
+ struct vgic_cpu *vgic_v3_cpu = &vcpu->arch.vgic_cpu;
+
+ /* num_pri_bits are initialized with HW supported values.
+ * We can rely safely on num_pri_bits even if VM has not
+ * restored ICC_CTLR_EL1 before restoring APnR registers.
+ */
It looks to me like userspace can then program active priorities with
higher numbers than what it will program num_pri_bits to later. Is that
not weird, or am I missing something?
As long as it is within HW supported priorities it is safe.
I know that it is safe on the hardware, but it is weird to define a VM
with some max priority and still be able to set a higher active priority
is it not?
On the other hand, if we cannot enforce this at runtime, it may not
matter?
Hint: I'd like for you to actually think about these constraints and
make sure the sematics of the emulated VM environment remain intact
across migrations.
I don't get these indirections. Why can't you call the functions
directly?
The code is same for accessing the registers hence added this indirection.
That's not answering my question.
What is the benefit of adding this indirection as opposed to having the
functions called directly?
To make my point clear: I hate this kind of preprocessor macro fun, and
I think it should only ever be used when there's a huge benefit in terms
of code reuse or simplicity of some sort. I don't see anything like
that in this case.
Thanks,
-Christoffer
On Tue, Nov 29, 2016 at 2:07 PM, Christoffer Dall
[off-list ref] wrote:
On Tue, Nov 29, 2016 at 01:08:26PM +0530, Vijay Kilari wrote:
quoted
On Tue, Nov 29, 2016 at 1:09 AM, Christoffer Dall
[off-list ref] wrote:
quoted
On Wed, Nov 23, 2016 at 06:31:53PM +0530, vijay.kilari at gmail.com wrote:
quoted
From: Vijaya Kumar K <redacted>
VGICv3 CPU interface registers are accessed using
KVM_DEV_ARM_VGIC_CPU_SYSREGS ioctl. These registers are accessed
as 64-bit. The cpu MPIDR value is passed along with register id.
is used to identify the cpu for registers access.
The VM that supports SEIs expect it on destination machine to handle
guest aborts and hence checked for ICC_CTLR_EL1.SEIS compatibility.
Similarly, VM that supports Affinity Level 3 that is required for AArch64
mode, is required to be supported on destination machine. Hence checked
for ICC_CTLR_EL1.A3V compatibility.
The CPU system register handling is spitted into two files
spitted? Did you mean 'split into' ?
quoted
vgic-sys-reg-common.c and vgic-sys-reg-v3.c.
The vgic-sys-reg-common.c handles read and write of VGIC CPU registers
So this is weird because everything in virt/kvm/arm/ is exactly supposed
to be common between arm and arm64 already.
I would rather that you had a copy of vgic-sys-reg-v3.c in arch/arm/kvm/
and in arch/arm64/kvm/ each taking care of its own architecture.
But note that I didn't actually require that you implemented support for
GICv3 migration on AArch32 hosts for these patches, I just didn't want
thigns to silently break.
If we cannot test the AArch32 implementation, we should potentially just
make sure that is not supported yet, return a proper error to userspace
and get the AArch64 host implementation correct.
I suggest you move your:
virt/kvm/arm/vgic/vgic-sys-reg-v3.c to
arch/arm64/kvm/vgic-sys-reg-v3.c
and rename
virt/kvm/arm/vgic/vgic-sys-reg-common.c to
virt/kvm/arm/vgic/vgic-sys-reg-v3.c
And then wait with the AArch32 host side for now, but just make sure it
compiles and returns an error as opposed to crashing the system if
someone tries to excercise this interface on an AArch32 host.
I will add arch/arm/kvm/vgic-coproc-v3.c (pls check if file name is ok or not?)
I would call it vgic-v3-coproc.c
quoted
and return -ENXIO as shown below and update document accordingly.
int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, bool is_write, u64 id,
u64 *reg)
{
/*
* TODO: Implement for AArch32
*/
return -ENXIO;
}
int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu, bool is_write, u64 id,
u64 *reg)
{
/*
* TODO: Implement for AArch32
*/
return -ENXIO;
}
quoted
quoted
quoted
for both AArch64 and AArch32 mode. The vgic-sys-reg-v3.c handles AArch64
mode and is compiled only for AArch64 mode.
Updated arch/arm/include/uapi/asm/kvm.h with new definitions
required to compile for AArch32.
The version of VGIC v3 specification is define here
Documentation/virtual/kvm/devices/arm-vgic-v3.txt
Signed-off-by: Pavel Fedin <redacted>
Signed-off-by: Vijaya Kumar K <redacted>
---
[...]
quoted
quoted
+static bool access_gic_aprn(struct kvm_vcpu *vcpu, bool is_write, u8 apr,
+ u8 idx, unsigned long *reg)
+{
+ struct vgic_cpu *vgic_v3_cpu = &vcpu->arch.vgic_cpu;
+
+ /* num_pri_bits are initialized with HW supported values.
+ * We can rely safely on num_pri_bits even if VM has not
+ * restored ICC_CTLR_EL1 before restoring APnR registers.
+ */
It looks to me like userspace can then program active priorities with
higher numbers than what it will program num_pri_bits to later. Is that
not weird, or am I missing something?
As long as it is within HW supported priorities it is safe.
I know that it is safe on the hardware, but it is weird to define a VM
with some max priority and still be able to set a higher active priority
is it not?
In that case, we need to cache the highest active priorities updated
by a VM in a variable
when APnR is restored and later check against num_pri_bits when
ICC_CTLR_EL1 is updated.
If the value cached is greater than num_pri_bits restored then reject
ICC_CTLR_EL1 restore.
This variable should be initialized with value 5 ( min priority)
On the other hand, if we cannot enforce this at runtime, it may not
matter?
At VM runtime irrespective of VM's num_pri_bits all the APnR registers that
HW supports are saved and restored.
Hint: I'd like for you to actually think about these constraints and
make sure the sematics of the emulated VM environment remain intact
across migrations.
I don't get these indirections. Why can't you call the functions
directly?
The code is same for accessing the registers hence added this indirection.
That's not answering my question.
What is the benefit of adding this indirection as opposed to having the
functions called directly?
In sys_reg_desc the access function is of type
bool (*access)(struct kvm_vcpu *,
struct sys_reg_params *,
const struct sys_reg_desc *);
Where as the each register access function is of type below to support
access to AArch32(later if not now).
bool access_gic_xxx(struct kvm_vcpu *vcpu, bool is_write, unsigned long *reg);
I can drop this macro and make function calls for each reg access.
To make my point clear: I hate this kind of preprocessor macro fun, and
I think it should only ever be used when there's a huge benefit in terms
of code reuse or simplicity of some sort. I don't see anything like
that in this case.
Thanks,
-Christoffer
From: Christoffer Dall <hidden> Date: 2016-11-29 10:51:03
On Tue, Nov 29, 2016 at 03:31:44PM +0530, Vijay Kilari wrote:
On Tue, Nov 29, 2016 at 2:07 PM, Christoffer Dall
[off-list ref] wrote:
quoted
On Tue, Nov 29, 2016 at 01:08:26PM +0530, Vijay Kilari wrote:
quoted
On Tue, Nov 29, 2016 at 1:09 AM, Christoffer Dall
[off-list ref] wrote:
quoted
On Wed, Nov 23, 2016 at 06:31:53PM +0530, vijay.kilari at gmail.com wrote:
quoted
From: Vijaya Kumar K <redacted>
VGICv3 CPU interface registers are accessed using
KVM_DEV_ARM_VGIC_CPU_SYSREGS ioctl. These registers are accessed
as 64-bit. The cpu MPIDR value is passed along with register id.
is used to identify the cpu for registers access.
The VM that supports SEIs expect it on destination machine to handle
guest aborts and hence checked for ICC_CTLR_EL1.SEIS compatibility.
Similarly, VM that supports Affinity Level 3 that is required for AArch64
mode, is required to be supported on destination machine. Hence checked
for ICC_CTLR_EL1.A3V compatibility.
The CPU system register handling is spitted into two files
spitted? Did you mean 'split into' ?
quoted
vgic-sys-reg-common.c and vgic-sys-reg-v3.c.
The vgic-sys-reg-common.c handles read and write of VGIC CPU registers
So this is weird because everything in virt/kvm/arm/ is exactly supposed
to be common between arm and arm64 already.
I would rather that you had a copy of vgic-sys-reg-v3.c in arch/arm/kvm/
and in arch/arm64/kvm/ each taking care of its own architecture.
But note that I didn't actually require that you implemented support for
GICv3 migration on AArch32 hosts for these patches, I just didn't want
thigns to silently break.
If we cannot test the AArch32 implementation, we should potentially just
make sure that is not supported yet, return a proper error to userspace
and get the AArch64 host implementation correct.
I suggest you move your:
virt/kvm/arm/vgic/vgic-sys-reg-v3.c to
arch/arm64/kvm/vgic-sys-reg-v3.c
and rename
virt/kvm/arm/vgic/vgic-sys-reg-common.c to
virt/kvm/arm/vgic/vgic-sys-reg-v3.c
And then wait with the AArch32 host side for now, but just make sure it
compiles and returns an error as opposed to crashing the system if
someone tries to excercise this interface on an AArch32 host.
I will add arch/arm/kvm/vgic-coproc-v3.c (pls check if file name is ok or not?)
I would call it vgic-v3-coproc.c
quoted
and return -ENXIO as shown below and update document accordingly.
int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, bool is_write, u64 id,
u64 *reg)
{
/*
* TODO: Implement for AArch32
*/
return -ENXIO;
}
int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu, bool is_write, u64 id,
u64 *reg)
{
/*
* TODO: Implement for AArch32
*/
return -ENXIO;
}
quoted
quoted
quoted
for both AArch64 and AArch32 mode. The vgic-sys-reg-v3.c handles AArch64
mode and is compiled only for AArch64 mode.
Updated arch/arm/include/uapi/asm/kvm.h with new definitions
required to compile for AArch32.
The version of VGIC v3 specification is define here
Documentation/virtual/kvm/devices/arm-vgic-v3.txt
Signed-off-by: Pavel Fedin <redacted>
Signed-off-by: Vijaya Kumar K <redacted>
---
[...]
quoted
quoted
+static bool access_gic_aprn(struct kvm_vcpu *vcpu, bool is_write, u8 apr,
+ u8 idx, unsigned long *reg)
+{
+ struct vgic_cpu *vgic_v3_cpu = &vcpu->arch.vgic_cpu;
+
+ /* num_pri_bits are initialized with HW supported values.
+ * We can rely safely on num_pri_bits even if VM has not
+ * restored ICC_CTLR_EL1 before restoring APnR registers.
+ */
It looks to me like userspace can then program active priorities with
higher numbers than what it will program num_pri_bits to later. Is that
not weird, or am I missing something?
As long as it is within HW supported priorities it is safe.
I know that it is safe on the hardware, but it is weird to define a VM
with some max priority and still be able to set a higher active priority
is it not?
In that case, we need to cache the highest active priorities updated
by a VM in a variable
when APnR is restored and later check against num_pri_bits when
ICC_CTLR_EL1 is updated.
If the value cached is greater than num_pri_bits restored then reject
ICC_CTLR_EL1 restore.
This variable should be initialized with value 5 ( min priority)
quoted
On the other hand, if we cannot enforce this at runtime, it may not
matter?
At VM runtime irrespective of VM's num_pri_bits all the APnR registers that
HW supports are saved and restored.
Yes, never mind my comment. Since we cannot enforce this constraint
once the VM runs, I don't think there's any concern here.
I don't get these indirections. Why can't you call the functions
directly?
The code is same for accessing the registers hence added this indirection.
That's not answering my question.
What is the benefit of adding this indirection as opposed to having the
functions called directly?
In sys_reg_desc the access function is of type
bool (*access)(struct kvm_vcpu *,
struct sys_reg_params *,
const struct sys_reg_desc *);
Where as the each register access function is of type below to support
access to AArch32(later if not now).
bool access_gic_xxx(struct kvm_vcpu *vcpu, bool is_write, unsigned long *reg);
I can drop this macro and make function calls for each reg access.
Please don't worry about the 32-bit side until we actually implement
that. And once we do, we can move things around in patches to support
the 32-bit side so that it makes sense to the reader.
So, for now, just have this one file, moved to arch/arm64/kvm/ where all
the access functions are static in this file and called directly from
the single dispatch function.
Thanks,
-Christoffer
@@ -510,6 +510,25 @@ static int vgic_v3_attr_regs_access(struct kvm_device *dev,regid,reg);break;}+caseKVM_DEV_ARM_VGIC_GRP_LEVEL_INFO:{+unsignedintinfo,intid;++info=(attr->attr&KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK)>>+KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT;+if(info==VGIC_LEVEL_INFO_LINE_LEVEL){+if(is_write)+tmp32=*reg;+intid=attr->attr&+KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK;+ret=vgic_v3_line_level_info_uaccess(vcpu,is_write,+intid,&tmp32);+if(!is_write)+*reg=tmp32;
I had a comment here about not having to use the tmp32 by modifying the
line_level_info function, that you seem to have missed.
Hint: The level info is not called from an MMIO path so you should be
able to just write it in a natural way.
Ok. Changed the prototype of vgic_v3_line_level_info_uaccess() to take
u64 reg instead of tmp32
quoted
+ } else {
+ ret = -EINVAL;
+ }
+ break;
+ }
default:
ret = -EINVAL;
break;
I think you also missed my comment about having to keep the pending
state in sync with the level state.
Which means you have to set the pending state when the line_level goes
up, and lower it when it goes down unless soft_pending is also set,
assuming it's configured as a level triggered interrupt.
If it's an edge-triggered interrupt, I think you only need to set the
pending state on a line being asserted and the rest should be adjusted
in case the user restores the configuration state to level triggered
later.
Is this ok?
void vgic_write_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid,
const u64 val)
{
int i;
for (i = 0; i < 32; i++) {
struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
spin_lock(&irq->irq_lock);
if (val & (1U << i)) {
irq->line_level = true;
irq->pending = true;
vgic_queue_irq_unlock(vcpu->kvm, irq);
} else {
if (irq->config == VGIC_CONFIG_EDGE ||
(irq->config == VGIC_CONFIG_LEVEL &&
!irq->soft_pending))
irq->line_level = false;
spin_unlock(&irq->irq_lock);
}
vgic_put_irq(vcpu->kvm, irq);
}
}
@@ -510,6 +510,25 @@ static int vgic_v3_attr_regs_access(struct kvm_device *dev,regid,reg);break;}+caseKVM_DEV_ARM_VGIC_GRP_LEVEL_INFO:{+unsignedintinfo,intid;++info=(attr->attr&KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK)>>+KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT;+if(info==VGIC_LEVEL_INFO_LINE_LEVEL){+if(is_write)+tmp32=*reg;+intid=attr->attr&+KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK;+ret=vgic_v3_line_level_info_uaccess(vcpu,is_write,+intid,&tmp32);+if(!is_write)+*reg=tmp32;
I had a comment here about not having to use the tmp32 by modifying the
line_level_info function, that you seem to have missed.
Hint: The level info is not called from an MMIO path so you should be
able to just write it in a natural way.
Ok. Changed the prototype of vgic_v3_line_level_info_uaccess() to take
u64 reg instead of tmp32
quoted
quoted
+ } else {
+ ret = -EINVAL;
+ }
+ break;
+ }
default:
ret = -EINVAL;
break;
I think you also missed my comment about having to keep the pending
state in sync with the level state.
Which means you have to set the pending state when the line_level goes
up, and lower it when it goes down unless soft_pending is also set,
assuming it's configured as a level triggered interrupt.
If it's an edge-triggered interrupt, I think you only need to set the
pending state on a line being asserted and the rest should be adjusted
in case the user restores the configuration state to level triggered
later.
Is this ok?
void vgic_write_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid,
const u64 val)
{
int i;
for (i = 0; i < 32; i++) {
struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
spin_lock(&irq->irq_lock);
if (val & (1U << i)) {
irq->line_level = true;
irq->pending = true;
vgic_queue_irq_unlock(vcpu->kvm, irq);
Actually, I'm not sure what the semantics of the line level ioctl should
be for edge-triggered interrupts? My inclination is that it shouldn't
have any effect at this point, but that would mean that at this point we
should only set the pending variable and try to queue the interrupt if
the config is level. But that also means that when we set the config
later, we need to try to queue the interrupt, which we don't do
currently, because we rely on the guest not fiddling with the config of
an enabled interrupt.
Could it be considered an error if user space tries to set the level for
an edge-triggered interrupt and therefore something we can just ignore
and assume that the corresponing interrupt will be configured as a
level-triggered one later?
Marc/Peter, thoughts on this one?
In any case we probably need to clarify the ABI in terms of this
particular KVM_DEV_AR_VGIC_GRP_LEVEL_INFO group and how it relates to
the config of edge vs. level of interrupts and ordering on restore...
Thanks,
-Christoffer
From: Peter Maydell <hidden> Date: 2016-11-30 07:10:51
On 29 November 2016 at 21:09, Christoffer Dall
[off-list ref] wrote:
Actually, I'm not sure what the semantics of the line level ioctl should
be for edge-triggered interrupts? My inclination is that it shouldn't
have any effect at this point, but that would mean that at this point we
should only set the pending variable and try to queue the interrupt if
the config is level. But that also means that when we set the config
later, we need to try to queue the interrupt, which we don't do
currently, because we rely on the guest not fiddling with the config of
an enabled interrupt.
Could it be considered an error if user space tries to set the level for
an edge-triggered interrupt and therefore something we can just ignore
and assume that the corresponing interrupt will be configured as a
level-triggered one later?
Userspace will always read the line-level values out and write
them back for migration, and I'd rather not make it have to
do cross-checks against whether the interrupt is edge or level
triggered to see whether it should write the level values into
the kernel. Telling the kernel the level for an edge-triggered
interrupt should be a no-op because it doesn't have any effect
on pending status.
In any case we probably need to clarify the ABI in terms of this
particular KVM_DEV_AR_VGIC_GRP_LEVEL_INFO group and how it relates to
the config of edge vs. level of interrupts and ordering on restore...
IIRC the QEMU code restores the config first. (There's a similar
ordering thing for GICv2 where we have to restore GICD_ICFGRn before
GICD_ISPENDRn.)
thanks
-- PMM
From: Christoffer Dall <hidden> Date: 2016-11-30 08:24:11
On Wed, Nov 30, 2016 at 07:10:51AM +0000, Peter Maydell wrote:
On 29 November 2016 at 21:09, Christoffer Dall
[off-list ref] wrote:
quoted
Actually, I'm not sure what the semantics of the line level ioctl should
be for edge-triggered interrupts? My inclination is that it shouldn't
have any effect at this point, but that would mean that at this point we
should only set the pending variable and try to queue the interrupt if
the config is level. But that also means that when we set the config
later, we need to try to queue the interrupt, which we don't do
currently, because we rely on the guest not fiddling with the config of
an enabled interrupt.
Could it be considered an error if user space tries to set the level for
an edge-triggered interrupt and therefore something we can just ignore
and assume that the corresponing interrupt will be configured as a
level-triggered one later?
Userspace will always read the line-level values out and write
them back for migration, and I'd rather not make it have to
do cross-checks against whether the interrupt is edge or level
triggered to see whether it should write the level values into
the kernel. Telling the kernel the level for an edge-triggered
interrupt should be a no-op because it doesn't have any effect
on pending status.
quoted
In any case we probably need to clarify the ABI in terms of this
particular KVM_DEV_AR_VGIC_GRP_LEVEL_INFO group and how it relates to
the config of edge vs. level of interrupts and ordering on restore...
IIRC the QEMU code restores the config first. (There's a similar
ordering thing for GICv2 where we have to restore GICD_ICFGRn before
GICD_ISPENDRn.)
So it sounds to me that we should add a note in the Documentation like
this:
@@ -193,6 +193,11 @@ Groups: Bit[n] indicates the status for interrupt vINTID + n.+ Getting or setting the level info for an edge-triggered interrupt is+ not guaranteed to work. To restore the complete state of the VGIC, the+ configuration (edge/level) of interrupts must be restored before+ restoring the level.+ SGIs and any interrupt with a higher ID than the number of interrupts supported, will be RAZ/WI. LPIs are always edge-triggered and are therefore not supported by this interface.
Vijay, this means that the first block in your if-statement should only
set pending and queue the interrupt if the interrupt is a level
triggered one.
(Peter, I thought you once argued that it was important for user space
to be able to save/restore the state without any ordering requirements,
but I may have misunderstood. It is still the option to add something
like the above to the docs but also do our best to allow any ordering of
level/config, but it becomes slightly more invasive.)
Thanks,
-Christoffer
From: Peter Maydell <hidden> Date: 2016-11-30 08:29:11
On 30 November 2016 at 08:24, Christoffer Dall
[off-list ref] wrote:
(Peter, I thought you once argued that it was important for user space
to be able to save/restore the state without any ordering requirements,
but I may have misunderstood. It is still the option to add something
like the above to the docs but also do our best to allow any ordering of
level/config, but it becomes slightly more invasive.)
Hmm; perhaps I should think about this a bit more.
-- PMM
On Wed, Nov 23, 2016 at 06:31:48PM +0530, vijay.kilari at gmail.com wrote:
quoted
From: Vijaya Kumar K <redacted>
Read and write of some registers like ISPENDR and ICPENDR
from userspace requires special handling when compared to
guest access for these registers.
Refer to Documentation/virtual/kvm/devices/arm-vgic-v3.txt
for handling of ISPENDR, ICPENDR registers handling.
Add infrastructure to support guest and userspace read
and write for the required registers
Also moved vgic_uaccess from vgic-mmio-v2.c to vgic-mmio.c
Signed-off-by: Vijaya Kumar K <redacted>
---
virt/kvm/arm/vgic/vgic-mmio-v2.c | 25 ----------
virt/kvm/arm/vgic/vgic-mmio-v3.c | 102 ++++++++++++++++++++++++++++++++-------
virt/kvm/arm/vgic/vgic-mmio.c | 78 +++++++++++++++++++++++++++---
virt/kvm/arm/vgic/vgic-mmio.h | 19 ++++++++
4 files changed, 175 insertions(+), 49 deletions(-)
@@ -207,6 +207,66 @@ static unsigned long vgic_mmio_read_v3_idregs(struct kvm_vcpu *vcpu,return0;}+staticunsignedlongvgic_v3_uaccess_read_pending(structkvm_vcpu*vcpu,+gpa_taddr,unsignedintlen)+{+u32intid=VGIC_ADDR_TO_INTID(addr,1);+u32value=0;+inti;++/*+*Aleveltriggerredinterruptpendingstateislatchedinboth+*"soft_pending"and"line_level"variables.Userspacewillsave+*andrestoresoft_pendingandline_levelseparately.+*RefertoDocumentation/virtual/kvm/devices/arm-vgic-v3.txt+*handlingofISPENDRandICPENDR.+*/+for(i=0;i<len*8;i++){+structvgic_irq*irq=vgic_get_irq(vcpu->kvm,vcpu,intid+i);++if(irq->config==VGIC_CONFIG_LEVEL&&irq->soft_pending)+value|=(1U<<i);+if(irq->config==VGIC_CONFIG_EDGE&&irq->pending)+value|=(1U<<i);++vgic_put_irq(vcpu->kvm,irq);+}++returnvalue;+}++staticvoidvgic_v3_uaccess_write_pending(structkvm_vcpu*vcpu,+gpa_taddr,unsignedintlen,+unsignedlongval)+{+u32intid=VGIC_ADDR_TO_INTID(addr,1);+inti;++for(i=0;i<len*8;i++){+structvgic_irq*irq=vgic_get_irq(vcpu->kvm,vcpu,intid+i);++spin_lock(&irq->irq_lock);+if(test_bit(i,&val)){+/* soft_pending is set irrespective of irq type+*(leveloredge)toavoiddependencythatVMshould+*restoreirqconfigbeforependinginfo.+*/
@@ -356,7 +416,7 @@ static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu, * We take some special care here to fix the calculation of the register * offset. */-#define REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(off, rd, wr, bpi, acc) \+#define REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(off, rd, wr, ur, uw, bpi, acc) \ { \ .reg_offset = off, \ .bits_per_irq = bpi, \
do we really want to return 0 here? -ENXIO?
I see that dispatch_mmio_read/write return 0 in that case but I don't
see any reason either? Other kvm_io_device_ops seem to return
-EOPNOTSUPP in such a case.
Thanks
Eric
On Wed, Nov 23, 2016 at 06:31:49PM +0530, vijay.kilari at gmail.com wrote:
quoted
From: Vijaya Kumar K <redacted>
VGICv3 Distributor and Redistributor registers are accessed using
KVM_DEV_ARM_VGIC_GRP_DIST_REGS and KVM_DEV_ARM_VGIC_GRP_REDIST_REGS
with KVM_SET_DEVICE_ATTR and KVM_GET_DEVICE_ATTR ioctls.
These registers are accessed as 32-bit and cpu mpidr
value passed along with register offset is used to identify the
cpu for redistributor registers access.
The version of VGIC v3 specification is define here
Couldn't you set
struct vgic_io_device dev = {
.regions = vgic_v3_dist_registers,
.nr_regions = ARRAY_SIZE(vgic_v3_dist_registers),
};
and reuse:
vgic_get_mmio_region(struct kvm_vcpu *vcpu, struct vgic_io_device
*iodev, gpa_t addr, int len)?
In such a case is vgic_validate_mmio_region_addr() still mandated?
quoted
+ break;
+ case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:{
+ regions = vgic_v3_rdbase_registers;
+ nr_regions = ARRAY_SIZE(vgic_v3_rdbase_registers);
+ break;
+ }
+ default:
+ return -ENXIO;
+ }
+
+ /* We only support aligned 32-bit accesses. */
+ if (addr & 3)
+ return -ENXIO;
+
+ return vgic_validate_mmio_region_addr(dev, regions, nr_regions, addr);
+}
/*
* Compare a given affinity (level 1-3 and a level 0 mask, from the SGI
* generation register ICC_SGI1R_EL1) with a given VCPU.
@@ -394,6 +394,28 @@ static int match_region(const void *key, const void *elt)sizeof(region[0]),match_region);}+/* Check if address falls within the region */+intvgic_validate_mmio_region_addr(structkvm_device*dev,+conststructvgic_register_region*regions,+intnr_regions,gpa_taddr)+{+inti,len;+intnr_irqs=dev->kvm->arch.vgic.nr_spis+VGIC_NR_PRIVATE_IRQS;++for(i=0;i<nr_regions;i++){+if(regions[i].bits_per_irq)+len=(regions[i].bits_per_irq*nr_irqs)/8;+else+len=regions[i].len;++if(regions[i].reg_offset<=addr&&+regions[i].reg_offset+len>addr)+return0;+}++return-ENXIO;+}+/**kvm_mmio_read_buf()returnsavalueinaformatwhereitcanbeconverted*toabytearrayandbedirectlyobservedastheguestwantedittoappear
Hi Vijay,
On 23/11/2016 14:01, vijay.kilari at gmail.com wrote:
quoted hunk
From: Vijaya Kumar K <redacted>
In order to implement vGICv3 CPU interface access, we will need to perform
table lookup of system registers. We would need both index_to_params() and
find_reg() exported for that purpose, but instead we export a single
function which combines them both.
Signed-off-by: Pavel Fedin <redacted>
Signed-off-by: Vijaya Kumar K <redacted>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Acked-by: Christoffer Dall <redacted>
---
arch/arm64/kvm/sys_regs.c | 22 +++++++++++++++-------
arch/arm64/kvm/sys_regs.h | 4 ++++
2 files changed, 19 insertions(+), 7 deletions(-)
Can't you use find_reg_by_id in index_to_sys_reg_desc too?
Besides Reviewed-by: Eric Auger [off-list ref]
Thanks
Eric
quoted hunk
/* Decode an index value, and find the sys_reg_desc entry. */
static const struct sys_reg_desc *index_to_sys_reg_desc(struct kvm_vcpu *vcpu,
u64 id)
@@ -1912,10 +1923,8 @@ static int get_invariant_sys_reg(u64 id, void __user *uaddr) struct sys_reg_params params; const struct sys_reg_desc *r;- if (!index_to_params(id, ¶ms))- return -ENOENT;-- r = find_reg(¶ms, invariant_sys_regs, ARRAY_SIZE(invariant_sys_regs));+ r = find_reg_by_id(id, ¶ms, invariant_sys_regs,+ ARRAY_SIZE(invariant_sys_regs)); if (!r) return -ENOENT;
@@ -1929,9 +1938,8 @@ static int set_invariant_sys_reg(u64 id, void __user *uaddr) int err; u64 val = 0; /* Make sure high bits are 0 for 32-bit regs */- if (!index_to_params(id, ¶ms))- return -ENOENT;- r = find_reg(¶ms, invariant_sys_regs, ARRAY_SIZE(invariant_sys_regs));+ r = find_reg_by_id(id, ¶ms, invariant_sys_regs,+ ARRAY_SIZE(invariant_sys_regs)); if (!r) return -ENOENT;
From: Christoffer Dall <hidden> Date: 2016-12-06 14:30:48
On Tue, Dec 06, 2016 at 12:42:09PM +0100, Auger Eric wrote:
Hi,
On 28/11/2016 14:05, Christoffer Dall wrote:
quoted
On Wed, Nov 23, 2016 at 06:31:48PM +0530, vijay.kilari at gmail.com wrote:
quoted
From: Vijaya Kumar K <redacted>
Read and write of some registers like ISPENDR and ICPENDR
from userspace requires special handling when compared to
guest access for these registers.
Refer to Documentation/virtual/kvm/devices/arm-vgic-v3.txt
for handling of ISPENDR, ICPENDR registers handling.
Add infrastructure to support guest and userspace read
and write for the required registers
Also moved vgic_uaccess from vgic-mmio-v2.c to vgic-mmio.c
Signed-off-by: Vijaya Kumar K <redacted>
---
virt/kvm/arm/vgic/vgic-mmio-v2.c | 25 ----------
virt/kvm/arm/vgic/vgic-mmio-v3.c | 102 ++++++++++++++++++++++++++++++++-------
virt/kvm/arm/vgic/vgic-mmio.c | 78 +++++++++++++++++++++++++++---
virt/kvm/arm/vgic/vgic-mmio.h | 19 ++++++++
4 files changed, 175 insertions(+), 49 deletions(-)
@@ -207,6 +207,66 @@ static unsigned long vgic_mmio_read_v3_idregs(struct kvm_vcpu *vcpu,return0;}+staticunsignedlongvgic_v3_uaccess_read_pending(structkvm_vcpu*vcpu,+gpa_taddr,unsignedintlen)+{+u32intid=VGIC_ADDR_TO_INTID(addr,1);+u32value=0;+inti;++/*+*Aleveltriggerredinterruptpendingstateislatchedinboth+*"soft_pending"and"line_level"variables.Userspacewillsave+*andrestoresoft_pendingandline_levelseparately.+*RefertoDocumentation/virtual/kvm/devices/arm-vgic-v3.txt+*handlingofISPENDRandICPENDR.+*/+for(i=0;i<len*8;i++){+structvgic_irq*irq=vgic_get_irq(vcpu->kvm,vcpu,intid+i);++if(irq->config==VGIC_CONFIG_LEVEL&&irq->soft_pending)+value|=(1U<<i);+if(irq->config==VGIC_CONFIG_EDGE&&irq->pending)+value|=(1U<<i);++vgic_put_irq(vcpu->kvm,irq);+}++returnvalue;+}++staticvoidvgic_v3_uaccess_write_pending(structkvm_vcpu*vcpu,+gpa_taddr,unsignedintlen,+unsignedlongval)+{+u32intid=VGIC_ADDR_TO_INTID(addr,1);+inti;++for(i=0;i<len*8;i++){+structvgic_irq*irq=vgic_get_irq(vcpu->kvm,vcpu,intid+i);++spin_lock(&irq->irq_lock);+if(test_bit(i,&val)){+/* soft_pending is set irrespective of irq type+*(leveloredge)toavoiddependencythatVMshould+*restoreirqconfigbeforependinginfo.+*/
I am confused by the comment above. Since we test the irq config here
don't we assume the config was restored before the pending state?
THe idea here was that if you're setting the value, then if you later
set the config to level, then the soft_pending state has already been
set properly, and if the irq stays an edge, then it doesn't matter what
that field is.
If you read the value, the assumption is that you're reading a
consistently configured GIC, if not, then all bets are off anyhow.
Does that make sense?
Perhaps only the comment should be adjusted or do you think the logic is
flawed?
Thanks,
-Christoffer
Hi Vijay,
On 28/11/2016 15:28, Christoffer Dall wrote:
On Wed, Nov 23, 2016 at 06:31:52PM +0530, vijay.kilari at gmail.com wrote:
quoted
From: Vijaya Kumar K <redacted>
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 <redacted>
---
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 | 22 ++++++++++++++++++++--
virt/kvm/arm/vgic/vgic.h | 5 +++++
5 files changed, 41 insertions(+), 20 deletions(-)
@@ -212,22 +212,6 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu *vcpu,}}-staticvoidvgic_set_vmcr(structkvm_vcpu*vcpu,structvgic_vmcr*vmcr)-{-if(kvm_vgic_global_state.type==VGIC_V2)-vgic_v2_set_vmcr(vcpu,vmcr);-else-vgic_v3_set_vmcr(vcpu,vmcr);-}--staticvoidvgic_get_vmcr(structkvm_vcpu*vcpu,structvgic_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. */
I am not able to understand why we use ICC_CTLR _*macros here? Please
could you explain it to me? Besides if we want to ignore the FIQen bit
can't we change the ICH_VMCR_CTLR_MASK value?
Thanks
Eric
Nit: I think this can be written more nicely as:
vmcr = ((vmcrp->ctlr >> ICC_CTLR_EL1_EOImode_SHIFT)
<< ICH_VMCR_EOIM_SHIFT) & ICH_VMCR_EOIM_MASK;
@@ -78,6 +78,9 @@ struct vgic_vmcr {u32abpr;u32bpr;u32pmr;+/* Below member variable are valid only for GICv3 */+u32grpen0;+u32grpen1;};structvgic_irq*vgic_get_irq(structkvm*kvm,structkvm_vcpu*vcpu,
@@ -138,6 +141,8 @@ int vgic_v3_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write,intvgic_v3_redist_uaccess(structkvm_vcpu*vcpu,boolis_write,intoffset,u32*val);intkvm_register_vgic_device(unsignedlongtype);+voidvgic_set_vmcr(structkvm_vcpu*vcpu,structvgic_vmcr*vmcr);+voidvgic_get_vmcr(structkvm_vcpu*vcpu,structvgic_vmcr*vmcr);intvgic_lazy_init(structkvm*kvm);intvgic_init(structkvm*kvm);
--
1.9.1
My comments on style above notwithstanding:
Reviewed-by: Christoffer Dall <redacted>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Christoffer Dall <hidden> Date: 2016-12-08 12:21:15
On Thu, Dec 08, 2016 at 12:52:39PM +0100, Auger Eric wrote:
Hi Vijay,
On 28/11/2016 15:28, Christoffer Dall wrote:
quoted
On Wed, Nov 23, 2016 at 06:31:52PM +0530, vijay.kilari at gmail.com wrote:
quoted
From: Vijaya Kumar K <redacted>
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 <redacted>
---
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 | 22 ++++++++++++++++++++--
virt/kvm/arm/vgic/vgic.h | 5 +++++
5 files changed, 41 insertions(+), 20 deletions(-)
@@ -212,22 +212,6 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu *vcpu,}}-staticvoidvgic_set_vmcr(structkvm_vcpu*vcpu,structvgic_vmcr*vmcr)-{-if(kvm_vgic_global_state.type==VGIC_V2)-vgic_v2_set_vmcr(vcpu,vmcr);-else-vgic_v3_set_vmcr(vcpu,vmcr);-}--staticvoidvgic_get_vmcr(structkvm_vcpu*vcpu,structvgic_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. */
I am not able to understand why we use ICC_CTLR _*macros here? Please
could you explain it to me? Besides if we want to ignore the FIQen bit
can't we change the ICH_VMCR_CTLR_MASK value?
This first statement is setting the vmcr to the ctlr's bit[1], but
placed in bit[0], and then the next statement is moving that bit value
to the corresponding place in the vmcr. I think this is correct
(although a little opaque).
There's also a newer series on the list, just so you know.
Thanks,
-Christoffer
Hi Christoffer,
On 08/12/2016 13:21, Christoffer Dall wrote:
On Thu, Dec 08, 2016 at 12:52:39PM +0100, Auger Eric wrote:
quoted
Hi Vijay,
On 28/11/2016 15:28, Christoffer Dall wrote:
quoted
On Wed, Nov 23, 2016 at 06:31:52PM +0530, vijay.kilari at gmail.com wrote:
quoted
From: Vijaya Kumar K <redacted>
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 <redacted>
---
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 | 22 ++++++++++++++++++++--
virt/kvm/arm/vgic/vgic.h | 5 +++++
5 files changed, 41 insertions(+), 20 deletions(-)
@@ -212,22 +212,6 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu *vcpu,}}-staticvoidvgic_set_vmcr(structkvm_vcpu*vcpu,structvgic_vmcr*vmcr)-{-if(kvm_vgic_global_state.type==VGIC_V2)-vgic_v2_set_vmcr(vcpu,vmcr);-else-vgic_v3_set_vmcr(vcpu,vmcr);-}--staticvoidvgic_get_vmcr(structkvm_vcpu*vcpu,structvgic_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. */
I am not able to understand why we use ICC_CTLR _*macros here? Please
could you explain it to me? Besides if we want to ignore the FIQen bit
can't we change the ICH_VMCR_CTLR_MASK value?
This first statement is setting the vmcr to the ctlr's bit[1], but
placed in bit[0], and then the next statement is moving that bit value
to the corresponding place in the vmcr. I think this is correct
(although a little opaque).
Yes the question was more about the semantic of the vmcrp->ctlr field. I
thought it was supposed to store ICH_VMCR_EL2 ctrl bits as they are and
not with a different layout.
There's also a newer series on the list, just so you know.
Argh OK I missed it. I will check the diffs in the AArch64 related patches.
Thanks
Eric
From: Christoffer Dall <hidden> Date: 2016-12-11 16:38:03
On Thu, Dec 08, 2016 at 01:50:43PM +0100, Auger Eric wrote:
Hi Christoffer,
On 08/12/2016 13:21, Christoffer Dall wrote:
quoted
On Thu, Dec 08, 2016 at 12:52:39PM +0100, Auger Eric wrote:
quoted
Hi Vijay,
On 28/11/2016 15:28, Christoffer Dall wrote:
quoted
On Wed, Nov 23, 2016 at 06:31:52PM +0530, vijay.kilari at gmail.com wrote:
quoted
From: Vijaya Kumar K <redacted>
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 <redacted>
---
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 | 22 ++++++++++++++++++++--
virt/kvm/arm/vgic/vgic.h | 5 +++++
5 files changed, 41 insertions(+), 20 deletions(-)
@@ -212,22 +212,6 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu *vcpu,}}-staticvoidvgic_set_vmcr(structkvm_vcpu*vcpu,structvgic_vmcr*vmcr)-{-if(kvm_vgic_global_state.type==VGIC_V2)-vgic_v2_set_vmcr(vcpu,vmcr);-else-vgic_v3_set_vmcr(vcpu,vmcr);-}--staticvoidvgic_get_vmcr(structkvm_vcpu*vcpu,structvgic_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. */
I am not able to understand why we use ICC_CTLR _*macros here? Please
could you explain it to me? Besides if we want to ignore the FIQen bit
can't we change the ICH_VMCR_CTLR_MASK value?
This first statement is setting the vmcr to the ctlr's bit[1], but
placed in bit[0], and then the next statement is moving that bit value
to the corresponding place in the vmcr. I think this is correct
(although a little opaque).
Yes the question was more about the semantic of the vmcrp->ctlr field. I
thought it was supposed to store ICH_VMCR_EL2 ctrl bits as they are and
not with a different layout.
my understanding is that vmcrp->ctlr is a representation of the
GICC_CTLR field and its bit layout (based on the existin v2 code, unless
I misread it) and the vmcr is the vmcr field.
I don't care much either way, as long as it's clear what the semantics
are. One problem with storing parts of the VMCR in the vmcrp->ctlr
field is that I don't think there's an architectural definition of the
concept 'ICH_VMCR_EL2 ctrl bits'.
Thanks,
-Christoffer
On Tue, Dec 6, 2016 at 5:12 PM, Auger Eric [off-list ref] wrote:
Hi,
On 28/11/2016 14:05, Christoffer Dall wrote:
quoted
On Wed, Nov 23, 2016 at 06:31:48PM +0530, vijay.kilari at gmail.com wrote:
quoted
From: Vijaya Kumar K <redacted>
Read and write of some registers like ISPENDR and ICPENDR
from userspace requires special handling when compared to
guest access for these registers.
Refer to Documentation/virtual/kvm/devices/arm-vgic-v3.txt
for handling of ISPENDR, ICPENDR registers handling.
Add infrastructure to support guest and userspace read
and write for the required registers
Also moved vgic_uaccess from vgic-mmio-v2.c to vgic-mmio.c
Signed-off-by: Vijaya Kumar K <redacted>
---
virt/kvm/arm/vgic/vgic-mmio-v2.c | 25 ----------
virt/kvm/arm/vgic/vgic-mmio-v3.c | 102 ++++++++++++++++++++++++++++++++-------
virt/kvm/arm/vgic/vgic-mmio.c | 78 +++++++++++++++++++++++++++---
virt/kvm/arm/vgic/vgic-mmio.h | 19 ++++++++
4 files changed, 175 insertions(+), 49 deletions(-)
@@ -207,6 +207,66 @@ static unsigned long vgic_mmio_read_v3_idregs(struct kvm_vcpu *vcpu,return0;}+staticunsignedlongvgic_v3_uaccess_read_pending(structkvm_vcpu*vcpu,+gpa_taddr,unsignedintlen)+{+u32intid=VGIC_ADDR_TO_INTID(addr,1);+u32value=0;+inti;++/*+*Aleveltriggerredinterruptpendingstateislatchedinboth+*"soft_pending"and"line_level"variables.Userspacewillsave+*andrestoresoft_pendingandline_levelseparately.+*RefertoDocumentation/virtual/kvm/devices/arm-vgic-v3.txt+*handlingofISPENDRandICPENDR.+*/+for(i=0;i<len*8;i++){+structvgic_irq*irq=vgic_get_irq(vcpu->kvm,vcpu,intid+i);++if(irq->config==VGIC_CONFIG_LEVEL&&irq->soft_pending)+value|=(1U<<i);+if(irq->config==VGIC_CONFIG_EDGE&&irq->pending)+value|=(1U<<i);++vgic_put_irq(vcpu->kvm,irq);+}++returnvalue;+}++staticvoidvgic_v3_uaccess_write_pending(structkvm_vcpu*vcpu,+gpa_taddr,unsignedintlen,+unsignedlongval)+{+u32intid=VGIC_ADDR_TO_INTID(addr,1);+inti;++for(i=0;i<len*8;i++){+structvgic_irq*irq=vgic_get_irq(vcpu->kvm,vcpu,intid+i);++spin_lock(&irq->irq_lock);+if(test_bit(i,&val)){+/* soft_pending is set irrespective of irq type+*(leveloredge)toavoiddependencythatVMshould+*restoreirqconfigbeforependinginfo.+*/
@@ -356,7 +416,7 @@ static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu, * We take some special care here to fix the calculation of the register * offset. */-#define REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(off, rd, wr, bpi, acc) \+#define REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(off, rd, wr, ur, uw, bpi, acc) \ { \ .reg_offset = off, \ .bits_per_irq = bpi, \
do we really want to return 0 here? -ENXIO?
I see that dispatch_mmio_read/write return 0 in that case but I don't
see any reason either? Other kvm_io_device_ops seem to return
-EOPNOTSUPP in such a case.
Hi Vijaya,
On 15/12/2016 08:36, Vijay Kilari wrote:
On Tue, Dec 6, 2016 at 5:12 PM, Auger Eric [off-list ref] wrote:
quoted
Hi,
On 28/11/2016 14:05, Christoffer Dall wrote:
quoted
On Wed, Nov 23, 2016 at 06:31:48PM +0530, vijay.kilari at gmail.com wrote:
quoted
From: Vijaya Kumar K <redacted>
Read and write of some registers like ISPENDR and ICPENDR
from userspace requires special handling when compared to
guest access for these registers.
Refer to Documentation/virtual/kvm/devices/arm-vgic-v3.txt
for handling of ISPENDR, ICPENDR registers handling.
Add infrastructure to support guest and userspace read
and write for the required registers
Also moved vgic_uaccess from vgic-mmio-v2.c to vgic-mmio.c
Signed-off-by: Vijaya Kumar K <redacted>
---
virt/kvm/arm/vgic/vgic-mmio-v2.c | 25 ----------
virt/kvm/arm/vgic/vgic-mmio-v3.c | 102 ++++++++++++++++++++++++++++++++-------
virt/kvm/arm/vgic/vgic-mmio.c | 78 +++++++++++++++++++++++++++---
virt/kvm/arm/vgic/vgic-mmio.h | 19 ++++++++
4 files changed, 175 insertions(+), 49 deletions(-)
@@ -207,6 +207,66 @@ static unsigned long vgic_mmio_read_v3_idregs(struct kvm_vcpu *vcpu,return0;}+staticunsignedlongvgic_v3_uaccess_read_pending(structkvm_vcpu*vcpu,+gpa_taddr,unsignedintlen)+{+u32intid=VGIC_ADDR_TO_INTID(addr,1);+u32value=0;+inti;++/*+*Aleveltriggerredinterruptpendingstateislatchedinboth+*"soft_pending"and"line_level"variables.Userspacewillsave+*andrestoresoft_pendingandline_levelseparately.+*RefertoDocumentation/virtual/kvm/devices/arm-vgic-v3.txt+*handlingofISPENDRandICPENDR.+*/+for(i=0;i<len*8;i++){+structvgic_irq*irq=vgic_get_irq(vcpu->kvm,vcpu,intid+i);++if(irq->config==VGIC_CONFIG_LEVEL&&irq->soft_pending)+value|=(1U<<i);+if(irq->config==VGIC_CONFIG_EDGE&&irq->pending)+value|=(1U<<i);++vgic_put_irq(vcpu->kvm,irq);+}++returnvalue;+}++staticvoidvgic_v3_uaccess_write_pending(structkvm_vcpu*vcpu,+gpa_taddr,unsignedintlen,+unsignedlongval)+{+u32intid=VGIC_ADDR_TO_INTID(addr,1);+inti;++for(i=0;i<len*8;i++){+structvgic_irq*irq=vgic_get_irq(vcpu->kvm,vcpu,intid+i);++spin_lock(&irq->irq_lock);+if(test_bit(i,&val)){+/* soft_pending is set irrespective of irq type+*(leveloredge)toavoiddependencythatVMshould+*restoreirqconfigbeforependinginfo.+*/
@@ -356,7 +416,7 @@ static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu, * We take some special care here to fix the calculation of the register * offset. */-#define REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(off, rd, wr, bpi, acc) \+#define REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(off, rd, wr, ur, uw, bpi, acc) \ { \ .reg_offset = off, \ .bits_per_irq = bpi, \
do we really want to return 0 here? -ENXIO?
I see that dispatch_mmio_read/write return 0 in that case but I don't
see any reason either? Other kvm_io_device_ops seem to return
-EOPNOTSUPP in such a case.
Besides the fact the V* was omitted (for instance VENG0) this looks good
to me.
The previous definitions of has omitted V. If we have to prefix V then
it needs to be changed for all other definitions of VMCR.
I propose to ignore it for now and can clean up later as a separate patch.