RE: [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@xxxxxxxxxxxxxxxxxxx>
> Sent: Wednesday, August 23, 2023 1:49 AM
> To: Saurabh Singh Sengar <ssengar@xxxxxxxxxxxxx>; linux-
> hyperv@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; x86@xxxxxxxxxx; linux-
> arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-arch@xxxxxxxxxxxxxxx
> Cc: patches@xxxxxxxxxxxxxxx; Michael Kelley (LINUX)
> <mikelley@xxxxxxxxxxxxx>; KY Srinivasan <kys@xxxxxxxxxxxxx>;
> wei.liu@xxxxxxxxxx; Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>; Dexuan Cui
> <decui@xxxxxxxxxxxxx>; apais@xxxxxxxxxxxxxxxxxxx; Tianyu Lan
> <Tianyu.Lan@xxxxxxxxxxxxx>; ssengar@xxxxxxxxxxxxxxxxxxx; MUKESH
> RATHOR <mukeshrathor@xxxxxxxxxxxxx>; stanislav.kinsburskiy@xxxxxxxxx;
> jinankjain@xxxxxxxxxxxxxxxxxxx; vkuznets <vkuznets@xxxxxxxxxx>;
> tglx@xxxxxxxxxxxxx; mingo@xxxxxxxxxx; bp@xxxxxxxxx;
> dave.hansen@xxxxxxxxxxxxxxx; hpa@xxxxxxxxx; will@xxxxxxxxxx;
> catalin.marinas@xxxxxxx
> 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@xxxxxxxxxxxxxxxxxxx>
> >> Sent: Saturday, August 19, 2023 12:30 AM
> >> To: Saurabh Singh Sengar <ssengar@xxxxxxxxxxxxx>; linux-
> >> hyperv@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; x86@xxxxxxxxxx;
> >> linux- arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-arch@xxxxxxxxxxxxxxx
> >> Cc: patches@xxxxxxxxxxxxxxx; Michael Kelley (LINUX)
> >> <mikelley@xxxxxxxxxxxxx>; KY Srinivasan <kys@xxxxxxxxxxxxx>;
> >> wei.liu@xxxxxxxxxx; Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>; Dexuan
> >> Cui <decui@xxxxxxxxxxxxx>; apais@xxxxxxxxxxxxxxxxxxx; Tianyu Lan
> >> <Tianyu.Lan@xxxxxxxxxxxxx>; ssengar@xxxxxxxxxxxxxxxxxxx; MUKESH
> >> RATHOR <mukeshrathor@xxxxxxxxxxxxx>;
> stanislav.kinsburskiy@xxxxxxxxx;
> >> jinankjain@xxxxxxxxxxxxxxxxxxx; vkuznets <vkuznets@xxxxxxxxxx>;
> >> tglx@xxxxxxxxxxxxx; mingo@xxxxxxxxxx; bp@xxxxxxxxx;
> >> dave.hansen@xxxxxxxxxxxxxxx; hpa@xxxxxxxxx; will@xxxxxxxxxx;
> >> catalin.marinas@xxxxxxx
> >> 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