On Thu, 29 Nov 2018 12:02:48 +1100 David Gibson <da...@gibson.dropbear.id.au> wrote:
> On Wed, Nov 28, 2018 at 10:35:51AM +0100, Greg Kurz wrote: > > On Wed, 28 Nov 2018 13:57:14 +1100 > > David Gibson <da...@gibson.dropbear.id.au> wrote: > > > > > On Fri, Nov 16, 2018 at 11:57:05AM +0100, Cédric Le Goater wrote: > > > > We will need to use xics_max_server_number() to create the sPAPRXive > > > > object modeling the interrupt controller of the machine which is > > > > created before the CPUs. > > > > > > > > Signed-off-by: Cédric Le Goater <c...@kaod.org> > > > > > > My only concern here is that this moves the spapr_set_vsmt_mode() > > > before some of the sanity checks in spapr_init_cpus(). Are we certain > > > there are no edge cases that could cause badness? > > > > > > > The early checks in spapr_init_cpus() filter out topologies that would > > result in partially filled cores. They're only related to the rest of > > the code that creates the boot CPUs. Before commit 1a5008fc17, > > spapr_set_vsmt_mode() was even being called before spapr_init_cpus(). > > The rationale to move it there was to ensure it is called before the > > first user of spapr->vsmt, which happens to be a call to > > xics_max_server_number(). > > Ok. > > > Now that xics_max_server_number() needs to be called even earlier, I think a > > better change is to have xics_max_server_number() to call > > spapr_set_vsmt_mode() > > if spapr->vsmt isn't set. > > I'd rather not do that, but instead move it statically to where it > needs to be. That sort of lazy/on-demand initialization can result in > really confusing behaviours depending on when a seemingly innocuous > data-returning function is called, so I consider it a code smell. > Fair enough, then: Reviewed-by: Greg Kurz <gr...@kaod.org> > > > > > > --- > > > > hw/ppc/spapr.c | 10 +++++----- > > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > > index 7afd1a175bf2..50cb9f9f4a02 100644 > > > > --- a/hw/ppc/spapr.c > > > > +++ b/hw/ppc/spapr.c > > > > @@ -2466,11 +2466,6 @@ static void spapr_init_cpus(sPAPRMachineState > > > > *spapr) > > > > boot_cores_nr = possible_cpus->len; > > > > } > > > > > > > > - /* VSMT must be set in order to be able to compute VCPU ids, ie to > > > > - * call xics_max_server_number() or spapr_vcpu_id(). > > > > - */ > > > > - spapr_set_vsmt_mode(spapr, &error_fatal); > > > > - > > > > if (smc->pre_2_10_has_unused_icps) { > > > > int i; > > > > > > > > @@ -2593,6 +2588,11 @@ static void spapr_machine_init(MachineState > > > > *machine) > > > > /* Setup a load limit for the ramdisk leaving room for SLOF and > > > > FDT */ > > > > load_limit = MIN(spapr->rma_size, RTAS_MAX_ADDR) - FW_OVERHEAD; > > > > > > > > + /* VSMT must be set in order to be able to compute VCPU ids, ie to > > > > + * call xics_max_server_number() or spapr_vcpu_id(). > > > > + */ > > > > + spapr_set_vsmt_mode(spapr, &error_fatal); > > > > + > > > > /* Set up Interrupt Controller before we create the VCPUs */ > > > > smc->irq->init(spapr, &error_fatal); > > > > > > > > > > > >
pgp35Qoq9Zp0U.pgp
Description: OpenPGP digital signature