[Qemu-devel] [RFC PATCH 1/5] ppc64: Express dependencies of 'pseries' and 'powernv' machines with kconfig

Thomas Huth posted 5 patches 6 years, 9 months ago
There is a newer version of this series
[Qemu-devel] [RFC PATCH 1/5] ppc64: Express dependencies of 'pseries' and 'powernv' machines with kconfig
Posted by Thomas Huth 6 years, 9 months ago
The POWERNV switch should always select ISA_IPMI_BT, then the other
IPMI options are turned on automatically now.
CONFIG_DIMM should always be selected by the pseries machine,
which in turn depends on CONFIG_MEM_DEVICE since DIMM implements
this interface.
CONFIG_VIRTIO_VGA can be dropped from default-configs/ppc64-softmmu.mak
completely since this device is already automatically enabled via
hw/display/Kconfig now.
CONFIG_SPAPR_RNG should stay in the ppc-softmmu.mak file since this
is a completely optional device.

Cc: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 default-configs/ppc64-softmmu.mak | 7 -------
 hw/intc/Kconfig                   | 4 +---
 hw/mem/Kconfig                    | 1 +
 hw/ppc/Kconfig                    | 9 +++++++++
 4 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak
index a0a9151..6f8d7ea 100644
--- a/default-configs/ppc64-softmmu.mak
+++ b/default-configs/ppc64-softmmu.mak
@@ -5,14 +5,7 @@ include ppc-softmmu.mak
 
 # For PowerNV
 CONFIG_POWERNV=y
-CONFIG_IPMI=y
-CONFIG_IPMI_LOCAL=y
-CONFIG_IPMI_EXTERN=y
-CONFIG_ISA_IPMI_BT=y
 
 # For pSeries
 CONFIG_PSERIES=y
-CONFIG_VIRTIO_VGA=y
-CONFIG_MEM_DEVICE=y
-CONFIG_DIMM=y
 CONFIG_SPAPR_RNG=y
diff --git a/hw/intc/Kconfig b/hw/intc/Kconfig
index 6eea14e..1e819d0 100644
--- a/hw/intc/Kconfig
+++ b/hw/intc/Kconfig
@@ -31,13 +31,11 @@ config OPENPIC_KVM
 
 config XICS
     bool
-    default y
-    depends on PSERIES
 
 config XICS_SPAPR
     bool
     default y
-    depends on PSERIES
+    depends on XICS && PSERIES
 
 config XICS_KVM
     bool
diff --git a/hw/mem/Kconfig b/hw/mem/Kconfig
index d1e635c..620fd4c 100644
--- a/hw/mem/Kconfig
+++ b/hw/mem/Kconfig
@@ -1,5 +1,6 @@
 config DIMM
     bool
+    select MEM_DEVICE
 
 config MEM_DEVICE
     bool
diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
index b0095e1..b44e3bd 100644
--- a/hw/ppc/Kconfig
+++ b/hw/ppc/Kconfig
@@ -1,11 +1,20 @@
 config PSERIES
     bool
+    select DIMM
+    select PCI
+    select VFIO
+    select XICS
 
 config SPAPR_RNG
     bool
+    depends on PSERIES
 
 config POWERNV
     bool
+    select ISA_IPMI_BT
+    select ISA_BUS
+    select MC146818RTC
+    select XICS
 
 config PPC405
     bool
-- 
1.8.3.1


Re: [Qemu-devel] [RFC PATCH 1/5] ppc64: Express dependencies of 'pseries' and 'powernv' machines with kconfig
Posted by Paolo Bonzini 6 years, 9 months ago
On 30/01/19 10:43, Thomas Huth wrote:
> CONFIG_SPAPR_RNG should stay in the ppc-softmmu.mak file since this
> is a completely optional device.

What about making it

    default y

and adding to the .mak file this:

#CONFIG_SPAPR_RNG=n

I think the two approaches are more or less equivalent, but
"#CONFIG_FOO=n" has a small advantage when the feature has a build-time
dependency, such as CONFIG_MILKYMIST_TMU2's dependency on OpenGL.  In
that case,

   CONFIG_MILKYMIST_TMU2=y

would report a contradiction if OpenGL is not available at build time, while

   default y
   ...
   #CONFIG_MILKYMIST_TMU2=n

would not.

In any case, there are many many such cases in x86 (QXL, HYPERV, FDC,
SEV, ISA_IPMI_*, APPLESMC, SGA, HPET, VTD, AMD_IOMMU, PVPANIC, TPM_TIS,
TPM_CRB) and they should be handled in the same way---and especially
they should be mentioned in the default-configs/ file under "Optional
devices".  So thanks for noticing this case!

Paolo

Re: [Qemu-devel] [RFC PATCH 1/5] ppc64: Express dependencies of 'pseries' and 'powernv' machines with kconfig
Posted by Thomas Huth 6 years, 9 months ago
On 2019-01-30 10:57, Paolo Bonzini wrote:
> On 30/01/19 10:43, Thomas Huth wrote:
>> CONFIG_SPAPR_RNG should stay in the ppc-softmmu.mak file since this
>> is a completely optional device.
> 
> What about making it
> 
>     default y
> 
> and adding to the .mak file this:
> 
> #CONFIG_SPAPR_RNG=n
> 
> I think the two approaches are more or less equivalent, but
> "#CONFIG_FOO=n" has a small advantage when the feature has a build-time
> dependency, such as CONFIG_MILKYMIST_TMU2's dependency on OpenGL.  In
> that case,
> 
>    CONFIG_MILKYMIST_TMU2=y
> 
> would report a contradiction if OpenGL is not available at build time, while
> 
>    default y
>    ...
>    #CONFIG_MILKYMIST_TMU2=n
> 
> would not.

I somehow dislike the idea of adding lines that are commented out by
default. For example, if someone later renames or removes the config
switch in the Kconfig files, the comment could stay there without being
noticed, while a CONFIG_XXX=y would cause a clean build failure if the
option is not available anymore.

 Thomas

Re: [Qemu-devel] [RFC PATCH 1/5] ppc64: Express dependencies of 'pseries' and 'powernv' machines with kconfig
Posted by Paolo Bonzini 6 years, 9 months ago
On 30/01/19 11:02, Thomas Huth wrote:
>> I think the two approaches are more or less equivalent, but
>> "#CONFIG_FOO=n" has a small advantage when the feature has a build-time
>> dependency, such as CONFIG_MILKYMIST_TMU2's dependency on OpenGL.  In
>> that case,
>>
>>    CONFIG_MILKYMIST_TMU2=y
>>
>> would report a contradiction if OpenGL is not available at build time, while
>>
>>    default y
>>    ...
>>    #CONFIG_MILKYMIST_TMU2=n
>>
>> would not.
> I somehow dislike the idea of adding lines that are commented out by
> default. For example, if someone later renames or removes the config
> switch in the Kconfig files, the comment could stay there without being
> noticed, while a CONFIG_XXX=y would cause a clean build failure if the
> option is not available anymore.

I agree.  On the other hand I don't see another solution for the
MILKYMIST_TMU2 and QXL cases, and consistency is a plus too.

How often are config switches renamed or removed, especially
user-visible optional devices though?  I see CONFIG_MEM_DEVICE,
CONFIG_PCI_APB, CONFIG_LIBDECNUMBER, CONFIG_XEN_I386,
CONFIG_MIPS_BOSTON, CONFIG_PIIX_PCI, CONFIG_ISA_MMIO, CONFIG_ICC_BUS
over the last few years but none probably would have been optional
devices in default-configs/ but rather in Kconfig files.

(Long term default-configs/ should probably be built automatically,
possibly by adding a description field to the Kconfig files, and that
would also ameliorate the issue you describe).

Paolo

Re: [Qemu-devel] [RFC PATCH 1/5] ppc64: Express dependencies of 'pseries' and 'powernv' machines with kconfig
Posted by Cédric Le Goater 6 years, 9 months ago
On 1/30/19 10:43 AM, Thomas Huth wrote:
> The POWERNV switch should always select ISA_IPMI_BT, then the other
> IPMI options are turned on automatically now.
> CONFIG_DIMM should always be selected by the pseries machine,
> which in turn depends on CONFIG_MEM_DEVICE since DIMM implements
> this interface.
> CONFIG_VIRTIO_VGA can be dropped from default-configs/ppc64-softmmu.mak
> completely since this device is already automatically enabled via
> hw/display/Kconfig now.
> CONFIG_SPAPR_RNG should stay in the ppc-softmmu.mak file since this
> is a completely optional device.
> 
> Cc: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  default-configs/ppc64-softmmu.mak | 7 -------
>  hw/intc/Kconfig                   | 4 +---
>  hw/mem/Kconfig                    | 1 +
>  hw/ppc/Kconfig                    | 9 +++++++++
>  4 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak
> index a0a9151..6f8d7ea 100644
> --- a/default-configs/ppc64-softmmu.mak
> +++ b/default-configs/ppc64-softmmu.mak
> @@ -5,14 +5,7 @@ include ppc-softmmu.mak
>  
>  # For PowerNV
>  CONFIG_POWERNV=y
> -CONFIG_IPMI=y
> -CONFIG_IPMI_LOCAL=y
> -CONFIG_IPMI_EXTERN=y
> -CONFIG_ISA_IPMI_BT=y
>  
>  # For pSeries
>  CONFIG_PSERIES=y
> -CONFIG_VIRTIO_VGA=y
> -CONFIG_MEM_DEVICE=y
> -CONFIG_DIMM=y
>  CONFIG_SPAPR_RNG=y
> diff --git a/hw/intc/Kconfig b/hw/intc/Kconfig
> index 6eea14e..1e819d0 100644
> --- a/hw/intc/Kconfig
> +++ b/hw/intc/Kconfig
> @@ -31,13 +31,11 @@ config OPENPIC_KVM
>  
>  config XICS
>      bool
> -    default y
> -    depends on PSERIES
>  
>  config XICS_SPAPR
>      bool
>      default y
> -    depends on PSERIES
> +    depends on XICS && PSERIES
>  
>  config XICS_KVM
>      bool
> diff --git a/hw/mem/Kconfig b/hw/mem/Kconfig
> index d1e635c..620fd4c 100644
> --- a/hw/mem/Kconfig
> +++ b/hw/mem/Kconfig
> @@ -1,5 +1,6 @@
>  config DIMM
>      bool
> +    select MEM_DEVICE
>  
>  config MEM_DEVICE
>      bool
> diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
> index b0095e1..b44e3bd 100644
> --- a/hw/ppc/Kconfig
> +++ b/hw/ppc/Kconfig
> @@ -1,11 +1,20 @@
>  config PSERIES
>      bool
> +    select DIMM
> +    select PCI
> +    select VFIO
> +    select XICS

Don't we want XICS_SPAPR ? or is there another toggle with KVM ?

>  config SPAPR_RNG
>      bool
> +    depends on PSERIES
>  
>  config POWERNV
>      bool
> +    select ISA_IPMI_BT

yes.

It is possible to start a PowerNV machine without defining a 
BT device but the machine relies on the BT device and the IPMI 
backend attached to it to powerdown. 

Ideally I would have preferred to define the device internally 
in the machine and plug it on the ISA bus but I couldn't find 
a way to do it cleanly. 

> +    select ISA_BUS
> +    select MC146818RTC
> +    select XICS

only for POWER8 machines. POWER9 uses XIVE.

Thanks,

C.

>  
>  config PPC405
>      bool
> 


Re: [Qemu-devel] [RFC PATCH 1/5] ppc64: Express dependencies of 'pseries' and 'powernv' machines with kconfig
Posted by Thomas Huth 6 years, 9 months ago
On 2019-01-30 12:00, Cédric Le Goater wrote:
> On 1/30/19 10:43 AM, Thomas Huth wrote:
>> The POWERNV switch should always select ISA_IPMI_BT, then the other
>> IPMI options are turned on automatically now.
>> CONFIG_DIMM should always be selected by the pseries machine,
>> which in turn depends on CONFIG_MEM_DEVICE since DIMM implements
>> this interface.
>> CONFIG_VIRTIO_VGA can be dropped from default-configs/ppc64-softmmu.mak
>> completely since this device is already automatically enabled via
>> hw/display/Kconfig now.
>> CONFIG_SPAPR_RNG should stay in the ppc-softmmu.mak file since this
>> is a completely optional device.
>>
>> Cc: Cédric Le Goater <clg@kaod.org>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  default-configs/ppc64-softmmu.mak | 7 -------
>>  hw/intc/Kconfig                   | 4 +---
>>  hw/mem/Kconfig                    | 1 +
>>  hw/ppc/Kconfig                    | 9 +++++++++
>>  4 files changed, 11 insertions(+), 10 deletions(-)
>>
>> diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak
>> index a0a9151..6f8d7ea 100644
>> --- a/default-configs/ppc64-softmmu.mak
>> +++ b/default-configs/ppc64-softmmu.mak
>> @@ -5,14 +5,7 @@ include ppc-softmmu.mak
>>  
>>  # For PowerNV
>>  CONFIG_POWERNV=y
>> -CONFIG_IPMI=y
>> -CONFIG_IPMI_LOCAL=y
>> -CONFIG_IPMI_EXTERN=y
>> -CONFIG_ISA_IPMI_BT=y
>>  
>>  # For pSeries
>>  CONFIG_PSERIES=y
>> -CONFIG_VIRTIO_VGA=y
>> -CONFIG_MEM_DEVICE=y
>> -CONFIG_DIMM=y
>>  CONFIG_SPAPR_RNG=y
>> diff --git a/hw/intc/Kconfig b/hw/intc/Kconfig
>> index 6eea14e..1e819d0 100644
>> --- a/hw/intc/Kconfig
>> +++ b/hw/intc/Kconfig
>> @@ -31,13 +31,11 @@ config OPENPIC_KVM
>>  
>>  config XICS
>>      bool
>> -    default y
>> -    depends on PSERIES
>>  
>>  config XICS_SPAPR
>>      bool
>>      default y
>> -    depends on PSERIES
>> +    depends on XICS && PSERIES
>>  
>>  config XICS_KVM
>>      bool
>> diff --git a/hw/mem/Kconfig b/hw/mem/Kconfig
>> index d1e635c..620fd4c 100644
>> --- a/hw/mem/Kconfig
>> +++ b/hw/mem/Kconfig
>> @@ -1,5 +1,6 @@
>>  config DIMM
>>      bool
>> +    select MEM_DEVICE
>>  
>>  config MEM_DEVICE
>>      bool
>> diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
>> index b0095e1..b44e3bd 100644
>> --- a/hw/ppc/Kconfig
>> +++ b/hw/ppc/Kconfig
>> @@ -1,11 +1,20 @@
>>  config PSERIES
>>      bool
>> +    select DIMM
>> +    select PCI
>> +    select VFIO
>> +    select XICS
> 
> Don't we want XICS_SPAPR ? or is there another toggle with KVM ?
> 
>>  config SPAPR_RNG
>>      bool
>> +    depends on PSERIES
>>  
>>  config POWERNV
>>      bool
>> +    select ISA_IPMI_BT
> 
> yes.
> 
> It is possible to start a PowerNV machine without defining a 
> BT device but the machine relies on the BT device and the IPMI 
> backend attached to it to powerdown. 
> 
> Ideally I would have preferred to define the device internally 
> in the machine and plug it on the ISA bus but I couldn't find 
> a way to do it cleanly. 
> 
>> +    select ISA_BUS
>> +    select MC146818RTC
>> +    select XICS
> 
> only for POWER8 machines. POWER9 uses XIVE.

Sure, but you don't know at compile time yet whether the user wants to
use a POWER8 or a POWER9 CPU. So this currently always needs to be selected.

 Thomas

Re: [Qemu-devel] [RFC PATCH 1/5] ppc64: Express dependencies of 'pseries' and 'powernv' machines with kconfig
Posted by Cédric Le Goater 6 years, 9 months ago
On 1/30/19 12:04 PM, Thomas Huth wrote:
> On 2019-01-30 12:00, Cédric Le Goater wrote:
>> On 1/30/19 10:43 AM, Thomas Huth wrote:
>>> The POWERNV switch should always select ISA_IPMI_BT, then the other
>>> IPMI options are turned on automatically now.
>>> CONFIG_DIMM should always be selected by the pseries machine,
>>> which in turn depends on CONFIG_MEM_DEVICE since DIMM implements
>>> this interface.
>>> CONFIG_VIRTIO_VGA can be dropped from default-configs/ppc64-softmmu.mak
>>> completely since this device is already automatically enabled via
>>> hw/display/Kconfig now.
>>> CONFIG_SPAPR_RNG should stay in the ppc-softmmu.mak file since this
>>> is a completely optional device.
>>>
>>> Cc: Cédric Le Goater <clg@kaod.org>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>  default-configs/ppc64-softmmu.mak | 7 -------
>>>  hw/intc/Kconfig                   | 4 +---
>>>  hw/mem/Kconfig                    | 1 +
>>>  hw/ppc/Kconfig                    | 9 +++++++++
>>>  4 files changed, 11 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak
>>> index a0a9151..6f8d7ea 100644
>>> --- a/default-configs/ppc64-softmmu.mak
>>> +++ b/default-configs/ppc64-softmmu.mak
>>> @@ -5,14 +5,7 @@ include ppc-softmmu.mak
>>>  
>>>  # For PowerNV
>>>  CONFIG_POWERNV=y
>>> -CONFIG_IPMI=y
>>> -CONFIG_IPMI_LOCAL=y
>>> -CONFIG_IPMI_EXTERN=y
>>> -CONFIG_ISA_IPMI_BT=y
>>>  
>>>  # For pSeries
>>>  CONFIG_PSERIES=y
>>> -CONFIG_VIRTIO_VGA=y
>>> -CONFIG_MEM_DEVICE=y
>>> -CONFIG_DIMM=y
>>>  CONFIG_SPAPR_RNG=y
>>> diff --git a/hw/intc/Kconfig b/hw/intc/Kconfig
>>> index 6eea14e..1e819d0 100644
>>> --- a/hw/intc/Kconfig
>>> +++ b/hw/intc/Kconfig
>>> @@ -31,13 +31,11 @@ config OPENPIC_KVM
>>>  
>>>  config XICS
>>>      bool
>>> -    default y
>>> -    depends on PSERIES
>>>  
>>>  config XICS_SPAPR
>>>      bool
>>>      default y
>>> -    depends on PSERIES
>>> +    depends on XICS && PSERIES
>>>  
>>>  config XICS_KVM
>>>      bool
>>> diff --git a/hw/mem/Kconfig b/hw/mem/Kconfig
>>> index d1e635c..620fd4c 100644
>>> --- a/hw/mem/Kconfig
>>> +++ b/hw/mem/Kconfig
>>> @@ -1,5 +1,6 @@
>>>  config DIMM
>>>      bool
>>> +    select MEM_DEVICE
>>>  
>>>  config MEM_DEVICE
>>>      bool
>>> diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
>>> index b0095e1..b44e3bd 100644
>>> --- a/hw/ppc/Kconfig
>>> +++ b/hw/ppc/Kconfig
>>> @@ -1,11 +1,20 @@
>>>  config PSERIES
>>>      bool
>>> +    select DIMM
>>> +    select PCI
>>> +    select VFIO
>>> +    select XICS
>>
>> Don't we want XICS_SPAPR ? or is there another toggle with KVM ?
>>
>>>  config SPAPR_RNG
>>>      bool
>>> +    depends on PSERIES
>>>  
>>>  config POWERNV
>>>      bool
>>> +    select ISA_IPMI_BT
>>
>> yes.
>>
>> It is possible to start a PowerNV machine without defining a 
>> BT device but the machine relies on the BT device and the IPMI 
>> backend attached to it to powerdown. 
>>
>> Ideally I would have preferred to define the device internally 
>> in the machine and plug it on the ISA bus but I couldn't find 
>> a way to do it cleanly. 
>>
>>> +    select ISA_BUS
>>> +    select MC146818RTC
>>> +    select XICS
>>
>> only for POWER8 machines. POWER9 uses XIVE.
> 
> Sure, but you don't know at compile time yet whether the user wants to
> use a POWER8 or a POWER9 CPU. So this currently always needs to be selected.

ah yes :) Please add XIVE then. I should get the Pnv model merged one day.

Thanks,

C.