On 07/08/2014 10:56 AM, Marc Zyngier wrote: > On Tue, Jul 08 2014 at 9:31:52 am BST, wanghaibin <wanghaibin.wang@xxxxxxxxxx> wrote: >> On 2014/6/19 17:45, Andre Przywara wrote: Hi Marc, >> >>> Some GICv3 registers can and will be accessed as 64 bit registers. >>> Currently the register handling code can only deal with 32 bit >>> accesses, so we do two consecutive calls to cover this. >>> >>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx> >>> --- >>> virt/kvm/arm/vgic.c | 48 +++++++++++++++++++++++++++++++++++++++++++++--- >>> 1 file changed, 45 insertions(+), 3 deletions(-) >>> >>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c >>> index 4c6b212..b3cf4c7 100644 >>> --- a/virt/kvm/arm/vgic.c >>> +++ b/virt/kvm/arm/vgic.c >>> @@ -906,6 +906,48 @@ static bool vgic_validate_access(const struct >>> vgic_dist *dist, >>> } >>> >>> /* >>> + * Call the respective handler function for the given range. >>> + * We split up any 64 bit accesses into two consecutive 32 bit >>> + * handler calls and merge the result afterwards. >>> + */ >>> +static bool call_range_handler(struct kvm_vcpu *vcpu, >>> + struct kvm_exit_mmio *mmio, >>> + unsigned long offset, >>> + const struct mmio_range *range) >>> +{ >>> + u32 *data32 = (void *)mmio->data; >>> + struct kvm_exit_mmio mmio32; >>> + bool ret; >>> + >>> + if (likely(mmio->len <= 4)) >>> + return range->handle_mmio(vcpu, mmio, offset); >>> + >>> + /* >>> + * We assume that any access greater than 4 bytes is actually >>> + * 8 bytes long, caused by a 64-bit access >>> + */ >>> + >>> + mmio32.len = 4; >>> + mmio32.is_write = mmio->is_write; >>> + >>> + mmio32.phys_addr = mmio->phys_addr + 4; >>> + if (mmio->is_write) >>> + *(u32 *)mmio32.data = data32[1]; >>> + ret = range->handle_mmio(vcpu, &mmio32, offset + 4); >>> + if (!mmio->is_write) >>> + data32[1] = *(u32 *)mmio32.data; >>> + >>> + mmio32.phys_addr = mmio->phys_addr; >>> + if (mmio->is_write) >>> + *(u32 *)mmio32.data = data32[0]; >>> + ret |= range->handle_mmio(vcpu, &mmio32, offset); >>> + if (!mmio->is_write) >>> + data32[0] = *(u32 *)mmio32.data; >>> + >>> + return ret; >>> +} >> >> >> It seems that this func will treat (the mmio->len >=4) as same as >> (mmio->len == 8). >> About VGIC_v2 module, it will be access successful that guest does >> (ldrb r0, [r1, #5], remember the GICD_CFG access BUG? you show this >> example to me), > > I don't get it. What you describe is a byte access, and will be handled > right at the beginning of the function. > >> there is no need the offset align and no need mmio->len == 4. >> >> Obviously, the 64-bits reg access func has many limits(Though I still >> think that guest will guarantee the offset align) :) . > > We don't trust the guest. Ever. > >>> + >>> +/* >>> * vgic_handle_mmio_range - handle an in-kernel MMIO access >>> * @vcpu: pointer to the vcpu performing the access >>> * @run: pointer to the kvm_run structure >>> @@ -936,10 +978,10 @@ static bool vgic_handle_mmio_range(struct kvm_vcpu *vcpu, struct kvm_run *run, >>> spin_lock(&vcpu->kvm->arch.vgic.lock); >>> offset -= range->base; >>> if (vgic_validate_access(dist, range, offset)) { >>> - updated_state = range->handle_mmio(vcpu, mmio, offset); >>> + updated_state = call_range_handler(vcpu, mmio, offset, range); >>> } else { >>> - vgic_reg_access(mmio, NULL, offset, >>> - ACCESS_READ_RAZ | ACCESS_WRITE_IGNORED); >> >> >> Likely, and we hope that the VM access VGIC is validate, but there >> maybe something wrong bring on this branch (at present,only irq >= >> dist->nr_irqs ) for VGIC invalidate access, >> So I think there only READ_RAZ and WRITE_IGNORED may be not enough, >> think that, VM may get the irq' attr and the irq is overlap, and the >> VGIC will return a 0 to VM, and without >> any warning info. >> Maybe there need some warning,or inject the abort to VM, and so on. > > I suggest you read the GICv2 spec: "A register bit corresponding to an > unimplemented interrupt is RAZ/WI". > >>> + if (!mmio->is_write) >>> + memset(mmio->data, 0, mmio->len); > > Now, this bit looks odd. Andre, why do you change this to a memset? I think after a comment from you ;-) > It > will break a BE setup, and vgic_reg_access should abstract that nicely > already (at least with Victor's patches). vgic_reg_access does not handle 64-bit values, but this function still has to deal with those (the split is later). But at this point it is only the RAZ case left. I don't see how this should impact BE, as I am writing mmio->len bytes of zeroes to the original address. This should be the same, BE or LE, right? Cheers, Andre. > >>> updated_state = false; >>> } >>> spin_unlock(&vcpu->kvm->arch.vgic.lock); >> > > Thanks, > > M. > _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm