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

[Nuno Das Neves]

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.
>>>> diff --git a/drivers/hv/mshv.h b/drivers/hv/mshv.h
>>>> new file mode 100644
>>>> index 000000000000..166480a73f3f
>>>> --- /dev/null
>>>> +++ b/drivers/hv/mshv.h
>>>> @@ -0,0 +1,156 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>>> +/*
>>>> + * Copyright (c) 2023, Microsoft Corporation.
>>>> + */
>>>> +
>>>> +#ifndef _MSHV_H_
>>>> +#define _MSHV_H_
>>>> +
>>>> +#include <linux/spinlock.h>
>>>> +#include <linux/mutex.h>
>>>> +#include <linux/semaphore.h>
>>>> +#include <linux/sched.h>
>>>> +#include <linux/srcu.h>
>>>> +#include <linux/wait.h>
>>>> +#include <uapi/linux/mshv.h>
>>>> +
>>>> +/*
>>>> + * Hyper-V hypercalls
>>>> + */
>>>> +
>>>> +int hv_call_withdraw_memory(u64 count, int node, u64 partition_id);
>>>> +int hv_call_create_partition(
>>>> +		u64 flags,
>>>> +		struct hv_partition_creation_properties creation_properties,
>>>> +		union hv_partition_isolation_properties isolation_properties,
>>>> +		u64 *partition_id);
>>>> +int hv_call_initialize_partition(u64 partition_id);
>>>> +int hv_call_finalize_partition(u64 partition_id);
>>>> +int hv_call_delete_partition(u64 partition_id);
>>>> +int hv_call_map_gpa_pages(
>>>> +		u64 partition_id,
>>>> +		u64 gpa_target,
>>>> +		u64 page_count, u32 flags,
>>>> +		struct page **pages);
>>>> +int hv_call_unmap_gpa_pages(
>>>> +		u64 partition_id,
>>>> +		u64 gpa_target,
>>>> +		u64 page_count, u32 flags);
>>>> +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);
>>>> +int hv_call_get_gpa_access_states(
>>>> +		u64 partition_id,
>>>> +		u32 count,
>>>> +		u64 gpa_base_pfn,
>>>> +		u64 state_flags,
>>>> +		int *written_total,
>>>> +		union hv_gpa_page_access_state *states);
>>>> +
>>>> +int hv_call_set_vp_registers(
>>>> +		u32 vp_index,
>>>> +		u64 partition_id,
>>>> +		u16 count,
>>>> +		union hv_input_vtl input_vtl,
>>>> +		struct hv_register_assoc *registers);
>>>
>>> Nit: Opportunity to fix many of the checkpatch.pl related to line break here
>>> and many other places.
>>>
>>
>> checkpatch.pl doesn't complain about anything in this file.
> 
> If we use --strict switch with checkpatch.pl we observe additional CHECK(s).
> I observe 159 CHECK(s) with this patch overall.
> (total: 1 errors, 7 warnings, 159 checks, 7460 lines checked)
> Few examples:
> 
> CHECK: Lines should not end with a '('
> #240: FILE: drivers/hv/hv_call.c:73:
> +int hv_call_set_vp_registers(
> 
> CHECK: Alignment should match open parenthesis
> #266: FILE: drivers/hv/hv_call.c:99:
> +               memcpy(input_page->elements, registers,
> +                       sizeof(struct hv_register_assoc) * rep_count);
> 
> 

I didn't try --strict before. Thanks, I'll fix up the formatting.

> I also see an error with flexible array, possibly we can fix that as well.
> 
> ERROR: Use C99 flexible arrays - see https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays
> #7468: FILE: include/uapi/linux/mshv.h:134:
> +       struct mshv_msi_routing_entry entries[0];
> +};
> 

Indeed, I'll fix it too.