| Message ID | 1462531568-9799-25-git-send-email-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/05/16 11:45, Andre Przywara wrote: > As the enable register handlers are shared between the v2 and v3 > emulation, their implementation goes into vgic-mmio.c, to be easily > referenced from the v3 emulation as well later. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> M.
On Fri, May 06, 2016 at 11:45:37AM +0100, Andre Przywara wrote: > As the enable register handlers are shared between the v2 and v3 > emulation, their implementation goes into vgic-mmio.c, to be easily > referenced from the v3 emulation as well later. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > Changelog RFC..v1: > - use lower bits of address to determine IRQ number > - remove TODO, confirmed to be fine > > Changelog v1 .. v2: > - adapt to new MMIO framework > > virt/kvm/arm/vgic/vgic-mmio-v2.c | 4 +-- > virt/kvm/arm/vgic/vgic-mmio.c | 56 ++++++++++++++++++++++++++++++++++++++++ > virt/kvm/arm/vgic/vgic-mmio.h | 11 ++++++++ > 3 files changed, 69 insertions(+), 2 deletions(-) > > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c > index 69e96f7..448d1da 100644 > --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c > +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c > @@ -72,9 +72,9 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = { > REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_IGROUP, > vgic_mmio_read_rao, vgic_mmio_write_wi, 1), > REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_SET, > - vgic_mmio_read_raz, vgic_mmio_write_wi, 1), > + vgic_mmio_read_enable, vgic_mmio_write_senable, 1), > REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_CLEAR, > - vgic_mmio_read_raz, vgic_mmio_write_wi, 1), > + vgic_mmio_read_enable, vgic_mmio_write_cenable, 1), > REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_SET, > vgic_mmio_read_raz, vgic_mmio_write_wi, 1), > REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_CLEAR, > diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c > index 41cf4f4..077ae86 100644 > --- a/virt/kvm/arm/vgic/vgic-mmio.c > +++ b/virt/kvm/arm/vgic/vgic-mmio.c > @@ -46,6 +46,62 @@ void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr, > /* Ignore */ > } > > +/* > + * Read accesses to both GICD_ICENABLER and GICD_ISENABLER return the value > + * of the enabled bit, so there is only one function for both here. > + */ > +unsigned long vgic_mmio_read_enable(struct kvm_vcpu *vcpu, > + gpa_t addr, unsigned int len) > +{ > + u32 intid = (addr & 0x7f) * 8; is there anything we can do about this to make it more intuitive? A macro to generate the mask/offset based on bits per interrupt or something? > + u32 value = 0; > + int i; > + > + /* Loop over all IRQs affected by this read */ > + for (i = 0; i < len * 8; i++) { > + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); > + > + if (irq->enabled) > + value |= (1U << i); I couldn't find the code anywhere that enforces word-aligned accesses to these registers. Do we have that? If that's not the case, doesn't this break of you do a non-word aligned access? > + } > + > + return extract_bytes(value, addr & 3, len); > +} > + > +void vgic_mmio_write_senable(struct kvm_vcpu *vcpu, > + gpa_t addr, unsigned int len, > + unsigned long val) > +{ > + u32 intid = (addr & 0x7f) * 8; > + int i; > + > + for_each_set_bit(i, &val, len * 8) { > + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); > + > + spin_lock(&irq->irq_lock); > + irq->enabled = true; > + vgic_queue_irq_unlock(vcpu->kvm, irq); > + } > +} > + > +void vgic_mmio_write_cenable(struct kvm_vcpu *vcpu, > + gpa_t addr, unsigned int len, > + unsigned long val) > +{ > + u32 intid = (addr & 0x7f) * 8; > + int i; > + > + for_each_set_bit(i, &val, len * 8) { > + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); > + > + spin_lock(&irq->irq_lock); > + > + irq->enabled = false; > + > + spin_unlock(&irq->irq_lock); nit: whitespace consistency with senable > + } > +} > + > static int match_region(const void *key, const void *elt) > { > const unsigned int offset = (unsigned long)key; > diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h > index 4f4dd2b..188909a 100644 > --- a/virt/kvm/arm/vgic/vgic-mmio.h > +++ b/virt/kvm/arm/vgic/vgic-mmio.h > @@ -74,6 +74,17 @@ unsigned long vgic_mmio_read_rao(struct kvm_vcpu *vcpu, > void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr, > unsigned int len, unsigned long val); > > +unsigned long vgic_mmio_read_enable(struct kvm_vcpu *vcpu, > + gpa_t addr, unsigned int len); > + > +void vgic_mmio_write_senable(struct kvm_vcpu *vcpu, > + gpa_t addr, unsigned int len, > + unsigned long val); > + > +void vgic_mmio_write_cenable(struct kvm_vcpu *vcpu, > + gpa_t addr, unsigned int len, > + unsigned long val); > + > unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev); > > #endif > -- > 2.7.3 > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 11/05/16 13:34, Christoffer Dall wrote: > On Fri, May 06, 2016 at 11:45:37AM +0100, Andre Przywara wrote: >> As the enable register handlers are shared between the v2 and v3 >> emulation, their implementation goes into vgic-mmio.c, to be easily >> referenced from the v3 emulation as well later. >> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> --- >> Changelog RFC..v1: >> - use lower bits of address to determine IRQ number >> - remove TODO, confirmed to be fine >> >> Changelog v1 .. v2: >> - adapt to new MMIO framework >> >> virt/kvm/arm/vgic/vgic-mmio-v2.c | 4 +-- >> virt/kvm/arm/vgic/vgic-mmio.c | 56 ++++++++++++++++++++++++++++++++++++++++ >> virt/kvm/arm/vgic/vgic-mmio.h | 11 ++++++++ >> 3 files changed, 69 insertions(+), 2 deletions(-) >> >> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c >> index 69e96f7..448d1da 100644 >> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c >> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c >> @@ -72,9 +72,9 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = { >> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_IGROUP, >> vgic_mmio_read_rao, vgic_mmio_write_wi, 1), >> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_SET, >> - vgic_mmio_read_raz, vgic_mmio_write_wi, 1), >> + vgic_mmio_read_enable, vgic_mmio_write_senable, 1), >> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_CLEAR, >> - vgic_mmio_read_raz, vgic_mmio_write_wi, 1), >> + vgic_mmio_read_enable, vgic_mmio_write_cenable, 1), >> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_SET, >> vgic_mmio_read_raz, vgic_mmio_write_wi, 1), >> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_CLEAR, >> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c >> index 41cf4f4..077ae86 100644 >> --- a/virt/kvm/arm/vgic/vgic-mmio.c >> +++ b/virt/kvm/arm/vgic/vgic-mmio.c >> @@ -46,6 +46,62 @@ void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr, >> /* Ignore */ >> } >> >> +/* >> + * Read accesses to both GICD_ICENABLER and GICD_ISENABLER return the value >> + * of the enabled bit, so there is only one function for both here. >> + */ >> +unsigned long vgic_mmio_read_enable(struct kvm_vcpu *vcpu, >> + gpa_t addr, unsigned int len) >> +{ >> + u32 intid = (addr & 0x7f) * 8; > > is there anything we can do about this to make it more intuitive? A > macro to generate the mask/offset based on bits per interrupt or > something? Yes, something where you give it the address and the bits-per-IRQ and it tells you the IRQ number. Not sure it is advisable to squash this into v4 still? > >> + u32 value = 0; >> + int i; >> + >> + /* Loop over all IRQs affected by this read */ >> + for (i = 0; i < len * 8; i++) { >> + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); >> + >> + if (irq->enabled) >> + value |= (1U << i); > > I couldn't find the code anywhere that enforces word-aligned accesses to > these registers. Do we have that? Not that I am aware of. I was suggesting this since we have one in the IROUTER function. Architecturally we don't need to support halfword accesses, it's: byte + word, word only or double-word + word, depending on the actual register, IIRC. As a fix we can at least deny (read: ignore) halfword accesses in general in the dispatcher. Shall I do this (two two-liners)? I think byte and word accesses are safe with the existing handlers last time I checked. > If that's not the case, doesn't this break of you do a non-word aligned > access? Why would it? vgic_data_host_to_mmio_bus and extract_bytes should cover this, shouldn't they? Cheers, Andre. > >> + } >> + >> + return extract_bytes(value, addr & 3, len); >> +} >> + >> +void vgic_mmio_write_senable(struct kvm_vcpu *vcpu, >> + gpa_t addr, unsigned int len, >> + unsigned long val) >> +{ >> + u32 intid = (addr & 0x7f) * 8; >> + int i; >> + >> + for_each_set_bit(i, &val, len * 8) { >> + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); >> + >> + spin_lock(&irq->irq_lock); >> + irq->enabled = true; >> + vgic_queue_irq_unlock(vcpu->kvm, irq); >> + } >> +} >> + >> +void vgic_mmio_write_cenable(struct kvm_vcpu *vcpu, >> + gpa_t addr, unsigned int len, >> + unsigned long val) >> +{ >> + u32 intid = (addr & 0x7f) * 8; >> + int i; >> + >> + for_each_set_bit(i, &val, len * 8) { >> + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); >> + >> + spin_lock(&irq->irq_lock); >> + >> + irq->enabled = false; >> + >> + spin_unlock(&irq->irq_lock); > > nit: whitespace consistency with senable > >> + } >> +} >> + >> static int match_region(const void *key, const void *elt) >> { >> const unsigned int offset = (unsigned long)key; >> diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h >> index 4f4dd2b..188909a 100644 >> --- a/virt/kvm/arm/vgic/vgic-mmio.h >> +++ b/virt/kvm/arm/vgic/vgic-mmio.h >> @@ -74,6 +74,17 @@ unsigned long vgic_mmio_read_rao(struct kvm_vcpu *vcpu, >> void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr, >> unsigned int len, unsigned long val); >> >> +unsigned long vgic_mmio_read_enable(struct kvm_vcpu *vcpu, >> + gpa_t addr, unsigned int len); >> + >> +void vgic_mmio_write_senable(struct kvm_vcpu *vcpu, >> + gpa_t addr, unsigned int len, >> + unsigned long val); >> + >> +void vgic_mmio_write_cenable(struct kvm_vcpu *vcpu, >> + gpa_t addr, unsigned int len, >> + unsigned long val); >> + >> unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev); >> >> #endif >> -- >> 2.7.3 >> > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/05/16 13:34, Christoffer Dall wrote: > On Fri, May 06, 2016 at 11:45:37AM +0100, Andre Przywara wrote: >> As the enable register handlers are shared between the v2 and v3 >> emulation, their implementation goes into vgic-mmio.c, to be easily >> referenced from the v3 emulation as well later. >> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> --- >> Changelog RFC..v1: >> - use lower bits of address to determine IRQ number >> - remove TODO, confirmed to be fine >> >> Changelog v1 .. v2: >> - adapt to new MMIO framework >> >> virt/kvm/arm/vgic/vgic-mmio-v2.c | 4 +-- >> virt/kvm/arm/vgic/vgic-mmio.c | 56 ++++++++++++++++++++++++++++++++++++++++ >> virt/kvm/arm/vgic/vgic-mmio.h | 11 ++++++++ >> 3 files changed, 69 insertions(+), 2 deletions(-) >> >> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c >> index 69e96f7..448d1da 100644 >> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c >> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c >> @@ -72,9 +72,9 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = { >> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_IGROUP, >> vgic_mmio_read_rao, vgic_mmio_write_wi, 1), >> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_SET, >> - vgic_mmio_read_raz, vgic_mmio_write_wi, 1), >> + vgic_mmio_read_enable, vgic_mmio_write_senable, 1), >> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_CLEAR, >> - vgic_mmio_read_raz, vgic_mmio_write_wi, 1), >> + vgic_mmio_read_enable, vgic_mmio_write_cenable, 1), >> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_SET, >> vgic_mmio_read_raz, vgic_mmio_write_wi, 1), >> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_CLEAR, >> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c >> index 41cf4f4..077ae86 100644 >> --- a/virt/kvm/arm/vgic/vgic-mmio.c >> +++ b/virt/kvm/arm/vgic/vgic-mmio.c >> @@ -46,6 +46,62 @@ void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr, >> /* Ignore */ >> } >> >> +/* >> + * Read accesses to both GICD_ICENABLER and GICD_ISENABLER return the value >> + * of the enabled bit, so there is only one function for both here. >> + */ >> +unsigned long vgic_mmio_read_enable(struct kvm_vcpu *vcpu, >> + gpa_t addr, unsigned int len) >> +{ >> + u32 intid = (addr & 0x7f) * 8; > > is there anything we can do about this to make it more intuitive? A > macro to generate the mask/offset based on bits per interrupt or > something? Untested (and thus probably wrong): #define VGIC_ADDR_IRQ_MASK(bits) ((1024 - 1) >> (4 - (bits)) #define VGIC_ADDR_IRQ_MULT(bits) (8 / (bits)) #define VGIC_ADDR_TO_INTID(addr, bits) (((addr) & VGIC_ADDR_IRQ_MASK(bits)) * \ VGIC_ADDR_IRQ_MULT(bits)) u32 intid = VGIC_ADDR_TO_INTID(addr, 1); Thanks, M.
On Wed, May 11, 2016 at 02:04:13PM +0100, Andre Przywara wrote: > Hi, > > On 11/05/16 13:34, Christoffer Dall wrote: > > On Fri, May 06, 2016 at 11:45:37AM +0100, Andre Przywara wrote: > >> As the enable register handlers are shared between the v2 and v3 > >> emulation, their implementation goes into vgic-mmio.c, to be easily > >> referenced from the v3 emulation as well later. > >> > >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> > >> --- > >> Changelog RFC..v1: > >> - use lower bits of address to determine IRQ number > >> - remove TODO, confirmed to be fine > >> > >> Changelog v1 .. v2: > >> - adapt to new MMIO framework > >> > >> virt/kvm/arm/vgic/vgic-mmio-v2.c | 4 +-- > >> virt/kvm/arm/vgic/vgic-mmio.c | 56 ++++++++++++++++++++++++++++++++++++++++ > >> virt/kvm/arm/vgic/vgic-mmio.h | 11 ++++++++ > >> 3 files changed, 69 insertions(+), 2 deletions(-) > >> > >> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c > >> index 69e96f7..448d1da 100644 > >> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c > >> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c > >> @@ -72,9 +72,9 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = { > >> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_IGROUP, > >> vgic_mmio_read_rao, vgic_mmio_write_wi, 1), > >> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_SET, > >> - vgic_mmio_read_raz, vgic_mmio_write_wi, 1), > >> + vgic_mmio_read_enable, vgic_mmio_write_senable, 1), > >> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_CLEAR, > >> - vgic_mmio_read_raz, vgic_mmio_write_wi, 1), > >> + vgic_mmio_read_enable, vgic_mmio_write_cenable, 1), > >> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_SET, > >> vgic_mmio_read_raz, vgic_mmio_write_wi, 1), > >> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_CLEAR, > >> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c > >> index 41cf4f4..077ae86 100644 > >> --- a/virt/kvm/arm/vgic/vgic-mmio.c > >> +++ b/virt/kvm/arm/vgic/vgic-mmio.c > >> @@ -46,6 +46,62 @@ void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr, > >> /* Ignore */ > >> } > >> > >> +/* > >> + * Read accesses to both GICD_ICENABLER and GICD_ISENABLER return the value > >> + * of the enabled bit, so there is only one function for both here. > >> + */ > >> +unsigned long vgic_mmio_read_enable(struct kvm_vcpu *vcpu, > >> + gpa_t addr, unsigned int len) > >> +{ > >> + u32 intid = (addr & 0x7f) * 8; > > > > is there anything we can do about this to make it more intuitive? A > > macro to generate the mask/offset based on bits per interrupt or > > something? > > Yes, something where you give it the address and the bits-per-IRQ and it > tells you the IRQ number. > Not sure it is advisable to squash this into v4 still? > > > > >> + u32 value = 0; > >> + int i; > >> + > >> + /* Loop over all IRQs affected by this read */ > >> + for (i = 0; i < len * 8; i++) { > >> + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); > >> + > >> + if (irq->enabled) > >> + value |= (1U << i); > > > > I couldn't find the code anywhere that enforces word-aligned accesses to > > these registers. Do we have that? > > Not that I am aware of. I was suggesting this since we have one in the > IROUTER function. Architecturally we don't need to support halfword > accesses, it's: byte + word, word only or double-word + word, depending > on the actual register, IIRC. > As a fix we can at least deny (read: ignore) halfword accesses in > general in the dispatcher. Shall I do this (two two-liners)? > I think byte and word accesses are safe with the existing handlers last > time I checked. > > > If that's not the case, doesn't this break of you do a non-word aligned > > access? > > Why would it? vgic_data_host_to_mmio_bus and extract_bytes should cover > this, shouldn't they? > I think this breaks on a simple byte access. Let's say you are accessing byte 1 (addr & 0x7ff == 1), then because you start your loop at 0, you're going to set bits [7:0] in the value variable, and then extract bits [15:8] in extract_bytes(), right? -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 11, 2016 at 02:04:13PM +0100, Andre Przywara wrote: > Hi, > > On 11/05/16 13:34, Christoffer Dall wrote: > > On Fri, May 06, 2016 at 11:45:37AM +0100, Andre Przywara wrote: > >> As the enable register handlers are shared between the v2 and v3 > >> emulation, their implementation goes into vgic-mmio.c, to be easily > >> referenced from the v3 emulation as well later. > >> > >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> > >> --- > >> Changelog RFC..v1: > >> - use lower bits of address to determine IRQ number > >> - remove TODO, confirmed to be fine > >> > >> Changelog v1 .. v2: > >> - adapt to new MMIO framework > >> > >> virt/kvm/arm/vgic/vgic-mmio-v2.c | 4 +-- > >> virt/kvm/arm/vgic/vgic-mmio.c | 56 ++++++++++++++++++++++++++++++++++++++++ > >> virt/kvm/arm/vgic/vgic-mmio.h | 11 ++++++++ > >> 3 files changed, 69 insertions(+), 2 deletions(-) > >> > >> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c > >> index 69e96f7..448d1da 100644 > >> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c > >> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c > >> @@ -72,9 +72,9 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = { > >> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_IGROUP, > >> vgic_mmio_read_rao, vgic_mmio_write_wi, 1), > >> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_SET, > >> - vgic_mmio_read_raz, vgic_mmio_write_wi, 1), > >> + vgic_mmio_read_enable, vgic_mmio_write_senable, 1), > >> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_CLEAR, > >> - vgic_mmio_read_raz, vgic_mmio_write_wi, 1), > >> + vgic_mmio_read_enable, vgic_mmio_write_cenable, 1), > >> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_SET, > >> vgic_mmio_read_raz, vgic_mmio_write_wi, 1), > >> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_CLEAR, > >> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c > >> index 41cf4f4..077ae86 100644 > >> --- a/virt/kvm/arm/vgic/vgic-mmio.c > >> +++ b/virt/kvm/arm/vgic/vgic-mmio.c > >> @@ -46,6 +46,62 @@ void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr, > >> /* Ignore */ > >> } > >> > >> +/* > >> + * Read accesses to both GICD_ICENABLER and GICD_ISENABLER return the value > >> + * of the enabled bit, so there is only one function for both here. > >> + */ > >> +unsigned long vgic_mmio_read_enable(struct kvm_vcpu *vcpu, > >> + gpa_t addr, unsigned int len) > >> +{ > >> + u32 intid = (addr & 0x7f) * 8; > > > > is there anything we can do about this to make it more intuitive? A > > macro to generate the mask/offset based on bits per interrupt or > > something? > > Yes, something where you give it the address and the bits-per-IRQ and it > tells you the IRQ number. > Not sure it is advisable to squash this into v4 still? > I think it should be fairly mechanical and easy to squash, so would prefer it, but if it gives you hours of headache, maybe not. -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/05/16 14:14, Christoffer Dall wrote: > On Wed, May 11, 2016 at 02:04:13PM +0100, Andre Przywara wrote: >> Hi, >> >> On 11/05/16 13:34, Christoffer Dall wrote: >>> On Fri, May 06, 2016 at 11:45:37AM +0100, Andre Przywara wrote: >>>> As the enable register handlers are shared between the v2 and v3 >>>> emulation, their implementation goes into vgic-mmio.c, to be easily >>>> referenced from the v3 emulation as well later. >>>> >>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>>> --- >>>> Changelog RFC..v1: >>>> - use lower bits of address to determine IRQ number >>>> - remove TODO, confirmed to be fine >>>> >>>> Changelog v1 .. v2: >>>> - adapt to new MMIO framework >>>> >>>> virt/kvm/arm/vgic/vgic-mmio-v2.c | 4 +-- >>>> virt/kvm/arm/vgic/vgic-mmio.c | 56 ++++++++++++++++++++++++++++++++++++++++ >>>> virt/kvm/arm/vgic/vgic-mmio.h | 11 ++++++++ >>>> 3 files changed, 69 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c >>>> index 69e96f7..448d1da 100644 >>>> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c >>>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c >>>> @@ -72,9 +72,9 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = { >>>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_IGROUP, >>>> vgic_mmio_read_rao, vgic_mmio_write_wi, 1), >>>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_SET, >>>> - vgic_mmio_read_raz, vgic_mmio_write_wi, 1), >>>> + vgic_mmio_read_enable, vgic_mmio_write_senable, 1), >>>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_CLEAR, >>>> - vgic_mmio_read_raz, vgic_mmio_write_wi, 1), >>>> + vgic_mmio_read_enable, vgic_mmio_write_cenable, 1), >>>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_SET, >>>> vgic_mmio_read_raz, vgic_mmio_write_wi, 1), >>>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_CLEAR, >>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c >>>> index 41cf4f4..077ae86 100644 >>>> --- a/virt/kvm/arm/vgic/vgic-mmio.c >>>> +++ b/virt/kvm/arm/vgic/vgic-mmio.c >>>> @@ -46,6 +46,62 @@ void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr, >>>> /* Ignore */ >>>> } >>>> >>>> +/* >>>> + * Read accesses to both GICD_ICENABLER and GICD_ISENABLER return the value >>>> + * of the enabled bit, so there is only one function for both here. >>>> + */ >>>> +unsigned long vgic_mmio_read_enable(struct kvm_vcpu *vcpu, >>>> + gpa_t addr, unsigned int len) >>>> +{ >>>> + u32 intid = (addr & 0x7f) * 8; >>> >>> is there anything we can do about this to make it more intuitive? A >>> macro to generate the mask/offset based on bits per interrupt or >>> something? >> >> Yes, something where you give it the address and the bits-per-IRQ and it >> tells you the IRQ number. >> Not sure it is advisable to squash this into v4 still? >> >>> >>>> + u32 value = 0; >>>> + int i; >>>> + >>>> + /* Loop over all IRQs affected by this read */ >>>> + for (i = 0; i < len * 8; i++) { >>>> + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); >>>> + >>>> + if (irq->enabled) >>>> + value |= (1U << i); >>> >>> I couldn't find the code anywhere that enforces word-aligned accesses to >>> these registers. Do we have that? >> >> Not that I am aware of. I was suggesting this since we have one in the >> IROUTER function. Architecturally we don't need to support halfword >> accesses, it's: byte + word, word only or double-word + word, depending >> on the actual register, IIRC. >> As a fix we can at least deny (read: ignore) halfword accesses in >> general in the dispatcher. Shall I do this (two two-liners)? >> I think byte and word accesses are safe with the existing handlers last >> time I checked. >> >>> If that's not the case, doesn't this break of you do a non-word aligned >>> access? >> >> Why would it? vgic_data_host_to_mmio_bus and extract_bytes should cover >> this, shouldn't they? >> > > I think this breaks on a simple byte access. Let's say you are > accessing byte 1 (addr & 0x7ff == 1), then because you start your loop > at 0, you're going to set bits [7:0] in the value variable, and then > extract bits [15:8] in extract_bytes(), right? Looks like, yes. I thought that it would take care of that, because a handler function shouldn't be bothered with this: it just acts on the lower bytes (till the length) and extract_bytes() does the rest. I think this is what the old magic vgic_reg_access does due to "regval >> word_offset". Do you care to fix this in extract_bytes? Cheers, Andre. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 11/05/16 14:13, Marc Zyngier wrote: > On 11/05/16 13:34, Christoffer Dall wrote: >> On Fri, May 06, 2016 at 11:45:37AM +0100, Andre Przywara wrote: >>> As the enable register handlers are shared between the v2 and v3 >>> emulation, their implementation goes into vgic-mmio.c, to be easily >>> referenced from the v3 emulation as well later. >>> >>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>> --- >>> Changelog RFC..v1: >>> - use lower bits of address to determine IRQ number >>> - remove TODO, confirmed to be fine >>> >>> Changelog v1 .. v2: >>> - adapt to new MMIO framework >>> >>> virt/kvm/arm/vgic/vgic-mmio-v2.c | 4 +-- >>> virt/kvm/arm/vgic/vgic-mmio.c | 56 ++++++++++++++++++++++++++++++++++++++++ >>> virt/kvm/arm/vgic/vgic-mmio.h | 11 ++++++++ >>> 3 files changed, 69 insertions(+), 2 deletions(-) >>> >>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c >>> index 69e96f7..448d1da 100644 >>> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c >>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c >>> @@ -72,9 +72,9 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = { >>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_IGROUP, >>> vgic_mmio_read_rao, vgic_mmio_write_wi, 1), >>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_SET, >>> - vgic_mmio_read_raz, vgic_mmio_write_wi, 1), >>> + vgic_mmio_read_enable, vgic_mmio_write_senable, 1), >>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_CLEAR, >>> - vgic_mmio_read_raz, vgic_mmio_write_wi, 1), >>> + vgic_mmio_read_enable, vgic_mmio_write_cenable, 1), >>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_SET, >>> vgic_mmio_read_raz, vgic_mmio_write_wi, 1), >>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_CLEAR, >>> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c >>> index 41cf4f4..077ae86 100644 >>> --- a/virt/kvm/arm/vgic/vgic-mmio.c >>> +++ b/virt/kvm/arm/vgic/vgic-mmio.c >>> @@ -46,6 +46,62 @@ void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr, >>> /* Ignore */ >>> } >>> >>> +/* >>> + * Read accesses to both GICD_ICENABLER and GICD_ISENABLER return the value >>> + * of the enabled bit, so there is only one function for both here. >>> + */ >>> +unsigned long vgic_mmio_read_enable(struct kvm_vcpu *vcpu, >>> + gpa_t addr, unsigned int len) >>> +{ >>> + u32 intid = (addr & 0x7f) * 8; >> >> is there anything we can do about this to make it more intuitive? A >> macro to generate the mask/offset based on bits per interrupt or >> something? > > Untested (and thus probably wrong): > > #define VGIC_ADDR_IRQ_MASK(bits) ((1024 - 1) >> (4 - (bits)) What about if bits is 8? Wouldn't GENMASK(bits + 10 - 2, 0) do the trick? > #define VGIC_ADDR_IRQ_MULT(bits) (8 / (bits)) IROUTER has a bits_per_irq value of 64. We just don't need to call it from there, but it would be nice if it could cover this. But I don't know a neat solution for this either. Cheers, Andre. > #define VGIC_ADDR_TO_INTID(addr, bits) (((addr) & VGIC_ADDR_IRQ_MASK(bits)) * \ > VGIC_ADDR_IRQ_MULT(bits)) > > u32 intid = VGIC_ADDR_TO_INTID(addr, 1); > > Thanks, > > M. > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 11, 2016 at 02:24:22PM +0100, Andre Przywara wrote: > > > On 11/05/16 14:14, Christoffer Dall wrote: > > On Wed, May 11, 2016 at 02:04:13PM +0100, Andre Przywara wrote: > >> Hi, > >> > >> On 11/05/16 13:34, Christoffer Dall wrote: > >>> On Fri, May 06, 2016 at 11:45:37AM +0100, Andre Przywara wrote: > >>>> As the enable register handlers are shared between the v2 and v3 > >>>> emulation, their implementation goes into vgic-mmio.c, to be easily > >>>> referenced from the v3 emulation as well later. > >>>> > >>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> > >>>> --- > >>>> Changelog RFC..v1: > >>>> - use lower bits of address to determine IRQ number > >>>> - remove TODO, confirmed to be fine > >>>> > >>>> Changelog v1 .. v2: > >>>> - adapt to new MMIO framework > >>>> > >>>> virt/kvm/arm/vgic/vgic-mmio-v2.c | 4 +-- > >>>> virt/kvm/arm/vgic/vgic-mmio.c | 56 ++++++++++++++++++++++++++++++++++++++++ > >>>> virt/kvm/arm/vgic/vgic-mmio.h | 11 ++++++++ > >>>> 3 files changed, 69 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c > >>>> index 69e96f7..448d1da 100644 > >>>> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c > >>>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c > >>>> @@ -72,9 +72,9 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = { > >>>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_IGROUP, > >>>> vgic_mmio_read_rao, vgic_mmio_write_wi, 1), > >>>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_SET, > >>>> - vgic_mmio_read_raz, vgic_mmio_write_wi, 1), > >>>> + vgic_mmio_read_enable, vgic_mmio_write_senable, 1), > >>>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_CLEAR, > >>>> - vgic_mmio_read_raz, vgic_mmio_write_wi, 1), > >>>> + vgic_mmio_read_enable, vgic_mmio_write_cenable, 1), > >>>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_SET, > >>>> vgic_mmio_read_raz, vgic_mmio_write_wi, 1), > >>>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_CLEAR, > >>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c > >>>> index 41cf4f4..077ae86 100644 > >>>> --- a/virt/kvm/arm/vgic/vgic-mmio.c > >>>> +++ b/virt/kvm/arm/vgic/vgic-mmio.c > >>>> @@ -46,6 +46,62 @@ void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr, > >>>> /* Ignore */ > >>>> } > >>>> > >>>> +/* > >>>> + * Read accesses to both GICD_ICENABLER and GICD_ISENABLER return the value > >>>> + * of the enabled bit, so there is only one function for both here. > >>>> + */ > >>>> +unsigned long vgic_mmio_read_enable(struct kvm_vcpu *vcpu, > >>>> + gpa_t addr, unsigned int len) > >>>> +{ > >>>> + u32 intid = (addr & 0x7f) * 8; > >>> > >>> is there anything we can do about this to make it more intuitive? A > >>> macro to generate the mask/offset based on bits per interrupt or > >>> something? > >> > >> Yes, something where you give it the address and the bits-per-IRQ and it > >> tells you the IRQ number. > >> Not sure it is advisable to squash this into v4 still? > >> > >>> > >>>> + u32 value = 0; > >>>> + int i; > >>>> + > >>>> + /* Loop over all IRQs affected by this read */ > >>>> + for (i = 0; i < len * 8; i++) { > >>>> + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); > >>>> + > >>>> + if (irq->enabled) > >>>> + value |= (1U << i); > >>> > >>> I couldn't find the code anywhere that enforces word-aligned accesses to > >>> these registers. Do we have that? > >> > >> Not that I am aware of. I was suggesting this since we have one in the > >> IROUTER function. Architecturally we don't need to support halfword > >> accesses, it's: byte + word, word only or double-word + word, depending > >> on the actual register, IIRC. > >> As a fix we can at least deny (read: ignore) halfword accesses in > >> general in the dispatcher. Shall I do this (two two-liners)? > >> I think byte and word accesses are safe with the existing handlers last > >> time I checked. > >> > >>> If that's not the case, doesn't this break of you do a non-word aligned > >>> access? > >> > >> Why would it? vgic_data_host_to_mmio_bus and extract_bytes should cover > >> this, shouldn't they? > >> > > > > I think this breaks on a simple byte access. Let's say you are > > accessing byte 1 (addr & 0x7ff == 1), then because you start your loop > > at 0, you're going to set bits [7:0] in the value variable, and then > > extract bits [15:8] in extract_bytes(), right? > > Looks like, yes. I thought that it would take care of that, because a > handler function shouldn't be bothered with this: it just acts on the > lower bytes (till the length) and extract_bytes() does the rest. I think > this is what the old magic vgic_reg_access does due to "regval >> > word_offset". > > Do you care to fix this in extract_bytes? > I don't think this is a fix to extract_bytes. I think the issue is with how we build the values. I though the semanticsehre were that the u32 value in this case would the entire 32-bit register, so can't we simply mask off the lower bits in the addr on entry to these functions and add additional checks if there are registers where specifically do not wish to support byte accessed? -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 11, 2016 at 02:13:48PM +0100, Marc Zyngier wrote: > On 11/05/16 13:34, Christoffer Dall wrote: > > On Fri, May 06, 2016 at 11:45:37AM +0100, Andre Przywara wrote: > >> As the enable register handlers are shared between the v2 and v3 > >> emulation, their implementation goes into vgic-mmio.c, to be easily > >> referenced from the v3 emulation as well later. > >> > >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> > >> --- > >> Changelog RFC..v1: > >> - use lower bits of address to determine IRQ number > >> - remove TODO, confirmed to be fine > >> > >> Changelog v1 .. v2: > >> - adapt to new MMIO framework > >> > >> virt/kvm/arm/vgic/vgic-mmio-v2.c | 4 +-- > >> virt/kvm/arm/vgic/vgic-mmio.c | 56 ++++++++++++++++++++++++++++++++++++++++ > >> virt/kvm/arm/vgic/vgic-mmio.h | 11 ++++++++ > >> 3 files changed, 69 insertions(+), 2 deletions(-) > >> > >> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c > >> index 69e96f7..448d1da 100644 > >> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c > >> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c > >> @@ -72,9 +72,9 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = { > >> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_IGROUP, > >> vgic_mmio_read_rao, vgic_mmio_write_wi, 1), > >> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_SET, > >> - vgic_mmio_read_raz, vgic_mmio_write_wi, 1), > >> + vgic_mmio_read_enable, vgic_mmio_write_senable, 1), > >> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_CLEAR, > >> - vgic_mmio_read_raz, vgic_mmio_write_wi, 1), > >> + vgic_mmio_read_enable, vgic_mmio_write_cenable, 1), > >> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_SET, > >> vgic_mmio_read_raz, vgic_mmio_write_wi, 1), > >> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_CLEAR, > >> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c > >> index 41cf4f4..077ae86 100644 > >> --- a/virt/kvm/arm/vgic/vgic-mmio.c > >> +++ b/virt/kvm/arm/vgic/vgic-mmio.c > >> @@ -46,6 +46,62 @@ void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr, > >> /* Ignore */ > >> } > >> > >> +/* > >> + * Read accesses to both GICD_ICENABLER and GICD_ISENABLER return the value > >> + * of the enabled bit, so there is only one function for both here. > >> + */ > >> +unsigned long vgic_mmio_read_enable(struct kvm_vcpu *vcpu, > >> + gpa_t addr, unsigned int len) > >> +{ > >> + u32 intid = (addr & 0x7f) * 8; > > > > is there anything we can do about this to make it more intuitive? A > > macro to generate the mask/offset based on bits per interrupt or > > something? > > Untested (and thus probably wrong): > > #define VGIC_ADDR_IRQ_MASK(bits) ((1024 - 1) >> (4 - (bits)) you're going to have to explain this to me, where is the (4 - bits) coming from? > #define VGIC_ADDR_IRQ_MULT(bits) (8 / (bits)) > #define VGIC_ADDR_TO_INTID(addr, bits) (((addr) & VGIC_ADDR_IRQ_MASK(bits)) * \ > VGIC_ADDR_IRQ_MULT(bits)) > > u32 intid = VGIC_ADDR_TO_INTID(addr, 1); > > Thanks, > > M. > -- > Jazz is not dead. It just smells funny... -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 11/05/16 14:13, Marc Zyngier wrote: > On 11/05/16 13:34, Christoffer Dall wrote: >> On Fri, May 06, 2016 at 11:45:37AM +0100, Andre Przywara wrote: >>> As the enable register handlers are shared between the v2 and v3 >>> emulation, their implementation goes into vgic-mmio.c, to be easily >>> referenced from the v3 emulation as well later. >>> >>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>> --- >>> Changelog RFC..v1: >>> - use lower bits of address to determine IRQ number >>> - remove TODO, confirmed to be fine >>> >>> Changelog v1 .. v2: >>> - adapt to new MMIO framework >>> >>> virt/kvm/arm/vgic/vgic-mmio-v2.c | 4 +-- >>> virt/kvm/arm/vgic/vgic-mmio.c | 56 ++++++++++++++++++++++++++++++++++++++++ >>> virt/kvm/arm/vgic/vgic-mmio.h | 11 ++++++++ >>> 3 files changed, 69 insertions(+), 2 deletions(-) >>> >>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c >>> index 69e96f7..448d1da 100644 >>> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c >>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c >>> @@ -72,9 +72,9 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = { >>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_IGROUP, >>> vgic_mmio_read_rao, vgic_mmio_write_wi, 1), >>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_SET, >>> - vgic_mmio_read_raz, vgic_mmio_write_wi, 1), >>> + vgic_mmio_read_enable, vgic_mmio_write_senable, 1), >>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_CLEAR, >>> - vgic_mmio_read_raz, vgic_mmio_write_wi, 1), >>> + vgic_mmio_read_enable, vgic_mmio_write_cenable, 1), >>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_SET, >>> vgic_mmio_read_raz, vgic_mmio_write_wi, 1), >>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_CLEAR, >>> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c >>> index 41cf4f4..077ae86 100644 >>> --- a/virt/kvm/arm/vgic/vgic-mmio.c >>> +++ b/virt/kvm/arm/vgic/vgic-mmio.c >>> @@ -46,6 +46,62 @@ void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr, >>> /* Ignore */ >>> } >>> >>> +/* >>> + * Read accesses to both GICD_ICENABLER and GICD_ISENABLER return the value >>> + * of the enabled bit, so there is only one function for both here. >>> + */ >>> +unsigned long vgic_mmio_read_enable(struct kvm_vcpu *vcpu, >>> + gpa_t addr, unsigned int len) >>> +{ >>> + u32 intid = (addr & 0x7f) * 8; >> >> is there anything we can do about this to make it more intuitive? A >> macro to generate the mask/offset based on bits per interrupt or >> something? > > Untested (and thus probably wrong): > > #define VGIC_ADDR_IRQ_MASK(bits) ((1024 - 1) >> (4 - (bits)) > #define VGIC_ADDR_IRQ_MULT(bits) (8 / (bits)) > #define VGIC_ADDR_TO_INTID(addr, bits) (((addr) & VGIC_ADDR_IRQ_MASK(bits)) * \ > VGIC_ADDR_IRQ_MULT(bits)) > Would: #define VGIC_ADDR_IRQ_MASK(bits) GENMASK_ULL((bits) + 10 - 1 - 1, 0) #define VGIC_ADDR_TO_INTID(addr, bits) (((addr) & \ VGIC_ADDR_IRQ_MASK(bits)) * 64 / (bits) / 8) work for all bitnesses? Cheers, Andre. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/05/16 14:39, Andre Przywara wrote: > Hi, > > On 11/05/16 14:13, Marc Zyngier wrote: >> On 11/05/16 13:34, Christoffer Dall wrote: >>> On Fri, May 06, 2016 at 11:45:37AM +0100, Andre Przywara wrote: >>>> As the enable register handlers are shared between the v2 and v3 >>>> emulation, their implementation goes into vgic-mmio.c, to be easily >>>> referenced from the v3 emulation as well later. >>>> >>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>>> --- >>>> Changelog RFC..v1: >>>> - use lower bits of address to determine IRQ number >>>> - remove TODO, confirmed to be fine >>>> >>>> Changelog v1 .. v2: >>>> - adapt to new MMIO framework >>>> >>>> virt/kvm/arm/vgic/vgic-mmio-v2.c | 4 +-- >>>> virt/kvm/arm/vgic/vgic-mmio.c | 56 ++++++++++++++++++++++++++++++++++++++++ >>>> virt/kvm/arm/vgic/vgic-mmio.h | 11 ++++++++ >>>> 3 files changed, 69 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c >>>> index 69e96f7..448d1da 100644 >>>> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c >>>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c >>>> @@ -72,9 +72,9 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = { >>>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_IGROUP, >>>> vgic_mmio_read_rao, vgic_mmio_write_wi, 1), >>>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_SET, >>>> - vgic_mmio_read_raz, vgic_mmio_write_wi, 1), >>>> + vgic_mmio_read_enable, vgic_mmio_write_senable, 1), >>>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_CLEAR, >>>> - vgic_mmio_read_raz, vgic_mmio_write_wi, 1), >>>> + vgic_mmio_read_enable, vgic_mmio_write_cenable, 1), >>>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_SET, >>>> vgic_mmio_read_raz, vgic_mmio_write_wi, 1), >>>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_CLEAR, >>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c >>>> index 41cf4f4..077ae86 100644 >>>> --- a/virt/kvm/arm/vgic/vgic-mmio.c >>>> +++ b/virt/kvm/arm/vgic/vgic-mmio.c >>>> @@ -46,6 +46,62 @@ void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr, >>>> /* Ignore */ >>>> } >>>> >>>> +/* >>>> + * Read accesses to both GICD_ICENABLER and GICD_ISENABLER return the value >>>> + * of the enabled bit, so there is only one function for both here. >>>> + */ >>>> +unsigned long vgic_mmio_read_enable(struct kvm_vcpu *vcpu, >>>> + gpa_t addr, unsigned int len) >>>> +{ >>>> + u32 intid = (addr & 0x7f) * 8; >>> >>> is there anything we can do about this to make it more intuitive? A >>> macro to generate the mask/offset based on bits per interrupt or >>> something? >> >> Untested (and thus probably wrong): >> >> #define VGIC_ADDR_IRQ_MASK(bits) ((1024 - 1) >> (4 - (bits)) > > What about if bits is 8? Gnii. This should be ((1024 - 1) >> (4 - ffs(bits)) > Wouldn't GENMASK(bits + 10 - 2, 0) do the trick? I really can't see how that works... It could be GENMASK(10 - 1 - ffs(bits), 0) though. >> #define VGIC_ADDR_IRQ_MULT(bits) (8 / (bits)) > > IROUTER has a bits_per_irq value of 64. We just don't need to call it > from there, but it would be nice if it could cover this. > But I don't know a neat solution for this either. Me neither, but I don't see that as a major issue so far. M.
On 11/05/16 15:18, Andre Przywara wrote: > Would: > > #define VGIC_ADDR_IRQ_MASK(bits) GENMASK_ULL((bits) + 10 - 1 - 1, 0) Nonsense. Must be ... ilog2(bits) ..., of course. Andre. > #define VGIC_ADDR_TO_INTID(addr, bits) (((addr) & \ > VGIC_ADDR_IRQ_MASK(bits)) * 64 / (bits) / 8) > > work for all bitnesses? -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c index 69e96f7..448d1da 100644 --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c @@ -72,9 +72,9 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = { REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_IGROUP, vgic_mmio_read_rao, vgic_mmio_write_wi, 1), REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_SET, - vgic_mmio_read_raz, vgic_mmio_write_wi, 1), + vgic_mmio_read_enable, vgic_mmio_write_senable, 1), REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_CLEAR, - vgic_mmio_read_raz, vgic_mmio_write_wi, 1), + vgic_mmio_read_enable, vgic_mmio_write_cenable, 1), REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_SET, vgic_mmio_read_raz, vgic_mmio_write_wi, 1), REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_CLEAR, diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c index 41cf4f4..077ae86 100644 --- a/virt/kvm/arm/vgic/vgic-mmio.c +++ b/virt/kvm/arm/vgic/vgic-mmio.c @@ -46,6 +46,62 @@ void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr, /* Ignore */ } +/* + * Read accesses to both GICD_ICENABLER and GICD_ISENABLER return the value + * of the enabled bit, so there is only one function for both here. + */ +unsigned long vgic_mmio_read_enable(struct kvm_vcpu *vcpu, + gpa_t addr, unsigned int len) +{ + u32 intid = (addr & 0x7f) * 8; + u32 value = 0; + int i; + + /* Loop over all IRQs affected by this read */ + for (i = 0; i < len * 8; i++) { + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); + + if (irq->enabled) + value |= (1U << i); + } + + return extract_bytes(value, addr & 3, len); +} + +void vgic_mmio_write_senable(struct kvm_vcpu *vcpu, + gpa_t addr, unsigned int len, + unsigned long val) +{ + u32 intid = (addr & 0x7f) * 8; + int i; + + for_each_set_bit(i, &val, len * 8) { + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); + + spin_lock(&irq->irq_lock); + irq->enabled = true; + vgic_queue_irq_unlock(vcpu->kvm, irq); + } +} + +void vgic_mmio_write_cenable(struct kvm_vcpu *vcpu, + gpa_t addr, unsigned int len, + unsigned long val) +{ + u32 intid = (addr & 0x7f) * 8; + int i; + + for_each_set_bit(i, &val, len * 8) { + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); + + spin_lock(&irq->irq_lock); + + irq->enabled = false; + + spin_unlock(&irq->irq_lock); + } +} + static int match_region(const void *key, const void *elt) { const unsigned int offset = (unsigned long)key; diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h index 4f4dd2b..188909a 100644 --- a/virt/kvm/arm/vgic/vgic-mmio.h +++ b/virt/kvm/arm/vgic/vgic-mmio.h @@ -74,6 +74,17 @@ unsigned long vgic_mmio_read_rao(struct kvm_vcpu *vcpu, void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr, unsigned int len, unsigned long val); +unsigned long vgic_mmio_read_enable(struct kvm_vcpu *vcpu, + gpa_t addr, unsigned int len); + +void vgic_mmio_write_senable(struct kvm_vcpu *vcpu, + gpa_t addr, unsigned int len, + unsigned long val); + +void vgic_mmio_write_cenable(struct kvm_vcpu *vcpu, + gpa_t addr, unsigned int len, + unsigned long val); + unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev); #endif
As the enable register handlers are shared between the v2 and v3 emulation, their implementation goes into vgic-mmio.c, to be easily referenced from the v3 emulation as well later. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- Changelog RFC..v1: - use lower bits of address to determine IRQ number - remove TODO, confirmed to be fine Changelog v1 .. v2: - adapt to new MMIO framework virt/kvm/arm/vgic/vgic-mmio-v2.c | 4 +-- virt/kvm/arm/vgic/vgic-mmio.c | 56 ++++++++++++++++++++++++++++++++++++++++ virt/kvm/arm/vgic/vgic-mmio.h | 11 ++++++++ 3 files changed, 69 insertions(+), 2 deletions(-)