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

[Greg Kurz]

On Wed, 28 Nov 2018 17:50:21 +0100
Cédric Le Goater <address@hidden> wrote:

> On 11/28/18 10:35 AM, 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().
> > 
> > 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.  
> 
> That 'smt' routine is black magic to me and I would not dare touching it.
> 

I don't suggest to change it, just to ensure it gets called before spapr->vsmt
gets used. But don't worry, I'll have a deeper look and send a patch :)

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