Re: [PATCH 3/5] irqchip/gic-v2: Parse and export virtual GIC information

[Marc Zyngier]

Julien,

On 08/02/16 16:47, Julien Grall wrote:
> For now, the firmware tables are parsed 2 times: once in the GIC
> drivers, the other timer when initializing the vGIC. It means code
> duplication and make more tedious to add the support for another
> firmware table (like ACPI).
> 
> Introduce a new structure and set of helpers to get/set the virtual GIC
> information. Also fill up the structure for GICv2.
> 
> Signed-off-by: Julien Grall 
> ---
> 
> Cc: Thomas Gleixner 
> Cc: Jason Cooper 
> Cc: Marc Zyngier 
> 
>  drivers/irqchip/irq-gic-common.c   | 13 ++
>  drivers/irqchip/irq-gic-common.h   |  3 ++
>  drivers/irqchip/irq-gic.c  | 78 
> +-
>  include/linux/irqchip/arm-gic-common.h | 34 +++
>  4 files changed, 127 insertions(+), 1 deletion(-)
>  create mode 100644 include/linux/irqchip/arm-gic-common.h
> 
> diff --git a/drivers/irqchip/irq-gic-common.c 
> b/drivers/irqchip/irq-gic-common.c
> index f174ce0..704caf4 100644
> --- a/drivers/irqchip/irq-gic-common.c
> +++ b/drivers/irqchip/irq-gic-common.c
> @@ -21,6 +21,19 @@
>  
>  #include "irq-gic-common.h"
>  
> +static const struct gic_kvm_info *gic_kvm_info;
> +
> +const struct gic_kvm_info *gic_get_kvm_info(void)
> +{
> + return gic_kvm_info;
> +}
> +
> +void gic_set_kvm_info(const struct gic_kvm_info *info)
> +{
> + WARN(gic_kvm_info != NULL, "gic_kvm_info already set\n");
> + gic_kvm_info = info;
> +}
> +
>  void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
>   void *data)
>  {
> diff --git a/drivers/irqchip/irq-gic-common.h 
> b/drivers/irqchip/irq-gic-common.h
> index fff697d..205e5fd 100644
> --- a/drivers/irqchip/irq-gic-common.h
> +++ b/drivers/irqchip/irq-gic-common.h
> @@ -19,6 +19,7 @@
>  
>  #include 
>  #include 
> +#include 
>  
>  struct gic_quirk {
>   const char *desc;
> @@ -35,4 +36,6 @@ void gic_cpu_config(void __iomem *base, void 
> (*sync_access)(void));
>  void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
>   void *data);
>  
> +void gic_set_kvm_info(const struct gic_kvm_info *info);
> +
>  #endif /* _IRQ_GIC_COMMON_H */
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 911758c..d3a09a4 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -102,6 +102,8 @@ static struct static_key supports_deactivate = 
> STATIC_KEY_INIT_TRUE;
>  
>  static struct gic_chip_data gic_data[CONFIG_ARM_GIC_MAX_NR] __read_mostly;
>  
> +static struct gic_kvm_info gic_v2_kvm_info;
> +
>  #ifdef CONFIG_GIC_NON_BANKED
>  static void __iomem *gic_get_percpu_base(union gic_base *base)
>  {
> @@ -1190,6 +1192,44 @@ static bool gic_check_eoimode(struct device_node 
> *node, void __iomem **base)
>   return true;
>  }
>  
> +static void __init gic_of_setup_kvm_info(struct device_node *node)
> +{
> + int ret;
> + struct resource r;
> + unsigned int irq;
> +
> + gic_v2_kvm_info.type = GIC_V2;
> +
> + irq = irq_of_parse_and_map(node, 0);
> + if (!irq)
> + gic_v2_kvm_info.maint_irq = -1;

Please don't do that. 0 *is* the value for an invalid interrupt, and
this is what you should expose here. Same for GICv3.

> + else
> + gic_v2_kvm_info.maint_irq = irq;
> +
> + ret = of_address_to_resource(node, 2, &r);
> + if (!ret) {
> + gic_v2_kvm_info.vctrl_base = r.start;
> + gic_v2_kvm_info.vctrl_size = resource_size(&r);
> + }
> +
> + ret = of_address_to_resource(node, 3, &r);
> + if (!ret) {
> + if (!PAGE_ALIGNED(r.start))
> + pr_warn("GICV physical address 0x%llx not page 
> aligned\n",
> + (unsigned long long)r.start);
> + else if (!PAGE_ALIGNED(resource_size(&r)))
> + pr_warn("GICV size 0x%llx not a multiple of page size 
> 0x%lx\n",
> + (unsigned long long)resource_size(&r),
> + PAGE_SIZE);
> + else {
> + gic_v2_kvm_info.vcpu_base = r.start;
> + gic_v2_kvm_info.vcpu_size = resource_size(&r);

This tends to make me think that this should actually be a proper
resource, and not a set of arbitrary fields.

> + }
> + }
> +
> + gic_set_kvm_info(&gic_v2_kvm_info);
> +}
> +
>  int __init
>  gic_of_init(struct device_node *node, struct device_node *parent)
>  {
> @@ -1219,8 +1259,10 @@ gic_of_init(struct device_node *node, struct 
> device_node *parent)
>  
>   __gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset,
>&node->fwnode);
> - if (!gic_cnt)
> + if (!gic_cnt) {
>   gic_init_physaddr(node);
> + gic_of_setup_kvm_info(node);
> + }
>  
>   if (parent) {
>   irq = irq_of_parse_and_map(node, 0);
> @@ -1247,6 +1289,32 @@ IRQCHIP_DECLARE(pl390, "arm,pl390", gic_of_init);
>  
>  #ifdef C

Re: [PATCH V1 0/7] Enable ACPI support for ARM KVM GIC

[Marc Zyngier]

On 08/02/16 16:47, Wei Huang wrote:
> 
> 
> On 2/8/16 10:39, Julien Grall wrote:
>> Hi,
>>
>> On 08/02/16 09:59, Marc Zyngier wrote:
>>> On 05/02/16 17:07, Wei Huang wrote:
 Wei Huang (7):
KVM: GIC: Move GIC DT probing code to GICv2 and GICv3 files
KVM: GIC: Add extra fields to store GICH and GICV resource info
KVM: GIC: Create a common probe function for GIC
KVM: GICv2: Extract the common code from DT
KVM: GICv2: Add ACPI probing function
KVM: GICv3: Extract the common code from DT
KVM: GICv3: Add ACPI probing function

   include/kvm/arm_vgic.h  |  14 ++--
   virt/kvm/arm/vgic-v2-emul.c |   4 +-
   virt/kvm/arm/vgic-v2.c  | 186
 +---
   virt/kvm/arm/vgic-v3.c  | 159
 -
   virt/kvm/arm/vgic.c |  22 +-
   5 files changed, 277 insertions(+), 108 deletions(-)

>>>
>>> So when I see this diffstat and the patches that follow, I cannot help
>>> but think that we do have a disconnect here. You do add a bunch of ACPI
>>> probing in KVM, to which I've already said no.
>>>
>>> I want to see the probing code in the GIC drivers, exported through a
>>> common structure that KVM can then use. That's it. Nothing else.
>>
>> I've been working on a patch series which rely on the GIC and arch timer
>> drivers to get the necessary information to initialize the different KVM
>> components (vGIC + vtimer).
>>
>> I think this achieves the goal you have in mind. I will post the series
>> in a few minutes.
>>
> 
> Julien,
> 
> Regarding arch_timer, there is an effort going on by Linaro. It has
> impact on KVM arch_timer init. See https://lkml.org/lkml/2016/2/1/658.

The KVM changes in this series are slightly pointless, and should be
addressed the same way the GIC should be addressed:

- Collect the information in the low-level code
- Expose it through a common infrastructure.

If you have different code paths for ACPI and DT outside of the core
probing code, then you are doing it wrong. ACPI is not "different".

M.
-- 
Jazz is not dead. It just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 6/6] kvm: arm64: Add ACPI support for virt arch timer

[Marc Zyngier]

On 01/02/16 20:26, fu@linaro.org wrote:
> From: Fu Wei 
> 
> This patch adds ACPI/GTDT support for virt arch timer
> using the API in GTDT driver.
> 
> Signed-off-by: Fu Wei 
> ---
>  virt/kvm/arm/arch_timer.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 0a279d3..4077347 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -385,6 +385,9 @@ static int kvm_timer_get_ppi(unsigned int *ppi)
>  {
>   struct device_node *np;
>   int ret = -EINVAL;
> +#ifdef CONFIG_ACPI_GTDT
> + struct arch_timer_data data;
> +#endif
>  
>   np = of_find_matching_node(NULL, arch_timer_of_match);
>   if (!np) {
> @@ -397,6 +400,11 @@ static int kvm_timer_get_ppi(unsigned int *ppi)
>   of_node_put(np);
>  
>  skip_of:
> +#ifdef CONFIG_ACPI_GTDT
> + if (!*ppi && !gtdt_arch_timer_data_init(NULL, &data))
> + *ppi = data.virt_ppi;
> +#endif
> +
>   if (*ppi)
>   return 0;
>  
> 

As I already pointed out in another thread hacking some KVM ACPI stuff,
this is the wrong approach.

We should have a *common* accessor in the timer code that exports the
relevant information, whatever the firmware "du jour" is.

See Julien's series, which seems to address the issue in a much more
convincing way:

https://lists.cs.columbia.edu/pipermail/kvmarm/2016-February/018531.html

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: PCIe passthrough support on ARM

[Edward Cragg]

On Mon, Feb 08, 2016 at 01:56:34PM +0100, Eric Auger wrote:
> > Do you mean the GIC itself? From registers, it appears to be a standard ARM
> > GIC, though i'm not sure exactly which one yet. However, it's stated in the
> > processor's datasheet that legacy interrupts aren't supported. It's one of 
> > the
> > newer Samsung Exynos processors.
> I meant GICv2m (featuring single or multiple MSI frames) or GICv3 ITS or
> any other proprietary interrupt controller IP supporting MSIs.

>From the manual, it seems from the reset values for the identification
registers that it is most likely a GICv2.

If we continue with this project, and once we get access to hardware, we'd be
very interested in helping you with testing this.

Thanks,

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

Re: [PATCH v3 21/23] arm64: hw_breakpoint: Allow EL2 breakpoints if running in HYP

[Catalin Marinas]

On Mon, Feb 08, 2016 at 04:45:44PM +, Marc Zyngier wrote:
> I was being overzealous, and your solution is clearly better. I ended up with 
> the following:
> 
> diff --git a/arch/arm64/include/asm/hw_breakpoint.h 
> b/arch/arm64/include/asm/hw_breakpoint.h
> index 9732908..c872b2f 100644
> --- a/arch/arm64/include/asm/hw_breakpoint.h
> +++ b/arch/arm64/include/asm/hw_breakpoint.h
[...]
> @@ -62,6 +45,7 @@ static inline void decode_ctrl_reg(u32 reg,
>  #define AARCH64_ESR_ACCESS_MASK  (1 << 6)
>  
>  /* Privilege Levels */
> +#define AARCH64_BREAKPOINT_EL2   0
>  #define AARCH64_BREAKPOINT_EL1   1
>  #define AARCH64_BREAKPOINT_EL0   2
>  
> @@ -76,6 +60,35 @@ static inline void decode_ctrl_reg(u32 reg,
>  #define ARM_KERNEL_STEP_ACTIVE   1
>  #define ARM_KERNEL_STEP_SUSPEND  2
>  
> +#define DBG_HMC_HYP  (1 << 13)
> +#define DBG_SSC_HYP  (3 << 14)
> +
> +static inline u32 encode_ctrl_reg(struct arch_hw_breakpoint_ctrl ctrl)
> +{
> + u32 val = (ctrl.len << 5) | (ctrl.type << 3) | ctrl.enabled;
> +
> + if (is_kernel_in_hyp_mode() && ctrl.privilege == AARCH64_BREAKPOINT_EL1)
> + val |= DBG_HMC_HYP | DBG_SSC_HYP;

Nitpick, for completeness:

val |= DBG_HMC_HYP | DBG_SSC_HYP | (AARCH64_BREAKPOINT_EL2 << 
1);

> + else
> + val |= ctrl.privilege << 1;
> +
> + return val;
> +}
> +
> +static inline void decode_ctrl_reg(u32 reg,
> +struct arch_hw_breakpoint_ctrl *ctrl)
> +{
> + ctrl->enabled   = reg & 0x1;
> + reg >>= 1;
> + ctrl->privilege = reg & 0x3;
> + if (ctrl->privilege == AARCH64_BREAKPOINT_EL2)
> + ctrl->privilege = AARCH64_BREAKPOINT_EL1;
> + reg >>= 2;
> + ctrl->type  = reg & 0x3;
> + reg >>= 2;
> + ctrl->len   = reg & 0xff;
> +}
> +
>  /*
>   * Limits.
>   * Changing these will require modifications to the register accessors.
> 
> Was that what you had in mind?

Looks fine:

Acked-by: Catalin Marinas 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 0/5] arm64: Add support of KVM with ACPI

[Julien Grall]

Hello,

This small series allows an ARM64 ACPI based platform to use KVM.

Currently the KVM code has to parse the firmware table to get the necessary
information to setup the virtual timer and virtual GIC.

However the parsing of those tables are already done in the GIC and arch
timer drivers.

This patch series introduces different helpers to retrieve the information
from different drivers avoiding to duplicate the parsing code.

Note there is patch series ([1] and [2]) adding support of KVM on ACPI,
although the approach chosen is completely different. The code to parse
the firmware tables are duplicated which I think make more complex to
support new firmware tables.

Regards,

[1] https://lists.cs.columbia.edu/pipermail/kvmarm/2016-February/018482.html
[2] https://lists.cs.columbia.edu/pipermail/kvmarm/2016-February/018355.html

Julien Grall (5):
  KVM: arm/arm64: arch_timer: Gather KVM specific information in a
structure
  KVM: arm/arm64: arch_timer: Rely on the arch timer to parse the
firmware tables
  irqchip/gic-v2: Parse and export virtual GIC information
  irqchip/gic-v3: Parse and export virtual GIC information
  KVM: arm/arm64: vgic: Rely on the GIC driver to parse the firmware
tables

 drivers/clocksource/arm_arch_timer.c   | 11 +++--
 drivers/irqchip/irq-gic-common.c   | 13 ++
 drivers/irqchip/irq-gic-common.h   |  3 ++
 drivers/irqchip/irq-gic-v3.c   | 43 +++
 drivers/irqchip/irq-gic.c  | 78 +-
 include/clocksource/arm_arch_timer.h   | 13 +++---
 include/kvm/arm_vgic.h |  7 +--
 include/linux/irqchip/arm-gic-common.h | 35 +++
 virt/kvm/arm/arch_timer.c  | 39 +
 virt/kvm/arm/vgic-v2.c | 66 ++--
 virt/kvm/arm/vgic-v3.c | 45 ++--
 virt/kvm/arm/vgic.c| 50 --
 12 files changed, 262 insertions(+), 141 deletions(-)
 create mode 100644 include/linux/irqchip/arm-gic-common.h

-- 
1.9.1

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

[PATCH 5/5] KVM: arm/arm64: vgic: Rely on the GIC driver to parse the firmware tables

[Julien Grall]

For now, the firmware tables are parsed 2 times: once in the GIC
drivers, the other time when initializing the vGIC. It means code
duplication and make more tedious to add the support for another
firmware table (like ACPI).

Use the recently introduced helper gic_get_kvm_info() to retrieve
information about the virtual GIC.

With this change, the virtual GIC becomes agnostic to the firmware
table and KVM will be able to initialize the vGIC on ACPI.

Signed-off-by: Julien Grall 
---
Cc: Christoffer Dall 
Cc: Marc Zyngier 
Cc: Gleb Natapov 
Cc: Paolo Bonzini 

 include/kvm/arm_vgic.h |  7 +++---
 virt/kvm/arm/vgic-v2.c | 66 ++
 virt/kvm/arm/vgic-v3.c | 45 ++
 virt/kvm/arm/vgic.c| 50 --
 4 files changed, 67 insertions(+), 101 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 13a3d53..ed62772 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -25,6 +25,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #define VGIC_NR_IRQS_LEGACY256
 #define VGIC_NR_SGIS   16
@@ -357,15 +358,15 @@ bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, struct 
irq_phys_map *map);
 #define vgic_initialized(k)(!!((k)->arch.vgic.nr_cpus))
 #define vgic_ready(k)  ((k)->arch.vgic.ready)
 
-int vgic_v2_probe(struct device_node *vgic_node,
+int vgic_v2_probe(const struct gic_kvm_info *gic_kvm_info,
  const struct vgic_ops **ops,
  const struct vgic_params **params);
 #ifdef CONFIG_KVM_ARM_VGIC_V3
-int vgic_v3_probe(struct device_node *vgic_node,
+int vgic_v3_probe(const struct gic_kvm_info *gic_kvm_info,
  const struct vgic_ops **ops,
  const struct vgic_params **params);
 #else
-static inline int vgic_v3_probe(struct device_node *vgic_node,
+static inline int vgic_v3_probe(const struct gic_kvm_info *gic_kvm_info,
const struct vgic_ops **ops,
const struct vgic_params **params)
 {
diff --git a/virt/kvm/arm/vgic-v2.c b/virt/kvm/arm/vgic-v2.c
index ff02f08..2598a05 100644
--- a/virt/kvm/arm/vgic-v2.c
+++ b/virt/kvm/arm/vgic-v2.c
@@ -20,9 +20,6 @@
 #include 
 #include 
 #include 
-#include 
-#include 
-#include 
 
 #include 
 
@@ -177,38 +174,37 @@ static const struct vgic_ops vgic_v2_ops = {
 static struct vgic_params vgic_v2_params;
 
 /**
- * vgic_v2_probe - probe for a GICv2 compatible interrupt controller in DT
- * @node:  pointer to the DT node
- * @ops:   address of a pointer to the GICv2 operations
- * @params:address of a pointer to HW-specific parameters
+ * vgic_v2_probe - probe for a GICv2 compatible interrupt controller
+ * @gic_kvm_info:  pointer to the GIC description
+ * @ops:   address of a pointer to the GICv2 operations
+ * @params:address of a pointer to HW-specific parameters
  *
  * Returns 0 if a GICv2 has been found, with the low level operations
  * in *ops and the HW parameters in *params. Returns an error code
  * otherwise.
  */
-int vgic_v2_probe(struct device_node *vgic_node,
- const struct vgic_ops **ops,
- const struct vgic_params **params)
+int vgic_v2_probe(const struct gic_kvm_info *gic_kvm_info,
+  const struct vgic_ops **ops,
+  const struct vgic_params **params)
 {
int ret;
-   struct resource vctrl_res;
-   struct resource vcpu_res;
struct vgic_params *vgic = &vgic_v2_params;
 
-   vgic->maint_irq = irq_of_parse_and_map(vgic_node, 0);
-   if (!vgic->maint_irq) {
-   kvm_err("error getting vgic maintenance irq from DT\n");
+   if (gic_kvm_info->maint_irq < 0) {
+   kvm_err("error getting vgic maintenance irq\n");
ret = -ENXIO;
goto out;
}
+   vgic->maint_irq = gic_kvm_info->maint_irq;
 
-   ret = of_address_to_resource(vgic_node, 2, &vctrl_res);
-   if (ret) {
-   kvm_err("Cannot obtain GICH resource\n");
+   if (!gic_kvm_info->vctrl_size) {
+   kvm_err("GICH not present in the firmware table\n");
+   ret = -ENXIO;
goto out;
}
 
-   vgic->vctrl_base = of_iomap(vgic_node, 2);
+   vgic->vctrl_base = ioremap(gic_kvm_info->vctrl_base,
+  gic_kvm_info->vctrl_size);
if (!vgic->vctrl_base) {
kvm_err("Cannot ioremap GICH\n");
ret = -ENOMEM;
@@ -219,30 +215,15 @@ int vgic_v2_probe(struct device_node *vgic_node,
vgic->nr_lr = (vgic->nr_lr & 0x3f) + 1;
 
ret = create_hyp_io_mappings(vgic->vctrl_base,
-vgic->vctrl_base + 
resource_size(&vctrl_res),
-vctrl_res.start);
+vgic->vctrl_base + 
gic_kvm_info->vctrl_size,
+ 

[PATCH 3/5] irqchip/gic-v2: Parse and export virtual GIC information

[Julien Grall]

For now, the firmware tables are parsed 2 times: once in the GIC
drivers, the other timer when initializing the vGIC. It means code
duplication and make more tedious to add the support for another
firmware table (like ACPI).

Introduce a new structure and set of helpers to get/set the virtual GIC
information. Also fill up the structure for GICv2.

Signed-off-by: Julien Grall 
---

Cc: Thomas Gleixner 
Cc: Jason Cooper 
Cc: Marc Zyngier 

 drivers/irqchip/irq-gic-common.c   | 13 ++
 drivers/irqchip/irq-gic-common.h   |  3 ++
 drivers/irqchip/irq-gic.c  | 78 +-
 include/linux/irqchip/arm-gic-common.h | 34 +++
 4 files changed, 127 insertions(+), 1 deletion(-)
 create mode 100644 include/linux/irqchip/arm-gic-common.h

diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
index f174ce0..704caf4 100644
--- a/drivers/irqchip/irq-gic-common.c
+++ b/drivers/irqchip/irq-gic-common.c
@@ -21,6 +21,19 @@
 
 #include "irq-gic-common.h"
 
+static const struct gic_kvm_info *gic_kvm_info;
+
+const struct gic_kvm_info *gic_get_kvm_info(void)
+{
+   return gic_kvm_info;
+}
+
+void gic_set_kvm_info(const struct gic_kvm_info *info)
+{
+   WARN(gic_kvm_info != NULL, "gic_kvm_info already set\n");
+   gic_kvm_info = info;
+}
+
 void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
void *data)
 {
diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h
index fff697d..205e5fd 100644
--- a/drivers/irqchip/irq-gic-common.h
+++ b/drivers/irqchip/irq-gic-common.h
@@ -19,6 +19,7 @@
 
 #include 
 #include 
+#include 
 
 struct gic_quirk {
const char *desc;
@@ -35,4 +36,6 @@ void gic_cpu_config(void __iomem *base, void 
(*sync_access)(void));
 void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
void *data);
 
+void gic_set_kvm_info(const struct gic_kvm_info *info);
+
 #endif /* _IRQ_GIC_COMMON_H */
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 911758c..d3a09a4 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -102,6 +102,8 @@ static struct static_key supports_deactivate = 
STATIC_KEY_INIT_TRUE;
 
 static struct gic_chip_data gic_data[CONFIG_ARM_GIC_MAX_NR] __read_mostly;
 
+static struct gic_kvm_info gic_v2_kvm_info;
+
 #ifdef CONFIG_GIC_NON_BANKED
 static void __iomem *gic_get_percpu_base(union gic_base *base)
 {
@@ -1190,6 +1192,44 @@ static bool gic_check_eoimode(struct device_node *node, 
void __iomem **base)
return true;
 }
 
+static void __init gic_of_setup_kvm_info(struct device_node *node)
+{
+   int ret;
+   struct resource r;
+   unsigned int irq;
+
+   gic_v2_kvm_info.type = GIC_V2;
+
+   irq = irq_of_parse_and_map(node, 0);
+   if (!irq)
+   gic_v2_kvm_info.maint_irq = -1;
+   else
+   gic_v2_kvm_info.maint_irq = irq;
+
+   ret = of_address_to_resource(node, 2, &r);
+   if (!ret) {
+   gic_v2_kvm_info.vctrl_base = r.start;
+   gic_v2_kvm_info.vctrl_size = resource_size(&r);
+   }
+
+   ret = of_address_to_resource(node, 3, &r);
+   if (!ret) {
+   if (!PAGE_ALIGNED(r.start))
+   pr_warn("GICV physical address 0x%llx not page 
aligned\n",
+   (unsigned long long)r.start);
+   else if (!PAGE_ALIGNED(resource_size(&r)))
+   pr_warn("GICV size 0x%llx not a multiple of page size 
0x%lx\n",
+   (unsigned long long)resource_size(&r),
+   PAGE_SIZE);
+   else {
+   gic_v2_kvm_info.vcpu_base = r.start;
+   gic_v2_kvm_info.vcpu_size = resource_size(&r);
+   }
+   }
+
+   gic_set_kvm_info(&gic_v2_kvm_info);
+}
+
 int __init
 gic_of_init(struct device_node *node, struct device_node *parent)
 {
@@ -1219,8 +1259,10 @@ gic_of_init(struct device_node *node, struct device_node 
*parent)
 
__gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset,
 &node->fwnode);
-   if (!gic_cnt)
+   if (!gic_cnt) {
gic_init_physaddr(node);
+   gic_of_setup_kvm_info(node);
+   }
 
if (parent) {
irq = irq_of_parse_and_map(node, 0);
@@ -1247,6 +1289,32 @@ IRQCHIP_DECLARE(pl390, "arm,pl390", gic_of_init);
 
 #ifdef CONFIG_ACPI
 static phys_addr_t cpu_phy_base __initdata;
+static struct
+{
+   u32 maint_irq;
+   int maint_irq_mode;
+   phys_addr_t vctrl_base;
+   phys_addr_t vcpu_base;
+} acpi_data __initdata;
+
+static void __init gic_acpi_setup_kvm_info(void)
+{
+   gic_v2_kvm_info.type = GIC_V2;
+
+   gic_v2_kvm_info.maint_irq = acpi_register_gsi(NULL,
+ acpi_data.maint_irq,
+   

[PATCH 2/5] KVM: arm/arm64: arch_timer: Rely on the arch timer to parse the firmware tables

[Julien Grall]

The firmware table is currently parsed by the virt timer code in order
to retrieve the virtual interrupt. However, this is already done by the
arch timer driver and therefore make the parsing code duplicated twice.

Introduce an helper to get the virtual interrupt number and use it in
the virtual timer code.

With this changes, the support for ACPI is coming free.

Signed-off-by: Julien Grall 
---
Cc: Daniel Lezcano 
Cc: Thomas Gleixner 
Cc: Christoffer Dall 
Cc: Marc Zyngier 
Cc: Gleb Natapov 
Cc: Paolo Bonzini 

 drivers/clocksource/arm_arch_timer.c |  2 ++
 include/clocksource/arm_arch_timer.h |  1 +
 virt/kvm/arm/arch_timer.c| 33 +++--
 3 files changed, 10 insertions(+), 26 deletions(-)

diff --git a/drivers/clocksource/arm_arch_timer.c 
b/drivers/clocksource/arm_arch_timer.c
index 6eb2c5d..3cdbefd 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -451,6 +451,8 @@ static struct arch_timer_kvm_info arch_timer_kvm_info;
 
 struct arch_timer_kvm_info *arch_timer_get_kvm_info(void)
 {
+   arch_timer_kvm_info.virtual_irq = arch_timer_ppi[VIRT_PPI];
+
return &arch_timer_kvm_info;
 }
 
diff --git a/include/clocksource/arm_arch_timer.h 
b/include/clocksource/arm_arch_timer.h
index 4d487f8..8890552 100644
--- a/include/clocksource/arm_arch_timer.h
+++ b/include/clocksource/arm_arch_timer.h
@@ -51,6 +51,7 @@ enum arch_timer_reg {
 
 struct arch_timer_kvm_info {
struct timecounter timecounter;
+   int virtual_irq;
 };
 
 #ifdef CONFIG_ARM_ARCH_TIMER
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index a669c6a..e4de549 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -17,7 +17,6 @@
  */
 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -375,45 +374,28 @@ static struct notifier_block kvm_timer_cpu_nb = {
.notifier_call = kvm_timer_cpu_notify,
 };
 
-static const struct of_device_id arch_timer_of_match[] = {
-   { .compatible   = "arm,armv7-timer",},
-   { .compatible   = "arm,armv8-timer",},
-   {},
-};
-
 int kvm_timer_hyp_init(void)
 {
-   struct device_node *np;
-   unsigned int ppi;
struct arch_timer_kvm_info *info;
int err;
 
info = arch_timer_get_kvm_info();
timecounter = &info->timecounter;
 
-   np = of_find_matching_node(NULL, arch_timer_of_match);
-   if (!np) {
-   kvm_err("kvm_arch_timer: can't find DT node\n");
+   host_vtimer_irq = info->virtual_irq;
+   if (host_vtimer_irq <= 0) {
+   kvm_err("kvm_arch_timer: can't find virtual timer info or 
config virtual timer interrupt\n");
return -ENODEV;
}
 
-   ppi = irq_of_parse_and_map(np, 2);
-   if (!ppi) {
-   kvm_err("kvm_arch_timer: no virtual timer interrupt\n");
-   err = -EINVAL;
-   goto out;
-   }
-
-   err = request_percpu_irq(ppi, kvm_arch_timer_handler,
+   err = request_percpu_irq(host_vtimer_irq, kvm_arch_timer_handler,
 "kvm guest timer", kvm_get_running_vcpus());
if (err) {
kvm_err("kvm_arch_timer: can't request interrupt %d (%d)\n",
-   ppi, err);
+   host_vtimer_irq, err);
goto out;
}
 
-   host_vtimer_irq = ppi;
-
err = __register_cpu_notifier(&kvm_timer_cpu_nb);
if (err) {
kvm_err("Cannot register timer CPU notifier\n");
@@ -426,14 +408,13 @@ int kvm_timer_hyp_init(void)
goto out_free;
}
 
-   kvm_info("%s IRQ%d\n", np->name, ppi);
+   kvm_info("vtimer IRQ%d\n", host_vtimer_irq);
on_each_cpu(kvm_timer_init_interrupt, NULL, 1);
 
goto out;
 out_free:
-   free_percpu_irq(ppi, kvm_get_running_vcpus());
+   free_percpu_irq(host_vtimer_irq, kvm_get_running_vcpus());
 out:
-   of_node_put(np);
return err;
 }
 
-- 
1.9.1

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

[PATCH 4/5] irqchip/gic-v3: Parse and export virtual GIC information

[Julien Grall]

Fill up the recently introduced gic_kvm_info with the virtual GIC
information.

Signed-off-by: Julien Grall 
---
Cc: Thomas Gleixner 
Cc: Jason Cooper 
Cc: Marc Zyngier 

 drivers/irqchip/irq-gic-v3.c   | 43 ++
 include/linux/irqchip/arm-gic-common.h |  1 +
 2 files changed, 44 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index d7be6dd..35f11ce 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -26,6 +26,7 @@
 #include 
 
 #include 
+#include 
 #include 
 
 #include 
@@ -53,6 +54,8 @@ struct gic_chip_data {
 static struct gic_chip_data gic_data __read_mostly;
 static struct static_key supports_deactivate = STATIC_KEY_INIT_TRUE;
 
+static struct gic_kvm_info gic_v3_kvm_info;
+
 #define gic_data_rdist()   (this_cpu_ptr(gic_data.rdists.rdist))
 #define gic_data_rdist_rd_base()   (gic_data_rdist()->rd_base)
 #define gic_data_rdist_sgi_base()  (gic_data_rdist_rd_base() + SZ_64K)
@@ -811,6 +814,44 @@ static void gicv3_enable_quirks(void)
 #endif
 }
 
+static void __init gic_of_setup_kvm_info(struct device_node *node)
+{
+   int ret;
+   struct resource r;
+   u32 gicv_idx;
+   unsigned int irq;
+
+   gic_v3_kvm_info.type = GIC_V3;
+
+   irq = irq_of_parse_and_map(node, 0);
+   if (!irq)
+   gic_v3_kvm_info.maint_irq = -1;
+   else
+   gic_v3_kvm_info.maint_irq = irq;
+
+   if (of_property_read_u32(node, "#redistributor-regions",
+&gicv_idx))
+   gicv_idx = 1;
+
+   gicv_idx += 3;  /* Also skip GICD, GICC, GICH */
+   ret = of_address_to_resource(node, gicv_idx, &r);
+   if (!ret) {
+   if (!PAGE_ALIGNED(r.start))
+   pr_warn("GICV physical address 0x%llx not page 
aligned\n",
+   (unsigned long long)r.start);
+   else if (!PAGE_ALIGNED(resource_size(&r)))
+   pr_warn("GICV size 0x%llx not a multiple of page size 
0x%lx\n",
+   (unsigned long long)resource_size(&r),
+   PAGE_SIZE);
+   else {
+   gic_v3_kvm_info.vcpu_base = r.start;
+   gic_v3_kvm_info.vcpu_size = resource_size(&r);
+   }
+   }
+
+   gic_set_kvm_info(&gic_v3_kvm_info);
+}
+
 static int __init gic_of_init(struct device_node *node, struct device_node 
*parent)
 {
void __iomem *dist_base;
@@ -908,6 +949,8 @@ static int __init gic_of_init(struct device_node *node, 
struct device_node *pare
gic_cpu_init();
gic_cpu_pm_init();
 
+   gic_of_setup_kvm_info(node);
+
return 0;
 
 out_free:
diff --git a/include/linux/irqchip/arm-gic-common.h 
b/include/linux/irqchip/arm-gic-common.h
index 30972b1..bfb10d0 100644
--- a/include/linux/irqchip/arm-gic-common.h
+++ b/include/linux/irqchip/arm-gic-common.h
@@ -14,6 +14,7 @@
 
 enum gic_type {
GIC_V2,
+   GIC_V3,
 };
 
 struct gic_kvm_info {
-- 
1.9.1

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

[PATCH 1/5] KVM: arm/arm64: arch_timer: Gather KVM specific information in a structure

[Julien Grall]

Introduce a structure which are filled up by the arch timer driver and
used by the virtual timer in KVM.

The first member of this structure will be the timecounter. More members
will be added later.

This is also dropping arch_timer_get_timecounter as it was only used by
the KVM code. Furthermore, a stub for the new helper hasn't been
introduced because KVM is requiring the arch timer for both ARM64 and
ARM32.

Signed-off-by: Julien Grall 
---

Cc: Daniel Lezcano 
Cc: Thomas Gleixner 
Cc: Christoffer Dall 
Cc: Marc Zyngier 
Cc: Gleb Natapov 
Cc: Paolo Bonzini 

 drivers/clocksource/arm_arch_timer.c |  9 +
 include/clocksource/arm_arch_timer.h | 12 ++--
 virt/kvm/arm/arch_timer.c|  6 +++---
 3 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/clocksource/arm_arch_timer.c 
b/drivers/clocksource/arm_arch_timer.c
index c64d543..6eb2c5d 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -447,11 +447,11 @@ static struct cyclecounter cyclecounter = {
.mask   = CLOCKSOURCE_MASK(56),
 };
 
-static struct timecounter timecounter;
+static struct arch_timer_kvm_info arch_timer_kvm_info;
 
-struct timecounter *arch_timer_get_timecounter(void)
+struct arch_timer_kvm_info *arch_timer_get_kvm_info(void)
 {
-   return &timecounter;
+   return &arch_timer_kvm_info;
 }
 
 static void __init arch_counter_register(unsigned type)
@@ -479,7 +479,8 @@ static void __init arch_counter_register(unsigned type)
clocksource_register_hz(&clocksource_counter, arch_timer_rate);
cyclecounter.mult = clocksource_counter.mult;
cyclecounter.shift = clocksource_counter.shift;
-   timecounter_init(&timecounter, &cyclecounter, start_count);
+   timecounter_init(&arch_timer_kvm_info.timecounter,
+&cyclecounter, start_count);
 
/* 56 bits minimum, so we assume worst case rollover */
sched_clock_register(arch_timer_read_counter, 56, arch_timer_rate);
diff --git a/include/clocksource/arm_arch_timer.h 
b/include/clocksource/arm_arch_timer.h
index 25d0914..4d487f8 100644
--- a/include/clocksource/arm_arch_timer.h
+++ b/include/clocksource/arm_arch_timer.h
@@ -49,11 +49,16 @@ enum arch_timer_reg {
 
 #define ARCH_TIMER_EVT_STREAM_FREQ 1   /* 100us */
 
+struct arch_timer_kvm_info {
+   struct timecounter timecounter;
+};
+
 #ifdef CONFIG_ARM_ARCH_TIMER
 
 extern u32 arch_timer_get_rate(void);
 extern u64 (*arch_timer_read_counter)(void);
-extern struct timecounter *arch_timer_get_timecounter(void);
+
+extern struct arch_timer_kvm_info *arch_timer_get_kvm_info(void);
 
 #else
 
@@ -67,11 +72,6 @@ static inline u64 arch_timer_read_counter(void)
return 0;
 }
 
-static inline struct timecounter *arch_timer_get_timecounter(void)
-{
-   return NULL;
-}
-
 #endif
 
 #endif
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 69bca18..a669c6a 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -385,11 +385,11 @@ int kvm_timer_hyp_init(void)
 {
struct device_node *np;
unsigned int ppi;
+   struct arch_timer_kvm_info *info;
int err;
 
-   timecounter = arch_timer_get_timecounter();
-   if (!timecounter)
-   return -ENODEV;
+   info = arch_timer_get_kvm_info();
+   timecounter = &info->timecounter;
 
np = of_find_matching_node(NULL, arch_timer_of_match);
if (!np) {
-- 
1.9.1

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

Re: [PATCH V1 0/7] Enable ACPI support for ARM KVM GIC

[Wei Huang]

On 2/8/16 10:39, Julien Grall wrote:
> Hi,
> 
> On 08/02/16 09:59, Marc Zyngier wrote:
>> On 05/02/16 17:07, Wei Huang wrote:
>>> Wei Huang (7):
>>>KVM: GIC: Move GIC DT probing code to GICv2 and GICv3 files
>>>KVM: GIC: Add extra fields to store GICH and GICV resource info
>>>KVM: GIC: Create a common probe function for GIC
>>>KVM: GICv2: Extract the common code from DT
>>>KVM: GICv2: Add ACPI probing function
>>>KVM: GICv3: Extract the common code from DT
>>>KVM: GICv3: Add ACPI probing function
>>>
>>>   include/kvm/arm_vgic.h  |  14 ++--
>>>   virt/kvm/arm/vgic-v2-emul.c |   4 +-
>>>   virt/kvm/arm/vgic-v2.c  | 186
>>> +---
>>>   virt/kvm/arm/vgic-v3.c  | 159
>>> -
>>>   virt/kvm/arm/vgic.c |  22 +-
>>>   5 files changed, 277 insertions(+), 108 deletions(-)
>>>
>>
>> So when I see this diffstat and the patches that follow, I cannot help
>> but think that we do have a disconnect here. You do add a bunch of ACPI
>> probing in KVM, to which I've already said no.
>>
>> I want to see the probing code in the GIC drivers, exported through a
>> common structure that KVM can then use. That's it. Nothing else.
> 
> I've been working on a patch series which rely on the GIC and arch timer
> drivers to get the necessary information to initialize the different KVM
> components (vGIC + vtimer).
> 
> I think this achieves the goal you have in mind. I will post the series
> in a few minutes.
> 

Julien,

Regarding arch_timer, there is an effort going on by Linaro. It has
impact on KVM arch_timer init. See https://lkml.org/lkml/2016/2/1/658.

-Wei

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

Re: [PATCH v3 21/23] arm64: hw_breakpoint: Allow EL2 breakpoints if running in HYP

[Marc Zyngier]

On 08/02/16 15:56, Catalin Marinas wrote:
> On Wed, Feb 03, 2016 at 06:00:14PM +, Marc Zyngier wrote:
>> @@ -76,6 +59,36 @@ static inline void decode_ctrl_reg(u32 reg,
>>  #define ARM_KERNEL_STEP_ACTIVE  1
>>  #define ARM_KERNEL_STEP_SUSPEND 2
>>  
>> +#define DBG_HMC_HYP (1 << 13)
>> +#define DBG_SSC_HYP (3 << 14)
>> +
>> +static inline u32 encode_ctrl_reg(struct arch_hw_breakpoint_ctrl ctrl)
>> +{
>> +u32 val = (ctrl.len << 5) | (ctrl.type << 3) | ctrl.enabled;
>> +
>> +if (is_kernel_in_hyp_mode() && ctrl.privilege == AARCH64_BREAKPOINT_EL1)
>> +val |= DBG_HMC_HYP | DBG_SSC_HYP;
>> +else
>> +val |= ctrl.privilege << 1;
>> +
>> +return val;
>> +}
>> +
>> +static inline void decode_ctrl_reg(u32 reg,
>> +   struct arch_hw_breakpoint_ctrl *ctrl)
>> +{
>> +ctrl->enabled   = reg & 0x1;
>> +reg >>= 1;
>> +if (is_kernel_in_hyp_mode())
>> +ctrl->privilege = !!(reg & (DBG_HMC_HYP >> 1));
> 
> I don't particularly like this part as it's not clear just by looking at
> the code that it, in fact, generates AARCH64_BREAKPOINT_EL1. I would
> make this clearer:
> 
>   if (is_kernel_in_hyp_mode() && (reg & (DBG_HMC_HYP >> 1)))
>   ctrl->privilege = AARCH64_BREAKPOINT_EL1;
> 
> Alternatively, you could define the PMC field value as:
> 
> #define AARCH64_BREAKPOINT_EL20
> 
> and change the privilege to EL1 after masking, something like:
> 
>   ctrl->privilege = reg & 0x3;
>   if (ctrl->privilege == AARCH64_BREAKPOINT_EL2)
>   ctrl->privilege = AARCH64_BREAKPOINT_EL1;
> 
> BTW, do we need to check is_kernel_in_hyp_mode() when decoding? Is there
> anything else that could have set this SSC/HMC/PMC fields other than
> encode_ctrl_reg()?

I was being overzealous, and your solution is clearly better. I ended up with 
the following:

diff --git a/arch/arm64/include/asm/hw_breakpoint.h 
b/arch/arm64/include/asm/hw_breakpoint.h
index 9732908..c872b2f 100644
--- a/arch/arm64/include/asm/hw_breakpoint.h
+++ b/arch/arm64/include/asm/hw_breakpoint.h
@@ -18,6 +18,7 @@
 
 #include 
 #include 
+#include 
 
 #ifdef __KERNEL__
 
@@ -35,24 +36,6 @@ struct arch_hw_breakpoint {
struct arch_hw_breakpoint_ctrl ctrl;
 };
 
-static inline u32 encode_ctrl_reg(struct arch_hw_breakpoint_ctrl ctrl)
-{
-   return (ctrl.len << 5) | (ctrl.type << 3) | (ctrl.privilege << 1) |
-   ctrl.enabled;
-}
-
-static inline void decode_ctrl_reg(u32 reg,
-  struct arch_hw_breakpoint_ctrl *ctrl)
-{
-   ctrl->enabled   = reg & 0x1;
-   reg >>= 1;
-   ctrl->privilege = reg & 0x3;
-   reg >>= 2;
-   ctrl->type  = reg & 0x3;
-   reg >>= 2;
-   ctrl->len   = reg & 0xff;
-}
-
 /* Breakpoint */
 #define ARM_BREAKPOINT_EXECUTE 0
 
@@ -62,6 +45,7 @@ static inline void decode_ctrl_reg(u32 reg,
 #define AARCH64_ESR_ACCESS_MASK(1 << 6)
 
 /* Privilege Levels */
+#define AARCH64_BREAKPOINT_EL2 0
 #define AARCH64_BREAKPOINT_EL1 1
 #define AARCH64_BREAKPOINT_EL0 2
 
@@ -76,6 +60,35 @@ static inline void decode_ctrl_reg(u32 reg,
 #define ARM_KERNEL_STEP_ACTIVE 1
 #define ARM_KERNEL_STEP_SUSPEND2
 
+#define DBG_HMC_HYP(1 << 13)
+#define DBG_SSC_HYP(3 << 14)
+
+static inline u32 encode_ctrl_reg(struct arch_hw_breakpoint_ctrl ctrl)
+{
+   u32 val = (ctrl.len << 5) | (ctrl.type << 3) | ctrl.enabled;
+
+   if (is_kernel_in_hyp_mode() && ctrl.privilege == AARCH64_BREAKPOINT_EL1)
+   val |= DBG_HMC_HYP | DBG_SSC_HYP;
+   else
+   val |= ctrl.privilege << 1;
+
+   return val;
+}
+
+static inline void decode_ctrl_reg(u32 reg,
+  struct arch_hw_breakpoint_ctrl *ctrl)
+{
+   ctrl->enabled   = reg & 0x1;
+   reg >>= 1;
+   ctrl->privilege = reg & 0x3;
+   if (ctrl->privilege == AARCH64_BREAKPOINT_EL2)
+   ctrl->privilege = AARCH64_BREAKPOINT_EL1;
+   reg >>= 2;
+   ctrl->type  = reg & 0x3;
+   reg >>= 2;
+   ctrl->len   = reg & 0xff;
+}
+
 /*
  * Limits.
  * Changing these will require modifications to the register accessors.

Was that what you had in mind?

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH V1 0/7] Enable ACPI support for ARM KVM GIC

[Marc Zyngier]

On 08/02/16 16:35, Wei Huang wrote:
> 
> 
> On 2/8/16 03:59, Marc Zyngier wrote:
>> Wei,
>>
>> On 05/02/16 17:07, Wei Huang wrote:
>>> This patch set enables ACPI support for KVM GIC. Note that the patches
>>> are in fact the V3 of previously submitted patches (search "Enable ACPI
>>> support for KVM ARM"). But because Fu Wei includes the arch_timer part
>>> in his series [1] and I have substantially re-written the GIC code in this
>>> revision, the version number is reset to v1. 
>>>
>>> By following Marc's prior comments, the main design idea is to let DT or
>>> ACPI code to fill out the "struct vgic_params" which are extended to
>>> include all GIC related info.
>>
>> I think you misread the comments I gave back in June (!), where I said:
>>
>> 
>> [...]
>>
>> Simply making available a global structure containing the base addresses
>> and interrupt should be enough, and could be shared with both DT and
>> ACPI. You could start your series by letting both GIC drivers expose
>> that information obtained through DT, convert KVM to use this structure,
>> and later on let ACPI fill in this structure too.
>>
>>> Anyway the difficulty is to find a common place to store and share info
>>> between other modules & KVM.
>>
>> Indeed. As a rule of thumb, I want to minimize the amount of gratuitous
>> divergence between DT and ACPI. So the sooner we extract the required
>> information from whatever firmware we have, the better.
>> 
>>
>>>
>>> [1] https://lkml.org/lkml/2016/2/1/658
>>>
>>> Thanks,
>>> -Wei
>>>
>>> Wei Huang (7):
>>>   KVM: GIC: Move GIC DT probing code to GICv2 and GICv3 files
>>>   KVM: GIC: Add extra fields to store GICH and GICV resource info
>>>   KVM: GIC: Create a common probe function for GIC
>>>   KVM: GICv2: Extract the common code from DT
>>>   KVM: GICv2: Add ACPI probing function
>>>   KVM: GICv3: Extract the common code from DT
>>>   KVM: GICv3: Add ACPI probing function
>>>
>>>  include/kvm/arm_vgic.h  |  14 ++--
>>>  virt/kvm/arm/vgic-v2-emul.c |   4 +-
>>>  virt/kvm/arm/vgic-v2.c  | 186 
>>> +---
>>>  virt/kvm/arm/vgic-v3.c  | 159 -
>>>  virt/kvm/arm/vgic.c |  22 +-
>>>  5 files changed, 277 insertions(+), 108 deletions(-)
>>>
>>
>> So when I see this diffstat and the patches that follow, I cannot help
>> but think that we do have a disconnect here. You do add a bunch of ACPI
>> probing in KVM, to which I've already said no.
>>
>> I want to see the probing code in the GIC drivers, exported through a
>> common structure that KVM can then use. That's it. Nothing else.
> 
> OK. I will create a V2 based on that idea.

You may want to synchronize with Julien (on CC), which has been working
on such a thing for a while.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH V1 0/7] Enable ACPI support for ARM KVM GIC

[Julien Grall]

Hi,

On 08/02/16 09:59, Marc Zyngier wrote:

On 05/02/16 17:07, Wei Huang wrote:

Wei Huang (7):
   KVM: GIC: Move GIC DT probing code to GICv2 and GICv3 files
   KVM: GIC: Add extra fields to store GICH and GICV resource info
   KVM: GIC: Create a common probe function for GIC
   KVM: GICv2: Extract the common code from DT
   KVM: GICv2: Add ACPI probing function
   KVM: GICv3: Extract the common code from DT
   KVM: GICv3: Add ACPI probing function

  include/kvm/arm_vgic.h  |  14 ++--
  virt/kvm/arm/vgic-v2-emul.c |   4 +-
  virt/kvm/arm/vgic-v2.c  | 186 +---
  virt/kvm/arm/vgic-v3.c  | 159 -
  virt/kvm/arm/vgic.c |  22 +-
  5 files changed, 277 insertions(+), 108 deletions(-)



So when I see this diffstat and the patches that follow, I cannot help
but think that we do have a disconnect here. You do add a bunch of ACPI
probing in KVM, to which I've already said no.

I want to see the probing code in the GIC drivers, exported through a
common structure that KVM can then use. That's it. Nothing else.


I've been working on a patch series which rely on the GIC and arch timer 
drivers to get the necessary information to initialize the different KVM 
components (vGIC + vtimer).


I think this achieves the goal you have in mind. I will post the series 
in a few minutes.


Regards,

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

Re: [PATCH V1 0/7] Enable ACPI support for ARM KVM GIC

[Wei Huang]

On 2/8/16 03:59, Marc Zyngier wrote:
> Wei,
> 
> On 05/02/16 17:07, Wei Huang wrote:
>> This patch set enables ACPI support for KVM GIC. Note that the patches
>> are in fact the V3 of previously submitted patches (search "Enable ACPI
>> support for KVM ARM"). But because Fu Wei includes the arch_timer part
>> in his series [1] and I have substantially re-written the GIC code in this
>> revision, the version number is reset to v1. 
>>
>> By following Marc's prior comments, the main design idea is to let DT or
>> ACPI code to fill out the "struct vgic_params" which are extended to
>> include all GIC related info.
> 
> I think you misread the comments I gave back in June (!), where I said:
> 
> 
> [...]
> 
> Simply making available a global structure containing the base addresses
> and interrupt should be enough, and could be shared with both DT and
> ACPI. You could start your series by letting both GIC drivers expose
> that information obtained through DT, convert KVM to use this structure,
> and later on let ACPI fill in this structure too.
> 
>> Anyway the difficulty is to find a common place to store and share info
>> between other modules & KVM.
> 
> Indeed. As a rule of thumb, I want to minimize the amount of gratuitous
> divergence between DT and ACPI. So the sooner we extract the required
> information from whatever firmware we have, the better.
> 
> 
>>
>> [1] https://lkml.org/lkml/2016/2/1/658
>>
>> Thanks,
>> -Wei
>>
>> Wei Huang (7):
>>   KVM: GIC: Move GIC DT probing code to GICv2 and GICv3 files
>>   KVM: GIC: Add extra fields to store GICH and GICV resource info
>>   KVM: GIC: Create a common probe function for GIC
>>   KVM: GICv2: Extract the common code from DT
>>   KVM: GICv2: Add ACPI probing function
>>   KVM: GICv3: Extract the common code from DT
>>   KVM: GICv3: Add ACPI probing function
>>
>>  include/kvm/arm_vgic.h  |  14 ++--
>>  virt/kvm/arm/vgic-v2-emul.c |   4 +-
>>  virt/kvm/arm/vgic-v2.c  | 186 
>> +---
>>  virt/kvm/arm/vgic-v3.c  | 159 -
>>  virt/kvm/arm/vgic.c |  22 +-
>>  5 files changed, 277 insertions(+), 108 deletions(-)
>>
> 
> So when I see this diffstat and the patches that follow, I cannot help
> but think that we do have a disconnect here. You do add a bunch of ACPI
> probing in KVM, to which I've already said no.
> 
> I want to see the probing code in the GIC drivers, exported through a
> common structure that KVM can then use. That's it. Nothing else.

OK. I will create a V2 based on that idea.

-Wei

> 
> Thanks,
> 
>   M.
> 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 23/23] arm64: Panic when VHE and non VHE CPUs coexist

[Mark Rutland]

On Mon, Feb 08, 2016 at 04:04:46PM +, Catalin Marinas wrote:
> On Wed, Feb 03, 2016 at 06:00:16PM +, Marc Zyngier wrote:
> > Having both VHE and non-VHE capable CPUs in the same system
> > is likely to be a recipe for disaster.
> > 
> > If the boot CPU has VHE, but a secondary is not, we won't be
> > able to downgrade and run the kernel at EL1. Add CPU hotplug
> > to the mix, and this produces a terrifying mess.
> > 
> > Let's solve the problem once and for all. If you mix VHE and
> > non-VHE CPUs in the same system, you deserve to loose, and this
> > patch makes sure you don't get a chance.
> > 
> > This is implemented by storing the kernel execution level in
> > a global variable. Secondaries will park themselves in a
> > WFI loop if they observe a mismatch. Also, the primary CPU
> > will detect that the secondary CPU has died on a mismatched
> > execution level. Panic will follow.
> > 
> > Signed-off-by: Marc Zyngier 
> > ---
> >  arch/arm64/include/asm/virt.h | 17 +
> >  arch/arm64/kernel/head.S  | 20 
> >  arch/arm64/kernel/smp.c   |  3 +++
> >  3 files changed, 40 insertions(+)
> > 
> > diff --git a/arch/arm64/include/asm/virt.h b/arch/arm64/include/asm/virt.h
> > index 9f22dd6..f81a345 100644
> > --- a/arch/arm64/include/asm/virt.h
> > +++ b/arch/arm64/include/asm/virt.h
> > @@ -36,6 +36,11 @@
> >   */
> >  extern u32 __boot_cpu_mode[2];
> >  
> > +/*
> > + * __run_cpu_mode records the mode the boot CPU uses for the kernel.
> > + */
> > +extern u32 __run_cpu_mode[2];
> > +
> >  void __hyp_set_vectors(phys_addr_t phys_vector_base);
> >  phys_addr_t __hyp_get_vectors(void);
> >  
> > @@ -60,6 +65,18 @@ static inline bool is_kernel_in_hyp_mode(void)
> > return el == CurrentEL_EL2;
> >  }
> >  
> > +static inline bool is_kernel_mode_mismatched(void)
> > +{
> > +   /*
> > +* A mismatched CPU will have written its own CurrentEL in
> > +* __run_cpu_mode[1] (initially set to zero) after failing to
> > +* match the value in __run_cpu_mode[0]. Thus, a non-zero
> > +* value in __run_cpu_mode[1] is enough to detect the
> > +* pathological case.
> > +*/
> > +   return !!ACCESS_ONCE(__run_cpu_mode[1]);
> > +}
> > +
> >  /* The section containing the hypervisor text */
> >  extern char __hyp_text_start[];
> >  extern char __hyp_text_end[];
> > diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> > index 6f2f377..f9b6a5b 100644
> > --- a/arch/arm64/kernel/head.S
> > +++ b/arch/arm64/kernel/head.S
> > @@ -578,7 +578,24 @@ ENTRY(set_cpu_boot_mode_flag)
> >  1: str w20, [x1]   // This CPU has booted in EL1
> > dmb sy
> > dc  ivac, x1// Invalidate potentially stale 
> > cache line
> > +   adr_l   x1, __run_cpu_mode
> > +   ldr w0, [x1]
> > +   mrs x20, CurrentEL
> > +   cbz x0, skip_el_check
> > +   cmp x0, x20
> > +   bne mismatched_el
> > ret
> > +skip_el_check: // Only the first CPU gets to set the 
> > rule
> > +   str w20, [x1]
> > +   dmb sy
> > +   dc  ivac, x1// Invalidate potentially stale cache line
> > +   ret
> > +mismatched_el:
> > +   str w20, [x1, #4]
> > +   dmb sy
> > +   dc  ivac, x1// Invalidate potentially stale cache line
> > +1: wfi
> > +   b   1b
> >  ENDPROC(set_cpu_boot_mode_flag)
> 
> Do we need to wait for the D-cache maintenance completion before
> entering WFI (like issuing a DSB)? Same for the skip_el_check path
> before the RET.

We would need that to complete the maintenance, yes.

However, given we're going into WFI immediately afterwards, and not
signalling completion to other CPUs, what we gain is somewhat
questionable.

Perhaps it's always better to do the maintenance on the read side (for
consistency with [1]).

Regardless, we'd probably want a DSB to ensure the write had
completed...

Mark.

[1] 
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-February/404661.html
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 23/23] arm64: Panic when VHE and non VHE CPUs coexist

[Catalin Marinas]

On Wed, Feb 03, 2016 at 06:00:16PM +, Marc Zyngier wrote:
> Having both VHE and non-VHE capable CPUs in the same system
> is likely to be a recipe for disaster.
> 
> If the boot CPU has VHE, but a secondary is not, we won't be
> able to downgrade and run the kernel at EL1. Add CPU hotplug
> to the mix, and this produces a terrifying mess.
> 
> Let's solve the problem once and for all. If you mix VHE and
> non-VHE CPUs in the same system, you deserve to loose, and this
> patch makes sure you don't get a chance.
> 
> This is implemented by storing the kernel execution level in
> a global variable. Secondaries will park themselves in a
> WFI loop if they observe a mismatch. Also, the primary CPU
> will detect that the secondary CPU has died on a mismatched
> execution level. Panic will follow.
> 
> Signed-off-by: Marc Zyngier 
> ---
>  arch/arm64/include/asm/virt.h | 17 +
>  arch/arm64/kernel/head.S  | 20 
>  arch/arm64/kernel/smp.c   |  3 +++
>  3 files changed, 40 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/virt.h b/arch/arm64/include/asm/virt.h
> index 9f22dd6..f81a345 100644
> --- a/arch/arm64/include/asm/virt.h
> +++ b/arch/arm64/include/asm/virt.h
> @@ -36,6 +36,11 @@
>   */
>  extern u32 __boot_cpu_mode[2];
>  
> +/*
> + * __run_cpu_mode records the mode the boot CPU uses for the kernel.
> + */
> +extern u32 __run_cpu_mode[2];
> +
>  void __hyp_set_vectors(phys_addr_t phys_vector_base);
>  phys_addr_t __hyp_get_vectors(void);
>  
> @@ -60,6 +65,18 @@ static inline bool is_kernel_in_hyp_mode(void)
>   return el == CurrentEL_EL2;
>  }
>  
> +static inline bool is_kernel_mode_mismatched(void)
> +{
> + /*
> +  * A mismatched CPU will have written its own CurrentEL in
> +  * __run_cpu_mode[1] (initially set to zero) after failing to
> +  * match the value in __run_cpu_mode[0]. Thus, a non-zero
> +  * value in __run_cpu_mode[1] is enough to detect the
> +  * pathological case.
> +  */
> + return !!ACCESS_ONCE(__run_cpu_mode[1]);
> +}
> +
>  /* The section containing the hypervisor text */
>  extern char __hyp_text_start[];
>  extern char __hyp_text_end[];
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index 6f2f377..f9b6a5b 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -578,7 +578,24 @@ ENTRY(set_cpu_boot_mode_flag)
>  1:   str w20, [x1]   // This CPU has booted in EL1
>   dmb sy
>   dc  ivac, x1// Invalidate potentially stale 
> cache line
> + adr_l   x1, __run_cpu_mode
> + ldr w0, [x1]
> + mrs x20, CurrentEL
> + cbz x0, skip_el_check
> + cmp x0, x20
> + bne mismatched_el
>   ret
> +skip_el_check:   // Only the first CPU gets to set the 
> rule
> + str w20, [x1]
> + dmb sy
> + dc  ivac, x1// Invalidate potentially stale cache line
> + ret
> +mismatched_el:
> + str w20, [x1, #4]
> + dmb sy
> + dc  ivac, x1// Invalidate potentially stale cache line
> +1:   wfi
> + b   1b
>  ENDPROC(set_cpu_boot_mode_flag)

Do we need to wait for the D-cache maintenance completion before
entering WFI (like issuing a DSB)? Same for the skip_el_check path
before the RET.

Otherwise:

Acked-by: Catalin Marinas 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 22/23] arm64: VHE: Add support for running Linux in EL2 mode

[Catalin Marinas]

On Wed, Feb 03, 2016 at 06:00:15PM +, Marc Zyngier wrote:
> With ARMv8.1 VHE, the architecture is able to (almost) transparently
> run the kernel at EL2, despite being written for EL1.
> 
> This patch takes care of the "almost" part, mostly preventing the kernel
> from dropping from EL2 to EL1, and setting up the HYP configuration.
> 
> Signed-off-by: Marc Zyngier 

Acked-by: Catalin Marinas 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 21/23] arm64: hw_breakpoint: Allow EL2 breakpoints if running in HYP

[Catalin Marinas]

On Wed, Feb 03, 2016 at 06:00:14PM +, Marc Zyngier wrote:
> @@ -76,6 +59,36 @@ static inline void decode_ctrl_reg(u32 reg,
>  #define ARM_KERNEL_STEP_ACTIVE   1
>  #define ARM_KERNEL_STEP_SUSPEND  2
>  
> +#define DBG_HMC_HYP  (1 << 13)
> +#define DBG_SSC_HYP  (3 << 14)
> +
> +static inline u32 encode_ctrl_reg(struct arch_hw_breakpoint_ctrl ctrl)
> +{
> + u32 val = (ctrl.len << 5) | (ctrl.type << 3) | ctrl.enabled;
> +
> + if (is_kernel_in_hyp_mode() && ctrl.privilege == AARCH64_BREAKPOINT_EL1)
> + val |= DBG_HMC_HYP | DBG_SSC_HYP;
> + else
> + val |= ctrl.privilege << 1;
> +
> + return val;
> +}
> +
> +static inline void decode_ctrl_reg(u32 reg,
> +struct arch_hw_breakpoint_ctrl *ctrl)
> +{
> + ctrl->enabled   = reg & 0x1;
> + reg >>= 1;
> + if (is_kernel_in_hyp_mode())
> + ctrl->privilege = !!(reg & (DBG_HMC_HYP >> 1));

I don't particularly like this part as it's not clear just by looking at
the code that it, in fact, generates AARCH64_BREAKPOINT_EL1. I would
make this clearer:

if (is_kernel_in_hyp_mode() && (reg & (DBG_HMC_HYP >> 1)))
ctrl->privilege = AARCH64_BREAKPOINT_EL1;

Alternatively, you could define the PMC field value as:

#define AARCH64_BREAKPOINT_EL2  0

and change the privilege to EL1 after masking, something like:

ctrl->privilege = reg & 0x3;
if (ctrl->privilege == AARCH64_BREAKPOINT_EL2)
ctrl->privilege = AARCH64_BREAKPOINT_EL1;

BTW, do we need to check is_kernel_in_hyp_mode() when decoding? Is there
anything else that could have set this SSC/HMC/PMC fields other than
encode_ctrl_reg()?

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

Re: [GIT PULL] KVM/ARM updates for 4.5-rc2

[Marc Zyngier]

On 08/02/16 15:23, Paolo Bonzini wrote:
> 
> 
> On 28/01/2016 11:30, Marc Zyngier wrote:
>> Hi Paolo,
>>
>> Please find below the KVM/ARM updates for 4.5-rc2. Mostly fixes as a
>> result of Shannon's work on PMU emulation, which has outlined a few
>> nits here and there, plus a correctness fix from Dave.
>>
>> Please pull!
>>
>> Thanks,
>>
>>  M.
>>
>> The following changes since commit 92e963f50fc74041b5e9e744c330dca48e04f08d:
>>
>>   Linux 4.5-rc1 (2016-01-24 13:06:47 -0800)
>>
>> are available in the git repository at:
>>
>>   git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git 
>> tags/kvm-arm-for-4.5-rc2
>>
>> for you to fetch changes up to 6327f35a2010c06a3bc2bfb14202a38764fb9920:
>>
>>   arm64: KVM: Fix guest dead loop when register accessor returns false 
>> (2016-01-24 21:56:01 +)
>>
>> 
>> KVM/ARM fixes for v4.5-rc2
>>
>> A few random fixes, mostly coming from the PMU work by Shannon:
>>
>> - fix for injecting faults coming from the guest's userspace
>> - cleanup for our CPTR_EL2 accessors (reserved bits)
>> - fix for a bug impacting perf (user/kernel discrimination)
>> - fix for a 32bit sysreg handling bug
>>
>> 
> 
> Finally pulled, thanks.

Thanks Paolo. I'll send another PR in a couple of days for another fix
that landed last week.

M.
-- 
Jazz is not dead. It just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [GIT PULL] KVM/ARM updates for 4.5-rc2

[Paolo Bonzini]

On 28/01/2016 11:30, Marc Zyngier wrote:
> Hi Paolo,
> 
> Please find below the KVM/ARM updates for 4.5-rc2. Mostly fixes as a
> result of Shannon's work on PMU emulation, which has outlined a few
> nits here and there, plus a correctness fix from Dave.
> 
> Please pull!
> 
> Thanks,
> 
>   M.
> 
> The following changes since commit 92e963f50fc74041b5e9e744c330dca48e04f08d:
> 
>   Linux 4.5-rc1 (2016-01-24 13:06:47 -0800)
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git 
> tags/kvm-arm-for-4.5-rc2
> 
> for you to fetch changes up to 6327f35a2010c06a3bc2bfb14202a38764fb9920:
> 
>   arm64: KVM: Fix guest dead loop when register accessor returns false 
> (2016-01-24 21:56:01 +)
> 
> 
> KVM/ARM fixes for v4.5-rc2
> 
> A few random fixes, mostly coming from the PMU work by Shannon:
> 
> - fix for injecting faults coming from the guest's userspace
> - cleanup for our CPTR_EL2 accessors (reserved bits)
> - fix for a bug impacting perf (user/kernel discrimination)
> - fix for a 32bit sysreg handling bug
> 
> 

Finally pulled, thanks.

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

Re: [PATCH v3 20/23] arm64: perf: Count EL2 events if the kernel is running in HYP

[Catalin Marinas]

On Wed, Feb 03, 2016 at 06:00:13PM +, Marc Zyngier wrote:
> When the kernel is running in HYP (with VHE), it is necessary to
> include EL2 events if the user requests counting kernel or
> hypervisor events.
> 
> Signed-off-by: Marc Zyngier 

Acked-by: Catalin Marinas 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 19/23] arm64: KVM: Move most of the fault decoding to C

[Catalin Marinas]

On Wed, Feb 03, 2016 at 06:00:12PM +, Marc Zyngier wrote:
> The fault decoding process (including computing the IPA in the case
> of a permission fault) would be much better done in C code, as we
> have a reasonable infrastructure to deal with the VHE/non-VHE
> differences.
> 
> Let's move the whole thing to C, including the workaround for
> erratum 834220, and just patch the odd ESR_EL2 access remaining
> in hyp-entry.S.
> 
> Signed-off-by: Marc Zyngier 

Acked-by: Catalin Marinas 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 05/23] arm64: Add ARM64_HAS_VIRT_HOST_EXTN feature

[Catalin Marinas]

On Wed, Feb 03, 2016 at 05:59:58PM +, Marc Zyngier wrote:
> Add a new ARM64_HAS_VIRT_HOST_EXTN features to indicate that the
> CPU has the ARMv8.1 VHE capability.
> 
> This will be used to trigger kernel patching in KVM.
> 
> Acked-by: Christoffer Dall 
> Signed-off-by: Marc Zyngier 

Acked-by: Catalin Marinas 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 03/23] arm/arm64: Add new is_kernel_in_hyp_mode predicate

[Catalin Marinas]

On Wed, Feb 03, 2016 at 05:59:56PM +, Marc Zyngier wrote:
> With ARMv8.1 VHE extension, it will be possible to run the kernel
> at EL2 (aka HYP mode). In order for the kernel to easily find out
> where it is running, add a new predicate that returns whether or
> not the kernel is in HYP mode.
> 
> For completeness, the 32bit code also get such a predicate (always
> returning false) so that code common to both architecture (timers,
> KVM) can use it transparently.
> 
> Acked-by: Christoffer Dall 
> Signed-off-by: Marc Zyngier 

Acked-by: Catalin Marinas 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH] arm/arm64: KVM: Handle out-of-RAM cache maintenance as a NOP

[Marc Zyngier]

So far, our handling of cache maintenance by VA has been pretty
simple: Either the access is in the guest RAM and generates a S2
fault, which results in the page being mapped RW, or we go down
the io_mem_abort() path, and nuke the guest.

The first one is fine, but the second one is extremely weird.
Treating the CM as an I/O is wrong, and nothing in the ARM ARM
indicates that we should generate a fault for something that
cannot end-up in the cache anyway (even if the guest maps it,
it will keep on faulting at stage-2 for emulation).

So let's just skip this instruction, and let the guest get away
with it.

Signed-off-by: Marc Zyngier 
---
 arch/arm/include/asm/kvm_emulate.h   |  5 +
 arch/arm/kvm/mmu.c   | 17 +
 arch/arm64/include/asm/kvm_emulate.h |  5 +
 3 files changed, 27 insertions(+)

diff --git a/arch/arm/include/asm/kvm_emulate.h 
b/arch/arm/include/asm/kvm_emulate.h
index 3095df0..f768797 100644
--- a/arch/arm/include/asm/kvm_emulate.h
+++ b/arch/arm/include/asm/kvm_emulate.h
@@ -143,6 +143,11 @@ static inline bool kvm_vcpu_dabt_iss1tw(struct kvm_vcpu 
*vcpu)
return kvm_vcpu_get_hsr(vcpu) & HSR_DABT_S1PTW;
 }
 
+static inline bool kvm_vcpu_dabt_iscm(struct kvm_vcpu *vcpu)
+{
+   return !!(kvm_vcpu_get_hsr(vcpu) & HSR_DABT_CM);
+}
+
 /* Get Access Size from a data abort */
 static inline int kvm_vcpu_dabt_get_as(struct kvm_vcpu *vcpu)
 {
diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index aba61fd..1a5f2ea 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -1431,6 +1431,23 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct 
kvm_run *run)
}
 
/*
+* Check for a cache maintenance operation. Since we
+* ended-up here, we know it is outside of any memory
+* slot. But we can't find out if that is for a device,
+* or if the guest is just being stupid (going all the
+* way to userspace is not an option - what would you
+* write?).
+*
+* So let's assume that the guest is just being
+* cautious, and skip the instruction.
+*/
+   if (kvm_vcpu_dabt_iscm(vcpu)) {
+   kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
+   ret = 1;
+   goto out_unlock;
+   }
+
+   /*
 * The IPA is reported as [MAX:12], so we need to
 * complement it with the bottom 12 bits from the
 * faulting VA. This is always 12 bits, irrespective
diff --git a/arch/arm64/include/asm/kvm_emulate.h 
b/arch/arm64/include/asm/kvm_emulate.h
index 3066328..01cdf5f 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -185,6 +185,11 @@ static inline bool kvm_vcpu_dabt_iss1tw(const struct 
kvm_vcpu *vcpu)
return !!(kvm_vcpu_get_hsr(vcpu) & ESR_ELx_S1PTW);
 }
 
+static inline bool kvm_vcpu_dabt_iscm(const struct kvm_vcpu *vcpu)
+{
+   return !!(kvm_vcpu_get_hsr(vcpu) & ESR_ELx_CM);
+}
+
 static inline int kvm_vcpu_dabt_get_as(const struct kvm_vcpu *vcpu)
 {
return 1 << ((kvm_vcpu_get_hsr(vcpu) & ESR_ELx_SAS) >> 
ESR_ELx_SAS_SHIFT);
-- 
2.1.4

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

Re: ARM PCI/MSI KVM passthrough with GICv2M

[Eric Auger]

Hi Alex, Christoffer,
On 02/08/2016 10:48 AM, Christoffer Dall wrote:
> On Fri, Feb 05, 2016 at 11:17:00AM -0700, Alex Williamson wrote:
>> On Fri, 5 Feb 2016 18:32:07 +0100
>> Eric Auger  wrote:
>>
>>> Hi Alex,
>>>
>>> I tried to sketch a proposal for guaranteeing the IRQ integrity when
>>> doing ARM PCI/MSI passthrough with ARM GICv2M msi-controller. This is
>>> based on extended VFIO group viability control, as detailed below.
>>>
>>> As opposed to ARM GICv3 ITS, this MSI controller does *not* support IRQ
>>> remapping. It can expose 1 or more 4kB MSI frame. Each frame contains a
>>> single register where the msi data is written.
>>>
>>> I would be grateful to you if you could tell me whether it makes any sense.
>>>
>>> Thanks in advance
>>>
>>> Best Regards
>>>
>>> Eric
>>>
>>>
>>> 1) GICv2m with a single 4kB single frame
>>>all devices having this msi-controller as msi-parent share this
>>>single MSI frame. Those devices can work on behalf of the host
>>>or work on behalf of 1 or more guests (KVM assigned devices). We
>>>must make sure either the host only or 1 single VM can access to the
>>>single frame to guarantee interrupt integrity: a device assigned
>>>to 1 VM should not be able to trigger MSI targeted to the host
>>>or another VM.
>>>
>>>I would propose to extend the VFIO notion of group viability.
>>>Currently a VFIO group is viable if:
>>>all devices belonging to the same group are bound to a VFIO driver
>>>or unbound.
>>>
>>>Let's imagine we extend the viability check as follows:
>>>
>>>0) keep the current viable check: all the devices belonging to
>>>   the group must be vfio bound or unbound.
>>>1) retrieve the MSI parent of the device and list all the
>>>   other devices using that MSI controller as MSI-parent (does not
>>>   look straightforward):
>>>2) they must be VFIO driver bound or unbound as well (meaning
>>>   they are not used by the host). If not, reject device attachment
>>>- in case they are VFIO bound (a VFIO group is set):
>>>  x if all VFIO containers are the same as the one of the device's
>>>we try to attach, that's OK. This means the other devices
>>>use different IOMMU mappings, eventually will target the
>>>MSI frame but they all work for the same user space client/VM.
>>>  x 1 or more devices has a different container than the device
>>>under attachment:
>>>It works on behalf of a different user space client/VM,
>>>we can't attach the new device. I think there is a case however
>>>where severals containers can be opened by a single QEMU.
>>>
>>> Of course the dynamic aspects, ie a new device showing up or an unbind
>>> event bring significant complexity.
>>>
>>> 2) GICv2M with multiple 4kB frames
>>>Each msi-frame is enumerated as msi-controller. The device tree
>>>statically defines which device is attached to each msi frame.
>>>In case devices are assigned we cannot change this attachment
>>>anyway since there might be physical contraints behind.
>>>So devices likely to be assigned to guests should be linked to a
>>>different MSI frame than devices that are not.
>>>
>>>I think extended viability concept can be used as well.
>>>
>>>This model still is not ideal: in case we have a SR-IOV device
>>>plugged onto an host bridge attached to a single MSI parent you won't
>>>be able anyway to have 1 Virtual Function working for host and 1 VF
>>>working for a guest. Only Interrupt translation (ITS) will bring that
>>>feature.
>>>
>>> 3) GICv3 ITS
>>>This one supports interrupt translation service ~ Intel
>>>IRQ remapping.
>>>This means a single frame can be used by all devices. A deviceID is
>>>used exclusively by the host or a guest. I assume the ITS driver
>>>allocates/populates deviceid interrupt translation table featuring
>>>separate LPI spaces ie by construction different ITT cannot feature
>>>same LPIs. So no need to do the extended viability test.
>>>
>>>The MSI controller should have a property telling whether
>>>it supports interrupt translation. This kind of property currently
>>>exists on IOMMU side for INTEL remapping.
>>>
>>
>> Hi Eric,
>>
>> Would anyone be terribly upset if we simply assume the worst case
>> scenario on GICv2m/M, have the IOMMU not claim IOMMU_CAP_INTR_REMAP, and
>> require the user to opt-in via the allow_unsafe_interrupts on the
>> vfio_iommu_type1 module?  That would make it very compatible with what
>> we already do on x86, where it really is all or nothing.  
> 
> meaning either you allow unsafe multiplexing with passthrough in every
> flavor (unsafely) or you don't allow it at all?
that's my understanding. if the iommu does not expose
IOMMU_CAP_INTR_REMAP, the end-user must explicitly turn
allow_unsafe_interrupts on. On ARM we will have the handle the fact the
interrupt translation is handled on interrupt con

Re: PCIe passthrough support on ARM

[Eric Auger]

Hi Edward,
On 02/08/2016 12:13 PM, Edward Cragg wrote:
> Hi Eric,
> 
> On Thu, Feb 04, 2016 at 06:30:34PM +0100, Eric Auger wrote:
>> Hi Edward,
>> On 02/04/2016 05:53 PM, Edward Cragg wrote:
>>> Hi,
>>>
>>> I'm involved in planning a project for which there is a requirement for PCIe
>>> passthrough in KVM on ARMv8. We have no hardware to test on at the moment.
>>>
>>> I understand that virtualisation support for ARM is quite young, and it 
>>> seems
>>> like support is trickling in at the moment for this sort of thing.
>>>
>>> Is anyone working on PCIe passthrough on ARM platforms who might be able to
>>> share some insight into what would be required to support this?
>> Yes I am currently working on this topic at the moment and especially on
>> ARM PCIe passthrough with MSI enabled. I did not test PCIe passthrough
>> with legacy INTx (MSI disabled) but this should work already with
>> upstreamed QEMU and kernel.
>>
>> With MSI enabled, the code is not upstreamed yet.
>>
>> I have a QEMU series:
>>
>> [RFC v2 0/8] KVM PCI/MSI passthrough with mach-virt
>> (https://www.mail-archive.com/qemu-devel@nongnu.org/msg349574.html)
>>
>> and a kernel series:
>> [PATCH 00/10] KVM PCIe/MSI passthrough on ARM/ARM64"
>>   (https://lkml.org/lkml/2016/1/26/371)
>>
>> This is functional, tested on v8 AMD HW with a GICv2M (single MSI frame).
>>
>> This is in an early stage, especially on kernel side since lots of
>> issues still need to be addressed, especially handling proper IRQ isolation.
> 
> Thank you, this is good to know.
> 
>> What kind of MSI controller do you plan to use?
> 
> Do you mean the GIC itself? From registers, it appears to be a standard ARM
> GIC, though i'm not sure exactly which one yet. However, it's stated in the
> processor's datasheet that legacy interrupts aren't supported. It's one of the
> newer Samsung Exynos processors.
I meant GICv2m (featuring single or multiple MSI frames) or GICv3 ITS or
any other proprietary interrupt controller IP supporting MSIs.

Best Regards

Eric
> 
> Thanks,
> 
> Ed
> 
>> Hope this helps
>>
>> Best Regards
>>
>> Eric
>>
>>>
>>> Thanks,
>>>
>>> Ed
>>> ___
>>> kvmarm mailing list
>>> kvmarm@lists.cs.columbia.edu
>>> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
>>>
>>
>> ___
>> kvmarm mailing list
>> kvmarm@lists.cs.columbia.edu
>> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

Re: [PATCH v11 21/21] KVM: ARM64: Add a new vcpu device control group for PMUv3

[Christoffer Dall]

On Fri, Feb 05, 2016 at 03:14:16PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao 
> 
> To configure the virtual PMUv3 overflow interrupt number, we use the
> vcpu kvm_device ioctl, encapsulating the KVM_ARM_VCPU_PMU_V3_IRQ
> attribute within the KVM_ARM_VCPU_PMU_V3_CTRL group.
> 
> After configuring the PMUv3, call the vcpu ioctl with attribute
> KVM_ARM_VCPU_PMU_V3_INIT to initialize the PMUv3.
> 
> Signed-off-by: Shannon Zhao 
> Acked-by: Peter Maydell 
> Reviewed-by: Andrew Jones 
> ---
> CC: Peter Maydell 
> ---
>  Documentation/virtual/kvm/devices/vcpu.txt |  24 ++
>  arch/arm/include/asm/kvm_host.h|  15 
>  arch/arm/kvm/arm.c |   3 +
>  arch/arm64/include/asm/kvm_host.h  |   6 ++
>  arch/arm64/include/uapi/asm/kvm.h  |   5 ++
>  arch/arm64/kvm/guest.c |  51 
>  include/kvm/arm_pmu.h  |  23 ++
>  virt/kvm/arm/pmu.c | 128 
> +
>  8 files changed, 255 insertions(+)
> 
> diff --git a/Documentation/virtual/kvm/devices/vcpu.txt 
> b/Documentation/virtual/kvm/devices/vcpu.txt
> index 3cc59c5..d626237 100644
> --- a/Documentation/virtual/kvm/devices/vcpu.txt
> +++ b/Documentation/virtual/kvm/devices/vcpu.txt
> @@ -6,3 +6,27 @@ KVM_GET_DEVICE_ATTR, and KVM_HAS_DEVICE_ATTR. The interface 
> uses the same struct
>  kvm_device_attr as other devices, but targets VCPU-wide settings and 
> controls.
>  
>  The groups and attributes per virtual cpu, if any, are architecture specific.
> +
> +1. GROUP: KVM_ARM_VCPU_PMU_V3_CTRL
> +Architectures: ARM64
> +
> +1.1. ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_IRQ
> +Parameters: in kvm_device_attr.addr the address for PMU overflow interrupt

so kvm_device_attr.addr is a pointer to an unsigned long or an int, or?

> +Returns: -EBUSY: The PMU overflow interrupt is already set
> + -ENXIO: The overflow interrupt not set when attempting to get it
> + -ENODEV: PMUv3 not supported
> + -EINVAL: Invalid PMU overflow interrupt number supplied
> +
> +A value describing the PMUv3 (Performance Monitor Unit v3) overflow interrupt
> +number for this vcpu. This interrupt could be a PPI or SPI, but the interrupt
> +type must be same for each vcpu. As a PPI, the interrupt number is same for 
> all

nit: s/is same/is the same/

> +vcpus, while as an SPI it must be different for each vcpu.

nit: s/it must be different for each vcpu/
   it must be a separate number per vcpu/

> +
> +1.2 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_INIT
> +Parameters: no additional parameter in kvm_device_attr.addr
> +Returns: -ENODEV: PMUv3 not supported
> + -ENXIO: PMUv3 not properly configured as required prior to calling 
> this
> + attribute
> + -EBUSY: PMUv3 already initialized
> +
> +Request the initialization of the PMUv3.
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index f9f2779..6dd0992 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -242,5 +242,20 @@ static inline void kvm_arm_init_debug(void) {}
>  static inline void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) {}
>  static inline void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) {}
>  static inline void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu) {}
> +static inline int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
> +  struct kvm_device_attr *attr)
> +{
> + return -ENXIO;
> +}
> +static inline int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
> +  struct kvm_device_attr *attr)
> +{
> + return -ENXIO;
> +}
> +static inline int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
> +  struct kvm_device_attr *attr)
> +{
> + return -ENXIO;
> +}
>  
>  #endif /* __ARM_KVM_HOST_H__ */
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 34d7395..dc8644f 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -833,6 +833,7 @@ static int kvm_arm_vcpu_set_attr(struct kvm_vcpu *vcpu,
>  
>   switch (attr->group) {
>   default:
> + ret = kvm_arm_vcpu_arch_set_attr(vcpu, attr);
>   break;
>   }
>  
> @@ -846,6 +847,7 @@ static int kvm_arm_vcpu_get_attr(struct kvm_vcpu *vcpu,
>  
>   switch (attr->group) {
>   default:
> + ret = kvm_arm_vcpu_arch_get_attr(vcpu, attr);
>   break;
>   }
>  
> @@ -859,6 +861,7 @@ static int kvm_arm_vcpu_has_attr(struct kvm_vcpu *vcpu,
>  
>   switch (attr->group) {
>   default:
> + ret = kvm_arm_vcpu_arch_has_attr(vcpu, attr);
>   break;
>   }
>  
> diff --git a/arch/arm64/include/asm/kvm_host.h 
> b/arch/arm64/include/asm/kvm_host.h
> index cb220b7..a855a30 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -359,5 +359,11 @@ void kvm_arm_init_de

Re: [PATCH v11 20/21] KVM: ARM: Introduce per-vcpu kvm device controls

[Christoffer Dall]

On Fri, Feb 05, 2016 at 03:14:15PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao 
> 
> In some cases it needs to get/set attributes specific to a vcpu and so
> needs something else than ONE_REG.
> 
> Let's copy the KVM_DEVICE approach, and define the respective ioctls
> for the vcpu file descriptor.
> 
> Signed-off-by: Shannon Zhao 
> Reviewed-by: Andrew Jones 
> Acked-by: Peter Maydell 
> ---
> CC: Peter Maydell 

strange cc section here?
> ---
>  Documentation/virtual/kvm/api.txt  | 10 +++---
>  Documentation/virtual/kvm/devices/vcpu.txt |  8 +
>  arch/arm/kvm/arm.c | 55 
> ++
>  arch/arm64/kvm/reset.c |  1 +
>  include/uapi/linux/kvm.h   |  1 +
>  5 files changed, 71 insertions(+), 4 deletions(-)
>  create mode 100644 Documentation/virtual/kvm/devices/vcpu.txt
> 
> diff --git a/Documentation/virtual/kvm/api.txt 
> b/Documentation/virtual/kvm/api.txt
> index 9684f8d..cb2ef0b 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2507,8 +2507,9 @@ struct kvm_create_device {
>  
>  4.80 KVM_SET_DEVICE_ATTR/KVM_GET_DEVICE_ATTR
>  
> -Capability: KVM_CAP_DEVICE_CTRL, KVM_CAP_VM_ATTRIBUTES for vm device
> -Type: device ioctl, vm ioctl
> +Capability: KVM_CAP_DEVICE_CTRL, KVM_CAP_VM_ATTRIBUTES for vm device,
> +  KVM_CAP_VCPU_ATTRIBUTES for vcpu device
> +Type: device ioctl, vm ioctl, vcpu ioctl
>  Parameters: struct kvm_device_attr
>  Returns: 0 on success, -1 on error
>  Errors:
> @@ -2533,8 +2534,9 @@ struct kvm_device_attr {
>  
>  4.81 KVM_HAS_DEVICE_ATTR
>  
> -Capability: KVM_CAP_DEVICE_CTRL, KVM_CAP_VM_ATTRIBUTES for vm device
> -Type: device ioctl, vm ioctl
> +Capability: KVM_CAP_DEVICE_CTRL, KVM_CAP_VM_ATTRIBUTES for vm device,
> +  KVM_CAP_VCPU_ATTRIBUTES for vcpu device
> +Type: device ioctl, vm ioctl, vcpu ioctl
>  Parameters: struct kvm_device_attr
>  Returns: 0 on success, -1 on error
>  Errors:
> diff --git a/Documentation/virtual/kvm/devices/vcpu.txt 
> b/Documentation/virtual/kvm/devices/vcpu.txt
> new file mode 100644
> index 000..3cc59c5
> --- /dev/null
> +++ b/Documentation/virtual/kvm/devices/vcpu.txt
> @@ -0,0 +1,8 @@
> +Generic vcpu interface
> +
> +
> +The virtual cpu "device" also accepts the ioctls KVM_SET_DEVICE_ATTR,
> +KVM_GET_DEVICE_ATTR, and KVM_HAS_DEVICE_ATTR. The interface uses the same 
> struct
> +kvm_device_attr as other devices, but targets VCPU-wide settings and 
> controls.
> +
> +The groups and attributes per virtual cpu, if any, are architecture specific.
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index d2c2cc3..34d7395 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -826,11 +826,51 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct 
> kvm_vcpu *vcpu,
>   return 0;
>  }
>  
> +static int kvm_arm_vcpu_set_attr(struct kvm_vcpu *vcpu,
> +  struct kvm_device_attr *attr)
> +{
> + int ret = -ENXIO;
> +
> + switch (attr->group) {
> + default:
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static int kvm_arm_vcpu_get_attr(struct kvm_vcpu *vcpu,
> +  struct kvm_device_attr *attr)
> +{
> + int ret = -ENXIO;
> +
> + switch (attr->group) {
> + default:
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static int kvm_arm_vcpu_has_attr(struct kvm_vcpu *vcpu,
> +  struct kvm_device_attr *attr)
> +{
> + int ret = -ENXIO;
> +
> + switch (attr->group) {
> + default:
> + break;
> + }
> +
> + return ret;
> +}
> +
>  long kvm_arch_vcpu_ioctl(struct file *filp,
>unsigned int ioctl, unsigned long arg)
>  {
>   struct kvm_vcpu *vcpu = filp->private_data;
>   void __user *argp = (void __user *)arg;
> + struct kvm_device_attr attr;
>  
>   switch (ioctl) {
>   case KVM_ARM_VCPU_INIT: {
> @@ -873,6 +913,21 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>   return -E2BIG;
>   return kvm_arm_copy_reg_indices(vcpu, user_list->reg);
>   }
> + case KVM_SET_DEVICE_ATTR: {
> + if (copy_from_user(&attr, argp, sizeof(attr)))
> + return -EFAULT;
> + return kvm_arm_vcpu_set_attr(vcpu, &attr);
> + }
> + case KVM_GET_DEVICE_ATTR: {
> + if (copy_from_user(&attr, argp, sizeof(attr)))
> + return -EFAULT;
> + return kvm_arm_vcpu_get_attr(vcpu, &attr);
> + }
> + case KVM_HAS_DEVICE_ATTR: {
> + if (copy_from_user(&attr, argp, sizeof(attr)))
> + return -EFAULT;
> + return kvm_arm_vcpu_has_attr(vcpu, &attr);
> + }

do we share the ioctl number space across device, VM, and VCPU file
descriptors?  If not, don't these need to be specifically defined as
VCPU ioctl numbers in kvm.h ?

Thanks,
-Ch

Re: [PATCH v11 19/21] KVM: ARM64: Add a new feature bit for PMUv3

[Christoffer Dall]

On Fri, Feb 05, 2016 at 03:14:14PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao 
> 
> To support guest PMUv3, use one bit of the VCPU INIT feature array.
> Initialize the PMU when initialzing the vcpu with that bit and PMU
> overflow interrupt set.
> 
> Signed-off-by: Shannon Zhao 
> Acked-by: Peter Maydell 
> Reviewed-by: Andrew Jones 
> ---
> CC: Peter Maydell 
> ---
>  Documentation/virtual/kvm/api.txt | 2 ++
>  arch/arm64/include/asm/kvm_host.h | 2 +-
>  arch/arm64/include/uapi/asm/kvm.h | 1 +
>  arch/arm64/kvm/reset.c| 3 +++
>  include/kvm/arm_pmu.h | 2 ++
>  include/uapi/linux/kvm.h  | 1 +
>  virt/kvm/arm/pmu.c| 9 +
>  7 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/virtual/kvm/api.txt 
> b/Documentation/virtual/kvm/api.txt
> index 07e4cdf..9684f8d 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2577,6 +2577,8 @@ Possible features:
> Depends on KVM_CAP_ARM_EL1_32BIT (arm64 only).
>   - KVM_ARM_VCPU_PSCI_0_2: Emulate PSCI v0.2 for the CPU.
> Depends on KVM_CAP_ARM_PSCI_0_2.
> + - KVM_ARM_VCPU_PMU_V3: Emulate PMUv3 for the CPU.
> +   Depends on KVM_CAP_ARM_PMU_V3.
>  
>  
>  4.83 KVM_ARM_PREFERRED_TARGET
> diff --git a/arch/arm64/include/asm/kvm_host.h 
> b/arch/arm64/include/asm/kvm_host.h
> index 6bab7fb..cb220b7 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -40,7 +40,7 @@
>  
>  #define KVM_MAX_VCPUS VGIC_V3_MAX_CPUS
>  
> -#define KVM_VCPU_MAX_FEATURES 3
> +#define KVM_VCPU_MAX_FEATURES 4
>  
>  int __attribute_const__ kvm_target_cpu(void);
>  int kvm_reset_vcpu(struct kvm_vcpu *vcpu);
> diff --git a/arch/arm64/include/uapi/asm/kvm.h 
> b/arch/arm64/include/uapi/asm/kvm.h
> index 2d4ca4b..6aedbe3 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -94,6 +94,7 @@ struct kvm_regs {
>  #define KVM_ARM_VCPU_POWER_OFF   0 /* CPU is started in OFF 
> state */
>  #define KVM_ARM_VCPU_EL1_32BIT   1 /* CPU running a 32bit VM */
>  #define KVM_ARM_VCPU_PSCI_0_22 /* CPU uses PSCI v0.2 */
> +#define KVM_ARM_VCPU_PMU_V3  3 /* Support guest PMUv3 */
>  
>  struct kvm_vcpu_init {
>   __u32 target;
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index dfbce78..cf4f28a 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -77,6 +77,9 @@ int kvm_arch_dev_ioctl_check_extension(long ext)
>   case KVM_CAP_GUEST_DEBUG_HW_WPS:
>   r = get_num_wrps();
>   break;
> + case KVM_CAP_ARM_PMU_V3:
> + r = kvm_arm_support_pmu_v3();
> + break;
>   case KVM_CAP_SET_GUEST_DEBUG:
>   r = 1;
>   break;
> diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
> index d73cb97..5f86e1d 100644
> --- a/include/kvm/arm_pmu.h
> +++ b/include/kvm/arm_pmu.h
> @@ -51,6 +51,7 @@ void kvm_pmu_software_increment(struct kvm_vcpu *vcpu, u64 
> val);
>  void kvm_pmu_handle_pmcr(struct kvm_vcpu *vcpu, u64 val);
>  void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, u64 data,
>   u64 select_idx);
> +bool kvm_arm_support_pmu_v3(void);
>  #else
>  struct kvm_pmu {
>  };
> @@ -77,6 +78,7 @@ static inline void kvm_pmu_software_increment(struct 
> kvm_vcpu *vcpu, u64 val) {}
>  static inline void kvm_pmu_handle_pmcr(struct kvm_vcpu *vcpu, u64 val) {}
>  static inline void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu,
> u64 data, u64 select_idx) {}
> +static inline bool kvm_arm_support_pmu_v3(void) { return false; }
>  #endif
>  
>  #endif
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 9da9051..dc16d30 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -850,6 +850,7 @@ struct kvm_ppc_smmu_info {
>  #define KVM_CAP_IOEVENTFD_ANY_LENGTH 122
>  #define KVM_CAP_HYPERV_SYNIC 123
>  #define KVM_CAP_S390_RI 124
> +#define KVM_CAP_ARM_PMU_V3 125
>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>  
> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
> index 6b525df..2467d62 100644
> --- a/virt/kvm/arm/pmu.c
> +++ b/virt/kvm/arm/pmu.c
> @@ -384,3 +384,12 @@ void kvm_pmu_set_counter_event_type(struct kvm_vcpu 
> *vcpu, u64 data,
>  
>   pmc->perf_event = event;
>  }
> +
> +bool kvm_arm_support_pmu_v3(void)
> +{
> + /* Check if HW_PERF_EVENTS are supported by checking the number of
> +  * hardware performance counters. This could ensure physical PMU and
> +  * PERF_EVENT driver existing.
> +  */

nit: coding style, opening comment block should be on its own line

I don't understand the last sentence.  Do you mean: "This ensures the
presence of a physical PMU and that CONFIG_PERF_EVENT is selected." ?

Thanks,
-Christoffer
___
kvmarm mailing

Re: [PATCH v11 16/21] KVM: ARM64: Add PMU overflow interrupt routing

[Christoffer Dall]

On Fri, Feb 05, 2016 at 03:14:11PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao 
> 
> When calling perf_event_create_kernel_counter to create perf_event,
> assign a overflow handler. Then when the perf event overflows, set the
> corresponding bit of guest PMOVSSET register. If this counter is enabled
> and its interrupt is enabled as well, kick the vcpu to sync the
> interrupt.
> 
> On VM entry, if there is counter overflowed, inject the interrupt with
> the level set to 1. Otherwise, inject the interrupt with the level set
> to 0.
> 
> Signed-off-by: Shannon Zhao 
> Reviewed-by: Marc Zyngier 
> Reviewed-by: Andrew Jones 
> ---
>  arch/arm/kvm/arm.c|  2 ++
>  include/kvm/arm_pmu.h |  2 ++
>  virt/kvm/arm/pmu.c| 50 +-
>  3 files changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index dda1959..f54264c 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -28,6 +28,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #define CREATE_TRACE_POINTS
>  #include "trace.h"
> @@ -577,6 +578,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct 
> kvm_run *run)
>* non-preemptible context.
>*/
>   preempt_disable();
> + kvm_pmu_flush_hwstate(vcpu);
>   kvm_timer_flush_hwstate(vcpu);
>   kvm_vgic_flush_hwstate(vcpu);
>  
> diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
> index 1f4bfa2..44a3c75 100644
> --- a/include/kvm/arm_pmu.h
> +++ b/include/kvm/arm_pmu.h
> @@ -44,6 +44,7 @@ u64 kvm_pmu_valid_counter_mask(struct kvm_vcpu *vcpu);
>  void kvm_pmu_disable_counter(struct kvm_vcpu *vcpu, u64 val);
>  void kvm_pmu_enable_counter(struct kvm_vcpu *vcpu, u64 val);
>  void kvm_pmu_overflow_set(struct kvm_vcpu *vcpu, u64 val);
> +void kvm_pmu_flush_hwstate(struct kvm_vcpu *vcpu);
>  void kvm_pmu_software_increment(struct kvm_vcpu *vcpu, u64 val);
>  void kvm_pmu_handle_pmcr(struct kvm_vcpu *vcpu, u64 val);
>  void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, u64 data,
> @@ -67,6 +68,7 @@ static inline u64 kvm_pmu_valid_counter_mask(struct 
> kvm_vcpu *vcpu)
>  static inline void kvm_pmu_disable_counter(struct kvm_vcpu *vcpu, u64 val) {}
>  static inline void kvm_pmu_enable_counter(struct kvm_vcpu *vcpu, u64 val) {}
>  static inline void kvm_pmu_overflow_set(struct kvm_vcpu *vcpu, u64 val) {}
> +static inline void kvm_pmu_flush_hwstate(struct kvm_vcpu *vcpu) {}
>  static inline void kvm_pmu_software_increment(struct kvm_vcpu *vcpu, u64 
> val) {}
>  static inline void kvm_pmu_handle_pmcr(struct kvm_vcpu *vcpu, u64 val) {}
>  static inline void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu,
> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
> index c8ea825..5f983cb 100644
> --- a/virt/kvm/arm/pmu.c
> +++ b/virt/kvm/arm/pmu.c
> @@ -21,6 +21,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  /**
>   * kvm_pmu_get_counter_value - get PMU counter value
> @@ -180,6 +181,52 @@ void kvm_pmu_overflow_set(struct kvm_vcpu *vcpu, u64 val)
>  }
>  
>  /**
> + * kvm_pmu_flush_hwstate - flush pmu state to cpu
> + * @vcpu: The vcpu pointer
> + *
> + * Inject virtual PMU IRQ if IRQ is pending for this cpu.
> + */
> +void kvm_pmu_flush_hwstate(struct kvm_vcpu *vcpu)
> +{
> + struct kvm_pmu *pmu = &vcpu->arch.pmu;
> + u64 overflow;
> +
> + if (!kvm_arm_pmu_v3_ready(vcpu))
> + return;
> +
> + if (!(vcpu_sys_reg(vcpu, PMCR_EL0) & ARMV8_PMCR_E))
> + return;

are we modeling the PMU interrupt as level-triggered?

In that case, shouldn't we lower the interrupt line on flush when
PMCR_EL0.E == 0 ?


Thanks,
-Christoffer
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v11 06/21] KVM: ARM64: Add access handler for PMCEID0 and PMCEID1 register

[Christoffer Dall]

On Fri, Feb 05, 2016 at 03:14:01PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao 
> 
> Add access handler which gets host value of PMCEID0 or PMCEID1 when
> guest access these registers. Writing action to PMCEID0 or PMCEID1 is
> UNDEFINED.
> 
> Signed-off-by: Shannon Zhao 
> ---
>  arch/arm64/kvm/sys_regs.c | 29 +
>  1 file changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index fc60041..06257e2 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -492,6 +492,27 @@ static bool access_pmselr(struct kvm_vcpu *vcpu, struct 
> sys_reg_params *p,
>   return true;
>  }
>  
> +static bool access_pmceid(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> +   const struct sys_reg_desc *r)
> +{
> + u64 pmceid;
> +
> + if (!kvm_arm_pmu_v3_ready(vcpu))
> + return trap_raz_wi(vcpu, p, r);
> +
> + if (p->is_write)
> + return false;

Isn't it really a BUG_ON(p->is_write) ?

Presumably a guest write to these registers will raise an undefined
exception in EL0/1 and we don't get here by any other path than the trap
handler, do we?

Otherwise looks good to me.

Thanks,
-Christoffer
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 0/8] KVM/ARM: Guest Entry/Exit optimizations

[Marc Zyngier]

I've recently been looking at our entry/exit costs, and profiling
figures did show some very low hanging fruits.

The most obvious cost is that accessing the GIC HW is slow. As in
"deadly slow", specially when GICv2 is involved. So not hammering the
HW when there is nothing to write is immediately beneficial, as this
is the most common cases (whatever people seem to think, interrupts
are a *rare* event).

Another easy thing to fix is the way we handle trapped system
registers. We do insist on (mostly) sorting them, but we do perform a
linear search on trap. We can switch to a binary search for free, and
get immediate benefits (the PMU code, being extremely trap-happy,
benefits immediately from this).

With these in place, I see an improvement of 20 to 30% (depending on
the platform) on our world-switch cycle count when running a set of
hand-crafted guests that are designed to only perform traps.

Methodology:

* NULL-hypercall guest: Perform 65536 PSCI_0_2_FN_PSCI_VERSION calls,
and then a power-off:

__start:
mov x19, #(1 << 16)
1:  mov x0, #0x8400
hvc #0
sub x19, x19, #1
cbnzx19, 1b
mov x0, #0x8400
add x0, x0, #9
hvc #0
b   .

* sysreg trap guest: Perform 2^20 PMSELR_EL0 accesses, and power-off:

__start:
mov x19, #(1 << 20)
1:  mrs x0, PMSELR_EL0
sub x19, x19, #1
cbnzx19, 1b
mov x0, #0x8400
add x0, x0, #9
hvc #0
b   .

* These guests are profiled using perf and kvmtool:

taskset -c 1 perf stat -e cycles:kh lkvm run -c1 --kernel do_sysreg.bin 2>&1 
>/dev/null| grep cycles

The result is then divided by the number of iterations (2^16 or 2^20).

These tests have been run on Seattle, Mustang, and LS2085, and shown
significant improvements in all cases. I've only touched the arm64
GIC code, but obviously the 32bit code should use it as well once
we've migrated it to C.

I've pushed out a branch (kvm-arm64/suck-less) to the usual location.

Thanks,

M.

Marc Zyngier (8):
  arm64: KVM: Switch the sys_reg search to be a binary search
  ARM: KVM: Properly sort the invariant table
  ARM: KVM: Enforce sorting of all CP tables
  ARM: KVM: Rename struct coproc_reg::is_64 to is_64bit
  ARM: KVM: Switch the CP reg search to be a binary search
  KVM: arm/arm64: timer: Add active state caching
  KVM: arm/arm64: Avoid accessing GICH registers
  KVM: arm64: Avoid accessing ICH registers

 arch/arm/kvm/arm.c  |   1 +
 arch/arm/kvm/coproc.c   |  74 ++-
 arch/arm/kvm/coproc.h   |   8 +-
 arch/arm64/kvm/hyp/vgic-v2-sr.c |  71 +++---
 arch/arm64/kvm/hyp/vgic-v3-sr.c | 288 
 arch/arm64/kvm/sys_regs.c   |  40 +++---
 include/kvm/arm_arch_timer.h|   5 +
 include/kvm/arm_vgic.h  |   8 +-
 virt/kvm/arm/arch_timer.c   |  31 +
 virt/kvm/arm/vgic-v3.c  |   4 +-
 10 files changed, 334 insertions(+), 196 deletions(-)

-- 
2.1.4

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

[PATCH 4/8] ARM: KVM: Rename struct coproc_reg::is_64 to is_64bit

[Marc Zyngier]

As we're going to play some tricks on the struct coproc_reg,
make sure its 64bit indicator field matches that of coproc_params.

Signed-off-by: Marc Zyngier 
---
 arch/arm/kvm/coproc.c | 4 ++--
 arch/arm/kvm/coproc.h | 8 
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
index 03f5d14..2a67f00 100644
--- a/arch/arm/kvm/coproc.c
+++ b/arch/arm/kvm/coproc.c
@@ -423,7 +423,7 @@ static const struct coproc_reg *find_reg(const struct 
coproc_params *params,
for (i = 0; i < num; i++) {
const struct coproc_reg *r = &table[i];
 
-   if (params->is_64bit != r->is_64)
+   if (params->is_64bit != r->is_64bit)
continue;
if (params->CRn != r->CRn)
continue;
@@ -1105,7 +1105,7 @@ static int write_demux_regids(u64 __user *uindices)
 static u64 cp15_to_index(const struct coproc_reg *reg)
 {
u64 val = KVM_REG_ARM | (15 << KVM_REG_ARM_COPROC_SHIFT);
-   if (reg->is_64) {
+   if (reg->is_64bit) {
val |= KVM_REG_SIZE_U64;
val |= (reg->Op1 << KVM_REG_ARM_OPC1_SHIFT);
/*
diff --git a/arch/arm/kvm/coproc.h b/arch/arm/kvm/coproc.h
index 88d24a3..5acd097 100644
--- a/arch/arm/kvm/coproc.h
+++ b/arch/arm/kvm/coproc.h
@@ -37,7 +37,7 @@ struct coproc_reg {
unsigned long Op1;
unsigned long Op2;
 
-   bool is_64;
+   bool is_64bit;
 
/* Trapped access from guest, if non-NULL. */
bool (*access)(struct kvm_vcpu *,
@@ -141,7 +141,7 @@ static inline int cmp_reg(const struct coproc_reg *i1,
return i1->Op1 - i2->Op1;
if (i1->Op2 != i2->Op2)
return i1->Op2 - i2->Op2;
-   return i2->is_64 - i1->is_64;
+   return i2->is_64bit - i1->is_64bit;
 }
 
 
@@ -150,8 +150,8 @@ static inline int cmp_reg(const struct coproc_reg *i1,
 #define CRm64(_x)   .CRn = _x, .CRm = 0
 #define Op1(_x).Op1 = _x
 #define Op2(_x).Op2 = _x
-#define is64   .is_64 = true
-#define is32   .is_64 = false
+#define is64   .is_64bit = true
+#define is32   .is_64bit = false
 
 bool access_vm_reg(struct kvm_vcpu *vcpu,
   const struct coproc_params *p,
-- 
2.1.4

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

[PATCH 8/8] KVM: arm64: Avoid accessing ICH registers

[Marc Zyngier]

Just like on GICv2, we're a bit hammer-happy with GICv3, and access
them more often than we should.

Adopt a policy similar to what we do for GICv2, only save/restoring
the minimal set of registers. As we don't access the registers
linearly anymore (we may skip some), the convoluted accessors become
slightly simpler, and we can drop the ugly indexing macro that
tended to confuse the reviewers.

Signed-off-by: Marc Zyngier 
---
 arch/arm64/kvm/hyp/vgic-v3-sr.c | 288 
 include/kvm/arm_vgic.h  |   6 -
 virt/kvm/arm/vgic-v3.c  |   4 +-
 3 files changed, 176 insertions(+), 122 deletions(-)

diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c
index 9142e082..d3813f5 100644
--- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
+++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c
@@ -39,12 +39,104 @@
asm volatile("msr_s " __stringify(r) ", %0" : : "r" (__val));\
} while (0)
 
-/* vcpu is already in the HYP VA space */
+static u64 __hyp_text __gic_v3_get_lr(unsigned int lr)
+{
+   switch (lr & 0xf) {
+   case 0:
+   return read_gicreg(ICH_LR0_EL2);
+   case 1:
+   return read_gicreg(ICH_LR1_EL2);
+   case 2:
+   return read_gicreg(ICH_LR2_EL2);
+   case 3:
+   return read_gicreg(ICH_LR3_EL2);
+   case 4:
+   return read_gicreg(ICH_LR4_EL2);
+   case 5:
+   return read_gicreg(ICH_LR5_EL2);
+   case 6:
+   return read_gicreg(ICH_LR6_EL2);
+   case 7:
+   return read_gicreg(ICH_LR7_EL2);
+   case 8:
+   return read_gicreg(ICH_LR8_EL2);
+   case 9:
+   return read_gicreg(ICH_LR9_EL2);
+   case 10:
+   return read_gicreg(ICH_LR10_EL2);
+   case 11:
+   return read_gicreg(ICH_LR11_EL2);
+   case 12:
+   return read_gicreg(ICH_LR12_EL2);
+   case 13:
+   return read_gicreg(ICH_LR13_EL2);
+   case 14:
+   return read_gicreg(ICH_LR14_EL2);
+   case 15:
+   return read_gicreg(ICH_LR15_EL2);
+   }
+
+   unreachable();
+}
+
+static void __hyp_text __gic_v3_set_lr(u64 val, int lr)
+{
+   switch (lr & 0xf) {
+   case 0:
+   write_gicreg(val, ICH_LR0_EL2);
+   break;
+   case 1:
+   write_gicreg(val, ICH_LR1_EL2);
+   break;
+   case 2:
+   write_gicreg(val, ICH_LR2_EL2);
+   break;
+   case 3:
+   write_gicreg(val, ICH_LR3_EL2);
+   break;
+   case 4:
+   write_gicreg(val, ICH_LR4_EL2);
+   break;
+   case 5:
+   write_gicreg(val, ICH_LR5_EL2);
+   break;
+   case 6:
+   write_gicreg(val, ICH_LR6_EL2);
+   break;
+   case 7:
+   write_gicreg(val, ICH_LR7_EL2);
+   break;
+   case 8:
+   write_gicreg(val, ICH_LR8_EL2);
+   break;
+   case 9:
+   write_gicreg(val, ICH_LR9_EL2);
+   break;
+   case 10:
+   write_gicreg(val, ICH_LR10_EL2);
+   break;
+   case 11:
+   write_gicreg(val, ICH_LR11_EL2);
+   break;
+   case 12:
+   write_gicreg(val, ICH_LR12_EL2);
+   break;
+   case 13:
+   write_gicreg(val, ICH_LR13_EL2);
+   break;
+   case 14:
+   write_gicreg(val, ICH_LR14_EL2);
+   break;
+   case 15:
+   write_gicreg(val, ICH_LR15_EL2);
+   break;
+   }
+}
+
 void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
 {
struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3;
u64 val;
-   u32 max_lr_idx, nr_pri_bits;
 
/*
 * Make sure stores to the GIC via the memory mapped interface
@@ -53,68 +145,50 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
dsb(st);
 
cpu_if->vgic_vmcr  = read_gicreg(ICH_VMCR_EL2);
-   cpu_if->vgic_misr  = read_gicreg(ICH_MISR_EL2);
-   cpu_if->vgic_eisr  = read_gicreg(ICH_EISR_EL2);
-   cpu_if->vgic_elrsr = read_gicreg(ICH_ELSR_EL2);
 
-   write_gicreg(0, ICH_HCR_EL2);
-   val = read_gicreg(ICH_VTR_EL2);
-   max_lr_idx = vtr_to_max_lr_idx(val);
-   nr_pri_bits = vtr_to_nr_pri_bits(val);
+   if (vcpu->arch.vgic_cpu.live_lrs) {
+   int i;
+   u32 max_lr_idx, nr_pri_bits;
 
-   switch (max_lr_idx) {
-   case 15:
-   cpu_if->vgic_lr[VGIC_V3_LR_INDEX(15)] = 
read_gicreg(ICH_LR15_EL2);
-   case 14:
-   cpu_if->vgic_lr[VGIC_V3_LR_INDEX(14)] = 
read_gicreg(ICH_LR14_EL2);
-   case 13:
-   cpu_if->vgic_lr[VGIC_V3_LR_INDEX(13)] = 
read_gicreg(ICH_LR13_EL2);
-   case 12:
-   cpu_if->vgic_lr[VGIC_V3_LR_INDEX(12)] = 
read_gicreg(ICH_LR12_EL2

[PATCH 2/8] ARM: KVM: Properly sort the invariant table

[Marc Zyngier]

Not having the invariant table properly sorted is an oddity, and
may get in the way of future optimisations. Let's fix it.

Signed-off-by: Marc Zyngier 
---
 arch/arm/kvm/coproc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
index f3d88dc..16c74f8 100644
--- a/arch/arm/kvm/coproc.c
+++ b/arch/arm/kvm/coproc.c
@@ -645,6 +645,9 @@ static struct coproc_reg invariant_cp15[] = {
{ CRn( 0), CRm( 0), Op1( 0), Op2( 3), is32, NULL, get_TLBTR },
{ CRn( 0), CRm( 0), Op1( 0), Op2( 6), is32, NULL, get_REVIDR },
 
+   { CRn( 0), CRm( 0), Op1( 1), Op2( 1), is32, NULL, get_CLIDR },
+   { CRn( 0), CRm( 0), Op1( 1), Op2( 7), is32, NULL, get_AIDR },
+
{ CRn( 0), CRm( 1), Op1( 0), Op2( 0), is32, NULL, get_ID_PFR0 },
{ CRn( 0), CRm( 1), Op1( 0), Op2( 1), is32, NULL, get_ID_PFR1 },
{ CRn( 0), CRm( 1), Op1( 0), Op2( 2), is32, NULL, get_ID_DFR0 },
@@ -660,9 +663,6 @@ static struct coproc_reg invariant_cp15[] = {
{ CRn( 0), CRm( 2), Op1( 0), Op2( 3), is32, NULL, get_ID_ISAR3 },
{ CRn( 0), CRm( 2), Op1( 0), Op2( 4), is32, NULL, get_ID_ISAR4 },
{ CRn( 0), CRm( 2), Op1( 0), Op2( 5), is32, NULL, get_ID_ISAR5 },
-
-   { CRn( 0), CRm( 0), Op1( 1), Op2( 1), is32, NULL, get_CLIDR },
-   { CRn( 0), CRm( 0), Op1( 1), Op2( 7), is32, NULL, get_AIDR },
 };
 
 /*
-- 
2.1.4

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

[PATCH 7/8] KVM: arm/arm64: Avoid accessing GICH registers

[Marc Zyngier]

GICv2 registers are *slow*. As in "terrifyingly slow". Which is bad.
But we're equaly bad, as we make a point in accessing them even if
we don't have any interrupt in flight.

A good solution is to first find out if we have anything useful to
write into the GIC, and if we don't, to simply not do it. This
involves tracking which LRs actually have something valid there.

Signed-off-by: Marc Zyngier 
---
 arch/arm64/kvm/hyp/vgic-v2-sr.c | 71 -
 include/kvm/arm_vgic.h  |  2 ++
 2 files changed, 51 insertions(+), 22 deletions(-)

diff --git a/arch/arm64/kvm/hyp/vgic-v2-sr.c b/arch/arm64/kvm/hyp/vgic-v2-sr.c
index e717612..874a08d 100644
--- a/arch/arm64/kvm/hyp/vgic-v2-sr.c
+++ b/arch/arm64/kvm/hyp/vgic-v2-sr.c
@@ -38,28 +38,40 @@ void __hyp_text __vgic_v2_save_state(struct kvm_vcpu *vcpu)
 
nr_lr = vcpu->arch.vgic_cpu.nr_lr;
cpu_if->vgic_vmcr = readl_relaxed(base + GICH_VMCR);
-   cpu_if->vgic_misr = readl_relaxed(base + GICH_MISR);
-   eisr0  = readl_relaxed(base + GICH_EISR0);
-   elrsr0 = readl_relaxed(base + GICH_ELRSR0);
-   if (unlikely(nr_lr > 32)) {
-   eisr1  = readl_relaxed(base + GICH_EISR1);
-   elrsr1 = readl_relaxed(base + GICH_ELRSR1);
-   } else {
-   eisr1 = elrsr1 = 0;
-   }
+
+   if (vcpu->arch.vgic_cpu.live_lrs) {
+   eisr0  = readl_relaxed(base + GICH_EISR0);
+   elrsr0 = readl_relaxed(base + GICH_ELRSR0);
+   cpu_if->vgic_misr = readl_relaxed(base + GICH_MISR);
+   cpu_if->vgic_apr= readl_relaxed(base + GICH_APR);
+
+   if (unlikely(nr_lr > 32)) {
+   eisr1  = readl_relaxed(base + GICH_EISR1);
+   elrsr1 = readl_relaxed(base + GICH_ELRSR1);
+   } else {
+   eisr1 = elrsr1 = 0;
+   }
+
 #ifdef CONFIG_CPU_BIG_ENDIAN
-   cpu_if->vgic_eisr  = ((u64)eisr0 << 32) | eisr1;
-   cpu_if->vgic_elrsr = ((u64)elrsr0 << 32) | elrsr1;
+   cpu_if->vgic_eisr  = ((u64)eisr0 << 32) | eisr1;
+   cpu_if->vgic_elrsr = ((u64)elrsr0 << 32) | elrsr1;
 #else
-   cpu_if->vgic_eisr  = ((u64)eisr1 << 32) | eisr0;
-   cpu_if->vgic_elrsr = ((u64)elrsr1 << 32) | elrsr0;
+   cpu_if->vgic_eisr  = ((u64)eisr1 << 32) | eisr0;
+   cpu_if->vgic_elrsr = ((u64)elrsr1 << 32) | elrsr0;
 #endif
-   cpu_if->vgic_apr= readl_relaxed(base + GICH_APR);
 
-   writel_relaxed(0, base + GICH_HCR);
+   for (i = 0; i < nr_lr; i++)
+   if (vcpu->arch.vgic_cpu.live_lrs & (1UL << i))
+   cpu_if->vgic_lr[i] = readl_relaxed(base + 
GICH_LR0 + (i * 4));
 
-   for (i = 0; i < nr_lr; i++)
-   cpu_if->vgic_lr[i] = readl_relaxed(base + GICH_LR0 + (i * 4));
+   writel_relaxed(0, base + GICH_HCR);
+
+   vcpu->arch.vgic_cpu.live_lrs = 0;
+   } else {
+   cpu_if->vgic_eisr = 0;
+   cpu_if->vgic_elrsr = ~0UL;
+   cpu_if->vgic_misr = 0;
+   }
 }
 
 /* vcpu is already in the HYP VA space */
@@ -70,15 +82,30 @@ void __hyp_text __vgic_v2_restore_state(struct kvm_vcpu 
*vcpu)
struct vgic_dist *vgic = &kvm->arch.vgic;
void __iomem *base = kern_hyp_va(vgic->vctrl_base);
int i, nr_lr;
+   u64 live_lrs = 0;
 
if (!base)
return;
 
-   writel_relaxed(cpu_if->vgic_hcr, base + GICH_HCR);
-   writel_relaxed(cpu_if->vgic_vmcr, base + GICH_VMCR);
-   writel_relaxed(cpu_if->vgic_apr, base + GICH_APR);
-
nr_lr = vcpu->arch.vgic_cpu.nr_lr;
+
for (i = 0; i < nr_lr; i++)
-   writel_relaxed(cpu_if->vgic_lr[i], base + GICH_LR0 + (i * 4));
+   if (cpu_if->vgic_lr[i] & GICH_LR_STATE)
+   live_lrs |= 1UL << i;
+
+   if (live_lrs) {
+   writel_relaxed(cpu_if->vgic_hcr, base + GICH_HCR);
+   writel_relaxed(cpu_if->vgic_apr, base + GICH_APR);
+   for (i = 0; i < nr_lr; i++) {
+   u32 val = 0;
+
+   if (live_lrs & (1UL << i))
+   val = cpu_if->vgic_lr[i];
+
+   writel_relaxed(val, base + GICH_LR0 + (i * 4));
+   }
+   }
+
+   writel_relaxed(cpu_if->vgic_vmcr, base + GICH_VMCR);
+   vcpu->arch.vgic_cpu.live_lrs = live_lrs;
 }
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 13a3d53..f473fd6 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -321,6 +321,8 @@ struct vgic_cpu {
 
/* Protected by the distributor's irq_phys_map_lock */
struct list_headirq_phys_map_list;
+
+   u64 live_lrs;
 };
 
 #define LR_EMPTY   0xff
-- 
2.1.4

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

[PATCH 1/8] arm64: KVM: Switch the sys_reg search to be a binary search

[Marc Zyngier]

Our 64bit sys_reg table is about 90 entries long (so far, and the
PMU support is likely to increase this). This means that on average,
it takes 45 comparaisons to find the right entry (and actually the
full 90 if we have to search the invariant table).

Not the most efficient thing. Specially when you think that this
table is already sorted. Switching to a binary search effectively
reduces the search to about 7 comparaisons. Slightly better!

As an added bonus, the comparaison is done by comparing all the
fields at once, instead of one at a time.

Signed-off-by: Marc Zyngier 
---
 arch/arm64/kvm/sys_regs.c | 40 ++--
 1 file changed, 22 insertions(+), 18 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index eec3598..0035869 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -20,6 +20,7 @@
  * along with this program.  If not, see .
  */
 
+#include 
 #include 
 #include 
 #include 
@@ -942,29 +943,32 @@ static const struct sys_reg_desc 
*get_target_table(unsigned target,
}
 }
 
+#define reg_to_match_value(x)  \
+   ({  \
+   unsigned long val;  \
+   val  = (x)->Op0 << 14;  \
+   val |= (x)->Op1 << 11;  \
+   val |= (x)->CRn << 7;   \
+   val |= (x)->CRm << 3;   \
+   val |= (x)->Op2;\
+   val;\
+})
+
+static int match_sys_reg(const void *key, const void *elt)
+{
+   const unsigned long pval = (unsigned long)key;
+   const struct sys_reg_desc *r = elt;
+
+   return pval - reg_to_match_value(r);
+}
+
 static const struct sys_reg_desc *find_reg(const struct sys_reg_params *params,
 const struct sys_reg_desc table[],
 unsigned int num)
 {
-   unsigned int i;
-
-   for (i = 0; i < num; i++) {
-   const struct sys_reg_desc *r = &table[i];
+   unsigned long pval = reg_to_match_value(params);
 
-   if (params->Op0 != r->Op0)
-   continue;
-   if (params->Op1 != r->Op1)
-   continue;
-   if (params->CRn != r->CRn)
-   continue;
-   if (params->CRm != r->CRm)
-   continue;
-   if (params->Op2 != r->Op2)
-   continue;
-
-   return r;
-   }
-   return NULL;
+   return bsearch((void *)pval, table, num, sizeof(table[0]), 
match_sys_reg);
 }
 
 int kvm_handle_cp14_load_store(struct kvm_vcpu *vcpu, struct kvm_run *run)
-- 
2.1.4

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

[PATCH 5/8] ARM: KVM: Switch the CP reg search to be a binary search

[Marc Zyngier]

Doing a linear search is a bit silly when we can do a binary search.
Not that we trap that so many things that it has become a burden yet,
but it makes sense to align it with the arm64 code.

Signed-off-by: Marc Zyngier 
---
 arch/arm/kvm/coproc.c | 41 +++--
 1 file changed, 23 insertions(+), 18 deletions(-)

diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
index 2a67f00..4f1c869 100644
--- a/arch/arm/kvm/coproc.c
+++ b/arch/arm/kvm/coproc.c
@@ -16,6 +16,8 @@
  * along with this program; if not, write to the Free Software
  * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
  */
+
+#include 
 #include 
 #include 
 #include 
@@ -414,29 +416,32 @@ static const struct coproc_reg *get_target_table(unsigned 
target, size_t *num)
return table->table;
 }
 
+#define reg_to_match_value(x)  \
+   ({  \
+   unsigned long val;  \
+   val  = (x)->CRn << 11;  \
+   val |= (x)->CRm << 7;   \
+   val |= (x)->Op1 << 4;   \
+   val |= (x)->Op2 << 1;   \
+   val |= !(x)->is_64bit;  \
+   val;\
+})
+
+static int match_reg(const void *key, const void *elt)
+{
+   const unsigned long pval = (unsigned long)key;
+   const struct coproc_reg *r = elt;
+
+   return pval - reg_to_match_value(r);
+}
+
 static const struct coproc_reg *find_reg(const struct coproc_params *params,
 const struct coproc_reg table[],
 unsigned int num)
 {
-   unsigned int i;
-
-   for (i = 0; i < num; i++) {
-   const struct coproc_reg *r = &table[i];
+   unsigned long pval = reg_to_match_value(params);
 
-   if (params->is_64bit != r->is_64bit)
-   continue;
-   if (params->CRn != r->CRn)
-   continue;
-   if (params->CRm != r->CRm)
-   continue;
-   if (params->Op1 != r->Op1)
-   continue;
-   if (params->Op2 != r->Op2)
-   continue;
-
-   return r;
-   }
-   return NULL;
+   return bsearch((void *)pval, table, num, sizeof(table[0]), match_reg);
 }
 
 static int emulate_cp15(struct kvm_vcpu *vcpu,
-- 
2.1.4

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

[PATCH 3/8] ARM: KVM: Enforce sorting of all CP tables

[Marc Zyngier]

Since we're obviously terrible at sorting the CP tables, make sure
we're going to do it properly (or fail to boot). arm64 has had the
same mechanism for a while, and nobody ever broke it...

Signed-off-by: Marc Zyngier 
---
 arch/arm/kvm/coproc.c | 25 +
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
index 16c74f8..03f5d14 100644
--- a/arch/arm/kvm/coproc.c
+++ b/arch/arm/kvm/coproc.c
@@ -381,17 +381,26 @@ static const struct coproc_reg cp15_regs[] = {
{ CRn(15), CRm( 0), Op1( 4), Op2( 0), is32, access_cbar},
 };
 
+static int check_reg_table(const struct coproc_reg *table, unsigned int n)
+{
+   unsigned int i;
+
+   for (i = 1; i < n; i++) {
+   if (cmp_reg(&table[i-1], &table[i]) >= 0) {
+   kvm_err("reg table %p out of order (%d)\n", table, i - 
1);
+   return 1;
+   }
+   }
+
+   return 0;
+}
+
 /* Target specific emulation tables */
 static struct kvm_coproc_target_table *target_tables[KVM_ARM_NUM_TARGETS];
 
 void kvm_register_target_coproc_table(struct kvm_coproc_target_table *table)
 {
-   unsigned int i;
-
-   for (i = 1; i < table->num; i++)
-   BUG_ON(cmp_reg(&table->table[i-1],
-  &table->table[i]) >= 0);
-
+   BUG_ON(check_reg_table(table->table, table->num));
target_tables[table->target] = table;
 }
 
@@ -1210,8 +1219,8 @@ void kvm_coproc_table_init(void)
unsigned int i;
 
/* Make sure tables are unique and in order. */
-   for (i = 1; i < ARRAY_SIZE(cp15_regs); i++)
-   BUG_ON(cmp_reg(&cp15_regs[i-1], &cp15_regs[i]) >= 0);
+   BUG_ON(check_reg_table(cp15_regs, ARRAY_SIZE(cp15_regs)));
+   BUG_ON(check_reg_table(invariant_cp15, ARRAY_SIZE(invariant_cp15)));
 
/* We abuse the reset function to overwrite the table itself. */
for (i = 0; i < ARRAY_SIZE(invariant_cp15); i++)
-- 
2.1.4

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

[PATCH 6/8] KVM: arm/arm64: timer: Add active state caching

[Marc Zyngier]

Programming the active state in the (re)distributor can be an
expensive operation so it makes some sense to try and reduce
the number of accesses as much as possible. So far, we
program the active state on each VM entry, but there is some
opportunity to do less.

An obvious solution is to cache the active state in memory,
and only program it in the HW when conditions change. But
because the HW can also change things under our feet (the active
state can transition from 1 to 0 when the guest does an EOI),
some precautions have to be taken, which amount to only caching
an "inactive" state, and always programing it otherwise.

With this in place, we observe a reduction of around 700 cycles
on a 2GHz GICv2 platform for a NULL hypercall.

Signed-off-by: Marc Zyngier 
---
 arch/arm/kvm/arm.c   |  1 +
 include/kvm/arm_arch_timer.h |  5 +
 virt/kvm/arm/arch_timer.c| 31 +++
 3 files changed, 37 insertions(+)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index dda1959..af7c1a3 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -320,6 +320,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
vcpu->cpu = -1;
 
kvm_arm_set_running_vcpu(NULL);
+   kvm_timer_vcpu_put(vcpu);
 }
 
 int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
index 1800227..b651aed 100644
--- a/include/kvm/arm_arch_timer.h
+++ b/include/kvm/arm_arch_timer.h
@@ -55,6 +55,9 @@ struct arch_timer_cpu {
 
/* VGIC mapping */
struct irq_phys_map *map;
+
+   /* Active IRQ state caching */
+   boolactive_cleared_last;
 };
 
 int kvm_timer_hyp_init(void);
@@ -74,4 +77,6 @@ bool kvm_timer_should_fire(struct kvm_vcpu *vcpu);
 void kvm_timer_schedule(struct kvm_vcpu *vcpu);
 void kvm_timer_unschedule(struct kvm_vcpu *vcpu);
 
+void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu);
+
 #endif
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 69bca18..bfec447 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -34,6 +34,11 @@ static struct timecounter *timecounter;
 static struct workqueue_struct *wqueue;
 static unsigned int host_vtimer_irq;
 
+void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu)
+{
+   vcpu->arch.timer_cpu.active_cleared_last = false;
+}
+
 static cycle_t kvm_phys_timer_read(void)
 {
return timecounter->cc->read(timecounter->cc);
@@ -130,6 +135,7 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, 
bool new_level)
 
BUG_ON(!vgic_initialized(vcpu->kvm));
 
+   timer->active_cleared_last = false;
timer->irq.level = new_level;
trace_kvm_timer_update_irq(vcpu->vcpu_id, timer->map->virt_irq,
   timer->irq.level);
@@ -242,10 +248,35 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
else
phys_active = false;
 
+   /*
+* We want to avoid hitting the (re)distributor as much as
+* possible, as this is a potentially expensive MMIO access
+* (not to mention locks in the irq layer), and a solution for
+* this is to cache the "active" state in memory.
+*
+* Things to consider: we cannot cache an "active set" state,
+* because the HW can change this behind our back (it becomes
+* "clear" in the HW). We must then restrict the caching to
+* the "clear" state.
+*
+* The cache is invalidated on:
+* - vcpu put, indicating that the HW cannot be trusted to be
+*   in a sane state on the next vcpu load,
+* - any change in the interrupt state
+*
+* Usage conditions:
+* - cached value is "active clear"
+* - value to be programmed is "active clear"
+*/
+   if (timer->active_cleared_last && !phys_active)
+   return;
+
ret = irq_set_irqchip_state(timer->map->irq,
IRQCHIP_STATE_ACTIVE,
phys_active);
WARN_ON(ret);
+
+   timer->active_cleared_last = !phys_active;
 }
 
 /**
-- 
2.1.4

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

Re: PCIe passthrough support on ARM

[Edward Cragg]

Hi Eric,

On Thu, Feb 04, 2016 at 06:30:34PM +0100, Eric Auger wrote:
> Hi Edward,
> On 02/04/2016 05:53 PM, Edward Cragg wrote:
> > Hi,
> > 
> > I'm involved in planning a project for which there is a requirement for PCIe
> > passthrough in KVM on ARMv8. We have no hardware to test on at the moment.
> > 
> > I understand that virtualisation support for ARM is quite young, and it 
> > seems
> > like support is trickling in at the moment for this sort of thing.
> > 
> > Is anyone working on PCIe passthrough on ARM platforms who might be able to
> > share some insight into what would be required to support this?
> Yes I am currently working on this topic at the moment and especially on
> ARM PCIe passthrough with MSI enabled. I did not test PCIe passthrough
> with legacy INTx (MSI disabled) but this should work already with
> upstreamed QEMU and kernel.
> 
> With MSI enabled, the code is not upstreamed yet.
> 
> I have a QEMU series:
> 
> [RFC v2 0/8] KVM PCI/MSI passthrough with mach-virt
> (https://www.mail-archive.com/qemu-devel@nongnu.org/msg349574.html)
> 
> and a kernel series:
> [PATCH 00/10] KVM PCIe/MSI passthrough on ARM/ARM64"
>   (https://lkml.org/lkml/2016/1/26/371)
> 
> This is functional, tested on v8 AMD HW with a GICv2M (single MSI frame).
> 
> This is in an early stage, especially on kernel side since lots of
> issues still need to be addressed, especially handling proper IRQ isolation.

Thank you, this is good to know.

> What kind of MSI controller do you plan to use?

Do you mean the GIC itself? From registers, it appears to be a standard ARM
GIC, though i'm not sure exactly which one yet. However, it's stated in the
processor's datasheet that legacy interrupts aren't supported. It's one of the
newer Samsung Exynos processors.

Thanks,

Ed

> Hope this helps
> 
> Best Regards
> 
> Eric
> 
> > 
> > Thanks,
> > 
> > Ed
> > ___
> > kvmarm mailing list
> > kvmarm@lists.cs.columbia.edu
> > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
> > 
> 
> ___
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH V1 0/7] Enable ACPI support for ARM KVM GIC

[Marc Zyngier]

Wei,

On 05/02/16 17:07, Wei Huang wrote:
> This patch set enables ACPI support for KVM GIC. Note that the patches
> are in fact the V3 of previously submitted patches (search "Enable ACPI
> support for KVM ARM"). But because Fu Wei includes the arch_timer part
> in his series [1] and I have substantially re-written the GIC code in this
> revision, the version number is reset to v1. 
> 
> By following Marc's prior comments, the main design idea is to let DT or
> ACPI code to fill out the "struct vgic_params" which are extended to
> include all GIC related info.

I think you misread the comments I gave back in June (!), where I said:


[...]

Simply making available a global structure containing the base addresses
and interrupt should be enough, and could be shared with both DT and
ACPI. You could start your series by letting both GIC drivers expose
that information obtained through DT, convert KVM to use this structure,
and later on let ACPI fill in this structure too.

> Anyway the difficulty is to find a common place to store and share info
> between other modules & KVM.

Indeed. As a rule of thumb, I want to minimize the amount of gratuitous
divergence between DT and ACPI. So the sooner we extract the required
information from whatever firmware we have, the better.


> 
> [1] https://lkml.org/lkml/2016/2/1/658
> 
> Thanks,
> -Wei
> 
> Wei Huang (7):
>   KVM: GIC: Move GIC DT probing code to GICv2 and GICv3 files
>   KVM: GIC: Add extra fields to store GICH and GICV resource info
>   KVM: GIC: Create a common probe function for GIC
>   KVM: GICv2: Extract the common code from DT
>   KVM: GICv2: Add ACPI probing function
>   KVM: GICv3: Extract the common code from DT
>   KVM: GICv3: Add ACPI probing function
> 
>  include/kvm/arm_vgic.h  |  14 ++--
>  virt/kvm/arm/vgic-v2-emul.c |   4 +-
>  virt/kvm/arm/vgic-v2.c  | 186 
> +---
>  virt/kvm/arm/vgic-v3.c  | 159 -
>  virt/kvm/arm/vgic.c |  22 +-
>  5 files changed, 277 insertions(+), 108 deletions(-)
> 

So when I see this diffstat and the patches that follow, I cannot help
but think that we do have a disconnect here. You do add a bunch of ACPI
probing in KVM, to which I've already said no.

I want to see the probing code in the GIC drivers, exported through a
common structure that KVM can then use. That's it. Nothing else.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: ARM PCI/MSI KVM passthrough with GICv2M

[Christoffer Dall]

On Fri, Feb 05, 2016 at 11:17:00AM -0700, Alex Williamson wrote:
> On Fri, 5 Feb 2016 18:32:07 +0100
> Eric Auger  wrote:
> 
> > Hi Alex,
> > 
> > I tried to sketch a proposal for guaranteeing the IRQ integrity when
> > doing ARM PCI/MSI passthrough with ARM GICv2M msi-controller. This is
> > based on extended VFIO group viability control, as detailed below.
> > 
> > As opposed to ARM GICv3 ITS, this MSI controller does *not* support IRQ
> > remapping. It can expose 1 or more 4kB MSI frame. Each frame contains a
> > single register where the msi data is written.
> > 
> > I would be grateful to you if you could tell me whether it makes any sense.
> > 
> > Thanks in advance
> > 
> > Best Regards
> > 
> > Eric
> > 
> > 
> > 1) GICv2m with a single 4kB single frame
> >all devices having this msi-controller as msi-parent share this
> >single MSI frame. Those devices can work on behalf of the host
> >or work on behalf of 1 or more guests (KVM assigned devices). We
> >must make sure either the host only or 1 single VM can access to the
> >single frame to guarantee interrupt integrity: a device assigned
> >to 1 VM should not be able to trigger MSI targeted to the host
> >or another VM.
> > 
> >I would propose to extend the VFIO notion of group viability.
> >Currently a VFIO group is viable if:
> >all devices belonging to the same group are bound to a VFIO driver
> >or unbound.
> > 
> >Let's imagine we extend the viability check as follows:
> > 
> >0) keep the current viable check: all the devices belonging to
> >   the group must be vfio bound or unbound.
> >1) retrieve the MSI parent of the device and list all the
> >   other devices using that MSI controller as MSI-parent (does not
> >   look straightforward):
> >2) they must be VFIO driver bound or unbound as well (meaning
> >   they are not used by the host). If not, reject device attachment
> >- in case they are VFIO bound (a VFIO group is set):
> >  x if all VFIO containers are the same as the one of the device's
> >we try to attach, that's OK. This means the other devices
> >use different IOMMU mappings, eventually will target the
> >MSI frame but they all work for the same user space client/VM.
> >  x 1 or more devices has a different container than the device
> >under attachment:
> >It works on behalf of a different user space client/VM,
> >we can't attach the new device. I think there is a case however
> >where severals containers can be opened by a single QEMU.
> > 
> > Of course the dynamic aspects, ie a new device showing up or an unbind
> > event bring significant complexity.
> > 
> > 2) GICv2M with multiple 4kB frames
> >Each msi-frame is enumerated as msi-controller. The device tree
> >statically defines which device is attached to each msi frame.
> >In case devices are assigned we cannot change this attachment
> >anyway since there might be physical contraints behind.
> >So devices likely to be assigned to guests should be linked to a
> >different MSI frame than devices that are not.
> > 
> >I think extended viability concept can be used as well.
> > 
> >This model still is not ideal: in case we have a SR-IOV device
> >plugged onto an host bridge attached to a single MSI parent you won't
> >be able anyway to have 1 Virtual Function working for host and 1 VF
> >working for a guest. Only Interrupt translation (ITS) will bring that
> >feature.
> > 
> > 3) GICv3 ITS
> >This one supports interrupt translation service ~ Intel
> >IRQ remapping.
> >This means a single frame can be used by all devices. A deviceID is
> >used exclusively by the host or a guest. I assume the ITS driver
> >allocates/populates deviceid interrupt translation table featuring
> >separate LPI spaces ie by construction different ITT cannot feature
> >same LPIs. So no need to do the extended viability test.
> > 
> >The MSI controller should have a property telling whether
> >it supports interrupt translation. This kind of property currently
> >exists on IOMMU side for INTEL remapping.
> > 
> 
> Hi Eric,
> 
> Would anyone be terribly upset if we simply assume the worst case
> scenario on GICv2m/M, have the IOMMU not claim IOMMU_CAP_INTR_REMAP, and
> require the user to opt-in via the allow_unsafe_interrupts on the
> vfio_iommu_type1 module?  That would make it very compatible with what
> we already do on x86, where it really is all or nothing.  

meaning either you allow unsafe multiplexing with passthrough in every
flavor (unsafely) or you don't allow it at all?

I didn't know such on option existed, but it seems to me that this fits
the bill exactly.


> My assumption
> is that GICv2 would be phased out in favor of GICv3, so there's always
> a hardware upgrade path to having more complete isolation, but the
> return on investment for figuring out whether