Re: [Qemu-devel] [PATCH v3 09/19] pc: delay setting number of boot CPUs to machine_done time

[Eduardo Habkost]

On Wed, Jul 13, 2016 at 09:56:25AM +0200, Igor Mammedov wrote:
> On Tue, 12 Jul 2016 14:18:22 -0300
> Eduardo Habkost <address@hidden> wrote:
> 
> > On Tue, Jul 12, 2016 at 02:48:43PM +0200, Igor Mammedov wrote:
> > > On Tue, 12 Jul 2016 00:29:08 -0300
> > > Eduardo Habkost <address@hidden> wrote:
> > >   
> > > > On Wed, Jul 06, 2016 at 08:20:45AM +0200, Igor Mammedov wrote:  
> > > > > currently present CPUs counter in CMOS only contains
> > > > > smp_cpus (i.e. initial CPUs specified with -smp X) and
> > > > > doesn't account for CPUs created with -device.
> > > > > If VM is started with additional CPUs added with
> > > > >  -device, it will hang in BIOS waiting for condition
> > > > >    smp_cpus == counted_cpus
> > > > > forever as counted_cpus will include -device CPUs as well
> > > > > and be more than smp_cpus.
> > > > > 
> > > > > make present CPUs counter in CMOS to count all CPUs
> > > > > (initial and coldplugged with -device) by delaying
> > > > > it to machine done time when it possible to count
> > > > > CPUs added with -device.    
> > > > 
> > > > Do you plan to fix the remaining code using smp_cpus? e.g.:  
> > > perhaps after Drew's -smp refactoring or maybe during it,
> > > it looks like good candidate for that series,
> > > I'll work with Drew on that.
> > >   
> > > > 
> > > > 1) x86_cpu_realizefn():
> > > >     if (cpu->env.features[FEAT_1_EDX] & CPUID_APIC || smp_cpus > 1) {
> > > >             x86_cpu_apic_create(cpu, &local_err);
> > > >     [...]
> > > >   (Maybe we should simply make realizefn fail if we try to create a 
> > > > second CPU
> > > >   using -device or device_add without CPUID_APIC)  
> > > wouldn't that break some setups that doing it but still able
> > > to boot?  
> > 
> > It would, that's why we need to do it only in the case of -device
> > or device_add. What about something like: 
> >   if (!(cpu->env.features[FEAT_1_EDX] & CPUID_APIC) && smp_cpus < 2 
> > &&total_cpus_already_created() > 0) {
> >       error_setg("we can't create a new VCPU without an APIC");
> >       return;
> -device/device_add is orthogonal here, cpu-add is also affected.
> 
> I'd woul make it a warning instead of hard error:
> "broken configuration, only 1st CPU will work when creating multiple CPUs 
> with APIC disabled,
> to fix it remove -apic/apic=off from -cpu option"

I forgot about cpu-add. You're right.

> 
> >   }
> > 
> > I believe this logic should be moved to PC code, eventually. Or
> > at least the process should be controlled by PC code (by setting
> > a force-apic-creation property in X86CPU, for example).
> smp_cpus check should definitely be part of PC code.
> 
> PS:
> You do not consider above as part of this series, do you?

No, it's just something to be done later.

-- 
Eduardo