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

[Nuno Das Neves <nunodasneves@linux.microsoft.com>]

On 7/9/2025 10:19 AM, Michael Kelley wrote:
> From: Naman Jain <namjain@linux.microsoft.com> Sent: Wednesday, June 11, 2025 12:27 AM
>> +
>> +union mshv_synic_overlay_page_msr {
>> +	u64 as_uint64;
>> +	struct {
>> +		u64 enabled: 1;
>> +		u64 reserved: 11;
>> +		u64 pfn: 52;
>> +	};
> 
> Since this appear to be a Hyper-V synthetic MSR, add __packed?
> 
>> +};
>> +
>> +union hv_register_vsm_capabilities {
>> +	u64 as_uint64;
>> +	struct {
>> +		u64 dr6_shared: 1;
>> +		u64 mbec_vtl_mask: 16;
>> +		u64 deny_lower_vtl_startup: 1;
>> +		u64 supervisor_shadow_stack: 1;
>> +		u64 hardware_hvpt_available: 1;
>> +		u64 software_hvpt_available: 1;
>> +		u64 hardware_hvpt_range_bits: 6;
>> +		u64 intercept_page_available: 1;
>> +		u64 return_action_available: 1;
>> +		u64 reserved: 35;
>> +	} __packed;
>> +};
>> +
>> +union hv_register_vsm_page_offsets {
>> +	struct {
>> +		u64 vtl_call_offset : 12;
>> +		u64 vtl_return_offset : 12;
>> +		u64 reserved_mbz : 40;
>> +	};
>> +	u64 as_uint64;
>> +} __packed;
> 
> We've usually put the __packed on the struct definition.  Consistency .... :-)
> 
> Don't these three register definitions belong somewhere in the
> hvhdk or hvgdk include files?
> 

I agree, hv_register_vsm_capabilities and hv_register_vsm_page_offsets
can be moved to the appropriate include/hyperv/ header/s.

Regarding mshv_synic_overlay_page_msr, it is a generic structure that
appears to be used for several overlay page MSRs (SIMP, SIEF, etc).

But, the type doesn't appear in the hv*dk headers explicitly; it's just
used internally by the hypervisor.

I think it should be renamed with a hv_ prefix to indicate it's part of
the hypervisor ABI, and a brief comment with the provenance:

/* SYNIC_OVERLAY_PAGE_MSR - internal, identical to hv_synic_simp */
union hv_synic_overlay_page_msr {
	/* <snip> */
};

And I'm fine with it staying in this file since it's only used here right
now, and doesn't really come from the one of the hyperv headers.

Nuno