On 2020-09-07 10:28, Jianyong Wu wrote:
-----Original Message-----
From: Marc Zyngier <maz@xxxxxxxxxx>
Sent: Monday, September 7, 2020 4:55 PM
To: Jianyong Wu <Jianyong.Wu@xxxxxxx>
[...]
>> arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_FEATUR
>> ES_FUNC_ID,
>> > + &hvc_res);
>> > + if (!(hvc_res.a0 | BIT(ARM_SMCCC_KVM_FUNC_KVM_PTP)))
>> > + return -EOPNOTSUPP;
>> > +
>> > + return 0;
>>
>> What happens if the
>> ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID function isn't
implemented
>> (on an old kernel or a non-KVM hypervisor)? The expected behaviour is
>> that a0 will contain SMCCC_RET_NOT_SUPPORTED, which is -1.
>> The result is that this function always returns "supported". Not an
>> acceptable behaviour.
>>
> Oh! it's really a stupid mistake, should be "&" not "|".
But even then. (-1 & whatever) is always true.
Yeah, what about checking if a0 is non-negative first? Like:
if (hvc_res.a0 < 0 || !(hvc_res.a0 & BIT(ARM_SMCCC_KVM_FUNC_KVM_PTP)))
return -EOPNOTSUPP;
I don't get it. You already carry a patch from Will that gives
you a way to check for a service (kvm_arm_hyp_service_available()).
Why do you need to reinvent the wheel?
M.
--
Jazz is not dead. It just smells funny...
