Re: [PATCH v5 06/13] target/arm: hvf: pass through CNTHCTL_EL2 and MDCCINT_EL1
>>> We'd like to remove the assert_hvf_ok() calls, so adding more isn't >>> really helping. Anyhow, >>> >>> Reviewed-by: Philippe Mathieu-Daudé >>> Tested-by: Philippe Mathieu-Daudé >> >> What is the preferred method going forward? >> >> Apple designed the HV API to be able to fail at every function, and it's >> rarely something that can be recovered from. >> >> In [PATCH v4 04/15] of this series, we saw an issue that was hidden >> because the return code was not properly checked (not calling from the >> owning thread). Previously, I submitted a patch (d5bd8d8267) for the >> same issue, because Apple had changed a function's behavior. This was >> caught by an assert_hvf_ok. >> >> I agree with you, that generally adding asserts is a bad design, but at >> the same time, HV is designed in a way, that the code would be littered >> with checks otherwise. > > This suggestion was I think mine, and I think partly I was > misled by the name of assert_hvf_ok(), because in fact it > isn't an assert(). (assert() should be specifically "if we > hit this this is a bug in QEMU", and "hvf returned an error" > is generally not that. It does call abort(), though, which > isn't much better.) > > But also I think the existence of assert_hvf_ok() encourages > it to be used in cases where actually we should be doing > something more sensible with an error return. For instance, > in hvf_accel_init() if we can't init HVF then we should > return an error code to the caller, which might fall back > to the TCG accelerator -- but instead we have an assert_hvf_ok(), > so fallback won't work because we'll just bomb out. > > The KVM API is also one where any API call (ioctl) can return an > error, but we don't generally assert() that they succeeded, except > in a few cases of "given where we are, for this to fail would > mean the kernel was broken". Instead we propagate error values > upwards when the function has a mechanism to do that, > and where appropriate we print an error message that's > hopefully reasonably comprehensible to the user and exit. > > -- PMM Thanks for the clarification. I think I understand the sentiment. Propagating the error codes when possible, is much preferred. But letting the calls go unchecked is problematic too, so I don't know if we can get around assert_hvf_ok in some shape or form. You're right, that it might encourage use as an easy fix. I'll keep an eye on it in my reviews.
Re: [PATCH v5 06/13] target/arm: hvf: pass through CNTHCTL_EL2 and MDCCINT_EL1
On Mon, 11 Aug 2025 at 11:08, Mads Ynddal wrote: > > > > We'd like to remove the assert_hvf_ok() calls, so adding more isn't > > really helping. Anyhow, > > > > Reviewed-by: Philippe Mathieu-Daudé > > Tested-by: Philippe Mathieu-Daudé > > What is the preferred method going forward? > > Apple designed the HV API to be able to fail at every function, and it's > rarely something that can be recovered from. > > In [PATCH v4 04/15] of this series, we saw an issue that was hidden > because the return code was not properly checked (not calling from the > owning thread). Previously, I submitted a patch (d5bd8d8267) for the > same issue, because Apple had changed a function's behavior. This was > caught by an assert_hvf_ok. > > I agree with you, that generally adding asserts is a bad design, but at > the same time, HV is designed in a way, that the code would be littered > with checks otherwise. This suggestion was I think mine, and I think partly I was misled by the name of assert_hvf_ok(), because in fact it isn't an assert(). (assert() should be specifically "if we hit this this is a bug in QEMU", and "hvf returned an error" is generally not that. It does call abort(), though, which isn't much better.) But also I think the existence of assert_hvf_ok() encourages it to be used in cases where actually we should be doing something more sensible with an error return. For instance, in hvf_accel_init() if we can't init HVF then we should return an error code to the caller, which might fall back to the TCG accelerator -- but instead we have an assert_hvf_ok(), so fallback won't work because we'll just bomb out. The KVM API is also one where any API call (ioctl) can return an error, but we don't generally assert() that they succeeded, except in a few cases of "given where we are, for this to fail would mean the kernel was broken". Instead we propagate error values upwards when the function has a mechanism to do that, and where appropriate we print an error message that's hopefully reasonably comprehensible to the user and exit. -- PMM
Re: [PATCH v5 06/13] target/arm: hvf: pass through CNTHCTL_EL2 and MDCCINT_EL1
> We'd like to remove the assert_hvf_ok() calls, so adding more isn't > really helping. Anyhow, > > Reviewed-by: Philippe Mathieu-Daudé > Tested-by: Philippe Mathieu-Daudé What is the preferred method going forward? Apple designed the HV API to be able to fail at every function, and it's rarely something that can be recovered from. In [PATCH v4 04/15] of this series, we saw an issue that was hidden because the return code was not properly checked (not calling from the owning thread). Previously, I submitted a patch (d5bd8d8267) for the same issue, because Apple had changed a function's behavior. This was caught by an assert_hvf_ok. I agree with you, that generally adding asserts is a bad design, but at the same time, HV is designed in a way, that the code would be littered with checks otherwise.
Re: [PATCH v5 06/13] target/arm: hvf: pass through CNTHCTL_EL2 and MDCCINT_EL1
On 28/7/25 15:41, Mohamed Mediouni wrote: HVF traps accesses to CNTHCTL_EL2. For nested guests, HVF traps accesses to MDCCINT_EL1. Pass through those accesses to the Hypervisor.framework library. Signed-off-by: Mohamed Mediouni --- target/arm/hvf/hvf.c | 16 1 file changed, 16 insertions(+) diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c index f14a3a3cbd..eefae3069f 100644 --- a/target/arm/hvf/hvf.c +++ b/target/arm/hvf/hvf.c @@ -297,6 +297,10 @@ void hvf_arm_init_debug(void) #define SYSREG_DBGWVR15_EL1 SYSREG(2, 0, 0, 15, 6) #define SYSREG_DBGWCR15_EL1 SYSREG(2, 0, 0, 15, 7) +/* EL2 registers */ +#define SYSREG_CNTHCTL_EL2SYSREG(3, 4, 14, 1, 0) +#define SYSREG_MDCCINT_EL1SYSREG(2, 0, 0, 2, 0) + #define WFX_IS_WFE (1 << 0) #define TMR_CTL_ENABLE (1 << 0) @@ -1372,6 +1376,12 @@ static int hvf_sysreg_read(CPUState *cpu, uint32_t reg, uint64_t *val) case SYSREG_OSDLR_EL1: /* Dummy register */ return 0; +case SYSREG_CNTHCTL_EL2: +assert_hvf_ok(hv_vcpu_get_sys_reg(cpu->accel->fd, HV_SYS_REG_CNTHCTL_EL2, val)); +return 0; We'd like to remove the assert_hvf_ok() calls, so adding more isn't really helping. Anyhow, Reviewed-by: Philippe Mathieu-Daudé Tested-by: Philippe Mathieu-Daudé
