Bug 264840 - dev/vmware/pvscsi: Expand vcpuHint to 16 bit to aliagn with host side change
Summary: dev/vmware/pvscsi: Expand vcpuHint to 16 bit to aliagn with host side change
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: amd64 Any
: --- Affects Some People
Assignee: freebsd-virtualization (Nobody)
URL: https://lore.kernel.org/lkml/20220620...
Keywords: easy, needs-qa, patch
Depends on:
Blocks:
 
Reported: 2022-06-23 08:10 UTC by Wentao Wang
Modified: 2022-11-05 20:31 UTC (History)
7 users (show)

See Also:
koobs: maintainer-feedback? (jpaetzel)
koobs: maintainer-feedback? (imp)
imp: mfc-stable13-
imp: mfc-stable12-


Attachments
vmw_pvscsi: Expand vcpuHint to 16 bit to aliagn with host side change. (419 bytes, patch)
2022-06-23 08:10 UTC, Wentao Wang
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wentao Wang 2022-06-23 08:10:46 UTC
Created attachment 234880 [details]
vmw_pvscsi: Expand vcpuHint to 16 bit to aliagn with host side change.

vcpuHint has been expanded to 16 bit on host side to enable interruptions to be routed to more CPUs. Guest side should align with the change. 
This change has been tested with hosts with 8-bit and 16-bit vcpuHint, on both platforms host side can get correct value.
Comment 1 Wentao Wang 2022-06-23 08:13:42 UTC
My similar accepted patch for Linux:
https://lore.kernel.org/lkml/20220620124744.578658384@linuxfoundation.org/
Comment 2 Zhenlei Huang freebsd_committer freebsd_triage 2022-10-17 01:13:27 UTC
I think we can give it another try when https://reviews.freebsd.org/D36838 is merged.

And, is the hypervisor (VMware in this context) responsible to process the endianness ?
> vcpuHint has been expanded to 16 bit on host side to enable interruptions to be routed to more CPUs.
On big-endian system, this seems break the vcpuHint function if guest side is not aligned with the change.
Comment 3 Wentao Wang 2022-10-31 06:51:41 UTC
(In reply to Zhenlei Huang from comment #2)
This driver is for ESXi product which only supports x86/x64. They are little-endian. So there is no need to consider big-endian system.
Comment 4 Zhenlei Huang freebsd_committer freebsd_triage 2022-10-31 07:16:21 UTC
(In reply to Wentao Wang from comment #3)
> This driver is for ESXi product which only supports x86/x64. They are little-endian. 
> So there is no need to consider big-endian system.

Then the patch looks good to me.
Comment 5 Wentao Wang 2022-11-01 07:19:03 UTC
(In reply to Zhenlei Huang from comment #4)
Thanks for the review.
Comment 6 Warner Losh freebsd_committer freebsd_triage 2022-11-01 15:29:18 UTC
I think this is a good change.
Comment 7 Warner Losh freebsd_committer freebsd_triage 2022-11-01 15:31:18 UTC
Wentao,

I notice the Linux change was posted from a vmware account, while this is posted from a gmail account. FreeBSD sets the 'author' to the actual person who wrote the change. Do you have a preference over which email address I should use?
Comment 8 Ed Maste freebsd_committer freebsd_triage 2022-11-01 16:45:11 UTC
(In reply to Zhenlei Huang from comment #2)
This change shouldn't wait for D36838, also MAXCPU can be set in the kernel config file for testing.
Comment 9 Zhenlei Huang freebsd_committer freebsd_triage 2022-11-01 17:07:21 UTC
(In reply to Ed Maste from comment #8)

> This change shouldn't wait for D36838, also MAXCPU can be set in the kernel config file for testing.

Yes, they are independent in principle.

> I think we can give it another try when https://reviews.freebsd.org/D36838 is merged.

What I thought after D36838 being landed ( depends on https://reviews.freebsd.org/D37067), it is easier to verify this patch actually works ( on FreeBSD VMs with CPUs count over 256 ).

Anyway, it is unlikely this patch does not work. So feel free for the commit order of D36838 and this one.
Comment 10 Wentao Wang 2022-11-02 03:17:52 UTC
(In reply to Warner Losh from comment #7)
Eagle eyes. Please use wwentao@vmware.com to consistent with the change if possible. Many thanks.
Comment 11 Warner Losh freebsd_committer freebsd_triage 2022-11-02 15:22:31 UTC
Fixed in 923704f7b8efefd988bedd88ab68540332efa3f8

Should be safe to MFC, since it is a nop for MAXCPU <= 256
Comment 12 commit-hook freebsd_committer freebsd_triage 2022-11-02 15:23:09 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=923704f7b8efefd988bedd88ab68540332efa3f8

commit 923704f7b8efefd988bedd88ab68540332efa3f8
Author:     Wentao Wang <wwentao@vmware.com>
AuthorDate: 2022-11-02 15:14:52 +0000
Commit:     Warner Losh <imp@FreeBSD.org>
CommitDate: 2022-11-02 15:14:52 +0000

    vmw_pvscsi: Expand vcpuHint to 16 bit to aliagn with host side change.

    vcpuHint has been expanded to 16 bit on host side to enable
    interruptions to be routed to more CPUs. Guest side should align with
    the change.

    This change has been tested with hosts with 8-bit and 16-bit vcpuHint,
    on both platforms host side can get correct value.

    This driver is for ESXi product which only supports x86/x64. They are
    little-endian. So there is no need to consider big-endian system.

    PR:             264840
    Reviewed by:    imp@, Zhenlei Huang

 sys/dev/vmware/pvscsi/pvscsi.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Comment 13 Warner Losh freebsd_committer freebsd_triage 2022-11-02 15:23:52 UTC
... but likely shouldn't unless there's other changes to merge (I hit save changes too quickly)
Comment 14 Yuri 2022-11-05 20:31:33 UTC
Just wanted to point out that there is also VMware Fusion Tech Preview for M1 Macs and it defaults to using pvscsi (so it's not only x86/x64); it's still little endian, of course, and pvscsi backed disks seem to work fine with 20221103 aarch64 snapshot.