Message-ID: <4716c74c-d798-4c34-8c64-a8de4d0a6b45@linux.microsoft.com>
Date: Thu, 19 Dec 2024 12:04:39 -0800
From: Easwar Hariharan <eahariha@...ux.microsoft.com>
To: Roman Kisel <romank@...ux.microsoft.com>
Cc: Nuno Das Neves <nunodasneves@...ux.microsoft.com>,
 eahariha@...ux.microsoft.com, hpa@...or.com, kys@...rosoft.com,
 bp@...en8.de, dave.hansen@...ux.intel.com, decui@...rosoft.com,
 haiyangz@...rosoft.com, mingo@...hat.com, mhklinux@...look.com,
 tglx@...utronix.de, tiala@...rosoft.com, wei.liu@...nel.org,
 linux-hyperv@...r.kernel.org, linux-kernel@...r.kernel.org, x86@...nel.org,
 apais@...rosoft.com, benhill@...rosoft.com, ssengar@...rosoft.com,
 sunilmut@...rosoft.com, vdso@...bites.dev
Subject: Re: [PATCH 1/2] hyperv: Fix pointer type for the output of the
 hypercall in get_vtl(void)

On 12/19/2024 12:00 PM, Roman Kisel wrote:
> 
> 
> On 12/19/2024 11:13 AM, Easwar Hariharan wrote:
>> On 12/19/2024 10:40 AM, Nuno Das Neves wrote:
>>> On 12/18/2024 12:54 PM, Roman Kisel wrote:
>>>> Commit bc905fa8b633 ("hyperv: Switch from hyperv-tlfs.h to hyperv/
>>>> hvhdk.h")
>>>> changed the type of the output pointer to `struct hv_register_assoc`
>>>> from
>>>> `struct hv_get_vp_registers_output`. That leads to an incorrect
>>>> computation,
>>>> and leaves the system broken.
>>>>
>>> My bad! But, lets not use `struct hv_get_registers_output`. Instead, use
>>> `struct hv_register_value`, since that is the more complete
>>> definition of a
>>> register value. The output of the get_vp_registers hypercall is just
>>> an array
>>> of these values.
>>>
>>> Ideally we remove `struct hv_get_vp_registers_output` at some point,
>>> since
>>> it serves the same role as `struct hv_register_value` but in a more
>>> limited
>>> capacity.
>>>
>>> Thanks
>>> Nuno
>>>
>>
>> I had much the same conversation with Roman off-list yesterday.
> Appreciate your help, Easwar!
> 
>>
>> The choice is between using hv_get_registers_output which is clearly the
>> output of the GetVpRegisters hypercall by name, albeit limited as you
>> said, and hv_register_value which is the more complete definition and
>> what the hypervisor actually returns, but does not currently include the
>> arm64 definitions in our copy of hvgdk_mini.h. hv_get_registers_output
>> and hv_register_value overlap in layout for Roman's purposes.
>>
>> FWIW, I'm in favor of adding the arm64 definitions to hv_register_value
>> and using it for this get_vtl() patch.
> I could do that, will wait to see if no objections.
> 
>>
>> This could be accompanied with migration of hv_get_vpreg128 in arm64/
>> and removal of struct hv_get_registers_output, or that could be deferred
>> to a later patch.
> Having an equivalent of `hv_get_vpreg128` would be awesome on x64!
> I'd bet something like that should exist in dom0/mshv already if you'd
> like to have a dedicated function. So perhaps we could let mshv take
> the lead with that?
> 
> If the desire is also to use the fast hypercall technique as
> `hv_get_vpreg128` does, then we'd need fast hypercalls on x64 that
> can use XMM registers (aka the fast extended hypercalls) that aren't
> implemented in the hyperv drivers from my read of the code (although
> it seems KVM knows how to do that when it emulates Hyper-V,
> arch/x86/kvm/hyperv.c, struct kvm_hv_hcall *hc).
> 
> These considerations make me lean towards deferring hv_get_vpreg128-like
> implementation to a later time.
>

To clarify, I didn't mean to include implementing extended fast
hypercalls with the XMM registers, or an implementation of
hv_get_vpreg128() for x64 in my suggestion.

Just to fix up the existing hv_get_vpreg128 in arch/arm64 to use
hv_register_value (or its Linux-specific helper as Nuno suggested).

Thanks,
Easwar

Powered by blists - more mailing lists