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