On Wed, Jun 20, 2012 at 12:36 PM, Marc Zyngier <marc.zyngier at arm.com> wrote:
> 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()?
>
that's fine, but it is a static in a vgic.c file used specifially to
access vgic registers only is it not?
>>
>>> +{
>>> + ? ? ? 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.
>
yeah, if you add a TODO, FIXME or similar, we'll catch it when we go
through the code and clean up bail-outs and turn them into alignment
fault injection and undefined exception injections. thanks.
>>> +
>>> + ? ? ? 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.
>
if no one complains when this goes to lkml, then just leave 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.
>
ok, good.
>>
>>> +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...
>
