[PATCH v3 24/55] KVM: arm/arm64: vgic-new: Add ENABLE registers handlers

[christoffer.dall@linaro.org (Christoffer Dall)]

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