| Message ID | 1495172439-1504-5-git-send-email-bharata@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > + err = kvmppc_configure_v3_mmu(cpu, > SPAPR_PROC_TABLE_RADIX, > + ((cpu->env.spr[SPR_LPCR] & LPCR_GTSE) ? > SPAPR_PROC_TABLE_GTSE : > + 0), spapr->patb_entry); > + } else { > + error_report("Radix guest is unsupported by the host"); > + return -EINVAL; > + } > + } > + > return err; > } >
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() ? 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); 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 { /* Hash guest (current), nothing to do */ } Regards, Bharata.
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
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 <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. 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.
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); } + if (spapr->patb_entry) { + PowerPCCPU *cpu = POWERPC_CPU(first_cpu); + if (kvmppc_has_cap_mmu_radix() && kvm_enabled()) { + err = kvmppc_configure_v3_mmu(cpu, SPAPR_PROC_TABLE_RADIX, + ((cpu->env.spr[SPR_LPCR] & LPCR_GTSE) ? SPAPR_PROC_TABLE_GTSE : + 0), spapr->patb_entry); + } else { + error_report("Radix guest is unsupported by the host"); + return -EINVAL; + } + } + return err; }
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(+)