[kvmarm] [PATCH v5 13/14] KVM: ARM: Handle I/O aborts

[Christoffer Dall]

On Mon, Jan 14, 2013 at 5:36 PM, Will Deacon <will.deacon at arm.com> wrote:
> On Mon, Jan 14, 2013 at 07:12:49PM +0000, Christoffer Dall wrote:
>> On Mon, Jan 14, 2013 at 2:00 PM, Will Deacon <will.deacon at arm.com> wrote:
>> > On Mon, Jan 14, 2013 at 06:53:14PM +0000, Alexander Graf wrote:
>> >> On 01/14/2013 07:50 PM, Will Deacon wrote:
>> >> > FWIW, KVM only needs this code for handling complex MMIO instructions, which
>> >> > aren't even generated by recent guest kernels. I'm inclined to suggest removing
>> >> > this emulation code from KVM entirely given that it's likely to bitrot as
>> >> > it is executed less and less often.
>> >>
>> >> That'd mean that you heavily limit what type of guests you're executing,
>> >> which I don't think is a good idea.
>> >
>> > To be honest, I don't think we know whether that's true or not. How many
>> > guests out there do writeback accesses to MMIO devices? Even on older
>> > Linux guests, it was dependent on how GCC felt.
>>
>> I don't think bitrot'ing is a valid argument: the code doesn't depend
>> on any other implementation state that's likely to change and break
>> this code (the instruction encoding is not exactly going to change).
>> And we should simply finish the selftest code to test this stuff
>> (which should be finished if the code is unified or not, and is on my
>> todo list).
>
> Maybe `bitrot' is the wrong word. The scenario I envisage is the addition
> of new instructions to the architecture which aren't handled by the current
> code, then we end up with emulation code that works for some percentage of
> the instruction set only. If the code is rarely used, it will likely go
> untouched until it crashes somebody's VM.
>

How is that worse than KVM crashing all VMs that use any of these
instructions for IO?

At least the code we have now has been tested with a number of old
kernels, and we know that it works. As for correctness, it will be the
case for all implementations and this type of code absolutely requires
a test suite.


>> > I see where you're coming from, I just don't think we can quantify it either
>> > way outside of Linux.
>> >
>> FWIW, I know of at least a couple of companies wanting to use KVM for
>> running non-Linux guests as well.
>
> Oh, I don't doubt that. The point is, do we have any idea how they behave
> under KVM? Do they generate complex MMIO accesses? Do they expect firmware
> shims, possibly sitting above hyp? Do they require a signed boot sequence?
> Do they run on Cortex-A15 (the only target CPU we have at the moment)?
>

No we don't know. But there's a fair chance that they do use complex
mmio instructions seeing as older kernels did, without anything
explicitly being involved.

>> But, however a shame, I can more easily maintain this single patch
>> out-of-tree, so I'm willing to drop this logic for now if it gets
>> things moving.
>
> I would hope that, if this code is actually required, you would consider
> merging it with what we have rather than maintaining it out-of-tree.
>
Of course I would, and I would also make an effort to unify the code
if it were merged now, I just don't have the cycles to do the unify
work right now, since it is without doubt a lengthy process.

So from that point of view, I don't quite see how it's better to leave
the code out at this point, but that is not up to me.

-Christoffer