Re: [PATCH 04/14] arm/arm64: KVM: wrap 64 bit MMIO accesses with two 32 bit ones

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux KVM]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux