[Qemu-devel] of apci_1_compatible in CPUHotplugFeatures

[Qemu-devel] of apci_1_compatible in CPUHotplugFeatures

[Dr. David Alan Gilbert]

Hi,
  I noticed that the acpi_1_compatible flag was misspelt as
                     apci_1_compatible

so have a trivial patch to fix that, but looking at it - are
thre any cases where a[cp]i_1_compatible can possibly be false?

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] of apci_1_compatible in CPUHotplugFeatures

[Michael S. Tsirkin]

On Tue, Jan 22, 2019 at 08:07:41PM +0000, Dr. David Alan Gilbert wrote:
> Hi,
>   I noticed that the acpi_1_compatible flag was misspelt as
>                      apci_1_compatible
> 
> so have a trivial patch to fix that, but looking at it - are
> thre any cases where a[cp]i_1_compatible can possibly be false?
> 
> Dave

legacy_cpu_hotplug so machine 2.6 and older - no?

> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] of apci_1_compatible in CPUHotplugFeatures

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> On Tue, Jan 22, 2019 at 08:07:41PM +0000, Dr. David Alan Gilbert wrote:
> > Hi,
> >   I noticed that the acpi_1_compatible flag was misspelt as
> >                      apci_1_compatible
> > 
> > so have a trivial patch to fix that, but looking at it - are
> > thre any cases where a[cp]i_1_compatible can possibly be false?
> > 
> > Dave
> 
> legacy_cpu_hotplug so machine 2.6 and older - no?

That doesn't seem to affect that flag by my reading;
the only place I see legacy_cpu_hotplug checked is
acpi-build.c:build_dsdt and we have:

    if (pcmc->legacy_cpu_hotplug) {
        build_legacy_cpu_hotplug_aml(dsdt, machine, pm->cpu_hp_io_base);
    } else {
        CPUHotplugFeatures opts = {
            .acpi_1_compatible = true, .has_legacy_cphp = true
        };
        build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
                       "\\_SB.PCI0", "\\_GPE._E02");
    }

so the 'opts' field is only used in the non-legacy case.

That's the only caller of build_cpus_aml, and I'm not seeing another
user of CPUHotplugFeatures.

Dave

> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] of apci_1_compatible in CPUHotplugFeatures

[Igor Mammedov]

On Wed, 23 Jan 2019 18:28:59 +0000
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:

> * Michael S. Tsirkin (mst@redhat.com) wrote:
> > On Tue, Jan 22, 2019 at 08:07:41PM +0000, Dr. David Alan Gilbert wrote:  
> > > Hi,
> > >   I noticed that the acpi_1_compatible flag was misspelt as
> > >                      apci_1_compatible
> > > 
> > > so have a trivial patch to fix that,
pls post it.

> > > but looking at it - are
> > > thre any cases where a[cp]i_1_compatible can possibly be false?
ATM it's not possible, but I've wrote it with intent to reuse
build_cpus_aml() in arm/virt board and there we shall use newer
aml_device() instead of legacy aml_processor(), hence a feature flag
to toggle behavior.


> > > 
> > > Dave  
> > 
> > legacy_cpu_hotplug so machine 2.6 and older - no?  
> 
> That doesn't seem to affect that flag by my reading;
> the only place I see legacy_cpu_hotplug checked is
> acpi-build.c:build_dsdt and we have:
> 
>     if (pcmc->legacy_cpu_hotplug) {
>         build_legacy_cpu_hotplug_aml(dsdt, machine, pm->cpu_hp_io_base);
>     } else {
>         CPUHotplugFeatures opts = {
>             .acpi_1_compatible = true, .has_legacy_cphp = true
>         };
>         build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
>                        "\\_SB.PCI0", "\\_GPE._E02");
>     }
> 
> so the 'opts' field is only used in the non-legacy case.
> 
> That's the only caller of build_cpus_aml, and I'm not seeing another
> user of CPUHotplugFeatures.
> 
> Dave
> 
> > > --
> > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK  
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] of apci_1_compatible in CPUHotplugFeatures

[Dr. David Alan Gilbert]

* Igor Mammedov (imammedo@redhat.com) wrote:
> On Wed, 23 Jan 2019 18:28:59 +0000
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> 
> > * Michael S. Tsirkin (mst@redhat.com) wrote:
> > > On Tue, Jan 22, 2019 at 08:07:41PM +0000, Dr. David Alan Gilbert wrote:  
> > > > Hi,
> > > >   I noticed that the acpi_1_compatible flag was misspelt as
> > > >                      apci_1_compatible
> > > > 
> > > > so have a trivial patch to fix that,
> pls post it.

Done.

> > > > but looking at it - are
> > > > thre any cases where a[cp]i_1_compatible can possibly be false?
> ATM it's not possible, but I've wrote it with intent to reuse
> build_cpus_aml() in arm/virt board and there we shall use newer
> aml_device() instead of legacy aml_processor(), hence a feature flag
> to toggle behavior.

OK, that's fine, I just couldn't see why the flag was there given the
current uses.

Dave

> 
> > > > 
> > > > Dave  
> > > 
> > > legacy_cpu_hotplug so machine 2.6 and older - no?  
> > 
> > That doesn't seem to affect that flag by my reading;
> > the only place I see legacy_cpu_hotplug checked is
> > acpi-build.c:build_dsdt and we have:
> > 
> >     if (pcmc->legacy_cpu_hotplug) {
> >         build_legacy_cpu_hotplug_aml(dsdt, machine, pm->cpu_hp_io_base);
> >     } else {
> >         CPUHotplugFeatures opts = {
> >             .acpi_1_compatible = true, .has_legacy_cphp = true
> >         };
> >         build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
> >                        "\\_SB.PCI0", "\\_GPE._E02");
> >     }
> > 
> > so the 'opts' field is only used in the non-legacy case.
> > 
> > That's the only caller of build_cpus_aml, and I'm not seeing another
> > user of CPUHotplugFeatures.
> > 
> > Dave
> > 
> > > > --
> > > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK  
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] of apci_1_compatible in CPUHotplugFeatures

[Michael S. Tsirkin]

On Fri, Jan 25, 2019 at 09:26:05AM +0100, Igor Mammedov wrote:
> On Wed, 23 Jan 2019 18:28:59 +0000
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> 
> > * Michael S. Tsirkin (mst@redhat.com) wrote:
> > > On Tue, Jan 22, 2019 at 08:07:41PM +0000, Dr. David Alan Gilbert wrote:  
> > > > Hi,
> > > >   I noticed that the acpi_1_compatible flag was misspelt as
> > > >                      apci_1_compatible
> > > > 
> > > > so have a trivial patch to fix that,
> pls post it.
> 
> > > > but looking at it - are
> > > > thre any cases where a[cp]i_1_compatible can possibly be false?
> ATM it's not possible, but I've wrote it with intent to reuse
> build_cpus_aml() in arm/virt board and there we shall use newer
> aml_device() instead of legacy aml_processor(), hence a feature flag
> to toggle behavior.

Are there imminent plans to do this? If not why don't we
drop unused code? One can always bring it back from git
history if need arises but for now there's no guarantee
it's not broken as it never runs.

> 
> > > > 
> > > > Dave  
> > > 
> > > legacy_cpu_hotplug so machine 2.6 and older - no?  
> > 
> > That doesn't seem to affect that flag by my reading;
> > the only place I see legacy_cpu_hotplug checked is
> > acpi-build.c:build_dsdt and we have:
> > 
> >     if (pcmc->legacy_cpu_hotplug) {
> >         build_legacy_cpu_hotplug_aml(dsdt, machine, pm->cpu_hp_io_base);
> >     } else {
> >         CPUHotplugFeatures opts = {
> >             .acpi_1_compatible = true, .has_legacy_cphp = true
> >         };
> >         build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
> >                        "\\_SB.PCI0", "\\_GPE._E02");
> >     }
> > 
> > so the 'opts' field is only used in the non-legacy case.
> > 
> > That's the only caller of build_cpus_aml, and I'm not seeing another
> > user of CPUHotplugFeatures.
> > 
> > Dave
> > 
> > > > --
> > > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK  
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] of apci_1_compatible in CPUHotplugFeatures

[Igor Mammedov]

On Fri, 25 Jan 2019 14:49:28 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Jan 25, 2019 at 09:26:05AM +0100, Igor Mammedov wrote:
> > On Wed, 23 Jan 2019 18:28:59 +0000
> > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> >   
> > > * Michael S. Tsirkin (mst@redhat.com) wrote:  
> > > > On Tue, Jan 22, 2019 at 08:07:41PM +0000, Dr. David Alan Gilbert wrote:    
> > > > > Hi,
> > > > >   I noticed that the acpi_1_compatible flag was misspelt as
> > > > >                      apci_1_compatible
> > > > > 
> > > > > so have a trivial patch to fix that,  
> > pls post it.
> >   
> > > > > but looking at it - are
> > > > > thre any cases where a[cp]i_1_compatible can possibly be false?  
> > ATM it's not possible, but I've wrote it with intent to reuse
> > build_cpus_aml() in arm/virt board and there we shall use newer
> > aml_device() instead of legacy aml_processor(), hence a feature flag
> > to toggle behavior.  
> 
> Are there imminent plans to do this? If not why don't we
> drop unused code? One can always bring it back from git
> history if need arises but for now there's no guarantee
> it's not broken as it never runs.
I'm working on it (I mean adding CPU hotplug support for ARM (QEMU side of it)),
so I'd keep it.


> >   
> > > > > 
> > > > > Dave    
> > > > 
> > > > legacy_cpu_hotplug so machine 2.6 and older - no?    
> > > 
> > > That doesn't seem to affect that flag by my reading;
> > > the only place I see legacy_cpu_hotplug checked is
> > > acpi-build.c:build_dsdt and we have:
> > > 
> > >     if (pcmc->legacy_cpu_hotplug) {
> > >         build_legacy_cpu_hotplug_aml(dsdt, machine, pm->cpu_hp_io_base);
> > >     } else {
> > >         CPUHotplugFeatures opts = {
> > >             .acpi_1_compatible = true, .has_legacy_cphp = true
> > >         };
> > >         build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
> > >                        "\\_SB.PCI0", "\\_GPE._E02");
> > >     }
> > > 
> > > so the 'opts' field is only used in the non-legacy case.
> > > 
> > > That's the only caller of build_cpus_aml, and I'm not seeing another
> > > user of CPUHotplugFeatures.
> > > 
> > > Dave
> > >   
> > > > > --
> > > > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK    
> > > --
> > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK