From mboxrd@z Thu Jan 1 00:00:00 1970 From: eric.auger@linaro.org (Eric Auger) Date: Thu, 11 Jun 2015 19:44:39 +0200 Subject: [PATCH 10/13] KVM: arm64: sync LPI properties and status between guest and KVM In-Reply-To: <1432893209-27313-11-git-send-email-andre.przywara@arm.com> References: <1432893209-27313-1-git-send-email-andre.przywara@arm.com> <1432893209-27313-11-git-send-email-andre.przywara@arm.com> Message-ID: <5579C907.3020700@linaro.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 05/29/2015 11:53 AM, Andre Przywara wrote: > The properties and status of the GICv3 LPIs are hold in tables in > (guest) memory. To achieve reasonable performance, we cache this > data in our own data structures, so we need to sync those two views > from time to time. This behaviour is well described in the GICv3 spec > and is also exercised by hardware, so the sync points are well known. > > Provide functions that read the guest memory and store the > information from the property and status table in the kernel. configuration and pending tables? > > Signed-off-by: Andre Przywara > --- > virt/kvm/arm/its-emul.c | 140 ++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 140 insertions(+) > > diff --git a/virt/kvm/arm/its-emul.c b/virt/kvm/arm/its-emul.c > index f75fb9e..afd440e 100644 > --- a/virt/kvm/arm/its-emul.c > +++ b/virt/kvm/arm/its-emul.c > @@ -50,6 +50,7 @@ struct its_itte { > struct its_collection *collection; > u32 lpi; > u32 event_id; > + u8 priority; > bool enabled; > unsigned long *pending; > }; > @@ -70,7 +71,140 @@ static struct its_itte *find_itte_by_lpi(struct kvm *kvm, int lpi) > return NULL; > } > > +#define LPI_PROP_ENABLE_BIT(p) ((p) & LPI_PROP_ENABLED) > +#define LPI_PROP_PRIORITY(p) ((p) & 0xfc) > + > +/* stores the priority and enable bit for a given LPI */ > +static void update_lpi_property(struct kvm *kvm, struct its_itte *itte, u8 prop) inline? > +{ > + itte->priority = LPI_PROP_PRIORITY(prop); > + itte->enabled = LPI_PROP_ENABLE_BIT(prop); > +} > + > +#define GIC_LPI_OFFSET 8192 > + > +/* We scan the table in chunks the size of the smallest page size */ in 4kB chunks? you can merge that comment with the one below I think > +#define CHUNK_SIZE 4096U > + > #define BASER_BASE_ADDRESS(x) ((x) & 0xfffffffff000ULL) > +#define PROPBASE_TSIZE(x) (1U << (x & 0x1f)) > + > +/* > + * Scan the whole LPI property table and put the LPI configuration it is called configuration table in archi spec. > + * data in our own data structures. This relies on the LPI being > + * mapped before. > + * We scan from two sides: > + * 1) for each byte in the table we care for the ones being enabled > + * 2) for each mapped LPI we look into the table to spot LPIs being disabled > + * Must be called with the ITS lock held. > + */ > +static bool its_update_lpi_properties(struct kvm *kvm) > +{ > + struct vgic_dist *dist = &kvm->arch.vgic; > + u8 *prop; > + u32 tsize; > + gpa_t propbase; > + int lpi = GIC_LPI_OFFSET; > + struct its_itte *itte; > + struct its_device *device; > + int ret; > + > + propbase = BASER_BASE_ADDRESS(dist->propbaser); > + tsize = PROPBASE_TSIZE(dist->propbaser); according to the spec the IDbits should be compared against GICD_TYPER.IDbits and treated accordingly? > + > + prop = kmalloc(CHUNK_SIZE, GFP_KERNEL); > + if (!prop) > + return false; > + > + while (tsize > 0) { > + int chunksize = min(tsize, CHUNK_SIZE); > + > + ret = kvm_read_guest(kvm, propbase, prop, chunksize); > + if (ret) { > + kfree(prop); > + break; > + } although benign, double kfree and we return true; is it what we want? > + > + /* > + * Updating the status for all allocated LPIs. We catch > + * those LPIs that get disabled. We really don't care > + * about unmapped LPIs, as they need to be updated > + * later manually anyway once they get mapped. > + */ > + for_each_lpi(device, itte, kvm) { > + /* > + * Is the LPI covered by that part of the table we > + * are currently looking at? > + */ not sure this comment is needed, although I like comments :-) > + if (itte->lpi < lpi) > + continue; > + if (itte->lpi >= lpi + chunksize) > + continue; could be combined > + > + update_lpi_property(kvm, itte, > + prop[itte->lpi - lpi]); > + } > + tsize -= chunksize; > + lpi += chunksize; > + propbase += chunksize; > + } > + > + kfree(prop); > + return true; > +} > + > +/* > + * Scan the whole LPI pending table and sync the pending bit in there > + * with our own data structures. This relies on the LPI being > + * mapped before. > + * Must be called with the ITS lock held. > + */ > +static bool its_sync_lpi_pending_table(struct kvm_vcpu *vcpu) > +{ > + struct vgic_dist *dist = &vcpu->kvm->arch.vgic; > + unsigned long *pendmask; > + u32 nr_lpis; > + gpa_t pendbase; > + int lpi = GIC_LPI_OFFSET; > + struct its_itte *itte; > + struct its_device *device; > + int ret; > + int lpi_bit, nr_bits; > + > + pendbase = BASER_BASE_ADDRESS(dist->pendbaser[vcpu->vcpu_id]); archi spec says the first 1kB of the table corresponds to other classes of IRQs. where is this offset applied? > + nr_lpis = GIC_LPI_OFFSET; > + > + pendmask = kmalloc(CHUNK_SIZE, GFP_KERNEL); > + if (!pendmask) > + return false; > + > + while (nr_lpis > 0) { > + nr_bits = min(nr_lpis, CHUNK_SIZE * 8); > + > + ret = kvm_read_guest(vcpu->kvm, pendbase, pendmask, > + nr_bits / 8); > + if (ret) > + break; return false? > + > + for_each_lpi(device, itte, vcpu->kvm) { > + lpi_bit = itte->lpi - lpi; > + if (lpi_bit < 0) > + continue; > + if (lpi_bit >= nr_bits) > + continue; > + if (test_bit(lpi_bit, pendmask)) > + set_bit(vcpu->vcpu_id, itte->pending); > + else > + clear_bit(vcpu->vcpu_id, itte->pending); > + } > + nr_lpis -= nr_bits; > + lpi += nr_bits; > + pendbase += nr_bits / 8; > + } > + > + kfree(pendmask); > + return true; > +} > > /* distributor lock is hold by the VGIC MMIO handler */ > static bool handle_mmio_misc_gits(struct kvm_vcpu *vcpu, > @@ -350,6 +484,12 @@ static const struct vgic_io_range vgicv3_its_ranges[] = { > > void vgic_enable_lpis(struct kvm_vcpu *vcpu) > { > + struct vgic_its *its = &vcpu->kvm->arch.vgic.its; > + > + spin_lock(&its->lock); > + its_update_lpi_properties(vcpu->kvm); > + its_sync_lpi_pending_table(vcpu); question of the flush ... Cheers Eric > + spin_unlock(&its->lock); > } > > int vits_init(struct kvm *kvm) > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Auger Subject: Re: [PATCH 10/13] KVM: arm64: sync LPI properties and status between guest and KVM Date: Thu, 11 Jun 2015 19:44:39 +0200 Message-ID: <5579C907.3020700@linaro.org> References: <1432893209-27313-1-git-send-email-andre.przywara@arm.com> <1432893209-27313-11-git-send-email-andre.przywara@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id BBC3954D80 for ; Thu, 11 Jun 2015 13:34:31 -0400 (EDT) Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id ym+vZeb+b7FE for ; Thu, 11 Jun 2015 13:34:30 -0400 (EDT) Received: from mail-wi0-f182.google.com (mail-wi0-f182.google.com [209.85.212.182]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id 96B4554D7F for ; Thu, 11 Jun 2015 13:34:30 -0400 (EDT) Received: by wibdq8 with SMTP id dq8so15091771wib.1 for ; Thu, 11 Jun 2015 10:44:52 -0700 (PDT) In-Reply-To: <1432893209-27313-11-git-send-email-andre.przywara@arm.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu To: Andre Przywara , christoffer.dall@linaro.org, marc.zyngier@arm.com Cc: kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org List-Id: kvmarm@lists.cs.columbia.edu On 05/29/2015 11:53 AM, Andre Przywara wrote: > The properties and status of the GICv3 LPIs are hold in tables in > (guest) memory. To achieve reasonable performance, we cache this > data in our own data structures, so we need to sync those two views > from time to time. This behaviour is well described in the GICv3 spec > and is also exercised by hardware, so the sync points are well known. > > Provide functions that read the guest memory and store the > information from the property and status table in the kernel. configuration and pending tables? > > Signed-off-by: Andre Przywara > --- > virt/kvm/arm/its-emul.c | 140 ++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 140 insertions(+) > > diff --git a/virt/kvm/arm/its-emul.c b/virt/kvm/arm/its-emul.c > index f75fb9e..afd440e 100644 > --- a/virt/kvm/arm/its-emul.c > +++ b/virt/kvm/arm/its-emul.c > @@ -50,6 +50,7 @@ struct its_itte { > struct its_collection *collection; > u32 lpi; > u32 event_id; > + u8 priority; > bool enabled; > unsigned long *pending; > }; > @@ -70,7 +71,140 @@ static struct its_itte *find_itte_by_lpi(struct kvm *kvm, int lpi) > return NULL; > } > > +#define LPI_PROP_ENABLE_BIT(p) ((p) & LPI_PROP_ENABLED) > +#define LPI_PROP_PRIORITY(p) ((p) & 0xfc) > + > +/* stores the priority and enable bit for a given LPI */ > +static void update_lpi_property(struct kvm *kvm, struct its_itte *itte, u8 prop) inline? > +{ > + itte->priority = LPI_PROP_PRIORITY(prop); > + itte->enabled = LPI_PROP_ENABLE_BIT(prop); > +} > + > +#define GIC_LPI_OFFSET 8192 > + > +/* We scan the table in chunks the size of the smallest page size */ in 4kB chunks? you can merge that comment with the one below I think > +#define CHUNK_SIZE 4096U > + > #define BASER_BASE_ADDRESS(x) ((x) & 0xfffffffff000ULL) > +#define PROPBASE_TSIZE(x) (1U << (x & 0x1f)) > + > +/* > + * Scan the whole LPI property table and put the LPI configuration it is called configuration table in archi spec. > + * data in our own data structures. This relies on the LPI being > + * mapped before. > + * We scan from two sides: > + * 1) for each byte in the table we care for the ones being enabled > + * 2) for each mapped LPI we look into the table to spot LPIs being disabled > + * Must be called with the ITS lock held. > + */ > +static bool its_update_lpi_properties(struct kvm *kvm) > +{ > + struct vgic_dist *dist = &kvm->arch.vgic; > + u8 *prop; > + u32 tsize; > + gpa_t propbase; > + int lpi = GIC_LPI_OFFSET; > + struct its_itte *itte; > + struct its_device *device; > + int ret; > + > + propbase = BASER_BASE_ADDRESS(dist->propbaser); > + tsize = PROPBASE_TSIZE(dist->propbaser); according to the spec the IDbits should be compared against GICD_TYPER.IDbits and treated accordingly? > + > + prop = kmalloc(CHUNK_SIZE, GFP_KERNEL); > + if (!prop) > + return false; > + > + while (tsize > 0) { > + int chunksize = min(tsize, CHUNK_SIZE); > + > + ret = kvm_read_guest(kvm, propbase, prop, chunksize); > + if (ret) { > + kfree(prop); > + break; > + } although benign, double kfree and we return true; is it what we want? > + > + /* > + * Updating the status for all allocated LPIs. We catch > + * those LPIs that get disabled. We really don't care > + * about unmapped LPIs, as they need to be updated > + * later manually anyway once they get mapped. > + */ > + for_each_lpi(device, itte, kvm) { > + /* > + * Is the LPI covered by that part of the table we > + * are currently looking at? > + */ not sure this comment is needed, although I like comments :-) > + if (itte->lpi < lpi) > + continue; > + if (itte->lpi >= lpi + chunksize) > + continue; could be combined > + > + update_lpi_property(kvm, itte, > + prop[itte->lpi - lpi]); > + } > + tsize -= chunksize; > + lpi += chunksize; > + propbase += chunksize; > + } > + > + kfree(prop); > + return true; > +} > + > +/* > + * Scan the whole LPI pending table and sync the pending bit in there > + * with our own data structures. This relies on the LPI being > + * mapped before. > + * Must be called with the ITS lock held. > + */ > +static bool its_sync_lpi_pending_table(struct kvm_vcpu *vcpu) > +{ > + struct vgic_dist *dist = &vcpu->kvm->arch.vgic; > + unsigned long *pendmask; > + u32 nr_lpis; > + gpa_t pendbase; > + int lpi = GIC_LPI_OFFSET; > + struct its_itte *itte; > + struct its_device *device; > + int ret; > + int lpi_bit, nr_bits; > + > + pendbase = BASER_BASE_ADDRESS(dist->pendbaser[vcpu->vcpu_id]); archi spec says the first 1kB of the table corresponds to other classes of IRQs. where is this offset applied? > + nr_lpis = GIC_LPI_OFFSET; > + > + pendmask = kmalloc(CHUNK_SIZE, GFP_KERNEL); > + if (!pendmask) > + return false; > + > + while (nr_lpis > 0) { > + nr_bits = min(nr_lpis, CHUNK_SIZE * 8); > + > + ret = kvm_read_guest(vcpu->kvm, pendbase, pendmask, > + nr_bits / 8); > + if (ret) > + break; return false? > + > + for_each_lpi(device, itte, vcpu->kvm) { > + lpi_bit = itte->lpi - lpi; > + if (lpi_bit < 0) > + continue; > + if (lpi_bit >= nr_bits) > + continue; > + if (test_bit(lpi_bit, pendmask)) > + set_bit(vcpu->vcpu_id, itte->pending); > + else > + clear_bit(vcpu->vcpu_id, itte->pending); > + } > + nr_lpis -= nr_bits; > + lpi += nr_bits; > + pendbase += nr_bits / 8; > + } > + > + kfree(pendmask); > + return true; > +} > > /* distributor lock is hold by the VGIC MMIO handler */ > static bool handle_mmio_misc_gits(struct kvm_vcpu *vcpu, > @@ -350,6 +484,12 @@ static const struct vgic_io_range vgicv3_its_ranges[] = { > > void vgic_enable_lpis(struct kvm_vcpu *vcpu) > { > + struct vgic_its *its = &vcpu->kvm->arch.vgic.its; > + > + spin_lock(&its->lock); > + its_update_lpi_properties(vcpu->kvm); > + its_sync_lpi_pending_table(vcpu); question of the flush ... Cheers Eric > + spin_unlock(&its->lock); > } > > int vits_init(struct kvm *kvm) >