Message-ID:
 <SN6PR02MB41578D18024EC5339690046CD45CA@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Tue, 22 Jul 2025 17:09:12 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Naman Jain <namjain@...ux.microsoft.com>, "K . Y . Srinivasan"
	<kys@...rosoft.com>, Haiyang Zhang <haiyangz@...rosoft.com>, Wei Liu
	<wei.liu@...nel.org>, Dexuan Cui <decui@...rosoft.com>
CC: Roman Kisel <romank@...ux.microsoft.com>, Anirudh Rayabharam
	<anrayabh@...ux.microsoft.com>, Saurabh Sengar <ssengar@...ux.microsoft.com>,
	Stanislav Kinsburskii <skinsburskii@...ux.microsoft.com>, Nuno Das Neves
	<nunodasneves@...ux.microsoft.com>, ALOK TIWARI <alok.a.tiwari@...cle.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>
Subject: RE: [PATCH v5 2/2] Drivers: hv: Introduce mshv_vtl driver

From: Naman Jain <namjain@...ux.microsoft.com> Sent: Tuesday, July 22, 2025 4:09 AM
> 
> On 7/18/2025 8:37 PM, Michael Kelley wrote:
> > From: Naman Jain <namjain@...ux.microsoft.com> Sent: Thursday, July 17, 2025 9:36 PM
> >>
> >> On 7/9/2025 10:49 PM, Michael Kelley wrote:
> >>> From: Naman Jain <namjain@...ux.microsoft.com> Sent: Wednesday, June 11, 2025 12:27 AM
> >
> > [snip]
> >
> >>
> >>> Separately, "allow_bitmap" size is 64K bytes, or 512K bits. Is that the
> >>> correct size?  From looking at mshv_vtl_hvcall_is_allowed(), I think this
> >>> bitmap is indexed by the HV call code, which is a 16 bit value. So you
> >>> only need 64K bits, and the size is too big by a factor of 8. In any case,
> >>> it seems like the size should not be expressed in terms of PAGE_SIZE.
> >>
> >> There are HVcall codes which are of type u16. So max(HVcall code) =
> >> 0xffff.
> >>
> >> For every HVcall that needs to be allowed, we are saving HVcall code
> >> info in a bitmap in below fashion:
> >> if x = HVCall code and bitmap is an array of u64, of size
> >> ((0xffff/64=1023) + 1)
> >>
> >> bitmap[x / 64] = (u64)1 << (x%64);
> >>
> >> Later on in mshv_vtl_hvcall_is_allowed(), we calculate the array index
> >> by dividing it by 64, and then see if call_code/64 bit is set.
> >
> > I didn't add comments in mshv_vtl_hvcall_is_allowed(), but that code
> > can be simplified by recognizing that the Linux kernel bitmap utilities
> > can operate on bitmaps that are much larger than just 64 bits. Let's
> > assume that the allow_bitmap field in struct mshv_vtl_hvcall_fds has
> > 64K bits, regardless of whether it is declared as an array of u64,
> > an array of u16, or an array of u8. Then mshv_vtl_hvcall_is_allowed()
> > can be implemented as a single line:
> >
> > 	return test_bit(call_code, fd->allow_bitmap);
> >
> > There's no need to figure out which array element contains the bit,
> > or to construct a mask to select that particular bit in the array element.
> > And since call_code is a u16, test_bit won't access outside the allocated
> > 64K bits.
> >
> 
> I understood it now. This works and is much better. Will incorporate it
> in next patch.
> 
> >>
> >> Coming to size of allow_bitmap[], it is independent of PAGE_SIZE, and
> >> can be safely initialized to 1024 (reducing by a factor of 8).
> >> bitmap_size's maximum value is going to be 1024 in current
> >> implementation, picking u64 was not mandatory, u16 will also work. Also,
> >> item_index is also u16, so I should make bitmap_size as u16.
> >
> > The key question for me is whether bitmap_size describes the number
> > of bits in allow_bitmap, or whether it describes the number of array
> > elements in the declared allow_bitmap array. It's more typical to
> > describe a bitmap size as the number of bits. Then the value is
> > independent of the array element size, as the array element size
> > usually doesn't really matter anyway if using the Linux kernel's
> > bitmap utilities. The array element size only matters in allocating
> > the correct amount of space is for whatever number of bits are
> > needed in the bitmap.
> >
> 
> I tried to put your suggestions in code. Please let me know if below
> works. I tested this and it works. Just that, I am a little hesitant in
> changing things on Userspace side of it, which passes these parameters
> in IOCTL. This way, userspace remains the same, the confusion of names
> may go away, and the code becomes simpler.

Yes, I suspected there might be constraints due to not wanting
to disturb existing user space code.

> 
> --- a/include/uapi/linux/mshv.h
> +++ b/include/uapi/linux/mshv.h
> @@ -332,7 +332,7 @@ struct mshv_vtl_set_poll_file {
>   };
> 
>   struct mshv_vtl_hvcall_setup {
> -       __u64 bitmap_size;
> +       __u64 bitmap_array_size;
>          __u64 allow_bitmap_ptr; /* pointer to __u64 */
>   };
> 
> 
> --- a/drivers/hv/mshv_vtl_main.c
> +++ b/drivers/hv/mshv_vtl_main.c
> @@ -52,10 +52,12 @@ static bool has_message;
>   static struct eventfd_ctx *flag_eventfds[HV_EVENT_FLAGS_COUNT];
>   static DEFINE_MUTEX(flag_lock);
>   static bool __read_mostly mshv_has_reg_page;
> -#define MAX_BITMAP_SIZE 1024
> +
> +/* hvcall code is of type u16, allocate a bitmap of size (1 << 16) to accomodate it */

s/accomodate/accommodate/	

> +#define MAX_BITMAP_SIZE (1 << 16)
> 
>   struct mshv_vtl_hvcall_fd {
> -       u64 allow_bitmap[MAX_BITMAP_SIZE];
> +       u64 allow_bitmap[MAX_BITMAP_SIZE / 64];

OK, this seems like a reasonable approach to get the correct
number of bits.

>          bool allow_map_initialized;
>          /*
>           * Used to protect hvcall setup in IOCTLs
> 
> @@ -1204,12 +1207,12 @@ static int mshv_vtl_hvcall_do_setup(struct mshv_vtl_hvcall_fd *fd,
>                             sizeof(struct mshv_vtl_hvcall_setup))) {
>                  return -EFAULT;
>          }
> -       if (hvcall_setup.bitmap_size > ARRAY_SIZE(fd->allow_bitmap)) {
> +       if (hvcall_setup.bitmap_array_size > ARRAY_SIZE(fd->allow_bitmap)) {
>                  return -EINVAL;
>          }
>          if (copy_from_user(&fd->allow_bitmap,
>                             (void __user *)hvcall_setup.allow_bitmap_ptr,
> -                          hvcall_setup.bitmap_size)) {
> +                          hvcall_setup.bitmap_array_size)) {

This still doesn't work. copy_from_user() expects a byte count as its
3rd argument. If we have 64K bits in the bitmap, that means 8K bytes,
and 1K for bitmap_array_size. So the value of the 3rd argument
here is 1K, and you'll be copying 1K bytes when you want to be
copying 8K bytes. The 3rd argument needs to be:

		hv_call_setup.bitmap_array_size * sizeof(u64)

It's a bit messy to have to add this multiply, and in the bigger
picture it might be cleaner to have the bitmap_array be
declared in units of u8 instead of u64, but that would affect
user space in a way that you probably want to avoid.

Have you checked that user space is passing in the correct values
to the ioctl? If the kernel side code is wrong, user space might be
wrong as well. And if user space is wrong, then you might have
the flexibility to change everything to use u8 instead of u64.

>                  return -EFAULT;
>          }
> 
> @@ -1221,11 +1224,7 @@ static int mshv_vtl_hvcall_do_setup(struct mshv_vtl_hvcall_fd *fd,
> 
>   static bool mshv_vtl_hvcall_is_allowed(struct mshv_vtl_hvcall_fd *fd, u16 call_code)
>   {
> -       u8 bits_per_item = 8 * sizeof(fd->allow_bitmap[0]);
> -       u16 item_index = call_code / bits_per_item;
> -       u64 mask = 1ULL << (call_code % bits_per_item);
> -
> -       return fd->allow_bitmap[item_index] & mask;
> +       return test_bit(call_code, (unsigned long *)fd->allow_bitmap);
>   }

Yes, the rest of this looks good.

Michael

Powered by blists - more mailing lists