Re: [PATCH v9] arm/kvm: Enable support for KVM_ARM_VCPU_PMU_V3_FILTER

[Cornelia Huck <cohuck@redhat.com>]

On Wed, Apr 10 2024, Thomas Huth <thuth@redhat.com> wrote:

> On 09/04/2024 09.47, Shaoqin Huang wrote:
>> Hi Thmoas,
>> 
>> On 4/9/24 13:33, Thomas Huth wrote:
>>>> +        assert_has_feature(qts, "host", "kvm-pmu-filter");
>>>
>>> So you assert here that the feature is available ...
>>>
>>>>           assert_has_feature(qts, "host", "kvm-steal-time");
>>>>           assert_has_feature(qts, "host", "sve");
>>>>           resp = do_query_no_props(qts, "host");
>>>> +        kvm_supports_pmu_filter = resp_get_feature_str(resp, 
>>>> "kvm-pmu-filter");
>>>>           kvm_supports_steal_time = resp_get_feature(resp, 
>>>> "kvm-steal-time");
>>>>           kvm_supports_sve = resp_get_feature(resp, "sve");
>>>>           vls = resp_get_sve_vls(resp);
>>>>           qobject_unref(resp);
>>>> +        if (kvm_supports_pmu_filter) { >
>>> ... why do you then need to check for its availability here again?
>>> I either don't understand this part of the code, or you could drop the 
>>> kvm_supports_pmu_filter variable and simply always execute the code below.
>> 
>> Thanks for your reviewing. I did so because all other feature like 
>> "kvm-steal-time" check its availability again. I don't know the original 
>> reason why they did that. I just followed it.
>> 
>> Do you think we should delete all the checking?
>
> resp_get_feature() seems to return a boolean value, so though these feature 
> could be there, they still could be disabled, I assume? Thus we likely need 
> to keep the check for those.

This had confused me as well when I looked at it the last time -- one
thing is to check whether we have a certain prop in the cpu model, the
other one whether we actually support it. Maybe this needs some
comments?