Re: [PATCH v6 2/2] Drivers: hv: Introduce mshv_vtl driver

[Naman Jain <namjain@linux.microsoft.com>]

On 7/27/2025 5:20 AM, Michael Kelley wrote:
> From: Naman Jain <namjain@linux.microsoft.com> Sent: Thursday, July 24, 2025 1:26 AM
>>
>> Provide an interface for Virtual Machine Monitor like OpenVMM and its
>> use as OpenHCL paravisor to control VTL0 (Virtual trust Level).
>> Expose devices and support IOCTLs for features like VTL creation,
>> VTL0 memory management, context switch, making hypercalls,
>> mapping VTL0 address space to VTL2 userspace, getting new VMBus
>> messages and channel events in VTL2 etc.
>>
>> Co-developed-by: Roman Kisel <romank@linux.microsoft.com>
>> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
>> Co-developed-by: Saurabh Sengar <ssengar@linux.microsoft.com>
>> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
>> Reviewed-by: Roman Kisel <romank@linux.microsoft.com>
>> Reviewed-by: Alok Tiwari <alok.a.tiwari@oracle.com>
>> Message-ID: <20250512140432.2387503-3-namjain@linux.microsoft.com>
>> Reviewed-by: Saurabh Sengar <ssengar@linux.microsoft.com>
>> Reviewed-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>
>> Signed-off-by: Naman Jain <namjain@linux.microsoft.com>
>> ---
>>   drivers/hv/Kconfig          |   22 +
>>   drivers/hv/Makefile         |    7 +-
>>   drivers/hv/mshv_vtl.h       |   52 ++
>>   drivers/hv/mshv_vtl_main.c  | 1508 +++++++++++++++++++++++++++++++++++
>>   include/hyperv/hvgdk_mini.h |  106 +++
>>   include/uapi/linux/mshv.h   |   80 ++
>>   6 files changed, 1774 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/hv/mshv_vtl.h
>>   create mode 100644 drivers/hv/mshv_vtl_main.c
>>
> 
> [snip]
> 
>> +
>> +static int mshv_vtl_set_reg(struct hv_register_assoc *regs)
>> +{
>> +	u64 reg64;
>> +	enum hv_register_name gpr_name;
>> +	int i;
>> +
>> +	gpr_name = regs->name;
>> +	reg64 = regs->value.reg64;
>> +
>> +	/* Search for the register in the table */
>> +	for (i = 0; i < ARRAY_SIZE(reg_table); i++) {
>> +		if (reg_table[i].reg_name == gpr_name) {
>> +			if (reg_table[i].debug_reg_num != -1) {
>> +				/* Handle debug registers */
>> +				if (gpr_name == HV_X64_REGISTER_DR6 &&
>> +				    !mshv_vsm_capabilities.dr6_shared)
>> +					goto hypercall;
>> +				native_set_debugreg(reg_table[i].debug_reg_num, reg64);
>> +			} else {
>> +				/* Handle MSRs */
>> +				wrmsrl(reg_table[i].msr_addr, reg64);
>> +			}
>> +			return 0;
>> +		}
>> +	}
>> +
>> +hypercall:
>> +	return 1;
>> +}
>> +
>> +static int mshv_vtl_get_reg(struct hv_register_assoc *regs)
>> +{
>> +	u64 *reg64;
>> +	enum hv_register_name gpr_name;
>> +	int i;
>> +
>> +	gpr_name = regs->name;
>> +	reg64 = (u64 *)&regs->value.reg64;
>> +
>> +	/* Search for the register in the table */
>> +	for (i = 0; i < ARRAY_SIZE(reg_table); i++) {
>> +		if (reg_table[i].reg_name == gpr_name) {
>> +			if (reg_table[i].debug_reg_num != -1) {
>> +				/* Handle debug registers */
>> +				if (gpr_name == HV_X64_REGISTER_DR6 &&
>> +				    !mshv_vsm_capabilities.dr6_shared)
>> +					goto hypercall;
>> +				*reg64 = native_get_debugreg(reg_table[i].debug_reg_num);
>> +			} else {
>> +				/* Handle MSRs */
>> +				rdmsrl(reg_table[i].msr_addr, *reg64);
>> +			}
>> +			return 0;
>> +		}
>> +	}
>> +
>> +hypercall:
>> +	return 1;
>> +}
>> +
> 
> One more comment on this patch. What do you think about
> combining mshv_vtl_set_reg() and mshv_vtl_get_reg() into a single
> function? The two functions have a lot code duplication that could be
> avoided. Here's my untested version (not even compile tested):
> 
> +static int mshv_vtl_get_set_reg(struct hv_register_assoc *regs, bool set)
> +{
> +	u64 *reg64;
> +	enum hv_register_name gpr_name;
> +	int i;
> +
> +	gpr_name = regs->name;
> +	reg64 = &regs->value.reg64;
> +
> +	/* Search for the register in the table */
> +	for (i = 0; i < ARRAY_SIZE(reg_table); i++) {
> +		if (reg_table[i].reg_name != gpr_name)
> +			continue;
> +		if (reg_table[i].debug_reg_num != -1) {
> +			/* Handle debug registers */
> +			if (gpr_name == HV_X64_REGISTER_DR6 &&
> +			    !mshv_vsm_capabilities.dr6_shared)
> +				goto hypercall;
> +			if (set)
> +				native_set_debugreg(reg_table[i].debug_reg_num, *reg64);
> +			else
> +				*reg64 = native_get_debugreg(reg_table[i].debug_reg_num);
> +		} else {
> +			/* Handle MSRs */
> +			if (set)
> +				wrmsrl(reg_table[i].msr_addr, *reg64);
> +			else
> +				rdmsrl(reg_table[i].msr_addr, *reg64);
> +		}
> +		return 0;
> +	}
> +
> +hypercall:
> +	return 1;
> +}
> +
> 
> Two call sites would need to be updated to pass "true" and "false",
> respectively, for the "set" parameter.
> 
> I changed the gpr_name matching to do "continue" on a mismatch
> just to avoid a level of indentation. It's functionally the same as your
> code.
> 
> Michael

Acked, looks good. Thanks for sharing the improvements. Sending v7 now.

Regards,
Naman