RE: [RFC PATCH] hyperv-tlfs: Change shared HV_REGISTER_* defines to HV_SYNTHETIC_REG_*

[Michael Kelley (LINUX)]

From: Nuno Das Neves <nunodasneves@xxxxxxxxxxxxxxxxxxx> Sent: Wednesday, March 15, 2023 3:54 PM
> 
> On 3/13/2023 6:56 PM, Michael Kelley (LINUX) wrote:
> > From: Nuno Das Neves <nunodasneves@xxxxxxxxxxxxxxxxxxx> Sent: Monday, March
> 13, 2023 2:11 PM
> >>
> >> On 3/10/2023 11:30 AM, Michael Kelley (LINUX) wrote:
> >>> From: Nuno Das Neves <nunodasneves@xxxxxxxxxxxxxxxxxxx> Sent: Tuesday, March 7,
> >> 2023 1:13 PM
> >>>>
> >>>> In x86 hyperv-tlfs, HV_REGISTER_ prefix is used to indicate MSRs
> >>>> accessed via rdmsrl/wrmsrl. But in ARM64, HV_REGISTER_ instead indicates
> >>>> VP registers accessed via get/set vp registers hypercall.
> >>>>
> >>>> This is due to HV_REGISTER_* names being used by hv_set/get_register,
> >>>> with the arch-specific version delegating to the appropriate mechanism.
> >>>>
> >>>> The problem is, using prefix HV_REGISTER_ for MSRs will conflict with
> >>>> VP registers when they are introduced for x86 in future.
> >>>>
> >>>> This patch solves the issue by:
> >>>>
> >>>> 1. Defining all the x86 MSRs with a consistent prefix: HV_X64_MSR_.
> >>>>    This is so HV_REGISTER_ can be reserved for VP registers.
> >>>>
> >>>> 2. Change the non-arch-specific alias used by hv_set/get_register to
> >>>>    HV_SYNTHETIC_REG.
> >>>
> >>> I definitely messed this up when I first did the ARM64 support a
> >>> few years back.  :-(    This is a necessary fix.
> >>>
> >>> What about keeping HV_REGISTER_ as the prefix for the architecture
> >>> independent alias, and creating a new prefix for the Hyper-V register
> >>> definition?  This would allow the existing hv_get/set_register()
> >>> invocations to remain unchanged, and eliminates the code churn
> >>> in the arch independent code.
> >>>> The HV_X64_MSR_ prefix is definitely good for the MSR addresses,
> >>> especially since a lot of definitions that are x86/x64 only are still in use.
> >>> Then perhaps use HV_HYP_REG_ or something similar for the Hyper-V
> >>> register definition.
> >>
> >> This could work.
> >>
> >> Ideally, we would use HV_REGISTER_ for the vp registers as it's a direct match
> >> to the name in HyperV e.g. HvRegisterVpIndex-> HV_REGISTER_VP_INDEX
> >
> > You make a good point.
> >
> >>
> >> However if you think it's better to reduce churn and go with a different
> >> name then that's fine by me.
> >
> > I was specifically thinking about 3 large-ish patch sets for Confidential VMs
> > that we have pending.  The Confidential VM patches have various changes
> > to the synic code in hv.c so it overlaps with your changes to the register
> > naming.  The Confidential VM patches need to be backported to earlier
> > Linux kernel versions, and I was trying to avoid unrelated churn to ease
> > the backport process.   How urgent is fixing this register naming problem?
> > If it could go after the Confidential VM patches, then there's less churn for
> > the backports.
> >
> 
> It is not urgent, but I wanted feedback on the approach because this needs to be
> fixed in some way for the /dev/mshv driver which adds all the vp register names,
> and I was hoping to use HV_REGISTER_ for those.
> 
> > But in the grand scheme of things, we can deal with the churn.  It's just
> > some manual work that isn't hard.  Net, I'm OK with either approach.
> >
> 
> In that case, I'd prefer to go with my original intention of changing the
> meaning of HV_REGISTER_ to be the vp registers, and adding the generic
> HV_SYNTHETIC_REG (or a shorter name as below).

Fair enough.

> 
> But, merging this change can indeed wait - I can include it in the /dev/mshv
> patch series. Since that will take some time to review/iterate on, it's likely
> this change wouldn't actually be merged for some time.
> 
> >>
> >> HV_HYP_REG_ is ok, though maybe HV_VP_REG_ is a bit more informative?
> >> "VP_REG" indicating it's relevant to HVCALL_GET/SET_VP_REGISTERS.
> >
> > Yes, HV_VP_REG_ is good as the register prefix if you decide to keep
> > HV_REGISTER_ as the architecture independent prefix.
> >
> >>
> >>>
> >>> If you don't like that suggestion, I wonder if the HV_SYNTHETIC_REG_
> >>> prefix could be shortened to help avoid line length problems.  Maybe
> >>> HV_SYNREG_ or HV_SYN_REG_ ?
> 
> 
> This is a good idea. I'm fine with either, will go with HV_SYN_REG_ if you
> don't have a preference.

OK

> Do you think it is necessary or worthwhile to also rename hv_get/set_register
> to hv_get/set_syn_reg?

I don't have a strong preference either way.   Your choice.

Michael