On Tue, Nov 04, 2014 at 12:18:16PM +0000, Andre Przywara wrote:
> Hi Christoffer,
>
> On 03/11/14 13:25, Christoffer Dall wrote:
> > On Fri, Oct 31, 2014 at 05:26:39PM +0000, 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 704be48..0cbdde9 100644
> >> --- a/virt/kvm/arm/vgic.c
> >> +++ b/virt/kvm/arm/vgic.c
> >> @@ -1033,6 +1033,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);
> >> +
> >> + /*
> >> + * Any access bigger than 4 bytes (that we currently handle in KVM)
> >> + * 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;
> >> +}
> >
> > Please think about the endianness issues here.
>
> I didn't only think about it, I traced the code and tested it:
> So it works like written above (I actually had a hickup in my kvmtool
> setup that denied booting the bigendian initrds, so I thought that BE
> was broken).
>
> So the GIC is always LE, that's why we swap the bytes to LE in any
> 32-bit register in mmio_data_{write,read}, which gets called for each
> vGIC register access via the vgic_reg_access() function.
>
> So the memory order that the actual register handler functions
> implicitly expect is always LE, regardless of the guest or host
> endianness. vgic_reg_access() makes this transparent for the host code.
>
> Now if we eventually assemble the 64-bit value from the two 32-bit
> values, we also have to always do this in LE fashion. Hence the
> hardcoded LE assignment here. Eventually this LE value will be copied
> into the guest, which will access it through readq, which uses
> le64_to_cpu() to convert it to the CPU native value.
>
> So the branch as posted (or present in the repo) works fine (boot-tested
> only so far) with all 8 combinations of (host endianness, guest
> endianness, guest v2/v3 GIC).
>
> I will add a comment to the function explaining this.
>
Yes, you're right. Thanks for the explanation. I think the key to
understanding that this works is the fact that mmio_data is always
written in LE in memory.
I was thrown off by the conversion you were making to a u32*, which you
don't really use, except as index mamipulation and to copy the data, but
that's fine.
Thanks for explaining this.
-Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
