pvg: clean up Kconfig

[PATCH 1/2] pvg: do not enable it on cross-architecture targets

Posted by Paolo Bonzini 11 months, 3 weeks ago

PVG is not cross-architecture; the PVG guest drivers with x86-64 macOS do not give
useful results with the aarch64 macOS host PVG framework, and vice versa.
To express this repurpose CONFIG_MAC_PVG, making it true only if the target has
the same architecture as the host.  Furthermore, remove apple-gfx.m unless
one of the devices is actually present.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 meson.build            | 6 ++++++
 Kconfig.host           | 3 +++
 hw/display/Kconfig     | 4 ----
 hw/display/meson.build | 9 +++------
 4 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/meson.build b/meson.build
index 0ee79c664d3..ad2c6b61930 100644
--- a/meson.build
+++ b/meson.build
@@ -3367,6 +3367,12 @@ foreach target : target_dirs
     target_kconfig += 'CONFIG_' + config_target['TARGET_ARCH'].to_upper() + '=y'
     target_kconfig += 'CONFIG_TARGET_BIG_ENDIAN=' + config_target['TARGET_BIG_ENDIAN']
 
+    # PVG is not cross-architecture.  Use accelerator_targets as a proxy to
+    # figure out which target can support PVG on this host
+    if pvg.found() and target in accelerator_targets.get('CONFIG_HVF', [])
+      target_kconfig += 'CONFIG_MAC_PVG=y'
+    endif
+
     config_input = meson.get_external_property(target, 'default')
     config_devices_mak = target + '-config-devices.mak'
     config_devices_mak = configure_file(
diff --git a/Kconfig.host b/Kconfig.host
index 842cbe0d6c5..933425c74b4 100644
--- a/Kconfig.host
+++ b/Kconfig.host
@@ -61,3 +61,6 @@ config HV_BALLOON_POSSIBLE
 
 config HAVE_RUST
     bool
+
+config MAC_PVG
+    bool
diff --git a/hw/display/Kconfig b/hw/display/Kconfig
index 2b53dfd7d26..1e95ab28ef4 100644
--- a/hw/display/Kconfig
+++ b/hw/display/Kconfig
@@ -141,10 +141,6 @@ config XLNX_DISPLAYPORT
 config DM163
     bool
 
-config MAC_PVG
-    bool
-    default y
-
 config MAC_PVG_MMIO
     bool
     depends on MAC_PVG && AARCH64
diff --git a/hw/display/meson.build b/hw/display/meson.build
index 94f4f05d36f..b9bdf219103 100644
--- a/hw/display/meson.build
+++ b/hw/display/meson.build
@@ -61,12 +61,9 @@ system_ss.add(when: 'CONFIG_ARTIST', if_true: files('artist.c'))
 
 system_ss.add(when: 'CONFIG_ATI_VGA', if_true: [files('ati.c', 'ati_2d.c', 'ati_dbg.c'), pixman])
 
-if host_os == 'darwin'
-  system_ss.add(when: 'CONFIG_MAC_PVG',         if_true: [files('apple-gfx.m'), pvg, metal])
-  system_ss.add(when: 'CONFIG_MAC_PVG_PCI',     if_true: [files('apple-gfx-pci.m'), pvg, metal])
-  if cpu == 'aarch64'
-    system_ss.add(when: 'CONFIG_MAC_PVG_MMIO',  if_true: [files('apple-gfx-mmio.m'), pvg, metal])
-  endif
+if pvg.found()
+  system_ss.add(when: 'CONFIG_MAC_PVG_PCI',     if_true: [files('apple-gfx.m', 'apple-gfx-pci.m'), pvg, metal])
+  system_ss.add(when: 'CONFIG_MAC_PVG_MMIO',    if_true: [files('apple-gfx.m', 'apple-gfx-mmio.m'), pvg, metal])
 endif
 
 if config_all_devices.has_key('CONFIG_VIRTIO_GPU')
-- 
2.48.1

Re: [PATCH 1/2] pvg: do not enable it on cross-architecture targets

Posted by Philippe Mathieu-Daudé 11 months, 3 weeks ago

On 20/2/25 14:33, Paolo Bonzini wrote:
> PVG is not cross-architecture; the PVG guest drivers with x86-64 macOS do not give
> useful results with the aarch64 macOS host PVG framework, and vice versa.
> To express this repurpose CONFIG_MAC_PVG, making it true only if the target has
> the same architecture as the host.  Furthermore, remove apple-gfx.m unless
> one of the devices is actually present.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   meson.build            | 6 ++++++
>   Kconfig.host           | 3 +++
>   hw/display/Kconfig     | 4 ----
>   hw/display/meson.build | 9 +++------
>   4 files changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/meson.build b/meson.build
> index 0ee79c664d3..ad2c6b61930 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -3367,6 +3367,12 @@ foreach target : target_dirs
>       target_kconfig += 'CONFIG_' + config_target['TARGET_ARCH'].to_upper() + '=y'
>       target_kconfig += 'CONFIG_TARGET_BIG_ENDIAN=' + config_target['TARGET_BIG_ENDIAN']
>   
> +    # PVG is not cross-architecture.  Use accelerator_targets as a proxy to
> +    # figure out which target can support PVG on this host
> +    if pvg.found() and target in accelerator_targets.get('CONFIG_HVF', [])
> +      target_kconfig += 'CONFIG_MAC_PVG=y'
> +    endif
> +
>       config_input = meson.get_external_property(target, 'default')
>       config_devices_mak = target + '-config-devices.mak'
>       config_devices_mak = configure_file(
> diff --git a/Kconfig.host b/Kconfig.host
> index 842cbe0d6c5..933425c74b4 100644
> --- a/Kconfig.host
> +++ b/Kconfig.host
> @@ -61,3 +61,6 @@ config HV_BALLOON_POSSIBLE
>   
>   config HAVE_RUST
>       bool
> +
> +config MAC_PVG
> +    bool
> diff --git a/hw/display/Kconfig b/hw/display/Kconfig
> index 2b53dfd7d26..1e95ab28ef4 100644
> --- a/hw/display/Kconfig
> +++ b/hw/display/Kconfig
> @@ -141,10 +141,6 @@ config XLNX_DISPLAYPORT
>   config DM163
>       bool
>   
> -config MAC_PVG
> -    bool
> -    default y
> -
>   config MAC_PVG_MMIO
>       bool
>       depends on MAC_PVG && AARCH64

Hmm what about keeping a MAC_PVG_COMMON instead:

-- >8 --
--- a/hw/display/Kconfig
+++ b/hw/display/Kconfig
@@ -141,15 +141,17 @@ config XLNX_DISPLAYPORT
  config DM163
      bool

-config MAC_PVG
+config MAC_PVG_COMMON
      bool
-    default y
+    depends on MAC_PVG

  config MAC_PVG_MMIO
      bool
-    depends on MAC_PVG && AARCH64
+    depends on AARCH64
+    select MAC_PVG_COMMON

  config MAC_PVG_PCI
      bool
-    depends on MAC_PVG && PCI
+    depends on PCI
+    select MAC_PVG_COMMON
      default y if PCI_DEVICES
---

> diff --git a/hw/display/meson.build b/hw/display/meson.build
> index 94f4f05d36f..b9bdf219103 100644
> --- a/hw/display/meson.build
> +++ b/hw/display/meson.build
> @@ -61,12 +61,9 @@ system_ss.add(when: 'CONFIG_ARTIST', if_true: files('artist.c'))
>   
>   system_ss.add(when: 'CONFIG_ATI_VGA', if_true: [files('ati.c', 'ati_2d.c', 'ati_dbg.c'), pixman])
>   
> -if host_os == 'darwin'
> -  system_ss.add(when: 'CONFIG_MAC_PVG',         if_true: [files('apple-gfx.m'), pvg, metal])
> -  system_ss.add(when: 'CONFIG_MAC_PVG_PCI',     if_true: [files('apple-gfx-pci.m'), pvg, metal])
> -  if cpu == 'aarch64'
> -    system_ss.add(when: 'CONFIG_MAC_PVG_MMIO',  if_true: [files('apple-gfx-mmio.m'), pvg, metal])
> -  endif
> +if pvg.found()
> +  system_ss.add(when: 'CONFIG_MAC_PVG_PCI',     if_true: [files('apple-gfx.m', 'apple-gfx-pci.m'), pvg, metal])
> +  system_ss.add(when: 'CONFIG_MAC_PVG_MMIO',    if_true: [files('apple-gfx.m', 'apple-gfx-mmio.m'), pvg, metal])
>   endif

Directly using here:

-- >8 --
diff --git a/hw/display/meson.build b/hw/display/meson.build
index 94f4f05d36f..f636ca0999c 100644
--- a/hw/display/meson.build
+++ b/hw/display/meson.build
@@ -61,13 +61,9 @@ system_ss.add(when: 'CONFIG_ARTIST', if_true: 
files('artist.c'))

  system_ss.add(when: 'CONFIG_ATI_VGA', if_true: [files('ati.c', 
'ati_2d.c', 'ati_dbg.c'), pixman])

-if host_os == 'darwin'
-  system_ss.add(when: 'CONFIG_MAC_PVG',         if_true: 
[files('apple-gfx.m'), pvg, metal])
-  system_ss.add(when: 'CONFIG_MAC_PVG_PCI',     if_true: 
[files('apple-gfx-pci.m'), pvg, metal])
-  if cpu == 'aarch64'
-    system_ss.add(when: 'CONFIG_MAC_PVG_MMIO',  if_true: 
[files('apple-gfx-mmio.m'), pvg, metal])
-  endif
-endif
+system_ss.add(when: 'CONFIG_MAC_PVG_COMMON', if_true: 
[files('apple-gfx.m'), pvg, metal])
+system_ss.add(when: 'CONFIG_MAC_PVG_PCI',    if_true: 
[files('apple-gfx-pci.m'), pvg, metal])
+system_ss.add(when: 'CONFIG_MAC_PVG_MMIO',   if_true: 
[files('apple-gfx-mmio.m'), pvg, metal])

  if config_all_devices.has_key('CONFIG_VIRTIO_GPU')
    virtio_gpu_ss = ss.source_set()
---

?

Re: [PATCH 1/2] pvg: do not enable it on cross-architecture targets

Posted by Paolo Bonzini 11 months, 3 weeks ago

On 2/20/25 16:19, Philippe Mathieu-Daudé wrote:
> Hmm what about keeping a MAC_PVG_COMMON instead:
> 
> -- >8 --
> --- a/hw/display/Kconfig
> +++ b/hw/display/Kconfig
> @@ -141,15 +141,17 @@ config XLNX_DISPLAYPORT
>   config DM163
>       bool
> 
> -config MAC_PVG
> +config MAC_PVG_COMMON
>       bool
> -    default y
> +    depends on MAC_PVG
> 
>   config MAC_PVG_MMIO
>       bool
> -    depends on MAC_PVG && AARCH64
> +    depends on AARCH64

It's possible, but the two "depends on MAC_PVG" cannot be removed here,
because otherwise MAC_PVG_PCI would be selected by "default y".  The
only reason to do so, in my opinion, was if one wanted to build pvg as
a module, otherwise it's just (a handful) more lines of code with no
particular benefit.

> -if host_os == 'darwin'
> -  system_ss.add(when: 'CONFIG_MAC_PVG',         if_true: [files('apple- 
> gfx.m'), pvg, metal])
> -  system_ss.add(when: 'CONFIG_MAC_PVG_PCI',     if_true: [files('apple- 
> gfx-pci.m'), pvg, metal])
> -  if cpu == 'aarch64'
> -    system_ss.add(when: 'CONFIG_MAC_PVG_MMIO',  if_true: [files('apple- 
> gfx-mmio.m'), pvg, metal])
> -  endif
> -endif
> +system_ss.add(when: 'CONFIG_MAC_PVG_COMMON', if_true: [files('apple- 
> gfx.m'), pvg, metal])
> +system_ss.add(when: 'CONFIG_MAC_PVG_PCI',    if_true: [files('apple- 
> gfx-pci.m'), pvg, metal])
> +system_ss.add(when: 'CONFIG_MAC_PVG_MMIO',   if_true: [files('apple- 
> gfx-mmio.m'), pvg, metal])

Removing the "if" is independent of whether you keep CONFIG_MAC_PVG_COMMON.
I can squash that in patch 2:

-if pvg.found()
-  system_ss.add(when: 'CONFIG_MAC_PVG_PCI',     if_true: [files('apple-gfx.m', 'apple-gfx-pci.m'), pvg])
-  system_ss.add(when: 'CONFIG_MAC_PVG_MMIO',    if_true: [files('apple-gfx.m', 'apple-gfx-mmio.m'), pvg])
-endif
+system_ss.add(when: [pvg, 'CONFIG_MAC_PVG_PCI'],     if_true: [files('apple-gfx.m', 'apple-gfx-pci.m')])
+system_ss.add(when: [pvg, 'CONFIG_MAC_PVG_MMIO'],    if_true: [files('apple-gfx.m', 'apple-gfx-mmio.m')])

(you need to keep the conditional, whether as "if" or "when:"; otherwise,
because Meson cannot know that the .m files will never be built, it
complains that you don't have an Objective-C compiler).

Paolo

Re: [PATCH 1/2] pvg: do not enable it on cross-architecture targets

Posted by Philippe Mathieu-Daudé 11 months, 2 weeks ago

On 21/2/25 18:01, Paolo Bonzini wrote:
> On 2/20/25 16:19, Philippe Mathieu-Daudé wrote:
>> Hmm what about keeping a MAC_PVG_COMMON instead:
>>
>> -- >8 --
>> --- a/hw/display/Kconfig
>> +++ b/hw/display/Kconfig
>> @@ -141,15 +141,17 @@ config XLNX_DISPLAYPORT
>>   config DM163
>>       bool
>>
>> -config MAC_PVG
>> +config MAC_PVG_COMMON
>>       bool
>> -    default y
>> +    depends on MAC_PVG
>>
>>   config MAC_PVG_MMIO
>>       bool
>> -    depends on MAC_PVG && AARCH64
>> +    depends on AARCH64
> 
> It's possible, but the two "depends on MAC_PVG" cannot be removed here,
> because otherwise MAC_PVG_PCI would be selected by "default y".  The
> only reason to do so, in my opinion, was if one wanted to build pvg as
> a module, otherwise it's just (a handful) more lines of code with no
> particular benefit.
> 
>> -if host_os == 'darwin'
>> -  system_ss.add(when: 'CONFIG_MAC_PVG',         if_true: 
>> [files('apple- gfx.m'), pvg, metal])
>> -  system_ss.add(when: 'CONFIG_MAC_PVG_PCI',     if_true: 
>> [files('apple- gfx-pci.m'), pvg, metal])
>> -  if cpu == 'aarch64'
>> -    system_ss.add(when: 'CONFIG_MAC_PVG_MMIO',  if_true: 
>> [files('apple- gfx-mmio.m'), pvg, metal])
>> -  endif
>> -endif
>> +system_ss.add(when: 'CONFIG_MAC_PVG_COMMON', if_true: [files('apple- 
>> gfx.m'), pvg, metal])
>> +system_ss.add(when: 'CONFIG_MAC_PVG_PCI',    if_true: [files('apple- 
>> gfx-pci.m'), pvg, metal])
>> +system_ss.add(when: 'CONFIG_MAC_PVG_MMIO',   if_true: [files('apple- 
>> gfx-mmio.m'), pvg, metal])
> 
> Removing the "if" is independent of whether you keep CONFIG_MAC_PVG_COMMON.
> I can squash that in patch 2:
> 
> -if pvg.found()
> -  system_ss.add(when: 'CONFIG_MAC_PVG_PCI',     if_true: [files('apple- 
> gfx.m', 'apple-gfx-pci.m'), pvg])
> -  system_ss.add(when: 'CONFIG_MAC_PVG_MMIO',    if_true: [files('apple- 
> gfx.m', 'apple-gfx-mmio.m'), pvg])
> -endif
> +system_ss.add(when: [pvg, 'CONFIG_MAC_PVG_PCI'],     if_true: 
> [files('apple-gfx.m', 'apple-gfx-pci.m')])
> +system_ss.add(when: [pvg, 'CONFIG_MAC_PVG_MMIO'],    if_true: 
> [files('apple-gfx.m', 'apple-gfx-mmio.m')])
> 
> (you need to keep the conditional, whether as "if" or "when:"; otherwise,
> because Meson cannot know that the .m files will never be built, it
> complains that you don't have an Objective-C compiler).

Ah, I haven't thought of that.

Series:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>