Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Date:   Tue, 26 Sep 2023 05:33:36 -0700
From:   Saurabh Singh Sengar <ssengar@linux.microsoft.com>
To:     Nuno Das Neves <nunodasneves@linux.microsoft.com>
Cc:     Greg KH <gregkh@linuxfoundation.org>, linux-hyperv@vger.kernel.org,
        linux-kernel@vger.kernel.org, x86@kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-arch@vger.kernel.org,
        patches@lists.linux.dev, mikelley@microsoft.com, kys@microsoft.com,
        wei.liu@kernel.org, haiyangz@microsoft.com, decui@microsoft.com,
        apais@linux.microsoft.com, Tianyu.Lan@microsoft.com,
        mukeshrathor@microsoft.com, stanislav.kinsburskiy@gmail.com,
        jinankjain@linux.microsoft.com, vkuznets@redhat.com,
        tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
        dave.hansen@linux.intel.com, hpa@zytor.com, will@kernel.org,
        catalin.marinas@arm.com
Subject: Re: [PATCH v3 15/15] Drivers: hv: Add modules to expose /dev/mshv to
 VMMs running on Hyper-V
Message-ID: <20230926123336.GA9863@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net>
References: <1695407915-12216-1-git-send-email-nunodasneves@linux.microsoft.com>
 <1695407915-12216-16-git-send-email-nunodasneves@linux.microsoft.com>
 <2023092342-staunch-chafe-1598@gregkh>
 <e235025e-abfa-4b31-8b83-416ec8ec4f72@linux.microsoft.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <e235025e-abfa-4b31-8b83-416ec8ec4f72@linux.microsoft.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

On Mon, Sep 25, 2023 at 05:07:24PM -0700, Nuno Das Neves wrote:
> Resend in plain text instead of HTML - oops!
> 
> On 9/23/2023 12:58 AM, Greg KH wrote:
> >On Fri, Sep 22, 2023 at 11:38:35AM -0700, Nuno Das Neves wrote:
> >>+static int mshv_vtl_get_vsm_regs(void)
> >>+{
> >>+	struct hv_register_assoc registers[2];
> >>+	union hv_input_vtl input_vtl;
> >>+	int ret, count = 2;
> >>+
> >>+	input_vtl.as_uint8 = 0;
> >>+	registers[0].name = HV_REGISTER_VSM_CODE_PAGE_OFFSETS;
> >>+	registers[1].name = HV_REGISTER_VSM_CAPABILITIES;
> >>+
> >>+	ret = hv_call_get_vp_registers(HV_VP_INDEX_SELF, HV_PARTITION_ID_SELF,
> >>+				       count, input_vtl, registers);
> >>+	if (ret)
> >>+		return ret;
> >>+
> >>+	mshv_vsm_page_offsets.as_uint64 = registers[0].value.reg64;
> >>+	mshv_vsm_capabilities.as_uint64 = registers[1].value.reg64;
> >>+
> >>+	pr_debug("%s: VSM code page offsets: %#016llx\n", __func__,
> >>+		 mshv_vsm_page_offsets.as_uint64);
> >>+	pr_info("%s: VSM capabilities: %#016llx\n", __func__,
> >>+		mshv_vsm_capabilities.as_uint64);
> >
> >When drivers are working properly, they are quiet.  This is very noisy
> >and probably is leaking memory addresses to userspace?
> >
> 
> I will remove these, thanks.
> 
> >Also, there is NEVER a need for __func__ in a pr_debug() line, it has
> >that for you automatically.
> >
> 
> Thank you, I didn't know this.
> 
> >Also, drivers should never call pr_*() calls, always use the proper
> >dev_*() calls instead.
> >
> 
> We only use struct device in one place in this driver, I think that
> is the only place it makes sense to use dev_*() over pr_*() calls.
> >
> >
> >>+
> >>+	return ret;
> >>+}
> >>+
> >>+static int mshv_vtl_configure_vsm_partition(void)
> >>+{
> >>+	union hv_register_vsm_partition_config config;
> >>+	struct hv_register_assoc reg_assoc;
> >>+	union hv_input_vtl input_vtl;
> >>+
> >>+	config.as_u64 = 0;
> >>+	config.default_vtl_protection_mask = HV_MAP_GPA_PERMISSIONS_MASK;
> >>+	config.enable_vtl_protection = 1;
> >>+	config.zero_memory_on_reset = 1;
> >>+	config.intercept_vp_startup = 1;
> >>+	config.intercept_cpuid_unimplemented = 1;
> >>+
> >>+	if (mshv_vsm_capabilities.intercept_page_available) {
> >>+		pr_debug("%s: using intercept page", __func__);
> >
> >Again, __func__ is not needed, you are providing it twice here for no
> >real reason except to waste storage space :)
> >
> 
> Thanks, I will review all the uses of pr_debug().
> 
> >>+		config.intercept_page = 1;
> >>+	}
> >>+
> >>+	reg_assoc.name = HV_REGISTER_VSM_PARTITION_CONFIG;
> >>+	reg_assoc.value.reg64 = config.as_u64;
> >>+	input_vtl.as_uint8 = 0;
> >>+
> >>+	return hv_call_set_vp_registers(HV_VP_INDEX_SELF, HV_PARTITION_ID_SELF,
> >>+				       1, input_vtl, &reg_assoc);
> >
> >
> >None of this needs to be unwound if initialization fails later on?
> >
> 
> I think unwinding this is not needed, not 100% sure.
> Saurabh, can you comment?

Yes unwinding is not useful here, as these are synthetic register
and there is no other use case of VSM supporting platforms other
than VSM configuration.

In a non VSM supported platform hv_call_set_vp_registers itself will
fail for HV_REGISTER_VSM_PARTITION_CONFIG.

- Saurabh

> 
> Thanks,
> Nuno
> 
> >thanks,
> >
> >greg k-h
>