From: vijay.kilari at gmail.com <hidden> Date: 2016-11-04 11:13:26
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
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
The patch 3 & 4 are picked from the Pavel's previous implementation.
http://www.spinics.net/lists/kvm/msg122040.html
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.
*** BLURB HERE ***
Vijaya Kumar K (7):
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
arch/arm64/include/uapi/asm/kvm.h | 13 ++
arch/arm64/kvm/Makefile | 1 +
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-kvm-device.c | 224 +++++++++++++++++++++++--
virt/kvm/arm/vgic/vgic-mmio-v2.c | 57 +------
virt/kvm/arm/vgic/vgic-mmio-v3.c | 200 ++++++++++++++++++++--
virt/kvm/arm/vgic/vgic-mmio.c | 149 ++++++++++++++++-
virt/kvm/arm/vgic/vgic-mmio.h | 28 ++++
virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 324 ++++++++++++++++++++++++++++++++++++
virt/kvm/arm/vgic/vgic-v3.c | 18 +-
virt/kvm/arm/vgic/vgic.h | 45 +++++
14 files changed, 1035 insertions(+), 104 deletions(-)
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-04 11:13:27
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 | 98 ++++++++++++++++++++++++++++++++--------
virt/kvm/arm/vgic/vgic-mmio.c | 78 ++++++++++++++++++++++++++++----
virt/kvm/arm/vgic/vgic-mmio.h | 19 ++++++++
4 files changed, 169 insertions(+), 51 deletions(-)
From: vijay.kilari at gmail.com <hidden> Date: 2016-11-04 11:13:28
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_DIST_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
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
Signed-off-by: Vijaya Kumar K <redacted>
---
arch/arm64/include/uapi/asm/kvm.h | 4 +
virt/kvm/arm/vgic/vgic-kvm-device.c | 149 +++++++++++++++++++++++++++++++++---
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 | 33 ++++++++
7 files changed, 276 insertions(+), 24 deletions(-)
@@ -610,6 +621,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.
@@ -716,3 +755,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-04 11:13:29
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-04 11:13:31
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 | 10 ++++++++--
virt/kvm/arm/vgic/vgic.h | 5 +++++
5 files changed, 29 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. */
@@ -63,6 +63,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,
From: vijay.kilari at gmail.com <hidden> Date: 2016-11-04 11:13:32
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 version of VGIC v3 specification is define here
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
Signed-off-by: Pavel Fedin <redacted>
Signed-off-by: Vijaya Kumar K <redacted>
---
arch/arm64/include/uapi/asm/kvm.h | 3 +
arch/arm64/kvm/Makefile | 1 +
include/kvm/arm_vgic.h | 9 +
virt/kvm/arm/vgic/vgic-kvm-device.c | 27 +++
virt/kvm/arm/vgic/vgic-mmio-v3.c | 19 +++
virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 324 ++++++++++++++++++++++++++++++++++++
virt/kvm/arm/vgic/vgic-v3.c | 8 +
virt/kvm/arm/vgic/vgic.h | 4 +
8 files changed, 395 insertions(+)
@@ -23,6 +23,7 @@#include"vgic.h"#include"vgic-mmio.h"+#include"sys_regs.h"/* extract @num bytes at @offset bytes offset in data */unsignedlongextract_bytes(u64data,unsignedintoffset,
@@ -639,6 +640,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;}
@@ -228,6 +228,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;}
@@ -328,6 +335,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-16 18:52:15
On Fri, Nov 04, 2016 at 04:43:27PM +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 | 98 ++++++++++++++++++++++++++++++++--------
virt/kvm/arm/vgic/vgic-mmio.c | 78 ++++++++++++++++++++++++++++----
virt/kvm/arm/vgic/vgic-mmio.h | 19 ++++++++
4 files changed, 169 insertions(+), 51 deletions(-)
@@ -209,6 +209,62 @@ 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)){+irq->pending=true;+irq->soft_pending=true;
In the vgic_mmio_write_spending function we only set the soft_pending
state to true if the interrupt is a level-triggered interrupt.
Should we check if that's the case here as well before setting the
soft_pending state?
Otherwise, this patch looks good.
Thanks,
-Christoffer
@@ -358,7 +414,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-16 18:52:21
On Fri, Nov 04, 2016 at 04:43:28PM +0530, vijay.kilari at gmail.com wrote:
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_DIST_REGS
DIST_REGS and 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
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
I think you should just point to the Documentation/... path in the
kernel now when it's merged.
@@ -610,6 +621,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.
@@ -716,3 +755,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-16 18:52:27
On Fri, Nov 04, 2016 at 04:43:31PM +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 | 10 ++++++++--
virt/kvm/arm/vgic/vgic.h | 5 +++++
5 files changed, 29 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. */
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...
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.
@@ -63,6 +63,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,
From: Christoffer Dall <hidden> Date: 2016-11-16 18:52:32
On Fri, Nov 04, 2016 at 04:43:32PM +0530, vijay.kilari at gmail.com wrote:
quoted hunk
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 version of VGIC v3 specification is define here
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
Signed-off-by: Pavel Fedin <redacted>
Signed-off-by: Vijaya Kumar K <redacted>
---
arch/arm64/include/uapi/asm/kvm.h | 3 +
arch/arm64/kvm/Makefile | 1 +
include/kvm/arm_vgic.h | 9 +
virt/kvm/arm/vgic/vgic-kvm-device.c | 27 +++
virt/kvm/arm/vgic/vgic-mmio-v3.c | 19 +++
virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 324 ++++++++++++++++++++++++++++++++++++
virt/kvm/arm/vgic/vgic-v3.c | 8 +
virt/kvm/arm/vgic/vgic.h | 4 +
8 files changed, 395 insertions(+)
Thi is making me wonder: Are we properly handling GICv3 save/restore
for AArch32 now that we have GICv3 support for AArch32? By properly I
mean that either it is clearly only supported on AArch64 systems or it's
supported on both AArch64 and AArch32, but it shouldn't break randomly
on AArch32.
@@ -23,6 +23,7 @@#include"vgic.h"#include"vgic-mmio.h"+#include"sys_regs.h"/* extract @num bytes at @offset bytes offset in data */unsignedlongextract_bytes(u64data,unsignedintoffset,
@@ -639,6 +640,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;}
hmmm, this looks weird to me, because vgic_v3_cpu->num_pri_bits I don't
understand which effect this is intended to have?
Sure, it may limit what you do with other registers later, but since
there's no ordering requirement that the ctlr be restored first, I'm not
sure it makes sense.
Also, since this field is RO in the ICH_VTR, we'll have a strange
situation during runtime after a GICv3 restore where the
vgic_v3_cpu->num_pri_its differs from the hardware's ICH_VTR_EL2 field,
which is never the case if you didn't do a save/restore.
Finally, should we somehow ensure that this field is set to the same
value across VCPUs or is that not an architectural requirement?
I'm really confused here. Is the vmcr.ctlr field in the ICC_CTLR_EL1
format or in the VMCR format? I would assume the former, since
otherwise I don't get the point with this indirection, and for GICv2
vmcr.ctlr captures the GICC_CTLR value and git_set_vmcr transforms this
into VMCR values.
Having a line that says "ctlr &= ~ICH_VMCR" should make some alarm bells
ring.
+ vgic_set_vmcr(vcpu, &vmcr);
Should we check compatibility between the source and destination for the
SEIS and A3V support here?
+ } 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 & ICH_VMCR_CBPR_MASK) >>
+ ICH_VMCR_CBPR_SHIFT) << ICC_CTLR_EL1_CBPR_SHIFT;
+ val |= ((vmcr.ctlr & ICH_VMCR_EOIM_MASK) >>
+ ICH_VMCR_EOIM_SHIFT) << ICC_CTLR_EL1_EOImode_SHIFT;
what's the rationale behind ignoring the case where userspace is using
unsupported priorities? Is it that this will be checked during
save/restore of the ctlr?
This sort of thing just looks like the case that's impossible to debug,
because userspace could be scratching its head trying to understand why
the value it wrote isn't recorded anywhere...
If there's a good rationale for doing it this way, then could we have a
comment to that effect?
@@ -228,6 +228,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;}
@@ -328,6 +335,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");
I think you misunderstood this part of the API. Userspace should be
able to both set an asserted and deasserted line level, regardless of
what the value was before. So you need to loop through all of them and
set the level as nneded.
On Thu, Nov 17, 2016 at 12:22 AM, Christoffer Dall
[off-list ref] wrote:
On Fri, Nov 04, 2016 at 04:43:27PM +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 | 98 ++++++++++++++++++++++++++++++++--------
virt/kvm/arm/vgic/vgic-mmio.c | 78 ++++++++++++++++++++++++++++----
virt/kvm/arm/vgic/vgic-mmio.h | 19 ++++++++
4 files changed, 169 insertions(+), 51 deletions(-)
@@ -209,6 +209,62 @@ 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)){+irq->pending=true;+irq->soft_pending=true;
In the vgic_mmio_write_spending function we only set the soft_pending
state to true if the interrupt is a level-triggered interrupt.
Should we check if that's the case here as well before setting the
soft_pending state?
Yes, can be done. But it puts hard requirement that irq config should
be restored
before updating pending state.
In any case, the soft_pending is used only if interrupt is level-triggered.
Otherwise, this patch looks good.
Thanks,
-Christoffer
From: Christoffer Dall <hidden> Date: 2016-11-17 11:40:16
On Thu, Nov 17, 2016 at 04:56:53PM +0530, Vijay Kilari wrote:
On Thu, Nov 17, 2016 at 12:22 AM, Christoffer Dall
[off-list ref] wrote:
quoted
On Fri, Nov 04, 2016 at 04:43:27PM +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 | 98 ++++++++++++++++++++++++++++++++--------
virt/kvm/arm/vgic/vgic-mmio.c | 78 ++++++++++++++++++++++++++++----
virt/kvm/arm/vgic/vgic-mmio.h | 19 ++++++++
4 files changed, 169 insertions(+), 51 deletions(-)
@@ -209,6 +209,62 @@ 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)){+irq->pending=true;+irq->soft_pending=true;
In the vgic_mmio_write_spending function we only set the soft_pending
state to true if the interrupt is a level-triggered interrupt.
Should we check if that's the case here as well before setting the
soft_pending state?
Yes, can be done. But it puts hard requirement that irq config should
be restored
before updating pending state.
Ah, I see.
ok, I think you should keep it the way it is then, but please add a
comment to that effect.
In any case, the soft_pending is used only if interrupt is level-triggered.
I noticed when looking at this series that there are quite a few
conflicts when applying to kvmarm/queue, so if you could rebase the next
verson on kvmarm/queue, then you'll make Marc's life a bit easier.
Thanks,
-Christoffer
On Thu, Nov 17, 2016 at 12:22 AM, Christoffer Dall
[off-list ref] wrote:
On Fri, Nov 04, 2016 at 04:43:31PM +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 | 10 ++++++++--
virt/kvm/arm/vgic/vgic.h | 5 +++++
5 files changed, 29 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. */
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.
On Thu, Nov 17, 2016 at 12:22 AM, Christoffer Dall
[off-list ref] wrote:
On Fri, Nov 04, 2016 at 04:43:32PM +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 version of VGIC v3 specification is define here
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
Signed-off-by: Pavel Fedin <redacted>
Signed-off-by: Vijaya Kumar K <redacted>
---
arch/arm64/include/uapi/asm/kvm.h | 3 +
arch/arm64/kvm/Makefile | 1 +
include/kvm/arm_vgic.h | 9 +
virt/kvm/arm/vgic/vgic-kvm-device.c | 27 +++
virt/kvm/arm/vgic/vgic-mmio-v3.c | 19 +++
virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 324 ++++++++++++++++++++++++++++++++++++
virt/kvm/arm/vgic/vgic-v3.c | 8 +
virt/kvm/arm/vgic/vgic.h | 4 +
8 files changed, 395 insertions(+)
Thi is making me wonder: Are we properly handling GICv3 save/restore
for AArch32 now that we have GICv3 support for AArch32? By properly I
mean that either it is clearly only supported on AArch64 systems or it's
supported on both AArch64 and AArch32, but it shouldn't break randomly
on AArch32.
It supports both AArch64 and AArch64 in handling of system registers
save/restore.
All system registers that we save/restore are 32-bit for both aarch64
and aarch32.
Though opcode op0 should be zero for aarch32, the remaining Op and CRn codes
are same. However the codes sent by qemu is matched and register
are handled properly irrespective of AArch32 or AArch64.
I don't have platform which support AArch32 guests to verify.
@@ -23,6 +23,7 @@#include"vgic.h"#include"vgic-mmio.h"+#include"sys_regs.h"/* extract @num bytes at @offset bytes offset in data */unsignedlongextract_bytes(u64data,unsignedintoffset,
@@ -639,6 +640,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;}
hmmm, this looks weird to me, because vgic_v3_cpu->num_pri_bits I don't
understand which effect this is intended to have?
Sure, it may limit what you do with other registers later, but since
there's no ordering requirement that the ctlr be restored first, I'm not
sure it makes sense.
Also, since this field is RO in the ICH_VTR, we'll have a strange
situation during runtime after a GICv3 restore where the
vgic_v3_cpu->num_pri_its differs from the hardware's ICH_VTR_EL2 field,
which is never the case if you didn't do a save/restore.
Yes, but in any case, vgic_v3_cpu->num_pri_bits will be always less
than HW supported
value.
Finally, should we somehow ensure that this field is set to the same
value across VCPUs or is that not an architectural requirement?
Yes it is nice to have it same across VCPUs. But should be ok as
we are ensuring value is not greater than HW supported value.
There is no single point of place where we can make such a check
I'm really confused here. Is the vmcr.ctlr field in the ICC_CTLR_EL1
format or in the VMCR format? I would assume the former, since
otherwise I don't get the point with this indirection, and for GICv2
vmcr.ctlr captures the GICC_CTLR value and git_set_vmcr transforms this
into VMCR values.
Having a line that says "ctlr &= ~ICH_VMCR" should make some alarm bells
ring.
I will check and fix it.
quoted
+ vgic_set_vmcr(vcpu, &vmcr);
Should we check compatibility between the source and destination for the
SEIS and A3V support here?
Can be checked. But I feel A3V check makes more sense than checking for
SEIS.
quoted
+ } 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 & ICH_VMCR_CBPR_MASK) >>
+ ICH_VMCR_CBPR_SHIFT) << ICC_CTLR_EL1_CBPR_SHIFT;
+ val |= ((vmcr.ctlr & ICH_VMCR_EOIM_MASK) >>
+ ICH_VMCR_EOIM_SHIFT) << ICC_CTLR_EL1_EOImode_SHIFT;
what's the rationale behind ignoring the case where userspace is using
unsupported priorities? Is it that this will be checked during
save/restore of the ctlr?
This sort of thing just looks like the case that's impossible to debug,
because userspace could be scratching its head trying to understand why
the value it wrote isn't recorded anywhere...
If there's a good rationale for doing it this way, then could we have a
comment to that effect?
Accessing umplemented priority registers raised UNDEF exception.
So userspace accesing should be ignored instead of recording unsupported
values.
@@ -228,6 +228,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;}
@@ -328,6 +335,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-17 16:01:54
On Thu, Nov 17, 2016 at 06:12:39PM +0530, Vijay Kilari wrote:
On Thu, Nov 17, 2016 at 12:22 AM, Christoffer Dall
[off-list ref] wrote:
quoted
On Fri, Nov 04, 2016 at 04:43:31PM +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 | 10 ++++++++--
virt/kvm/arm/vgic/vgic.h | 5 +++++
5 files changed, 29 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. */
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
quoted
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.
So you should add a comment or the very least add to the commit message
that we ignore the FIQen bit, because our GIC emulation always implies
SRE=1 which means the vFIQEn bit is also RES1 (if I got this right).
-Christoffer
From: Christoffer Dall <hidden> Date: 2016-11-17 16:09:51
On Thu, Nov 17, 2016 at 09:25:59PM +0530, Vijay Kilari wrote:
On Thu, Nov 17, 2016 at 12:22 AM, Christoffer Dall
[off-list ref] wrote:
quoted
On Fri, Nov 04, 2016 at 04:43:32PM +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 version of VGIC v3 specification is define here
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
Signed-off-by: Pavel Fedin <redacted>
Signed-off-by: Vijaya Kumar K <redacted>
---
arch/arm64/include/uapi/asm/kvm.h | 3 +
arch/arm64/kvm/Makefile | 1 +
include/kvm/arm_vgic.h | 9 +
virt/kvm/arm/vgic/vgic-kvm-device.c | 27 +++
virt/kvm/arm/vgic/vgic-mmio-v3.c | 19 +++
virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 324 ++++++++++++++++++++++++++++++++++++
virt/kvm/arm/vgic/vgic-v3.c | 8 +
virt/kvm/arm/vgic/vgic.h | 4 +
8 files changed, 395 insertions(+)
Thi is making me wonder: Are we properly handling GICv3 save/restore
for AArch32 now that we have GICv3 support for AArch32? By properly I
mean that either it is clearly only supported on AArch64 systems or it's
supported on both AArch64 and AArch32, but it shouldn't break randomly
on AArch32.
It supports both AArch64 and AArch64 in handling of system registers
save/restore.
All system registers that we save/restore are 32-bit for both aarch64
and aarch32.
Though opcode op0 should be zero for aarch32, the remaining Op and CRn codes
are same. However the codes sent by qemu is matched and register
are handled properly irrespective of AArch32 or AArch64.
I don't have platform which support AArch32 guests to verify.
Actually this is not about the guest, it's about an ARMv8 AArch32 host
that has a GICv3.
I just tried to do a v7 compile with your patches, and it results in an
epic failure, so there's something for you to look at.
@@ -23,6 +23,7 @@#include"vgic.h"#include"vgic-mmio.h"+#include"sys_regs.h"/* extract @num bytes at @offset bytes offset in data */unsignedlongextract_bytes(u64data,unsignedintoffset,
@@ -639,6 +640,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;}
hmmm, this looks weird to me, because vgic_v3_cpu->num_pri_bits I don't
understand which effect this is intended to have?
Sure, it may limit what you do with other registers later, but since
there's no ordering requirement that the ctlr be restored first, I'm not
sure it makes sense.
Also, since this field is RO in the ICH_VTR, we'll have a strange
situation during runtime after a GICv3 restore where the
vgic_v3_cpu->num_pri_its differs from the hardware's ICH_VTR_EL2 field,
which is never the case if you didn't do a save/restore.
Yes, but in any case, vgic_v3_cpu->num_pri_bits will be always less
than HW supported
value.
So answer my question: What is the intended effect of writing this
value? Is it just so that if you migrate this platform back again, then
you're checking compatibility with what the guest would potentially do,
or should you maintain the num_pri_bits limitation during runtime
somehow?
quoted
Finally, should we somehow ensure that this field is set to the same
value across VCPUs or is that not an architectural requirement?
Yes it is nice to have it same across VCPUs. But should be ok as
we are ensuring value is not greater than HW supported value.
Does the architecture allow having a different number of priority bits
supported across CPUs? If not, you shouldn't allow a VM programming
things that way either.
There is no single point of place where we can make such a check
I'm really confused here. Is the vmcr.ctlr field in the ICC_CTLR_EL1
format or in the VMCR format? I would assume the former, since
otherwise I don't get the point with this indirection, and for GICv2
vmcr.ctlr captures the GICC_CTLR value and git_set_vmcr transforms this
into VMCR values.
Having a line that says "ctlr &= ~ICH_VMCR" should make some alarm bells
ring.
I will check and fix it.
quoted
quoted
+ vgic_set_vmcr(vcpu, &vmcr);
Should we check compatibility between the source and destination for the
SEIS and A3V support here?
Can be checked. But I feel A3V check makes more sense than checking for
SEIS.
Please argue the *why* for whatever you end up doing with respect to
both bits in the commit message of your next patch revision.
quoted
quoted
+ } 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 & ICH_VMCR_CBPR_MASK) >>
+ ICH_VMCR_CBPR_SHIFT) << ICC_CTLR_EL1_CBPR_SHIFT;
+ val |= ((vmcr.ctlr & ICH_VMCR_EOIM_MASK) >>
+ ICH_VMCR_EOIM_SHIFT) << ICC_CTLR_EL1_EOImode_SHIFT;
what's the rationale behind ignoring the case where userspace is using
unsupported priorities? Is it that this will be checked during
save/restore of the ctlr?
This sort of thing just looks like the case that's impossible to debug,
because userspace could be scratching its head trying to understand why
the value it wrote isn't recorded anywhere...
If there's a good rationale for doing it this way, then could we have a
comment to that effect?
Accessing umplemented priority registers raised UNDEF exception.
So userspace accesing should be ignored instead of recording unsupported
values.
That's not what I asked.
I asked why it's silently ignored as opposed to raising an error visible
to user space?
@@ -228,6 +228,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;}
@@ -328,6 +335,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");
On Thu, Nov 17, 2016 at 9:39 PM, Christoffer Dall
[off-list ref] wrote:
On Thu, Nov 17, 2016 at 09:25:59PM +0530, Vijay Kilari wrote:
quoted
On Thu, Nov 17, 2016 at 12:22 AM, Christoffer Dall
[off-list ref] wrote:
quoted
On Fri, Nov 04, 2016 at 04:43:32PM +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 version of VGIC v3 specification is define here
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
Signed-off-by: Pavel Fedin <redacted>
Signed-off-by: Vijaya Kumar K <redacted>
---
arch/arm64/include/uapi/asm/kvm.h | 3 +
arch/arm64/kvm/Makefile | 1 +
include/kvm/arm_vgic.h | 9 +
virt/kvm/arm/vgic/vgic-kvm-device.c | 27 +++
virt/kvm/arm/vgic/vgic-mmio-v3.c | 19 +++
virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 324 ++++++++++++++++++++++++++++++++++++
virt/kvm/arm/vgic/vgic-v3.c | 8 +
virt/kvm/arm/vgic/vgic.h | 4 +
8 files changed, 395 insertions(+)
Thi is making me wonder: Are we properly handling GICv3 save/restore
for AArch32 now that we have GICv3 support for AArch32? By properly I
mean that either it is clearly only supported on AArch64 systems or it's
supported on both AArch64 and AArch32, but it shouldn't break randomly
on AArch32.
It supports both AArch64 and AArch64 in handling of system registers
save/restore.
All system registers that we save/restore are 32-bit for both aarch64
and aarch32.
Though opcode op0 should be zero for aarch32, the remaining Op and CRn codes
are same. However the codes sent by qemu is matched and register
are handled properly irrespective of AArch32 or AArch64.
I don't have platform which support AArch32 guests to verify.
Actually this is not about the guest, it's about an ARMv8 AArch32 host
that has a GICv3.
I just tried to do a v7 compile with your patches, and it results in an
epic failure, so there's something for you to look at.
@@ -23,6 +23,7 @@#include"vgic.h"#include"vgic-mmio.h"+#include"sys_regs.h"/* extract @num bytes at @offset bytes offset in data */unsignedlongextract_bytes(u64data,unsignedintoffset,
@@ -639,6 +640,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;}
hmmm, this looks weird to me, because vgic_v3_cpu->num_pri_bits I don't
understand which effect this is intended to have?
Sure, it may limit what you do with other registers later, but since
there's no ordering requirement that the ctlr be restored first, I'm not
sure it makes sense.
Also, since this field is RO in the ICH_VTR, we'll have a strange
situation during runtime after a GICv3 restore where the
vgic_v3_cpu->num_pri_its differs from the hardware's ICH_VTR_EL2 field,
which is never the case if you didn't do a save/restore.
Yes, but in any case, vgic_v3_cpu->num_pri_bits will be always less
than HW supported
value.
So answer my question: What is the intended effect of writing this
value? Is it just so that if you migrate this platform back again, then
you're checking compatibility with what the guest would potentially do,
Yes and also to limit the valid aprn registers access as you said above.
But that has ordering restriction. Which I think we should follow.
or should you maintain the num_pri_bits limitation during runtime
somehow?
Once after checking compatibility, at runtime it is not updated
and this value is not used at all in VGIC further
quoted
quoted
Finally, should we somehow ensure that this field is set to the same
value across VCPUs or is that not an architectural requirement?
Yes it is nice to have it same across VCPUs. But should be ok as
we are ensuring value is not greater than HW supported value.
Does the architecture allow having a different number of priority bits
supported across CPUs? If not, you shouldn't allow a VM programming
things that way either.
AFAIK, architecturally it is not mentioned any where in the spec that priority
bits should be same across CPUs.
quoted
There is no single point of place where we can make such a check
I'm really confused here. Is the vmcr.ctlr field in the ICC_CTLR_EL1
format or in the VMCR format? I would assume the former, since
otherwise I don't get the point with this indirection, and for GICv2
vmcr.ctlr captures the GICC_CTLR value and git_set_vmcr transforms this
into VMCR values.
Having a line that says "ctlr &= ~ICH_VMCR" should make some alarm bells
ring.
I will check and fix it.
quoted
quoted
+ vgic_set_vmcr(vcpu, &vmcr);
Should we check compatibility between the source and destination for the
SEIS and A3V support here?
Can be checked. But I feel A3V check makes more sense than checking for
SEIS.
Please argue the *why* for whatever you end up doing with respect to
both bits in the commit message of your next patch revision.
quoted
quoted
quoted
+ } 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 & ICH_VMCR_CBPR_MASK) >>
+ ICH_VMCR_CBPR_SHIFT) << ICC_CTLR_EL1_CBPR_SHIFT;
+ val |= ((vmcr.ctlr & ICH_VMCR_EOIM_MASK) >>
+ ICH_VMCR_EOIM_SHIFT) << ICC_CTLR_EL1_EOImode_SHIFT;
what's the rationale behind ignoring the case where userspace is using
unsupported priorities? Is it that this will be checked during
save/restore of the ctlr?
This sort of thing just looks like the case that's impossible to debug,
because userspace could be scratching its head trying to understand why
the value it wrote isn't recorded anywhere...
If there's a good rationale for doing it this way, then could we have a
comment to that effect?
Accessing umplemented priority registers raised UNDEF exception.
So userspace accesing should be ignored instead of recording unsupported
values.
That's not what I asked.
I asked why it's silently ignored as opposed to raising an error visible
to user space?
@@ -228,6 +228,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;}
@@ -328,6 +335,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");
On Thu, Nov 17, 2016 at 9:39 PM, Christoffer Dall
[off-list ref] wrote:
On Thu, Nov 17, 2016 at 09:25:59PM +0530, Vijay Kilari wrote:
quoted
On Thu, Nov 17, 2016 at 12:22 AM, Christoffer Dall
[off-list ref] wrote:
quoted
On Fri, Nov 04, 2016 at 04:43:32PM +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 version of VGIC v3 specification is define here
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
Signed-off-by: Pavel Fedin <redacted>
Signed-off-by: Vijaya Kumar K <redacted>
---
arch/arm64/include/uapi/asm/kvm.h | 3 +
arch/arm64/kvm/Makefile | 1 +
include/kvm/arm_vgic.h | 9 +
virt/kvm/arm/vgic/vgic-kvm-device.c | 27 +++
virt/kvm/arm/vgic/vgic-mmio-v3.c | 19 +++
virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 324 ++++++++++++++++++++++++++++++++++++
virt/kvm/arm/vgic/vgic-v3.c | 8 +
virt/kvm/arm/vgic/vgic.h | 4 +
8 files changed, 395 insertions(+)
Thi is making me wonder: Are we properly handling GICv3 save/restore
for AArch32 now that we have GICv3 support for AArch32? By properly I
mean that either it is clearly only supported on AArch64 systems or it's
supported on both AArch64 and AArch32, but it shouldn't break randomly
on AArch32.
It supports both AArch64 and AArch64 in handling of system registers
save/restore.
All system registers that we save/restore are 32-bit for both aarch64
and aarch32.
Though opcode op0 should be zero for aarch32, the remaining Op and CRn codes
are same. However the codes sent by qemu is matched and register
are handled properly irrespective of AArch32 or AArch64.
I don't have platform which support AArch32 guests to verify.
Actually this is not about the guest, it's about an ARMv8 AArch32 host
that has a GICv3.
I just tried to do a v7 compile with your patches, and it results in an
epic failure, so there's something for you to look at.
Could you please share you config file?. I tried with multi_v7 defconfig with
CONFIG KVM and CONFIG_KVM_ARM_HOST enabled. it compiled for me.
From: Christoffer Dall <hidden> Date: 2016-11-20 13:20:05
On Sat, Nov 19, 2016 at 12:18:53AM +0530, Vijay Kilari wrote:
On Thu, Nov 17, 2016 at 9:39 PM, Christoffer Dall
[off-list ref] wrote:
quoted
On Thu, Nov 17, 2016 at 09:25:59PM +0530, Vijay Kilari wrote:
quoted
On Thu, Nov 17, 2016 at 12:22 AM, Christoffer Dall
[off-list ref] wrote:
quoted
On Fri, Nov 04, 2016 at 04:43:32PM +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 version of VGIC v3 specification is define here
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
Signed-off-by: Pavel Fedin <redacted>
Signed-off-by: Vijaya Kumar K <redacted>
---
arch/arm64/include/uapi/asm/kvm.h | 3 +
arch/arm64/kvm/Makefile | 1 +
include/kvm/arm_vgic.h | 9 +
virt/kvm/arm/vgic/vgic-kvm-device.c | 27 +++
virt/kvm/arm/vgic/vgic-mmio-v3.c | 19 +++
virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 324 ++++++++++++++++++++++++++++++++++++
virt/kvm/arm/vgic/vgic-v3.c | 8 +
virt/kvm/arm/vgic/vgic.h | 4 +
8 files changed, 395 insertions(+)
Thi is making me wonder: Are we properly handling GICv3 save/restore
for AArch32 now that we have GICv3 support for AArch32? By properly I
mean that either it is clearly only supported on AArch64 systems or it's
supported on both AArch64 and AArch32, but it shouldn't break randomly
on AArch32.
It supports both AArch64 and AArch64 in handling of system registers
save/restore.
All system registers that we save/restore are 32-bit for both aarch64
and aarch32.
Though opcode op0 should be zero for aarch32, the remaining Op and CRn codes
are same. However the codes sent by qemu is matched and register
are handled properly irrespective of AArch32 or AArch64.
I don't have platform which support AArch32 guests to verify.
Actually this is not about the guest, it's about an ARMv8 AArch32 host
that has a GICv3.
I just tried to do a v7 compile with your patches, and it results in an
epic failure, so there's something for you to look at.
Could you please share you config file?. I tried with multi_v7 defconfig with
CONFIG KVM and CONFIG_KVM_ARM_HOST enabled. it compiled for me.
I think this has to do with which branch you apply your patches to.
When applied to kvmarm/next, it fails.
Here's the integration I did:
https://git.linaro.org/people/christoffer.dall/linux-kvm-arm.git tmp-gicv3-migrate-v8
Here's the config:
https://transfer.sh/xkAxp/.config
Here's the compile output:
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-mmio-v3.c:26:22: fatal error: sys_regs.h: No such file or directory
#include "sys_regs.h"
^
compilation terminated.
make[2]: *** [arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-mmio-v3.o] Error 1
make[2]: *** Waiting for unfinished jobs....
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c: In function ?vgic_v3_parse_attr?:
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:438:29: error: ?KVM_DEV_ARM_VGIC_V3_MPIDR_MASK? undeclared (first use in this function)
vgic_mpidr = (attr->attr & KVM_DEV_ARM_VGIC_V3_MPIDR_MASK) >>
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:438:29: note: each undeclared identifier is reported only once for each function it appears in
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:439:9: error: ?KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT? undeclared (first use in this function)
KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT;
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:441:2: error: implicit declaration of function ?MPIDR_LEVEL_SHIFT? [-Werror=implicit-function-declaration]
mpidr_reg = VGIC_TO_MPIDR(vgic_mpidr);
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c: In function ?vgic_v3_attr_regs_access?:
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:497:7: error: ?KVM_DEV_ARM_VGIC_GRP_REDIST_REGS? undeclared (first use in this function)
case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:505:7: error: ?KVM_DEV_ARM_VGIC_CPU_SYSREGS? undeclared (first use in this function)
case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:508:25: error: ?KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK? undeclared (first use in this function)
regid = (attr->attr & KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK);
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:513:7: error: ?KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO? undeclared (first use in this function)
case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: {
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:516:24: error: ?KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK? undeclared (first use in this function)
info = (attr->attr & KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK) >>
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:517:4: error: ?KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT? undeclared (first use in this function)
KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT;
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:518:15: error: ?VGIC_LEVEL_INFO_LINE_LEVEL? undeclared (first use in this function)
if (info == VGIC_LEVEL_INFO_LINE_LEVEL) {
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:522:5: error: ?KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK? undeclared (first use in this function)
KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK;
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c: In function ?vgic_v3_set_attr?:
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:554:7: error: ?KVM_DEV_ARM_VGIC_GRP_REDIST_REGS? undeclared (first use in this function)
case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS: {
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:565:7: error: ?KVM_DEV_ARM_VGIC_CPU_SYSREGS? undeclared (first use in this function)
case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:574:7: error: ?KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO? undeclared (first use in this function)
case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: {
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c: In function ?vgic_v3_get_attr?:
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:600:7: error: ?KVM_DEV_ARM_VGIC_GRP_REDIST_REGS? undeclared (first use in this function)
case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS: {
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:611:7: error: ?KVM_DEV_ARM_VGIC_CPU_SYSREGS? undeclared (first use in this function)
case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:620:7: error: ?KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO? undeclared (first use in this function)
case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: {
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c: In function ?vgic_v3_has_attr?:
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:647:7: error: ?KVM_DEV_ARM_VGIC_GRP_REDIST_REGS? undeclared (first use in this function)
case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:648:7: error: ?KVM_DEV_ARM_VGIC_CPU_SYSREGS? undeclared (first use in this function)
case KVM_DEV_ARM_VGIC_CPU_SYSREGS:
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:652:7: error: ?KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO? undeclared (first use in this function)
case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: {
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:653:22: error: ?KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK? undeclared (first use in this function)
if (((attr->attr & KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK) >>
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:654:9: error: ?KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT? undeclared (first use in this function)
KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT) ==
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:655:9: error: ?VGIC_LEVEL_INFO_LINE_LEVEL? undeclared (first use in this function)
VGIC_LEVEL_INFO_LINE_LEVEL)
^
cc1: some warnings being treated as errors
make[2]: *** [arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.o] Error 1
make[1]: *** [arch/arm/kvm] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [sub-make] Error 2
Thanks,
-Christoffer
From: Christoffer Dall <hidden> Date: 2016-11-21 10:19:30
On Fri, Nov 18, 2016 at 10:28:34PM +0530, Vijay Kilari wrote:
On Thu, Nov 17, 2016 at 9:39 PM, Christoffer Dall
[off-list ref] wrote:
quoted
On Thu, Nov 17, 2016 at 09:25:59PM +0530, Vijay Kilari wrote:
quoted
On Thu, Nov 17, 2016 at 12:22 AM, Christoffer Dall
[off-list ref] wrote:
quoted
On Fri, Nov 04, 2016 at 04:43:32PM +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 version of VGIC v3 specification is define here
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
Signed-off-by: Pavel Fedin <redacted>
Signed-off-by: Vijaya Kumar K <redacted>
---
arch/arm64/include/uapi/asm/kvm.h | 3 +
arch/arm64/kvm/Makefile | 1 +
include/kvm/arm_vgic.h | 9 +
virt/kvm/arm/vgic/vgic-kvm-device.c | 27 +++
virt/kvm/arm/vgic/vgic-mmio-v3.c | 19 +++
virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 324 ++++++++++++++++++++++++++++++++++++
virt/kvm/arm/vgic/vgic-v3.c | 8 +
virt/kvm/arm/vgic/vgic.h | 4 +
8 files changed, 395 insertions(+)
Thi is making me wonder: Are we properly handling GICv3 save/restore
for AArch32 now that we have GICv3 support for AArch32? By properly I
mean that either it is clearly only supported on AArch64 systems or it's
supported on both AArch64 and AArch32, but it shouldn't break randomly
on AArch32.
It supports both AArch64 and AArch64 in handling of system registers
save/restore.
All system registers that we save/restore are 32-bit for both aarch64
and aarch32.
Though opcode op0 should be zero for aarch32, the remaining Op and CRn codes
are same. However the codes sent by qemu is matched and register
are handled properly irrespective of AArch32 or AArch64.
I don't have platform which support AArch32 guests to verify.
Actually this is not about the guest, it's about an ARMv8 AArch32 host
that has a GICv3.
I just tried to do a v7 compile with your patches, and it results in an
epic failure, so there's something for you to look at.
@@ -23,6 +23,7 @@#include"vgic.h"#include"vgic-mmio.h"+#include"sys_regs.h"/* extract @num bytes at @offset bytes offset in data */unsignedlongextract_bytes(u64data,unsignedintoffset,
@@ -639,6 +640,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;}
hmmm, this looks weird to me, because vgic_v3_cpu->num_pri_bits I don't
understand which effect this is intended to have?
Sure, it may limit what you do with other registers later, but since
there's no ordering requirement that the ctlr be restored first, I'm not
sure it makes sense.
Also, since this field is RO in the ICH_VTR, we'll have a strange
situation during runtime after a GICv3 restore where the
vgic_v3_cpu->num_pri_its differs from the hardware's ICH_VTR_EL2 field,
which is never the case if you didn't do a save/restore.
Yes, but in any case, vgic_v3_cpu->num_pri_bits will be always less
than HW supported
value.
So answer my question: What is the intended effect of writing this
value? Is it just so that if you migrate this platform back again, then
you're checking compatibility with what the guest would potentially do,
Yes
Then add a comment explaining that
and also to limit the valid aprn registers access as you said above.
But that has ordering restriction. Which I think we should follow.
I'm sorry, now I'm confused. Is there an ordering requirement in the
API, or how should we follow this?
quoted
or should you maintain the num_pri_bits limitation during runtime
somehow?
Once after checking compatibility, at runtime it is not updated
and this value is not used at all in VGIC further
quoted
quoted
quoted
Finally, should we somehow ensure that this field is set to the same
value across VCPUs or is that not an architectural requirement?
Yes it is nice to have it same across VCPUs. But should be ok as
we are ensuring value is not greater than HW supported value.
Does the architecture allow having a different number of priority bits
supported across CPUs? If not, you shouldn't allow a VM programming
things that way either.
AFAIK, architecturally it is not mentioned any where in the spec that priority
bits should be same across CPUs.
ok
quoted
quoted
There is no single point of place where we can make such a check
I'm really confused here. Is the vmcr.ctlr field in the ICC_CTLR_EL1
format or in the VMCR format? I would assume the former, since
otherwise I don't get the point with this indirection, and for GICv2
vmcr.ctlr captures the GICC_CTLR value and git_set_vmcr transforms this
into VMCR values.
Having a line that says "ctlr &= ~ICH_VMCR" should make some alarm bells
ring.
I will check and fix it.
quoted
quoted
+ vgic_set_vmcr(vcpu, &vmcr);
Should we check compatibility between the source and destination for the
SEIS and A3V support here?
Can be checked. But I feel A3V check makes more sense than checking for
SEIS.
Please argue the *why* for whatever you end up doing with respect to
both bits in the commit message of your next patch revision.
quoted
quoted
quoted
+ } 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 & ICH_VMCR_CBPR_MASK) >>
+ ICH_VMCR_CBPR_SHIFT) << ICC_CTLR_EL1_CBPR_SHIFT;
+ val |= ((vmcr.ctlr & ICH_VMCR_EOIM_MASK) >>
+ ICH_VMCR_EOIM_SHIFT) << ICC_CTLR_EL1_EOImode_SHIFT;
what's the rationale behind ignoring the case where userspace is using
unsupported priorities? Is it that this will be checked during
save/restore of the ctlr?
This sort of thing just looks like the case that's impossible to debug,
because userspace could be scratching its head trying to understand why
the value it wrote isn't recorded anywhere...
If there's a good rationale for doing it this way, then could we have a
comment to that effect?
Accessing umplemented priority registers raised UNDEF exception.
So userspace accesing should be ignored instead of recording unsupported
values.
That's not what I asked.
I asked why it's silently ignored as opposed to raising an error visible
to user space?
On Mon, Nov 21, 2016 at 3:49 PM, Christoffer Dall
[off-list ref] wrote:
On Fri, Nov 18, 2016 at 10:28:34PM +0530, Vijay Kilari wrote:
quoted
On Thu, Nov 17, 2016 at 9:39 PM, Christoffer Dall
[off-list ref] wrote:
quoted
On Thu, Nov 17, 2016 at 09:25:59PM +0530, Vijay Kilari wrote:
quoted
On Thu, Nov 17, 2016 at 12:22 AM, Christoffer Dall
[off-list ref] wrote:
quoted
On Fri, Nov 04, 2016 at 04:43:32PM +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 version of VGIC v3 specification is define here
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
Signed-off-by: Pavel Fedin <redacted>
Signed-off-by: Vijaya Kumar K <redacted>
---
arch/arm64/include/uapi/asm/kvm.h | 3 +
arch/arm64/kvm/Makefile | 1 +
include/kvm/arm_vgic.h | 9 +
virt/kvm/arm/vgic/vgic-kvm-device.c | 27 +++
virt/kvm/arm/vgic/vgic-mmio-v3.c | 19 +++
virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 324 ++++++++++++++++++++++++++++++++++++
virt/kvm/arm/vgic/vgic-v3.c | 8 +
virt/kvm/arm/vgic/vgic.h | 4 +
8 files changed, 395 insertions(+)
Thi is making me wonder: Are we properly handling GICv3 save/restore
for AArch32 now that we have GICv3 support for AArch32? By properly I
mean that either it is clearly only supported on AArch64 systems or it's
supported on both AArch64 and AArch32, but it shouldn't break randomly
on AArch32.
It supports both AArch64 and AArch64 in handling of system registers
save/restore.
All system registers that we save/restore are 32-bit for both aarch64
and aarch32.
Though opcode op0 should be zero for aarch32, the remaining Op and CRn codes
are same. However the codes sent by qemu is matched and register
are handled properly irrespective of AArch32 or AArch64.
I don't have platform which support AArch32 guests to verify.
Actually this is not about the guest, it's about an ARMv8 AArch32 host
that has a GICv3.
I just tried to do a v7 compile with your patches, and it results in an
epic failure, so there's something for you to look at.
@@ -23,6 +23,7 @@#include"vgic.h"#include"vgic-mmio.h"+#include"sys_regs.h"/* extract @num bytes at @offset bytes offset in data */unsignedlongextract_bytes(u64data,unsignedintoffset,
@@ -639,6 +640,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;}
hmmm, this looks weird to me, because vgic_v3_cpu->num_pri_bits I don't
understand which effect this is intended to have?
Sure, it may limit what you do with other registers later, but since
there's no ordering requirement that the ctlr be restored first, I'm not
sure it makes sense.
Also, since this field is RO in the ICH_VTR, we'll have a strange
situation during runtime after a GICv3 restore where the
vgic_v3_cpu->num_pri_its differs from the hardware's ICH_VTR_EL2 field,
which is never the case if you didn't do a save/restore.
Yes, but in any case, vgic_v3_cpu->num_pri_bits will be always less
than HW supported
value.
So answer my question: What is the intended effect of writing this
value? Is it just so that if you migrate this platform back again, then
you're checking compatibility with what the guest would potentially do,
Yes
Then add a comment explaining that
quoted
and also to limit the valid aprn registers access as you said above.
But that has ordering restriction. Which I think we should follow.
I'm sorry, now I'm confused. Is there an ordering requirement in the
API, or how should we follow this?
There is no ordering requirement mentioned in the API doc.
However the APRn registers depends on num_pri_bits. Hence first
ICC_CTLR_EL1 should be restored before APRn restore.
If ordering is not followed then APRn registers restore is allowed
as per hw supported num_pri_bits.
This should be mentioned in doc.
On Sun, Nov 20, 2016 at 6:50 PM, Christoffer Dall
[off-list ref] wrote:
On Sat, Nov 19, 2016 at 12:18:53AM +0530, Vijay Kilari wrote:
quoted
On Thu, Nov 17, 2016 at 9:39 PM, Christoffer Dall
[off-list ref] wrote:
quoted
On Thu, Nov 17, 2016 at 09:25:59PM +0530, Vijay Kilari wrote:
quoted
On Thu, Nov 17, 2016 at 12:22 AM, Christoffer Dall
[off-list ref] wrote:
quoted
On Fri, Nov 04, 2016 at 04:43:32PM +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 version of VGIC v3 specification is define here
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
Signed-off-by: Pavel Fedin <redacted>
Signed-off-by: Vijaya Kumar K <redacted>
---
arch/arm64/include/uapi/asm/kvm.h | 3 +
arch/arm64/kvm/Makefile | 1 +
include/kvm/arm_vgic.h | 9 +
virt/kvm/arm/vgic/vgic-kvm-device.c | 27 +++
virt/kvm/arm/vgic/vgic-mmio-v3.c | 19 +++
virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 324 ++++++++++++++++++++++++++++++++++++
virt/kvm/arm/vgic/vgic-v3.c | 8 +
virt/kvm/arm/vgic/vgic.h | 4 +
8 files changed, 395 insertions(+)
Thi is making me wonder: Are we properly handling GICv3 save/restore
for AArch32 now that we have GICv3 support for AArch32? By properly I
mean that either it is clearly only supported on AArch64 systems or it's
supported on both AArch64 and AArch32, but it shouldn't break randomly
on AArch32.
It supports both AArch64 and AArch64 in handling of system registers
save/restore.
All system registers that we save/restore are 32-bit for both aarch64
and aarch32.
Though opcode op0 should be zero for aarch32, the remaining Op and CRn codes
are same. However the codes sent by qemu is matched and register
are handled properly irrespective of AArch32 or AArch64.
I don't have platform which support AArch32 guests to verify.
Actually this is not about the guest, it's about an ARMv8 AArch32 host
that has a GICv3.
I just tried to do a v7 compile with your patches, and it results in an
epic failure, so there's something for you to look at.
Could you please share you config file?. I tried with multi_v7 defconfig with
CONFIG KVM and CONFIG_KVM_ARM_HOST enabled. it compiled for me.
Thanks for shareing the details, I could reproduce them.
However virt/kvm/arm/vgic/vgic-sys-reg-v3.c is written with
sys_regs_desc for AArch64.
For AArch32/v7, it has be to coproc_reg. I propose to add separate file for arm
which handles ICC* reg save/restore using coproc_reg.
Here's the compile output:
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-mmio-v3.c:26:22: fatal error: sys_regs.h: No such file or directory
#include "sys_regs.h"
^
compilation terminated.
make[2]: *** [arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-mmio-v3.o] Error 1
make[2]: *** Waiting for unfinished jobs....
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c: In function ?vgic_v3_parse_attr?:
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:438:29: error: ?KVM_DEV_ARM_VGIC_V3_MPIDR_MASK? undeclared (first use in this function)
vgic_mpidr = (attr->attr & KVM_DEV_ARM_VGIC_V3_MPIDR_MASK) >>
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:438:29: note: each undeclared identifier is reported only once for each function it appears in
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:439:9: error: ?KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT? undeclared (first use in this function)
KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT;
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:441:2: error: implicit declaration of function ?MPIDR_LEVEL_SHIFT? [-Werror=implicit-function-declaration]
mpidr_reg = VGIC_TO_MPIDR(vgic_mpidr);
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c: In function ?vgic_v3_attr_regs_access?:
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:497:7: error: ?KVM_DEV_ARM_VGIC_GRP_REDIST_REGS? undeclared (first use in this function)
case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:505:7: error: ?KVM_DEV_ARM_VGIC_CPU_SYSREGS? undeclared (first use in this function)
case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:508:25: error: ?KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK? undeclared (first use in this function)
regid = (attr->attr & KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK);
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:513:7: error: ?KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO? undeclared (first use in this function)
case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: {
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:516:24: error: ?KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK? undeclared (first use in this function)
info = (attr->attr & KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK) >>
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:517:4: error: ?KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT? undeclared (first use in this function)
KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT;
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:518:15: error: ?VGIC_LEVEL_INFO_LINE_LEVEL? undeclared (first use in this function)
if (info == VGIC_LEVEL_INFO_LINE_LEVEL) {
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:522:5: error: ?KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK? undeclared (first use in this function)
KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK;
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c: In function ?vgic_v3_set_attr?:
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:554:7: error: ?KVM_DEV_ARM_VGIC_GRP_REDIST_REGS? undeclared (first use in this function)
case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS: {
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:565:7: error: ?KVM_DEV_ARM_VGIC_CPU_SYSREGS? undeclared (first use in this function)
case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:574:7: error: ?KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO? undeclared (first use in this function)
case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: {
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c: In function ?vgic_v3_get_attr?:
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:600:7: error: ?KVM_DEV_ARM_VGIC_GRP_REDIST_REGS? undeclared (first use in this function)
case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS: {
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:611:7: error: ?KVM_DEV_ARM_VGIC_CPU_SYSREGS? undeclared (first use in this function)
case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:620:7: error: ?KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO? undeclared (first use in this function)
case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: {
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c: In function ?vgic_v3_has_attr?:
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:647:7: error: ?KVM_DEV_ARM_VGIC_GRP_REDIST_REGS? undeclared (first use in this function)
case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:648:7: error: ?KVM_DEV_ARM_VGIC_CPU_SYSREGS? undeclared (first use in this function)
case KVM_DEV_ARM_VGIC_CPU_SYSREGS:
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:652:7: error: ?KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO? undeclared (first use in this function)
case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: {
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:653:22: error: ?KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK? undeclared (first use in this function)
if (((attr->attr & KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK) >>
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:654:9: error: ?KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT? undeclared (first use in this function)
KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT) ==
^
/home/christoffer/src/kvmarm/linux/arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.c:655:9: error: ?VGIC_LEVEL_INFO_LINE_LEVEL? undeclared (first use in this function)
VGIC_LEVEL_INFO_LINE_LEVEL)
^
cc1: some warnings being treated as errors
make[2]: *** [arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-kvm-device.o] Error 1
make[1]: *** [arch/arm/kvm] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [sub-make] Error 2
Thanks,
-Christoffer
From: Christoffer Dall <hidden> Date: 2016-11-21 13:41:13
On Mon, Nov 21, 2016 at 07:02:36PM +0530, Vijay Kilari wrote:
On Sun, Nov 20, 2016 at 6:50 PM, Christoffer Dall
[off-list ref] wrote:
quoted
On Sat, Nov 19, 2016 at 12:18:53AM +0530, Vijay Kilari wrote:
quoted
On Thu, Nov 17, 2016 at 9:39 PM, Christoffer Dall
[off-list ref] wrote:
quoted
On Thu, Nov 17, 2016 at 09:25:59PM +0530, Vijay Kilari wrote:
quoted
On Thu, Nov 17, 2016 at 12:22 AM, Christoffer Dall
[off-list ref] wrote:
quoted
On Fri, Nov 04, 2016 at 04:43:32PM +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 version of VGIC v3 specification is define here
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
Signed-off-by: Pavel Fedin <redacted>
Signed-off-by: Vijaya Kumar K <redacted>
---
arch/arm64/include/uapi/asm/kvm.h | 3 +
arch/arm64/kvm/Makefile | 1 +
include/kvm/arm_vgic.h | 9 +
virt/kvm/arm/vgic/vgic-kvm-device.c | 27 +++
virt/kvm/arm/vgic/vgic-mmio-v3.c | 19 +++
virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 324 ++++++++++++++++++++++++++++++++++++
virt/kvm/arm/vgic/vgic-v3.c | 8 +
virt/kvm/arm/vgic/vgic.h | 4 +
8 files changed, 395 insertions(+)
Thi is making me wonder: Are we properly handling GICv3 save/restore
for AArch32 now that we have GICv3 support for AArch32? By properly I
mean that either it is clearly only supported on AArch64 systems or it's
supported on both AArch64 and AArch32, but it shouldn't break randomly
on AArch32.
It supports both AArch64 and AArch64 in handling of system registers
save/restore.
All system registers that we save/restore are 32-bit for both aarch64
and aarch32.
Though opcode op0 should be zero for aarch32, the remaining Op and CRn codes
are same. However the codes sent by qemu is matched and register
are handled properly irrespective of AArch32 or AArch64.
I don't have platform which support AArch32 guests to verify.
Actually this is not about the guest, it's about an ARMv8 AArch32 host
that has a GICv3.
I just tried to do a v7 compile with your patches, and it results in an
epic failure, so there's something for you to look at.
Could you please share you config file?. I tried with multi_v7 defconfig with
CONFIG KVM and CONFIG_KVM_ARM_HOST enabled. it compiled for me.
Thanks for shareing the details, I could reproduce them.
However virt/kvm/arm/vgic/vgic-sys-reg-v3.c is written with
sys_regs_desc for AArch64.
For AArch32/v7, it has be to coproc_reg. I propose to add separate file for arm
which handles ICC* reg save/restore using coproc_reg.
That might make sense. In that case they want to be moved into
arch/arm/kvm/ and arch/arm64/kvm/
-Christoffer
From: Christoffer Dall <hidden> Date: 2016-11-21 13:43:11
On Mon, Nov 21, 2016 at 06:56:08PM +0530, Vijay Kilari wrote:
On Mon, Nov 21, 2016 at 3:49 PM, Christoffer Dall
[off-list ref] wrote:
quoted
On Fri, Nov 18, 2016 at 10:28:34PM +0530, Vijay Kilari wrote:
quoted
On Thu, Nov 17, 2016 at 9:39 PM, Christoffer Dall
[off-list ref] wrote:
quoted
On Thu, Nov 17, 2016 at 09:25:59PM +0530, Vijay Kilari wrote:
quoted
On Thu, Nov 17, 2016 at 12:22 AM, Christoffer Dall
[off-list ref] wrote:
quoted
On Fri, Nov 04, 2016 at 04:43:32PM +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 version of VGIC v3 specification is define here
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
Signed-off-by: Pavel Fedin <redacted>
Signed-off-by: Vijaya Kumar K <redacted>
---
arch/arm64/include/uapi/asm/kvm.h | 3 +
arch/arm64/kvm/Makefile | 1 +
include/kvm/arm_vgic.h | 9 +
virt/kvm/arm/vgic/vgic-kvm-device.c | 27 +++
virt/kvm/arm/vgic/vgic-mmio-v3.c | 19 +++
virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 324 ++++++++++++++++++++++++++++++++++++
virt/kvm/arm/vgic/vgic-v3.c | 8 +
virt/kvm/arm/vgic/vgic.h | 4 +
8 files changed, 395 insertions(+)
Thi is making me wonder: Are we properly handling GICv3 save/restore
for AArch32 now that we have GICv3 support for AArch32? By properly I
mean that either it is clearly only supported on AArch64 systems or it's
supported on both AArch64 and AArch32, but it shouldn't break randomly
on AArch32.
It supports both AArch64 and AArch64 in handling of system registers
save/restore.
All system registers that we save/restore are 32-bit for both aarch64
and aarch32.
Though opcode op0 should be zero for aarch32, the remaining Op and CRn codes
are same. However the codes sent by qemu is matched and register
are handled properly irrespective of AArch32 or AArch64.
I don't have platform which support AArch32 guests to verify.
Actually this is not about the guest, it's about an ARMv8 AArch32 host
that has a GICv3.
I just tried to do a v7 compile with your patches, and it results in an
epic failure, so there's something for you to look at.
@@ -23,6 +23,7 @@#include"vgic.h"#include"vgic-mmio.h"+#include"sys_regs.h"/* extract @num bytes at @offset bytes offset in data */unsignedlongextract_bytes(u64data,unsignedintoffset,
@@ -639,6 +640,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;}
hmmm, this looks weird to me, because vgic_v3_cpu->num_pri_bits I don't
understand which effect this is intended to have?
Sure, it may limit what you do with other registers later, but since
there's no ordering requirement that the ctlr be restored first, I'm not
sure it makes sense.
Also, since this field is RO in the ICH_VTR, we'll have a strange
situation during runtime after a GICv3 restore where the
vgic_v3_cpu->num_pri_its differs from the hardware's ICH_VTR_EL2 field,
which is never the case if you didn't do a save/restore.
Yes, but in any case, vgic_v3_cpu->num_pri_bits will be always less
than HW supported
value.
So answer my question: What is the intended effect of writing this
value? Is it just so that if you migrate this platform back again, then
you're checking compatibility with what the guest would potentially do,
Yes
Then add a comment explaining that
quoted
and also to limit the valid aprn registers access as you said above.
But that has ordering restriction. Which I think we should follow.
I'm sorry, now I'm confused. Is there an ordering requirement in the
API, or how should we follow this?
There is no ordering requirement mentioned in the API doc.
However the APRn registers depends on num_pri_bits. Hence first
ICC_CTLR_EL1 should be restored before APRn restore.
If ordering is not followed then APRn registers restore is allowed
as per hw supported num_pri_bits.
This should be mentioned in doc.
How about just having a consistency check function that you call from
uaccess updates to both functions, and in that way avoid requireing any
ordering which is likely to not be followed etc.?
Thanks,
-Christoffer