On 07/08/2014 10:31 AM, wanghaibin wrote:
> On 2014/6/19 17:45, Andre Przywara wrote:
>
>> 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),
> there is no need the offset align and no need mmio->len == 4.
First: for GICv2 we do not allow anything bigger than 32 bits, this will
be checked earlier in vgic_v2_handle_mmio().
For any access bigger than 4 but smaller than 8 bytes I don't see how
this could be done with one machine instruction: you can load bytes,
halfwords, words or 64-bit values and can you load multiple 32-bit
entities (ARMv7) or two 32-bit or 64-bit values.
So this is 1, 2, 4, 8 only, given we don't handle ldp or ldm at a HYP trap.
Am I missing something?
>
> Obviously, the 64-bits reg access func has many limits(Though I still think that guest will guarantee the offset align) :) .
You mean unaligned 64-bit accesses? Does the v8 architecture allow this?
I will check how this could be handled. As Marc mentioned, we cannot
trust the guest.
Regards,
Andre.
>> +
>> +/*
>> * 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.
>
>> + if (!mmio->is_write)
>> + memset(mmio->data, 0, mmio->len);
>> updated_state = false;
>> }
>> spin_unlock(&vcpu->kvm->arch.vgic.lock);
>
>
>
> _______________________________________________
> kvmarm mailing list
> kvmarm@xxxxxxxxxxxxxxxxxxxxx
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
>
-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
