Re: [RFC PATCH 24/33] irqchip/gic-v3-its: Add VPE scheduling

[Shanker Donthineni]

Hi Marc,

On 01/17/2017 04:20 AM, Marc Zyngier wrote:

When a VPE is scheduled to run, the corresponding redistributor must
be told so, by setting VPROPBASER to the VM's property table, and
VPENDBASER to the vcpu's pending table.

When scheduled out, we preserve the IDAI and PendingLast bits. The
latter is specially important, as it tells the hypervisor that
there are pending interrupts for this vcpu.

Signed-off-by: Marc Zyngier 
---
  drivers/irqchip/irq-gic-v3-its.c   | 57
++
  include/linux/irqchip/arm-gic-v3.h | 63
++
  2 files changed, 120 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3-its.c
b/drivers/irqchip/irq-gic-v3-its.c
index 598e25b..f918d59 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -143,6 +143,7 @@ static DEFINE_IDA(its_vpeid_ida);
  
  #define gic_data_rdist()		(raw_cpu_ptr(gic_rdists->rdist))

  #define gic_data_rdist_rd_base()  (gic_data_rdist()->rd_base)
+#define gic_data_rdist_vlpi_base() (gic_data_rdist_rd_base() +
SZ_128K)
  
  static struct its_collection *dev_event_to_col(struct its_device

*its_dev,
   u32 event)
@@ -2039,8 +2040,64 @@ static const struct irq_domain_ops its_domain_ops =
{
.deactivate = its_irq_domain_deactivate,
  };
  
+static int its_vpe_set_vcpu_affinity(struct irq_data *d, void *vcpu_info)

+{
+   struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
+   struct its_cmd_info *info = vcpu_info;
+   u64 val;
+
+   switch (info->cmd_type) {
+   case SCHEDULE_VPE:
+   {
+   void * __iomem vlpi_base = gic_data_rdist_vlpi_base();
+
+   /* Schedule the VPE */
+   val  = virt_to_phys(page_address(vpe->its_vm->vprop_page))
&
+   GENMASK_ULL(51, 12);
+   val |= (LPI_NRBITS - 1) & GICR_VPROPBASER_IDBITS_MASK;
+   val |= GICR_VPROPBASER_RaWb;
I know Write-allocation hints are not that important for PROP table, but 
like to know any reason why Wa hints are not enabled? Enable Ra if there 
is no strong reason behind it.

+   val |= GICR_VPROPBASER_InnerShareable;
+   gits_write_vpropbaser(val, vlpi_base + GICR_VPROPBASER);
+
+   val  = virt_to_phys(page_address(vpe->vpt_page)) &
GENMASK(51, 16);
+   val |= GICR_VPENDBASER_WaWb;
it's very important to enable read allocation hints for performance 
point of view. I think both Ra and Wa can be enabled without loosing any 
functionality.




+   val |= GICR_VPENDBASER_NonShareable;
+   val |= GICR_PENDBASER_PendingLast;
+   val |= vpe->idai ? GICR_PENDBASER_IDAI : 0;
+   val |= GICR_PENDBASER_Valid;
+   gits_write_vpendbaser(val, vlpi_base + GICR_VPENDBASER);
+
+   return 0;
+   }
+
+   case DESCHEDULE_VPE:
+   {
+   void * __iomem vlpi_base = gic_data_rdist_vlpi_base();
+
+   /* We're being scheduled out */
+   val = gits_read_vpendbaser(vlpi_base + GICR_VPENDBASER);
+   val &= ~GICR_PENDBASER_Valid;
+   gits_write_vpendbaser(val, vlpi_base + GICR_VPENDBASER);
+
+   val = gits_read_vpendbaser(vlpi_base + GICR_VPENDBASER);
+   while (val & GICR_PENDBASER_Dirty) {
+   cpu_relax();
+   val = gits_read_vpendbaser(vlpi_base +
GICR_VPENDBASER);
+   }
+
+   vpe->idai = !!(val & GICR_PENDBASER_IDAI);
+   vpe->pending_last = !!(val & GICR_PENDBASER_PendingLast);
+   return 0;
+   }
+
+   default:
+   return -EINVAL;
+   }
+}
+
  static struct irq_chip its_vpe_irq_chip = {
.name   = "GICv4-vpe",
+   .irq_set_vcpu_affinity  = its_vpe_set_vcpu_affinity,
  };
  
  static int its_vpe_id_alloc(void)

diff --git a/include/linux/irqchip/arm-gic-v3.h
b/include/linux/irqchip/arm-gic-v3.h
index 1b3a070..2789c9a 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -209,6 +209,69 @@
  #define LPI_PROP_ENABLED  (1 << 0)
  
  /*

+ * Re-Distributor registers, offsets from VLPI_base
+ */
+#define GICR_VPROPBASER0x0070
+
+#define GICR_VPROPBASER_IDBITS_MASK0x1f
+
+#define GICR_VPROPBASER_SHAREABILITY_SHIFT (10)
+#define GICR_VPROPBASER_INNER_CACHEABILITY_SHIFT   (7)
+#define GICR_VPROPBASER_OUTER_CACHEABILITY_SHIFT   (56)
+
+#define GICR_VPROPBASER_SHAREABILITY_MASK  \
+   GIC_BASER_SHAREABILITY(GICR_VPROPBASER, SHAREABILITY_MASK)
+#define GICR_VPROPBASER_INNER_CACHEABILITY_MASK
\
+   GIC_BASER_CACHEABILITY(GICR_VPROPBASER, INNER, MASK)
+#define GICR_VPROPBASER_OUTER_CACHEABILITY_MASK
\
+   GIC_BASER_CACHEABILITY(GICR_VPROPBASER, OUTER, MASK)
+#define 

Re: [RFC PATCH 28/33] irqchip/gic-v3-its: Support VPE doorbell invalidation even when !DirectLPI

[Shanker Donthineni]

Hi Marc,


On 01/17/2017 04:20 AM, Marc Zyngier wrote:

When we don't have the DirectLPI feature, we must work around the
architecture shortcomings to be able to perform the required
invalidation.

For this, we create a fake device whose sole purpose is to
provide a way to issue a map/inv/unmap sequence (and the corresponding
sync operations). That's 6 commands and a full serialization point
to be able to do this.

You just have hope the hypervisor won't do that too often...

Signed-off-by: Marc Zyngier 
---
  drivers/irqchip/irq-gic-v3-its.c | 59
++--
  1 file changed, 57 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c
b/drivers/irqchip/irq-gic-v3-its.c
index 008fb71..3787579 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -133,6 +133,9 @@ struct its_device {
u32 device_id;
  };
  
+static struct its_device *vpe_proxy_dev;

+static DEFINE_RAW_SPINLOCK(vpe_proxy_dev_lock);
+
  static LIST_HEAD(its_nodes);
  static DEFINE_SPINLOCK(its_lock);
  static struct rdists *gic_rdists;
@@ -993,8 +996,35 @@ static void lpi_update_config(struct irq_data *d, u8
clr, u8 set)
struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
void __iomem *rdbase;
  
-		rdbase = per_cpu_ptr(gic_rdists->rdist,

vpe->col_idx)->rd_base;
-   writeq_relaxed(d->hwirq, rdbase + GICR_INVLPIR);
+   if (gic_rdists->has_direct_lpi) {
+   rdbase = per_cpu_ptr(gic_rdists->rdist,
vpe->col_idx)->rd_base;
+   writeq_relaxed(d->hwirq, rdbase + GICR_INVLPIR);
+   } else {
+   /*
+* This is insane.
+*
+* If a GICv4 doesn't implement Direct LPIs,
+* the only way to perform an invalidate is to
+* use a fake device to issue a MAP/INV/UNMAP
+* sequence. Since each of these commands has
+* a sync operation, this is really fast. Not.
+*
+* We always use event 0, and this serialize
+* all VPE invalidations in the system.
+*
+* Broken by design(tm).
+*/
+   unsigned long flags;
+
+   raw_spin_lock_irqsave(_proxy_dev_lock, flags);
+
+   vpe_proxy_dev->event_map.col_map[0] =
vpe->col_idx;
+   its_send_mapvi(vpe_proxy_dev, vpe->vpe_db_lpi, 0);
+   its_send_inv(vpe_proxy_dev, 0);
+   its_send_discard(vpe_proxy_dev, 0);
+
+   raw_spin_unlock_irqrestore(_proxy_dev_lock,
flags);
+   }
}
  }
  
@@ -2481,6 +2511,31 @@ static struct irq_domain *its_init_vpe_domain(void)

struct fwnode_handle *handle;
struct irq_domain *domain;
  
+	if (gic_rdists->has_direct_lpi) {

+   pr_info("ITS: Using DirectLPI for VPE invalidation\n");
+   } else {
+   struct its_node *its;
+
+   list_for_each_entry(its, _nodes, entry) {
+   u32 devid;
+
+   if (!its->is_v4)
+   continue;
+
+   /* Use the last possible DevID */
+   devid = GENMASK(its->device_ids - 1, 0);
How do we know this 'devid' is not being used by real hardware devices? 
I think we need some kind check in its_msi_prepare() to skip this device 
or WARN.

Unfortunately Qualcomm doesn't support Direct LPI feature.


--
Shanker Donthineni
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

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

Re: [RFC PATCH 24/33] irqchip/gic-v3-its: Add VPE scheduling

[Shanker Donthineni]

Hi Marc,


On 01/17/2017 04:20 AM, Marc Zyngier wrote:

When a VPE is scheduled to run, the corresponding redistributor must
be told so, by setting VPROPBASER to the VM's property table, and
VPENDBASER to the vcpu's pending table.

When scheduled out, we preserve the IDAI and PendingLast bits. The
latter is specially important, as it tells the hypervisor that
there are pending interrupts for this vcpu.

Signed-off-by: Marc Zyngier 
---
  drivers/irqchip/irq-gic-v3-its.c   | 57
++
  include/linux/irqchip/arm-gic-v3.h | 63
++
  2 files changed, 120 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3-its.c
b/drivers/irqchip/irq-gic-v3-its.c
index 598e25b..f918d59 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -143,6 +143,7 @@ static DEFINE_IDA(its_vpeid_ida);
  
  #define gic_data_rdist()		(raw_cpu_ptr(gic_rdists->rdist))

  #define gic_data_rdist_rd_base()  (gic_data_rdist()->rd_base)
+#define gic_data_rdist_vlpi_base() (gic_data_rdist_rd_base() +
SZ_128K)
  
  static struct its_collection *dev_event_to_col(struct its_device

*its_dev,
   u32 event)
@@ -2039,8 +2040,64 @@ static const struct irq_domain_ops its_domain_ops =
{
.deactivate = its_irq_domain_deactivate,
  };
  
+static int its_vpe_set_vcpu_affinity(struct irq_data *d, void *vcpu_info)

+{
+   struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
+   struct its_cmd_info *info = vcpu_info;
+   u64 val;
+
+   switch (info->cmd_type) {
+   case SCHEDULE_VPE:
+   {
+   void * __iomem vlpi_base = gic_data_rdist_vlpi_base();
+
+   /* Schedule the VPE */
+   val  = virt_to_phys(page_address(vpe->its_vm->vprop_page))
&
+   GENMASK_ULL(51, 12);
+   val |= (LPI_NRBITS - 1) & GICR_VPROPBASER_IDBITS_MASK;
+   val |= GICR_VPROPBASER_RaWb;
+   val |= GICR_VPROPBASER_InnerShareable;
+   gits_write_vpropbaser(val, vlpi_base + GICR_VPROPBASER);
+
+   val  = virt_to_phys(page_address(vpe->vpt_page)) &
GENMASK(51, 16);
+   val |= GICR_VPENDBASER_WaWb;
+   val |= GICR_VPENDBASER_NonShareable;
+   val |= GICR_PENDBASER_PendingLast;
Why always enabling PendingLast bit? You are keeping the pending last 
bit in vpe->pending_last but not used here.

+   val |= vpe->idai ? GICR_PENDBASER_IDAI : 0;
+   val |= GICR_PENDBASER_Valid;
What is the status of the virtual CPU interface (ICH_HCR_EL2.En) at the 
time of changing vPE state to resident? I believe the GICR will start 
processing a pending table immediately after setting V=1 and with valid 
PHYS address, GICR signals a virtual IRQ to virtual CPU if it has 
outstanding interrupts. With my understanding of GIC spec, a virtual CPU 
interface must be enabled before setting GICR_VPENDBASERR.Valid=1. 
Otherwise, it leads to flooded vSet and Release messages between GICR 
and CPU interface.


You can find Vset details in section 'A.4.18 VSet (IRI)' of GIC spec.

The CPU interface must Release an interrupt, ensuring that V == 1, if it 
cannot handle the interrupt for either of the

following reasons:
 The interrupt group is disabled. This includes when the VM 
interface is disabled, that is, when

GICH_HCR.En or ICH_HCR.En, as appropriate, is cleared to 0.


+   gits_write_vpendbaser(val, vlpi_base + GICR_VPENDBASER);
+
+   return 0;
+   }
+
+   case DESCHEDULE_VPE:
+   {
+   void * __iomem vlpi_base = gic_data_rdist_vlpi_base();
+
+   /* We're being scheduled out */
+   val = gits_read_vpendbaser(vlpi_base + GICR_VPENDBASER);
+   val &= ~GICR_PENDBASER_Valid;
+   gits_write_vpendbaser(val, vlpi_base + GICR_VPENDBASER);
+
+   val = gits_read_vpendbaser(vlpi_base + GICR_VPENDBASER);
+   while (val & GICR_PENDBASER_Dirty) {
+   cpu_relax();
+   val = gits_read_vpendbaser(vlpi_base +
GICR_VPENDBASER);
+   }
+
+   vpe->idai = !!(val & GICR_PENDBASER_IDAI);
+   vpe->pending_last = !!(val & GICR_PENDBASER_PendingLast);
+   return 0;
+   }
+
+   default:
+   return -EINVAL;
+   }
+}
+
  static struct irq_chip its_vpe_irq_chip = {
.name   = "GICv4-vpe",
+   .irq_set_vcpu_affinity  = its_vpe_set_vcpu_affinity,
  };
  
  static int its_vpe_id_alloc(void)

diff --git a/include/linux/irqchip/arm-gic-v3.h
b/include/linux/irqchip/arm-gic-v3.h
index 1b3a070..2789c9a 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -209,6 +209,69 @@
  #define LPI_PROP_ENABLED  (1 << 0)
  
  /*

+ * Re-Distributor registers, offsets from VLPI_base
+ 

Re: [RFC PATCH 23/33] irqchip/gic-v3-its: Add VPENDBASER/VPROPBASER accessors

[Shanker Donthineni]

Hi Marc,


On 01/17/2017 04:20 AM, Marc Zyngier wrote:

V{PEND,PROP}BASER being 64bit registers, they need some ad-hoc
accessors on 32bit, specially given that VPENDBASER contains
a Valid bit, making the access a bit convoluted.

Signed-off-by: Marc Zyngier 
---
  arch/arm/include/asm/arch_gicv3.h   | 28 
  arch/arm64/include/asm/arch_gicv3.h |  5 +
  2 files changed, 33 insertions(+)

diff --git a/arch/arm/include/asm/arch_gicv3.h
b/arch/arm/include/asm/arch_gicv3.h
index 2747590..3f18832 100644
--- a/arch/arm/include/asm/arch_gicv3.h
+++ b/arch/arm/include/asm/arch_gicv3.h
@@ -291,5 +291,33 @@ static inline u64 __gic_readq_nonatomic(const
volatile void __iomem *addr)
   */
  #define gits_write_cwriter(v, c)  __gic_writeq_nonatomic(v, c)
  
+/*

+ * GITS_VPROPBASER - hi and lo bits may be accessed independently.
+ */
+#define gits_write_vpropbaser(v, c)__gic_writeq_nonatomic(v, c)
+
+/*
+ * GITS_VPENDBASER - the Valid bit must be cleared before changing
+ * anything else.
+ */
+static inline void gits_write_vpendbaser(u64 val, void * __iomem addr)
+{
+   u32 tmp;
+
+   tmp = readl_relaxed(addr + 4);
+   if (tmp & GICR_PENDBASER_Valid) {
+   tmp &= ~GICR_PENDBASER_Valid;
+   writel_relaxed(tmp, addr + 4);
+   }
+
+   /*
+* Use the fact that __gic_writeq_nonatomic writes the second
+* half of the 64bit quantity after the first.
+*/
+   __gic_writeq_nonatomic(val, addr);
I'm not sure whether software has to check a register write pending bit 
GICR_CTLR.RWP or not. GICv3 spec says, the effect of a write to 
GICR_VPENDBASER register is not guaranteed to be visible throughout the 
affinity hierarchy,as indicated by GICR_CTLR.RWP == 0.


--
Shanker Donthineni
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

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

Re: [RFC PATCH 21/33] irqchip/gic-v3-its: Add VPE irq domain allocation/teardown

[Shanker Donthineni]

Hi Marc,


On 01/17/2017 04:20 AM, Marc Zyngier wrote:

When creating a VM, the low level GICv4 code is responsible for:
- allocating each VPE a unique VPEID
- allocating a doorbell interrupt for each VPE
- allocating the pending tables for each VPE
- allocating the property table for the VM

This of course has to be reversed when the VM is brought down.

All of this is wired into the irq domain alloc/free methods.

Signed-off-by: Marc Zyngier 
---
  drivers/irqchip/irq-gic-v3-its.c | 174
+++
  1 file changed, 174 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3-its.c
b/drivers/irqchip/irq-gic-v3-its.c
index ddd8096..54d0075 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -139,6 +139,7 @@ static struct rdists *gic_rdists;
  static struct irq_domain *its_parent;
  
  static unsigned long its_list_map;

+static DEFINE_IDA(its_vpeid_ida);
  
  #define gic_data_rdist()		(raw_cpu_ptr(gic_rdists->rdist))

  #define gic_data_rdist_rd_base()  (gic_data_rdist()->rd_base)
@@ -1146,6 +1147,11 @@ static struct page *its_allocate_prop_table(gfp_t
gfp_flags)
return prop_page;
  }
  
+static void its_free_prop_table(struct page *prop_page)

+{
+   free_pages((unsigned long)page_address(prop_page),
+  get_order(LPI_PROPBASE_SZ));
+}
  
  static int __init its_alloc_lpi_tables(void)

  {
@@ -1444,6 +1450,12 @@ static struct page
*its_allocate_pending_table(gfp_t gfp_flags)
return pend_page;
  }
  
+static void its_free_pending_table(struct page *pt)

+{
+   free_pages((unsigned long)page_address(pt),
+  get_order(max(LPI_PENDBASE_SZ, SZ_64K)));
+}
+
  static void its_cpu_init_lpis(void)
  {
void __iomem *rbase = gic_data_rdist_rd_base();
@@ -1666,6 +1678,34 @@ static bool its_alloc_device_table(struct its_node
*its, u32 dev_id)
return its_alloc_table_entry(baser, dev_id);
  }
  
+static bool its_alloc_vpe_table(u32 vpe_id)

+{
+   struct its_node *its;
+
+   /*
+* Make sure the L2 tables are allocated on *all* v4 ITSs. We
+* could try and only do it on ITSs corresponding to devices
+* that have interrupts targeted at this VPE, but the
+* complexity becomes crazy (and you have tons of memory
+* anyway, right?).
+*/
+   list_for_each_entry(its, _nodes, entry) {
+   struct its_baser *baser;
+
+   if (!its->is_v4)
+   continue;
+
+   baser = its_get_baser(its, GITS_BASER_TYPE_VCPU);
+   if (!baser)
+   return false;
+
+   if (!its_alloc_table_entry(baser, vpe_id))
+   return false;
+   }
+
+   return true;
+}
+
  static struct its_device *its_create_device(struct its_node *its, u32
dev_id,
int nvecs)
  {
@@ -1922,7 +1962,141 @@ static struct irq_chip its_vpe_irq_chip = {
.name   = "GICv4-vpe",
  };
  
+static int its_vpe_id_alloc(void)

+{
+   return ida_simple_get(_vpeid_ida, 0, 1 << 16, GFP_KERNEL);
+}
+
+static void its_vpe_id_free(u16 id)
+{
+   ida_simple_remove(_vpeid_ida, id);
+}
+
+static int its_vpe_init(struct its_vpe *vpe)
+{
+   struct page *vpt_page;
+   int vpe_id;
+
+   /* Allocate vpe_id */
+   vpe_id = its_vpe_id_alloc();
+   if (vpe_id < 0)
+   return vpe_id;
+
+   /* Allocate VPT */
+   vpt_page = its_allocate_pending_table(GFP_KERNEL);
+   if (vpt_page) {



Change to 'if (!vpt_page)'.


--
Shanker Donthineni
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

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

Re: [RFC PATCH 17/33] irqchip/gic-v3-its: Add VLPI configuration hook

[Shanker Donthineni]

On 01/17/2017 04:20 AM, Marc Zyngier wrote:

Add the skeleton irq_set_vcpu_affinity method that will be used
to configure VLPIs.

Signed-off-by: Marc Zyngier 
---
  drivers/irqchip/irq-gic-v3-its.c | 33 +
  1 file changed, 33 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3-its.c
b/drivers/irqchip/irq-gic-v3-its.c
index 0dbc8b0..1bd78ca 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -36,6 +36,7 @@
  
  #include 

  #include 
+#include 
  
  #include 

  #include 
@@ -771,6 +772,37 @@ static int its_irq_set_irqchip_state(struct irq_data
*d,
return 0;
  }
  
+static int its_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu_info)

+{
+   struct its_device *its_dev = irq_data_get_irq_chip_data(d);
+   struct its_cmd_info *info = vcpu_info;
+   u32 event = its_get_event_id(d);
+
+   /* Need a v4 ITS */
+   if (!its_dev->its->is_v4 || !info)
+   return -EINVAL;
+
+   switch (info->cmd_type) {
+   case MAP_VLPI:
+   {
+   return 0;
+   }
+
+   case UNMAP_VLPI:
+   {
+   return 0;
+   }
+
+   case PROP_UPDATE_VLPI:
+   {
+   return 0;
+   }
+
+   default:
+   return -EINVAL;
+   }

Missing a return statement.

--
Shanker Donthineni
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

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

Re: [PATCH V8 06/10] acpi: apei: panic OS with fatal error status block

[Baicar, Tyler]

Hello James,


On 2/9/2017 3:48 AM, James Morse wrote:

Hi Jonathan, Tyler,

On 01/02/17 17:16, Tyler Baicar wrote:

From: "Jonathan (Zhixiong) Zhang" 

Even if an error status block's severity is fatal, the kernel does not
honor the severity level and panic.

With the firmware first model, the platform could inform the OS about a
fatal hardware error through the non-NMI GHES notification type. The OS
should panic when a hardware error record is received with this
severity.

Call panic() after CPER data in error status block is printed if
severity is fatal, before each error section is handled.
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 8756172..86c1f15 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -687,6 +689,13 @@ static int ghes_ack_error(struct acpi_hest_generic_v2 
*generic_v2)
return rc;
  }
  
+static void __ghes_call_panic(void)

+{
+   if (panic_timeout == 0)
+   panic_timeout = ghes_panic_timeout;
+   panic("Fatal hardware error!");
+}
+

__ghes_panic() also has:

__ghes_print_estatus(KERN_EMERG, ghes->generic, ghes->estatus);

Which prints this estatus regardless of rate limiting and cache-ing.

[ ... ]


@@ -698,6 +707,10 @@ static int ghes_proc(struct ghes *ghes)
if (ghes_print_estatus(NULL, ghes->generic, ghes->estatus))

ghes_print_estatus() uses some custom rate limiting '2 messages every 5
seconds', GHES_SEV_PANIC shares the same limit as GHES_SEV_RECOVERABLE.

I think its possible to get 2 recoverable messages, then a panic in a 5 second
window. The rate limit will kick in to stop the panic estatus block being
printed, but we still go on to call panic() without the real reason being 
printed...

(the caching thing only seems to consider identical messages, given we would
never see two panic messages, I don't think that will cause any problems.)


ghes_estatus_cache_add(ghes->generic, ghes->estatus);
}
+   if (ghes_severity(ghes->estatus->error_severity) >= GHES_SEV_PANIC) {
+   __ghes_call_panic();
+   }
+

I think this ghes_severity() then panic() should go above the:

if (!ghes_estatus_cached(ghes->estatus)) {

and we should call __ghes_print_estatus() here too, to make sure the message
definitely got out!
Okay, that makes sense. If we move this up, is there a problem with 
calling __ghes_panic() instead of making the __ghes_print_estatus() and 
__ghes_call_panic() calls here? It looks like that will just add a call 
to oops_begin() and ghes_print_queued_estatus() as well, but this is 
what ghes_notify_nmi() does if the severity is panic.


Thanks,
Tyler

With that,
Reviewed-by: James Morse 


Thanks,

James


--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

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

Re: [RFC PATCH 11/33] irqchip/gic-v3-its: Split out pending table allocation

[Shanker Donthineni]

Hi Marc,


On 01/17/2017 04:20 AM, Marc Zyngier wrote:

Just as for the property table, let's move the pending table
allocation to a separate function.

Signed-off-by: Marc Zyngier 
---
  drivers/irqchip/irq-gic-v3-its.c | 29 -
  1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c
b/drivers/irqchip/irq-gic-v3-its.c
index 14305db1..dce8f8c 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1188,6 +1188,24 @@ static int its_alloc_collections(struct its_node
*its)
return 0;
  }
  
+static struct page *its_allocate_pending_table(gfp_t gfp_flags)

+{
PEND and PROP table sizes are defined as compile time macros, but as per 
ITS spec implementation 24bit LPI space is also possible. It would be 
nicer to parametrize

both the tables sizes so that it would easier to enable 24bit LPI later.

Actually Qualcomm server chips support 24bit IDBITS.

--
Shanker Donthineni
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

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

Re: [RFC PATCH 10/33] irqchip/gic-v4-its: Allow use of indirect VCPU tables

[Shanker Donthineni]

Hi Marc,


On 01/17/2017 04:20 AM, Marc Zyngier wrote:

The VCPU tables can be quite sparse as well, and it makes sense
to use indirect tables as well if possible.
The VCPU table has maximum of 2^16 entries as compared to 2^32 entries 
in device table. ITS hardware implementations may not support indirect 
table because of low memory requirement.

Signed-off-by: Marc Zyngier 
---
  drivers/irqchip/irq-gic-v3-its.c | 20 +---
  1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c
b/drivers/irqchip/irq-gic-v3-its.c
index c92ff4d..14305db1 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1060,10 +1060,13 @@ static int its_setup_baser(struct its_node *its,
struct its_baser *baser,
return 0;
  }
  
-static bool its_parse_baser_device(struct its_node *its, struct its_baser

*baser,
-  u32 psz, u32 *order)
+static bool its_parse_indirect_baser(struct its_node *its,
+struct its_baser *baser,
+u32 psz, u32 *order)
  {
-   u64 esz = GITS_BASER_ENTRY_SIZE(its_read_baser(its, baser));
+   u64 tmp = its_read_baser(its, baser);
+   u64 type = GITS_BASER_TYPE(tmp);
+   u64 esz = GITS_BASER_ENTRY_SIZE(tmp);
u64 val = GITS_BASER_InnerShareable | GITS_BASER_WaWb;
u32 ids = its->device_ids;
u32 new_order = *order;
@@ -1102,8 +1105,9 @@ static bool its_parse_baser_device(struct its_node
*its, struct its_baser *baser
if (new_order >= MAX_ORDER) {
new_order = MAX_ORDER - 1;
ids = ilog2(PAGE_ORDER_TO_SIZE(new_order) / (int)esz);
-   pr_warn("ITS@%pa: Device Table too large, reduce ids
%u->%u\n",
-   >phys_base, its->device_ids, ids);
+   pr_warn("ITS@%pa: %s Table too large, reduce ids
%u->%u\n",
+   >phys_base, its_base_type_string[type],
+   its->device_ids, ids);
}
  
  	*order = new_order;

@@ -1154,8 +1158,10 @@ static int its_alloc_tables(struct its_node *its)
if (type == GITS_BASER_TYPE_NONE)
continue;
  
-		if (type == GITS_BASER_TYPE_DEVICE)

-   indirect = its_parse_baser_device(its, baser, psz,
);
Try to allocate maximum memory as possible then attempt enabling 
indirection table.


#define ITS_VPES_MAX(65536)

if (type == GITS_BASER_TYPE_VCPU)
 order = get_order(esz * ITS_VPES_MAX);

On Qualcomm implementation, 1MBytes, 64536  * 16Byte (vPE entry size) 
memory is enough to sparse 16bit vPE.

+   if (type == GITS_BASER_TYPE_DEVICE ||
+   type == GITS_BASER_TYPE_VCPU)
+   indirect = its_parse_indirect_baser(its, baser,
+   psz, );
  
  		err = its_setup_baser(its, baser, cache, shr, psz, order,

indirect);
if (err < 0) {


--
Shanker Donthineni
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

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

Re: [RFC PATCH 06/33] irqchip/gic-v3-its: Add probing for VLPI properties

[Shanker Donthineni]

On 01/17/2017 04:20 AM, Marc Zyngier wrote:

Add the probing code for the ITS VLPI support. This includes
configuring the ITS number if not supporting the single VMOVP
command feature.

Signed-off-by: Marc Zyngier 
---
  drivers/irqchip/irq-gic-v3-its.c   | 47
++
  include/linux/irqchip/arm-gic-v3.h |  4 
  2 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c
b/drivers/irqchip/irq-gic-v3-its.c
index 9304dd2..99f6130 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -103,6 +103,7 @@ struct its_node {
u32 ite_size;
u32 device_ids;
int numa_node;
+   boolis_v4;
  };
  
  #define ITS_ITT_ALIGN		SZ_256

@@ -135,6 +136,8 @@ static DEFINE_SPINLOCK(its_lock);
  static struct rdists *gic_rdists;
  static struct irq_domain *its_parent;
  
+static unsigned long its_list_map;

+
  #define gic_data_rdist()  (raw_cpu_ptr(gic_rdists->rdist))
  #define gic_data_rdist_rd_base()  (gic_data_rdist()->rd_base)
  
@@ -1661,8 +1664,8 @@ static int __init its_probe_one(struct resource

*res,
  {
struct its_node *its;
void __iomem *its_base;
-   u32 val;
-   u64 baser, tmp;
+   u32 val, ctlr;
+   u64 baser, tmp, typer;
int err;
  
  	its_base = ioremap(res->start, resource_size(res));

@@ -1695,9 +1698,44 @@ static int __init its_probe_one(struct resource
*res,
raw_spin_lock_init(>lock);
INIT_LIST_HEAD(>entry);
INIT_LIST_HEAD(>its_device_list);
+   typer = gic_read_typer(its_base + GITS_TYPER);
its->base = its_base;
its->phys_base = res->start;
-   its->ite_size = ((gic_read_typer(its_base + GITS_TYPER) >> 4) &
0xf) + 1;
+   its->ite_size = ((typer >> 4) & 0xf) + 1;

I think we should move bit manipulations to a macro, some thing like this.
its->ite_size = GITS_TYPER_ITEBITS(typer);

#define GITS_TYPER_ITEBITS_SHIFT4
#define GITS_TYPER_ITEBITS(r)   r) >> 
GITS_TYPER_ITEBITS_SHIFT) & 0xf) + 1)



+   its->is_v4 = !!(typer & GITS_TYPER_VLPIS);
+   if (its->is_v4 && !(typer & GITS_TYPER_VMOVP)) {
+   int its_number;
+
+   its_number = find_first_zero_bit(_list_map, 16);
+   if (its_number >= 16) {
+   pr_err("ITS@%pa: No ITSList entry available!\n",
+  >start);
+   err = -EINVAL;
+   goto out_free_its;
+   }
+
+   ctlr = readl_relaxed(its_base + GITS_CTLR);
+   ctlr &= ~GITS_CTLR_ITS_NUMBER;
+   ctlr |= its_number << GITS_CTLR_ITS_NUMBER_SHIFT;
+   writel_relaxed(ctlr, its_base + GITS_CTLR);
+   ctlr = readl_relaxed(its_base + GITS_CTLR);
+   if ((ctlr & GITS_CTLR_ITS_NUMBER) != (its_number <<
GITS_CTLR_ITS_NUMBER_SHIFT)) {
+   its_number = ctlr & GITS_CTLR_ITS_NUMBER;
+   its_number >>= GITS_CTLR_ITS_NUMBER_SHIFT;
+   }
+
+   if (test_and_set_bit(its_number, _list_map)) {
+   pr_err("ITS@%pa: Duplicate ITSList entry %d\n",
+  >start, its_number);
+   err = -EINVAL;
+   goto out_free_its;
+   }
+
+   pr_info("ITS@%pa: Using ITS number %d\n", >start,
its_number);
+   } else {
+   pr_info("ITS@%pa: Single VMOVP capable\n", >start);
+   }

Can we move to a separate function for code readability purpose?

--
Shanker Donthineni
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

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

Re: [RFC PATCH 02/33] irqchip/gic-v3: Add VLPI/DirectLPI discovery

[Shanker Donthineni]

On 01/17/2017 04:20 AM, Marc Zyngier wrote:

Add helper functions that probe for VLPI and DirectLPI properties.

Signed-off-by: Marc Zyngier 
---
  drivers/irqchip/irq-gic-v3.c   | 22 ++
  include/linux/irqchip/arm-gic-v3.h |  3 +++
  2 files changed, 25 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 5cadec0..8a6de91 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -514,6 +514,24 @@ static int gic_populate_rdist(void)
return -ENODEV;
  }
  
+static int __gic_update_vlpi_properties(struct redist_region *region,

+   void __iomem *ptr)
+{
+   u64 typer = gic_read_typer(ptr + GICR_TYPER);
+   gic_data.rdists.has_vlpis &= !!(typer & GICR_TYPER_VLPIS);
+   gic_data.rdists.has_direct_lpi &= !!(typer &
GICR_TYPER_DirectLPIS);
+
+   return 1;
+}
+
+static void gic_update_vlpi_properties(void)
+{
+   gic_scan_rdist_properties(__gic_update_vlpi_properties);
+   pr_info("%sVLPI support, %sdirect LPI support\n",

Would be better if we keep one space after 'no'?

--
Shanker Donthineni
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

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

Re: [RFC PATCH 01/33] irqchip/gic-v3: Add redistributor iterator

[Shanker Donthineni]

Hi Marc,


On 01/17/2017 04:20 AM, Marc Zyngier wrote:

In order to discover the VLPI properties, we need to iterate over
the redistributor regions. As we already have code that does this,
let's factor it out and make it slightly more generic.

Signed-off-by: Marc Zyngier 
---
  drivers/irqchip/irq-gic-v3.c | 77

  1 file changed, 56 insertions(+), 21 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index c132f29..5cadec0 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -421,24 +421,15 @@ static void __init gic_dist_init(void)
gic_write_irouter(affinity, base + GICD_IROUTER + i * 8);
  }
  
-static int gic_populate_rdist(void)

+static int gic_scan_rdist_properties(int (*fn)(struct redist_region *,
+  void __iomem *))
I don't see this function is parsing GICR properties, may be it makes 
readable on changing name to gic_redist_iterator().



  {
-   unsigned long mpidr = cpu_logical_map(smp_processor_id());
-   u64 typer;
-   u32 aff;
+   int ret = 0;
For readability purpose set  ret = ENODEV, to cover error case where 
gic_data.nr_redist_regions == 0.

int i;
  
-	/*

-* Convert affinity to a 32bit value that can be matched to
-* GICR_TYPER bits [63:32].
-*/
-   aff = (MPIDR_AFFINITY_LEVEL(mpidr, 3) << 24 |
-  MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16 |
-  MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8 |
-  MPIDR_AFFINITY_LEVEL(mpidr, 0));
-
for (i = 0; i < gic_data.nr_redist_regions; i++) {
void __iomem *ptr =
gic_data.redist_regions[i].redist_base;
+   u64 typer;
u32 reg;
  
  		reg = readl_relaxed(ptr + GICR_PIDR2) &

GIC_PIDR2_ARCH_MASK;
@@ -450,14 +441,14 @@ static int gic_populate_rdist(void)
  
  		do {

typer = gic_read_typer(ptr + GICR_TYPER);
-   if ((typer >> 32) == aff) {
-   u64 offset = ptr -
gic_data.redist_regions[i].redist_base;
-   gic_data_rdist_rd_base() = ptr;
-   gic_data_rdist()->phys_base =
gic_data.redist_regions[i].phys_base + offset;
-   pr_info("CPU%d: found redistributor %lx
region %d:%pa\n",
-   smp_processor_id(), mpidr, i,
-   _data_rdist()->phys_base);
+   ret = fn(gic_data.redist_regions + i, ptr);
+   switch (ret) {
+   case 0:
return 0;
+   case -1:
+   break;
+   default:
+   ret = 0;
}
  
  			if (gic_data.redist_regions[i].single_redist)

@@ -473,9 +464,53 @@ static int gic_populate_rdist(void)
} while (!(typer & GICR_TYPER_LAST));
  
  
+	if (ret == -1)

+   ret = -ENODEV;
+
__gic_populate_rdist() returns 1 to try next entry in the list. We 
should not return value 0 here if no matching entry is found otherwise 
the gic_populate_rdist() assumes that it found the corresponding GICR.


+   return 0;
+}
+
+static int __gic_populate_rdist(struct redist_region *region, void
__iomem *ptr)
+{
+   unsigned long mpidr = cpu_logical_map(smp_processor_id());
+   u64 typer;
+   u32 aff;
+
+   /*
+* Convert affinity to a 32bit value that can be matched to
+* GICR_TYPER bits [63:32].
+*/
+   aff = (MPIDR_AFFINITY_LEVEL(mpidr, 3) << 24 |
+  MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16 |
+  MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8 |
+  MPIDR_AFFINITY_LEVEL(mpidr, 0));
+
+   typer = gic_read_typer(ptr + GICR_TYPER);
+   if ((typer >> 32) == aff) {
+   u64 offset = ptr - region->redist_base;
+   gic_data_rdist_rd_base() = ptr;
+   gic_data_rdist()->phys_base = region->phys_base + offset;
+
+   pr_info("CPU%d: found redistributor %lx region %d:%pa\n",
+   smp_processor_id(), mpidr,
+   (int)(region - gic_data.redist_regions),
+   _data_rdist()->phys_base);
+   return 0;
+   }
+
+   /* Try next one */
+   return 1;
+}
+
+static int gic_populate_rdist(void)
+{
+   if (gic_scan_rdist_properties(__gic_populate_rdist) == 0)

what about 'if (!gic_scan_rdist_properties(__gic_populate_rdist))'?

+   return 0;
+
/* We couldn't even deal with ourselves... */
WARN(true, "CPU%d: mpidr %lx has no re-distributor!\n",
-smp_processor_id(), mpidr);
+smp_processor_id(),
+(unsigned long)cpu_logical_map(smp_processor_id()));
return -ENODEV;
  }
  

Re: [RFC PATCH 00/33] irqchip: Core support for GICv4

[Thomas Gleixner]

Marc,

On Tue, 17 Jan 2017, Marc Zyngier wrote:

> This series implements the core support for GICv4. And despite its
> size, it does exactly *nothing*. What it adds is an infrastructure
> that a hypervisor (KVM) can use to route VLPIs to a guest.

That's a very well done patch set and it was a pleasure to review!

Thanks,

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

Re: [RFC PATCH 33/33] irqchip/gic-v4: Enable low-level GICv4 operations

[Thomas Gleixner]

On Tue, 17 Jan 2017, Marc Zyngier wrote:

> Get the show on the road...
> 
> Signed-off-by: Marc Zyngier 

Reviewed-by: Thomas Gleixner 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH 32/33] irqchip/gic-v4: Add some basic documentation

[Thomas Gleixner]

On Tue, 17 Jan 2017, Marc Zyngier wrote:

> Do a braindump of the way things are supposed to work.
> 
> Signed-off-by: Marc Zyngier 

Nice !

Reviewed-by: Thomas Gleixner 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH 31/33] irqchip/gic-v4: Add VLPI configuration interface

[Thomas Gleixner]

On Tue, 17 Jan 2017, Marc Zyngier wrote:
> Add the required interfaces to map, unmap and update a VLPI.
> 
> Signed-off-by: Marc Zyngier 

Reviewed-by: Thomas Gleixner 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH 30/33] irqchip/gic-v4: Add VPE command interface

[Thomas Gleixner]

On Tue, 17 Jan 2017, Marc Zyngier wrote:

> Add the required interfaces to schedule a VPE and perform a
> VINVALL command.
> 
> Signed-off-by: Marc Zyngier 

Reviewed-by: Thomas Gleixner 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH 29/33] irqchip/gic-v4: Add per-VM VPE domain creation

[Thomas Gleixner]

On Tue, 17 Jan 2017, Marc Zyngier wrote:

> When creating a VM, it is very convenient to have an irq domain
> containing all the doorbell interrupts associated with that VM
> (each interrupt representing a VPE).
> 
> Signed-off-by: Marc Zyngier 

Reviewed-by: Thomas Gleixner 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH 28/33] irqchip/gic-v3-its: Support VPE doorbell invalidation even when !DirectLPI

[Thomas Gleixner]

On Tue, 17 Jan 2017, Marc Zyngier wrote:
> When we don't have the DirectLPI feature, we must work around the
> architecture shortcomings to be able to perform the required
> invalidation.
> 
> For this, we create a fake device whose sole purpose is to
> provide a way to issue a map/inv/unmap sequence (and the corresponding
> sync operations). That's 6 commands and a full serialization point
> to be able to do this.
> 
> You just have hope the hypervisor won't do that too often...
> 
> Signed-off-by: Marc Zyngier 

Reviewed-by: Thomas Gleixner 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH 27/33] irqchip/gic-v3-its: Add VPE interrupt masking

[Thomas Gleixner]

On Tue, 17 Jan 2017, Marc Zyngier wrote:

> When masking/unmasking a doorbell interrupt, it is necessary
> to issue an invalidation to the corresponding redistributor.
> We use the DirectLPI feature by writting directly to the corresponding
> redistributor.
> 
> Signed-off-by: Marc Zyngier 

Reviewed-by: Thomas Gleixner 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH 26/33] irqchip/gic-v3-its: Add VPE affinity changes

[Thomas Gleixner]

On Tue, 17 Jan 2017, Marc Zyngier wrote:

> When we're about to run a vcpu, it is crucial that the redistributor
> associated with the physical CPU is being told about the new residency.
> 
> This is abstracted by hijacking the irq_set_affinity method for the
> doorbell interrupt associated with the VPE. It is expected that the
> hypervisor will call this method before scheduling the VPE.
> 
> Signed-off-by: Marc Zyngier 

Reviewed-by: Thomas Gleixner 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH 24/33] irqchip/gic-v3-its: Add VPE scheduling

[Thomas Gleixner]

On Tue, 17 Jan 2017, Marc Zyngier wrote:
> +static int its_vpe_set_vcpu_affinity(struct irq_data *d, void *vcpu_info)
> +{
> + struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
> + struct its_cmd_info *info = vcpu_info;
> + u64 val;
> +
> + switch (info->cmd_type) {
> + case SCHEDULE_VPE:
> + {
> + void * __iomem vlpi_base = gic_data_rdist_vlpi_base();

Moving the vlpi_base line on top of the fucntion spares you lines and
brackets. No point in having the same thing twice

Thanks,

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

Re: [RFC PATCH 25/33] irqchip/gic-v3-its: Add VPE invalidation hook

[Thomas Gleixner]

On Tue, 17 Jan 2017, Marc Zyngier wrote:

> When a guest issues a INVALL command targetting a collection, it must
> be translated into a VINVALL for the VPE that has this collection.
> 
> This patch implements a hook that offers this functionallity to the
> hypervisor.
> 
> Signed-off-by: Marc Zyngier 

Reviewed-by: Thomas Gleixner 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH 23/33] irqchip/gic-v3-its: Add VPENDBASER/VPROPBASER accessors

[Thomas Gleixner]

On Tue, 17 Jan 2017, Marc Zyngier wrote:

> V{PEND,PROP}BASER being 64bit registers, they need some ad-hoc
> accessors on 32bit, specially given that VPENDBASER contains
> a Valid bit, making the access a bit convoluted.
> 
> Signed-off-by: Marc Zyngier 

Reviewed-by: Thomas Gleixner 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH 22/33] irqchip/gic-v3-its: Add VPE irq domain [de]activation

[Thomas Gleixner]

On Tue, 17 Jan 2017, Marc Zyngier wrote:

> On activation, a VPE is mapped using the VMAPP command, followed
> by a VINVALL for a good measure. On deactivation, the VPE is
> simply unmapped.
> 
> Signed-off-by: Marc Zyngier 

Reviewed-by: Thomas Gleixner 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH 21/33] irqchip/gic-v3-its: Add VPE irq domain allocation/teardown

[Thomas Gleixner]

On Tue, 17 Jan 2017, Marc Zyngier wrote:
> +static int its_vpe_irq_domain_alloc(struct irq_domain *domain, unsigned int 
> virq,
> + unsigned int nr_irqs, void *args)
> +{
> + msi_alloc_info_t *info = args;
> + struct its_vpe **vpes = info->scratchpad[0].ptr;
> + struct its_vm *vm = vpes[0]->its_vm;
> + unsigned long *bitmap;
> + struct page *vprop_page;
> + int base, nr_ids, i, err = 0;
> +
> + bitmap = its_lpi_alloc_chunks(nr_irqs, , _ids);
> + if (!bitmap)
> + return -ENOMEM;
> +
> + if (nr_ids < nr_irqs) {
> + its_lpi_free_chunks(bitmap, base, nr_ids);
> + return -ENOMEM;
> + }
> +
> + vprop_page = its_allocate_prop_table(GFP_KERNEL);
> + if (!vprop_page) {
> + its_lpi_free_chunks(bitmap, base, nr_ids);
> + return ENOMEM;
> + }
> +
> + for (i = 0; i < nr_irqs; i++) {
> + vpes[i]->vpe_db_lpi = base + i;
> + err = its_vpe_init(vpes[i]);
> + if (err)
> + break;
> + irq_domain_set_hwirq_and_chip(domain,
> +   virq + i, vpes[i]->vpe_db_lpi,
> +   _vpe_irq_chip, vpes[i]);
> + set_bit(i, bitmap);
> + }
> +
> + if (err) {
> + while (--i >= 0) {
> + its_vpe_teardown(vpes[i]);
> + clear_bit(i, bitmap);
> + }
> +
> + its_lpi_free_chunks(bitmap, base, nr_ids);
> + its_free_prop_table(vprop_page);

Couldn't you just do:

if (err) {
if (i > 0)
its_vpe_irq_domain_free(domain, virq, i - 1)

Hmm?

Thanks,

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

Re: [RFC PATCH 20/33] irqchip/gic-v3-its: Add VPE domain infrastructure

[Thomas Gleixner]

On Tue, 17 Jan 2017, Marc Zyngier wrote:
> @@ -2266,6 +2294,8 @@ int __init its_init(struct fwnode_handle *handle, 
> struct rdists *rdists,
>   struct irq_domain *parent_domain)
>  {
>   struct device_node *of_node;
> + struct its_node *its;
> + bool has_v4 = false;
>  
>   its_parent = parent_domain;
>   of_node = to_of_node(handle);
> @@ -2283,5 +2313,11 @@ int __init its_init(struct fwnode_handle *handle, 
> struct rdists *rdists,
>   its_alloc_lpi_tables();
>   its_lpi_init(rdists->id_bits);
>  
> + list_for_each_entry(its, _nodes, entry)
> + has_v4 |= its->is_v4;

Hmm, can you have mixed version its nodes? If not, you probably should have
some sanity check (not necessarily here). If yes, then how does that work?

Thanks,

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

Re: [RFC PATCH 19/33] irqchip/gic-v3-its: Add VLPI configuration handling

[Thomas Gleixner]

On Tue, 17 Jan 2017, Marc Zyngier wrote:

> When a VLPI is reconfigured (enabled, disabled, change in priority),
> the full configuration byte must be written, and the caches invalidated.
> 
> Also, when using the irq_mask/irq_unmask methods, it is necessary
> to disable the doorbell for that particular interrupt (by mapping it
> to 1023) on top of clearing the Enable bit.
> 
> Signed-off-by: Marc Zyngier 

Aside of the switch/case part:

Reviewed-by: Thomas Gleixner 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH 17/33] irqchip/gic-v3-its: Add VLPI configuration hook

[Thomas Gleixner]

On Tue, 17 Jan 2017, Marc Zyngier wrote:
> +static int its_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu_info)
> +{
> + struct its_device *its_dev = irq_data_get_irq_chip_data(d);
> + struct its_cmd_info *info = vcpu_info;
> + u32 event = its_get_event_id(d);
> +
> + /* Need a v4 ITS */
> + if (!its_dev->its->is_v4 || !info)
> + return -EINVAL;
> +
> + switch (info->cmd_type) {
> + case MAP_VLPI:
> + {

Looking at the later patches which add functionality here, I'd rather avoid
these massive case { } blocks and stick the functionality into seperate
helper functions.

Thanks,

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

Re: [RFC PATCH 16/33] irqchip/gic-v3-its: Add GICv4 ITS command definitions

[Thomas Gleixner]

On Tue, 17 Jan 2017, Marc Zyngier wrote:

> Add the new GICv4 ITS command definitions, most of them, being
> defined in terms of their physical counterparts.
> 
> Signed-off-by: Marc Zyngier 

Reviewed-by: Thomas Gleixner 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH 15/33] irqchip/gic-v4: Add management structure definitions

[Thomas Gleixner]

On Tue, 17 Jan 2017, Marc Zyngier wrote:

> Add a bunch of GICv4-specific data structures that will get used in
> subsequent patches.
> 
> Signed-off-by: Marc Zyngier 

Reviewed-by: Thomas Gleixner 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH 14/33] irqchip/gic-v3-its: Generalize LPI configuration

[Thomas Gleixner]

On Tue, 17 Jan 2017, Marc Zyngier wrote:

> We're are going to need to change a bit more than just the enable
> bit in the LPI property table in the future. So let's change the
> LPI configuration funtion to take a set of bits to be cleared,
> and a set of bits to be set.
> 
> This way, we'll be able to use it when a guest updates an LPI
> property (priority, for example).
> 
> Signed-off-by: Marc Zyngier 

Reviewed-by: Thomas Gleixner 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH 13/33] irqchip/gic-v3-its: Generalize device table allocation

[Thomas Gleixner]

On Tue, 17 Jan 2017, Marc Zyngier wrote:

> As we want to use 2-level tables for VCPUs, let's hack the device
> table allocator in order to make it slightly more generic. It
> will get reused in subsequent patches.
> 
> Signed-off-by: Marc Zyngier 

Reviewed-by: Thomas Gleixner 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH 12/33] irqchip/gic-v3-its: Rework LPI freeing

[Thomas Gleixner]

On Tue, 17 Jan 2017, Marc Zyngier wrote:

> Rework LPI deallocation so that it can be reused by the v4 support
> code.
> 
> Signed-off-by: Marc Zyngier 

Reviewed-by: Thomas Gleixner 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH 11/33] irqchip/gic-v3-its: Split out pending table allocation

[Thomas Gleixner]

On Tue, 17 Jan 2017, Marc Zyngier wrote:

> Just as for the property table, let's move the pending table
> allocation to a separate function.
> 
> Signed-off-by: Marc Zyngier 

Reviewed-by: Thomas Gleixner 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH 10/33] irqchip/gic-v4-its: Allow use of indirect VCPU tables

[Thomas Gleixner]

On Tue, 17 Jan 2017, Marc Zyngier wrote:

> The VCPU tables can be quite sparse as well, and it makes sense
> to use indirect tables as well if possible.
> 
> Signed-off-by: Marc Zyngier 

Reviewed-by: Thomas Gleixner 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH 08/33] irqchip/gic-v3-its: Implement irq_set_irqchip_state for pending state

[Thomas Gleixner]

On Tue, 17 Jan 2017, Marc Zyngier wrote:

> Allow the pending state of an LPI to be set or cleared via
> irq_set_irqchip_state.
> 
> Signed-off-by: Marc Zyngier 

Reviewed-by: Thomas Gleixner 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH 07/33] irqchip/gic-v3-its: Macro-ize its_send_single_command

[Thomas Gleixner]

On Tue, 17 Jan 2017, Marc Zyngier wrote:
> +
> +static __its_send_single_cmd(its_send_single_command, its_cmd_builder_t,
> +  struct its_collection, its_build_sync_cmd)

I'm fine with that macro magic, but the above is very unintuitive as it
looks like a normal function.

The way this is done in other places is to have:

#define BUILD_CMD_FUNC(name,) \


and then have

BUILD_CMD_FUNC(its_send_single_command, );

That makes it entirely obvious that this is macro magic.

Thanks,

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

Re: [RFC v2 05/19] KVM: arm64: ITS: Implement vgic_its_has_attr_regs and attr_regs_access

[Auger Eric]

Hi Andre,

On 10/02/2017 18:06, Andre Przywara wrote:
> Hi,
> 
> On 10/02/17 12:26, Auger Eric wrote:
>> Hi Andre,
>>
>> On 10/02/2017 12:57, Andre Przywara wrote:
>>> On 08/02/17 11:43, Eric Auger wrote:
>>>
>>> Salut Eric,
>>>
>>> one minor thing below, but first a general question:
>>> I take it that the state of the ITS (enabled/disabled) shouldn't matter
>>> when it comes to reading/writing registers, right? Because this is
>>> totally under guest control and userland shouldn't mess with it?
>>>
>>> Maybe this is handled somewhere in the next patches, but how are we
>>> supposed to write CBASER and the BASERs, for instance, if the handler
>>> bails out early when the ITS is already enabled?
>> Well I mentioned in the KVM ITS device documentation 
> 
> Oh, I am one of those guys who read the documentation at the very end
> ;-) Sorry, my bad.
> 
>> that userspace
>> accesses to those registers have the same behavior as guest accesses. As
>> such it is not possible to write into CBASER and BASERs when the ITS is
>> already enabled. But isn't it correct to forbid such userspace accesses
>> at this moment? What is the use case?
> 
> So the idea is to observe an order when restoring the registers? And
> relying on the ITS being disabled upon creation, so that we can write to
> those registers? Then restoring CTLR as the very last to get things going?
Yes that's what I currently do on QEMU side.
> 
> I think we need some special handling for CWRITER as well, but I just
> see that we have some bug in our ITS emulation (processing commands
> while the ITS being disabled and not triggering command processing upon
> ITS enablement). Waiting for Marc's verdict on this.
> I think the patch I came up with should fix this particular issue here.
OK Looking forward to reviewing it.

Thanks

Eric
> 
> But apart from that it should work, I think.
> 
> Cheers,
> Andre.
> 
 This patch implements vgic_its_has_attr_regs and vgic_its_attr_regs_access
 upon the MMIO framework. VGIC ITS KVM device KVM_DEV_ARM_VGIC_GRP_ITS_REGS
 group becomes functional.

 At least GITS_CREADR requires to differentiate a guest write action from a
 user access. As such let's introduce a new uaccess_its_write
 vgic_register_region callback.

 Signed-off-by: Eric Auger 
 ---
  virt/kvm/arm/vgic/vgic-its.c  | 74 
 ---
  virt/kvm/arm/vgic/vgic-mmio.h |  9 --
  2 files changed, 69 insertions(+), 14 deletions(-)

 diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
 index 43bb17e..e9c8f9f 100644
 --- a/virt/kvm/arm/vgic/vgic-its.c
 +++ b/virt/kvm/arm/vgic/vgic-its.c
 @@ -1287,13 +1287,14 @@ static void vgic_mmio_write_its_baser(struct kvm 
 *kvm,
*regptr = reg;
  }
  
 -#define REGISTER_ITS_DESC(off, rd, wr, length, acc)   \
 +#define REGISTER_ITS_DESC(off, rd, wr, uwr, length, acc)  \
  { \
.reg_offset = off,  \
.len = length,  \
.access_flags = acc,\
.its_read = rd, \
.its_write = wr,\
 +  .uaccess_its_write = uwr,   \
  }
  
  static void its_mmio_write_wi(struct kvm *kvm, struct vgic_its *its,
 @@ -1304,28 +1305,28 @@ static void its_mmio_write_wi(struct kvm *kvm, 
 struct vgic_its *its,
  
  static struct vgic_register_region its_registers[] = {
REGISTER_ITS_DESC(GITS_CTLR,
 -  vgic_mmio_read_its_ctlr, vgic_mmio_write_its_ctlr, 4,
 +  vgic_mmio_read_its_ctlr, vgic_mmio_write_its_ctlr, NULL, 4,
VGIC_ACCESS_32bit),
REGISTER_ITS_DESC(GITS_IIDR,
 -  vgic_mmio_read_its_iidr, its_mmio_write_wi, 4,
 +  vgic_mmio_read_its_iidr, its_mmio_write_wi, NULL, 4,
VGIC_ACCESS_32bit),
REGISTER_ITS_DESC(GITS_TYPER,
 -  vgic_mmio_read_its_typer, its_mmio_write_wi, 8,
 +  vgic_mmio_read_its_typer, its_mmio_write_wi, NULL, 8,
VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
REGISTER_ITS_DESC(GITS_CBASER,
 -  vgic_mmio_read_its_cbaser, vgic_mmio_write_its_cbaser, 8,
 +  vgic_mmio_read_its_cbaser, vgic_mmio_write_its_cbaser, NULL, 8,
VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
REGISTER_ITS_DESC(GITS_CWRITER,
 -  vgic_mmio_read_its_cwriter, vgic_mmio_write_its_cwriter, 8,
 -  VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
 +  vgic_mmio_read_its_cwriter, vgic_mmio_write_its_cwriter, NULL,
 +  8, VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
REGISTER_ITS_DESC(GITS_CREADR,
 -

Re: [RFC PATCH 06/33] irqchip/gic-v3-its: Add probing for VLPI properties

[Thomas Gleixner]

On Tue, 17 Jan 2017, Marc Zyngier wrote:
> + typer = gic_read_typer(its_base + GITS_TYPER);
>   its->base = its_base;
>   its->phys_base = res->start;
> - its->ite_size = ((gic_read_typer(its_base + GITS_TYPER) >> 4) & 0xf) + 
> 1;
> + its->ite_size = ((typer >> 4) & 0xf) + 1;
> + its->is_v4 = !!(typer & GITS_TYPER_VLPIS);
> + if (its->is_v4 && !(typer & GITS_TYPER_VMOVP)) {
> + int its_number;
> +
> + its_number = find_first_zero_bit(_list_map, 16);

s/16/ITS_MAX_ENTITIES or whatever.

> + if (its_number >= 16) {
> + pr_err("ITS@%pa: No ITSList entry available!\n",
> +>start);
> + err = -EINVAL;
> + goto out_free_its;
> + }
> +
> + ctlr = readl_relaxed(its_base + GITS_CTLR);
> + ctlr &= ~GITS_CTLR_ITS_NUMBER;
> + ctlr |= its_number << GITS_CTLR_ITS_NUMBER_SHIFT;
> + writel_relaxed(ctlr, its_base + GITS_CTLR);
> + ctlr = readl_relaxed(its_base + GITS_CTLR);
> + if ((ctlr & GITS_CTLR_ITS_NUMBER) != (its_number << 
> GITS_CTLR_ITS_NUMBER_SHIFT)) {
> + its_number = ctlr & GITS_CTLR_ITS_NUMBER;
> + its_number >>= GITS_CTLR_ITS_NUMBER_SHIFT;
> + }
> +
> + if (test_and_set_bit(its_number, _list_map)) {

You just established above that the bit is not set. I assume that this is
code which has no concurrency concerns

Thanks,

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

Re: [RFC PATCH 04/33] irqchip/gic-v3-its: Move LPI definitions around

[Thomas Gleixner]

On Tue, 17 Jan 2017, Marc Zyngier wrote:

> The various LPI definitions are in the middle of the code, and
> would be better placed at the beginning, given that we're going
> to use some of them much earlier.
> 
> Signed-off-by: Marc Zyngier 

Reviewed-by: Thomas Gleixner 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH 03/33] irqchip/gic-v3-its: Refactor command encoding

[Thomas Gleixner]

On Tue, 17 Jan 2017, Marc Zyngier wrote:
> +static void its_mask_encode(u64 *raw_cmd, u64 val, int h, int l)

I'd rather name h/l in a way which makes it clear that they are describing
a bit range. msb/lsb perhaps.

> +{
> + u64 mask = GENMASK_ULL(h, l);

New line missing here.

> + *raw_cmd &= ~mask;
> + *raw_cmd |= (val << l) & mask;
> +}
> +
>  static void its_encode_cmd(struct its_cmd_block *cmd, u8 cmd_nr)
>  {
> - cmd->raw_cmd[0] &= ~0xffULL;
> - cmd->raw_cmd[0] |= cmd_nr;
> + its_mask_encode(>raw_cmd[0], cmd_nr, 7, 0);

I assume that the manual provides a name for these bit ranges. Wouldn't it
be helpful to use them in these functions?

Looks good otherwise.

Thanks,

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

Re: [RFC PATCH 02/33] irqchip/gic-v3: Add VLPI/DirectLPI discovery

[Thomas Gleixner]

On Tue, 17 Jan 2017, Marc Zyngier wrote:

> Add helper functions that probe for VLPI and DirectLPI properties.
> 
> Signed-off-by: Marc Zyngier 

Reviewed-by: Thomas Gleixner 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH 01/33] irqchip/gic-v3: Add redistributor iterator

[Thomas Gleixner]

On Tue, 17 Jan 2017, Marc Zyngier wrote:

> In order to discover the VLPI properties, we need to iterate over
> the redistributor regions. As we already have code that does this,
> let's factor it out and make it slightly more generic.
> 
> Signed-off-by: Marc Zyngier 

Reviewed-by: Thomas Gleixner 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm