Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH v2 4/4] spapr: Fix migration of Radix guests

[Bharata B Rao]

On Tue, May 23, 2017 at 06:42:28PM +1000, Suraj Jitindar Singh wrote:
> On Tue, 2017-05-23 at 10:18 +0530, Bharata B Rao wrote:
> > On Mon, May 22, 2017 at 04:30:50PM +1000, Suraj Jitindar Singh wrote:
> > > On Fri, 2017-05-19 at 11:10 +0530, Bharata B Rao wrote:
> > > > Fix migration of radix guests by ensuring that we issue
> > > > KVM_PPC_CONFIGURE_V3_MMU for radix case post migration.
> > > > 
> > > > Reported-by: Nageswara R Sastry <address@hidden>
> > > > Signed-off-by: Bharata B Rao <address@hidden>
> > > > ---
> > > >  hw/ppc/spapr.c | 12 ++++++++++++
> > > >  1 file changed, 12 insertions(+)
> > > > 
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index daf335c..8f20f14 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -1400,6 +1400,18 @@ static int spapr_post_load(void *opaque,
> > > > int
> > > > version_id)
> > > >          err = spapr_rtc_import_offset(&spapr->rtc, spapr-
> > > > > rtc_offset);
> > > > 
> > > >      }
> > > 
> > > This will break migration for tcg radix guests.
> > > 
> > > Given that there is essentially nothing special we need to do on
> > > incoming tcg migration, I suggest we make it:
> > > 
> > > if (spapr->patb_entry && kvm_enabled()) {
> > >     [snip]
> > > }
> > > 
> > > >  
> > > > +    if (spapr->patb_entry) {
> > > > +        PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> > > > +        if (kvmppc_has_cap_mmu_radix() && kvm_enabled()) {
> > > 
> > > Why not make this a bit more generic? Something like:
> > > 
> > > err = kvmppc_configure_v3_mmu(cpu, !!(spapr->patb_entry &
> > > PATBE1_GR),
> > > !!(cpu->env.spr[SPR_LPCR] & LPCR_GTSE), spapr->patb_entry);
> > > if (err) {
> > >     error_report("Process table config unsupported by the host");
> > >     return -EINVAL;
> > > }
> > > 
> > > return err;
> > > 
> > > While I don't think it's possible currently, there is nothing
> > > inherently incorrect about having a non-zero process table entry
> > > for a
> > > hash guest. And this saves a future update.
> > 
> > How about having this logic in spapr_post_load() ?
> 
> Looks a lot better :)
> 
> > 
> >     if (spapr->patb_entry) {
> >         /* Can be Hash(in future?) or Radix guest (current) */
> >         PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> >         bool radix = !!(spapr->patb_entry & PATBE1_GR);
> >         bool gtse = !!(cpu->env.spr[SPR_LPCR] & LPCR_GTSE);
> > 
> 
> Don't think we need this if statement though. When hash with patb entry
> is possible it will still need to call kvmppc_configure_v3_mmu on
> incoming migration, that isn't radix specific.

Right.

> 
> >         if (radix) {
> >             /* Radix guest, configure MMU */
> >             /* kvmppc_configure_v3_mmu() is NOP for TCG */
> >             err = kvmppc_configure_v3_mmu(cpu, radix, gtse, spapr-
> > >patb_entry);
> >             if (err) {
> >                 error_report("Process table config unsupported by the
> > host");
> >                 return -EINVAL;
> >             }
> >         } else {
> >             /* Can be Hash guest (in future ?), nothing to do */
> >         }
> >     } else {
> 
> Don't need this else statement. Can just have the comment below if you
> feel it's necessary.

Was just being verbose with empty else blocks, will not have them in the
actual patch.

Thanks for the review.

Regards,
Bharata.