Hi Christoffer, Marc, On 07/10/2018 10:33 AM, Marc Zyngier wrote: > 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. Sorry I did not have time to review the series properly. My understanding is we generally care about migration between different kernels. For the ITS, we made the IIDR writable too to manage the ABI version properly. See ab01c6bdacc43c41c6b326889f4358f5afc38bf9. But maybe I missed the crux of the matter. Thanks Eric > > Thanks, > > M. >
