Page MenuHomeFreeBSD

enabling new hypercalls using HvCallSetVpRegisters and HvCallGetVpRegisters
ClosedPublic

Authored by schakrabarti_microsoft.com on Aug 18 2022, 8:56 PM.

Details

Summary

Enabling HvCallSetVpRegisters and HvCallGetVpRegisters for hypercalls to read and write to specific MSRs.
This is required for implementing wrmsr and rdmsr, which is required for Hyper-V vmbus driver for ARM64.

Also we need to use arm smccc hvc 1.2 version as we need to access registers beyond X0-X3 for HvCallGetVpRegisters.
Currently scoping it only for Hyper-V.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

If possible, please check the style against with style(9), like function definitions. src/.clang-format may help.

sys/dev/hyperv/vmbus/aarch64/hyperv_machdep.c
75 ↗(On Diff #109537)

Sneaked debug code?

sys/dev/hyperv/vmbus/aarch64/hyperv_machdep.h
3 ↗(On Diff #109537)

"All rights reserved" is not required nowadays. Not a big deal but you can omit this.

sys/dev/hyperv/vmbus/aarch64/hyperv_machdep.c
3 ↗(On Diff #109551)

Same comment here and following files as previous - "All rights reserved." no longer necessary [unless required by your legal team/template], and add SPDX tags.

sys/dev/hyperv/vmbus/aarch64/hyperv_machdep.h
3 ↗(On Diff #109537)

And on a related note most files have SPDX tags now e.g. SPDX-License-Identifier: BSD-2-Clause

https://wiki.freebsd.org/SPDX
https://spdx.org/licenses/BSD-2-Clause.html

sys/dev/hyperv/vmbus/aarch64/hyperv_machdep.c
54 ↗(On Diff #109551)

I suggest inserting a blank line between function declarations and definitions.

59 ↗(On Diff #109551)

Small style(9) nit: "local variable declarations first, followed by one blank line, followed by the first statement."

98 ↗(On Diff #109551)

Small style(9) nit: "Values in return statements should be enclosed in parentheses."

106 ↗(On Diff #109551)

Small style(9) nit: "local variable declarations first, followed by one blank line, followed by the first statement."

sys/dev/hyperv/vmbus/aarch64/hyperv_machdep.h
56 ↗(On Diff #109551)

I suggest inserting a blank line before include guard, as the #ifndef part.

sys/dev/hyperv/vmbus/aarch64/smccc_1_2.h
27 ↗(On Diff #109551)

style(9) nit: "After any copyright and license comment, there is a blank line"

sys/dev/hyperv/vmbus/aarch64/smccc_1_2_arm64.S
27 ↗(On Diff #109551)

style(9) nit: "After any copyright and license comment, there is a blank line"

36 ↗(On Diff #109551)

Sneaked leading white space.

46 ↗(On Diff #109551)

Sneaked leading white space.

50 ↗(On Diff #109551)

Sneaked leading white space.

sys/dev/hyperv/vmbus/aarch64/hyperv_machdep.c
54 ↗(On Diff #109551)

Those declarations are already in hyperv_machdep.h. Does it need to repeat again?

sys/dev/hyperv/vmbus/aarch64/smccc_1_2_arm64.S
33 ↗(On Diff #109551)

This should be moved to sys/dev/psci/smccc_arm64.S. I've created D36297 to make it easier to add new handler functions that support both the hvc and smc interface.

Addressed SPDX license and some styling changes.

I think this should be the last style nits. As D36297 is committed, please move the SMCCC 1.2 stuff to sys/dev/psci.

sys/dev/hyperv/vmbus/aarch64/hyperv_machdep.h
1 ↗(On Diff #109819)

Sorry that I forgot this, the license header comment should use /*-.

From style(9):

An automatic script collects license information from the tree for all comments that start in the first column with “/*-”.

And so as other files.

2 ↗(On Diff #109819)

Please add a blank line after SPDX line.

sys/dev/hyperv/vmbus/aarch64/smccc_1_2.h
32 ↗(On Diff #109819)

@andrew: Should this also move to sys/dev/psci/smccc.h ?

sys/dev/hyperv/vmbus/aarch64/smccc_1_2_arm64.S
35 ↗(On Diff #109819)

The white space after the instruction is mixing in this subroutine, there are tab, single space and 4 spaces. I suggest follow other files, like:

<tab>stp<tab>x1,<space>,x19,<space>[sp, #-16]!

smccc related changes moved to original smccc files and also addressed styling comments.

schakrabarti_microsoft.com added inline comments.
sys/dev/hyperv/vmbus/aarch64/smccc_1_2_arm64.S
33 ↗(On Diff #109551)

will move it to sys/dev/psci/smccc_arm64.S. Thanks for pointing.

lwhsu added inline comments.
sys/dev/hyperv/vmbus/aarch64/hyperv_machdep.c
3 ↗(On Diff #109823)

Drop -2023 as it's not that year yet?

49 ↗(On Diff #109823)

Does HV_SMCCC_FUNC_NUMBER needs to be surrounded by parentheses?

Also note that the two \ are not aligned vertically.

58 ↗(On Diff #109823)

This is exactly the same as the HV_FUNC_ID macro.

And can it be used in the arm_smccc_hvc() call directly and remove hv_func_id local variable in the next line?

106 ↗(On Diff #109823)

This is exactly the same as the HV_FUNC_ID macro.

And can it be used in the arm_smccc_hvc() call directly and remove hv_func_id local variable in the next line?

sys/dev/psci/smccc_arm64.S
73

Why two spaces between , and [? for those 3 lines?

85

Also those 3 lines.

sys/dev/psci/smccc_arm64.S
64

Can you rebase this past R10:48a7e53db78293ba2723cecfb3a3d5d23ea8e44d & update to be a macro so we can also add smc support.

fixed the review comments for adding arm_smccc_hvc according to the new style.

sys/dev/hyperv/vmbus/aarch64/hyperv_machdep.c
59 ↗(On Diff #110330)

You can pass NULL as the result if you don't need it.

sys/dev/psci/smccc.h
115

Can you add the arm_smccc_1_2_smc definition?

sys/dev/psci/smccc_arm64.S
54–55

You can remove this comment & replace it with a newline to seperate the above from the 1.2 macro.

69

\insn #0

This revision is now accepted and ready to land.Sep 8 2022, 11:57 AM

I would suggest split into two commits, one adds new functions in psci and then uses them in hyperv, but I'm also fine for being one.

sys/dev/psci/smccc_arm64.S
85

Small nit: trailing white space.