On 20/06/12 17:47, Christoffer Dall wrote: > 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? As I said, it's only a helper. I'll make it vgic specific, and if it can be reused at any point, we'll move it somewhere else. >>>> + 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. Just went through the ARM ARM again, and learned something new, as always. Alignment faults have priority over stage 2 translation faults, so the guest will receive a fault directly, without us having to inject anything. Problem solved! ;-) M. -- Jazz is not dead. It just smells funny...
