Re: [PATCH v5 10/10] Drivers: hv: Introduce mshv_root module to expose /dev/mshv to VMMs

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

On 2/26/2025 8:59 PM, Easwar Hariharan wrote:
> On 2/26/2025 3:08 PM, Nuno Das Neves wrote:
>> Provide a set of IOCTLs for creating and managing child partitions when
>> running as root partition on Hyper-V. The new driver is enabled via
>> CONFIG_MSHV_ROOT.
>>
>> A brief overview of the interface:
>>
>> MSHV_CREATE_PARTITION is the entry point, returning a file descriptor
>> representing a child partition. IOCTLs on this fd can be used to map
>> memory, create VPs, etc.
>>
>> Creating a VP returns another file descriptor representing that VP which
>> in turn has another set of corresponding IOCTLs for running the VP,
>> getting/setting state, etc.
>>
>> MSHV_ROOT_HVCALL is a generic "passthrough" hypercall IOCTL which can be
>> used for a number of partition or VP hypercalls. This is for hypercalls
>> that do not affect any state in the kernel driver, such as getting and
>> setting VP registers and partition properties, translating addresses,
>> etc. It is "passthrough" because the binary input and output for the
>> hypercall is only interpreted by the VMM - the kernel driver does
>> nothing but insert the VP and partition id where necessary (which are
>> always in the same place), and execute the hypercall.
>>
>> Co-developed-by: Wei Liu <wei.liu@kernel.org>
>> Signed-off-by: Wei Liu <wei.liu@kernel.org>
>> Co-developed-by: Stanislav Kinsburskii <skinsburskii@linux.microsoft.com>
>> Signed-off-by: Stanislav Kinsburskii <skinsburskii@linux.microsoft.com>
>> Co-developed-by: Praveen K Paladugu <prapal@linux.microsoft.com>
>> Signed-off-by: Praveen K Paladugu <prapal@linux.microsoft.com>
>> Co-developed-by: Mukesh Rathor <mrathor@linux.microsoft.com>
>> Signed-off-by: Mukesh Rathor <mrathor@linux.microsoft.com>
>> Co-developed-by: Jinank Jain <jinankjain@microsoft.com>
>> Signed-off-by: Jinank Jain <jinankjain@microsoft.com>
>> Co-developed-by: Muminul Islam <muislam@microsoft.com>
>> Signed-off-by: Muminul Islam <muislam@microsoft.com>
>> Co-developed-by: Anirudh Rayabharam <anrayabh@linux.microsoft.com>
>> Signed-off-by: Anirudh Rayabharam <anrayabh@linux.microsoft.com>
>> Signed-off-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>
>> ---
> 
> I see some issues reported by checkpatch, both vanilla and --strict.
> <snip>

Yes, most of them are from --strict.

The macro argument reuse ones are a non-issue I think. I suppose this
could be cleaned up for the vp_ and pt_ macros, I might do that.

"struct mutex/spinlock_t definition without comment" - I'm not sure
if that's really needed. The code that uses these primitives
demonstrates their purpose better than a comment, I think.

"Avoid CamelCase" - Some Hyper-V definitions that use the original
CamelCase definitions are introduced in this patch. These are
stats-related - partition and vp statistics that can be gathered
from the hypervisor. In a future patch these will be converted to
strings and displayed in debugfs, and... hmm, to be honest I'm not
sure why they need to remain in CamelCase when we convert everything
else to Linux style... For now there are only 2 of these definitions
and they're only defined in mshv_root_main.c so I think it's ok.
I'll consider what to do when the rest of the stats code is proposed,
which includes a big chunk of these CamelCase definitions.

"Use of volatile is usually wrong" - I admit I'm not an expert in
this area. We use it for a pointer to hv_synic_event_ring, similar
to how it is used to access hv_synic_event_flags_page in vmbus_drv.c.

"Added, moved or deleted file(s), does MAINTAINERS need updating?" -
drivers/hv is already listed in MAINTAINERS.

Thanks
Nuno

> 
> Thanks,
> Easwar (he/him)