Re: [PATCH] updated: arm64: KVM: vgic: deal with GIC sub-page alignment

[Itaru Kitayama]

Forgot to mention that all tests/attempts were done on an ACPI-based
system.

On 7/9/16 9:27 AM, Itaru Kitayama wrote:

Marc,

I'm at the latest commit, fa9301d in the gicv-align branch at the
moment, with that an unmodified QEMU guest boots fine on a 64Kb page
granular host, thanks to the trapping you introduced.
(I wanted to get some performance statistics of the new feature with
perf, but on arm64 most of them are unsupported)

However, if I rebuild the kernel using defconfig with only the kernel
page size changed to 16Kb it does not boot at least on Overdrive 3000.


On 7/1/16 6:20 PM, Marc Zyngier wrote:

On 01/07/16 09:59, Itaru Kitayama wrote:

Marc,
That's good news. Can I assume you'd keep the
KVM_DEV_ARM_VGIC_GRP_ADDR_OFFSET attribute, or would you introduce new
ones? I ask because either way we need the userland support, be it QEMU
or kvmtool. I'm carrying the small QEMU patch at this moment.


You can look at the kvm-arm64/gicv-align branch in my tree, which
contains some of the stuff. At the moment, the attribute is named
KVM_DEV_ARM_VGIC_GRP_GICC_OFFSET, but I don't mind changing the name.

Thanks,

M.

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

Re: [PATCH] updated: arm64: KVM: vgic: deal with GIC sub-page alignment

[Itaru Kitayama]

Marc,

I'm at the latest commit, fa9301d in the gicv-align branch at the 
moment, with that an unmodified QEMU guest boots fine on a 64Kb page 
granular host, thanks to the trapping you introduced.
(I wanted to get some performance statistics of the new feature with 
perf, but on arm64 most of them are unsupported)


However, if I rebuild the kernel using defconfig with only the kernel 
page size changed to 16Kb it does not boot at least on Overdrive 3000.



On 7/1/16 6:20 PM, Marc Zyngier wrote:

On 01/07/16 09:59, Itaru Kitayama wrote:

Marc,
That's good news. Can I assume you'd keep the
KVM_DEV_ARM_VGIC_GRP_ADDR_OFFSET attribute, or would you introduce new
ones? I ask because either way we need the userland support, be it QEMU
or kvmtool. I'm carrying the small QEMU patch at this moment.


You can look at the kvm-arm64/gicv-align branch in my tree, which
contains some of the stuff. At the moment, the attribute is named
KVM_DEV_ARM_VGIC_GRP_GICC_OFFSET, but I don't mind changing the name.

Thanks,

M.

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

Re: [RFC v6 4/6] KVM: arm/arm64: enable irqchip routing

[Radim Krčmář]

2016-07-08 10:52+0200, Andrew Jones:
> On Fri, Jul 08, 2016 at 10:16:53AM +0200, Auger Eric wrote:
>> On 07/07/2016 19:20, Andrew Jones wrote:
>> > On Wed, Jul 06, 2016 at 10:47:53AM +0200, Eric Auger wrote:
>> >> diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c
>> >> @@ -29,7 +29,9 @@
>> >>  #include 
>> >>  #include 
>> >>  #include 
>> >> +#if !defined(CONFIG_ARM) && !defined(CONFIG_ARM64)
>> >>  #include "irq.h"
>> >> +#endif
>> > 
>> > Instead of doing this, shouldn't we add arch/arm[64]/kvm/irq.h files.
>> > Probably a simple one like ./arch/s390/kvm/irq.h ?
>> 
>> Well I considered this solution in the past but I did not find much to
>> put there (it was even void). typically irqchip_in_kernel is in
>> include/kvm/arm_vgic.h since the macro can be shared between arm/arm64.
> 
> I think I'd prefer a nearly empty file to the #ifdef's, but Paolo and
> Radim should chime in.

I concur, hiding ugliness in header files is what we strive for.

The files could #include , which might make
their existence easier to understand.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v8 08/17] KVM: arm64: handle ITS related GICv3 redistributor registers

[Christoffer Dall]

On Tue, Jul 05, 2016 at 12:23:00PM +0100, Andre Przywara wrote:
> In the GICv3 redistributor there are the PENDBASER and PROPBASER
> registers which we did not emulate so far, as they only make sense
> when having an ITS. In preparation for that emulate those MMIO
> accesses by storing the 64-bit data written into it into a variable
> which we later read in the ITS emulation.
> We also sanitise the registers, making sure RES0 regions are respected
> and checking for valid memory attributes.
> 
> Signed-off-by: Andre Przywara 
> ---
>  include/kvm/arm_vgic.h   |  13 
>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 143 
> ++-
>  virt/kvm/arm/vgic/vgic-mmio.h|   8 +++
>  virt/kvm/arm/vgic/vgic-v3.c  |  11 ++-
>  4 files changed, 171 insertions(+), 4 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 450b4da..f6f860d 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -146,6 +146,14 @@ struct vgic_dist {
>   struct vgic_irq *spis;
>  
>   struct vgic_io_device   dist_iodev;
> +
> + /*
> +  * Contains the address of the LPI configuration table.

Is this field the address or the actual format of the GICR_PROPBASER ?

If the former, the type should potentially be gpa_t, if the latter, you
shouldn't say that this is just the address :)

> +  * Since we report GICR_TYPER.CommonLPIAff as 0b00, we can share
> +  * one address across all redistributors.
> +  * GICv3 spec: 6.1.2 "LPI Configuration tables"
> +  */
> + u64 propbaser;
>  };
>  
>  struct vgic_v2_cpu_if {
> @@ -200,6 +208,11 @@ struct vgic_cpu {
>*/
>   struct vgic_io_device   rd_iodev;
>   struct vgic_io_device   sgi_iodev;
> +
> + /* Points to the LPI pending tables for the redistributor */
> + u64 pendbaser;

ditto

> +
> + bool lpis_enabled;
>  };
>  
>  int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool 
> write);
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c 
> b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index bfcafbd..9dd8632 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -29,6 +29,19 @@ static unsigned long extract_bytes(unsigned long data, 
> unsigned int offset,
>   return (data >> (offset * 8)) & GENMASK_ULL(num * 8 - 1, 0);
>  }
>  
> +/* allows updates of any half of a 64-bit register (or the whole thing) */

when I see this function I have to read the bit fiddling to understand
the parameters and semantics.

actually, I'm not sure I read this correctly, but could you mean:

  /*
   * Update @len bytes at @offset bytes offset in @reg from the least
   * significant bytes in @val?
   */

Also, I don't get why this would be limited to either halfs or the whole
of a 64-bit register, but maybe I'm reading the code wrong.

Didn't we have a reviewed function for the vgic that was called
update_bytes or inser_bytes before, that we could use or did we never
actually review that?


> +static u64 update_64bit_reg(u64 reg, unsigned int offset, unsigned int len,
> + unsigned long val)
> +{
> + int lower = (offset & 4) * 8;
> + int upper = lower + 8 * len - 1;
> +
> + reg &= ~GENMASK_ULL(upper, lower);
> + val &= GENMASK_ULL(len * 8 - 1, 0);
> +
> + return reg | ((u64)val << lower);

this casting is weird.  Why is val not either a u64 or a u32?  Or the
whole lot could be unsigned long/unsigned int like extract_bytes.

> +}
> +
>  static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
>   gpa_t addr, unsigned int len)
>  {
> @@ -152,6 +165,132 @@ static unsigned long vgic_mmio_read_v3_idregs(struct 
> kvm_vcpu *vcpu,
>   return 0;
>  }
>  
> +/* We want to avoid outer shareable. */
> +u64 vgic_sanitise_shareability(u64 reg)
> +{
> + switch (reg & GIC_BASER_SHAREABILITY_MASK) {
> + case GIC_BASER_OuterShareable:
> + return GIC_BASER_InnerShareable;
> + default:
> + return reg;
> + }
> +}
> +
> +/* Non-cacheable or same-as-inner are OK. */
> +u64 vgic_sanitise_outer_cacheability(u64 reg)
> +{
> + switch (reg & GIC_BASER_CACHE_MASK) {
> + case GIC_BASER_CACHE_SameAsInner:
> + case GIC_BASER_CACHE_nC:
> + return reg;
> + default:
> + return GIC_BASER_CACHE_nC;
> + }
> +}
> +
> +/* Avoid any inner non-cacheable mapping. */
> +u64 vgic_sanitise_inner_cacheability(u64 reg)
> +{
> + switch (reg & GIC_BASER_CACHE_MASK) {
> + case GIC_BASER_CACHE_nCnB:
> + case GIC_BASER_CACHE_nC:
> + return GIC_BASER_CACHE_RaWb;
> + default:
> + return reg;
> + }
> +}

nit: My OCD sets in here because the functions above are not ordered
similarly to the calls below :)

also, why do you need to apply the mask in the sanitize functions when
you've just applied it in the callers?  

Re: [PATCH v8 11/17] KVM: arm64: implement basic ITS register handlers

[Marc Zyngier]

On 05/07/16 12:23, Andre Przywara wrote:
> Add emulation for some basic MMIO registers used in the ITS emulation.
> This includes:
> - GITS_{CTLR,TYPER,IIDR}
> - ID registers
> - GITS_{CBASER,CREADR,CWRITER}
>   (which implement the ITS command buffer handling)
> - GITS_BASER
> 
> Most of the handlers are pretty straight forward, only the CWRITER
> handler is a bit more involved by taking the new its_cmd mutex and
> then iterating over the command buffer.
> The registers holding base addresses and attributes are sanitised before
> storing them.
> 
> Signed-off-by: Andre Przywara 
> ---
>  include/kvm/arm_vgic.h   |  16 ++
>  virt/kvm/arm/vgic/vgic-its.c | 376 
> +--
>  virt/kvm/arm/vgic/vgic-mmio-v3.c |   8 +-
>  virt/kvm/arm/vgic/vgic-mmio.h|   6 +
>  virt/kvm/arm/vgic/vgic.c |  12 +-
>  5 files changed, 401 insertions(+), 17 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index eb82c7d..17d3929 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -22,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #define VGIC_V3_MAX_CPUS 255
>  #define VGIC_V2_MAX_CPUS 8
> @@ -128,6 +129,21 @@ struct vgic_its {
>   boolenabled;
>   boolinitialized;
>   struct vgic_io_device   iodev;
> +
> + /* These registers correspond to GITS_BASER{0,1} */
> + u64 baser_device_table;
> + u64 baser_coll_table;
> +
> + /* Protects the command queue */
> + struct mutexcmd_lock;
> + u64 cbaser;
> + u32 creadr;
> + u32 cwriter;
> +
> + /* Protects the device and collection lists */
> + struct mutexits_lock;
> + struct list_headdevice_list;
> + struct list_headcollection_list;
>  };
>  
>  struct vgic_dist {
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index d49bdad..a9336a4 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -21,6 +21,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  #include 
> @@ -32,6 +33,307 @@
>  #include "vgic.h"
>  #include "vgic-mmio.h"
>  
> +struct its_device {
> + struct list_head dev_list;
> +
> + /* the head for the list of ITTEs */
> + struct list_head itt_head;
> + u32 device_id;
> +};
> +
> +#define COLLECTION_NOT_MAPPED ((u32)~0)
> +
> +struct its_collection {
> + struct list_head coll_list;
> +
> + u32 collection_id;
> + u32 target_addr;
> +};
> +
> +#define its_is_collection_mapped(coll) ((coll) && \
> + ((coll)->target_addr != COLLECTION_NOT_MAPPED))
> +
> +struct its_itte {
> + struct list_head itte_list;
> +
> + struct its_collection *collection;
> + u32 lpi;
> + u32 event_id;
> +};
> +
> +#define CBASER_ADDRESS(x)((x) & GENMASK_ULL(51, 12))
> +
> +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)
> +{
> + u64 reg = GITS_TYPER_PLPIS;
> +
> + /*
> +  * We use linear CPU numbers for redistributor addressing,
> +  * so GITS_TYPER.PTA is 0.
> +  * Also we force all PROPBASER registers to be the same, so
> +  * CommonLPIAff is 0 as well.
> +  * To avoid memory waste in the guest, we keep the number of IDBits and
> +  * DevBits low - as least for the time being.
> +  */
> + reg |= 0x0f << GITS_TYPER_DEVBITS_SHIFT;
> + reg |= 0x0f << GITS_TYPER_IDBITS_SHIFT;
> +
> + return extract_bytes(reg, addr & 7, len);
> +}
> +
> +static unsigned long vgic_mmio_read_its_iidr(struct kvm *kvm,
> +  struct vgic_its *its,
> +  gpa_t addr, unsigned int len)
> +{
> + return (PRODUCT_ID_KVM << 24) | (IMPLEMENTER_ARM << 0);
> +}
> +
> +static unsigned long vgic_mmio_read_its_idregs(struct kvm *kvm,
> +  

Re: [PATCH v8 09/17] KVM: arm64: introduce ITS emulation file with MMIO framework

[André Przywara]

On 08/07/16 14:34, Marc Zyngier wrote:
> On 05/07/16 12:23, Andre Przywara wrote:
>> The ARM GICv3 ITS emulation code goes into a separate file, but needs
>> to be connected to the GICv3 emulation, of which it is an option.
>> The ITS MMIO handlers require the respective ITS pointer to be passed in,
>> so we amend the existing VGIC MMIO framework to let it cope with that.
>> Also we introduce the basic ITS data structure and initialize it, but
>> don't return any success yet, as we are not yet ready for the show.
>>
>> Signed-off-by: Andre Przywara 
>> ---
>>  arch/arm64/kvm/Makefile  |   1 +
>>  include/kvm/arm_vgic.h   |  14 +-
>>  virt/kvm/arm/vgic/vgic-its.c | 100 +
>>  virt/kvm/arm/vgic/vgic-mmio-v2.c |  40 +++
>>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 104 
>> ++-
>>  virt/kvm/arm/vgic/vgic-mmio.c|  36 +++---
>>  virt/kvm/arm/vgic/vgic-mmio.h|  31 +---
>>  virt/kvm/arm/vgic/vgic.h |   7 +++
>>  8 files changed, 266 insertions(+), 67 deletions(-)
>>  create mode 100644 virt/kvm/arm/vgic/vgic-its.c
>>
>> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
>> index f00b2cd..a5b9664 100644
>> --- a/arch/arm64/kvm/Makefile
>> +++ b/arch/arm64/kvm/Makefile
>> @@ -29,5 +29,6 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-mmio.o
>>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-mmio-v2.o
>>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-mmio-v3.o
>>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-kvm-device.o
>> +kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-its.o
>>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/arch_timer.o
>>  kvm-$(CONFIG_KVM_ARM_PMU) += $(KVM)/arm/pmu.o
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index f6f860d..f606641 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -108,15 +108,27 @@ struct vgic_irq {
>>  };
>>  
>>  struct vgic_register_region;
>> +struct vgic_its;
>>  
>>  struct vgic_io_device {
>>  gpa_t base_addr;
>> -struct kvm_vcpu *redist_vcpu;
>> +union {
>> +struct kvm_vcpu *redist_vcpu;
>> +struct vgic_its *its;
>> +};
> 
> The only question that springs to mind is...
> 
>>  const struct vgic_register_region *regions;
>>  int nr_regions;
>>  struct kvm_io_device dev;
>>  };
>>  
>> +struct vgic_its {
>> +/* The base address of the ITS control register frame */
>> +gpa_t   vgic_its_base;
>> +
>> +boolenabled;
>> +struct vgic_io_device   iodev;
>> +};
>> +
>>  struct vgic_dist {
>>  boolin_kernel;
>>  boolready;
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> new file mode 100644
>> index 000..ab8d244
>> --- /dev/null
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -0,0 +1,100 @@
>> +/*
>> + * GICv3 ITS emulation
>> + *
>> + * Copyright (C) 2015,2016 ARM Ltd.
>> + * Author: Andre Przywara 
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see .
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include 
>> +
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include "vgic.h"
>> +#include "vgic-mmio.h"
>> +
>> +#define REGISTER_ITS_DESC(off, rd, wr, length, acc) \
>> +{   \
>> +.reg_offset = off,  \
>> +.len = length,  \
>> +.access_flags = acc,\
>> +.iodev_type = IODEV_ITS,\
> 
> ... why isn't this at the device level? It doesn't make much sense to
> have it at the register level (we never access a register in isolation,
> we always access it relatively to a device).
> 
> And given that the *only* time you actually evaluate this flag is in
> dispatch_mmio_read/write, there is zero benefit in duplicating it all
> over the place.
> 
> Smaller structures, smaller patch. Am I missing something?

Looks possible. I think I tried something like this in the beginning and
hit some wall - possibly the one in my head ;-). Also I found it saner
to have the type associated with the declaration instead of adjusting
this in the 

Re: [PATCH v8 09/17] KVM: arm64: introduce ITS emulation file with MMIO framework

[Marc Zyngier]

On 08/07/16 14:34, Marc Zyngier wrote:
> On 05/07/16 12:23, Andre Przywara wrote:
>> The ARM GICv3 ITS emulation code goes into a separate file, but needs
>> to be connected to the GICv3 emulation, of which it is an option.
>> The ITS MMIO handlers require the respective ITS pointer to be passed in,
>> so we amend the existing VGIC MMIO framework to let it cope with that.
>> Also we introduce the basic ITS data structure and initialize it, but
>> don't return any success yet, as we are not yet ready for the show.
>>
>> Signed-off-by: Andre Przywara 
>> ---
>>  arch/arm64/kvm/Makefile  |   1 +
>>  include/kvm/arm_vgic.h   |  14 +-
>>  virt/kvm/arm/vgic/vgic-its.c | 100 +
>>  virt/kvm/arm/vgic/vgic-mmio-v2.c |  40 +++
>>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 104 
>> ++-
>>  virt/kvm/arm/vgic/vgic-mmio.c|  36 +++---
>>  virt/kvm/arm/vgic/vgic-mmio.h|  31 +---
>>  virt/kvm/arm/vgic/vgic.h |   7 +++
>>  8 files changed, 266 insertions(+), 67 deletions(-)
>>  create mode 100644 virt/kvm/arm/vgic/vgic-its.c
>>
>> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
>> index f00b2cd..a5b9664 100644
>> --- a/arch/arm64/kvm/Makefile
>> +++ b/arch/arm64/kvm/Makefile
>> @@ -29,5 +29,6 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-mmio.o
>>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-mmio-v2.o
>>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-mmio-v3.o
>>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-kvm-device.o
>> +kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-its.o
>>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/arch_timer.o
>>  kvm-$(CONFIG_KVM_ARM_PMU) += $(KVM)/arm/pmu.o
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index f6f860d..f606641 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -108,15 +108,27 @@ struct vgic_irq {
>>  };
>>  
>>  struct vgic_register_region;
>> +struct vgic_its;
>>  
>>  struct vgic_io_device {
>>  gpa_t base_addr;
>> -struct kvm_vcpu *redist_vcpu;
>> +union {
>> +struct kvm_vcpu *redist_vcpu;
>> +struct vgic_its *its;
>> +};
> 
> The only question that springs to mind is...
> 
>>  const struct vgic_register_region *regions;
>>  int nr_regions;
>>  struct kvm_io_device dev;
>>  };
>>  
>> +struct vgic_its {
>> +/* The base address of the ITS control register frame */
>> +gpa_t   vgic_its_base;
>> +
>> +boolenabled;
>> +struct vgic_io_device   iodev;
>> +};
>> +
>>  struct vgic_dist {
>>  boolin_kernel;
>>  boolready;
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> new file mode 100644
>> index 000..ab8d244
>> --- /dev/null
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -0,0 +1,100 @@
>> +/*
>> + * GICv3 ITS emulation
>> + *
>> + * Copyright (C) 2015,2016 ARM Ltd.
>> + * Author: Andre Przywara 
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see .
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include 
>> +
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include "vgic.h"
>> +#include "vgic-mmio.h"
>> +
>> +#define REGISTER_ITS_DESC(off, rd, wr, length, acc) \
>> +{   \
>> +.reg_offset = off,  \
>> +.len = length,  \
>> +.access_flags = acc,\
>> +.iodev_type = IODEV_ITS,\
> 
> ... why isn't this at the device level? It doesn't make much sense to
> have it at the register level (we never access a register in isolation,
> we always access it relatively to a device).
> 
> And given that the *only* time you actually evaluate this flag is in
> dispatch_mmio_read/write, there is zero benefit in duplicating it all
> over the place.
> 
> Smaller structures, smaller patch. Am I missing something?

And for the record, here's what I've cooked on top of your patch:

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index c64db0f..95eab74 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -112,12 

Re: [PATCH v8 09/17] KVM: arm64: introduce ITS emulation file with MMIO framework

[Marc Zyngier]

On 05/07/16 12:23, Andre Przywara wrote:
> The ARM GICv3 ITS emulation code goes into a separate file, but needs
> to be connected to the GICv3 emulation, of which it is an option.
> The ITS MMIO handlers require the respective ITS pointer to be passed in,
> so we amend the existing VGIC MMIO framework to let it cope with that.
> Also we introduce the basic ITS data structure and initialize it, but
> don't return any success yet, as we are not yet ready for the show.
> 
> Signed-off-by: Andre Przywara 
> ---
>  arch/arm64/kvm/Makefile  |   1 +
>  include/kvm/arm_vgic.h   |  14 +-
>  virt/kvm/arm/vgic/vgic-its.c | 100 +
>  virt/kvm/arm/vgic/vgic-mmio-v2.c |  40 +++
>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 104 
> ++-
>  virt/kvm/arm/vgic/vgic-mmio.c|  36 +++---
>  virt/kvm/arm/vgic/vgic-mmio.h|  31 +---
>  virt/kvm/arm/vgic/vgic.h |   7 +++
>  8 files changed, 266 insertions(+), 67 deletions(-)
>  create mode 100644 virt/kvm/arm/vgic/vgic-its.c
> 
> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> index f00b2cd..a5b9664 100644
> --- a/arch/arm64/kvm/Makefile
> +++ b/arch/arm64/kvm/Makefile
> @@ -29,5 +29,6 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-mmio.o
>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-mmio-v2.o
>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-mmio-v3.o
>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-kvm-device.o
> +kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-its.o
>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/arch_timer.o
>  kvm-$(CONFIG_KVM_ARM_PMU) += $(KVM)/arm/pmu.o
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index f6f860d..f606641 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -108,15 +108,27 @@ struct vgic_irq {
>  };
>  
>  struct vgic_register_region;
> +struct vgic_its;
>  
>  struct vgic_io_device {
>   gpa_t base_addr;
> - struct kvm_vcpu *redist_vcpu;
> + union {
> + struct kvm_vcpu *redist_vcpu;
> + struct vgic_its *its;
> + };

The only question that springs to mind is...

>   const struct vgic_register_region *regions;
>   int nr_regions;
>   struct kvm_io_device dev;
>  };
>  
> +struct vgic_its {
> + /* The base address of the ITS control register frame */
> + gpa_t   vgic_its_base;
> +
> + boolenabled;
> + struct vgic_io_device   iodev;
> +};
> +
>  struct vgic_dist {
>   boolin_kernel;
>   boolready;
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> new file mode 100644
> index 000..ab8d244
> --- /dev/null
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -0,0 +1,100 @@
> +/*
> + * GICv3 ITS emulation
> + *
> + * Copyright (C) 2015,2016 ARM Ltd.
> + * Author: Andre Przywara 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see .
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +
> +#include "vgic.h"
> +#include "vgic-mmio.h"
> +
> +#define REGISTER_ITS_DESC(off, rd, wr, length, acc)  \
> +{\
> + .reg_offset = off,  \
> + .len = length,  \
> + .access_flags = acc,\
> + .iodev_type = IODEV_ITS,\

... why isn't this at the device level? It doesn't make much sense to
have it at the register level (we never access a register in isolation,
we always access it relatively to a device).

And given that the *only* time you actually evaluate this flag is in
dispatch_mmio_read/write, there is zero benefit in duplicating it all
over the place.

Smaller structures, smaller patch. Am I missing something?

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

Re: [PATCH v8 06/17] KVM: arm/arm64: VGIC: add refcounting for IRQs

[André Przywara]

On 08/07/16 14:09, Marc Zyngier wrote:
> On 08/07/16 13:54, André Przywara wrote:
>> On 08/07/16 11:50, Marc Zyngier wrote:
>>> On 08/07/16 11:28, Andre Przywara wrote:
 Hi,

 On 07/07/16 16:00, Marc Zyngier wrote:
> On 05/07/16 12:22, Andre Przywara wrote:



>> @@ -236,6 +254,7 @@ retry:
>>  goto retry;
>>  }
>>  
>> +kref_get(>refcount);
>
> Could you use vgic_get_irq() instead? I know it is slightly overkill,
> but I can already tell that we'll need to add some tracing in both the
> put and get helpers in order to do some debugging. Having straight
> kref_get/put is going to make this tracing difficult, so let's not go 
> there.

 I'd rather not.
 1) Putting the IRQ on the ap_list is the "other user" of the
 refcounting, I don't want to mix that unnecessarily with the
 vgic_get_irq() (as in: get the struct by the number) use case. That may
 actually help tracing, since we can have separate tracepoints to
 distinguish them.
>>>
>>> And yet you end-up doing a vgic_put_irq() in the fold operation. Which
>>> is wrong, by the way, as the interrupt is still in in ap_list. This
>>> should be moved to the prune operation.
>>>
 2) This would violate the locking order, since we hold the irq_lock here
 and possibly take the lpi_list_lock in vgic_get_irq().
 I don't think we can or should drop the irq_lock and re-take it just for
 this.
>>>
>>> That's a much more convincing argument. And when you take the above into
>>> account, you realize that your locking is not correct. You shouldn't be
>>> dropping the refcount in fold, but in prune, meaning that you're holding
>>> the ap_lock and the irq_lock, same as when you inserted the interrupt in
>>> the list.
>>>
>>> This is outlining an essential principle: if your locking/refcounting is
>>> not symmetric, it is likely that you're doing something wrong, and that
>>> should bother you really badly.
>>
>> Can you point me to the exact location where it's not symmetric?
>> I just looked at it again and can't find the issue.
>> I "put" it in v[23]_fold because we did a vgic_get_irq a few lines
>> before to translate the LR's intid into our struct vgic_irq pointer.
> 
> Right. I misread that one, apologies.
> 
>> The vgic_get_irq() call isn't in this patch, because we used it already
>> before and this patch is just adding the respective puts.
>> The only asymmetry I could find is the expected one when it comes to and
>> goes from the ap_list.
> 
> So I assume this is the pendent of the kref_get call?

Yes.

> 
> @@ -386,6 +412,7 @@ retry:
>   list_del(>ap_list);
>   irq->vcpu = NULL;
>   spin_unlock(>irq_lock);
> + vgic_put_irq(vcpu->kvm, irq);
>   continue;
> 
> If that's the case, please add a comment, because this is really hard to
> find out which vgic_put_irq() balances with a kref_get() and not a
> vgic_get_irq().

Yes, that's what I thought as well just _after_ having hit the Send
button ...

I think much of the confusion stems from the fact that we used
vgic_get_irq() before, only that it wasn't a "get" in the refcounting
get-put sense.

Cheers,
Andre.

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

Re: [PATCH v8 06/17] KVM: arm/arm64: VGIC: add refcounting for IRQs

[Marc Zyngier]

On 08/07/16 13:54, André Przywara wrote:
> On 08/07/16 11:50, Marc Zyngier wrote:
>> On 08/07/16 11:28, Andre Przywara wrote:
>>> Hi,
>>>
>>> On 07/07/16 16:00, Marc Zyngier wrote:
 On 05/07/16 12:22, Andre Przywara wrote:
> In the moment our struct vgic_irq's are statically allocated at guest
> creation time. So getting a pointer to an IRQ structure is trivial and
> safe. LPIs are more dynamic, they can be mapped and unmapped at any time
> during the guest's _runtime_.
> In preparation for supporting LPIs we introduce reference counting for
> those structures using the kernel's kref infrastructure.
> Since private IRQs and SPIs are statically allocated, the refcount never
> drops to 0 at the moment, but we increase it when an IRQ gets onto a VCPU
> list and decrease it when it gets removed.
> This introduces vgic_put_irq(), which wraps kref_put and hides the
> release function from the callers.
>
> Signed-off-by: Andre Przywara 
> ---
>  include/kvm/arm_vgic.h   |  1 +
>  virt/kvm/arm/vgic/vgic-init.c|  2 ++
>  virt/kvm/arm/vgic/vgic-mmio-v2.c |  8 +++
>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 20 +++--
>  virt/kvm/arm/vgic/vgic-mmio.c| 25 -
>  virt/kvm/arm/vgic/vgic-v2.c  |  1 +
>  virt/kvm/arm/vgic/vgic-v3.c  |  1 +
>  virt/kvm/arm/vgic/vgic.c | 48 
> +++-
>  virt/kvm/arm/vgic/vgic.h |  1 +
>  9 files changed, 89 insertions(+), 18 deletions(-)
>
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 5142e2a..450b4da 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -96,6 +96,7 @@ struct vgic_irq {
>   bool active;/* not used for LPIs */
>   bool enabled;
>   bool hw;/* Tied to HW IRQ */
> + struct kref refcount;   /* Used for LPIs */
>   u32 hwintid;/* HW INTID number */
>   union {
>   u8 targets; /* GICv2 target VCPUs mask */
> diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
> index 90cae48..ac3c1a5 100644
> --- a/virt/kvm/arm/vgic/vgic-init.c
> +++ b/virt/kvm/arm/vgic/vgic-init.c
> @@ -177,6 +177,7 @@ static int kvm_vgic_dist_init(struct kvm *kvm, 
> unsigned int nr_spis)
>   spin_lock_init(>irq_lock);
>   irq->vcpu = NULL;
>   irq->target_vcpu = vcpu0;
> + kref_init(>refcount);
>   if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
>   irq->targets = 0;
>   else
> @@ -211,6 +212,7 @@ static void kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
>   irq->vcpu = NULL;
>   irq->target_vcpu = vcpu;
>   irq->targets = 1U << vcpu->vcpu_id;
> + kref_init(>refcount);
>   if (vgic_irq_is_sgi(i)) {
>   /* SGIs */
>   irq->enabled = 1;
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c 
> b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index a213936..4152348 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -102,6 +102,7 @@ static void vgic_mmio_write_sgir(struct kvm_vcpu 
> *source_vcpu,
>   irq->source |= 1U << source_vcpu->vcpu_id;
>  
>   vgic_queue_irq_unlock(source_vcpu->kvm, irq);
> + vgic_put_irq(source_vcpu->kvm, irq);
>   }
>  }
>  
> @@ -116,6 +117,8 @@ static unsigned long vgic_mmio_read_target(struct 
> kvm_vcpu *vcpu,
>   struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>  
>   val |= (u64)irq->targets << (i * 8);
> +
> + vgic_put_irq(vcpu->kvm, irq);
>   }
>  
>   return val;
> @@ -143,6 +146,7 @@ static void vgic_mmio_write_target(struct kvm_vcpu 
> *vcpu,
>   irq->target_vcpu = kvm_get_vcpu(vcpu->kvm, target);
>  
>   spin_unlock(>irq_lock);
> + vgic_put_irq(vcpu->kvm, irq);
>   }
>  }
>  
> @@ -157,6 +161,8 @@ static unsigned long vgic_mmio_read_sgipend(struct 
> kvm_vcpu *vcpu,
>   struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>  
>   val |= (u64)irq->source << (i * 8);
> +
> + vgic_put_irq(vcpu->kvm, irq);
>   }
>   return val;
>  }
> @@ -178,6 +184,7 @@ static void vgic_mmio_write_sgipendc(struct kvm_vcpu 
> *vcpu,
>   irq->pending = false;
>  
>   spin_unlock(>irq_lock);
> + vgic_put_irq(vcpu->kvm, irq);
>   }
>  }
>  
> @@ -201,6 +208,7 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu 
> *vcpu,
>   } else {
>   

Re: [PATCH v8 06/17] KVM: arm/arm64: VGIC: add refcounting for IRQs

[André Przywara]

On 08/07/16 11:50, Marc Zyngier wrote:
> On 08/07/16 11:28, Andre Przywara wrote:
>> Hi,
>>
>> On 07/07/16 16:00, Marc Zyngier wrote:
>>> On 05/07/16 12:22, Andre Przywara wrote:
 In the moment our struct vgic_irq's are statically allocated at guest
 creation time. So getting a pointer to an IRQ structure is trivial and
 safe. LPIs are more dynamic, they can be mapped and unmapped at any time
 during the guest's _runtime_.
 In preparation for supporting LPIs we introduce reference counting for
 those structures using the kernel's kref infrastructure.
 Since private IRQs and SPIs are statically allocated, the refcount never
 drops to 0 at the moment, but we increase it when an IRQ gets onto a VCPU
 list and decrease it when it gets removed.
 This introduces vgic_put_irq(), which wraps kref_put and hides the
 release function from the callers.

 Signed-off-by: Andre Przywara 
 ---
  include/kvm/arm_vgic.h   |  1 +
  virt/kvm/arm/vgic/vgic-init.c|  2 ++
  virt/kvm/arm/vgic/vgic-mmio-v2.c |  8 +++
  virt/kvm/arm/vgic/vgic-mmio-v3.c | 20 +++--
  virt/kvm/arm/vgic/vgic-mmio.c| 25 -
  virt/kvm/arm/vgic/vgic-v2.c  |  1 +
  virt/kvm/arm/vgic/vgic-v3.c  |  1 +
  virt/kvm/arm/vgic/vgic.c | 48 
 +++-
  virt/kvm/arm/vgic/vgic.h |  1 +
  9 files changed, 89 insertions(+), 18 deletions(-)

 diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
 index 5142e2a..450b4da 100644
 --- a/include/kvm/arm_vgic.h
 +++ b/include/kvm/arm_vgic.h
 @@ -96,6 +96,7 @@ struct vgic_irq {
bool active;/* not used for LPIs */
bool enabled;
bool hw;/* Tied to HW IRQ */
 +  struct kref refcount;   /* Used for LPIs */
u32 hwintid;/* HW INTID number */
union {
u8 targets; /* GICv2 target VCPUs mask */
 diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
 index 90cae48..ac3c1a5 100644
 --- a/virt/kvm/arm/vgic/vgic-init.c
 +++ b/virt/kvm/arm/vgic/vgic-init.c
 @@ -177,6 +177,7 @@ static int kvm_vgic_dist_init(struct kvm *kvm, 
 unsigned int nr_spis)
spin_lock_init(>irq_lock);
irq->vcpu = NULL;
irq->target_vcpu = vcpu0;
 +  kref_init(>refcount);
if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
irq->targets = 0;
else
 @@ -211,6 +212,7 @@ static void kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
irq->vcpu = NULL;
irq->target_vcpu = vcpu;
irq->targets = 1U << vcpu->vcpu_id;
 +  kref_init(>refcount);
if (vgic_irq_is_sgi(i)) {
/* SGIs */
irq->enabled = 1;
 diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c 
 b/virt/kvm/arm/vgic/vgic-mmio-v2.c
 index a213936..4152348 100644
 --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
 +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
 @@ -102,6 +102,7 @@ static void vgic_mmio_write_sgir(struct kvm_vcpu 
 *source_vcpu,
irq->source |= 1U << source_vcpu->vcpu_id;
  
vgic_queue_irq_unlock(source_vcpu->kvm, irq);
 +  vgic_put_irq(source_vcpu->kvm, irq);
}
  }
  
 @@ -116,6 +117,8 @@ static unsigned long vgic_mmio_read_target(struct 
 kvm_vcpu *vcpu,
struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
  
val |= (u64)irq->targets << (i * 8);
 +
 +  vgic_put_irq(vcpu->kvm, irq);
}
  
return val;
 @@ -143,6 +146,7 @@ static void vgic_mmio_write_target(struct kvm_vcpu 
 *vcpu,
irq->target_vcpu = kvm_get_vcpu(vcpu->kvm, target);
  
spin_unlock(>irq_lock);
 +  vgic_put_irq(vcpu->kvm, irq);
}
  }
  
 @@ -157,6 +161,8 @@ static unsigned long vgic_mmio_read_sgipend(struct 
 kvm_vcpu *vcpu,
struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
  
val |= (u64)irq->source << (i * 8);
 +
 +  vgic_put_irq(vcpu->kvm, irq);
}
return val;
  }
 @@ -178,6 +184,7 @@ static void vgic_mmio_write_sgipendc(struct kvm_vcpu 
 *vcpu,
irq->pending = false;
  
spin_unlock(>irq_lock);
 +  vgic_put_irq(vcpu->kvm, irq);
}
  }
  
 @@ -201,6 +208,7 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu 
 *vcpu,
} else {
spin_unlock(>irq_lock);
}
 +  vgic_put_irq(vcpu->kvm, irq);
}

Re: [PATCH v8 06/17] KVM: arm/arm64: VGIC: add refcounting for IRQs

[Marc Zyngier]

On 08/07/16 11:28, Andre Przywara wrote:
> Hi,
> 
> On 07/07/16 16:00, Marc Zyngier wrote:
>> On 05/07/16 12:22, Andre Przywara wrote:
>>> In the moment our struct vgic_irq's are statically allocated at guest
>>> creation time. So getting a pointer to an IRQ structure is trivial and
>>> safe. LPIs are more dynamic, they can be mapped and unmapped at any time
>>> during the guest's _runtime_.
>>> In preparation for supporting LPIs we introduce reference counting for
>>> those structures using the kernel's kref infrastructure.
>>> Since private IRQs and SPIs are statically allocated, the refcount never
>>> drops to 0 at the moment, but we increase it when an IRQ gets onto a VCPU
>>> list and decrease it when it gets removed.
>>> This introduces vgic_put_irq(), which wraps kref_put and hides the
>>> release function from the callers.
>>>
>>> Signed-off-by: Andre Przywara 
>>> ---
>>>  include/kvm/arm_vgic.h   |  1 +
>>>  virt/kvm/arm/vgic/vgic-init.c|  2 ++
>>>  virt/kvm/arm/vgic/vgic-mmio-v2.c |  8 +++
>>>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 20 +++--
>>>  virt/kvm/arm/vgic/vgic-mmio.c| 25 -
>>>  virt/kvm/arm/vgic/vgic-v2.c  |  1 +
>>>  virt/kvm/arm/vgic/vgic-v3.c  |  1 +
>>>  virt/kvm/arm/vgic/vgic.c | 48 
>>> +++-
>>>  virt/kvm/arm/vgic/vgic.h |  1 +
>>>  9 files changed, 89 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>>> index 5142e2a..450b4da 100644
>>> --- a/include/kvm/arm_vgic.h
>>> +++ b/include/kvm/arm_vgic.h
>>> @@ -96,6 +96,7 @@ struct vgic_irq {
>>> bool active;/* not used for LPIs */
>>> bool enabled;
>>> bool hw;/* Tied to HW IRQ */
>>> +   struct kref refcount;   /* Used for LPIs */
>>> u32 hwintid;/* HW INTID number */
>>> union {
>>> u8 targets; /* GICv2 target VCPUs mask */
>>> diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
>>> index 90cae48..ac3c1a5 100644
>>> --- a/virt/kvm/arm/vgic/vgic-init.c
>>> +++ b/virt/kvm/arm/vgic/vgic-init.c
>>> @@ -177,6 +177,7 @@ static int kvm_vgic_dist_init(struct kvm *kvm, unsigned 
>>> int nr_spis)
>>> spin_lock_init(>irq_lock);
>>> irq->vcpu = NULL;
>>> irq->target_vcpu = vcpu0;
>>> +   kref_init(>refcount);
>>> if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
>>> irq->targets = 0;
>>> else
>>> @@ -211,6 +212,7 @@ static void kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
>>> irq->vcpu = NULL;
>>> irq->target_vcpu = vcpu;
>>> irq->targets = 1U << vcpu->vcpu_id;
>>> +   kref_init(>refcount);
>>> if (vgic_irq_is_sgi(i)) {
>>> /* SGIs */
>>> irq->enabled = 1;
>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c 
>>> b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>>> index a213936..4152348 100644
>>> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
>>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>>> @@ -102,6 +102,7 @@ static void vgic_mmio_write_sgir(struct kvm_vcpu 
>>> *source_vcpu,
>>> irq->source |= 1U << source_vcpu->vcpu_id;
>>>  
>>> vgic_queue_irq_unlock(source_vcpu->kvm, irq);
>>> +   vgic_put_irq(source_vcpu->kvm, irq);
>>> }
>>>  }
>>>  
>>> @@ -116,6 +117,8 @@ static unsigned long vgic_mmio_read_target(struct 
>>> kvm_vcpu *vcpu,
>>> struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>>>  
>>> val |= (u64)irq->targets << (i * 8);
>>> +
>>> +   vgic_put_irq(vcpu->kvm, irq);
>>> }
>>>  
>>> return val;
>>> @@ -143,6 +146,7 @@ static void vgic_mmio_write_target(struct kvm_vcpu 
>>> *vcpu,
>>> irq->target_vcpu = kvm_get_vcpu(vcpu->kvm, target);
>>>  
>>> spin_unlock(>irq_lock);
>>> +   vgic_put_irq(vcpu->kvm, irq);
>>> }
>>>  }
>>>  
>>> @@ -157,6 +161,8 @@ static unsigned long vgic_mmio_read_sgipend(struct 
>>> kvm_vcpu *vcpu,
>>> struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>>>  
>>> val |= (u64)irq->source << (i * 8);
>>> +
>>> +   vgic_put_irq(vcpu->kvm, irq);
>>> }
>>> return val;
>>>  }
>>> @@ -178,6 +184,7 @@ static void vgic_mmio_write_sgipendc(struct kvm_vcpu 
>>> *vcpu,
>>> irq->pending = false;
>>>  
>>> spin_unlock(>irq_lock);
>>> +   vgic_put_irq(vcpu->kvm, irq);
>>> }
>>>  }
>>>  
>>> @@ -201,6 +208,7 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu 
>>> *vcpu,
>>> } else {
>>> spin_unlock(>irq_lock);
>>> }
>>> +   vgic_put_irq(vcpu->kvm, irq);
>>> }
>>>  }
>>>  
>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c 
>>> b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>>> index 

Re: [PATCH v8 06/17] KVM: arm/arm64: VGIC: add refcounting for IRQs

[Andre Przywara]

Hi,

On 07/07/16 16:00, Marc Zyngier wrote:
> On 05/07/16 12:22, Andre Przywara wrote:
>> In the moment our struct vgic_irq's are statically allocated at guest
>> creation time. So getting a pointer to an IRQ structure is trivial and
>> safe. LPIs are more dynamic, they can be mapped and unmapped at any time
>> during the guest's _runtime_.
>> In preparation for supporting LPIs we introduce reference counting for
>> those structures using the kernel's kref infrastructure.
>> Since private IRQs and SPIs are statically allocated, the refcount never
>> drops to 0 at the moment, but we increase it when an IRQ gets onto a VCPU
>> list and decrease it when it gets removed.
>> This introduces vgic_put_irq(), which wraps kref_put and hides the
>> release function from the callers.
>>
>> Signed-off-by: Andre Przywara 
>> ---
>>  include/kvm/arm_vgic.h   |  1 +
>>  virt/kvm/arm/vgic/vgic-init.c|  2 ++
>>  virt/kvm/arm/vgic/vgic-mmio-v2.c |  8 +++
>>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 20 +++--
>>  virt/kvm/arm/vgic/vgic-mmio.c| 25 -
>>  virt/kvm/arm/vgic/vgic-v2.c  |  1 +
>>  virt/kvm/arm/vgic/vgic-v3.c  |  1 +
>>  virt/kvm/arm/vgic/vgic.c | 48 
>> +++-
>>  virt/kvm/arm/vgic/vgic.h |  1 +
>>  9 files changed, 89 insertions(+), 18 deletions(-)
>>
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index 5142e2a..450b4da 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -96,6 +96,7 @@ struct vgic_irq {
>>  bool active;/* not used for LPIs */
>>  bool enabled;
>>  bool hw;/* Tied to HW IRQ */
>> +struct kref refcount;   /* Used for LPIs */
>>  u32 hwintid;/* HW INTID number */
>>  union {
>>  u8 targets; /* GICv2 target VCPUs mask */
>> diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
>> index 90cae48..ac3c1a5 100644
>> --- a/virt/kvm/arm/vgic/vgic-init.c
>> +++ b/virt/kvm/arm/vgic/vgic-init.c
>> @@ -177,6 +177,7 @@ static int kvm_vgic_dist_init(struct kvm *kvm, unsigned 
>> int nr_spis)
>>  spin_lock_init(>irq_lock);
>>  irq->vcpu = NULL;
>>  irq->target_vcpu = vcpu0;
>> +kref_init(>refcount);
>>  if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
>>  irq->targets = 0;
>>  else
>> @@ -211,6 +212,7 @@ static void kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
>>  irq->vcpu = NULL;
>>  irq->target_vcpu = vcpu;
>>  irq->targets = 1U << vcpu->vcpu_id;
>> +kref_init(>refcount);
>>  if (vgic_irq_is_sgi(i)) {
>>  /* SGIs */
>>  irq->enabled = 1;
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c 
>> b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> index a213936..4152348 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> @@ -102,6 +102,7 @@ static void vgic_mmio_write_sgir(struct kvm_vcpu 
>> *source_vcpu,
>>  irq->source |= 1U << source_vcpu->vcpu_id;
>>  
>>  vgic_queue_irq_unlock(source_vcpu->kvm, irq);
>> +vgic_put_irq(source_vcpu->kvm, irq);
>>  }
>>  }
>>  
>> @@ -116,6 +117,8 @@ static unsigned long vgic_mmio_read_target(struct 
>> kvm_vcpu *vcpu,
>>  struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>>  
>>  val |= (u64)irq->targets << (i * 8);
>> +
>> +vgic_put_irq(vcpu->kvm, irq);
>>  }
>>  
>>  return val;
>> @@ -143,6 +146,7 @@ static void vgic_mmio_write_target(struct kvm_vcpu *vcpu,
>>  irq->target_vcpu = kvm_get_vcpu(vcpu->kvm, target);
>>  
>>  spin_unlock(>irq_lock);
>> +vgic_put_irq(vcpu->kvm, irq);
>>  }
>>  }
>>  
>> @@ -157,6 +161,8 @@ static unsigned long vgic_mmio_read_sgipend(struct 
>> kvm_vcpu *vcpu,
>>  struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>>  
>>  val |= (u64)irq->source << (i * 8);
>> +
>> +vgic_put_irq(vcpu->kvm, irq);
>>  }
>>  return val;
>>  }
>> @@ -178,6 +184,7 @@ static void vgic_mmio_write_sgipendc(struct kvm_vcpu 
>> *vcpu,
>>  irq->pending = false;
>>  
>>  spin_unlock(>irq_lock);
>> +vgic_put_irq(vcpu->kvm, irq);
>>  }
>>  }
>>  
>> @@ -201,6 +208,7 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu 
>> *vcpu,
>>  } else {
>>  spin_unlock(>irq_lock);
>>  }
>> +vgic_put_irq(vcpu->kvm, irq);
>>  }
>>  }
>>  
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c 
>> b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> index fc7b6c9..bfcafbd 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> @@ -80,15 +80,17 @@ 

Re: [RFC v6 4/6] KVM: arm/arm64: enable irqchip routing

[Andrew Jones]

On Fri, Jul 08, 2016 at 10:16:53AM +0200, Auger Eric wrote:
> Hi Drew,
> 
> On 07/07/2016 19:20, Andrew Jones wrote:
> > On Wed, Jul 06, 2016 at 10:47:53AM +0200, Eric Auger wrote:
> >> This patch adds compilation and link against irqchip.
> >>
> >> Main motivation behind using irqchip code is to enable MSI
> >> routing code. In the future irqchip routing may also be useful
> >> when targeting multiple irqchips.
> >>
> >> Routing standard callbacks now are implemented in vgic-irqfd:
> >> - kvm_set_routing_entry
> >> - kvm_set_irq
> >> - kvm_set_msi
> >>
> >> They only are supported with new_vgic code.
> >>
> >> Both HAVE_KVM_IRQCHIP and HAVE_KVM_IRQ_ROUTING are defined.
> >> KVM_CAP_IRQ_ROUTING is advertised and KVM_SET_GSI_ROUTING is allowed.
> >>
> >> So from now on IRQCHIP routing is enabled and a routing table entry
> >> must exist for irqfd injection to succeed for a given SPI. This patch
> >> builds a default flat irqchip routing table (gsi=irqchip.pin) covering
> >> all the VGIC SPI indexes. This routing table is overwritten by the
> >> first first user-space call to KVM_SET_GSI_ROUTING ioctl.
> >>
> >> MSI routing setup is not yet allowed.
> >>
> >> Signed-off-by: Eric Auger 
> >>
> >> ---
> >> v5 -> v6:
> >> - rebase on top of Andre's v8 + removal of old vgic
> >>
> >> v4 -> v5:
> >> - vgic_irqfd.c was renamed into vgic-irqfd.c by Andre
> >> - minor comment changes
> >> - remove trace_kvm_set_irq since it is called in irqchip
> >> - remove CONFIG_HAVE_KVM_MSI setting (done in KVM section)
> >> - despite Christoffer's question, in kvm_set_msi, I kept the copy from
> >>   the input "struct kvm_kernel_irq_routing_entry *e" imposed by the
> >>   irqchip callback API into the struct kvm_msi * passed to
> >>   vits_inject_msi. Since vits_inject_msi is directly called by
> >>   kvm_send_userspace_msi which takes a struct kvm_msi*, makes sense
> >>   to me to keep the copy.
> >> - squash former [PATCH v4 5/7] KVM: arm/arm64: build a default routing
> >>   table into that patch
> >> - handle default routing table alloc failure
> >>
> >> v3 -> v4:
> >> - provide support only for new-vgic
> >> - code previously in vgic.c now in vgic_irqfd.c
> >>
> >> v2 -> v3:
> >> - unconditionally set devid and KVM_MSI_VALID_DEVID flag as suggested
> >>   by Andre (KVM_IRQ_ROUTING_EXTENDED_MSI type not used anymore)
> >> - vgic_irqfd_set_irq now is static
> >> - propagate flags
> >> - add comments
> >>
> >> v1 -> v2:
> >> - fix bug reported by Andre related to msi.flags and msi.devid setting
> >>   in kvm_send_userspace_msi
> >> - avoid injecting reserved IRQ numbers in vgic_irqfd_set_irq
> >>
> >> RFC -> PATCH
> >> - reword api.txt:
> >>   x move MSI routing comments in a subsequent patch,
> >>   x clearly state GSI routing does not apply to KVM_IRQ_LINE
> >> ---
> >>  Documentation/virtual/kvm/api.txt | 12 --
> >>  arch/arm/include/asm/kvm_host.h   |  2 +
> >>  arch/arm/kvm/Kconfig  |  2 +
> >>  arch/arm/kvm/Makefile |  1 +
> >>  arch/arm64/include/asm/kvm_host.h |  1 +
> >>  arch/arm64/kvm/Kconfig|  2 +
> >>  arch/arm64/kvm/Makefile   |  1 +
> >>  virt/kvm/arm/vgic/vgic-init.c | 27 +
> >>  virt/kvm/arm/vgic/vgic-irqfd.c| 82 
> >> ++-
> >>  virt/kvm/arm/vgic/vgic.c  |  7 
> >>  virt/kvm/irqchip.c|  2 +
> >>  11 files changed, 109 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/Documentation/virtual/kvm/api.txt 
> >> b/Documentation/virtual/kvm/api.txt
> >> index 0065c8e..3bb60d3 100644
> >> --- a/Documentation/virtual/kvm/api.txt
> >> +++ b/Documentation/virtual/kvm/api.txt
> >> @@ -1433,13 +1433,16 @@ KVM_ASSIGN_DEV_IRQ. Partial deassignment of host 
> >> or guest IRQ is allowed.
> >>  4.52 KVM_SET_GSI_ROUTING
> >>  
> >>  Capability: KVM_CAP_IRQ_ROUTING
> >> -Architectures: x86 s390
> >> +Architectures: x86 s390 arm arm64
> >>  Type: vm ioctl
> >>  Parameters: struct kvm_irq_routing (in)
> >>  Returns: 0 on success, -1 on error
> >>  
> >>  Sets the GSI routing table entries, overwriting any previously set 
> >> entries.
> >>  
> >> +On arm/arm64, GSI routing has the following limitation:
> >> +- GSI routing does not apply to KVM_IRQ_LINE but only to KVM_IRQFD.
> >> +
> >>  struct kvm_irq_routing {
> >>__u32 nr;
> >>__u32 flags;
> >> @@ -2368,9 +2371,10 @@ Note that closing the resamplefd is not sufficient 
> >> to disable the
> >>  irqfd.  The KVM_IRQFD_FLAG_RESAMPLE is only necessary on assignment
> >>  and need not be specified with KVM_IRQFD_FLAG_DEASSIGN.
> >>  
> >> -On ARM/ARM64, the gsi field in the kvm_irqfd struct specifies the Shared
> >> -Peripheral Interrupt (SPI) index, such that the GIC interrupt ID is
> >> -given by gsi + 32.
> >> +On arm/arm64, gsi routing being supported, the following can happen:
> >> +- in case no routing entry is associated to this gsi, injection fails
> >> +- in case the gsi is associated to an irqchip routing 

Re: [RFC v6 4/6] KVM: arm/arm64: enable irqchip routing

[Auger Eric]

Hi Drew,

On 07/07/2016 19:20, Andrew Jones wrote:
> On Wed, Jul 06, 2016 at 10:47:53AM +0200, Eric Auger wrote:
>> This patch adds compilation and link against irqchip.
>>
>> Main motivation behind using irqchip code is to enable MSI
>> routing code. In the future irqchip routing may also be useful
>> when targeting multiple irqchips.
>>
>> Routing standard callbacks now are implemented in vgic-irqfd:
>> - kvm_set_routing_entry
>> - kvm_set_irq
>> - kvm_set_msi
>>
>> They only are supported with new_vgic code.
>>
>> Both HAVE_KVM_IRQCHIP and HAVE_KVM_IRQ_ROUTING are defined.
>> KVM_CAP_IRQ_ROUTING is advertised and KVM_SET_GSI_ROUTING is allowed.
>>
>> So from now on IRQCHIP routing is enabled and a routing table entry
>> must exist for irqfd injection to succeed for a given SPI. This patch
>> builds a default flat irqchip routing table (gsi=irqchip.pin) covering
>> all the VGIC SPI indexes. This routing table is overwritten by the
>> first first user-space call to KVM_SET_GSI_ROUTING ioctl.
>>
>> MSI routing setup is not yet allowed.
>>
>> Signed-off-by: Eric Auger 
>>
>> ---
>> v5 -> v6:
>> - rebase on top of Andre's v8 + removal of old vgic
>>
>> v4 -> v5:
>> - vgic_irqfd.c was renamed into vgic-irqfd.c by Andre
>> - minor comment changes
>> - remove trace_kvm_set_irq since it is called in irqchip
>> - remove CONFIG_HAVE_KVM_MSI setting (done in KVM section)
>> - despite Christoffer's question, in kvm_set_msi, I kept the copy from
>>   the input "struct kvm_kernel_irq_routing_entry *e" imposed by the
>>   irqchip callback API into the struct kvm_msi * passed to
>>   vits_inject_msi. Since vits_inject_msi is directly called by
>>   kvm_send_userspace_msi which takes a struct kvm_msi*, makes sense
>>   to me to keep the copy.
>> - squash former [PATCH v4 5/7] KVM: arm/arm64: build a default routing
>>   table into that patch
>> - handle default routing table alloc failure
>>
>> v3 -> v4:
>> - provide support only for new-vgic
>> - code previously in vgic.c now in vgic_irqfd.c
>>
>> v2 -> v3:
>> - unconditionally set devid and KVM_MSI_VALID_DEVID flag as suggested
>>   by Andre (KVM_IRQ_ROUTING_EXTENDED_MSI type not used anymore)
>> - vgic_irqfd_set_irq now is static
>> - propagate flags
>> - add comments
>>
>> v1 -> v2:
>> - fix bug reported by Andre related to msi.flags and msi.devid setting
>>   in kvm_send_userspace_msi
>> - avoid injecting reserved IRQ numbers in vgic_irqfd_set_irq
>>
>> RFC -> PATCH
>> - reword api.txt:
>>   x move MSI routing comments in a subsequent patch,
>>   x clearly state GSI routing does not apply to KVM_IRQ_LINE
>> ---
>>  Documentation/virtual/kvm/api.txt | 12 --
>>  arch/arm/include/asm/kvm_host.h   |  2 +
>>  arch/arm/kvm/Kconfig  |  2 +
>>  arch/arm/kvm/Makefile |  1 +
>>  arch/arm64/include/asm/kvm_host.h |  1 +
>>  arch/arm64/kvm/Kconfig|  2 +
>>  arch/arm64/kvm/Makefile   |  1 +
>>  virt/kvm/arm/vgic/vgic-init.c | 27 +
>>  virt/kvm/arm/vgic/vgic-irqfd.c| 82 
>> ++-
>>  virt/kvm/arm/vgic/vgic.c  |  7 
>>  virt/kvm/irqchip.c|  2 +
>>  11 files changed, 109 insertions(+), 30 deletions(-)
>>
>> diff --git a/Documentation/virtual/kvm/api.txt 
>> b/Documentation/virtual/kvm/api.txt
>> index 0065c8e..3bb60d3 100644
>> --- a/Documentation/virtual/kvm/api.txt
>> +++ b/Documentation/virtual/kvm/api.txt
>> @@ -1433,13 +1433,16 @@ KVM_ASSIGN_DEV_IRQ. Partial deassignment of host or 
>> guest IRQ is allowed.
>>  4.52 KVM_SET_GSI_ROUTING
>>  
>>  Capability: KVM_CAP_IRQ_ROUTING
>> -Architectures: x86 s390
>> +Architectures: x86 s390 arm arm64
>>  Type: vm ioctl
>>  Parameters: struct kvm_irq_routing (in)
>>  Returns: 0 on success, -1 on error
>>  
>>  Sets the GSI routing table entries, overwriting any previously set entries.
>>  
>> +On arm/arm64, GSI routing has the following limitation:
>> +- GSI routing does not apply to KVM_IRQ_LINE but only to KVM_IRQFD.
>> +
>>  struct kvm_irq_routing {
>>  __u32 nr;
>>  __u32 flags;
>> @@ -2368,9 +2371,10 @@ Note that closing the resamplefd is not sufficient to 
>> disable the
>>  irqfd.  The KVM_IRQFD_FLAG_RESAMPLE is only necessary on assignment
>>  and need not be specified with KVM_IRQFD_FLAG_DEASSIGN.
>>  
>> -On ARM/ARM64, the gsi field in the kvm_irqfd struct specifies the Shared
>> -Peripheral Interrupt (SPI) index, such that the GIC interrupt ID is
>> -given by gsi + 32.
>> +On arm/arm64, gsi routing being supported, the following can happen:
>> +- in case no routing entry is associated to this gsi, injection fails
>> +- in case the gsi is associated to an irqchip routing entry,
>> +  irqchip.pin + 32 corresponds to the injected SPI ID.
>>  
>>  4.76 KVM_PPC_ALLOCATE_HTAB
>>  
>> diff --git a/arch/arm/include/asm/kvm_host.h 
>> b/arch/arm/include/asm/kvm_host.h
>> index 3c40facd..161997e 100644
>> --- a/arch/arm/include/asm/kvm_host.h
>> +++