| Message ID | 20160713015909.28793-15-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 13/07/16 02:59, Andre Przywara wrote: > The (system-wide) LPI configuration table is held in a table in > (guest) memory. To achieve reasonable performance, we cache this data > in our struct vgic_irq. If the guest updates the configuration data > (which consists of the enable bit and the priority value), it issues > an INV or INVALL command to allow us to update our information. > Provide functions that update that information for one LPI or all LPIs > mapped to a specific collection. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > virt/kvm/arm/vgic/vgic-its.c | 39 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c > index f400ef1..60108f8 100644 > --- a/virt/kvm/arm/vgic/vgic-its.c > +++ b/virt/kvm/arm/vgic/vgic-its.c > @@ -64,6 +64,45 @@ struct its_itte { > > #define CBASER_ADDRESS(x) ((x) & GENMASK_ULL(51, 12)) > #define PENDBASER_ADDRESS(x) ((x) & GENMASK_ULL(51, 16)) > +#define PROPBASER_ADDRESS(x) ((x) & GENMASK_ULL(51, 12)) Again, these masks are wrong as we limit the ITS to 48 bits PA. > + > +#define GIC_LPI_OFFSET 8192 > + > +#define LPI_PROP_ENABLE_BIT(p) ((p) & LPI_PROP_ENABLED) > +#define LPI_PROP_PRIORITY(p) ((p) & 0xfc) > + > +/* > + * Reads the configuration data for a given LPI from guest memory and > + * updates the fields in struct vgic_irq. > + * If filter_vcpu is not NULL, applies only if the IRQ is targeting this > + * VCPU. Unconditionally applies if filter_vcpu is NULL. > + */ > +static int update_lpi_config(struct kvm *kvm, struct vgic_irq *irq, > + struct kvm_vcpu *filter_vcpu) > +{ > + u64 propbase = PROPBASER_ADDRESS(kvm->arch.vgic.propbaser); > + u8 prop; > + int ret; > + > + ret = kvm_read_guest(kvm, propbase + irq->intid - GIC_LPI_OFFSET, > + &prop, 1); > + > + if (ret) > + return ret; > + > + spin_lock(&irq->irq_lock); > + > + if (!filter_vcpu || filter_vcpu == irq->target_vcpu) { > + irq->priority = LPI_PROP_PRIORITY(prop); > + irq->enabled = LPI_PROP_ENABLE_BIT(prop); > + > + vgic_queue_irq_unlock(kvm, irq); > + } else { > + spin_unlock(&irq->irq_lock); > + } > + > + return 0; > +} > > /* > * Create a snapshot of the current LPI list, so that we can enumerate all > Thanks, M.
Hi, On 14/07/16 10:46, Marc Zyngier wrote: > On 13/07/16 02:59, Andre Przywara wrote: >> The (system-wide) LPI configuration table is held in a table in >> (guest) memory. To achieve reasonable performance, we cache this data >> in our struct vgic_irq. If the guest updates the configuration data >> (which consists of the enable bit and the priority value), it issues >> an INV or INVALL command to allow us to update our information. >> Provide functions that update that information for one LPI or all LPIs >> mapped to a specific collection. >> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> --- >> virt/kvm/arm/vgic/vgic-its.c | 39 +++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 39 insertions(+) >> >> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c >> index f400ef1..60108f8 100644 >> --- a/virt/kvm/arm/vgic/vgic-its.c >> +++ b/virt/kvm/arm/vgic/vgic-its.c >> @@ -64,6 +64,45 @@ struct its_itte { >> >> #define CBASER_ADDRESS(x) ((x) & GENMASK_ULL(51, 12)) >> #define PENDBASER_ADDRESS(x) ((x) & GENMASK_ULL(51, 16)) >> +#define PROPBASER_ADDRESS(x) ((x) & GENMASK_ULL(51, 12)) > > Again, these masks are wrong as we limit the ITS to 48 bits PA. Those masks match the architecture description of the register. We limit to 48 bits of PA when the guest writes to those registers. I'd like to keep this limitation in one place only. Cheers, Andre.
On 14/07/16 11:00, Andre Przywara wrote: > Hi, > > On 14/07/16 10:46, Marc Zyngier wrote: >> On 13/07/16 02:59, Andre Przywara wrote: >>> The (system-wide) LPI configuration table is held in a table in >>> (guest) memory. To achieve reasonable performance, we cache this data >>> in our struct vgic_irq. If the guest updates the configuration data >>> (which consists of the enable bit and the priority value), it issues >>> an INV or INVALL command to allow us to update our information. >>> Provide functions that update that information for one LPI or all LPIs >>> mapped to a specific collection. >>> >>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>> --- >>> virt/kvm/arm/vgic/vgic-its.c | 39 +++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 39 insertions(+) >>> >>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c >>> index f400ef1..60108f8 100644 >>> --- a/virt/kvm/arm/vgic/vgic-its.c >>> +++ b/virt/kvm/arm/vgic/vgic-its.c >>> @@ -64,6 +64,45 @@ struct its_itte { >>> >>> #define CBASER_ADDRESS(x) ((x) & GENMASK_ULL(51, 12)) >>> #define PENDBASER_ADDRESS(x) ((x) & GENMASK_ULL(51, 16)) >>> +#define PROPBASER_ADDRESS(x) ((x) & GENMASK_ULL(51, 12)) >> >> Again, these masks are wrong as we limit the ITS to 48 bits PA. > > Those masks match the architecture description of the register. We limit > to 48 bits of PA when the guest writes to those registers. I'd like to > keep this limitation in one place only. Which makes it hard to understand for everyone. If you want something truly generic, assign proper names to these defines: #define CBASER_ARCHITECTURAL_PA_BITS 52 #define CBASER_IMPLEMENTATION_PA_BITS 48 and repaint all your defines to use macros along those lines. If not, just change everything to 48 and let's be done with it. Thanks, M.
diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c index f400ef1..60108f8 100644 --- a/virt/kvm/arm/vgic/vgic-its.c +++ b/virt/kvm/arm/vgic/vgic-its.c @@ -64,6 +64,45 @@ struct its_itte { #define CBASER_ADDRESS(x) ((x) & GENMASK_ULL(51, 12)) #define PENDBASER_ADDRESS(x) ((x) & GENMASK_ULL(51, 16)) +#define PROPBASER_ADDRESS(x) ((x) & GENMASK_ULL(51, 12)) + +#define GIC_LPI_OFFSET 8192 + +#define LPI_PROP_ENABLE_BIT(p) ((p) & LPI_PROP_ENABLED) +#define LPI_PROP_PRIORITY(p) ((p) & 0xfc) + +/* + * Reads the configuration data for a given LPI from guest memory and + * updates the fields in struct vgic_irq. + * If filter_vcpu is not NULL, applies only if the IRQ is targeting this + * VCPU. Unconditionally applies if filter_vcpu is NULL. + */ +static int update_lpi_config(struct kvm *kvm, struct vgic_irq *irq, + struct kvm_vcpu *filter_vcpu) +{ + u64 propbase = PROPBASER_ADDRESS(kvm->arch.vgic.propbaser); + u8 prop; + int ret; + + ret = kvm_read_guest(kvm, propbase + irq->intid - GIC_LPI_OFFSET, + &prop, 1); + + if (ret) + return ret; + + spin_lock(&irq->irq_lock); + + if (!filter_vcpu || filter_vcpu == irq->target_vcpu) { + irq->priority = LPI_PROP_PRIORITY(prop); + irq->enabled = LPI_PROP_ENABLE_BIT(prop); + + vgic_queue_irq_unlock(kvm, irq); + } else { + spin_unlock(&irq->irq_lock); + } + + return 0; +} /* * Create a snapshot of the current LPI list, so that we can enumerate all
The (system-wide) LPI configuration table is held in a table in (guest) memory. To achieve reasonable performance, we cache this data in our struct vgic_irq. If the guest updates the configuration data (which consists of the enable bit and the priority value), it issues an INV or INVALL command to allow us to update our information. Provide functions that update that information for one LPI or all LPIs mapped to a specific collection. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- virt/kvm/arm/vgic/vgic-its.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)