On Wed, Apr 10 2024, Thomas Huth <thuth@xxxxxxxxxx> 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?
