On Wed, Jun 20, 2012 at 1:04 PM, Marc Zyngier <marc.zyngier at arm.com> wrote: > 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! ;-) > wonderful!
