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

[Tamas K Lengyel <tamas.k.lengyel@xxxxxxxxx>]

On Wed, Oct 26, 2022 at 8:22 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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.

No, there shouldn't have been any alteration of the previous behavior other than assuming the buffer sent by the toolstack is zero initialized when the default list is requested. Either way, adding the separate hypercall is fine by me.

Tamas