diff mbox series

hw/arm/Kconfig: no need to enable ACPI_MEMORY_HOTPLUG explicitly

Message ID 20210812033405.362985-1-ani@anisinha.ca (mailing list archive)
State New, archived
Headers show
Series hw/arm/Kconfig: no need to enable ACPI_MEMORY_HOTPLUG explicitly | expand

Commit Message

Ani Sinha Aug. 12, 2021, 3:34 a.m. UTC 
ACPI_MEMORY_HOTPLUG is implicitly turned on when ACPI_HW_REDUCED is selected.
ACPI_HW_REDUCED is already enabled. No need to turn on ACPI_MEMORY_HOTPLUG
explicitly. This is a minor cleanup.

Signed-off-by: Ani Sinha <ani@anisinha.ca>
---
 hw/arm/Kconfig | 1 -
 1 file changed, 1 deletion(-)

Comments

Ani Sinha Aug. 17, 2021, 4:44 a.m. UTC | #1 
ping ...

On Thu, 12 Aug 2021, Ani Sinha wrote:

> ACPI_MEMORY_HOTPLUG is implicitly turned on when ACPI_HW_REDUCED is selected.
> ACPI_HW_REDUCED is already enabled. No need to turn on ACPI_MEMORY_HOTPLUG
> explicitly. This is a minor cleanup.
>
> Signed-off-by: Ani Sinha <ani@anisinha.ca>
> ---
>  hw/arm/Kconfig | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 4ba0aca067..38cf9f44e2 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -25,7 +25,6 @@ config ARM_VIRT
>      select ACPI_PCI
>      select MEM_DEVICE
>      select DIMM
> -    select ACPI_MEMORY_HOTPLUG
>      select ACPI_HW_REDUCED
>      select ACPI_NVDIMM
>      select ACPI_APEI
> --
> 2.25.1
>
>
Peter Maydell Aug. 19, 2021, 1:05 p.m. UTC | #2 
On Tue, 17 Aug 2021 at 05:45, Ani Sinha <ani@anisinha.ca> wrote:
>
> ping ...

You didn't cc any of the ACPI maintainers; I have done so.

Is it intended that ACPI_HW_REDUCED must always imply
ACPI_MEMORY_HOTPLUG, or is it just a coincidence that the
virt board happens to want both, and so we select both ?

> On Thu, 12 Aug 2021, Ani Sinha wrote:
>
> > ACPI_MEMORY_HOTPLUG is implicitly turned on when ACPI_HW_REDUCED is selected.
> > ACPI_HW_REDUCED is already enabled. No need to turn on ACPI_MEMORY_HOTPLUG
> > explicitly. This is a minor cleanup.
> >
> > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > ---
> >  hw/arm/Kconfig | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> > index 4ba0aca067..38cf9f44e2 100644
> > --- a/hw/arm/Kconfig
> > +++ b/hw/arm/Kconfig
> > @@ -25,7 +25,6 @@ config ARM_VIRT
> >      select ACPI_PCI
> >      select MEM_DEVICE
> >      select DIMM
> > -    select ACPI_MEMORY_HOTPLUG
> >      select ACPI_HW_REDUCED
> >      select ACPI_NVDIMM
> >      select ACPI_APEI
> > --
> > 2.25.1
> >
> >


-- PMM
Ani Sinha Aug. 19, 2021, 1:25 p.m. UTC | #3 
On Thu, 19 Aug 2021, Peter Maydell wrote:

> On Tue, 17 Aug 2021 at 05:45, Ani Sinha <ani@anisinha.ca> wrote:
> >
> > ping ...
>
> You didn't cc any of the ACPI maintainers; I have done so.
>

oops! my bad. I used checkpatch and did not verify if Igor/Michael was
cc'd.

> Is it intended that ACPI_HW_REDUCED must always imply
> ACPI_MEMORY_HOTPLUG, or is it just a coincidence that the
> virt board happens to want both, and so we select both ?
>

From a purely code inspection point of view, I noticed that
generic_event_device.c depends on CONFIG_ACPI_HW_REDUCED. The GED does use
memory hotplug apis - for example acpi_ged_device_plug_cb() uses
acpi_memory_plug_cb() etc.

Hence, as it stands today, CONFIG_ACPI_HW_REDUCED will need to select ACPI
memory hotplug. Unless we remove the GED device's dependence on
ACPI_HW_REDUCED that is. I cannot comment whether that would be wise or if
we should reorg the code in some other way.


> > On Thu, 12 Aug 2021, Ani Sinha wrote:
> >
> > > ACPI_MEMORY_HOTPLUG is implicitly turned on when ACPI_HW_REDUCED is selected.
> > > ACPI_HW_REDUCED is already enabled. No need to turn on ACPI_MEMORY_HOTPLUG
> > > explicitly. This is a minor cleanup.
> > >
> > > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > > ---
> > >  hw/arm/Kconfig | 1 -
> > >  1 file changed, 1 deletion(-)
> > >
> > > diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> > > index 4ba0aca067..38cf9f44e2 100644
> > > --- a/hw/arm/Kconfig
> > > +++ b/hw/arm/Kconfig
> > > @@ -25,7 +25,6 @@ config ARM_VIRT
> > >      select ACPI_PCI
> > >      select MEM_DEVICE
> > >      select DIMM
> > > -    select ACPI_MEMORY_HOTPLUG
> > >      select ACPI_HW_REDUCED
> > >      select ACPI_NVDIMM
> > >      select ACPI_APEI
> > > --
> > > 2.25.1
> > >
> > >
>
>
> -- PMM
>
Ani Sinha Aug. 19, 2021, 1:36 p.m. UTC | #4 
On Thu, 19 Aug 2021, Ani Sinha wrote:

>
>
> On Thu, 19 Aug 2021, Peter Maydell wrote:
>
> > On Tue, 17 Aug 2021 at 05:45, Ani Sinha <ani@anisinha.ca> wrote:
> > >
> > > ping ...
> >
> > You didn't cc any of the ACPI maintainers; I have done so.
> >
>
> oops! my bad. I used checkpatch and did not verify if Igor/Michael was
> cc'd.
>
> > Is it intended that ACPI_HW_REDUCED must always imply
> > ACPI_MEMORY_HOTPLUG, or is it just a coincidence that the
> > virt board happens to want both, and so we select both ?
> >
>
> From a purely code inspection point of view, I noticed that
> generic_event_device.c depends on CONFIG_ACPI_HW_REDUCED. The GED does use
> memory hotplug apis - for example acpi_ged_device_plug_cb() uses
> acpi_memory_plug_cb() etc.
>
> Hence, as it stands today, CONFIG_ACPI_HW_REDUCED will need to select ACPI
> memory hotplug. Unless we remove the GED device's dependence on
> ACPI_HW_REDUCED that is. I cannot comment whether that would be wise or if
> we should reorg the code in some other way.

The other question we should ask is whether arm platform requires
ACPI_MEMORY_HOTPLUG independent of ACPI_HW_REDUCED/GED device? If that is
the case, then maybe we should keep that config option as is.
Maybe @qemu-arm can answer that?

>
> > > On Thu, 12 Aug 2021, Ani Sinha wrote:
> > >
> > > > ACPI_MEMORY_HOTPLUG is implicitly turned on when ACPI_HW_REDUCED is selected.
> > > > ACPI_HW_REDUCED is already enabled. No need to turn on ACPI_MEMORY_HOTPLUG
> > > > explicitly. This is a minor cleanup.
> > > >
> > > > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > > > ---
> > > >  hw/arm/Kconfig | 1 -
> > > >  1 file changed, 1 deletion(-)
> > > >
> > > > diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> > > > index 4ba0aca067..38cf9f44e2 100644
> > > > --- a/hw/arm/Kconfig
> > > > +++ b/hw/arm/Kconfig
> > > > @@ -25,7 +25,6 @@ config ARM_VIRT
> > > >      select ACPI_PCI
> > > >      select MEM_DEVICE
> > > >      select DIMM
> > > > -    select ACPI_MEMORY_HOTPLUG
> > > >      select ACPI_HW_REDUCED
> > > >      select ACPI_NVDIMM
> > > >      select ACPI_APEI
> > > > --
> > > > 2.25.1
> > > >
> > > >
> >
> >
> > -- PMM
> >
>
Philippe Mathieu-Daudé Aug. 19, 2021, 2:49 p.m. UTC | #5 
Cc'ing Shameer Kolothum.

On 8/19/21 3:36 PM, Ani Sinha wrote:
> On Thu, 19 Aug 2021, Ani Sinha wrote:
>> On Thu, 19 Aug 2021, Peter Maydell wrote:
>>> On Tue, 17 Aug 2021 at 05:45, Ani Sinha <ani@anisinha.ca> wrote:

>>> Is it intended that ACPI_HW_REDUCED must always imply
>>> ACPI_MEMORY_HOTPLUG, or is it just a coincidence that the
>>> virt board happens to want both, and so we select both ?

The ACPI dependency was missing (see commit 36b79e3219d,
"hw/acpi/Kconfig: Add missing Kconfig dependencies (build error)",
now we don't need it explicitly.

>> From a purely code inspection point of view, I noticed that
>> generic_event_device.c depends on CONFIG_ACPI_HW_REDUCED. The GED does use
>> memory hotplug apis - for example acpi_ged_device_plug_cb() uses
>> acpi_memory_plug_cb() etc.
>>
>> Hence, as it stands today, CONFIG_ACPI_HW_REDUCED will need to select ACPI
>> memory hotplug. Unless we remove the GED device's dependence on
>> ACPI_HW_REDUCED that is. I cannot comment whether that would be wise or if
>> we should reorg the code in some other way.
> 
> The other question we should ask is whether arm platform requires
> ACPI_MEMORY_HOTPLUG independent of ACPI_HW_REDUCED/GED device? If that is
> the case, then maybe we should keep that config option as is.
> Maybe @qemu-arm can answer that?

Or git-log:

commit cff51ac978c4fa0b3d0de0fd62d772d9003f123e
Author: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Date:   Wed Sep 18 14:06:27 2019 +0100

    hw/arm/virt: Enable device memory cold/hot plug with ACPI boot

    This initializes the GED device with base memory and irq, configures
    ged memory hotplug event and builds the corresponding aml code. With
    this, both hot and cold plug of device memory is enabled now for
    Guest with ACPI boot. Memory cold plug support with Guest DT boot is
    not yet supported.

>>>> On Thu, 12 Aug 2021, Ani Sinha wrote:
>>>>

Please prepend here 'Since commit 36b79e3219d ("hw/acpi/Kconfig: Add
missing Kconfig dependencies"),'

With it:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>>>>> ACPI_MEMORY_HOTPLUG is implicitly turned on when ACPI_HW_REDUCED is selected.
>>>>> ACPI_HW_REDUCED is already enabled. No need to turn on ACPI_MEMORY_HOTPLUG
>>>>> explicitly. This is a minor cleanup.
>>>>>
>>>>> Signed-off-by: Ani Sinha <ani@anisinha.ca>
>>>>> ---
>>>>>  hw/arm/Kconfig | 1 -
>>>>>  1 file changed, 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
>>>>> index 4ba0aca067..38cf9f44e2 100644
>>>>> --- a/hw/arm/Kconfig
>>>>> +++ b/hw/arm/Kconfig
>>>>> @@ -25,7 +25,6 @@ config ARM_VIRT
>>>>>      select ACPI_PCI
>>>>>      select MEM_DEVICE
>>>>>      select DIMM
>>>>> -    select ACPI_MEMORY_HOTPLUG
>>>>>      select ACPI_HW_REDUCED
>>>>>      select ACPI_NVDIMM
>>>>>      select ACPI_APEI
>>>>> --
>>>>> 2.25.1
Shameerali Kolothum Thodi Aug. 19, 2021, 3:21 p.m. UTC | #6 
> -----Original Message-----
> From: Philippe Mathieu-Daudé [mailto:philmd@redhat.com]
> Sent: 19 August 2021 15:50
> To: Ani Sinha <ani@anisinha.ca>
> Cc: Peter Maydell <peter.maydell@linaro.org>; QEMU Developers
> <qemu-devel@nongnu.org>; qemu-arm <qemu-arm@nongnu.org>; Michael S.
> Tsirkin <mst@redhat.com>; Igor Mammedov <imammedo@redhat.com>;
> Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Subject: Re: [PATCH] hw/arm/Kconfig: no need to enable
> ACPI_MEMORY_HOTPLUG explicitly
> 
> Cc'ing Shameer Kolothum.
> 
> On 8/19/21 3:36 PM, Ani Sinha wrote:
> > On Thu, 19 Aug 2021, Ani Sinha wrote:
> >> On Thu, 19 Aug 2021, Peter Maydell wrote:
> >>> On Tue, 17 Aug 2021 at 05:45, Ani Sinha <ani@anisinha.ca> wrote:
> 
> >>> Is it intended that ACPI_HW_REDUCED must always imply
> >>> ACPI_MEMORY_HOTPLUG, or is it just a coincidence that the virt board
> >>> happens to want both, and so we select both ?
> 
> The ACPI dependency was missing (see commit 36b79e3219d,
> "hw/acpi/Kconfig: Add missing Kconfig dependencies (build error)", now we
> don't need it explicitly.

Yes. And it looks like ACPI_NVDIMM also can be removed now.

Regards,
Shameer

> >> From a purely code inspection point of view, I noticed that
> >> generic_event_device.c depends on CONFIG_ACPI_HW_REDUCED. The GED
> >> does use memory hotplug apis - for example acpi_ged_device_plug_cb()
> >> uses
> >> acpi_memory_plug_cb() etc.
> >>
> >> Hence, as it stands today, CONFIG_ACPI_HW_REDUCED will need to select
> >> ACPI memory hotplug. Unless we remove the GED device's dependence on
> >> ACPI_HW_REDUCED that is. I cannot comment whether that would be wise
> >> or if we should reorg the code in some other way.
> >
> > The other question we should ask is whether arm platform requires
> > ACPI_MEMORY_HOTPLUG independent of ACPI_HW_REDUCED/GED device?
> If that
> > is the case, then maybe we should keep that config option as is.
> > Maybe @qemu-arm can answer that?
> 
> Or git-log:
> 
> commit cff51ac978c4fa0b3d0de0fd62d772d9003f123e
> Author: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Date:   Wed Sep 18 14:06:27 2019 +0100
> 
>     hw/arm/virt: Enable device memory cold/hot plug with ACPI boot
> 
>     This initializes the GED device with base memory and irq, configures
>     ged memory hotplug event and builds the corresponding aml code. With
>     this, both hot and cold plug of device memory is enabled now for
>     Guest with ACPI boot. Memory cold plug support with Guest DT boot is
>     not yet supported.
> 
> >>>> On Thu, 12 Aug 2021, Ani Sinha wrote:
> >>>>
> 
> Please prepend here 'Since commit 36b79e3219d ("hw/acpi/Kconfig: Add
> missing Kconfig dependencies"),'
> 
> With it:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> >>>>> ACPI_MEMORY_HOTPLUG is implicitly turned on when
> ACPI_HW_REDUCED is selected.
> >>>>> ACPI_HW_REDUCED is already enabled. No need to turn on
> >>>>> ACPI_MEMORY_HOTPLUG explicitly. This is a minor cleanup.
> >>>>>
> >>>>> Signed-off-by: Ani Sinha <ani@anisinha.ca>
> >>>>> ---
> >>>>>  hw/arm/Kconfig | 1 -
> >>>>>  1 file changed, 1 deletion(-)
> >>>>>
> >>>>> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig index
> >>>>> 4ba0aca067..38cf9f44e2 100644
> >>>>> --- a/hw/arm/Kconfig
> >>>>> +++ b/hw/arm/Kconfig
> >>>>> @@ -25,7 +25,6 @@ config ARM_VIRT
> >>>>>      select ACPI_PCI
> >>>>>      select MEM_DEVICE
> >>>>>      select DIMM
> >>>>> -    select ACPI_MEMORY_HOTPLUG
> >>>>>      select ACPI_HW_REDUCED
> >>>>>      select ACPI_NVDIMM
> >>>>>      select ACPI_APEI
> >>>>> --
> >>>>> 2.25.1
diff mbox series

Patch

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 4ba0aca067..38cf9f44e2 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -25,7 +25,6 @@  config ARM_VIRT
     select ACPI_PCI
     select MEM_DEVICE
     select DIMM
-    select ACPI_MEMORY_HOTPLUG
     select ACPI_HW_REDUCED
     select ACPI_NVDIMM
     select ACPI_APEI