On 2014/4/11 6:07, Andre Przywara wrote:
>> From: Haibin Wang <wanghaibin.wang@xxxxxxxxxx>
>>
>> It needs to use the bitmap to save the GICD_ICFGRn value
>> (the real hardware register need two bits pre irq), Here
>> before access this cfg bitmap, we did "offset >>= 1;". Later,
>> it use the vgic_cfg_expand function to expend these bits.
>>
>> But, It's incorrect to pass offset parameter to vgic_reg_access,
>> it should pass "offset << 1".
>
> Good catch, but I think the patch is wrong:
> 1) If you write above (and mean) "shift left", you should do so in
> the patch, too ;-)
>
>> - vgic_reg_access(mmio, &val, offset,
>> + vgic_reg_access(mmio, &val, offset >> 1,
>
OK, That is a my mistake, there muse be "offset << 1", so shamed!
> 2) We should not drop the LSB of offset, otherwise byte accesses to
> uneven addresses will be wrong (think of ldrb r0, [r1, #5])
>
Yes, I agree with you and your patch as follow.
I ignored this access method like your said(ldrb r0, [r1, #5]).
Though I think the GICv2 driver which VM used will guarantee that the
access method you said will never happened.
Thanks!
> So I propose the below patch instead.
>
> Cheers,
> Andre.
>
> ----------------
> KVM: arm/arm64: vgic: fix GICD_ICFGR register accesses
>
> Since KVM internally represents the ICFGR registers by stuffing two
> of them into one word, the offset for accessing the internal
> representation and the one for the MMIO based access are different.
> So keep the original offset around, but adjust the internal array
> offset by one bit.
>
> Reported-by: Haibin Wang <wanghaibin.wang@xxxxxxxxxx>
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> ---
> virt/kvm/arm/vgic.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 8ca405c..2c71535 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -548,11 +548,10 @@ static bool handle_mmio_cfg_reg(struct kvm_vcpu *vcpu,
> u32 val;
> u32 *reg;
>
> - offset >>= 1;
> reg = vgic_bitmap_get_reg(&vcpu->kvm->arch.vgic.irq_cfg,
> - vcpu->vcpu_id, offset);
> + vcpu->vcpu_id, offset >> 1);
>
> - if (offset & 2)
> + if (offset & 4)
> val = *reg >> 16;
> else
> val = *reg & 0xffff;
> @@ -561,13 +560,13 @@ static bool handle_mmio_cfg_reg(struct kvm_vcpu *vcpu,
> vgic_reg_access(mmio, &val, offset,
> ACCESS_READ_VALUE | ACCESS_WRITE_VALUE);
> if (mmio->is_write) {
> - if (offset < 4) {
> + if (offset < 8) {
> *reg = ~0U; /* Force PPIs/SGIs to 1 */
> return false;
> }
>
> val = vgic_cfg_compress(val);
> - if (offset & 2) {
> + if (offset & 4) {
> *reg &= 0xffff;
> *reg |= val << 16;
> } else {
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
