| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<SN6PR02MB4157093D1661A64B9677AB6FD49DA@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Sun, 18 May 2025 21:15:59 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Roman Kisel <romank@...ux.microsoft.com>, "arnd@...db.de" <arnd@...db.de>,
"bp@...en8.de" <bp@...en8.de>, "catalin.marinas@....com"
<catalin.marinas@....com>, "corbet@....net" <corbet@....net>,
"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
"decui@...rosoft.com" <decui@...rosoft.com>, "haiyangz@...rosoft.com"
<haiyangz@...rosoft.com>, "hpa@...or.com" <hpa@...or.com>,
"kys@...rosoft.com" <kys@...rosoft.com>, "mingo@...hat.com"
<mingo@...hat.com>, "tglx@...utronix.de" <tglx@...utronix.de>,
"wei.liu@...nel.org" <wei.liu@...nel.org>, "will@...nel.org"
<will@...nel.org>, "x86@...nel.org" <x86@...nel.org>,
"linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>, "linux-arch@...r.kernel.org"
<linux-arch@...r.kernel.org>
CC: "apais@...rosoft.com" <apais@...rosoft.com>, "benhill@...rosoft.com"
<benhill@...rosoft.com>, "bperkins@...rosoft.com" <bperkins@...rosoft.com>,
"sunilmut@...rosoft.com" <sunilmut@...rosoft.com>
Subject: RE: [PATCH hyperv-next v2 3/4] arch: hyperv: Get/set SynIC
synth.registers via paravisor
From: Roman Kisel <romank@...ux.microsoft.com> Sent: Sunday, May 11, 2025 4:08 PM
>
> The confidential VMBus is built on the guest talking to the
> paravisor only.
>
> Provide functions that allow manipulating the SynIC registers
> via paravisor.
>
> Signed-off-by: Roman Kisel <romank@...ux.microsoft.com>
> ---
> arch/arm64/hyperv/mshyperv.c | 19 +++++++++++++++++++
> arch/arm64/include/asm/mshyperv.h | 3 +++
> arch/x86/include/asm/mshyperv.h | 3 +++
> arch/x86/kernel/cpu/mshyperv.c | 28 ++++++++++++++++++++++++++++
> 4 files changed, 53 insertions(+)
>
> diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
> index 4fdc26ade1d7..8778b6831062 100644
> --- a/arch/arm64/hyperv/mshyperv.c
> +++ b/arch/arm64/hyperv/mshyperv.c
> @@ -134,3 +134,22 @@ bool hv_is_hyperv_initialized(void)
> return hyperv_initialized;
> }
> EXPORT_SYMBOL_GPL(hv_is_hyperv_initialized);
> +
> +/*
> + * Not supported yet.
> + */
> +u64 hv_pv_get_synic_register(unsigned int reg, int *err)
> +{
> + *err = -ENODEV;
> + return !0ULL;
> +}
> +EXPORT_SYMBOL_GPL(hv_pv_get_synic_register);
> +
> +/*
> + * Not supported yet.
> + */
> +int hv_pv_set_synic_register(unsigned int reg, u64 val)
> +{
> + return -ENODEV;
> +}
> +EXPORT_SYMBOL_GPL(hv_pv_set_synic_register);
Since we introduced support for arm64 a few years back, we've generally
been putting arch-neutral stubs as __weak functions in hv_common.c.
The x86 implementation overrides the weak functions, and for arm64
the __weak functions *are* the stubs. As the comment says in
hv_common.c, this approach avoids cluttering arm64 with a bunch of stub
functions. Of course, when/if the arm64 side is implemented, the __weak
stub may be considered superfluous, depending on what kind of bet
you want to make on a 3rd architecture showing up in the future. :-)
But regardless, keeping the stubs in hv_common.c simplifies the process
by avoiding the need for arm64 maintainer involvement in approving
content-free stubs.
Separately, the use of "pv" in the naming may be problematic. I
associate "pv" with para-virtualization, and the pv_ops mechanism,
which is decidedly different from the paravisor concept. What about
using "para" instead of "pv" in these names, and other places in this
patch set? I see that KVM has some use of "para" and I'm not sure
what that is in the KVM context. But it's not nearly as pervasive as
"pv" is. Or maybe "pvr" is a better choice here.
> diff --git a/arch/arm64/include/asm/mshyperv.h
> b/arch/arm64/include/asm/mshyperv.h
> index b721d3134ab6..bce37a58dff0 100644
> --- a/arch/arm64/include/asm/mshyperv.h
> +++ b/arch/arm64/include/asm/mshyperv.h
> @@ -53,6 +53,9 @@ static inline u64 hv_get_non_nested_msr(unsigned int reg)
> return hv_get_msr(reg);
> }
>
> +u64 hv_pv_get_synic_register(unsigned int reg, int *err);
> +int hv_pv_set_synic_register(unsigned int reg, u64 val);
> +
> /* SMCCC hypercall parameters */
> #define HV_SMCCC_FUNC_NUMBER 1
> #define HV_FUNC_ID ARM_SMCCC_CALL_VAL( \
> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> index bab5ccfc60a7..0a4b01c1f094 100644
> --- a/arch/x86/include/asm/mshyperv.h
> +++ b/arch/x86/include/asm/mshyperv.h
> @@ -307,6 +307,9 @@ static __always_inline u64 hv_raw_get_msr(unsigned int reg)
> return __rdmsr(reg);
> }
>
> +u64 hv_pv_get_synic_register(unsigned int reg, int *err);
> +int hv_pv_set_synic_register(unsigned int reg, u64 val);
> +
> #else /* CONFIG_HYPERV */
> static inline void hyperv_init(void) {}
> static inline void hyperv_setup_mmu_ops(void) {}
> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> index 3e2533954675..4f6e3d02f730 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -89,6 +89,34 @@ void hv_set_non_nested_msr(unsigned int reg, u64 value)
> }
> EXPORT_SYMBOL_GPL(hv_set_non_nested_msr);
>
> +/*
> + * Not every paravisor supports getting SynIC registers, and
> + * this function may fail. The caller has to make sure that this function
> + * runs on the CPU of interest.
This last sentence confused me a bit, as I wasn't sure what "CPU of
interest" meant. Alok Tiwari's comments suggest "target CPU",
which to me is still imprecise. The point is that a SynIC is a
per-CPU resource, and it can only be accessed from the CPU to
which it belongs. Maybe restate as, "The register for the SynIC
of the running CPU is accessed."
> + */
> +u64 hv_pv_get_synic_register(unsigned int reg, int *err)
The function signature here seems a bit non-standard. I would
have expected the return value to indicate success or an error
code, with the location of the return register value being an
argument. Then it is more parallel to the corresponding
"set" function below.
> +{
> + if (!hv_is_synic_msr(reg)) {
> + *err = -ENODEV;
> + return !0ULL;
> + }
> + return native_read_msr_safe(reg, err);
> +}
> +EXPORT_SYMBOL_GPL(hv_pv_get_synic_register);
> +
> +/*
> + * Not every paravisor supports setting SynIC registers, and
> + * this function may fail. The caller has to make sure that this function
> + * runs on the CPU of interest.
Same confusion here with the last sentence.
> + */
> +int hv_pv_set_synic_register(unsigned int reg, u64 val)
> +{
> + if (!hv_is_synic_msr(reg))
> + return -ENODEV;
> + return wrmsrl_safe(reg, val);
> +}
> +EXPORT_SYMBOL_GPL(hv_pv_set_synic_register);
> +
> u64 hv_get_msr(unsigned int reg)
> {
> if (hv_nested)
> --
> 2.43.0
>
Powered by blists - more mailing lists