On 20/06/12 00:08, Christoffer Dall wrote:
> On Mon, May 14, 2012 at 9:05 AM, Marc Zyngier <marc.zyngier at arm.com> wrote:
>> Wire the initial in-kernel MMIO support code for the VGIC, used
>> for the distributor emulation.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier at arm.com>
[...]
>> diff --git a/arch/arm/kvm/vgic.c b/arch/arm/kvm/vgic.c
>> new file mode 100644
>> index 0000000..f7856a9
>> --- /dev/null
>> +++ b/arch/arm/kvm/vgic.c
>> +
>> +static void mmio_do_copy(struct kvm_run *run, u32 *reg, u32 offset, int mode)
>
> Ah, reg is mmio-mapped gic_reg. That's confusing.
>
> I think a comment here may be helpful. Also, I'm not sure about the
> name. Isn't this in fact vgic_ctrl_access(...) or something like that?
Well, nothing in this function is GIC specific, really. It's just a
helper for accessing a register with a particular mode. How about
mmio_access()?
>
>> +{
>> + int u32off = offset & 3;
>
> I fail to see why it's named u32off, is it because it's a word offset?
Yes. I'll rename it to word_offset to make things clearer.
>
>> + int shift = u32off * 8;
>> + u32 mask;
>> + u32 regval;
>> +
>> + /*
>> + * We do silly things on cross-register accesses, so pretend
>> + * they do not exist. Will have to be handled though...
>> + */
>
> By 'We' do you mean this function or the ARM/GIC architecture? Is this
> actually allowed? Why does it need to be handled and is it too
> complicated to handle right away?
'We' as the function. Cross-register (actually, any unaligned) access on
device memory should give an Alignment fault. Not something we handle yet.
>
>> + if (WARN_ON((u32off + run->mmio.len) > 4))
>> + run->mmio.len = 4 - u32off;
>
> does this fix make sense? Does it actually happen or should we just
> bail out? Seems like the WARN_ON could generate a lot of host
> interference and flood the host log if the guest can keep provoking
> it. Unsure how this is called so far though, will look in following
> patches.
Bailing out is probably the best, together with the injection of an
Alignment fault in the guest. I'll remove this and add a comment.
>> +
>> + mask = ((u32)-1) >> (u32off * 8);
>
> consider (~0U) to avoid the cast?
Yup.
>> + if (reg)
>> + regval = *reg;
>> + else {
>> + BUG_ON(mode != (ACCESS_READ_RAZ | ACCESS_WRITE_IGNORED));
>> + regval = 0;
>> + }
>> +
>> + if (run->mmio.is_write) {
>> + u32 data = (*((u32 *)run->mmio.data) & mask) << shift;
>> + switch (ACCESS_WRITE_MASK(mode)) {
>> + case ACCESS_WRITE_IGNORED:
>> + return;
>> +
>
> I don't think these spaces before the next 'case' line is commonplace
> in the kernel...
It is not uncommon, but I don't mind removing them.
>> + case ACCESS_WRITE_SETBIT:
>> + regval |= data;
>> + break;
>> +
>> + case ACCESS_WRITE_CLEARBIT:
>> + regval &= ~data;
>> + break;
>> +
>> + case ACCESS_WRITE_VALUE:
>> + regval = (regval & ~(mask << shift)) | data;
>> + break;
>> + }
>> + *reg = regval;
>> + } else {
>> + switch (ACCESS_READ_MASK(mode)) {
>> + case ACCESS_READ_RAZ:
>> + regval = 0;
>> + /* fall through */
>> +
>> + case ACCESS_READ_VALUE:
>> + *((u32 *)run->mmio.data) = (regval >> shift) & mask;
>> + }
>> + }
>> +}
>> +
>> +/* All this should really be generic code... FIXME!!! */
>
> generic code as in arch-generic? are there similar constructs in
> x86/powerpc/...? The FIXME!!! is quite strong, should we fix it now or
> is it ok for this code to live in KVM for somer time after a merge?
Generic as in KVM common code. This should really be handled by the
kvm_io_bus_*() stuff. Unfortunately, it is not suitable yet (it only
passes the kvm struct around, while we actually need the vcpu struct). I
think we can get away with merging this, and the screaming FIXME should
be an incentive to keep it in mind.
>
>> +struct mmio_range {
>> + unsigned long base;
>> + unsigned long len;
>> + void (*handle_mmio)(struct kvm_vcpu *vcpu, struct kvm_run *run,
>> + u32 offset);
>> +};
>> +
>> +static const struct mmio_range vgic_ranges[] = {
>> + {}
>> +};
>> +
>> +static const
>> +struct mmio_range *find_matching_range(const struct mmio_range *ranges,
>> + struct kvm_run *run)
>> +{
>> + const struct mmio_range *r = ranges;
>
> consider newline
OK.
>> + while (r->len) {
>> + if (run->mmio.phys_addr >= r->base &&
>> + (run->mmio.phys_addr + run->mmio.len) <= (r->base + r->len))
>> + return r;
>> + r++;
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +int vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run)
>
> document the exported function please.
OK.
>> +{
>> + const struct mmio_range *range;
>> +
>> + if (!irqchip_in_kernel(vcpu->kvm))
>> + return KVM_EXIT_MMIO;
>> +
>> + range = find_matching_range(vgic_ranges, run);
>> + if (!range || !range->handle_mmio)
>> + return KVM_EXIT_MMIO;
>> +
>> + pr_debug("emulating %d %08llx %d\n", run->mmio.is_write,
>> + run->mmio.phys_addr, run->mmio.len);
>
> kvm_debug?
OK.
>> + range->handle_mmio(vcpu, run, run->mmio.phys_addr - range->base);
>> + kvm_handle_mmio_return(vcpu, run);
>> +
>> + return KVM_EXIT_UNKNOWN;
>> +}
>> --
>> 1.7.7.1
>>
>>
>>
>> _______________________________________________
>> Android-virt mailing list
>> Android-virt at lists.cs.columbia.edu
>> https://lists.cs.columbia.edu/cucslists/listinfo/android-virt
>
--
Jazz is not dead. It just smells funny...
