From: Naman Jain <namj...@linux.microsoft.com> Sent: Wednesday, July 23, 2025
2:59 AM
>
> On 7/22/2025 10:39 PM, Michael Kelley wrote:
> > From: Naman Jain <namj...@linux.microsoft.com> Sent: Tuesday, July 22, 2025
> > 4:09 AM
> >>
> >> On 7/18/2025 8:37 PM, Michael Kelley wrote:
> >>> From: Naman Jain <namj...@linux.microsoft.com> Sent: Thursday, July 17,
> >>> 2025 9:36 PM
> >>>>
> >>>> On 7/9/2025 10:49 PM, Michael Kelley wrote:
> >>>>> From: Naman Jain <namj...@linux.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/
>
> Acked.
>
> >
> >> +#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.
> >
>
>
> This is correct in Userspace:
> https://github.com/microsoft/openvmm/blob/main/openhcl/hcl/src/ioctl.rs#L905
>
I'm not really Rust literate, but it does look like userspace is
setting the bitmap_size to the number of *bytes*. User space has
used u64 as its underlying type, which is OK as long as the ioctl
API is understood to take a byte count in the bitmap_size field,
and user space populates that field with a byte count, which it does.
> This was wrong in kernel code internally from the beginning. This did
> not get caught because we took a large array size (2 * PAGE_SIZE) which
> never failed the check
> (hvcall_setup.bitmap_array_size > ARRAY_SIZE(fd->allow_bitmap))
> and hvcall_setup.bitmap_size always had number of bytes to copy.
>
> We can make allow_bitmap as an array of u8 here, irrespective of how
> userspace is setting the bits in the bitmap because it is finally doing
> the same thing. I'll make the change.
Yep. So on the kernel side, bitmap_array_size is a *byte* count,
and user space and the kernel side are in agreement. ;-)
>
>
> >> 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
>
> For CPU hotplug, I checked internally and we do not support hotplug in
> VTL2 for OpenHCL. Hotplugging vCPUs in VTL0 is transparent to VTL2 and
> these CPUs remain online in VTL2. VTL2 also won't be hotplugging CPUs
> when VTL0 is running.
Good to know.
>
> I am planning to put a comment mentioning this in the two places where
> we check for cpu_online() in the driver. Preventing hotplug from
> happening through hotplug notifiers is a bit overkill for me, as of now,
> but if you feel that should be done, I will work on it.
I'm marginally OK with just a comment because VTL2 code is not
running general-purpose workloads.
>
> Since good enough changes are done in this version, I will send the next
> version soon, unless there are follow-up comments here which needs
> something to change. We can then have a fresh look at it and work
> together to enhance it.
Sounds good.
Michael
