On 2/10/2024 2:37 AM, Michael Kelley wrote:
>
> Overall, this looks good to me. It cleans up the mess I made five
> years ago when first separating x86 from ARM64. :-(
>
> See one comment below, but otherwise,
>
> Reviewed-by: Michael Kelley <mhklinux@xxxxxxxxxxx>
>
Thanks! Responded to your comment below.
>> #if IS_ENABLED(CONFIG_HYPERV)
>> -static inline unsigned int hv_get_nested_reg(unsigned int reg)
>> +static inline unsigned int hv_get_nested_msr(unsigned int reg)
>> {
>> - if (hv_is_sint_reg(reg))
>> - return reg - HV_REGISTER_SINT0 + HV_REGISTER_NESTED_SINT0;
>> + if (hv_is_sint_msr(reg))
>> + return reg - HV_MSR_SINT0 + HV_MSR_NESTED_SINT0;
>>
>> switch (reg) {
>> - case HV_REGISTER_SIMP:
>> - return HV_REGISTER_NESTED_SIMP;
>> - case HV_REGISTER_SIEFP:
>> - return HV_REGISTER_NESTED_SIEFP;
>> - case HV_REGISTER_SVERSION:
>> - return HV_REGISTER_NESTED_SVERSION;
>> - case HV_REGISTER_SCONTROL:
>> - return HV_REGISTER_NESTED_SCONTROL;
>> - case HV_REGISTER_EOM:
>> - return HV_REGISTER_NESTED_EOM;
>> + case HV_MSR_SIMP:
>> + return HV_MSR_NESTED_SIMP;
>> + case HV_MSR_SIEFP:
>> + return HV_MSR_NESTED_SIEFP;
>> + case HV_MSR_SVERSION:
>> + return HV_MSR_NESTED_SVERSION;
>> + case HV_MSR_SCONTROL:
>> + return HV_MSR_NESTED_SCONTROL;
>> + case HV_MSR_EOM:
>> + return HV_MSR_NESTED_EOM;
>> default:
>> return reg;
>> }
>> }
>
> This function is x86 specific, but you are using the generic HV_MSR_* flavor
> instead of the x86 specific HV_X64_MSR_* flavor like in other x86 specific code.
> Both flavors work, but I wondered if there is any reason for using the generic flavor.
>> I remember debating myself the merits of one approach vs. the other, but I
> don't think there was a solid argument either way. Given that you are
> doing the work to get this all fixed like it should have been originally, I would
> argue for being consistently one way or the other. ARM64 specific code is
> *not* using the generic HV_MSR_* flavor, so I suspect x86 code should not
> either.
>
> Michael
>
I see your point about keeping it consistent within the file.
My thinking was that hv_get_nested_msr is not inherently x86-specific, even though it
lives in arch/x86 today. However, I realize that even if this function *is* moved to
generic code in the future, at that time it could be changed to the generic prefix.
Doing so in this patch could be confusing since it introduces an inconsistency.
So, I will take your advice and keep it HV_X64_MSR_* for everything in this file.
Thanks again!
Nuno
