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

[Suraj Jitindar Singh <sjitindarsingh@gmail.com>]

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 <rnsastry@linux.vnet.ibm.com>
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > >  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.

>         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.

>         /* Hash guest (current), nothing to do */
>     }
> 
> Regards,
> Bharata.
> 

Thanks,
Suraj