Michael Kelley (LINUX)
2023-Mar-17 13:43 UTC
[RFC PATCH] hyperv-tlfs: Change shared HV_REGISTER_* defines to HV_SYNTHETIC_REG_*
From: Nuno Das Neves <nunodasneves at linux.microsoft.com> Sent: Wednesday, March 15, 2023 3:54 PM> > On 3/13/2023 6:56 PM, Michael Kelley (LINUX) wrote: > > From: Nuno Das Neves <nunodasneves at linux.microsoft.com> Sent: Monday, March > 13, 2023 2:11 PM > >> > >> On 3/10/2023 11:30 AM, Michael Kelley (LINUX) wrote: > >>> From: Nuno Das Neves <nunodasneves at linux.microsoft.com> 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