Re: [RFC PATCH 1/3] hw/intc/s390_flic: Migrate pending state

[Thomas Huth]

On 26/05/2024 22.33, David Hildenbrand wrote:
> Am 26.05.24 um 21:44 schrieb Richard Henderson:
> > On 5/26/24 08:53, David Hildenbrand wrote:
> > > Am 25.05.24 um 15:12 schrieb Nicholas Piggin:
> > > > The flic pending state is not migrated, so if the machine is migrated
> > > > while an interrupt is pending, it can be lost. This shows up in
> > > > qtest migration test, an extint is pending (due to console writes?)
> > > > and the CPU waits via s390_cpu_set_psw and expects the interrupt to
> > > > wake it. However when the flic pending state is lost, s390_cpu_has_int
> > > > returns false, so s390_cpu_exec_interrupt falls through to halting
> > > > again.
> > > >
> > > > Fix this by migrating pending. This prevents the qtest from hanging.
> > > > Does service_param need to be migrated? Or the IO lists?
> > > >
> > > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > > > ---
> > > >   hw/intc/s390_flic.c | 1 +
> > > >   1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
> > > > index 6771645699..b70cf2295a 100644
> > > > --- a/hw/intc/s390_flic.c
> > > > +++ b/hw/intc/s390_flic.c
> > > > @@ -369,6 +369,7 @@ static const VMStateDescription 
> > > > qemu_s390_flic_vmstate = {
> > > >       .fields = (const VMStateField[]) {
> > > >           VMSTATE_UINT8(simm, QEMUS390FLICState),
> > > >           VMSTATE_UINT8(nimm, QEMUS390FLICState),
> > > > +        VMSTATE_UINT32(pending, QEMUS390FLICState),
> > > >           VMSTATE_END_OF_LIST()
> > > >       }
> > > >   };
> > >
> > > Likely you have to handle this using QEMU compat machines.
> >
> > Well, since existing migration is broken, I don't think you have to preserve 
>
> Migration is broken only in some case "while an interrupt is pending, it can 
> be lost".
>
> > compatibility.  But you do have to bump the version number.
>
> Looking at it, this is TCG only, so likely we don't care that much about 
> migration compatibility. But I have no idea what level of compatibility we 
> want to support there.

Yes, this seems to only affect the TCG-only flic device, where migration has 
never been working very reliably. So I think we don't really need the whole 
compat-machine dance here. But I think we should at least bump the 
version_id to 2 now and then use

   VMSTATE_UINT32_V(pending, QEMUS390FLICState, 2);

for the new field. That way we would at least support forward migrations 
without too much hassle.

 Thomas