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

[Wei Liu]

On Tue, Mar 18, 2025 at 05:45:46PM +0000, Michael Kelley wrote:
> From: Wei Liu <wei.liu@xxxxxxxxxx> Sent: Tuesday, March 18, 2025 10:25 AM
> > 
> > On Mon, Mar 17, 2025 at 11:51:52PM +0000, Michael Kelley wrote:
> > > From: Nuno Das Neves <nunodasneves@xxxxxxxxxxxxxxxxxxx> Sent: Wednesday,
> > February 26, 2025 3:08 PM
> > [...]
> > > > +static long
> > > > +mshv_vp_ioctl_get_set_state(struct mshv_vp *vp,
> > > > +			    struct mshv_get_set_vp_state __user *user_args,
> > > > +			    bool is_set)
> > > > +{
> > > > +	struct mshv_get_set_vp_state args;
> > > > +	long ret = 0;
> > > > +	union hv_output_get_vp_state vp_state;
> > > > +	u32 data_sz;
> > > > +	struct hv_vp_state_data state_data = {};
> > > > +
> > > > +	if (copy_from_user(&args, user_args, sizeof(args)))
> > > > +		return -EFAULT;
> > > > +
> > > > +	if (args.type >= MSHV_VP_STATE_COUNT || mshv_field_nonzero(args, rsvd) ||
> > > > +	    !args.buf_sz || !PAGE_ALIGNED(args.buf_sz) ||
> > > > +	    !PAGE_ALIGNED(args.buf_ptr))
> > > > +		return -EINVAL;
> > > > +
> > > > +	if (!access_ok((void __user *)args.buf_ptr, args.buf_sz))
> > > > +		return -EFAULT;
> > > > +
> > > > +	switch (args.type) {
> > > > +	case MSHV_VP_STATE_LAPIC:
> > > > +		state_data.type = HV_GET_SET_VP_STATE_LAPIC_STATE;
> > > > +		data_sz = HV_HYP_PAGE_SIZE;
> > > > +		break;
> > > > +	case MSHV_VP_STATE_XSAVE:
> > >
> > > Just FYI, you can put a semicolon after the colon on the above line, which
> > > adds a null statement, and then the C compiler will accept the definition
> > > of local variable data_sz_64 without needing the odd-looking braces.
> > >
> > > See https://stackoverflow.com/questions/92396/why-cant-variables-be-declared-in-a-switch-statement/19830820
> > >
> > 
> > This is a rarely seen pattern in the kernel, so I would prefer to keep
> > the braces for clarity.
> > 
> >  $ git grep -A5 -P 'case\s+\w+:;$'
> > 
> > This shows a few places are using this pattern. But they are not
> > declaring variables afterwards.
> > 
> 
> The braces just looked a little odd, particularly the way they are indented. Another
> alternative is to move the variable to function scope, and avoid the issue altogether.
> But I'm fine regardless of which approach you take, including keeping it like it is.
> 
> > > I learn something new every day! :-)
> > >
> > 
> > Yep, me too.
> > 
> > Thanks for reviewing the code. Nuno will address the comments. I can fix
> > them up.
> 
> FYI, I may submit a few more comments on the v6 version of the patches.
> If there are changes you want to make based on my comments, I don't care
> if you fix up the existing patches, or take them later as follow up patches.
> And of course, you may choose to not make changes.

We will aim to  address as many comments as possible in the first
submission to reduce the number of follow-up patches. This driver will
be backported to replace the one running internally in Microsoft, so the
fewer patches we need to backport the better.

Wei.

> 
> Michael