Re: [PATCH] xen/x86: Make XEN_DOMCTL_get_vcpu_msrs more configurable

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.10.2022 13:58, Tamas K Lengyel wrote:
> On Wed, Oct 26, 2022, 7:48 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
>> On 26.10.2022 13:34, Tamas K Lengyel wrote:
>>> On Wed, Oct 26, 2022, 7:06 AM Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>
>>> wrote:
>>>
>>>> On 24/10/2022 17:58, Tamas K Lengyel wrote:
>>>>> Currently the XEN_DOMCTL_get_vcpu_msrs is only capable of gathering a
>>>> handful
>>>>> of predetermined vcpu MSRs. In our use-case gathering the vPMU MSRs by
>> an
>>>>> external privileged tool is necessary, thus we extend the domctl to
>>>> allow for
>>>>> querying for any guest MSRs. To remain compatible with the existing
>>>> setup if
>>>>> no specific MSR is requested via the domctl the default list is
>> returned.
>>>>>
>>>>> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxx>
>>>>
>>>> Naming aside, XEN_DOMCTL_{get,set}_vcpu_msrs is supposed to be "get me
>>>> all MSRs needed to migrate a vCPU".  (I do intend to retire the
>>>> hypercall as part of fixing the Xen side of migration, but that's ages
>>>> away)
>>>>
>>>> It seems like what you want is something more like
>>>> XEN_DOMCTL_{rd,wr}msr_list  (convenient timing, given the recent ISE
>>>> update).  I think those would be better as a separate pair of
>>>> hypercalls, rather than trying to repurpose an existing hypercall.
>>>>
>>>>
>>>> As for actually getting the values, please fix up guest_{rd,wr}msr() to
>>>> access the perf MSRs safely.  I know the vpmu MSR handling is in a
>>>> tragic state, but this new get_msr subop is making the problem even more
>>>> tangled.
>>>>
>>>
>>> Adding a separate hypercall is fine.
>>
>> Which will then also avoid altering the behavior of the existing hypercall:
>> You can't just assume e.g. input fields to be zeroed (or otherwise
>> suitably set) by existing callers, when those were previously output only.
>>
> 
> I did add a memset to zero it on the single caller I could find.

Some may deem this sufficient on the assumption that all users should
go through the libxenguest function. But then at the minimum you want
to update documentation in the public header. Yet then this wasn't
the only issue I spotted (hence the use of "e.g.") - you also alter
documented behavior as to the returned number of MSRs when the input
value was too small, if I'm not mistaken.

Jan