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

2017-02-16 Thread Auger Eric 
Hi Marc,
On 17/01/2017 11:20, Marc Zyngier wrote:
> Move the LPI property table allocation into its own function, as
> this is going to be required for those associated with VMs in
> the future.
> 
> Signed-off-by: Marc Zyngier 
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 28 ++--
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
> b/drivers/irqchip/irq-gic-v3-its.c
> index b28fb19..c92ff4d 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -889,15 +889,31 @@ static void its_lpi_free(struct event_lpi_map *map)
>   kfree(map->col_map);
>  }
>  
> +static struct page *its_allocate_prop_table(gfp_t gfp_flags)
> +{
> + struct page *prop_page;
> +
> + prop_page = alloc_pages(gfp_flags, get_order(LPI_PROPBASE_SZ));
> + if (!prop_page)
> + return NULL;
> +
> + /* Priority 0xa0, Group-1, disabled */
> + memset(page_address(prop_page),
> +LPI_PROP_DEFAULT_PRIO | LPI_PROP_GROUP1,
> +LPI_PROPBASE_SZ);
> +
> + /* Make sure the GIC will observe the written configuration */
> + gic_flush_dcache_to_poc(page_address(gic_rdists->prop_page), 
> LPI_PROPBASE_SZ);
s/gic_rdists->prop_page/prop_page.

With that fix,

Reviewed-by: Eric Auger 

Eric


>  
> + return prop_page;
> +}
>  
>  
>  static int __init its_alloc_lpi_tables(void)
>  {
>   phys_addr_t paddr;
>  
> - gic_rdists->prop_page = alloc_pages(GFP_NOWAIT,
> -get_order(LPI_PROPBASE_SZ));
> + gic_rdists->prop_page = its_allocate_prop_table(GFP_NOWAIT);
>   if (!gic_rdists->prop_page) {
>   pr_err("Failed to allocate PROPBASE\n");
>   return -ENOMEM;
> @@ -906,14 +922,6 @@ static int __init its_alloc_lpi_tables(void)
>   paddr = page_to_phys(gic_rdists->prop_page);
>   pr_info("GIC: using LPI property table @%pa\n", );
>  
> - /* Priority 0xa0, Group-1, disabled */
> - memset(page_address(gic_rdists->prop_page),
> -LPI_PROP_DEFAULT_PRIO | LPI_PROP_GROUP1,
> -LPI_PROPBASE_SZ);
> -
> - /* Make sure the GIC will observe the written configuration */
> - gic_flush_dcache_to_poc(page_address(gic_rdists->prop_page), 
> LPI_PROPBASE_SZ);
> -
>   return 0;
>  }
>  
> 
___
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

2017-02-16 Thread Auger Eric 


On 17/01/2017 11:20, Marc Zyngier wrote:
> Most ITS commands do operate on a collection object, and require
> a SYNC command to be performed on that collection in order to
> guarantee the execution of the first command.
> 
> With GICv4 ITS, another set of commands perform similar operations
> on a VPE object, and a VSYNC operations must be executed to guarantee
> their execution.
> 
> Given the similarities (post a command, perform a synchronization
> operation on a sync object), it makes sense to reuse the same
> mechanism for both class of commands.
> 
> Let's start with turning its_send_single_command into a huge macro
> that performs the bulk of the work, and a set of helpers that
> make this macro useeable for the GICv3 ITS commands.
s/useeable/usable
> 
> Signed-off-by: Marc Zyngier 
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 82 
> ++--
>  1 file changed, 45 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
> b/drivers/irqchip/irq-gic-v3-its.c
> index 99f6130..520b764 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -484,43 +484,51 @@ static void its_wait_for_range_completion(struct 
> its_node *its,
>   }
>  }
>  
> -static void its_send_single_command(struct its_node *its,
> - its_cmd_builder_t builder,
> - struct its_cmd_desc *desc)
> -{
> - struct its_cmd_block *cmd, *sync_cmd, *next_cmd;
> - struct its_collection *sync_col;
> - unsigned long flags;
> -
> - raw_spin_lock_irqsave(>lock, flags);
> -
> - cmd = its_allocate_entry(its);
> - if (!cmd) { /* We're soo screewed... */
> - pr_err_ratelimited("ITS can't allocate, dropping command\n");
> - raw_spin_unlock_irqrestore(>lock, flags);
> - return;
> - }
> - sync_col = builder(cmd, desc);
> - its_flush_cmd(its, cmd);
> -
> - if (sync_col) {
> - sync_cmd = its_allocate_entry(its);
> - if (!sync_cmd) {
> - pr_err_ratelimited("ITS can't SYNC, skipping\n");
> - goto post;
> - }
> - its_encode_cmd(sync_cmd, GITS_CMD_SYNC);
> - its_encode_target(sync_cmd, sync_col->target_address);
> - its_fixup_cmd(sync_cmd);
> - its_flush_cmd(its, sync_cmd);
> - }
> -
> -post:
> - next_cmd = its_post_commands(its);
> - raw_spin_unlock_irqrestore(>lock, flags);
> -
> - its_wait_for_range_completion(its, cmd, next_cmd);
> -}
> +#define __its_send_single_cmd(name, buildtype, synctype, buildfn)\
> +void name(struct its_node *its,  
> \
> +   buildtype builder,\
> +   struct its_cmd_desc *desc)\
> +{\
> + struct its_cmd_block *cmd, *sync_cmd, *next_cmd;\
> + synctype *sync_obj; \
> + unsigned long flags;\
> + \
> + raw_spin_lock_irqsave(>lock, flags);   \
> + \
> + cmd = its_allocate_entry(its);  \
Not related to this patch but I see that its_allocate_entry can spin for
up to 1s waiting for the ITS queue to drain. What are the circumstances
that can cause this? Also isn't it an issue in that case to hold the
spinlock for so long?

Thanks

Eric
> + if (!cmd) { /* We're soo screewed... */ \
> + raw_spin_unlock_irqrestore(>lock, flags);  \
> + return; \
> + }   \
> + sync_obj = builder(cmd, desc);  \
> + its_flush_cmd(its, cmd);\
> + \
> + if (sync_obj) { \
> + sync_cmd = its_allocate_entry(its); \
> + if (!sync_cmd)  \
> + goto post;  \
> + \
> + buildfn(sync_cmd, sync_obj);\
> + its_fixup_cmd(sync_cmd);\
> + its_flush_cmd(its, sync_cmd);   \
> + }   \
> +

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

2017-02-16 Thread Auger Eric 


On 17/01/2017 11:20, 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: Eric Auger 


Eric

> ---
>  drivers/irqchip/irq-gic-v3-its.c | 78 
> 
>  1 file changed, 78 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
> b/drivers/irqchip/irq-gic-v3-its.c
> index 520b764..b28fb19 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -163,6 +163,11 @@ struct its_cmd_desc {
>   struct {
>   struct its_device *dev;
>   u32 event_id;
> + } its_clear_cmd;
> +
> + struct {
> + struct its_device *dev;
> + u32 event_id;
>   } its_int_cmd;
>  
>   struct {
> @@ -376,6 +381,40 @@ static struct its_collection *its_build_inv_cmd(struct 
> its_cmd_block *cmd,
>   return col;
>  }
>  
> +static struct its_collection *its_build_int_cmd(struct its_cmd_block *cmd,
> + struct its_cmd_desc *desc)
> +{
> + struct its_collection *col;
> +
> + col = dev_event_to_col(desc->its_int_cmd.dev,
> +desc->its_int_cmd.event_id);
> +
> + its_encode_cmd(cmd, GITS_CMD_INT);
> + its_encode_devid(cmd, desc->its_int_cmd.dev->device_id);
> + its_encode_event_id(cmd, desc->its_int_cmd.event_id);
> +
> + its_fixup_cmd(cmd);
> +
> + return col;
> +}
> +
> +static struct its_collection *its_build_clear_cmd(struct its_cmd_block *cmd,
> +   struct its_cmd_desc *desc)
> +{
> + struct its_collection *col;
> +
> + col = dev_event_to_col(desc->its_clear_cmd.dev,
> +desc->its_clear_cmd.event_id);
> +
> + its_encode_cmd(cmd, GITS_CMD_CLEAR);
> + its_encode_devid(cmd, desc->its_clear_cmd.dev->device_id);
> + its_encode_event_id(cmd, desc->its_clear_cmd.event_id);
> +
> + its_fixup_cmd(cmd);
> +
> + return col;
> +}
> +
>  static struct its_collection *its_build_invall_cmd(struct its_cmd_block *cmd,
>  struct its_cmd_desc *desc)
>  {
> @@ -530,6 +569,26 @@ static void its_build_sync_cmd(struct its_cmd_block 
> *sync_cmd,
>  static __its_send_single_cmd(its_send_single_command, its_cmd_builder_t,
>struct its_collection, its_build_sync_cmd)
>  
> +static void its_send_int(struct its_device *dev, u32 event_id)
> +{
> + struct its_cmd_desc desc;
> +
> + desc.its_int_cmd.dev = dev;
> + desc.its_int_cmd.event_id = event_id;
> +
> + its_send_single_command(dev->its, its_build_int_cmd, );
> +}
> +
> +static void its_send_clear(struct its_device *dev, u32 event_id)
> +{
> + struct its_cmd_desc desc;
> +
> + desc.its_clear_cmd.dev = dev;
> + desc.its_clear_cmd.event_id = event_id;
> +
> + its_send_single_command(dev->its, its_build_clear_cmd, );
> +}
> +
>  static void its_send_inv(struct its_device *dev, u32 event_id)
>  {
>   struct its_cmd_desc desc;
> @@ -693,6 +752,24 @@ static void its_irq_compose_msi_msg(struct irq_data *d, 
> struct msi_msg *msg)
>   iommu_dma_map_msi_msg(d->irq, msg);
>  }
>  
> +static int its_irq_set_irqchip_state(struct irq_data *d,
> +  enum irqchip_irq_state which,
> +  bool state)
> +{
> + struct its_device *its_dev = irq_data_get_irq_chip_data(d);
> + u32 event = its_get_event_id(d);
> +
> + if (which != IRQCHIP_STATE_PENDING)
> + return -EINVAL;
> +
> + if (state)
> + its_send_int(its_dev, event);
> + else
> + its_send_clear(its_dev, event);
> +
> + return 0;
> +}
> +
>  static struct irq_chip its_irq_chip = {
>   .name   = "ITS",
>   .irq_mask   = its_mask_irq,
> @@ -700,6 +777,7 @@ static struct irq_chip its_irq_chip = {
>   .irq_eoi= irq_chip_eoi_parent,
>   .irq_set_affinity   = its_set_affinity,
>   .irq_compose_msi_msg= its_irq_compose_msi_msg,
> + .irq_set_irqchip_state  = its_irq_set_irqchip_state,
>  };
>  
>  /*
> 
___
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

2017-02-16 Thread Auger Eric 
Hi Marc,

On 17/01/2017 11:20, 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 
> ---
>  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)
nit: Personally I would keep the previous name since when I read the new
one I have the impression indirect is always turned on.

Besides

Reviewed-by: Eric Auger 

Eric

>  {
> - 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, 
> );
> + 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) {
> 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] KVM: race-free exit from KVM_RUN without POSIX signals

2017-02-16 Thread David Hildenbrand 
>   post_kvm_run_save(vcpu);
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 7964b970b9ad..f51d5082a377 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -218,7 +218,8 @@ struct kvm_hyperv_exit {
>  struct kvm_run {
>   /* in */
>   __u8 request_interrupt_window;
> - __u8 padding1[7];
> + __u8 immediate_exit;

As mentioned already on IRC, maybe something like "block_vcpu_run" would
fit better now.

But this is also ok and looks good to me.

Reviewed-by: David Hildenbrand 

-- 
Thanks,

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

Re: [PATCH V10 05/10] acpi: apei: handle SEA notification type for ARMv8

2017-02-16 Thread Ard Biesheuvel 
On 15 February 2017 at 19:51, Tyler Baicar  wrote:
> ARM APEI extension proposal added SEA (Synchronous External Abort)
> notification type for ARMv8.
> Add a new GHES error source handling function for SEA. If an error
> source's notification type is SEA, then this function can be registered
> into the SEA exception handler. That way GHES will parse and report
> SEA exceptions when they occur.
> An SEA can interrupt code that had interrupts masked and is treated as
> an NMI. To aid this the page of address space for mapping APEI buffers
> while in_nmi() is always reserved, and ghes_ioremap_pfn_nmi() is
> changed to use the helper methods to find the prot_t to map with in
> the same way as ghes_ioremap_pfn_irq().
>
> Signed-off-by: Tyler Baicar 
> Signed-off-by: Jonathan (Zhixiong) Zhang 
> Signed-off-by: Naveen Kaje 
> ---
>  arch/arm64/Kconfig|  2 ++
>  arch/arm64/mm/fault.c | 13 
>  drivers/acpi/apei/Kconfig | 14 +
>  drivers/acpi/apei/ghes.c  | 77 
> +++
>  include/acpi/ghes.h   |  7 +
>  5 files changed, 107 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 1117421..8557556 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -53,6 +53,7 @@ config ARM64
> select HANDLE_DOMAIN_IRQ
> select HARDIRQS_SW_RESEND
> select HAVE_ACPI_APEI if (ACPI && EFI)
> +   select HAVE_ACPI_APEI_SEA if (ACPI && EFI)
> select HAVE_ALIGNED_STRUCT_PAGE if SLUB
> select HAVE_ARCH_AUDITSYSCALL
> select HAVE_ARCH_BITREVERSE
> @@ -88,6 +89,7 @@ config ARM64
> select HAVE_IRQ_TIME_ACCOUNTING
> select HAVE_MEMBLOCK
> select HAVE_MEMBLOCK_NODE_MAP if NUMA
> +   select HAVE_NMI if HAVE_ACPI_APEI_SEA
> select HAVE_PATA_PLATFORM
> select HAVE_PERF_EVENTS
> select HAVE_PERF_REGS
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index d178dc0..4e35c72 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -41,6 +41,8 @@
>  #include 
>  #include 
>
> +#include 
> +
>  static const char *fault_name(unsigned int esr);
>
>  #ifdef CONFIG_KPROBES
> @@ -498,6 +500,17 @@ static int do_sea(unsigned long addr, unsigned int esr, 
> struct pt_regs *regs)
> pr_err("Synchronous External Abort: %s (0x%08x) at 0x%016lx\n",
>  fault_name(esr), esr, addr);
>
> +   /*
> +* Synchronous aborts may interrupt code which had interrupts masked.
> +* Before calling out into the wider kernel tell the interested
> +* subsystems.
> +*/
> +   if(IS_ENABLED(HAVE_ACPI_APEI_SEA)) {

Missing space after 'if'

> +   nmi_enter();
> +   ghes_notify_sea();
> +   nmi_exit();
> +   }
> +
> info.si_signo = SIGBUS;
> info.si_errno = 0;
> info.si_code  = 0;
> diff --git a/drivers/acpi/apei/Kconfig b/drivers/acpi/apei/Kconfig
> index b0140c8..ef7f7bd 100644
> --- a/drivers/acpi/apei/Kconfig
> +++ b/drivers/acpi/apei/Kconfig
> @@ -4,6 +4,20 @@ config HAVE_ACPI_APEI
>  config HAVE_ACPI_APEI_NMI
> bool
>
> +config HAVE_ACPI_APEI_SEA

HAVE_xxx Kconfig options are typically non user selectable, so I
suggest to drop the HAVE_ prefix here. Also, you should probably make
it 'default y' rather than select it elsewhere; this will still honour
the dependency on ARM64 && ACPI_APEI_GHES



> +   bool "APEI Synchronous External Abort logging/recovering support"
> +   depends on ARM64 && ACPI_APEI_GHES
> +   help
> + This option should be enabled if the system supports
> + firmware first handling of SEA (Synchronous External Abort).
> + SEA happens with certain faults of data abort or instruction
> + abort synchronous exceptions on ARMv8 systems. If a system
> + supports firmware first handling of SEA, the platform analyzes
> + and handles hardware error notifications from SEA, and it may then
> + form a HW error record for the OS to parse and handle. This
> + option allows the OS to look for such hardware error record, and
> + take appropriate action.
> +
>  config ACPI_APEI
> bool "ACPI Platform Error Interface (APEI)"
> select MISC_FILESYSTEMS
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index b25e7cf..87045dc 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -114,11 +114,7 @@
>   * Two virtual pages are used, one for IRQ/PROCESS context, the other for
>   * NMI context (optionally).
>   */
> -#ifdef CONFIG_HAVE_ACPI_APEI_NMI
>  #define GHES_IOREMAP_PAGES   2
> -#else
> -#define GHES_IOREMAP_PAGES   1
> -#endif
>  #define GHES_IOREMAP_IRQ_PAGE(base)(base)
>  #define GHES_IOREMAP_NMI_PAGE(base)((base) + PAGE_SIZE)
>
> @@

Re: [PATCH V10 03/10] efi: parse ARM processor error

2017-02-16 Thread Ard Biesheuvel 
On 15 February 2017 at 19:51, Tyler Baicar  wrote:
> Add support for ARM Common Platform Error Record (CPER).
> UEFI 2.6 specification adds support for ARM specific
> processor error information to be reported as part of the
> CPER records. This provides more detail on for processor error logs.
>
> Signed-off-by: Tyler Baicar 
> Signed-off-by: Jonathan (Zhixiong) Zhang 
> Signed-off-by: Naveen Kaje 
> Reviewed-by: James Morse 

Reviewed-by: Ard Biesheuvel 

> ---
>  drivers/firmware/efi/cper.c | 133 
> 
>  include/linux/cper.h|  54 ++
>  2 files changed, 187 insertions(+)
>
> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index 8fa4e23..c2b0a12 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -110,12 +110,15 @@ void cper_print_bits(const char *pfx, unsigned int bits,
>  static const char * const proc_type_strs[] = {
> "IA32/X64",
> "IA64",
> +   "ARM",
>  };
>
>  static const char * const proc_isa_strs[] = {
> "IA32",
> "IA64",
> "X64",
> +   "ARM A32/T32",
> +   "ARM A64",
>  };
>
>  static const char * const proc_error_type_strs[] = {
> @@ -139,6 +142,18 @@ void cper_print_bits(const char *pfx, unsigned int bits,
> "corrected",
>  };
>
> +static const char * const arm_reg_ctx_strs[] = {
> +   "AArch32 general purpose registers",
> +   "AArch32 EL1 context registers",
> +   "AArch32 EL2 context registers",
> +   "AArch32 secure context registers",
> +   "AArch64 general purpose registers",
> +   "AArch64 EL1 context registers",
> +   "AArch64 EL2 context registers",
> +   "AArch64 EL3 context registers",
> +   "Misc. system register structure",
> +};
> +
>  static void cper_print_proc_generic(const char *pfx,
> const struct cper_sec_proc_generic *proc)
>  {
> @@ -184,6 +199,114 @@ static void cper_print_proc_generic(const char *pfx,
> printk("%s""IP: 0x%016llx\n", pfx, proc->ip);
>  }
>
> +static void cper_print_proc_arm(const char *pfx,
> +   const struct cper_sec_proc_arm *proc)
> +{
> +   int i, len, max_ctx_type;
> +   struct cper_arm_err_info *err_info;
> +   struct cper_arm_ctx_info *ctx_info;
> +   char newpfx[64];
> +
> +   printk("%s""section length: %d\n", pfx, proc->section_length);
> +   printk("%s""MIDR: 0x%016llx\n", pfx, proc->midr);
> +
> +   len = proc->section_length - (sizeof(*proc) +
> +   proc->err_info_num * (sizeof(*err_info)));
> +   if (len < 0) {
> +   printk("%s""section length is too small\n", pfx);
> +   printk("%s""firmware-generated error record is incorrect\n", 
> pfx);
> +   printk("%s""ERR_INFO_NUM is %d\n", pfx, proc->err_info_num);
> +   return;
> +   }
> +
> +   if (proc->validation_bits & CPER_ARM_VALID_MPIDR)
> +   printk("%s""MPIDR: 0x%016llx\n", pfx, proc->mpidr);
> +   if (proc->validation_bits & CPER_ARM_VALID_AFFINITY_LEVEL)
> +   printk("%s""error affinity level: %d\n", pfx,
> +   proc->affinity_level);
> +   if (proc->validation_bits & CPER_ARM_VALID_RUNNING_STATE) {
> +   printk("%s""running state: 0x%x\n", pfx, proc->running_state);
> +   printk("%s""PSCI state: %d\n", pfx, proc->psci_state);
> +   }
> +
> +   snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP);
> +
> +   err_info = (struct cper_arm_err_info *)(proc + 1);
> +   for (i = 0; i < proc->err_info_num; i++) {
> +   printk("%s""Error info structure %d:\n", pfx, i);
> +   printk("%s""version:%d\n", newpfx, err_info->version);
> +   printk("%s""length:%d\n", newpfx, err_info->length);
> +   if (err_info->validation_bits &
> +   CPER_ARM_INFO_VALID_MULTI_ERR) {
> +   if (err_info->multiple_error == 0)
> +   printk("%s""single error\n", newpfx);
> +   else if (err_info->multiple_error == 1)
> +   printk("%s""multiple errors\n", newpfx);
> +   else
> +   printk("%s""multiple errors count:%u\n",
> +   newpfx, err_info->multiple_error);
> +   }
> +   if (err_info->validation_bits & CPER_ARM_INFO_VALID_FLAGS) {
> +   if (err_info->flags & CPER_ARM_INFO_FLAGS_FIRST)
> +   printk("%s""first error captured\n", newpfx);
> +   if (err_info->flags & CPER_ARM_INFO_FLAGS_LAST)
> +   printk("%s""last error captured\n", newpfx);
> +

Re: [PATCH V10 02/10] ras: acpi/apei: cper: generic error data entry v3 per ACPI 6.1

2017-02-16 Thread Ard Biesheuvel 
On 15 February 2017 at 19:51, Tyler Baicar  wrote:
> Currently when a RAS error is reported it is not timestamped.
> The ACPI 6.1 spec adds the timestamp field to the generic error
> data entry v3 structure. The timestamp of when the firmware
> generated the error is now being reported.
>
> Signed-off-by: Tyler Baicar 
> Signed-off-by: Jonathan (Zhixiong) Zhang 
> Signed-off-by: Richard Ruigrok 
> Signed-off-by: Naveen Kaje 
> Reviewed-by: James Morse 

Reviewed-by: Ard Biesheuvel 

> ---
>  drivers/acpi/apei/ghes.c|  9 ---
>  drivers/firmware/efi/cper.c | 63 
> +++--
>  include/acpi/ghes.h | 22 
>  3 files changed, 77 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 5e1ec41..b25e7cf 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -420,7 +420,8 @@ static void ghes_handle_memory_failure(struct 
> acpi_hest_generic_data *gdata, int
> int flags = -1;
> int sec_sev = ghes_severity(gdata->error_severity);
> struct cper_sec_mem_err *mem_err;
> -   mem_err = (struct cper_sec_mem_err *)(gdata + 1);
> +
> +   mem_err = acpi_hest_generic_data_payload(gdata);
>
> if (!(mem_err->validation_bits & CPER_MEM_VALID_PA))
> return;
> @@ -457,7 +458,8 @@ static void ghes_do_proc(struct ghes *ghes,
> if (!uuid_le_cmp(*(uuid_le *)gdata->section_type,
>  CPER_SEC_PLATFORM_MEM)) {
> struct cper_sec_mem_err *mem_err;
> -   mem_err = (struct cper_sec_mem_err *)(gdata+1);
> +
> +   mem_err = acpi_hest_generic_data_payload(gdata);
> ghes_edac_report_mem_error(ghes, sev, mem_err);
>
> arch_apei_report_mem_error(sev, mem_err);
> @@ -467,7 +469,8 @@ static void ghes_do_proc(struct ghes *ghes,
> else if (!uuid_le_cmp(*(uuid_le *)gdata->section_type,
>   CPER_SEC_PCIE)) {
> struct cper_sec_pcie *pcie_err;
> -   pcie_err = (struct cper_sec_pcie *)(gdata+1);
> +
> +   pcie_err = acpi_hest_generic_data_payload(gdata);
> if (sev == GHES_SEV_RECOVERABLE &&
> sec_sev == GHES_SEV_RECOVERABLE &&
> pcie_err->validation_bits & 
> CPER_PCIE_VALID_DEVICE_ID &&
> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index d425374..8fa4e23 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -32,6 +32,9 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
> +#include 
>
>  #define INDENT_SP  " "
>
> @@ -386,13 +389,37 @@ static void cper_print_pcie(const char *pfx, const 
> struct cper_sec_pcie *pcie,
> pfx, pcie->bridge.secondary_status, pcie->bridge.control);
>  }
>
> +static void cper_estatus_print_section_v300(const char *pfx,
> +   const struct acpi_hest_generic_data_v300 *gdata)
> +{
> +   __u8 hour, min, sec, day, mon, year, century, *timestamp;
> +
> +   if (gdata->validation_bits & ACPI_HEST_GEN_VALID_TIMESTAMP) {
> +   timestamp = (__u8 *)&(gdata->time_stamp);
> +   sec = bcd2bin(timestamp[0]);
> +   min = bcd2bin(timestamp[1]);
> +   hour = bcd2bin(timestamp[2]);
> +   day = bcd2bin(timestamp[4]);
> +   mon = bcd2bin(timestamp[5]);
> +   year = bcd2bin(timestamp[6]);
> +   century = bcd2bin(timestamp[7]);
> +   printk("%stime: %7s %02d%02d-%02d-%02d %02d:%02d:%02d\n", pfx,
> +   0x01 & *(timestamp + 3) ? "precise" : "", century,
> +   year, mon, day, hour, min, sec);
> +   }
> +}
> +
>  static void cper_estatus_print_section(
> -   const char *pfx, const struct acpi_hest_generic_data *gdata, int 
> sec_no)
> +   const char *pfx, struct acpi_hest_generic_data *gdata, int sec_no)
>  {
> uuid_le *sec_type = (uuid_le *)gdata->section_type;
> __u16 severity;
> char newpfx[64];
>
> +   if (acpi_hest_generic_data_version(gdata) >= 3)
> +   cper_estatus_print_section_v300(pfx,
> +   (const struct acpi_hest_generic_data_v300 *)gdata);
> +
> severity = gdata->error_severity;
> printk("%s""Error %d, type: %s\n", pfx, sec_no,
>cper_severity_str(severity));
> @@ -403,14 +430,18 @@ static void cper_estatus_print_section(
>
> snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP);
> if (!uuid_le_cmp(*sec_type, CPER_SEC_PROC_GENERIC)) {
> -   struct

Re: [PATCH] KVM: race-free exit from KVM_RUN without POSIX signals

2017-02-16 Thread Radim Krčmář 
2017-02-15 15:43+0100, Paolo Bonzini:
> The purpose of the KVM_SET_SIGNAL_MASK API is to let userspace "kick"
> a VCPU out of KVM_RUN through a POSIX signal.  A signal is attached
> to a dummy signal handler; by blocking the signal outside KVM_RUN and
> unblocking it inside, this possible race is closed:
> 
>   VCPU thread service thread
>--
> check flag
>   set flag
>   raise signal
> (signal handler does nothing)
> KVM_RUN
> 
> However, one issue with KVM_SET_SIGNAL_MASK is that it has to take
> tsk->sighand->siglock on every KVM_RUN.  This lock is often on a
> remote NUMA node, because it is on the node of a thread's creator.
> Taking this lock can be very expensive if there are many userspace
> exits (as is the case for SMP Windows VMs without Hyper-V reference
> time counter).
> 
> As an alternative, we can put the flag directly in kvm_run so that
> KVM can see it:
> 
>   VCPU thread service thread
>--
>   raise signal
> signal handler
>   set run->immediate_exit
> KVM_RUN
>   check run->immediate_exit
> 
> Signed-off-by: Paolo Bonzini 
> ---

The old immediate exit with signal did more work, but none of it should
affect user-space, so it looks like another minor optimization,

Reviewed-by: Radim Krčmář 

>   change from RFC:
>   - implement in each architecture to ensure MMIO is completed
> [Radim]
>   - do not clear the flag [David Hildenbrand, offlist]
> 
>  Documentation/virtual/kvm/api.txt | 13 -
>  arch/arm/kvm/arm.c|  4 
>  arch/mips/kvm/mips.c  |  7 ++-
>  arch/powerpc/kvm/powerpc.c|  6 +-
>  arch/s390/kvm/kvm-s390.c  |  4 
>  arch/x86/kvm/x86.c|  6 +-
>  include/uapi/linux/kvm.h  |  4 +++-
>  7 files changed, 39 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/virtual/kvm/api.txt 
> b/Documentation/virtual/kvm/api.txt
> index e4f2cdcf78eb..925b1b6be073 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -3389,7 +3389,18 @@ struct kvm_run {
>  Request that KVM_RUN return when it becomes possible to inject external
>  interrupts into the guest.  Useful in conjunction with KVM_INTERRUPT.
>  
> - __u8 padding1[7];
> + __u8 immediate_exit;
> +
> +This field is polled once when KVM_RUN starts; if non-zero, KVM_RUN
> +exits immediately, returning -EINTR.  In the common scenario where a
> +signal is used to "kick" a VCPU out of KVM_RUN, this field can be used
> +to avoid usage of KVM_SET_SIGNAL_MASK, which has worse scalability.
> +Rather than blocking the signal outside KVM_RUN, userspace can set up
> +a signal handler that sets run->immediate_exit to a non-zero value.
> +
> +This field is ignored if KVM_CAP_IMMEDIATE_EXIT is not available.
> +
> + __u8 padding1[6];
>  
>   /* out */
>   __u32 exit_reason;
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 21c493a9e5c9..c9a2103faeb9 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -206,6 +206,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long 
> ext)
>   case KVM_CAP_ARM_PSCI_0_2:
>   case KVM_CAP_READONLY_MEM:
>   case KVM_CAP_MP_STATE:
> + case KVM_CAP_IMMEDIATE_EXIT:
>   r = 1;
>   break;
>   case KVM_CAP_COALESCED_MMIO:
> @@ -604,6 +605,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct 
> kvm_run *run)
>   return ret;
>   }
>  
> + if (run->immediate_exit)
> + return -EINTR;
> +
>   if (vcpu->sigset_active)
>   sigprocmask(SIG_SETMASK, >sigset, );
>  
> diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c
> index 31ee5ee0010b..ed81e5ac1426 100644
> --- a/arch/mips/kvm/mips.c
> +++ b/arch/mips/kvm/mips.c
> @@ -397,7 +397,7 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu 
> *vcpu,
>  
>  int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  {
> - int r = 0;
> + int r = -EINTR;
>   sigset_t sigsaved;
>  
>   if (vcpu->sigset_active)
> @@ -409,6 +409,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct 
> kvm_run *run)
>   vcpu->mmio_needed = 0;
>   }
>  
> + if (run->immediate_exit)
> + goto out;
> +
>   lose_fpu(1);
>  
>   local_irq_disable();
> @@ -429,6 +432,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct 
> kvm_run *run)
>   guest_exit_irqoff();
>   local_irq_enable();
>  
> +out:
>   if (vcpu->sigset_active)
>   sigprocmask(SIG_SETMASK, , NULL);
>  
> @@ -1021,6

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

2017-02-16 Thread Auger Eric 
Hi,
On 17/01/2017 11:20, Marc Zyngier wrote:
> Most ITS commands do operate on a collection object, and require
> a SYNC command to be performed on that collection in order to
> guarantee the execution of the first command.
> 
> With GICv4 ITS, another set of commands perform similar operations
> on a VPE object, and a VSYNC operations must be executed to guarantee
> their execution.
> 
> Given the similarities (post a command, perform a synchronization
> operation on a sync object), it makes sense to reuse the same
> mechanism for both class of commands.
> 
> Let's start with turning its_send_single_command into a huge macro
> that performs the bulk of the work, and a set of helpers that
> make this macro useeable for the GICv3 ITS commands.
s/useeable/usable

Eric
> 
> Signed-off-by: Marc Zyngier 
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 82 
> ++--
>  1 file changed, 45 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
> b/drivers/irqchip/irq-gic-v3-its.c
> index 99f6130..520b764 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -484,43 +484,51 @@ static void its_wait_for_range_completion(struct 
> its_node *its,
>   }
>  }
>  
> -static void its_send_single_command(struct its_node *its,
> - its_cmd_builder_t builder,
> - struct its_cmd_desc *desc)
> -{
> - struct its_cmd_block *cmd, *sync_cmd, *next_cmd;
> - struct its_collection *sync_col;
> - unsigned long flags;
> -
> - raw_spin_lock_irqsave(>lock, flags);
> -
> - cmd = its_allocate_entry(its);
> - if (!cmd) { /* We're soo screewed... */
> - pr_err_ratelimited("ITS can't allocate, dropping command\n");
> - raw_spin_unlock_irqrestore(>lock, flags);
> - return;
> - }
> - sync_col = builder(cmd, desc);
> - its_flush_cmd(its, cmd);
> -
> - if (sync_col) {
> - sync_cmd = its_allocate_entry(its);
> - if (!sync_cmd) {
> - pr_err_ratelimited("ITS can't SYNC, skipping\n");
> - goto post;
> - }
> - its_encode_cmd(sync_cmd, GITS_CMD_SYNC);
> - its_encode_target(sync_cmd, sync_col->target_address);
> - its_fixup_cmd(sync_cmd);
> - its_flush_cmd(its, sync_cmd);
> - }
> -
> -post:
> - next_cmd = its_post_commands(its);
> - raw_spin_unlock_irqrestore(>lock, flags);
> -
> - its_wait_for_range_completion(its, cmd, next_cmd);
> -}
> +#define __its_send_single_cmd(name, buildtype, synctype, buildfn)\
> +void name(struct its_node *its,  
> \
> +   buildtype builder,\
> +   struct its_cmd_desc *desc)\
> +{\
> + struct its_cmd_block *cmd, *sync_cmd, *next_cmd;\
> + synctype *sync_obj; \
> + unsigned long flags;\
> + \
> + raw_spin_lock_irqsave(>lock, flags);   \
> + \
> + cmd = its_allocate_entry(its);  \
> + if (!cmd) { /* We're soo screewed... */ \
> + raw_spin_unlock_irqrestore(>lock, flags);  \
> + return; \
> + }   \
> + sync_obj = builder(cmd, desc);  \
> + its_flush_cmd(its, cmd);\
> + \
> + if (sync_obj) { \
> + sync_cmd = its_allocate_entry(its); \
> + if (!sync_cmd)  \
> + goto post;  \
> + \
> + buildfn(sync_cmd, sync_obj);\
> + its_fixup_cmd(sync_cmd);\
> + its_flush_cmd(its, sync_cmd);   \
> + }   \
> + \
> +post:
> \
> + next_cmd = its_post_commands(its);  \
> + raw_spin_unlock_irqrestore(>lock,

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

2017-02-16 Thread Auger Eric 
Marc,

On 17/01/2017 11:20, 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: Eric Auger 

Eric

> ---
>  drivers/irqchip/irq-gic-v3-its.c | 78 
> 
>  1 file changed, 78 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
> b/drivers/irqchip/irq-gic-v3-its.c
> index 520b764..b28fb19 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -163,6 +163,11 @@ struct its_cmd_desc {
>   struct {
>   struct its_device *dev;
>   u32 event_id;
> + } its_clear_cmd;
> +
> + struct {
> + struct its_device *dev;
> + u32 event_id;
>   } its_int_cmd;
>  
>   struct {
> @@ -376,6 +381,40 @@ static struct its_collection *its_build_inv_cmd(struct 
> its_cmd_block *cmd,
>   return col;
>  }
>  
> +static struct its_collection *its_build_int_cmd(struct its_cmd_block *cmd,
> + struct its_cmd_desc *desc)
> +{
> + struct its_collection *col;
> +
> + col = dev_event_to_col(desc->its_int_cmd.dev,
> +desc->its_int_cmd.event_id);
> +
> + its_encode_cmd(cmd, GITS_CMD_INT);
> + its_encode_devid(cmd, desc->its_int_cmd.dev->device_id);
> + its_encode_event_id(cmd, desc->its_int_cmd.event_id);
> +
> + its_fixup_cmd(cmd);
> +
> + return col;
> +}
> +
> +static struct its_collection *its_build_clear_cmd(struct its_cmd_block *cmd,
> +   struct its_cmd_desc *desc)
> +{
> + struct its_collection *col;
> +
> + col = dev_event_to_col(desc->its_clear_cmd.dev,
> +desc->its_clear_cmd.event_id);
> +
> + its_encode_cmd(cmd, GITS_CMD_CLEAR);
> + its_encode_devid(cmd, desc->its_clear_cmd.dev->device_id);
> + its_encode_event_id(cmd, desc->its_clear_cmd.event_id);
> +
> + its_fixup_cmd(cmd);
> +
> + return col;
> +}
> +
>  static struct its_collection *its_build_invall_cmd(struct its_cmd_block *cmd,
>  struct its_cmd_desc *desc)
>  {
> @@ -530,6 +569,26 @@ static void its_build_sync_cmd(struct its_cmd_block 
> *sync_cmd,
>  static __its_send_single_cmd(its_send_single_command, its_cmd_builder_t,
>struct its_collection, its_build_sync_cmd)
>  
> +static void its_send_int(struct its_device *dev, u32 event_id)
> +{
> + struct its_cmd_desc desc;
> +
> + desc.its_int_cmd.dev = dev;
> + desc.its_int_cmd.event_id = event_id;
> +
> + its_send_single_command(dev->its, its_build_int_cmd, );
> +}
> +
> +static void its_send_clear(struct its_device *dev, u32 event_id)
> +{
> + struct its_cmd_desc desc;
> +
> + desc.its_clear_cmd.dev = dev;
> + desc.its_clear_cmd.event_id = event_id;
> +
> + its_send_single_command(dev->its, its_build_clear_cmd, );
> +}
> +
>  static void its_send_inv(struct its_device *dev, u32 event_id)
>  {
>   struct its_cmd_desc desc;
> @@ -693,6 +752,24 @@ static void its_irq_compose_msi_msg(struct irq_data *d, 
> struct msi_msg *msg)
>   iommu_dma_map_msi_msg(d->irq, msg);
>  }
>  
> +static int its_irq_set_irqchip_state(struct irq_data *d,
> +  enum irqchip_irq_state which,
> +  bool state)
> +{
> + struct its_device *its_dev = irq_data_get_irq_chip_data(d);
> + u32 event = its_get_event_id(d);
> +
> + if (which != IRQCHIP_STATE_PENDING)
> + return -EINVAL;
> +
> + if (state)
> + its_send_int(its_dev, event);
> + else
> + its_send_clear(its_dev, event);
> +
> + return 0;
> +}
> +
>  static struct irq_chip its_irq_chip = {
>   .name   = "ITS",
>   .irq_mask   = its_mask_irq,
> @@ -700,6 +777,7 @@ static struct irq_chip its_irq_chip = {
>   .irq_eoi= irq_chip_eoi_parent,
>   .irq_set_affinity   = its_set_affinity,
>   .irq_compose_msi_msg= its_irq_compose_msi_msg,
> + .irq_set_irqchip_state  = its_irq_set_irqchip_state,
>  };
>  
>  /*
> 
___
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

2017-02-16 Thread Auger Eric 
Hi,

On 13/02/2017 11:00, Thomas Gleixner wrote:
> 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

I understand this covers the case where the ITS_number field is RO. In
such a case the its_number has changed just above compared to the first
find_first_zero_bit?

Besides

Reviewed-by: Eric Auger 

Thanks

Eric


> 
> Thanks,
> 
>   tglx
> 
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
___
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

2017-02-16 Thread Auger Eric 
Hi Marc,

On 17/01/2017 11:20, 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: Eric Auger 

Eric
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 27 +++
>  1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
> b/drivers/irqchip/irq-gic-v3-its.c
> index 49b681e..2ca27f6 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -49,6 +49,21 @@
>  #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING  (1 << 0)
>  
>  /*
> + * We allocate 64kB for PROPBASE. That gives us at most 64K LPIs to
> + * deal with (one configuration byte per interrupt). PENDBASE has to
> + * be 64kB aligned (one bit per LPI, plus 8192 bits for SPI/PPI/SGI).
> + */
> +#define LPI_PROPBASE_SZ  SZ_64K
> +#define LPI_PENDBASE_SZ  (LPI_PROPBASE_SZ / 8 + SZ_1K)
> +
> +/*
> + * This is how many bits of ID we need, including the useless ones.
> + */
> +#define LPI_NRBITS   ilog2(LPI_PROPBASE_SZ + SZ_8K)
> +
> +#define LPI_PROP_DEFAULT_PRIO0xa0
> +
> +/*
>   * Collection structure - just an ID, and a redistributor address to
>   * ping. We use one per CPU as a bag of interrupts assigned to this
>   * CPU.
> @@ -779,20 +794,8 @@ static void its_lpi_free(struct event_lpi_map *map)
>   kfree(map->col_map);
>  }
>  
> -/*
> - * We allocate 64kB for PROPBASE. That gives us at most 64K LPIs to
> - * deal with (one configuration byte per interrupt). PENDBASE has to
> - * be 64kB aligned (one bit per LPI, plus 8192 bits for SPI/PPI/SGI).
> - */
> -#define LPI_PROPBASE_SZ  SZ_64K
> -#define LPI_PENDBASE_SZ  (LPI_PROPBASE_SZ / 8 + SZ_1K)
>  
> -/*
> - * This is how many bits of ID we need, including the useless ones.
> - */
> -#define LPI_NRBITS   ilog2(LPI_PROPBASE_SZ + SZ_8K)
>  
> -#define LPI_PROP_DEFAULT_PRIO0xa0
>  
>  static int __init its_alloc_lpi_tables(void)
>  {
> 
___
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

2017-02-16 Thread Auger Eric 
Hi,

On 17/01/2017 11:20, Marc Zyngier wrote:
> Add helper functions that probe for VLPI and DirectLPI properties.
> 
> Signed-off-by: Marc Zyngier 
Besides the returned value previous questions,

Reviewed-by: Eric Auger 

Eric


> ---
>  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",
> + !gic_data.rdists.has_vlpis ? "no " : "",
> + !gic_data.rdists.has_direct_lpi ? "no " : "");
> +}
> +
>  static void gic_cpu_sys_reg_init(void)
>  {
>   /*
> @@ -975,6 +993,8 @@ static int __init gic_init_bases(void __iomem *dist_base,
>   gic_data.domain = irq_domain_create_tree(handle, _irq_domain_ops,
>_data);
>   gic_data.rdists.rdist = alloc_percpu(typeof(*gic_data.rdists.rdist));
> + gic_data.rdists.has_vlpis = true;
> + gic_data.rdists.has_direct_lpi = true;
>  
>   if (WARN_ON(!gic_data.domain) || WARN_ON(!gic_data.rdists.rdist)) {
>   err = -ENOMEM;
> @@ -983,6 +1003,8 @@ static int __init gic_init_bases(void __iomem *dist_base,
>  
>   set_handle_irq(gic_handle_irq);
>  
> + gic_update_vlpi_properties();
> +
>   if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) && gic_dist_supports_lpis())
>   its_init(handle, _data.rdists, gic_data.domain);
>  
> diff --git a/include/linux/irqchip/arm-gic-v3.h 
> b/include/linux/irqchip/arm-gic-v3.h
> index e808f8a..b162bfa 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -200,6 +200,7 @@
>  
>  #define GICR_TYPER_PLPIS (1U << 0)
>  #define GICR_TYPER_VLPIS (1U << 1)
> +#define GICR_TYPER_DirectLPIS(1U << 3)
>  #define GICR_TYPER_LAST  (1U << 4)
>  
>  #define GIC_V3_REDIST_SIZE   0x2
> @@ -427,6 +428,8 @@ struct rdists {
>   struct page *prop_page;
>   int id_bits;
>   u64 flags;
> + boolhas_vlpis;
> + boolhas_direct_lpi;
>  };
>  
>  struct irq_domain;
> 
___
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

2017-02-16 Thread Auger Eric 


On 17/01/2017 11:20, Marc Zyngier wrote:
> The way we encode the various ITS command fields is both tedious
> and error prone. Let's introduce a helper function that performs
> the encoding, and convert the existing encoders to use that
> helper.
> 
> Signed-off-by: Marc Zyngier 
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 34 --
>  1 file changed, 16 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
> b/drivers/irqchip/irq-gic-v3-its.c
> index 69b040f..49b681e 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -193,58 +193,56 @@ struct its_cmd_block {
>  typedef struct its_collection *(*its_cmd_builder_t)(struct its_cmd_block *,
>   struct its_cmd_desc *);
>  
> +static void its_mask_encode(u64 *raw_cmd, u64 val, int h, int l)
> +{
> + u64 mask = GENMASK_ULL(h, l);
> + *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);
>  }
>  
>  static void its_encode_devid(struct its_cmd_block *cmd, u32 devid)
>  {
> - cmd->raw_cmd[0] &= BIT_ULL(32) - 1;
> - cmd->raw_cmd[0] |= ((u64)devid) << 32;
> + its_mask_encode(>raw_cmd[0], devid, 63, 32);
>  }
>  
>  static void its_encode_event_id(struct its_cmd_block *cmd, u32 id)
>  {
> - cmd->raw_cmd[1] &= ~0xULL;
> - cmd->raw_cmd[1] |= id;
> + its_mask_encode(>raw_cmd[1], id, 31, 0);
>  }
>  
>  static void its_encode_phys_id(struct its_cmd_block *cmd, u32 phys_id)
>  {
> - cmd->raw_cmd[1] &= 0xULL;
> - cmd->raw_cmd[1] |= ((u64)phys_id) << 32;
> + its_mask_encode(>raw_cmd[1], phys_id, 63, 32);
>  }
>  
>  static void its_encode_size(struct its_cmd_block *cmd, u8 size)
>  {
> - cmd->raw_cmd[1] &= ~0x1fULL;
> - cmd->raw_cmd[1] |= size & 0x1f;
> + its_mask_encode(>raw_cmd[1], size, 4, 0);
>  }
>  
>  static void its_encode_itt(struct its_cmd_block *cmd, u64 itt_addr)
>  {
> - cmd->raw_cmd[2] &= ~0xULL;
> - cmd->raw_cmd[2] |= itt_addr & 0xff00ULL;
> + its_mask_encode(>raw_cmd[2], itt_addr >> 8, 50, 8);
>  }
>  
>  static void its_encode_valid(struct its_cmd_block *cmd, int valid)
>  {
> - cmd->raw_cmd[2] &= ~(1ULL << 63);
> - cmd->raw_cmd[2] |= ((u64)!!valid) << 63;
> + its_mask_encode(>raw_cmd[2], !!valid, 63, 63);
>  }
>  
>  static void its_encode_target(struct its_cmd_block *cmd, u64 target_addr)
>  {
> - cmd->raw_cmd[2] &= ~(0xULL << 16);
> - cmd->raw_cmd[2] |= (target_addr & (0xULL << 16));
> + its_mask_encode(>raw_cmd[2], target_addr >> 16, 50, 16);
Theoretically isn't it 16:51? In the previous code this was encoded as
16-47 I think.

Besides
Reviewed-by: Eric Auger 


Thanks

Eric
>  }
>  
>  static void its_encode_collection(struct its_cmd_block *cmd, u16 col)
>  {
> - cmd->raw_cmd[2] &= ~0xULL;
> - cmd->raw_cmd[2] |= col;
> + its_mask_encode(>raw_cmd[2], col, 15, 0);
>  }
>  
>  static inline void its_fixup_cmd(struct its_cmd_block *cmd)
> 
___
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

2017-02-16 Thread Auger Eric 
Hi Marc,

On 17/01/2017 11:20, 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 *))
>  {
> - unsigned long mpidr = cpu_logical_map(smp_processor_id());
> - u64 typer;
> - u32 aff;
> + int ret = 0;
in case
if (reg != GIC_PIDR2_ARCH_GICv3 &&
reg != GIC_PIDR2_ARCH_GICv4) is true you now return 0. Don' 
you
want to return -ENODEV as before?


>   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));
>   }
Assuming you don't find the TYPER that matches the cpu (I don't know if
it is possible), last fn() call will return 1 and the function will
return 0. Before it returned -ENODEV I think.

Thanks

Eric
>  
> + if (ret == -1)
> + ret = -ENODEV;
> +
> + 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)
> + 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: [PATCH] KVM: arm64: VGIC: fix command handling while ITS being disabled

2017-02-16 Thread Auger Eric 
Hi Andre,

On 16/02/2017 11:41, Andre Przywara wrote:
> The ITS spec says that ITS commands are only processed when the ITS
> is enabled (section 8.19.4, Enabled, bit[0]). Our emulation was not taking
> this into account.
> Fix this by checking the enabled state before handling CWRITER writes.
nit: you actually check the ITS state before executing commands.
> 
> On the other hand that means that CWRITER could advance while the ITS
> is disabled, and enabling it would need those commands to be processed.
> Fix this case as well by refactoring actual command processing and
> calling this from both the GITS_CWRITER and GITS_CTLR handlers.
> 
> Signed-off-by: Andre Przywara 
Reviewed-by: Eric Auger 

Thanks

Eric
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 109 
> ++-
>  1 file changed, 65 insertions(+), 44 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 8c2b3cd..ff86106 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -360,29 +360,6 @@ static int its_sync_lpi_pending_table(struct kvm_vcpu 
> *vcpu)
>   return ret;
>  }
>  
> -static unsigned long vgic_mmio_read_its_ctlr(struct kvm *vcpu,
> -  struct vgic_its *its,
> -  gpa_t addr, unsigned int len)
> -{
> - u32 reg = 0;
> -
> - mutex_lock(>cmd_lock);
> - if (its->creadr == its->cwriter)
> - reg |= GITS_CTLR_QUIESCENT;
> - if (its->enabled)
> - reg |= GITS_CTLR_ENABLE;
> - mutex_unlock(>cmd_lock);
> -
> - return reg;
> -}
> -
> -static void vgic_mmio_write_its_ctlr(struct kvm *kvm, struct vgic_its *its,
> -  gpa_t addr, unsigned int len,
> -  unsigned long val)
> -{
> - its->enabled = !!(val & GITS_CTLR_ENABLE);
> -}
> -
>  static unsigned long vgic_mmio_read_its_typer(struct kvm *kvm,
> struct vgic_its *its,
> gpa_t addr, unsigned int len)
> @@ -1161,33 +1138,16 @@ static void vgic_mmio_write_its_cbaser(struct kvm 
> *kvm, struct vgic_its *its,
>  #define ITS_CMD_SIZE 32
>  #define ITS_CMD_OFFSET(reg)  ((reg) & GENMASK(19, 5))
>  
> -/*
> - * By writing to CWRITER the guest announces new commands to be processed.
> - * To avoid any races in the first place, we take the its_cmd lock, which
> - * protects our ring buffer variables, so that there is only one user
> - * per ITS handling commands at a given time.
> - */
> -static void vgic_mmio_write_its_cwriter(struct kvm *kvm, struct vgic_its 
> *its,
> - gpa_t addr, unsigned int len,
> - unsigned long val)
> +/* Must be called with the cmd_lock held. */
> +static void vgic_its_process_commands(struct kvm *kvm, struct vgic_its *its)
>  {
>   gpa_t cbaser;
>   u64 cmd_buf[4];
> - u32 reg;
>  
> - if (!its)
> - return;
> -
> - mutex_lock(>cmd_lock);
> -
> - reg = update_64bit_reg(its->cwriter, addr & 7, len, val);
> - reg = ITS_CMD_OFFSET(reg);
> - if (reg >= ITS_CMD_BUFFER_SIZE(its->cbaser)) {
> - mutex_unlock(>cmd_lock);
> + /* Commands are only processed when the ITS is enabled. */
> + if (!its->enabled)
>   return;
> - }
>  
> - its->cwriter = reg;
>   cbaser = CBASER_ADDRESS(its->cbaser);
>  
>   while (its->cwriter != its->creadr) {
> @@ -1207,6 +1167,34 @@ static void vgic_mmio_write_its_cwriter(struct kvm 
> *kvm, struct vgic_its *its,
>   if (its->creadr == ITS_CMD_BUFFER_SIZE(its->cbaser))
>   its->creadr = 0;
>   }
> +}
> +
> +/*
> + * By writing to CWRITER the guest announces new commands to be processed.
> + * To avoid any races in the first place, we take the its_cmd lock, which
> + * protects our ring buffer variables, so that there is only one user
> + * per ITS handling commands at a given time.
> + */
> +static void vgic_mmio_write_its_cwriter(struct kvm *kvm, struct vgic_its 
> *its,
> + gpa_t addr, unsigned int len,
> + unsigned long val)
> +{
> + u64 reg;
> +
> + if (!its)
> + return;
> +
> + mutex_lock(>cmd_lock);
> +
> + reg = update_64bit_reg(its->cwriter, addr & 7, len, val);
> + reg = ITS_CMD_OFFSET(reg);
> + if (reg >= ITS_CMD_BUFFER_SIZE(its->cbaser)) {
> + mutex_unlock(>cmd_lock);
> + return;
> + }
> + its->cwriter = reg;
> +
> + vgic_its_process_commands(kvm, its);
>  
>   mutex_unlock(>cmd_lock);
>  }
> @@ -1287,6 +1275,39 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
>   *regptr = reg;
>  }
>  
> +static unsigned long vgic_mmio_read_its_ctlr(struct kvm

[PATCH] KVM: arm64: VGIC: fix command handling while ITS being disabled

2017-02-16 Thread Andre Przywara 
The ITS spec says that ITS commands are only processed when the ITS
is enabled (section 8.19.4, Enabled, bit[0]). Our emulation was not taking
this into account.
Fix this by checking the enabled state before handling CWRITER writes.

On the other hand that means that CWRITER could advance while the ITS
is disabled, and enabling it would need those commands to be processed.
Fix this case as well by refactoring actual command processing and
calling this from both the GITS_CWRITER and GITS_CTLR handlers.

Signed-off-by: Andre Przywara 
---
 virt/kvm/arm/vgic/vgic-its.c | 109 ++-
 1 file changed, 65 insertions(+), 44 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 8c2b3cd..ff86106 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -360,29 +360,6 @@ static int its_sync_lpi_pending_table(struct kvm_vcpu 
*vcpu)
return ret;
 }
 
-static unsigned long vgic_mmio_read_its_ctlr(struct kvm *vcpu,
-struct vgic_its *its,
-gpa_t addr, unsigned int len)
-{
-   u32 reg = 0;
-
-   mutex_lock(>cmd_lock);
-   if (its->creadr == its->cwriter)
-   reg |= GITS_CTLR_QUIESCENT;
-   if (its->enabled)
-   reg |= GITS_CTLR_ENABLE;
-   mutex_unlock(>cmd_lock);
-
-   return reg;
-}
-
-static void vgic_mmio_write_its_ctlr(struct kvm *kvm, struct vgic_its *its,
-gpa_t addr, unsigned int len,
-unsigned long val)
-{
-   its->enabled = !!(val & GITS_CTLR_ENABLE);
-}
-
 static unsigned long vgic_mmio_read_its_typer(struct kvm *kvm,
  struct vgic_its *its,
  gpa_t addr, unsigned int len)
@@ -1161,33 +1138,16 @@ static void vgic_mmio_write_its_cbaser(struct kvm *kvm, 
struct vgic_its *its,
 #define ITS_CMD_SIZE   32
 #define ITS_CMD_OFFSET(reg)((reg) & GENMASK(19, 5))
 
-/*
- * By writing to CWRITER the guest announces new commands to be processed.
- * To avoid any races in the first place, we take the its_cmd lock, which
- * protects our ring buffer variables, so that there is only one user
- * per ITS handling commands at a given time.
- */
-static void vgic_mmio_write_its_cwriter(struct kvm *kvm, struct vgic_its *its,
-   gpa_t addr, unsigned int len,
-   unsigned long val)
+/* Must be called with the cmd_lock held. */
+static void vgic_its_process_commands(struct kvm *kvm, struct vgic_its *its)
 {
gpa_t cbaser;
u64 cmd_buf[4];
-   u32 reg;
 
-   if (!its)
-   return;
-
-   mutex_lock(>cmd_lock);
-
-   reg = update_64bit_reg(its->cwriter, addr & 7, len, val);
-   reg = ITS_CMD_OFFSET(reg);
-   if (reg >= ITS_CMD_BUFFER_SIZE(its->cbaser)) {
-   mutex_unlock(>cmd_lock);
+   /* Commands are only processed when the ITS is enabled. */
+   if (!its->enabled)
return;
-   }
 
-   its->cwriter = reg;
cbaser = CBASER_ADDRESS(its->cbaser);
 
while (its->cwriter != its->creadr) {
@@ -1207,6 +1167,34 @@ static void vgic_mmio_write_its_cwriter(struct kvm *kvm, 
struct vgic_its *its,
if (its->creadr == ITS_CMD_BUFFER_SIZE(its->cbaser))
its->creadr = 0;
}
+}
+
+/*
+ * By writing to CWRITER the guest announces new commands to be processed.
+ * To avoid any races in the first place, we take the its_cmd lock, which
+ * protects our ring buffer variables, so that there is only one user
+ * per ITS handling commands at a given time.
+ */
+static void vgic_mmio_write_its_cwriter(struct kvm *kvm, struct vgic_its *its,
+   gpa_t addr, unsigned int len,
+   unsigned long val)
+{
+   u64 reg;
+
+   if (!its)
+   return;
+
+   mutex_lock(>cmd_lock);
+
+   reg = update_64bit_reg(its->cwriter, addr & 7, len, val);
+   reg = ITS_CMD_OFFSET(reg);
+   if (reg >= ITS_CMD_BUFFER_SIZE(its->cbaser)) {
+   mutex_unlock(>cmd_lock);
+   return;
+   }
+   its->cwriter = reg;
+
+   vgic_its_process_commands(kvm, its);
 
mutex_unlock(>cmd_lock);
 }
@@ -1287,6 +1275,39 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
*regptr = reg;
 }
 
+static unsigned long vgic_mmio_read_its_ctlr(struct kvm *vcpu,
+struct vgic_its *its,
+gpa_t addr, unsigned int len)
+{
+   u32 reg = 0;
+
+   mutex_lock(>cmd_lock);
+   if (its->creadr == its->cwriter)
+   reg |= GITS_CTLR_QUIESCENT;
+   if (its->enabled)
+   reg