[PATCH-for-10.1 0/3] accel/hvf: Do not abort in hvf_arm_get_*_ipa_bit_size()

[PATCH-for-10.1 0/3] accel/hvf: Do not abort in hvf_arm_get_*_ipa_bit_size()

[Philippe Mathieu-Daudé]

Have get_physical_address_range() return when HVF is not
usable, allowing to try another accelerator if requested
with '-accel hvf:tcg', reported here:
https://gitlab.com/qemu-project/qemu/-/issues/2981

Philippe Mathieu-Daudé (3):
  accel/hvf: Display executable bit as 'X'
  accel/hvf: Do not abort in hvf_arm_get_*_ipa_bit_size()
  hw/arm/virt: Warn when HVF doesn't report IPA bit length

 target/arm/hvf_arm.h | 11 +++++++++++
 accel/hvf/hvf-all.c  |  2 +-
 hw/arm/virt.c        |  8 ++++++--
 target/arm/hvf/hvf.c |  8 ++------
 4 files changed, 20 insertions(+), 9 deletions(-)

-- 
2.49.0

[PATCH-for-10.1 1/3] accel/hvf: Display executable bit as 'X'

[Philippe Mathieu-Daudé]

Developers are custom to read RWX, not RWE.
Replace E -> X.

Reported-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 accel/hvf/hvf-all.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/accel/hvf/hvf-all.c b/accel/hvf/hvf-all.c
index e67a8105a66..0a4b498e836 100644
--- a/accel/hvf/hvf-all.c
+++ b/accel/hvf/hvf-all.c
@@ -84,7 +84,7 @@ static int do_hvf_set_memory(hvf_slot *slot, hv_memory_flags_t flags)
     trace_hvf_vm_map(slot->start, slot->size, slot->mem, flags,
                      flags & HV_MEMORY_READ ?  'R' : '-',
                      flags & HV_MEMORY_WRITE ? 'W' : '-',
-                     flags & HV_MEMORY_EXEC ?  'E' : '-');
+                     flags & HV_MEMORY_EXEC ?  'X' : '-');
     ret = hv_vm_map(slot->mem, slot->start, slot->size, flags);
     assert_hvf_ok(ret);
     return 0;
-- 
2.49.0

Re: [PATCH-for-10.1 1/3] accel/hvf: Display executable bit as 'X'

[Alex Bennée]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Developers are custom to read RWX, not RWE.
> Replace E -> X.
>
> Reported-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH-for-10.1 1/3] accel/hvf: Display executable bit as 'X'

[Mads Ynddal]

> On 16 Jul 2025, at 19.28, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> 
> Developers are custom to read RWX, not RWE.
> Replace E -> X.
> 
> Reported-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> accel/hvf/hvf-all.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Mads Ynddal <mads@ynddal.dk>

Re: [PATCH-for-10.1 1/3] accel/hvf: Display executable bit as 'X'

[Xiaoyao Li]

On 7/17/2025 1:28 AM, Philippe Mathieu-Daudé wrote:
> Developers are custom to read RWX, not RWE.
> Replace E -> X.
> 
> Reported-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

> ---
>   accel/hvf/hvf-all.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/accel/hvf/hvf-all.c b/accel/hvf/hvf-all.c
> index e67a8105a66..0a4b498e836 100644
> --- a/accel/hvf/hvf-all.c
> +++ b/accel/hvf/hvf-all.c
> @@ -84,7 +84,7 @@ static int do_hvf_set_memory(hvf_slot *slot, hv_memory_flags_t flags)
>       trace_hvf_vm_map(slot->start, slot->size, slot->mem, flags,
>                        flags & HV_MEMORY_READ ?  'R' : '-',
>                        flags & HV_MEMORY_WRITE ? 'W' : '-',
> -                     flags & HV_MEMORY_EXEC ?  'E' : '-');
> +                     flags & HV_MEMORY_EXEC ?  'X' : '-');
>       ret = hv_vm_map(slot->mem, slot->start, slot->size, flags);
>       assert_hvf_ok(ret);
>       return 0;

Re: [PATCH-for-10.1 1/3] accel/hvf: Display executable bit as 'X'

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 1140 bytes --]

On Thu, 17 Jul 2025, Xiaoyao Li wrote:
> On 7/17/2025 1:28 AM, Philippe Mathieu-Daudé wrote:
>> Developers are custom to read RWX, not RWE.

"accustomed to"

>> Replace E -> X.
>> 
>> Reported-by: Alex Bennée <alex.bennee@linaro.org>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>
>
>> ---
>>   accel/hvf/hvf-all.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/accel/hvf/hvf-all.c b/accel/hvf/hvf-all.c
>> index e67a8105a66..0a4b498e836 100644
>> --- a/accel/hvf/hvf-all.c
>> +++ b/accel/hvf/hvf-all.c
>> @@ -84,7 +84,7 @@ static int do_hvf_set_memory(hvf_slot *slot, 
>> hv_memory_flags_t flags)
>>       trace_hvf_vm_map(slot->start, slot->size, slot->mem, flags,
>>                        flags & HV_MEMORY_READ ?  'R' : '-',
>>                        flags & HV_MEMORY_WRITE ? 'W' : '-',
>> -                     flags & HV_MEMORY_EXEC ?  'E' : '-');
>> +                     flags & HV_MEMORY_EXEC ?  'X' : '-');
>>       ret = hv_vm_map(slot->mem, slot->start, slot->size, flags);
>>       assert_hvf_ok(ret);
>>       return 0;
>
>
>

[PATCH-for-10.1 2/3] accel/hvf: Do not abort in hvf_arm_get_*_ipa_bit_size()

[Philippe Mathieu-Daudé]

Do not abort in hvf_arm_get_default_ipa_bit_size()
and hvf_arm_get_max_ipa_bit_size() when the IPA can
not be fetched. Return 0 (and document it).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/arm/hvf_arm.h | 11 +++++++++++
 target/arm/hvf/hvf.c |  8 ++------
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/target/arm/hvf_arm.h b/target/arm/hvf_arm.h
index ea82f2691df..21a69e7d105 100644
--- a/target/arm/hvf_arm.h
+++ b/target/arm/hvf_arm.h
@@ -22,7 +22,18 @@ void hvf_arm_init_debug(void);
 
 void hvf_arm_set_cpu_features_from_host(ARMCPU *cpu);
 
+/**
+ * hvf_arm_get_default_ipa_bit_size:
+ *
+ * Returns the default intermediate physical address bit length or 0 on error.
+ */
 uint32_t hvf_arm_get_default_ipa_bit_size(void);
+
+/**
+ * hvf_arm_get_max_ipa_bit_size:
+ *
+ * Returns the maximum intermediate physical address bit length or 0 on error.
+ */
 uint32_t hvf_arm_get_max_ipa_bit_size(void);
 
 #endif
diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
index c9cfcdc08bb..180fd94def2 100644
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -943,24 +943,20 @@ uint32_t hvf_arm_get_default_ipa_bit_size(void)
 {
     uint32_t default_ipa_size;
     hv_return_t ret = hv_vm_config_get_default_ipa_size(&default_ipa_size);
-    assert_hvf_ok(ret);
-
-    return default_ipa_size;
+    return ret == HV_SUCCESS ? default_ipa_size : 0;
 }
 
 uint32_t hvf_arm_get_max_ipa_bit_size(void)
 {
     uint32_t max_ipa_size;
     hv_return_t ret = hv_vm_config_get_max_ipa_size(&max_ipa_size);
-    assert_hvf_ok(ret);
-
     /*
      * We clamp any IPA size we want to back the VM with to a valid PARange
      * value so the guest doesn't try and map memory outside of the valid range.
      * This logic just clamps the passed in IPA bit size to the first valid
      * PARange value <= to it.
      */
-    return round_down_to_parange_bit_size(max_ipa_size);
+    return ret == HV_SUCCESS ? round_down_to_parange_bit_size(max_ipa_size) : 0;
 }
 
 void hvf_arm_set_cpu_features_from_host(ARMCPU *cpu)
-- 
2.49.0

Re: [PATCH-for-10.1 2/3] accel/hvf: Do not abort in hvf_arm_get_*_ipa_bit_size()

[Mads Ynddal]

> On 16 Jul 2025, at 19.28, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> 
> Do not abort in hvf_arm_get_default_ipa_bit_size()
> and hvf_arm_get_max_ipa_bit_size() when the IPA can
> not be fetched. Return 0 (and document it).
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> target/arm/hvf_arm.h | 11 +++++++++++
> target/arm/hvf/hvf.c |  8 ++------
> 2 files changed, 13 insertions(+), 6 deletions(-)

I looked up the different uses of hvf_arm_get_max_ipa_bit_size. There's
an unchecked use in clamp_id_aa64mmfr0_parange_to_ipa_size, but I guess
it'll never reach that far, with your changes to
virt_hvf_get_physical_address_range.

clamp_id_aa64mmfr0_parange_to_ipa_size is already surrounded by other
hv-calls and assert_hvf_ok, so either leave it, or assert ipa_size!=0?

[PATCH-for-10.1 3/3] hw/arm/virt: Warn when HVF doesn't report IPA bit length

[Philippe Mathieu-Daudé]

Emit a warning when HVF doesn't return the IPA bit length
and return -1 as "this accelerator is not usable", allowing
QEMU to try with the next one (when using '-accel hvf:tcg').

Reported-by: Ivan Krasilnikov
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2981
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/virt.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index ef6be3660f5..062812bf252 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -3149,8 +3149,12 @@ static int virt_hvf_get_physical_address_range(MachineState *ms)
 {
     VirtMachineState *vms = VIRT_MACHINE(ms);
 
-    int default_ipa_size = hvf_arm_get_default_ipa_bit_size();
-    int max_ipa_size = hvf_arm_get_max_ipa_bit_size();
+    uint32_t default_ipa_size = hvf_arm_get_default_ipa_bit_size();
+    uint32_t max_ipa_size = hvf_arm_get_max_ipa_bit_size();
+    if (!default_ipa_size || !max_ipa_size) {
+        warn_report("HVF didn't report IPA bit length");
+        return -1;
+    }
 
     /* We freeze the memory map to compute the highest gpa */
     virt_set_memmap(vms, max_ipa_size);
-- 
2.49.0

Re: [PATCH-for-10.1 3/3] hw/arm/virt: Warn when HVF doesn't report IPA bit length

[Mads Ynddal]

> On 16 Jul 2025, at 19.28, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> 
> Emit a warning when HVF doesn't return the IPA bit length
> and return -1 as "this accelerator is not usable", allowing
> QEMU to try with the next one (when using '-accel hvf:tcg').
> 
> Reported-by: Ivan Krasilnikov
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2981
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> hw/arm/virt.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)

I haven't been able to verify that hv_vm_config_get_max_ipa_size and
hv_vm_config_get_default_ipa_size fail if HVF is not available, but
assuming so, it looks fine to me.

Reviewed-by: Mads Ynddal <mads@ynddal.dk>

Re: [PATCH-for-10.1 3/3] hw/arm/virt: Warn when HVF doesn't report IPA bit length

[Philippe Mathieu-Daudé]

On 17/7/25 12:06, Mads Ynddal wrote:
> 
>> On 16 Jul 2025, at 19.28, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Emit a warning when HVF doesn't return the IPA bit length
>> and return -1 as "this accelerator is not usable", allowing
>> QEMU to try with the next one (when using '-accel hvf:tcg').
>>
>> Reported-by: Ivan Krasilnikov
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2981
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> hw/arm/virt.c | 8 ++++++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
> 
> I haven't been able to verify that hv_vm_config_get_max_ipa_size and
> hv_vm_config_get_default_ipa_size fail if HVF is not available, but

This happens with nested macOS guest. Maybe we are missing an earlier
check whether HVF is usable or not, but we shouldn't brutally abort().

I'll try to update the patch description. Annoyingly the GitLab issue
reporter isn't Cc'ed via the mailing list.

> assuming so, it looks fine to me.
> 
> Reviewed-by: Mads Ynddal <mads@ynddal.dk>

Re: [PATCH-for-10.1 3/3] hw/arm/virt: Warn when HVF doesn't report IPA bit length

[Mads Ynddal]

> This happens with nested macOS guest.

I took another look at the stack trace in the issue. Everything should
be fine then.

> Maybe we are missing an earlier check whether HVF is usable or not, but
> we shouldn't brutally abort().

I agree. I think with this patchset you're doing it practically as early
as possible with accel_init_machine.

Re: [PATCH-for-10.1 3/3] hw/arm/virt: Warn when HVF doesn't report IPA bit length

[Peter Maydell]

On Wed, 16 Jul 2025 at 18:28, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Emit a warning when HVF doesn't return the IPA bit length
> and return -1 as "this accelerator is not usable", allowing
> QEMU to try with the next one (when using '-accel hvf:tcg').
>
> Reported-by: Ivan Krasilnikov
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2981
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/arm/virt.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index ef6be3660f5..062812bf252 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -3149,8 +3149,12 @@ static int virt_hvf_get_physical_address_range(MachineState *ms)
>  {
>      VirtMachineState *vms = VIRT_MACHINE(ms);
>
> -    int default_ipa_size = hvf_arm_get_default_ipa_bit_size();
> -    int max_ipa_size = hvf_arm_get_max_ipa_bit_size();
> +    uint32_t default_ipa_size = hvf_arm_get_default_ipa_bit_size();
> +    uint32_t max_ipa_size = hvf_arm_get_max_ipa_bit_size();
> +    if (!default_ipa_size || !max_ipa_size) {
> +        warn_report("HVF didn't report IPA bit length");
> +        return -1;
> +    }

Even if we avoid blowing up in this function, won't
hvf_accel_init() immediately fail in hvf_arch_vm_create()
and either exit(1) or assert() depending on what error
code it got back ?

It's unfortunate that there's no convenient way we can
check "hvf is basically going to work" in hvf_accel_init()
before we get into the machine-specific hook. "HVF didn't
report IPA bit length" isn't really a very good message
to report for "HVF isn't going to work at all here".

(More generally, I think the hvf code rather overuses
assert_hvf_ok(). I don't think the KVM code uses asserts
for error checking like this.)

thanks
-- PMM

Re: [PATCH-for-10.1 0/3] accel/hvf: Do not abort in hvf_arm_get_*_ipa_bit_size()

[Peter Maydell]

On Wed, 16 Jul 2025 at 18:28, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Have get_physical_address_range() return when HVF is not
> usable, allowing to try another accelerator if requested
> with '-accel hvf:tcg', reported here:
> https://gitlab.com/qemu-project/qemu/-/issues/2981
>
> Philippe Mathieu-Daudé (3):
>   accel/hvf: Display executable bit as 'X'
>   accel/hvf: Do not abort in hvf_arm_get_*_ipa_bit_size()
>   hw/arm/virt: Warn when HVF doesn't report IPA bit length


I've applied patch 1 to target-arm.next, since it's not
really related to the patch 2 and 3 bugfix.

thanks
-- PMM

Re: [PATCH-for-10.1 0/3] accel/hvf: Do not abort in hvf_arm_get_*_ipa_bit_size()

[Philippe Mathieu-Daudé]

On 21/7/25 14:40, Peter Maydell wrote:
> On Wed, 16 Jul 2025 at 18:28, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Have get_physical_address_range() return when HVF is not
>> usable, allowing to try another accelerator if requested
>> with '-accel hvf:tcg', reported here:
>> https://gitlab.com/qemu-project/qemu/-/issues/2981
>>
>> Philippe Mathieu-Daudé (3):
>>    accel/hvf: Display executable bit as 'X'
>>    accel/hvf: Do not abort in hvf_arm_get_*_ipa_bit_size()
>>    hw/arm/virt: Warn when HVF doesn't report IPA bit length
> 
> 
> I've applied patch 1 to target-arm.next, since it's not
> really related to the patch 2 and 3 bugfix.

Thank you.