enabling new hypercalls using HvCallSetVpRegisters and HvCallGetVpRegisters

[schakrabarti_microsoft.com]

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.

[lwhsu]

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.

[schakrabarti_microsoft.com]

Fixed the format and fixed comments.

[emaste]

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

[lwhsu]

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.

[lwhsu]

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?

[andrew]

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.

[schakrabarti_microsoft.com]

Addressed SPDX license and some styling changes.

[lwhsu]

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]!

[schakrabarti_microsoft.com]

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

[schakrabarti_microsoft.com]

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]

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.

[schakrabarti_microsoft.com]

addressed all the review comments.

[andrew]

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.

[schakrabarti_microsoft.com]

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

[andrew]

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

[schakrabarti_microsoft.com]

Addressed review comments.

[lwhsu]

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.