Re: [PATCH v2 6/6] KVM: arm/arm64: vgic: Allow configuration of interrupt groups

Marc Zyngier
2018-07-10T08:33:50Z

On 09/07/18 23:48, Christoffer Dall wrote:
> On Mon, Jul 09, 2018 at 09:42:40AM +0100, Marc Zyngier wrote:
>> On 04/07/18 10:38, Christoffer Dall wrote:
>>> Implement the required MMIO accessors for GICv2 and GICv3 for the
>>> IGROUPR distributor and redistributor registers.
>>>
>>> This can allow guests to change behavior compared to running on previous
>>> versions of KVM, but only to align with the architecture and hardware
>>> implementations.
>>>
>>> This also allows userspace to configure the groups for interrupts. Note
>>> that this potentially results in GICv2 guests not receiving interrupts
>>> after migration if migrating from an older kernel that exposes GICv2
>>> interrupts as group 1.
>>>
>>> Cc: Andrew Jones <drjones@redhat.com>
>>> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
>>> ---
>>> I implemented (but stashed) a version of this which predicated the
>>> behavior based on the value of GICD_IIDR revision field, falling back to
>>> ignoring writes and resetting GICv2 groups to 0 if the guest wrote a
>>> revision less than 2. However, current QEMU implementations simply
>>> don't write the GICD_IIDR, so this doesn't help at all without changing
>>> QEMU anyhow.
>>>
>>> The only actual fix I can see here to work around the problem in the
>>> kernel is to require an opt-in to allow restoring groups from userspace,
>>> but that's a lot of logic to support cross-kernel version migration.
>>>
>>> Question: Do we expect that cross-kernel version migration is a critical
>>> feature that people really expect to work, and do we actually have
>>> examples of catering to this in the kernel elsewhere? (Also, how would
>>> then that relate to the whole 'adding a new sysreg breaks migration'
>>> situation?)
>>
>> I don't really get why QEMU doesn't try to restore GICD_IIDR, while it
>> is definitely trying to restore RO sysregs (and that's how we detect
>> incompatibilities).
>>
>> I think we should at least give userspace a chance to do the right
>> thing. If it doesn't, well, too bad.
>
> This series should give userspace an option to adjust its behavior.
>
> My main concern is that this version of the series results in the worst
> kind of migration failures, where the guest simply doesn't run on the
> destination side with no warnings or error messages returned to the
> user..

I agree, which is why I'm suggesting we make IIDR writeable. And yes, it
still requires userspace to be fixed to actually write IIDR.

>
> We could add logic to return an error code if trying to write a
> different revision than what the kernel has (similar to the invariant
> sysregs), so that a simple fix to QEMU to save restore the GICD_IIDR
> register at least results in an error being returned to userspace.
>
> However, as QEMU doesn't do anything useful here today (not blaming
> anyone, I wrote the apparently broken GIC save/restore code for QEMU
> myself), we could also argue that QEMU might as well just fix things up
> if it detects a different IIDR.
>
>>
>> How bad is that "writable GICD_IIDR" patch? Because at this stage, and
>> in the absence of any comment, I'm close to just pick that series and
>> merge it for 4.19.
>>
>
> My guess is that a patch to "save/restore GICD_IIDR" is simple enough,
> but requires additional logic in the kernel that returns an error if the
> GICD_IIDR don't match on write.
>
> A patch which changes the groups and bumps the IIDR in userspace is
> probably more complex.
>
> Sounds like I should add the GICD_IIDR checking patch. Thoughts?

I'm quite keen on that. It makes the userspace change trivial, aligns it
somehow on the sysreg and ITS ABI behaviours.

> What I would really like to know is whether this is really an issue or
> not. Do people who run products based on KVM, such as RHEV, have any
> expectations about cross-kernel version migration?

I'd like to know as well.

Thanks,

M.
--
Jazz is not dead. It just smells funny...