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@xxxxxxxxxx> >>> Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxx> >>> --- >>> 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...
