On Tue, Jul 08 2014 at 10:27:41 am BST, Andre Przywara <andre.przywara@xxxxxxx> wrote:
> 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 ;-)
In that case, it is obvious that I'm getting old! ;-)
>> 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?
Ah, yes, mmio->data is a byte array. This always gets me. Should be OK
then.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
