Re: [PATCH v5 0/7] Add RAS virtualization support to SEA/SEI notification type

[gengdongjiu]

Jonathan,
   Thanks for the review, will correct the typo issue in the next patch version.


On 2017/8/22 15:54, Jonathan Cameron wrote:
> On Fri, 18 Aug 2017 22:11:50 +0800
> Dongjiu Geng  wrote:
> 
>> In the firmware-first RAS solution, corrupt data is detected in a
>> memory location when guest OS application software executing at EL0
>> or guest OS kernel El1 software are reading from the memory. The
>> memory node records errors in an error record accessible using
>> system registers.
>>
>> Because SCR_EL3.EA is 1, then CPU will trap to El3 firmware, EL3
>> firmware records the error to APEI table through reading system
>> register.
>>
>> Because the error was taken from a lower Exception leve, if the
> 
> leve -> level
> 
>> exception is SEA/SEI and HCR_EL2.TEA/HCR_EL2.AMO is 1, firmware
>> sets ESR_EL2/FAR_El to fake a exception trap to EL2, then
>> transfers to hypervisor.
>>
>> Hypervisor calls the momory failure to deal with this error, momory
> 
> momory -> memory
> 
> memory failure -> memory failure function? Or callback perhaps?
> 
>> failure read the APEI table and decide whether it needs to deliver
>> SIGBUS signal to user space, the advantage of using SIGBUS signal
>> to notify user space is that it can be compatible Non-Kvm users.
> 
> Seems like a good description to me. Thanks.
> 
> Jonathan
> 
>>
>> Dongjiu Geng (5):
>>   acpi: apei: Add SEI notification type support for ARMv8
>>   support user space to query RAS extension feature
>>   arm64: kvm: route synchronous external abort exceptions to el2
>>   KVM: arm/arm64: Allow get exception syndrome and
>>   arm64: kvm: handle SEI notification and inject virtual SError
>>
>> James Morse (1):
>>   KVM: arm64: Save ESR_EL2 on guest SError
>>
>> Xie XiuQi (1):
>>   arm64: cpufeature: Detect CPU RAS Extentions
>>
>>  arch/arm/include/asm/kvm_host.h  |  2 ++
>>  arch/arm/kvm/guest.c |  5 +++
>>  arch/arm64/Kconfig   | 16 ++
>>  arch/arm64/include/asm/barrier.h |  1 +
>>  arch/arm64/include/asm/cpucaps.h |  3 +-
>>  arch/arm64/include/asm/kvm_arm.h |  2 ++
>>  arch/arm64/include/asm/kvm_emulate.h | 17 ++
>>  arch/arm64/include/asm/kvm_host.h|  2 ++
>>  arch/arm64/include/asm/sysreg.h  |  5 +++
>>  arch/arm64/include/asm/system_misc.h |  1 +
>>  arch/arm64/include/uapi/asm/kvm.h|  5 +++
>>  arch/arm64/kernel/cpufeature.c   | 13 
>>  arch/arm64/kernel/process.c  |  3 ++
>>  arch/arm64/kvm/guest.c   | 48 +
>>  arch/arm64/kvm/handle_exit.c | 21 +++--
>>  arch/arm64/kvm/hyp/switch.c  | 29 +++--
>>  arch/arm64/kvm/reset.c   |  3 ++
>>  arch/arm64/mm/fault.c| 21 +++--
>>  drivers/acpi/apei/Kconfig| 15 +
>>  drivers/acpi/apei/ghes.c | 60 
>> +++-
>>  include/acpi/ghes.h  |  2 +-
>>  include/uapi/linux/kvm.h |  3 ++
>>  virt/kvm/arm/arm.c   |  7 +
>>  23 files changed, 254 insertions(+), 30 deletions(-)
>>
> 
> 
> .
> 

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 09/27] arm64/sve: Signal frame and context structure definition

[Alex Bennée]

Dave Martin  writes:

> On Tue, Aug 22, 2017 at 11:22:44AM +0100, Alex Bennée wrote:
>>
>> Dave Martin  writes:
>>
>> > This patch defines the representation that will be used for the SVE
>> > register state in the signal frame, and implements support for
>> > saving and restoring the SVE registers around signals.
>> >
>> > The same layout will also be used for the in-kernel task state.
>> >
>> > Due to the variability of the SVE vector length, it is not possible
>> > to define a fixed C struct to describe all the registers.  Instead,
>> > Macros are defined in sigcontext.h to facilitate access to the
>> > parts of the structure.
>> >
>> > Signed-off-by: Dave Martin 
>> > ---
>> >  arch/arm64/include/uapi/asm/sigcontext.h | 113 
>> > ++-
>> >  1 file changed, 112 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/arch/arm64/include/uapi/asm/sigcontext.h 
>> > b/arch/arm64/include/uapi/asm/sigcontext.h
>> > index f0a76b9..0533bdf 100644
>> > --- a/arch/arm64/include/uapi/asm/sigcontext.h
>> > +++ b/arch/arm64/include/uapi/asm/sigcontext.h
>> > @@ -16,6 +16,8 @@
>> >  #ifndef _UAPI__ASM_SIGCONTEXT_H
>> >  #define _UAPI__ASM_SIGCONTEXT_H
>> >
>> > +#ifndef __ASSEMBLY__
>> > +
>> >  #include 
>> >
>> >  /*
>> > @@ -41,10 +43,11 @@ struct sigcontext {
>> >   *
>> >   *0x210   fpsimd_context
>> >   * 0x10   esr_context
>> > + *0x8a0   sve_context (vl <= 64) (optional)
>> >   * 0x20   extra_context (optional)
>> >   * 0x10   terminator (null _aarch64_ctx)
>> >   *
>> > - *0xdb0   (reserved for future allocation)
>> > + *0x510   (reserved for future allocation)
>> >   *
>> >   * New records that can exceed this space need to be opt-in for 
>> > userspace, so
>> >   * that an expanded signal frame is not generated unexpectedly.  The 
>> > mechanism
>> > @@ -116,4 +119,112 @@ struct extra_context {
>> >__u32 __reserved[3];
>> >  };
>> >
>> > +#define SVE_MAGIC 0x53564501
>> > +
>> > +struct sve_context {
>> > +  struct _aarch64_ctx head;
>> > +  __u16 vl;
>> > +  __u16 __reserved[3];
>> > +};
>> > +
>> > +#endif /* !__ASSEMBLY__ */
>> > +
>> > +/*
>> > + * The SVE architecture leaves space for future expansion of the
>> > + * vector length beyond its initial architectural limit of 2048 bits
>> > + * (16 quadwords).
>> > + */
>> > +#define SVE_VQ_MIN1
>> > +#define SVE_VQ_MAX0x200
>> > +
>> > +#define SVE_VL_MIN(SVE_VQ_MIN * 0x10)
>> > +#define SVE_VL_MAX(SVE_VQ_MAX * 0x10)
>> > +
>> > +#define SVE_NUM_ZREGS 32
>> > +#define SVE_NUM_PREGS 16
>> > +
>> > +#define sve_vl_valid(vl) \
>> > +  ((vl) % 0x10 == 0 && (vl) >= SVE_VL_MIN && (vl) <= SVE_VL_MAX)
>> > +#define sve_vq_from_vl(vl)((vl) / 0x10)
>> > +#define sve_vl_from_vq(vq)((vq) * 0x10)
>>
>> I got a little confused first time through over what VQ and VL where.
>> Maybe it would make sense to expand a little more from first principles?
>>
>>   /*
>>* The SVE architecture defines vector registers as a multiple of 128
>>* bit quadwords. The current architectural limit is 2048 bits (16
>>* quadwords) but there is room for future expansion beyond that.
>> */
>
> This comes up in several places and so I didn't want to comment it
> repeatedly everywhere.
>
> Instead, I wrote up something in section 2 (Vector length terminology)
> of Documentation/arm64/sve.txt -- see patch 25.  Can you take a look and
> see whether that's adequate?

Ahh, I hadn't got to that yet. I'm unsure to the order the kernel likes
to put things but I like to put design documents at the front of the
patch queue as they are useful primers and saves you having to patch a:

modified   arch/arm64/include/uapi/asm/sigcontext.h
@@ -132,19 +132,24 @@ struct sve_context {
 /*
  * The SVE architecture leaves space for future expansion of the
  * vector length beyond its initial architectural limit of 2048 bits
- * (16 quadwords).
+ * (16 quadwords). See Documentation/arm64/sve.txt for a summary of
+ * the terminology of Vector Quads (VQ) and Vector Lengths (VL).
  */
+
+#define SVE_VQ_BITS 128  /* 128 bits in one quadword */
+#define SVE_VQ_BYTES(SVE_VQ_BITS / 8)
+
 #define SVE_VQ_MIN 1
 #define SVE_VQ_MAX 0x200

-#define SVE_VL_MIN (SVE_VQ_MIN * 0x10)
-#define SVE_VL_MAX (SVE_VQ_MAX * 0x10)
+#define SVE_VL_MIN (SVE_VQ_MIN * SVE_VQ_BYTES)
+#define SVE_VL_MAX (SVE_VQ_MAX * SVE_VQ_BYTES)

 #define SVE_NUM_ZREGS  32
 #define SVE_NUM_PREGS  16

 #define sve_vl_valid(vl) \
-   ((vl) % 0x10 == 0 && (vl) >= SVE_VL_MIN && (vl) <= SVE_VL_MAX)
+   ((vl) % SVE_VQ_BYTES == 0 && (vl) >= SVE_VL_MIN && (vl) <= SVE_VL_MAX)
 #define sve_vq_from_vl(vl) ((vl) / 0x10)
 #define 

Re: [PATCH 12/27] arm64/sve: Support vector length resetting for new processes

[Dave Martin]

On Tue, Aug 22, 2017 at 05:22:11PM +0100, Alex Bennée wrote:
> 
> Dave Martin  writes:
> 
> > It's desirable to be able to reset the vector length to some sane
> > default for new processes, since the new binary and its libraries
> > processes may or may not be SVE-aware.
> >
> > This patch tracks the desired post-exec vector length (if any) in a
> > new thread member sve_vl_onexec, and adds a new thread flag
> > TIF_SVE_VL_INHERIT to control whether to inherit or reset the
> > vector length.  Currently these are inactive.  Subsequent patches
> > will provide the capability to configure them.
> >
> > Signed-off-by: Dave Martin 
> > ---
> >  arch/arm64/include/asm/processor.h   |  1 +
> >  arch/arm64/include/asm/thread_info.h |  1 +
> >  arch/arm64/kernel/fpsimd.c   | 13 +
> >  3 files changed, 15 insertions(+)
> >
> > diff --git a/arch/arm64/include/asm/processor.h 
> > b/arch/arm64/include/asm/processor.h
> > index 969feed..da8802a 100644
> > --- a/arch/arm64/include/asm/processor.h
> > +++ b/arch/arm64/include/asm/processor.h
> > @@ -87,6 +87,7 @@ struct thread_struct {
> > struct fpsimd_state fpsimd_state;
> > void*sve_state; /* SVE registers, if any */
> > u16 sve_vl; /* SVE vector length */
> > +   u16 sve_vl_onexec;  /* SVE vl after next
> > exec */
> 
> Inconsistent size usage here as well...

Agreed, the same history applies as for sve_vl.  I'll replace this with
an unsigned int.

[...]

Cheers
---Dave
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v3 3/4] kvm: arm/arm64: vgic-v3: add ICH_AP[01]Rn accessors for GICv3

[wanghaibin]

This patch is used for GICv2 on GICv3.

About GICV_APRn hardware register access,the SPEC says:
When System register access is enabled for EL2, these registers access
ICH_AP1Rn_EL2, and all active priorities for virtual machines are held
in ICH_AP1Rn_EL2 regardless of interrupt group.

For GICv3 hardware, we access the active priorities from ICH_AP1Rn_EL2
in this scene.

Aiming at the userspace access the undefined APR registers which the
hardwate doesn't implement, it will be treates ad raz/wi.

Signed-off-by: wanghaibin 
---
 virt/kvm/arm/vgic/vgic-mmio.c | 16 +--
 virt/kvm/arm/vgic/vgic-v3.c   | 48 +++
 virt/kvm/arm/vgic/vgic.h  |  2 ++
 3 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index 80261b7..738d800 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -494,14 +494,26 @@ static int match_region(const void *key, const void *elt)
 
 void vgic_set_apr(struct kvm_vcpu *vcpu, u32 idx, u32 val)
 {
-   if (kvm_vgic_global_state.type == VGIC_V2)
+   u32 vgic_model = vcpu->kvm->arch.vgic.vgic_model;
+
+   if (kvm_vgic_global_state.type == VGIC_V2) {
vgic_v2_set_apr(vcpu, idx, val);
+   } else {
+   if (vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
+   vgic_v3_set_apr(vcpu, 1, idx, val);
+   }
 }
 
 u32 vgic_get_apr(struct kvm_vcpu *vcpu, u32 idx)
 {
-   if (kvm_vgic_global_state.type == VGIC_V2)
+   u32 vgic_model = vcpu->kvm->arch.vgic.vgic_model;
+
+   if (kvm_vgic_global_state.type == VGIC_V2) {
return vgic_v2_get_apr(vcpu, idx);
+   } else {
+   if (vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
+   return vgic_v3_get_apr(vcpu, 1, idx);
+   }
 
return 0;
 }
diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index 96ea597..2625dfd 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -160,6 +160,54 @@ void vgic_v3_clear_lr(struct kvm_vcpu *vcpu, int lr)
vcpu->arch.vgic_cpu.vgic_v3.vgic_lr[lr] = 0;
 }
 
+static bool vgic_v3_apr_access_valid(struct kvm_vcpu *vcpu, u32 idx)
+{
+   struct vgic_cpu *vgic_v3_cpu = >arch.vgic_cpu;
+
+   if (idx > 3)
+   return false;
+
+   switch (vgic_v3_cpu->num_pri_bits) {
+   case 7:
+   return true;
+   case 6:
+   if (idx > 1)
+   return false;
+   break;
+   default:
+   if (idx > 0)
+   return false;
+   }
+
+   return true;
+}
+
+void vgic_v3_set_apr(struct kvm_vcpu *vcpu, u8 group, u32 idx, u32 val)
+{
+   struct vgic_v3_cpu_if *cpu_if = >arch.vgic_cpu.vgic_v3;
+
+   if (!vgic_v3_apr_access_valid(vcpu, idx))
+   return;
+
+   if (group)
+   cpu_if->vgic_ap1r[idx] = val;
+   else
+   cpu_if->vgic_ap0r[idx] = val;
+}
+
+u32 vgic_v3_get_apr(struct kvm_vcpu *vcpu, u8 group, u32 idx)
+{
+   struct vgic_v3_cpu_if *cpu_if = >arch.vgic_cpu.vgic_v3;
+
+   if (!vgic_v3_apr_access_valid(vcpu, idx))
+   return 0;
+
+   if (group)
+   return cpu_if->vgic_ap1r[idx];
+   else
+   return cpu_if->vgic_ap0r[idx];
+}
+
 void vgic_v3_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp)
 {
struct vgic_v3_cpu_if *cpu_if = >arch.vgic_cpu.vgic_v3;
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index 441ded7..19b0f8b 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -181,6 +181,8 @@ static inline void vgic_get_irq_kref(struct vgic_irq *irq)
 void vgic_v3_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr);
 void vgic_v3_clear_lr(struct kvm_vcpu *vcpu, int lr);
 void vgic_v3_set_underflow(struct kvm_vcpu *vcpu);
+void vgic_v3_set_apr(struct kvm_vcpu *vcpu, u8 group, u32 idx, u32 val);
+u32 vgic_v3_get_apr(struct kvm_vcpu *vcpu, u8 group, u32 idx);
 void vgic_v3_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
 void vgic_v3_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
 void vgic_v3_enable(struct kvm_vcpu *vcpu);
-- 
1.8.3.1


___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v3 1/4] kvm: arm/arm64: vgic: Implement the vGICv2 GICC_APRn uaccess interface.

[wanghaibin]

The SPEC defined GICC_APR (n = 0-3, 32-bit per register) to provide
information about interrupt active priorities for GICv2, and the user
access will traverse all of these registers no matter how many
priority levels we supported.

So we have to implement the uaccess interface cover all of these registers.

Signed-off-by: wanghaibin 
---
 virt/kvm/arm/vgic/vgic-mmio-v2.c | 22 --
 virt/kvm/arm/vgic/vgic-mmio.c|  9 +
 virt/kvm/arm/vgic/vgic.h |  3 +++
 3 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index 37522e6..2c1297c 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -303,6 +303,23 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu,
vgic_set_vmcr(vcpu, );
 }
 
+static unsigned long vgic_v2_uaccess_read_activeprio(struct kvm_vcpu *vcpu,
+gpa_t addr, unsigned int 
len)
+{
+   u32 idx = (addr & 0x0c) / sizeof(u32);
+
+   return vgic_get_apr(vcpu, idx);
+}
+
+static void vgic_v2_uaccess_write_activeprio(struct kvm_vcpu *vcpu,
+gpa_t addr, unsigned int len,
+unsigned long val)
+{
+   u32 idx = (addr & 0x0c) / sizeof(u32);
+
+   return vgic_set_apr(vcpu, idx, val);
+}
+
 static const struct vgic_register_region vgic_v2_dist_registers[] = {
REGISTER_DESC_WITH_LENGTH(GIC_DIST_CTRL,
vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
@@ -363,8 +380,9 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu,
REGISTER_DESC_WITH_LENGTH(GIC_CPU_ALIAS_BINPOINT,
vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4,
VGIC_ACCESS_32bit),
-   REGISTER_DESC_WITH_LENGTH(GIC_CPU_ACTIVEPRIO,
-   vgic_mmio_read_raz, vgic_mmio_write_wi, 16,
+   REGISTER_DESC_WITH_LENGTH_UACCESS(GIC_CPU_ACTIVEPRIO,
+   vgic_mmio_read_raz, vgic_mmio_write_wi,
+   vgic_v2_uaccess_read_activeprio, 
vgic_v2_uaccess_write_activeprio, 16,
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_LENGTH(GIC_CPU_IDENT,
vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4,
diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index c1e4bdd..fdf9804 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -492,6 +492,15 @@ static int match_region(const void *key, const void *elt)
   sizeof(regions[0]), match_region);
 }
 
+void vgic_set_apr(struct kvm_vcpu *vcpu, u32 idx, u32 val)
+{
+}
+
+u32 vgic_get_apr(struct kvm_vcpu *vcpu, u32 idx)
+{
+   return 0;
+}
+
 void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
 {
if (kvm_vgic_global_state.type == VGIC_V2)
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index bba7fa2..c2be5b7 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -209,6 +209,9 @@ int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, 
bool is_write, u64 id,
 int vgic_v3_line_level_info_uaccess(struct kvm_vcpu *vcpu, bool is_write,
u32 intid, u64 *val);
 int kvm_register_vgic_device(unsigned long type);
+
+void vgic_set_apr(struct kvm_vcpu *vcpu, u32 idx, u32 val);
+u32 vgic_get_apr(struct kvm_vcpu *vcpu, u32 idx);
 void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
 void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
 int vgic_lazy_init(struct kvm *kvm);
-- 
1.8.3.1


___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v3 2/4] kvm: arm/arm64: vgic-v2: Add GICH_APRn accessors for GICv2

[wanghaibin]

This patch is used for GICv2 on GICv2.

About GICV_APRn hardware register access,the SPEC says:
When System register access is disabled for EL2, these registers access
GICH_APRn, and all active priorities for virtual machines are held in
GICH_APRn regardless of interrupt group.

For GICv2 hardware, there are at most 5 priority bits are implemented in
GICH_LRn.Priority, so we only need to be concerned with GICH_APR0.
The other GICH_APRn (n = 1-3) access should be treated as raz/wi.

Signed-off-by: wanghaibin 
---
 virt/kvm/arm/vgic/vgic-mmio.c |  5 +
 virt/kvm/arm/vgic/vgic-v2.c   | 18 ++
 virt/kvm/arm/vgic/vgic.h  |  2 ++
 3 files changed, 25 insertions(+)

diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index fdf9804..80261b7 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -494,10 +494,15 @@ static int match_region(const void *key, const void *elt)
 
 void vgic_set_apr(struct kvm_vcpu *vcpu, u32 idx, u32 val)
 {
+   if (kvm_vgic_global_state.type == VGIC_V2)
+   vgic_v2_set_apr(vcpu, idx, val);
 }
 
 u32 vgic_get_apr(struct kvm_vcpu *vcpu, u32 idx)
 {
+   if (kvm_vgic_global_state.type == VGIC_V2)
+   return vgic_v2_get_apr(vcpu, idx);
+
return 0;
 }
 
diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
index e4187e5..65daf35 100644
--- a/virt/kvm/arm/vgic/vgic-v2.c
+++ b/virt/kvm/arm/vgic/vgic-v2.c
@@ -172,6 +172,24 @@ void vgic_v2_clear_lr(struct kvm_vcpu *vcpu, int lr)
vcpu->arch.vgic_cpu.vgic_v2.vgic_lr[lr] = 0;
 }
 
+void vgic_v2_set_apr(struct kvm_vcpu *vcpu, u32 idx, u32 val)
+{
+   struct vgic_v2_cpu_if *cpu_if = >arch.vgic_cpu.vgic_v2;
+
+   if (idx == 0)
+   cpu_if->vgic_apr = val;
+}
+
+u32 vgic_v2_get_apr(struct kvm_vcpu *vcpu, u32 idx)
+{
+   struct vgic_v2_cpu_if *cpu_if = >arch.vgic_cpu.vgic_v2;
+
+   if (idx == 0)
+   return cpu_if->vgic_apr;
+   else
+   return 0;
+}
+
 void vgic_v2_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp)
 {
struct vgic_v2_cpu_if *cpu_if = >arch.vgic_cpu.vgic_v2;
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index c2be5b7..441ded7 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -155,6 +155,8 @@ int vgic_v2_dist_uaccess(struct kvm_vcpu *vcpu, bool 
is_write,
 int offset, u32 *val);
 int vgic_v2_cpuif_uaccess(struct kvm_vcpu *vcpu, bool is_write,
  int offset, u32 *val);
+void vgic_v2_set_apr(struct kvm_vcpu *vcpu, u32 idx, u32 val);
+u32 vgic_v2_get_apr(struct kvm_vcpu *vcpu, u32 idx);
 void vgic_v2_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
 void vgic_v2_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
 void vgic_v2_enable(struct kvm_vcpu *vcpu);
-- 
1.8.3.1


___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v3 0/4] kvm: arm/arm64: vgic: APRn uaccess support.

[wanghaibin]

v3: Coding style fix.
Add the valid APRn access check which Marc proposed. 

v2: Split the patch again to make it easier for review
some fixes were proposed by Marc

v1: the problem describe:
In the case (GICv2 on GICv3 migration), I did the test on my board as follow:
vm boot => migrate to destination => shutdown at destination => start at 
destination 
=> migrate back to source ... go round and begin again;

I tested many times, but unlucky, there maybe failed by accident when shutdown 
the vm 
at destination. (GICv3 on GICv3 migration, 1000+ times, That's OK).
while failed,  we can watch the interrupts in the vm, some vcpus of the vm can 
not 
receive the virt timer interrupt. And, these vcpus will 100% with top tool at 
host.

vgic_state debug file at destination shows(a active virt timer interrupt) that:
VCPU 0 TYP   ID TGT_ID PLAEHC HWID   TARGET SRC PRI VCPU_ID
---

   PPI   25  0 0101   0 160  -1
   PPI   26  0 0101   0 160  -1
   PPI   27  0 01   271   0 160   0


I added log to print ICH* registers for VCPU0 at destination:
**HCR:0x1, VMCR:0xf0ec0001,SRE:0x0, ELRSR:0xe**
-AP0R:0: 0x0--
-AP0R:1: 0x0--
-AP0R:2: 0x0--
-AP0R:3: 0x0--
-AP1R:0: 0x0--
-AP1R:1: 0x0--
-AP1R:2: 0x0--
-AP1R:3: 0x0--
-LR:0: 0xa0a0001b001b--
-LR:1: 0x0--
-LR:2: 0x0--
-LR:3: 0x0--

and the ICH_AP1R0 value is 0x1 at source.

At present, QEMU have supproted GICC_APRn put/set interface for emulated GICv2,
and kvm does not support the uaccess interface. This patchset try to support 
this.


wanghaibin (4):
  kvm: arm/arm64: vgic: Implement the vGICv2 GICC_APRn uaccess
interface.
  kvm: arm/arm64: vgic-v2: Add GICH_APRn accessors for GICv2
  kvm: arm/arm64: vgic-v3: add ICH_AP[01]Rn accessors for GICv3
  kvm: arm/arm64: vgic: clean up vGICv3 ICC_APRn sysreg uaccess

 arch/arm64/kvm/vgic-sys-reg-v3.c | 25 +
 virt/kvm/arm/vgic/vgic-mmio-v2.c | 22 --
 virt/kvm/arm/vgic/vgic-mmio.c| 28 +++
 virt/kvm/arm/vgic/vgic-v2.c  | 18 +++
 virt/kvm/arm/vgic/vgic-v3.c  | 48 
 virt/kvm/arm/vgic/vgic.h |  7 ++
 6 files changed, 126 insertions(+), 22 deletions(-)

-- 
1.8.3.1


___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v3 4/4] kvm: arm/arm64: vgic: clean up vGICv3 ICC_APRn sysreg uaccess

[wanghaibin]

It will be better that hide the underlying implementation for emulated GIC.
This patch extend the vgic_set/get_apr func (with group parameter) more general,
so that, the vGICv3 ICC_APRn sysreg uaccess can call this general interface
for GICv3 on GICv3.

Signed-off-by: wanghaibin 
---
 arch/arm64/kvm/vgic-sys-reg-v3.c | 25 +
 virt/kvm/arm/vgic/vgic-mmio-v2.c |  4 ++--
 virt/kvm/arm/vgic/vgic-mmio.c| 10 ++
 virt/kvm/arm/vgic/vgic.h |  4 ++--
 4 files changed, 15 insertions(+), 28 deletions(-)

diff --git a/arch/arm64/kvm/vgic-sys-reg-v3.c b/arch/arm64/kvm/vgic-sys-reg-v3.c
index 116786d..f0d8d36 100644
--- a/arch/arm64/kvm/vgic-sys-reg-v3.c
+++ b/arch/arm64/kvm/vgic-sys-reg-v3.c
@@ -188,23 +188,6 @@ static bool access_gic_grpen1(struct kvm_vcpu *vcpu, 
struct sys_reg_params *p,
return true;
 }
 
-static void vgic_v3_access_apr_reg(struct kvm_vcpu *vcpu,
-  struct sys_reg_params *p, u8 apr, u8 idx)
-{
-   struct vgic_v3_cpu_if *vgicv3 = >arch.vgic_cpu.vgic_v3;
-   uint32_t *ap_reg;
-
-   if (apr)
-   ap_reg = >vgic_ap1r[idx];
-   else
-   ap_reg = >vgic_ap0r[idx];
-
-   if (p->is_write)
-   *ap_reg = p->regval;
-   else
-   p->regval = *ap_reg;
-}
-
 static bool access_gic_aprn(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
const struct sys_reg_desc *r, u8 apr)
 {
@@ -218,19 +201,21 @@ static bool access_gic_aprn(struct kvm_vcpu *vcpu, struct 
sys_reg_params *p,
 */
switch (vgic_v3_cpu->num_pri_bits) {
case 7:
-   vgic_v3_access_apr_reg(vcpu, p, apr, idx);
break;
case 6:
if (idx > 1)
goto err;
-   vgic_v3_access_apr_reg(vcpu, p, apr, idx);
break;
default:
if (idx > 0)
goto err;
-   vgic_v3_access_apr_reg(vcpu, p, apr, idx);
}
 
+   if (p->is_write)
+   vgic_set_apr(vcpu, apr, idx, p->regval);
+   else
+   p->regval = vgic_get_apr(vcpu, apr, idx);
+
return true;
 err:
if (!p->is_write)
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index 2c1297c..373e69f 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -308,7 +308,7 @@ static unsigned long vgic_v2_uaccess_read_activeprio(struct 
kvm_vcpu *vcpu,
 {
u32 idx = (addr & 0x0c) / sizeof(u32);
 
-   return vgic_get_apr(vcpu, idx);
+   return vgic_get_apr(vcpu, 0, idx);
 }
 
 static void vgic_v2_uaccess_write_activeprio(struct kvm_vcpu *vcpu,
@@ -317,7 +317,7 @@ static void vgic_v2_uaccess_write_activeprio(struct 
kvm_vcpu *vcpu,
 {
u32 idx = (addr & 0x0c) / sizeof(u32);
 
-   return vgic_set_apr(vcpu, idx, val);
+   return vgic_set_apr(vcpu, 0, idx, val);
 }
 
 static const struct vgic_register_region vgic_v2_dist_registers[] = {
diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index 738d800..2cc64e4 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -492,7 +492,7 @@ static int match_region(const void *key, const void *elt)
   sizeof(regions[0]), match_region);
 }
 
-void vgic_set_apr(struct kvm_vcpu *vcpu, u32 idx, u32 val)
+void vgic_set_apr(struct kvm_vcpu *vcpu, u8 group, u32 idx, u32 val)
 {
u32 vgic_model = vcpu->kvm->arch.vgic.vgic_model;
 
@@ -501,10 +501,12 @@ void vgic_set_apr(struct kvm_vcpu *vcpu, u32 idx, u32 val)
} else {
if (vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
vgic_v3_set_apr(vcpu, 1, idx, val);
+   else
+   vgic_v3_set_apr(vcpu, group, idx, val);
}
 }
 
-u32 vgic_get_apr(struct kvm_vcpu *vcpu, u32 idx)
+u32 vgic_get_apr(struct kvm_vcpu *vcpu, u8 group, u32 idx)
 {
u32 vgic_model = vcpu->kvm->arch.vgic.vgic_model;
 
@@ -513,9 +515,9 @@ u32 vgic_get_apr(struct kvm_vcpu *vcpu, u32 idx)
} else {
if (vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
return vgic_v3_get_apr(vcpu, 1, idx);
+   else
+   return vgic_v3_get_apr(vcpu, group, idx);
}
-
-   return 0;
 }
 
 void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index 19b0f8b..eb1cd73 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -214,8 +214,8 @@ int vgic_v3_line_level_info_uaccess(struct kvm_vcpu *vcpu, 
bool is_write,
u32 intid, u64 *val);
 int kvm_register_vgic_device(unsigned long type);
 
-void vgic_set_apr(struct kvm_vcpu *vcpu, u32 idx, u32 val);
-u32 vgic_get_apr(struct kvm_vcpu *vcpu, u32 idx);
+void vgic_set_apr(struct kvm_vcpu *vcpu, u8 

Re: [PATCH 09/27] arm64/sve: Signal frame and context structure definition

[Alex Bennée]

Dave Martin  writes:

> On Tue, Aug 22, 2017 at 02:53:49PM +0100, Alex Bennée wrote:
>>
>> Dave Martin  writes:
>>
>> > On Tue, Aug 22, 2017 at 11:22:44AM +0100, Alex Bennée wrote:
>> >>
>> >> Dave Martin  writes:
>
> [...]
>
>> >> > +/*
>> >> > + * The SVE architecture leaves space for future expansion of the
>> >> > + * vector length beyond its initial architectural limit of 2048 bits
>> >> > + * (16 quadwords).
>> >> > + */
>> >> > +#define SVE_VQ_MIN 1
>> >> > +#define SVE_VQ_MAX 0x200
>> >> > +
>> >> > +#define SVE_VL_MIN (SVE_VQ_MIN * 0x10)
>> >> > +#define SVE_VL_MAX (SVE_VQ_MAX * 0x10)
>> >> > +
>> >> > +#define SVE_NUM_ZREGS  32
>> >> > +#define SVE_NUM_PREGS  16
>> >> > +
>> >> > +#define sve_vl_valid(vl) \
>> >> > +   ((vl) % 0x10 == 0 && (vl) >= SVE_VL_MIN && (vl) <= SVE_VL_MAX)
>> >> > +#define sve_vq_from_vl(vl) ((vl) / 0x10)
>> >> > +#define sve_vl_from_vq(vq) ((vq) * 0x10)
>> >>
>> >> I got a little confused first time through over what VQ and VL where.
>> >> Maybe it would make sense to expand a little more from first principles?
>> >>
>> >>   /*
>> >>* The SVE architecture defines vector registers as a multiple of 128
>> >>* bit quadwords. The current architectural limit is 2048 bits (16
>> >>* quadwords) but there is room for future expansion beyond that.
>> >> */
>> >
>> > This comes up in several places and so I didn't want to comment it
>> > repeatedly everywhere.
>> >
>> > Instead, I wrote up something in section 2 (Vector length terminology)
>> > of Documentation/arm64/sve.txt -- see patch 25.  Can you take a look and
>> > see whether that's adequate?
>>
>> Ahh, I hadn't got to that yet. I'm unsure to the order the kernel likes
>> to put things but I like to put design documents at the front of the
>
> I don't have a strong opinion on that -- I had preferred not to add a
> document describing stuff that doesn't exist at the time of commit.
> I could commit a stub document at the start of the series, and then
> commit the real document later.
>
> Either way, it seemed overkill.
>
> Perhaps I should have drawn more attention to the documentation in the
> cover letter, and encouraged reviewers to look at it first.  My
> experience is that people don't often read cover letters...
>
> Now the series is posted, I'm minded to keep the order as-is, unless you
> think it's a big issue.
>
> Adding a reference to the document seems a reasonable thing to do,
> so I could add that.
>
>> patch queue as they are useful primers and saves you having to patch a:
>>
>> modified   arch/arm64/include/uapi/asm/sigcontext.h
>> @@ -132,19 +132,24 @@ struct sve_context {
>>  /*
>>   * The SVE architecture leaves space for future expansion of the
>>   * vector length beyond its initial architectural limit of 2048 bits
>> - * (16 quadwords).
>> + * (16 quadwords). See Documentation/arm64/sve.txt for a summary of
>> + * the terminology of Vector Quads (VQ) and Vector Lengths (VL).
>>   */
>> +
>> +#define SVE_VQ_BITS 128  /* 128 bits in one quadword */
>> +#define SVE_VQ_BYTES(SVE_VQ_BITS / 8)
>> +
>
> I was trying to keep extraneous #defines to a minimum, since this is a
> uapi header, and people may depend on anything defined here.
>
> I think SVE_VQ_BYTES is reasonable to have, and this allows us to
> rewrite a few hard-coded 0x10s and 16s symbolically which is probably a
> good idea -- I'll add this.
>
> SVE_VQ_BITS looks redundant to me though.  It wouldn't be used for any
> purpose other than defining SVE_VQ_BYTES.

Yeah I was more concerned with getting rid of the magic 0x10's than
showing exactly how many bits something is.

>
>>  #define SVE_VQ_MIN  1
>>  #define SVE_VQ_MAX  0x200
>>
>> -#define SVE_VL_MIN  (SVE_VQ_MIN * 0x10)
>> -#define SVE_VL_MAX  (SVE_VQ_MAX * 0x10)
>> +#define SVE_VL_MIN  (SVE_VQ_MIN * SVE_VQ_BYTES)
>> +#define SVE_VL_MAX  (SVE_VQ_MAX * SVE_VQ_BYTES)
>>
>>  #define SVE_NUM_ZREGS   32
>>  #define SVE_NUM_PREGS   16
>>
>>  #define sve_vl_valid(vl) \
>> -((vl) % 0x10 == 0 && (vl) >= SVE_VL_MIN && (vl) <= SVE_VL_MAX)
>> +((vl) % SVE_VQ_BYTES == 0 && (vl) >= SVE_VL_MIN && (vl) <= SVE_VL_MAX)
>>  #define sve_vq_from_vl(vl)  ((vl) / 0x10)
>>  #define sve_vl_from_vq(vq)  ((vq) * 0x10)
>
> [...]
>
> Cheers
> ---Dave


--
Alex Bennée
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 11/27] arm64/sve: Core task context handling

[Dave Martin]

On Tue, Aug 22, 2017 at 05:21:19PM +0100, Alex Bennée wrote:
> 
> Dave Martin  writes:

[...]

> > --- a/arch/arm64/include/asm/processor.h
> > +++ b/arch/arm64/include/asm/processor.h
> > @@ -85,6 +85,8 @@ struct thread_struct {
> > unsigned long   tp2_value;
> >  #endif
> > struct fpsimd_state fpsimd_state;
> > +   void*sve_state; /* SVE registers, if any */
> > +   u16 sve_vl; /* SVE vector length */
> 
> sve_vl is implicitly cast to unsigned int bellow - it should be
> consistent.

Agreed -- I think this can just be unsigned int here, which is the type
I use everywhere except when encoding the vector length in the signal
frame and ptrace data.

During development, there was an additional flags field here (now
merged into the thread_info flags).  u16 was big enough for the largest
architectural vector length, so it seemed "tidy" to make both u16s.
Elsewhere, this created a risk of overflow if you try to compute say
the size of the whole register file from a u16, so I generally used
unsigned int for local variables in functions that handle the vl.

> Given the allocation functions rely on sve_vl being valid it might be
> worth noting where this is set/live from?

Agreed, I need to look more closely at exactly how to justify the
soundness of this in order to thin out BUG_ONs.

If you need any pointers, please ping me.  In the meantime, I would
rather not colour your judgement by infecting you with my assumptions ;)

> > unsigned long   fault_address;  /* fault info */
> > unsigned long   fault_code; /* ESR_EL1 value */
> > struct debug_info   debug;  /* debugging */
> > diff --git a/arch/arm64/include/asm/thread_info.h 
> > b/arch/arm64/include/asm/thread_info.h
> > index 46c3b93..1a4b30b 100644
> > --- a/arch/arm64/include/asm/thread_info.h
> > +++ b/arch/arm64/include/asm/thread_info.h
> 
> 
> And I see there are other comments from Ard.

Yup, I've already picked those up.

I'll be posting a v2 with feedback applied once I've reviewed the
existing BUG_ON()s -- should happen sometime this week.

Cheers
---Dave
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 4/8] KVM: arm/arm64: vgic: restructure kvm_vgic_(un)map_phys_irq

[Auger Eric]

Hi Christoffer,

On 21/07/2017 13:44, Christoffer Dall wrote:
> On Thu, Jun 15, 2017 at 02:52:36PM +0200, Eric Auger wrote:
>> We want to reuse the core of the map/unmap functions for IRQ
>> forwarding. Let's move the computation of the hwirq in
>> kvm_vgic_map_phys_irq and pass the linux IRQ as parameter.
>> the host_irq is added to struct vgic_irq.
>>
>> We introduce kvm_vgic_map/unmap_irq which take a struct vgic_irq
>> handle as a parameter.
> 
> So this is to avoid the linux-to-hardware irq number translation in
> other callers?
Yes that's correct. Also forwarding code caller needs to hold the lock,
reason why I pass a vgic_irq as a param.
> 
> I am sort of guessing that we need to store the host_irq number so that
> we can probe into the physical GIC in later patches?  That may be good
> to note in this commit message if you respin.
done

Thanks

Eric
> 
> Thanks,
> -Christoffer
> 
>>
>> Signed-off-by: Eric Auger 
>> ---
>>  include/kvm/arm_vgic.h|  8 ---
>>  virt/kvm/arm/arch_timer.c | 24 +--
>>  virt/kvm/arm/vgic/vgic.c  | 60 
>> +++
>>  3 files changed, 51 insertions(+), 41 deletions(-)
>>
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index ef71858..cceb31d 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -112,6 +112,7 @@ struct vgic_irq {
>>  bool hw;/* Tied to HW IRQ */
>>  struct kref refcount;   /* Used for LPIs */
>>  u32 hwintid;/* HW INTID number */
>> +unsigned int host_irq;  /* linux irq corresponding to hwintid */
>>  union {
>>  u8 targets; /* GICv2 target VCPUs mask */
>>  u32 mpidr;  /* GICv3 target VCPU */
>> @@ -301,9 +302,10 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, 
>> unsigned int intid,
>>  bool level);
>>  int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid, unsigned int 
>> intid,
>> bool level);
>> -int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 
>> phys_irq);
>> -int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int virt_irq);
>> -bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq);
>> +int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
>> +  u32 vintid);
>> +int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int vintid);
>> +bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int vintid);
>>  
>>  int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
>>  
>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
>> index 5976609..57a30ab 100644
>> --- a/virt/kvm/arm/arch_timer.c
>> +++ b/virt/kvm/arm/arch_timer.c
>> @@ -617,9 +617,6 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
>>  {
>>  struct arch_timer_cpu *timer = >arch.timer_cpu;
>>  struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
>> -struct irq_desc *desc;
>> -struct irq_data *data;
>> -int phys_irq;
>>  int ret;
>>  
>>  if (timer->enabled)
>> @@ -632,26 +629,7 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
>>  if (!vgic_initialized(vcpu->kvm))
>>  return -ENODEV;
>>  
>> -/*
>> - * Find the physical IRQ number corresponding to the host_vtimer_irq
>> - */
>> -desc = irq_to_desc(host_vtimer_irq);
>> -if (!desc) {
>> -kvm_err("%s: no interrupt descriptor\n", __func__);
>> -return -EINVAL;
>> -}
>> -
>> -data = irq_desc_get_irq_data(desc);
>> -while (data->parent_data)
>> -data = data->parent_data;
>> -
>> -phys_irq = data->hwirq;
>> -
>> -/*
>> - * Tell the VGIC that the virtual interrupt is tied to a
>> - * physical interrupt. We do that once per VCPU.
>> - */
>> -ret = kvm_vgic_map_phys_irq(vcpu, vtimer->irq.irq, phys_irq);
>> +ret = kvm_vgic_map_phys_irq(vcpu, host_vtimer_irq, vtimer->irq.irq);
>>  if (ret)
>>  return ret;
>>  
>> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
>> index 83b24d2..075f073 100644
>> --- a/virt/kvm/arm/vgic/vgic.c
>> +++ b/virt/kvm/arm/vgic/vgic.c
>> @@ -17,6 +17,8 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>> +#include 
>>  
>>  #include "vgic.h"
>>  
>> @@ -392,38 +394,66 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, 
>> unsigned int intid,
>>  return 0;
>>  }
>>  
>> -int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq)
>> +/* @irq->irq_lock must be held */
>> +static int kvm_vgic_map_irq(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
>> +unsigned int host_irq)
>>  {
>> -struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, virt_irq);
>> +struct irq_desc *desc;
>> +struct irq_data *data;
>>  
>> -BUG_ON(!irq);
>> -
>> -spin_lock(>irq_lock);
>> +

Re: [PATCH v2 5/8] KVM: arm/arm64: vgic: Handle mapped level sensitive SPIs

[Auger Eric]

Hi Christoffer,

On 21/07/2017 14:11, Christoffer Dall wrote:
> On Thu, Jun 15, 2017 at 02:52:37PM +0200, Eric Auger wrote:
>> Currently, the line level of unmapped level sensitive SPIs is
>> toggled down by the maintenance IRQ handler/resamplefd mechanism.
>>
>> As mapped SPI completion is not trapped, we cannot rely on this
>> mechanism and the line level needs to be observed at distributor
>> level instead.
>>
>> This patch handles the physical IRQ case in vgic_validate_injection
>> and get the line level of a mapped SPI at distributor level.
>>
>> Signed-off-by: Eric Auger 
>>
>> ---
>>
>> v1 -> v2:
>> - renamed is_unshared_mapped into is_mapped_spi
>> - changes to kvm_vgic_map_phys_irq moved in the previous patch
>> - make vgic_validate_injection more readable
>> - reword the commit message
>> ---
>>  virt/kvm/arm/vgic/vgic.c | 16 ++--
>>  virt/kvm/arm/vgic/vgic.h |  7 ++-
>>  2 files changed, 20 insertions(+), 3 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
>> index 075f073..2e35ac7 100644
>> --- a/virt/kvm/arm/vgic/vgic.c
>> +++ b/virt/kvm/arm/vgic/vgic.c
>> @@ -139,6 +139,17 @@ void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq)
>>  kfree(irq);
>>  }
>>  
>> +bool irq_line_level(struct vgic_irq *irq)
>> +{
>> +bool line_level = irq->line_level;
>> +
>> +if (unlikely(is_mapped_spi(irq)))
>> +WARN_ON(irq_get_irqchip_state(irq->host_irq,
>> +  IRQCHIP_STATE_PENDING,
>> +  _level));
>> +return line_level;
>> +}
>> +
>>  /**
>>   * kvm_vgic_target_oracle - compute the target vcpu for an irq
>>   *
>> @@ -236,13 +247,14 @@ static void vgic_sort_ap_list(struct kvm_vcpu *vcpu)
>>  
>>  /*
>>   * Only valid injection if changing level for level-triggered IRQs or for a
>> - * rising edge.
>> + * rising edge. Injection of virtual interrupts associated to physical
>> + * interrupts always is valid.
> 
> why?  I don't remember this now, and that means I probably won't in the
> future either.

Sorry for the late reply.

The life cycle of the physically mapped IRQ is as follows:
- pIRQ becomes pending
- pIRQ is acknowledged by the physical handler and becomes active
- vIRQ gets injected as part of the physical handler chain
  (VFIO->irqfd kvm_vgic_inject_irq for instance). Linux irq cannot
  hit until vIRQ=pIRQ deactivation
- guest deactivates the vIRQ which automatically deactivates the pIRQ


So to me if we are about to vgic_validate_injection() an injection of a
physically mapped vIRQ this means the vgic state is ready to accept it:
previous occurence was deactivated. There cannot be any state
inconsistency around the line_level/level.

Do you agree?

I will add this description at least in the commit message.
> 
> When I look at this now, I'm thinking, if we're not going to change
> anything, why proceed beyond validate injection?

don't catch this one. validation always succeeds and then we further
handle the IRQ.

Thanks

Eric
> 
>>   */
>>  static bool vgic_validate_injection(struct vgic_irq *irq, bool level)
>>  {
>>  switch (irq->config) {
>>  case VGIC_CONFIG_LEVEL:
>> -return irq->line_level != level;
>> +return (irq->line_level != level || 
>> unlikely(is_mapped_spi(irq)));
>>  case VGIC_CONFIG_EDGE:
>>  return level;
>>  }
>> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
>> index bba7fa2..da254ae 100644
>> --- a/virt/kvm/arm/vgic/vgic.h
>> +++ b/virt/kvm/arm/vgic/vgic.h
>> @@ -96,14 +96,19 @@
>>  /* we only support 64 kB translation table page size */
>>  #define KVM_ITS_L1E_ADDR_MASK   GENMASK_ULL(51, 16)
>>  
>> +bool irq_line_level(struct vgic_irq *irq);
>> +
>>  static inline bool irq_is_pending(struct vgic_irq *irq)
>>  {
>>  if (irq->config == VGIC_CONFIG_EDGE)
>>  return irq->pending_latch;
>>  else
>> -return irq->pending_latch || irq->line_level;
>> +return irq->pending_latch || irq_line_level(irq);
>>  }
>>  
>> +#define is_mapped_spi(i) \
>> +((i)->hw && (i)->intid >= VGIC_NR_PRIVATE_IRQS && (i)->intid < 1020)
>> +
> 
> nit: why is this not a static inline ?
> 
>>  /*
>>   * This struct provides an intermediate representation of the fields 
>> contained
>>   * in the GICH_VMCR and ICH_VMCR registers, such that code exporting the GIC
>> -- 
>> 2.5.5
>>
> 
> Thanks,
> -Christoffer
> 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 11/27] arm64/sve: Core task context handling

[Alex Bennée]

Dave Martin  writes:

> This patch adds the core support for switching and managing the SVE
> architectural state of user tasks.
>
> Calls to the existing FPSIMD low-level save/restore functions are
> factored out as new functions task_fpsimd_{save,load}(), since SVE
> now dynamically may or may not need to be handled at these points
> depending on the kernel configuration, hardware features discovered
> at boot, and the runtime state of the task.  To make these
> decisions as fast as possible, const cpucaps are used where
> feasible, via the system_supports_sve() helper.
>
> The SVE registers are only tracked for threads that have explicitly
> used SVE, indicated by the new thread flag TIF_SVE.  Otherwise, the
> FPSIMD view of the architectural state is stored in
> thread.fpsimd_state as usual.
>
> When in use, the SVE registers are not stored directly in
> thread_struct due to their potentially large and variable size.
> Because the task_struct slab allocator must be configured very
> early during kernel boot, it is also tricky to configure it
> correctly to match the maximum vector length provided by the
> hardware, since this depends on examining secondary CPUs as well as
> the primary.  Instead, a pointer sve_state in thread_struct points
> to a dynamically allocated buffer containing the SVE register data,
> and code is added to allocate, duplicate and free this buffer at
> appropriate times.
>
> TIF_SVE is set when taking an SVE access trap from userspace, if
> suitable hardware support has been detected.  This enables SVE for
> the thread: a subsequent return to userspace will disable the trap
> accordingly.  If such a trap is taken without sufficient hardware
> support, SIGILL is sent to the thread instead as if an undefined
> instruction had been executed: this may happen if userspace tries
> to use SVE in a system where not all CPUs support it for example.
>
> The kernel may clear TIF_SVE and disable SVE for the thread
> whenever an explicit syscall is made by userspace, though this is
> considered an optimisation opportunity rather than a deterministic
> guarantee: the kernel may not do this on every syscall, but it is
> permitted to do so.  For backwards compatibility reasons and
> conformance with the spirit of the base AArch64 procedure call
> standard, the subset of the SVE register state that aliases the
> FPSIMD registers is still preserved across a syscall even if this
> happens.
>
> TIF_SVE is also cleared, and SVE disabled, on exec: this is an
> obvious slow path and a hint that we are running a new binary that
> may not use SVE.
>
> Code is added to sync data between thread.fpsimd_state and
> thread.sve_state whenever enabling/disabling SVE, in a manner
> consistent with the SVE architectural programmer's model.
>
> Signed-off-by: Dave Martin 
> ---
>  arch/arm64/include/asm/fpsimd.h  |  19 +++
>  arch/arm64/include/asm/processor.h   |   2 +
>  arch/arm64/include/asm/thread_info.h |   1 +
>  arch/arm64/include/asm/traps.h   |   2 +
>  arch/arm64/kernel/entry.S|  14 +-
>  arch/arm64/kernel/fpsimd.c   | 241 
> ++-
>  arch/arm64/kernel/process.c  |   6 +-
>  arch/arm64/kernel/traps.c|   4 +-
>  8 files changed, 279 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
> index 026a7c7..72090a1 100644
> --- a/arch/arm64/include/asm/fpsimd.h
> +++ b/arch/arm64/include/asm/fpsimd.h
> @@ -20,6 +20,8 @@
>
>  #ifndef __ASSEMBLY__
>
> +#include 
> +
>  /*
>   * FP/SIMD storage area has:
>   *  - FPSR and FPCR
> @@ -72,6 +74,23 @@ extern void sve_load_state(void const *state, u32 const 
> *pfpsr,
>  unsigned long vq_minus_1);
>  extern unsigned int sve_get_vl(void);
>
> +#ifdef CONFIG_ARM64_SVE
> +
> +extern size_t sve_state_size(struct task_struct const *task);
> +
> +extern void sve_alloc(struct task_struct *task);
> +extern void fpsimd_release_thread(struct task_struct *task);
> +extern void fpsimd_dup_sve(struct task_struct *dst,
> +struct task_struct const *src);
> +
> +#else /* ! CONFIG_ARM64_SVE */
> +
> +static void __maybe_unused sve_alloc(struct task_struct *task) { }
> +static void __maybe_unused fpsimd_release_thread(struct task_struct *task) { 
> }
> +static void __maybe_unused fpsimd_dup_sve(struct task_struct *dst,
> +   struct task_struct const *src) { }
> +#endif /* ! CONFIG_ARM64_SVE */
> +
>  /* For use by EFI runtime services calls only */
>  extern void __efi_fpsimd_begin(void);
>  extern void __efi_fpsimd_end(void);
> diff --git a/arch/arm64/include/asm/processor.h 
> b/arch/arm64/include/asm/processor.h
> index b7334f1..969feed 100644
> --- a/arch/arm64/include/asm/processor.h
> +++ b/arch/arm64/include/asm/processor.h
> @@ -85,6 +85,8 @@ struct thread_struct {
>   unsigned long   tp2_value;
>  

Re: [PATCH v2 5/8] KVM: arm/arm64: vgic: Handle mapped level sensitive SPIs

[Auger Eric]

Hi,

On 25/07/2017 17:41, Marc Zyngier wrote:
> On 25/07/17 15:48, Christoffer Dall wrote:
>> On Tue, Jul 25, 2017 at 02:47:55PM +0100, Marc Zyngier wrote:
>>> On 21/07/17 14:03, Christoffer Dall wrote:
 On Fri, Jul 07, 2017 at 09:41:42AM +0200, Auger Eric wrote:
> Hi Marc,
>
> On 04/07/2017 14:15, Marc Zyngier wrote:
>> Hi Eric,
>>
>> On 15/06/17 13:52, Eric Auger wrote:
>>> Currently, the line level of unmapped level sensitive SPIs is
>>> toggled down by the maintenance IRQ handler/resamplefd mechanism.
>>>
>>> As mapped SPI completion is not trapped, we cannot rely on this
>>> mechanism and the line level needs to be observed at distributor
>>> level instead.
>>>
>>> This patch handles the physical IRQ case in vgic_validate_injection
>>> and get the line level of a mapped SPI at distributor level.
>>>
>>> Signed-off-by: Eric Auger 
>>>
>>> ---
>>>
>>> v1 -> v2:
>>> - renamed is_unshared_mapped into is_mapped_spi
>>> - changes to kvm_vgic_map_phys_irq moved in the previous patch
>>> - make vgic_validate_injection more readable
>>> - reword the commit message
>>> ---
>>>  virt/kvm/arm/vgic/vgic.c | 16 ++--
>>>  virt/kvm/arm/vgic/vgic.h |  7 ++-
>>>  2 files changed, 20 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
>>> index 075f073..2e35ac7 100644
>>> --- a/virt/kvm/arm/vgic/vgic.c
>>> +++ b/virt/kvm/arm/vgic/vgic.c
>>> @@ -139,6 +139,17 @@ void vgic_put_irq(struct kvm *kvm, struct vgic_irq 
>>> *irq)
>>> kfree(irq);
>>>  }
>>>  
>>> +bool irq_line_level(struct vgic_irq *irq)
>>> +{
>>> +   bool line_level = irq->line_level;
>>> +
>>> +   if (unlikely(is_mapped_spi(irq)))
>>> +   WARN_ON(irq_get_irqchip_state(irq->host_irq,
>>> + IRQCHIP_STATE_PENDING,
>>> + _level));
>>> +   return line_level;
>>> +}
>>> +
>>>  /**
>>>   * kvm_vgic_target_oracle - compute the target vcpu for an irq
>>>   *
>>> @@ -236,13 +247,14 @@ static void vgic_sort_ap_list(struct kvm_vcpu 
>>> *vcpu)
>>>  
>>>  /*
>>>   * Only valid injection if changing level for level-triggered IRQs or 
>>> for a
>>> - * rising edge.
>>> + * rising edge. Injection of virtual interrupts associated to physical
>>> + * interrupts always is valid.
>>>   */
>>>  static bool vgic_validate_injection(struct vgic_irq *irq, bool level)
>>>  {
>>> switch (irq->config) {
>>> case VGIC_CONFIG_LEVEL:
>>> -   return irq->line_level != level;
>>> +   return (irq->line_level != level || 
>>> unlikely(is_mapped_spi(irq)));
>>> case VGIC_CONFIG_EDGE:
>>> return level;
>>> }
>>> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
>>> index bba7fa2..da254ae 100644
>>> --- a/virt/kvm/arm/vgic/vgic.h
>>> +++ b/virt/kvm/arm/vgic/vgic.h
>>> @@ -96,14 +96,19 @@
>>>  /* we only support 64 kB translation table page size */
>>>  #define KVM_ITS_L1E_ADDR_MASK  GENMASK_ULL(51, 16)
>>>  
>>> +bool irq_line_level(struct vgic_irq *irq);
>>> +
>>>  static inline bool irq_is_pending(struct vgic_irq *irq)
>>>  {
>>> if (irq->config == VGIC_CONFIG_EDGE)
>>> return irq->pending_latch;
>>> else
>>> -   return irq->pending_latch || irq->line_level;
>>> +   return irq->pending_latch || irq_line_level(irq);
>>
>> I'm a bit concerned that an edge interrupt doesn't take the distributor
>> state into account here. Why is that so? Once an SPI is forwarded to a
>> guest, a large part of the edge vs level differences move into the HW,
>> and are not that different anymore from a SW PoV.
>
> As pointed out by Christoffer in https://lkml.org/lkml/2017/6/8/322,
> isn't it a bit risky in general to poke the physical state instead of
> the virtual state. For level sensitive, to me we don't really have many
> other alternatives. For edge, we are not obliged to.

 I think we need to be clear on the fundamental question of whether or
 not we consider pending_latch and/or line_level for mapped interrupts.

 I can definitely see the argument that the pending state is kept in
 hardware, so if you want to know that for a mapped interrupt, ask the
 hardware.

 The upside of this appraoch is a clean separation of state and we avoid
 any logic to synchronize a virtual state with the physical state.

 The downside is that it's slower to peek into the physical GIC than to
 read a variable from memory, and 

Re: [PATCH 09/27] arm64/sve: Signal frame and context structure definition

[Dave Martin]

On Tue, Aug 22, 2017 at 04:03:20PM +0100, Alex Bennée wrote:
> 
> Dave Martin  writes:
> 
> > On Tue, Aug 22, 2017 at 02:53:49PM +0100, Alex Bennée wrote:

[...]

> >> +
> >> +#define SVE_VQ_BITS 128  /* 128 bits in one quadword */
> >> +#define SVE_VQ_BYTES(SVE_VQ_BITS / 8)
> >> +
> >
> > I was trying to keep extraneous #defines to a minimum, since this is a
> > uapi header, and people may depend on anything defined here.
> >
> > I think SVE_VQ_BYTES is reasonable to have, and this allows us to
> > rewrite a few hard-coded 0x10s and 16s symbolically which is probably a
> > good idea -- I'll add this.
> >
> > SVE_VQ_BITS looks redundant to me though.  It wouldn't be used for any
> > purpose other than defining SVE_VQ_BYTES.
> 
> Yeah I was more concerned with getting rid of the magic 0x10's than
> showing exactly how many bits something is.

OK, I'll take SVE_VQ_BYTES and use it in the appropriate places.
There are a few 0x10s/16s in the series that can use this instead
of being open-coded.

Cheers
---Dave
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 09/27] arm64/sve: Signal frame and context structure definition

[Dave Martin]

On Tue, Aug 22, 2017 at 02:53:49PM +0100, Alex Bennée wrote:
> 
> Dave Martin  writes:
> 
> > On Tue, Aug 22, 2017 at 11:22:44AM +0100, Alex Bennée wrote:
> >>
> >> Dave Martin  writes:

[...]

> >> > +/*
> >> > + * The SVE architecture leaves space for future expansion of the
> >> > + * vector length beyond its initial architectural limit of 2048 bits
> >> > + * (16 quadwords).
> >> > + */
> >> > +#define SVE_VQ_MIN  1
> >> > +#define SVE_VQ_MAX  0x200
> >> > +
> >> > +#define SVE_VL_MIN  (SVE_VQ_MIN * 0x10)
> >> > +#define SVE_VL_MAX  (SVE_VQ_MAX * 0x10)
> >> > +
> >> > +#define SVE_NUM_ZREGS   32
> >> > +#define SVE_NUM_PREGS   16
> >> > +
> >> > +#define sve_vl_valid(vl) \
> >> > +((vl) % 0x10 == 0 && (vl) >= SVE_VL_MIN && (vl) <= SVE_VL_MAX)
> >> > +#define sve_vq_from_vl(vl)  ((vl) / 0x10)
> >> > +#define sve_vl_from_vq(vq)  ((vq) * 0x10)
> >>
> >> I got a little confused first time through over what VQ and VL where.
> >> Maybe it would make sense to expand a little more from first principles?
> >>
> >>   /*
> >>* The SVE architecture defines vector registers as a multiple of 128
> >>* bit quadwords. The current architectural limit is 2048 bits (16
> >>* quadwords) but there is room for future expansion beyond that.
> >> */
> >
> > This comes up in several places and so I didn't want to comment it
> > repeatedly everywhere.
> >
> > Instead, I wrote up something in section 2 (Vector length terminology)
> > of Documentation/arm64/sve.txt -- see patch 25.  Can you take a look and
> > see whether that's adequate?
> 
> Ahh, I hadn't got to that yet. I'm unsure to the order the kernel likes
> to put things but I like to put design documents at the front of the

I don't have a strong opinion on that -- I had preferred not to add a
document describing stuff that doesn't exist at the time of commit.
I could commit a stub document at the start of the series, and then
commit the real document later.

Either way, it seemed overkill.

Perhaps I should have drawn more attention to the documentation in the
cover letter, and encouraged reviewers to look at it first.  My
experience is that people don't often read cover letters...

Now the series is posted, I'm minded to keep the order as-is, unless you
think it's a big issue.

Adding a reference to the document seems a reasonable thing to do,
so I could add that.

> patch queue as they are useful primers and saves you having to patch a:
> 
> modified   arch/arm64/include/uapi/asm/sigcontext.h
> @@ -132,19 +132,24 @@ struct sve_context {
>  /*
>   * The SVE architecture leaves space for future expansion of the
>   * vector length beyond its initial architectural limit of 2048 bits
> - * (16 quadwords).
> + * (16 quadwords). See Documentation/arm64/sve.txt for a summary of
> + * the terminology of Vector Quads (VQ) and Vector Lengths (VL).
>   */
> +
> +#define SVE_VQ_BITS 128  /* 128 bits in one quadword */
> +#define SVE_VQ_BYTES(SVE_VQ_BITS / 8)
> +

I was trying to keep extraneous #defines to a minimum, since this is a
uapi header, and people may depend on anything defined here.

I think SVE_VQ_BYTES is reasonable to have, and this allows us to
rewrite a few hard-coded 0x10s and 16s symbolically which is probably a
good idea -- I'll add this.

SVE_VQ_BITS looks redundant to me though.  It wouldn't be used for any
purpose other than defining SVE_VQ_BYTES.

>  #define SVE_VQ_MIN   1
>  #define SVE_VQ_MAX   0x200
> 
> -#define SVE_VL_MIN   (SVE_VQ_MIN * 0x10)
> -#define SVE_VL_MAX   (SVE_VQ_MAX * 0x10)
> +#define SVE_VL_MIN   (SVE_VQ_MIN * SVE_VQ_BYTES)
> +#define SVE_VL_MAX   (SVE_VQ_MAX * SVE_VQ_BYTES)
> 
>  #define SVE_NUM_ZREGS32
>  #define SVE_NUM_PREGS16
> 
>  #define sve_vl_valid(vl) \
> - ((vl) % 0x10 == 0 && (vl) >= SVE_VL_MIN && (vl) <= SVE_VL_MAX)
> + ((vl) % SVE_VQ_BYTES == 0 && (vl) >= SVE_VL_MIN && (vl) <= SVE_VL_MAX)
>  #define sve_vq_from_vl(vl)   ((vl) / 0x10)
>  #define sve_vl_from_vq(vq)   ((vq) * 0x10)

[...]

Cheers
---Dave
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 10/27] arm64/sve: Low-level CPU setup

[Dave Martin]

On Tue, Aug 22, 2017 at 04:04:28PM +0100, Alex Bennée wrote:
> 
> Dave Martin  writes:
> 
> > To enable the kernel to use SVE, all SVE traps from EL1 must be
> > disabled.  To take maximum advantage of the hardware, the full
> > available vector length also needs to be enabled for EL1 by
> > programming ZCR_EL2.LEN.  (The kernel will program ZCR_EL1.LEN as
> > required, but this cannot override the limit set by ZCR_EL2.)
> >
> > In advance of full SVE support being implemented for userspace, it
> > also necessary to ensure that SVE traps from EL0 are enabled.
> >
> > This patch makes the appropriate changes to the primary and
> > secondary CPU initialisation code.
> >
> > Signed-off-by: Dave Martin 
> > ---
> >  arch/arm64/kernel/head.S | 13 -
> >  arch/arm64/mm/proc.S | 14 --
> >  2 files changed, 24 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> > index 973df7d..0ae1713 100644
> > --- a/arch/arm64/kernel/head.S
> > +++ b/arch/arm64/kernel/head.S
> > @@ -514,8 +514,19 @@ CPU_LE(movkx0, #0x30d0, lsl #16)   
> > // Clear EE and E0E on LE systems
> > mov x0, #0x33ff
> > msr cptr_el2, x0// Disable copro. traps to EL2
> >
> > +   /* SVE register access */
> > +   mrs x1, id_aa64pfr0_el1
> > +   ubfxx1, x1, #ID_AA64PFR0_SVE_SHIFT, #4
> > +   cbz x1, 7f
> > +
> > +   bic x0, x0, #CPTR_EL2_TZ// Also disable SVE traps
> > +   msr cptr_el2, x0// Disable copro. traps
> > to EL2
> 
> It seems a shame to write to cptr_el2 twice rather than compute and
> write.
> 
> > +   isb
> 
> Especially as the second one needs an isb :-/
> 
> But I don't see a much neater way of doing it so:
> 
> Reviewed-by: Alex Bennée 

Thanks

Originally I think I did merge the CPTR writes here, but since this is
slow-path anyway, I figured it was better to keep the code simple rather
than introducing new interdependencies.

I could revisit it if someone has a strong view.

Cheers
---Dave

> 
> > +   mov x1, #ZCR_ELx_LEN_MASK   // SVE: Enable full vector
> > +   msr_s   SYS_ZCR_EL2, x1 // length for EL1.
> > +
> > /* Hypervisor stub */
> > -   adr_l   x0, __hyp_stub_vectors
> > +7: adr_l   x0, __hyp_stub_vectors
> > msr vbar_el2, x0

[...]

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 12/27] arm64/sve: Support vector length resetting for new processes

[Alex Bennée]

Dave Martin  writes:

> It's desirable to be able to reset the vector length to some sane
> default for new processes, since the new binary and its libraries
> processes may or may not be SVE-aware.
>
> This patch tracks the desired post-exec vector length (if any) in a
> new thread member sve_vl_onexec, and adds a new thread flag
> TIF_SVE_VL_INHERIT to control whether to inherit or reset the
> vector length.  Currently these are inactive.  Subsequent patches
> will provide the capability to configure them.
>
> Signed-off-by: Dave Martin 
> ---
>  arch/arm64/include/asm/processor.h   |  1 +
>  arch/arm64/include/asm/thread_info.h |  1 +
>  arch/arm64/kernel/fpsimd.c   | 13 +
>  3 files changed, 15 insertions(+)
>
> diff --git a/arch/arm64/include/asm/processor.h 
> b/arch/arm64/include/asm/processor.h
> index 969feed..da8802a 100644
> --- a/arch/arm64/include/asm/processor.h
> +++ b/arch/arm64/include/asm/processor.h
> @@ -87,6 +87,7 @@ struct thread_struct {
>   struct fpsimd_state fpsimd_state;
>   void*sve_state; /* SVE registers, if any */
>   u16 sve_vl; /* SVE vector length */
> + u16 sve_vl_onexec;  /* SVE vl after next
> exec */

Inconsistent size usage here as well...

>   unsigned long   fault_address;  /* fault info */
>   unsigned long   fault_code; /* ESR_EL1 value */
>   struct debug_info   debug;  /* debugging */
> diff --git a/arch/arm64/include/asm/thread_info.h 
> b/arch/arm64/include/asm/thread_info.h
> index 1a4b30b..bf9c552 100644
> --- a/arch/arm64/include/asm/thread_info.h
> +++ b/arch/arm64/include/asm/thread_info.h
> @@ -97,6 +97,7 @@ struct thread_info {
>  #define TIF_SINGLESTEP   21
>  #define TIF_32BIT22  /* 32bit process */
>  #define TIF_SVE  23  /* Scalable Vector Extension in 
> use */
> +#define TIF_SVE_VL_INHERIT   24  /* Inherit sve_vl_onexec across exec */
>
>  #define _TIF_SIGPENDING  (1 << TIF_SIGPENDING)
>  #define _TIF_NEED_RESCHED(1 << TIF_NEED_RESCHED)
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 37dd1b2..80ecb2d 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -108,6 +108,9 @@ static void task_fpsimd_save(void);
>   */
>  static DEFINE_PER_CPU(struct fpsimd_state *, fpsimd_last_state);
>
> +/* Default VL for tasks that don't set it explicitly: */
> +static int sve_default_vl = -1;
> +
>  static void sve_free(struct task_struct *task)
>  {
>   kfree(task->thread.sve_state);
> @@ -392,6 +395,9 @@ void fpsimd_flush_thread(void)
>   clear_thread_flag(TIF_SVE);
>   sve_free(current);
>
> + current->thread.sve_vl = current->thread.sve_vl_onexec ?
> + current->thread.sve_vl_onexec : sve_default_vl;
> +
>   /*
>* User tasks must have a valid vector length set, but tasks
>* forked early (e.g., init) may not initially have one.
> @@ -401,6 +407,13 @@ void fpsimd_flush_thread(void)
>* If not, something went badly wrong.
>*/
>   BUG_ON(!sve_vl_valid(current->thread.sve_vl));
> +
> + /*
> +  * If the task is not set to inherit, ensure that the vector
> +  * length will be reset by a subsequent exec:
> +  */
> + if (!test_thread_flag(TIF_SVE_VL_INHERIT))
> + current->thread.sve_vl_onexec = 0;
>   }
>
>   set_thread_flag(TIF_FOREIGN_FPSTATE);


--
Alex Bennée
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 10/27] arm64/sve: Low-level CPU setup

[Alex Bennée]

Dave Martin  writes:

> To enable the kernel to use SVE, all SVE traps from EL1 must be
> disabled.  To take maximum advantage of the hardware, the full
> available vector length also needs to be enabled for EL1 by
> programming ZCR_EL2.LEN.  (The kernel will program ZCR_EL1.LEN as
> required, but this cannot override the limit set by ZCR_EL2.)
>
> In advance of full SVE support being implemented for userspace, it
> also necessary to ensure that SVE traps from EL0 are enabled.
>
> This patch makes the appropriate changes to the primary and
> secondary CPU initialisation code.
>
> Signed-off-by: Dave Martin 
> ---
>  arch/arm64/kernel/head.S | 13 -
>  arch/arm64/mm/proc.S | 14 --
>  2 files changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index 973df7d..0ae1713 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -514,8 +514,19 @@ CPU_LE(  movkx0, #0x30d0, lsl #16)   // 
> Clear EE and E0E on LE systems
>   mov x0, #0x33ff
>   msr cptr_el2, x0// Disable copro. traps to EL2
>
> + /* SVE register access */
> + mrs x1, id_aa64pfr0_el1
> + ubfxx1, x1, #ID_AA64PFR0_SVE_SHIFT, #4
> + cbz x1, 7f
> +
> + bic x0, x0, #CPTR_EL2_TZ// Also disable SVE traps
> + msr cptr_el2, x0// Disable copro. traps
> to EL2

It seems a shame to write to cptr_el2 twice rather than compute and
write.

> + isb

Especially as the second one needs an isb :-/

But I don't see a much neater way of doing it so:

Reviewed-by: Alex Bennée 

> + mov x1, #ZCR_ELx_LEN_MASK   // SVE: Enable full vector
> + msr_s   SYS_ZCR_EL2, x1 // length for EL1.
> +
>   /* Hypervisor stub */
> - adr_l   x0, __hyp_stub_vectors
> +7:   adr_l   x0, __hyp_stub_vectors
>   msr vbar_el2, x0
>
>   /* spsr */
> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
> index 877d42f..dd22ef2 100644
> --- a/arch/arm64/mm/proc.S
> +++ b/arch/arm64/mm/proc.S
> @@ -27,6 +27,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #ifdef CONFIG_ARM64_64K_PAGES
>  #define TCR_TG_FLAGS TCR_TG0_64K | TCR_TG1_64K
> @@ -186,8 +187,17 @@ ENTRY(__cpu_setup)
>   tlbivmalle1 // Invalidate local TLB
>   dsb nsh
>
> - mov x0, #3 << 20
> - msr cpacr_el1, x0   // Enable FP/ASIMD
> + mov x0, #3 << 20// FEN
> +
> + /* SVE */
> + mrs x5, id_aa64pfr0_el1
> + ubfxx5, x5, #ID_AA64PFR0_SVE_SHIFT, #4
> + cbz x5, 1f
> +
> + bic x0, x0, #CPACR_EL1_ZEN
> + orr x0, x0, #CPACR_EL1_ZEN_EL1EN// SVE: trap for EL0, not EL1
> +1:   msr cpacr_el1, x0   // Enable FP/ASIMD
> +
>   mov x0, #1 << 12// Reset mdscr_el1 and disable
>   msr mdscr_el1, x0   // access to the DCC from EL0
>   isb // Unmask debug exceptions now,


--
Alex Bennée
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 11/27] arm64/sve: Core task context handling

[Alex Bennée]

Dave Martin  writes:

> On Tue, Aug 22, 2017 at 05:21:19PM +0100, Alex Bennée wrote:
>>
>> Dave Martin  writes:
>
> [...]
>
>> > --- a/arch/arm64/include/asm/processor.h
>> > +++ b/arch/arm64/include/asm/processor.h
>> > @@ -85,6 +85,8 @@ struct thread_struct {
>> >unsigned long   tp2_value;
>> >  #endif
>> >struct fpsimd_state fpsimd_state;
>> > +  void*sve_state; /* SVE registers, if any */
>> > +  u16 sve_vl; /* SVE vector length */
>>
>> sve_vl is implicitly cast to unsigned int bellow - it should be
>> consistent.
>
> Agreed -- I think this can just be unsigned int here, which is the type
> I use everywhere except when encoding the vector length in the signal
> frame and ptrace data.
>
> During development, there was an additional flags field here (now
> merged into the thread_info flags).  u16 was big enough for the largest
> architectural vector length, so it seemed "tidy" to make both u16s.
> Elsewhere, this created a risk of overflow if you try to compute say
> the size of the whole register file from a u16, so I generally used
> unsigned int for local variables in functions that handle the vl.
>
>> Given the allocation functions rely on sve_vl being valid it might be
>> worth noting where this is set/live from?
>
> Agreed, I need to look more closely at exactly how to justify the
> soundness of this in order to thin out BUG_ONs.
>
> If you need any pointers, please ping me.  In the meantime, I would
> rather not colour your judgement by infecting you with my assumptions ;)
>
>> >unsigned long   fault_address;  /* fault info */
>> >unsigned long   fault_code; /* ESR_EL1 value */
>> >struct debug_info   debug;  /* debugging */
>> > diff --git a/arch/arm64/include/asm/thread_info.h 
>> > b/arch/arm64/include/asm/thread_info.h
>> > index 46c3b93..1a4b30b 100644
>> > --- a/arch/arm64/include/asm/thread_info.h
>> > +++ b/arch/arm64/include/asm/thread_info.h
>> 
>>
>> And I see there are other comments from Ard.
>
> Yup, I've already picked those up.
>
> I'll be posting a v2 with feedback applied once I've reviewed the
> existing BUG_ON()s -- should happen sometime this week.

We shall see how far I get tomorrow - you won't get any more from me
after that until I get back from holiday so don't delay v2 on my account
;-)

>
> Cheers
> ---Dave


--
Alex Bennée
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v5 0/7] Add RAS virtualization support to SEA/SEI notification type

[Jonathan Cameron]

On Fri, 18 Aug 2017 22:11:50 +0800
Dongjiu Geng  wrote:

> In the firmware-first RAS solution, corrupt data is detected in a
> memory location when guest OS application software executing at EL0
> or guest OS kernel El1 software are reading from the memory. The
> memory node records errors in an error record accessible using
> system registers.
> 
> Because SCR_EL3.EA is 1, then CPU will trap to El3 firmware, EL3
> firmware records the error to APEI table through reading system
> register.
> 
> Because the error was taken from a lower Exception leve, if the

leve -> level

> exception is SEA/SEI and HCR_EL2.TEA/HCR_EL2.AMO is 1, firmware
> sets ESR_EL2/FAR_El to fake a exception trap to EL2, then
> transfers to hypervisor.
> 
> Hypervisor calls the momory failure to deal with this error, momory

momory -> memory

memory failure -> memory failure function? Or callback perhaps?

> failure read the APEI table and decide whether it needs to deliver
> SIGBUS signal to user space, the advantage of using SIGBUS signal
> to notify user space is that it can be compatible Non-Kvm users.

Seems like a good description to me. Thanks.

Jonathan

> 
> Dongjiu Geng (5):
>   acpi: apei: Add SEI notification type support for ARMv8
>   support user space to query RAS extension feature
>   arm64: kvm: route synchronous external abort exceptions to el2
>   KVM: arm/arm64: Allow get exception syndrome and
>   arm64: kvm: handle SEI notification and inject virtual SError
> 
> James Morse (1):
>   KVM: arm64: Save ESR_EL2 on guest SError
> 
> Xie XiuQi (1):
>   arm64: cpufeature: Detect CPU RAS Extentions
> 
>  arch/arm/include/asm/kvm_host.h  |  2 ++
>  arch/arm/kvm/guest.c |  5 +++
>  arch/arm64/Kconfig   | 16 ++
>  arch/arm64/include/asm/barrier.h |  1 +
>  arch/arm64/include/asm/cpucaps.h |  3 +-
>  arch/arm64/include/asm/kvm_arm.h |  2 ++
>  arch/arm64/include/asm/kvm_emulate.h | 17 ++
>  arch/arm64/include/asm/kvm_host.h|  2 ++
>  arch/arm64/include/asm/sysreg.h  |  5 +++
>  arch/arm64/include/asm/system_misc.h |  1 +
>  arch/arm64/include/uapi/asm/kvm.h|  5 +++
>  arch/arm64/kernel/cpufeature.c   | 13 
>  arch/arm64/kernel/process.c  |  3 ++
>  arch/arm64/kvm/guest.c   | 48 +
>  arch/arm64/kvm/handle_exit.c | 21 +++--
>  arch/arm64/kvm/hyp/switch.c  | 29 +++--
>  arch/arm64/kvm/reset.c   |  3 ++
>  arch/arm64/mm/fault.c| 21 +++--
>  drivers/acpi/apei/Kconfig| 15 +
>  drivers/acpi/apei/ghes.c | 60 
> +++-
>  include/acpi/ghes.h  |  2 +-
>  include/uapi/linux/kvm.h |  3 ++
>  virt/kvm/arm/arm.c   |  7 +
>  23 files changed, 254 insertions(+), 30 deletions(-)
> 

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v5 1/7] arm64: cpufeature: Detect CPU RAS Extentions

[Jonathan Cameron]

On Fri, 18 Aug 2017 22:11:51 +0800
Dongjiu Geng  wrote:

> From: Xie XiuQi 
> 
> ARM's v8.2 Extentions add support for Reliability, Availability and
> Serviceability (RAS). On CPUs with these extensions system software

extensions, system software

> can use additional barriers to isolate errors and determine if faults
> are pending.
> 
> Add cpufeature detection and a barrier in the context-switch code.
> There is no need to use alternatives for this as CPUs that don't
> support this feature will treat the instruction as a nop.
> 
> Platform level RAS support may require additional firmware support.
> 
> Signed-off-by: Xie XiuQi 
> [Rebased, added esb and config option, reworded commit message]
> Signed-off-by: James Morse 
> ---
>  arch/arm64/Kconfig   | 16 
>  arch/arm64/include/asm/barrier.h |  1 +
>  arch/arm64/include/asm/cpucaps.h |  3 ++-
>  arch/arm64/include/asm/sysreg.h  |  2 ++
>  arch/arm64/kernel/cpufeature.c   | 13 +
>  arch/arm64/kernel/process.c  |  3 +++
>  6 files changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index dfd908630631..4d87aa963d83 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -960,6 +960,22 @@ config ARM64_UAO
> regular load/store instructions if the cpu does not implement the
> feature.
>  
> +config ARM64_RAS_EXTN
> + bool "Enable support for RAS CPU Extensions"
> + default y
> + help
> +   CPUs that support the Reliability, Availability and Serviceability
> +   (RAS) Extensions, part of ARMv8.2 are able to track faults and
> +   errors, classify them and report them to software.
> +
> +   On CPUs with these extensions system software can use additional
> +   barriers to determine if faults are pending and read the
> +   classification from a new set of registers.
> +
> +   Selecting this feature will allow the kernel to use these barriers
> +   and access the new registers if the system supports the extension.
> +   Platform RAS features may additionally depend on firmware support.
> +
>  endmenu
>  
>  config ARM64_MODULE_CMODEL_LARGE
> diff --git a/arch/arm64/include/asm/barrier.h 
> b/arch/arm64/include/asm/barrier.h
> index 0fe7e43b7fbc..8b0a0eb67625 100644
> --- a/arch/arm64/include/asm/barrier.h
> +++ b/arch/arm64/include/asm/barrier.h
> @@ -30,6 +30,7 @@
>  #define isb()asm volatile("isb" : : : "memory")
>  #define dmb(opt) asm volatile("dmb " #opt : : : "memory")
>  #define dsb(opt) asm volatile("dsb " #opt : : : "memory")
> +#define esb()asm volatile("hint #16"  : : : "memory")
>  
>  #define mb() dsb(sy)
>  #define rmb()dsb(ld)
> diff --git a/arch/arm64/include/asm/cpucaps.h 
> b/arch/arm64/include/asm/cpucaps.h
> index 8d2272c6822c..f93bf77f1f74 100644
> --- a/arch/arm64/include/asm/cpucaps.h
> +++ b/arch/arm64/include/asm/cpucaps.h
> @@ -39,7 +39,8 @@
>  #define ARM64_WORKAROUND_QCOM_FALKOR_E1003   18
>  #define ARM64_WORKAROUND_858921  19
>  #define ARM64_WORKAROUND_CAVIUM_3011520
> +#define ARM64_HAS_RAS_EXTN   21
>  
> -#define ARM64_NCAPS  21
> +#define ARM64_NCAPS  22
>  
>  #endif /* __ASM_CPUCAPS_H */
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 248339e4aaf5..35b786b43ee4 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -331,6 +331,7 @@
>  #define ID_AA64ISAR1_JSCVT_SHIFT 12
>  
>  /* id_aa64pfr0 */
> +#define ID_AA64PFR0_RAS_SHIFT28
>  #define ID_AA64PFR0_GIC_SHIFT24
>  #define ID_AA64PFR0_ASIMD_SHIFT  20
>  #define ID_AA64PFR0_FP_SHIFT 16
> @@ -339,6 +340,7 @@
>  #define ID_AA64PFR0_EL1_SHIFT4
>  #define ID_AA64PFR0_EL0_SHIFT0
>  
> +#define ID_AA64PFR0_RAS_V1   0x1
>  #define ID_AA64PFR0_FP_NI0xf
>  #define ID_AA64PFR0_FP_SUPPORTED 0x0
>  #define ID_AA64PFR0_ASIMD_NI 0xf
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 9f9e0064c8c1..a807ab55ee10 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -124,6 +124,7 @@ static const struct arm64_ftr_bits ftr_id_aa64isar1[] = {
>  };
>  
>  static const struct arm64_ftr_bits ftr_id_aa64pfr0[] = {
> + ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_EXACT, 
> ID_AA64PFR0_RAS_SHIFT, 4, 0),
>   ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_EXACT, 
> ID_AA64PFR0_GIC_SHIFT, 4, 0),
>   S_ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, 
> ID_AA64PFR0_ASIMD_SHIFT, 4, ID_AA64PFR0_ASIMD_NI),
>   S_ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, 
> ID_AA64PFR0_FP_SHIFT, 4, ID_AA64PFR0_FP_NI),
> 

Re: [PATCH v5 7/7] arm64: kvm: handle SEI notification and inject virtual SError

[Jonathan Cameron]

On Fri, 18 Aug 2017 22:11:57 +0800
Dongjiu Geng  wrote:

> After receive SError, KVM firstly call memory failure to
> deal with the Error. If memory failure wants user space to
> handle it, it will notify user space. This patch adds support
> to userspace that injects virtual SError with specified
> syndrome. This syndrome value will be set to the VSESR_EL2.
> VSESR_EL2 is a new RAS extensions register which provides the
> syndrome value reported to software on taking a virtual SError
> interrupt exception.
> 
> Signed-off-by: Dongjiu Geng 
> Signed-off-by: Quanming Wu 

Content seems fine, some real nitpicks inline.

> ---
>  arch/arm/include/asm/kvm_host.h  |  2 ++
>  arch/arm/kvm/guest.c |  5 +
>  arch/arm64/include/asm/kvm_emulate.h | 10 ++
>  arch/arm64/include/asm/kvm_host.h|  2 ++
>  arch/arm64/include/asm/sysreg.h  |  3 +++
>  arch/arm64/include/asm/system_misc.h |  1 +
>  arch/arm64/kvm/guest.c   | 13 +
>  arch/arm64/kvm/handle_exit.c | 21 +++--
>  arch/arm64/kvm/hyp/switch.c  | 14 ++
>  include/uapi/linux/kvm.h |  2 ++
>  virt/kvm/arm/arm.c   |  7 +++
>  11 files changed, 78 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 127e2dd2e21c..bdb6ea690257 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -244,6 +244,8 @@ int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const 
> struct kvm_one_reg *);
>  int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run,
>   int exception_index);
>  
> +int kvm_vcpu_ioctl_sei(struct kvm_vcpu *vcpu, u64 *syndrome);
> +
>  static inline void __cpu_init_hyp_mode(phys_addr_t pgd_ptr,
>  unsigned long hyp_stack_ptr,
>  unsigned long vector_ptr)
> diff --git a/arch/arm/kvm/guest.c b/arch/arm/kvm/guest.c
> index 1e0784ebbfd6..c23df72d9bec 100644
> --- a/arch/arm/kvm/guest.c
> +++ b/arch/arm/kvm/guest.c
> @@ -248,6 +248,11 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
>   return -EINVAL;
>  }
>  

Perhaps a comment here on why the stub doesn't do anything?
Obvious in the context of this patch, but perhaps not when someone is looking
at this out of context.

> +int kvm_vcpu_ioctl_sei(struct kvm_vcpu *vcpu, u64 *syndrome)
> +{
> + return 0;
> +}
> +
>  int __attribute_const__ kvm_target_cpu(void)
>  {
>   switch (read_cpuid_part()) {
> diff --git a/arch/arm64/include/asm/kvm_emulate.h 
> b/arch/arm64/include/asm/kvm_emulate.h
> index 47983db27de2..74213bd539dc 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -155,6 +155,16 @@ static inline u32 kvm_vcpu_get_hsr(const struct kvm_vcpu 
> *vcpu)
>   return vcpu->arch.fault.esr_el2;
>  }
>  
> +static inline u32 kvm_vcpu_get_vsesr(const struct kvm_vcpu *vcpu)
> +{
> + return vcpu->arch.fault.vsesr_el2;
> +}
> +
> +static inline void kvm_vcpu_set_vsesr(struct kvm_vcpu *vcpu, unsigned long 
> val)
> +{
> + vcpu->arch.fault.vsesr_el2 = val;
> +}
> +
>  static inline int kvm_vcpu_get_condition(const struct kvm_vcpu *vcpu)
>  {
>   u32 esr = kvm_vcpu_get_hsr(vcpu);
> diff --git a/arch/arm64/include/asm/kvm_host.h 
> b/arch/arm64/include/asm/kvm_host.h
> index d68630007b14..57b011261597 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -88,6 +88,7 @@ struct kvm_vcpu_fault_info {
>   u32 esr_el2;/* Hyp Syndrom Register */
>   u64 far_el2;/* Hyp Fault Address Register */
>   u64 hpfar_el2;  /* Hyp IPA Fault Address Register */
> + u32 vsesr_el2;  /* Virtual SError Exception Syndrome Register */
>  };
>  
>  /*
> @@ -381,6 +382,7 @@ int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
>  struct kvm_device_attr *attr);
>  int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
>  struct kvm_device_attr *attr);
> +int kvm_vcpu_ioctl_sei(struct kvm_vcpu *vcpu, u64 *syndrome);
>  
>  static inline void __cpu_init_stage2(void)
>  {
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 35b786b43ee4..06059eef0f5d 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -86,6 +86,9 @@
>  #define REG_PSTATE_PAN_IMM   sys_reg(0, 0, 4, 0, 4)
>  #define REG_PSTATE_UAO_IMM   sys_reg(0, 0, 4, 0, 3)
>  
> +/* virtual SError exception syndrome register in armv8.2 */
> +#define REG_VSESR_EL2sys_reg(3, 4, 5, 2, 3)
> +
>  #define SET_PSTATE_PAN(x) __emit_inst(0xd500 | REG_PSTATE_PAN_IMM |  
> \
> (!!x)<<8 | 0x1f)
>  #define SET_PSTATE_UAO(x) 

Re: [PATCH v5 3/7] acpi: apei: Add SEI notification type support for ARMv8

[Jonathan Cameron]

On Fri, 18 Aug 2017 22:11:53 +0800
Dongjiu Geng  wrote:

> ARMV8.2 requires implementation of the RAS extension, in
> this extension it adds SEI(SError Interrupt) notification
> type, this patch addes a new GHES error source handling
> function for SEI. Because this error source parse and handling
> method are similar with the SEA. so use one function to handle
> them.

Because the error source parsing and handling methods are similar
to that for the SEA, use one function to handle both.

> 
> In current code logic, The two functions ghes_sea_add() and

No capital after , 

> ghes_sea_remove() are only called when CONFIG_ACPI_APEI_SEA
> and CONFIG_ACPI_APEI_SEI are defined. If not, it will return
> errors in the ghes_probe() and do not continue, so remove the
> useless code that handling CONFIG_ACPI_APEI_SEA and
> CONFIG_ACPI_APEI_SEI do not defined.

If not, it will return errors in the ghes_probe() and not contiue.
Hence, remove the unnecessary handling when CONFIG_ACPI_APEI_SEA
and CONFIG_ACPI_APEI_SEI are not defined.

> 
> Expose one API ghes_notify_sex() to external, external modules
> can call this exposed APIs to parse and handling the SEA/SEI.

Expose one API ghes_notify_sec() to external users.  External modules...
> 
> Signed-off-by: Dongjiu Geng 

Some really trivial suggestions inline.  Feel free to ignore them as
they are a matter of personal taste.

Jonathan

> ---
>  arch/arm64/mm/fault.c | 21 +++--
>  drivers/acpi/apei/Kconfig | 15 
>  drivers/acpi/apei/ghes.c  | 60 
> ++-
>  include/acpi/ghes.h   |  2 +-
>  4 files changed, 74 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index 2509e4fe6992..0aa92a69c280 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -585,7 +585,7 @@ static int do_sea(unsigned long addr, unsigned int esr, 
> struct pt_regs *regs)
>   if (interrupts_enabled(regs))
>   nmi_enter();
>  
> - ret = ghes_notify_sea();
> + ret = ghes_notify_sex(ACPI_HEST_NOTIFY_SEA);
>  
>   if (interrupts_enabled(regs))
>   nmi_exit();
> @@ -682,7 +682,24 @@ int handle_guest_sea(phys_addr_t addr, unsigned int esr)
>   int ret = -ENOENT;
>  
>   if (IS_ENABLED(CONFIG_ACPI_APEI_SEA))
> - ret = ghes_notify_sea();
> + ret = ghes_notify_sex(ACPI_HEST_NOTIFY_SEA);
> +
> + return ret;
> +}
> +
> +/*
> + * Handle SError interrupt that occur in a guest kernel.
> + *
> + * The return value will be zero if the SEI was successfully handled
> + * and non-zero if there was an error processing the error or there was
> + * no error to process.
> + */
> +int handle_guest_sei(phys_addr_t addr, unsigned int esr)
> +{
> + int ret = -ENOENT;
> +
> + if (IS_ENABLED(CONFIG_ACPI_APEI_SEI))
> + ret = ghes_notify_sex(ACPI_HEST_NOTIFY_SEI);
>  
>   return ret;
>  }
> diff --git a/drivers/acpi/apei/Kconfig b/drivers/acpi/apei/Kconfig
> index de14d49a5c90..556370c763ec 100644
> --- a/drivers/acpi/apei/Kconfig
> +++ b/drivers/acpi/apei/Kconfig
> @@ -54,6 +54,21 @@ config ACPI_APEI_SEA
> option allows the OS to look for such hardware error record, and
> take appropriate action.
>  
> +config ACPI_APEI_SEI
> + bool "APEI Asynchronous SError Interrupt logging/recovering support"
> + depends on ARM64 && ACPI_APEI_GHES
> + default y
> + help
> +   This option should be enabled if the system supports
> +   firmware first handling of SEI (asynchronous SError interrupt).
> +
> +   SEI happens with invalid instruction access or asynchronous exceptions
> +   on ARMv8 systems. If a system supports firmware first handling of SEI,
> +   the platform analyzes and handles hardware error notifications from
> +   SEI, and it may then form a HW error record for the OS to parse and
> +   handle. This option allows the OS to look for such hardware error
> +   record, and take appropriate action.
> +
>  config ACPI_APEI_MEMORY_FAILURE
>   bool "APEI memory error recovering support"
>   depends on ACPI_APEI && MEMORY_FAILURE
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index d661d452b238..705738aa48b8 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -813,20 +813,21 @@ static struct notifier_block ghes_notifier_hed = {
>   .notifier_call = ghes_notify_hed,
>  };
>  
> -#ifdef CONFIG_ACPI_APEI_SEA
>  static LIST_HEAD(ghes_sea);
> +static LIST_HEAD(ghes_sei);
>  
>  /*
>   * Return 0 only if one of the SEA error sources successfully reported an 
> error
>   * record sent from the firmware.
>   */
> -int ghes_notify_sea(void)
> +
> +int ghes_handle_sex(struct list_head *head)
>  {
>   struct ghes *ghes;
>   int ret = -ENOENT;
>  
>   rcu_read_lock();
> - 

Re: [PATCH v5 4/7] support user space to query RAS extension feature

[Jonathan Cameron]

On Fri, 18 Aug 2017 22:11:54 +0800
Dongjiu Geng  wrote:

> In armv8.2 RAS extension, it adds virtual SError exception
> syndrome registeri(VSESR_EL2), user space will specify that
> value. so user space will check whether CPU feature has RAS
> extension. if has, it will specify the virtual SError syndrome
> value. Otherwise, it will not set. This patch adds this support

In the armv8.2 RAS extension, a virtual SError exception syndrome
registeri(VSESR_EL) is added.  This value may be specified from userspace.
Userspace will want to check if the CPU has the RAS extension.
If it has, it wil specify the virtual SError syndrome value, otherwise
it will not be set.  This patch adds support for querying the availability
of this extension.

Code is fine.
> 
> Signed-off-by: Dongjiu Geng 
> ---
>  arch/arm64/kvm/reset.c   | 3 +++
>  include/uapi/linux/kvm.h | 1 +
>  2 files changed, 4 insertions(+)
> 
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index 3256b9228e75..b7313ee028e9 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -77,6 +77,9 @@ int kvm_arch_dev_ioctl_check_extension(struct kvm *kvm, 
> long ext)
>   case KVM_CAP_ARM_PMU_V3:
>   r = kvm_arm_support_pmu_v3();
>   break;
> + case KVM_CAP_ARM_RAS_EXTENSION:
> + r = cpus_have_const_cap(ARM64_HAS_RAS_EXTN);
> + break;
>   case KVM_CAP_SET_GUEST_DEBUG:
>   case KVM_CAP_VCPU_ATTRIBUTES:
>   r = 1;
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 6cd63c18708a..5a2a338cae57 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -929,6 +929,7 @@ struct kvm_ppc_resize_hpt {
>  #define KVM_CAP_PPC_SMT_POSSIBLE 147
>  #define KVM_CAP_HYPERV_SYNIC2 148
>  #define KVM_CAP_HYPERV_VP_INDEX 149
> +#define KVM_CAP_ARM_RAS_EXTENSION 150
>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>  

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v5 6/7] KVM: arm64: Allow get exception information from userspace

[Jonathan Cameron]

On Fri, 18 Aug 2017 22:11:56 +0800
Dongjiu Geng  wrote:

> when userspace gets SIGBUS signal, it does not know whether
> this is a synchronous external abort or SError, so needs
> to get the exception syndrome. so this patch allows userspace
> can get this values. For syndrome, only give userspace
> syndrome EC and ISS.
> 
> Now we move the synchronous external abort injection logic to
> userspace, when userspace injects the SEA exception to guest
> OS, it needs to specify the far_el1 value, so this patch give
> the exception virtual address to user space.
> 
> Signed-off-by: Dongjiu Geng 
> Signed-off-by: Quanming Wu 

A couple of really trivial formatting points inline.

> ---
>  arch/arm64/include/uapi/asm/kvm.h |  5 +
>  arch/arm64/kvm/guest.c| 35 +++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/arch/arm64/include/uapi/asm/kvm.h 
> b/arch/arm64/include/uapi/asm/kvm.h
> index 9f3ca24bbcc6..514261f682b8 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -181,6 +181,11 @@ struct kvm_arch_memory_slot {
>  #define KVM_REG_ARM64_SYSREG_OP2_MASK0x0007
>  #define KVM_REG_ARM64_SYSREG_OP2_SHIFT   0
>  
> +/* AArch64 fault registers */
> +#define KVM_REG_ARM64_FAULT  (0x0014 << KVM_REG_ARM_COPROC_SHIFT)
> +#define KVM_REG_ARM64_FAULT_ESR_EC_ISS   (0)
> +#define KVM_REG_ARM64_FAULT_FAR  (1)
> +
>  #define ARM64_SYS_REG_SHIFT_MASK(x,n) \
>   (((x) << KVM_REG_ARM64_SYSREG_ ## n ## _SHIFT) & \
>   KVM_REG_ARM64_SYSREG_ ## n ## _MASK)
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index 5c7f657dd207..cb383c310f18 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -128,6 +128,38 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const 
> struct kvm_one_reg *reg)
>  out:
>   return err;
>  }
> +static int get_fault_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg 
> *reg)
> +{
> + void __user *uaddr = (void __user *)(unsigned long)reg->addr;
> + u32 ec, value;
> + u32 id = reg->id & ~(KVM_REG_ARCH_MASK |
> + KVM_REG_SIZE_MASK | KVM_REG_ARM64_FAULT);
> +
> + switch (id) {
> + case KVM_REG_ARM64_FAULT_ESR_EC_ISS:
> + /* The user space needs to know the fault exception
> +  * class field
> +  */

The rest of this file uses the multiline comment syntax
/*
 * The user...
 */

> + ec = kvm_vcpu_get_hsr(vcpu) & ESR_ELx_EC_MASK;
> + value = ec | (kvm_vcpu_get_hsr(vcpu) & ESR_ELx_ISS_MASK);
Same as

value = kvm_vpcu_get_hsr(vcpu) & (ESR_ELx_EC_MASK | ESR_ELx_ISS_MASK);

?

> +
> + if (copy_to_user(uaddr, , KVM_REG_SIZE(reg->id)) != 0)
> + return -EFAULT;
> + break;
> + case KVM_REG_ARM64_FAULT_FAR:
> + /* when user space injects synchronized abort, it needs
> +  * to inject the fault address.
> +  */

Again, multiline comment syntax.

> + if (copy_to_user(uaddr, &(vcpu->arch.fault.far_el2),
> + KVM_REG_SIZE(reg->id)) != 0)
> + return -EFAULT;
> + break;
> + default:
> + return -ENOENT;
> + }
> + return 0;
> +}
> +
>  
>  int kvm_arch_vcpu_ioctl_get_regs(struct kvm_vcpu *vcpu, struct kvm_regs 
> *regs)
>  {
> @@ -243,6 +275,9 @@ int kvm_arm_get_reg(struct kvm_vcpu *vcpu, const struct 
> kvm_one_reg *reg)
>   if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_CORE)
>   return get_core_reg(vcpu, reg);
>  
> + if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM64_FAULT)
> + return get_fault_reg(vcpu, reg);
> +
>   if (is_timer_reg(reg->id))
>   return get_timer_reg(vcpu, reg);
>  

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 09/27] arm64/sve: Signal frame and context structure definition

[Alex Bennée]

Dave Martin  writes:

> This patch defines the representation that will be used for the SVE
> register state in the signal frame, and implements support for
> saving and restoring the SVE registers around signals.
>
> The same layout will also be used for the in-kernel task state.
>
> Due to the variability of the SVE vector length, it is not possible
> to define a fixed C struct to describe all the registers.  Instead,
> Macros are defined in sigcontext.h to facilitate access to the
> parts of the structure.
>
> Signed-off-by: Dave Martin 
> ---
>  arch/arm64/include/uapi/asm/sigcontext.h | 113 
> ++-
>  1 file changed, 112 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/uapi/asm/sigcontext.h 
> b/arch/arm64/include/uapi/asm/sigcontext.h
> index f0a76b9..0533bdf 100644
> --- a/arch/arm64/include/uapi/asm/sigcontext.h
> +++ b/arch/arm64/include/uapi/asm/sigcontext.h
> @@ -16,6 +16,8 @@
>  #ifndef _UAPI__ASM_SIGCONTEXT_H
>  #define _UAPI__ASM_SIGCONTEXT_H
>
> +#ifndef __ASSEMBLY__
> +
>  #include 
>
>  /*
> @@ -41,10 +43,11 @@ struct sigcontext {
>   *
>   *   0x210   fpsimd_context
>   *0x10   esr_context
> + *   0x8a0   sve_context (vl <= 64) (optional)
>   *0x20   extra_context (optional)
>   *0x10   terminator (null _aarch64_ctx)
>   *
> - *   0xdb0   (reserved for future allocation)
> + *   0x510   (reserved for future allocation)
>   *
>   * New records that can exceed this space need to be opt-in for userspace, so
>   * that an expanded signal frame is not generated unexpectedly.  The 
> mechanism
> @@ -116,4 +119,112 @@ struct extra_context {
>   __u32 __reserved[3];
>  };
>
> +#define SVE_MAGIC0x53564501
> +
> +struct sve_context {
> + struct _aarch64_ctx head;
> + __u16 vl;
> + __u16 __reserved[3];
> +};
> +
> +#endif /* !__ASSEMBLY__ */
> +
> +/*
> + * The SVE architecture leaves space for future expansion of the
> + * vector length beyond its initial architectural limit of 2048 bits
> + * (16 quadwords).
> + */
> +#define SVE_VQ_MIN   1
> +#define SVE_VQ_MAX   0x200
> +
> +#define SVE_VL_MIN   (SVE_VQ_MIN * 0x10)
> +#define SVE_VL_MAX   (SVE_VQ_MAX * 0x10)
> +
> +#define SVE_NUM_ZREGS32
> +#define SVE_NUM_PREGS16
> +
> +#define sve_vl_valid(vl) \
> + ((vl) % 0x10 == 0 && (vl) >= SVE_VL_MIN && (vl) <= SVE_VL_MAX)
> +#define sve_vq_from_vl(vl)   ((vl) / 0x10)
> +#define sve_vl_from_vq(vq)   ((vq) * 0x10)

I got a little confused first time through over what VQ and VL where.
Maybe it would make sense to expand a little more from first principles?

  /*
   * The SVE architecture defines vector registers as a multiple of 128
   * bit quadwords. The current architectural limit is 2048 bits (16
   * quadwords) but there is room for future expansion beyond that.
*/

  #define SVE_VQ_BITS 128  /* 128 bits in one quadword */
  #define SVE_VQ_BYTES(SVE_VQ_BITS / 8)

  #define SVE_VQ_MIN1
  #define SVE_VQ_MAX0x200/* see ZCR_ELx[8:0] */

  #define SVE_VL_MIN_BYTES  (SVE_VQ_MIN * SVE_VQ_BYTES)
  #define SVE_VL_MAX_BYTES  (SVE_VQ_MAX * SVE_VQ_BYTES)

  #define SVE_NUM_ZREGS 32
  #define SVE_NUM_PREGS 16

  #define sve_vl_valid(vl)  \
  ((vl) % SVE_VQ_BYTES == 0 && (vl) >= SVE_VL_MIN && (vl) <= SVE_VL_MAX)
  #define sve_vq_from_vl(vl)((vl) / SVE_VQ_BYTES)
  #define sve_vl_from_vq(vq)((vq) * SVE_VQ_BYTES)


> +
> +/*
> + * If the SVE registers are currently live for the thread at signal delivery,
> + * sve_context.head.size >=
> + *   SVE_SIG_CONTEXT_SIZE(sve_vq_from_vl(sve_context.vl))
> + * and the register data may be accessed using the SVE_SIG_*() macros.
> + *
> + * If sve_context.head.size <
> + *   SVE_SIG_CONTEXT_SIZE(sve_vq_from_vl(sve_context.vl)),
> + * the SVE registers were not live for the thread and no register data
> + * is included: in this case, the SVE_SIG_*() macros should not be
> + * used except for this check.
> + *
> + * The same convention applies when returning from a signal: a caller
> + * will need to remove or resize the sve_context block if it wants to
> + * make the SVE registers live when they were previously non-live or
> + * vice-versa.  This may require the the caller to allocate fresh
> + * memory and/or move other context blocks in the signal frame.
> + *
> + * Changing the vector length during signal return is not permitted:
> + * sve_context.vl must equal the thread's current vector length when
> + * doing a sigreturn.
> + *
> + *
> + * Note: for all these macros, the "vq" argument denotes the SVE
> + * vector length in quadwords (i.e., units of 128 bits).
> + *
> + * The correct way to obtain vq is to use sve_vq_from_vl(vl).  The
> + * result is valid if and only if sve_vl_valid(vl) is true. 

Re: [PATCH 09/27] arm64/sve: Signal frame and context structure definition

[Dave Martin]

On Tue, Aug 22, 2017 at 11:22:44AM +0100, Alex Bennée wrote:
> 
> Dave Martin  writes:
> 
> > This patch defines the representation that will be used for the SVE
> > register state in the signal frame, and implements support for
> > saving and restoring the SVE registers around signals.
> >
> > The same layout will also be used for the in-kernel task state.
> >
> > Due to the variability of the SVE vector length, it is not possible
> > to define a fixed C struct to describe all the registers.  Instead,
> > Macros are defined in sigcontext.h to facilitate access to the
> > parts of the structure.
> >
> > Signed-off-by: Dave Martin 
> > ---
> >  arch/arm64/include/uapi/asm/sigcontext.h | 113 
> > ++-
> >  1 file changed, 112 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/include/uapi/asm/sigcontext.h 
> > b/arch/arm64/include/uapi/asm/sigcontext.h
> > index f0a76b9..0533bdf 100644
> > --- a/arch/arm64/include/uapi/asm/sigcontext.h
> > +++ b/arch/arm64/include/uapi/asm/sigcontext.h
> > @@ -16,6 +16,8 @@
> >  #ifndef _UAPI__ASM_SIGCONTEXT_H
> >  #define _UAPI__ASM_SIGCONTEXT_H
> >
> > +#ifndef __ASSEMBLY__
> > +
> >  #include 
> >
> >  /*
> > @@ -41,10 +43,11 @@ struct sigcontext {
> >   *
> >   * 0x210   fpsimd_context
> >   *  0x10   esr_context
> > + * 0x8a0   sve_context (vl <= 64) (optional)
> >   *  0x20   extra_context (optional)
> >   *  0x10   terminator (null _aarch64_ctx)
> >   *
> > - * 0xdb0   (reserved for future allocation)
> > + * 0x510   (reserved for future allocation)
> >   *
> >   * New records that can exceed this space need to be opt-in for userspace, 
> > so
> >   * that an expanded signal frame is not generated unexpectedly.  The 
> > mechanism
> > @@ -116,4 +119,112 @@ struct extra_context {
> > __u32 __reserved[3];
> >  };
> >
> > +#define SVE_MAGIC  0x53564501
> > +
> > +struct sve_context {
> > +   struct _aarch64_ctx head;
> > +   __u16 vl;
> > +   __u16 __reserved[3];
> > +};
> > +
> > +#endif /* !__ASSEMBLY__ */
> > +
> > +/*
> > + * The SVE architecture leaves space for future expansion of the
> > + * vector length beyond its initial architectural limit of 2048 bits
> > + * (16 quadwords).
> > + */
> > +#define SVE_VQ_MIN 1
> > +#define SVE_VQ_MAX 0x200
> > +
> > +#define SVE_VL_MIN (SVE_VQ_MIN * 0x10)
> > +#define SVE_VL_MAX (SVE_VQ_MAX * 0x10)
> > +
> > +#define SVE_NUM_ZREGS  32
> > +#define SVE_NUM_PREGS  16
> > +
> > +#define sve_vl_valid(vl) \
> > +   ((vl) % 0x10 == 0 && (vl) >= SVE_VL_MIN && (vl) <= SVE_VL_MAX)
> > +#define sve_vq_from_vl(vl) ((vl) / 0x10)
> > +#define sve_vl_from_vq(vq) ((vq) * 0x10)
> 
> I got a little confused first time through over what VQ and VL where.
> Maybe it would make sense to expand a little more from first principles?
> 
>   /*
>* The SVE architecture defines vector registers as a multiple of 128
>* bit quadwords. The current architectural limit is 2048 bits (16
>* quadwords) but there is room for future expansion beyond that.
> */

This comes up in several places and so I didn't want to comment it
repeatedly everywhere.

Instead, I wrote up something in section 2 (Vector length terminology)
of Documentation/arm64/sve.txt -- see patch 25.  Can you take a look and
see whether that's adequate?

[...]

Cheers
---Dave
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm