| Message ID | 20181116105729.23240-13-clg@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ppc: support for the XIVE interrupt controller (POWER9) | expand |
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 <clg@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? > --- > 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); >
On Wed, 28 Nov 2018 13:57:14 +1100 David Gibson <david@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 <clg@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(). 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. > > --- > > 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); > > >
On 11/28/18 10:35 AM, Greg Kurz wrote: > On Wed, 28 Nov 2018 13:57:14 +1100 > David Gibson <david@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 <clg@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(). > > 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. 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); >>> >> >
On Wed, 28 Nov 2018 17:50:21 +0100 Cédric Le Goater <clg@kaod.org> wrote: > On 11/28/18 10:35 AM, Greg Kurz wrote: > > On Wed, 28 Nov 2018 13:57:14 +1100 > > David Gibson <david@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 <clg@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(). > > > > 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); > >>> > >> > > >
On Wed, Nov 28, 2018 at 10:35:51AM +0100, Greg Kurz wrote: > On Wed, 28 Nov 2018 13:57:14 +1100 > David Gibson <david@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 <clg@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. > > > > --- > > > 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); > > > > > >
On Thu, 29 Nov 2018 12:02:48 +1100 David Gibson <david@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 <david@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 <clg@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 <groug@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); > > > > > > > > > > > >
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);
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 <clg@kaod.org> --- hw/ppc/spapr.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)