Re: [Qemu-devel] [Qemu-ppc] [PATCH v5 12/36] spapr: initialize VSMT before initializing the IRQ backend

[David Gibson]

On Wed, Nov 28, 2018 at 10:35:51AM +0100, Greg Kurz wrote:
> On Wed, 28 Nov 2018 13:57:14 +1100
> David Gibson <address@hidden> 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 <address@hidden>  
> > 
> > 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.

> 
> > > ---
> > >  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);
> > >    
> > 
> 



-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature