[PATCH v2 15/15] Drivers: hv: Add modules to expose /dev/mshv to VMMs running on Hyper-V

[Saurabh Singh Sengar]

> -----Original Message-----
> From: Nuno Das Neves <nunodasneves at linux.microsoft.com>
> Sent: Wednesday, August 23, 2023 1:49 AM
> To: Saurabh Singh Sengar <ssengar at microsoft.com>; linux-
> hyperv at vger.kernel.org; linux-kernel at vger.kernel.org; x86 at kernel.org; linux-
> arm-kernel at lists.infradead.org; linux-arch at vger.kernel.org
> Cc: patches at lists.linux.dev; Michael Kelley (LINUX)
> <mikelley at microsoft.com>; KY Srinivasan <kys at microsoft.com>;
> wei.liu at kernel.org; Haiyang Zhang <haiyangz at microsoft.com>; Dexuan Cui
> <decui at microsoft.com>; apais at linux.microsoft.com; Tianyu Lan
> <Tianyu.Lan at microsoft.com>; ssengar at linux.microsoft.com; MUKESH
> RATHOR <mukeshrathor at microsoft.com>; stanislav.kinsburskiy at gmail.com;
> jinankjain at linux.microsoft.com; vkuznets <vkuznets at redhat.com>;
> tglx at linutronix.de; mingo at redhat.com; bp at alien8.de;
> dave.hansen at linux.intel.com; hpa at zytor.com; will at kernel.org;
> catalin.marinas at arm.com
> Subject: Re: [PATCH v2 15/15] Drivers: hv: Add modules to expose /dev/mshv
> to VMMs running on Hyper-V
> 
> On 8/19/2023 10:19 PM, Saurabh Singh Sengar wrote:
> >
> >
> >> -----Original Message-----
> >> From: Nuno Das Neves <nunodasneves at linux.microsoft.com>
> >> Sent: Saturday, August 19, 2023 12:30 AM
> >> To: Saurabh Singh Sengar <ssengar at microsoft.com>; linux-
> >> hyperv at vger.kernel.org; linux-kernel at vger.kernel.org; x86 at kernel.org;
> >> linux- arm-kernel at lists.infradead.org; linux-arch at vger.kernel.org
> >> Cc: patches at lists.linux.dev; Michael Kelley (LINUX)
> >> <mikelley at microsoft.com>; KY Srinivasan <kys at microsoft.com>;
> >> wei.liu at kernel.org; Haiyang Zhang <haiyangz at microsoft.com>; Dexuan
> >> Cui <decui at microsoft.com>; apais at linux.microsoft.com; Tianyu Lan
> >> <Tianyu.Lan at microsoft.com>; ssengar at linux.microsoft.com; MUKESH
> >> RATHOR <mukeshrathor at microsoft.com>;
> stanislav.kinsburskiy at gmail.com;
> >> jinankjain at linux.microsoft.com; vkuznets <vkuznets at redhat.com>;
> >> tglx at linutronix.de; mingo at redhat.com; bp at alien8.de;
> >> dave.hansen at linux.intel.com; hpa at zytor.com; will at kernel.org;
> >> catalin.marinas at arm.com
> >> Subject: Re: [PATCH v2 15/15] Drivers: hv: Add modules to expose
> >> /dev/mshv to VMMs running on Hyper-V
> >>
> >> On 8/18/2023 6:08 AM, Saurabh Singh Sengar wrote:
> >>>> +
> >>>> +config MSHV_VTL
> >>>> +	tristate "Microsoft Hyper-V VTL driver"
> >>>> +	depends on MSHV
> >>>> +	select HYPERV_VTL_MODE
> >>>> +	select TRANSPARENT_HUGEPAGE
> >>>
> >>> TRANSPARENT_HUGEPAGE can be avoided for now.
> >>>
> >>
> >> I will remove it in the next version. Thanks.
> >>>> +
> >>>> +#define HV_GET_REGISTER_BATCH_SIZE	\
> >>>> +	(HV_HYP_PAGE_SIZE / sizeof(union hv_register_value))
> >>>> +#define HV_SET_REGISTER_BATCH_SIZE	\
> >>>> +	((HV_HYP_PAGE_SIZE - sizeof(struct hv_input_set_vp_registers)) \
> >>>> +		/ sizeof(struct hv_register_assoc))
> >>>> +
> >>>> +int hv_call_get_vp_registers(
> >>>> +		u32 vp_index,
> >>>> +		u64 partition_id,
> >>>> +		u16 count,
> >>>> +		union hv_input_vtl input_vtl,
> >>>> +		struct hv_register_assoc *registers) {
> >>>> +	struct hv_input_get_vp_registers *input_page;
> >>>> +	union hv_register_value *output_page;
> >>>> +	u16 completed = 0;
> >>>> +	unsigned long remaining = count;
> >>>> +	int rep_count, i;
> >>>> +	u64 status;
> >>>> +	unsigned long flags;
> >>>> +
> >>>> +	local_irq_save(flags);
> >>>> +
> >>>> +	input_page = *this_cpu_ptr(hyperv_pcpu_input_arg);
> >>>> +	output_page = *this_cpu_ptr(hyperv_pcpu_output_arg);
> >>>> +
> >>>> +	input_page->partition_id = partition_id;
> >>>> +	input_page->vp_index = vp_index;
> >>>> +	input_page->input_vtl.as_uint8 = input_vtl.as_uint8;
> >>>> +	input_page->rsvd_z8 = 0;
> >>>> +	input_page->rsvd_z16 = 0;
> >>>> +
> >>>> +	while (remaining) {
> >>>> +		rep_count = min(remaining, HV_GET_REGISTER_BATCH_SIZE);
> >>>> +		for (i = 0; i < rep_count; ++i)
> >>>> +			input_page->names[i] = registers[i].name;
> >>>> +
> >>>> +		status = hv_do_rep_hypercall(HVCALL_GET_VP_REGISTERS,
> >>>> rep_count,
> >>>> +					     0, input_page, output_page);
> >>>
> >>> Is there any possibility that count value is passed 0 by mistake ?
> >>> In that case status will remain uninitialized.
> >>>
> >>
> >> These lines ensure rep_count is never 0 here:
> >>
> >> 	while (remaining) {
> >> 		rep_count = min(remaining, HV_GET_REGISTER_BATCH_SIZE);
> >>
> >> Remaining can't be 0 or the loop would exit, and
> >> HV_GET_REGISTER_BATCH_SIZE is not 0, or we would never get any
> registers.
> >
> > There is a parameter in this function "count". I was checking if there
> > is any possibility that is passed as 0 by mistake ? this will lead to
> > "remaining" value as 0 and loop will never execute. Which results using
> "status" uninitialized later in the function.
> >
> >
> 
> Ah ok, thanks! I will change it to return immediately in case count is 0.

Or you can initialize status with appropriate error value, either way is fine.
Please consider fixing the same issue in hv_call_set_vp_registers as well.

- Saurabh