Re: [PATCH v3 2/5] Drivers: hv: Setup synic registers in case of nested root partition

[Jinank Jain]

On 11/4/2022 4:11 PM, Anirudh Rayabharam wrote:
> On Thu, Nov 03, 2022 at 01:04:04PM +0000, Jinank Jain wrote:
> > Child partitions are free to allocate SynIC message and event page but in
> > case of root partition it must use the pages allocated by Microsoft
> > Hypervisor (MSHV). Base address for these pages can be found using
> > synthetic MSRs exposed by MSHV. There is a slight difference in those MSRs
> > for nested vs non-nested root partition.
> >
> > Signed-off-by: Jinank Jain <jinankjain@xxxxxxxxxxxxxxxxxxx>
> > ---
> >   arch/x86/include/asm/hyperv-tlfs.h | 11 +++++++++++
> >   arch/x86/include/asm/mshyperv.h    | 24 ++++++++++++++++++++++++
> >   drivers/hv/hv.c                    | 18 +++++++++++++-----
> >   3 files changed, 48 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
> > index d9a611565859..0319091e2019 100644
> > --- a/arch/x86/include/asm/hyperv-tlfs.h
> > +++ b/arch/x86/include/asm/hyperv-tlfs.h
> > @@ -225,6 +225,17 @@ enum hv_isolation_type {
> >   #define HV_REGISTER_SINT14			0x4000009E
> >   #define HV_REGISTER_SINT15			0x4000009F
> >   
> > +/*
> > + * Define synthetic interrupt controller model specific registers for
> > + * nested hypervisor.
> > + */
> > +#define HV_REGISTER_NESTED_SCONTROL            0x40001080
> > +#define HV_REGISTER_NESTED_SVERSION            0x40001081
> > +#define HV_REGISTER_NESTED_SIEFP               0x40001082
> > +#define HV_REGISTER_NESTED_SIMP                0x40001083
> > +#define HV_REGISTER_NESTED_EOM                 0x40001084
> > +#define HV_REGISTER_NESTED_SINT0               0x40001090
> > +
> >   /*
> >    * Synthetic Timer MSRs. Four timers per vcpu.
> >    */
> > diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> > index 3c39923e5969..b0f16d06a0c5 100644
> > --- a/arch/x86/include/asm/mshyperv.h
> > +++ b/arch/x86/include/asm/mshyperv.h
> > @@ -200,10 +200,31 @@ static inline bool hv_is_synic_reg(unsigned int reg)
> >   	return false;
> >   }
> >   
> > +static inline unsigned int hv_get_nested_reg(unsigned int reg)
> > +{
> > +	switch (reg) {
> > +	case HV_REGISTER_SIMP:
> > +		return HV_REGISTER_NESTED_SIMP;
> > +	case HV_REGISTER_NESTED_SIEFP:
> > +		return HV_REGISTER_SIEFP;
> > +	case HV_REGISTER_SCONTROL:
> > +		return HV_REGISTER_NESTED_SCONTROL;
> > +	case HV_REGISTER_SINT0:
> > +		return HV_REGISTER_NESTED_SINT0;
> > +	case HV_REGISTER_EOM:
> > +		return HV_REGISTER_NESTED_EOM;
> > +	default:
> > +		return reg;
> > +	}
> > +}
> > +
> >   static inline u64 hv_get_register(unsigned int reg)
> >   {
> >   	u64 value;
> >   
> > +	if (hv_nested)
> > +		reg = hv_get_nested_reg(reg);
> With this change the nested root cannot read it's own SynIC MSRs using
> this method. It will always read the SynIC MSRs corresponding to the
> nesting hypervisor. Similar is the case with hv_set_register.
>
> Will there never be a need for nested root to read/write it's own SynIC
> MSRs? For e.g. to set up inter partition communication at the L2 level.
>
> Anirudh.

As far as the current use case goes I doubt there would be a use case 
like that. In future if such a scneario arises, I think we can add an 
additional parameter to hv_(get|set)_register, called nested and 
depending on the value of that parameter we can return the value of 
nested or non-nested registers. This would be very similar to the design 
I posted in revision 1 of this patch series.

Jinank.

> > +
> >   	if (hv_is_synic_reg(reg) && hv_isolation_type_snp())
> >   		hv_ghcb_msr_read(reg, &value);
> >   	else
> > @@ -213,6 +234,9 @@ static inline u64 hv_get_register(unsigned int reg)
> >   
> >   static inline void hv_set_register(unsigned int reg, u64 value)
> >   {
> > +	if (hv_nested)
> > +		reg = hv_get_nested_reg(reg);
> > +
> >   	if (hv_is_synic_reg(reg) && hv_isolation_type_snp()) {
> >   		hv_ghcb_msr_write(reg, value);
> >   
> > diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
> > index 4d6480d57546..9e1eb50cc76f 100644
> > --- a/drivers/hv/hv.c
> > +++ b/drivers/hv/hv.c
> > @@ -147,7 +147,7 @@ int hv_synic_alloc(void)
> >   		 * Synic message and event pages are allocated by paravisor.
> >   		 * Skip these pages allocation here.
> >   		 */
> > -		if (!hv_isolation_type_snp()) {
> > +		if (!hv_isolation_type_snp() && !hv_root_partition) {
> >   			hv_cpu->synic_message_page =
> >   				(void *)get_zeroed_page(GFP_ATOMIC);
> >   			if (hv_cpu->synic_message_page == NULL) {
> > @@ -188,8 +188,16 @@ void hv_synic_free(void)
> >   		struct hv_per_cpu_context *hv_cpu
> >   			= per_cpu_ptr(hv_context.cpu_context, cpu);
> >   
> > -		free_page((unsigned long)hv_cpu->synic_event_page);
> > -		free_page((unsigned long)hv_cpu->synic_message_page);
> > +		if (hv_root_partition) {
> > +			if (hv_cpu->synic_event_page != NULL)
> > +				memunmap(hv_cpu->synic_event_page);
> > +
> > +			if (hv_cpu->synic_message_page != NULL)
> > +				memunmap(hv_cpu->synic_message_page);
> > +		} else {
> > +			free_page((unsigned long)hv_cpu->synic_event_page);
> > +			free_page((unsigned long)hv_cpu->synic_message_page);
> > +		}
> >   		free_page((unsigned long)hv_cpu->post_msg_page);
> >   	}
> >   
> > @@ -216,7 +224,7 @@ void hv_synic_enable_regs(unsigned int cpu)
> >   	simp.as_uint64 = hv_get_register(HV_REGISTER_SIMP);
> >   	simp.simp_enabled = 1;
> >   
> > -	if (hv_isolation_type_snp()) {
> > +	if (hv_isolation_type_snp() || hv_root_partition) {
> >   		hv_cpu->synic_message_page
> >   			= memremap(simp.base_simp_gpa << HV_HYP_PAGE_SHIFT,
> >   				   HV_HYP_PAGE_SIZE, MEMREMAP_WB);
> > @@ -233,7 +241,7 @@ void hv_synic_enable_regs(unsigned int cpu)
> >   	siefp.as_uint64 = hv_get_register(HV_REGISTER_SIEFP);
> >   	siefp.siefp_enabled = 1;
> >   
> > -	if (hv_isolation_type_snp()) {
> > +	if (hv_isolation_type_snp() || hv_root_partition) {
> >   		hv_cpu->synic_event_page =
> >   			memremap(siefp.base_siefp_gpa << HV_HYP_PAGE_SHIFT,
> >   				 HV_HYP_PAGE_SIZE, MEMREMAP_WB);
> > --
> > 2.25.1