| 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
| ||
|
Date: Wed, 15 May 2024 11:11:33 -0700
From: Roman Kisel <romank@...ux.microsoft.com>
To: Michael Kelley <mhklinux@...look.com>, "arnd@...db.de" <arnd@...db.de>,
"bhelgaas@...gle.com" <bhelgaas@...gle.com>, "bp@...en8.de" <bp@...en8.de>,
"catalin.marinas@....com" <catalin.marinas@....com>,
"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>, "kw@...ux.com" <kw@...ux.com>,
"kys@...rosoft.com" <kys@...rosoft.com>, "lenb@...nel.org"
<lenb@...nel.org>, "lpieralisi@...nel.org" <lpieralisi@...nel.org>,
"mingo@...hat.com" <mingo@...hat.com>, "rafael@...nel.org"
<rafael@...nel.org>, "robh@...nel.org" <robh@...nel.org>,
"tglx@...utronix.de" <tglx@...utronix.de>,
"wei.liu@...nel.org" <wei.liu@...nel.org>, "will@...nel.org"
<will@...nel.org>, "linux-acpi@...r.kernel.org"
<linux-acpi@...r.kernel.org>,
"linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
"x86@...nel.org" <x86@...nel.org>
Cc: "ssengar@...rosoft.com" <ssengar@...rosoft.com>,
"sunilmut@...rosoft.com" <sunilmut@...rosoft.com>,
"vdso@...bites.dev" <vdso@...bites.dev>
Subject: Re: [PATCH v2 3/6] drivers/hv: arch-neutral implementation of
get_vtl()
On 5/15/2024 6:38 AM, Michael Kelley wrote:
> From: Roman Kisel <romank@...ux.microsoft.com> Sent: Tuesday, May 14, 2024 3:44 PM
>>
>> To run in the VTL mode, Hyper-V drivers have to know what
>> VTL the system boots in, and the arm64/hyperv code does not
>> have the means to compute that.
>>
>> Refactor the code to hoist the function that detects VTL,
>> make it arch-neutral to be able to employ it to get the VTL
>> on arm64.
>
> Nit: In patches that just refactor and move some code around,
> patch authors will often include a statement like "No functional
> changes" (or the slightly more doubtful "No functional changes
> intended") as a separate line at the end of the commit message.
> It's just something the reader/reviewer can cross-check against.
>
>>
>> Signed-off-by: Roman Kisel <romank@...ux.microsoft.com>
>> ---
>> arch/x86/hyperv/hv_init.c | 34 -----------------------
>> arch/x86/include/asm/hyperv-tlfs.h | 7 -----
>> drivers/hv/hv_common.c | 43 ++++++++++++++++++++++++++++++
>> include/asm-generic/hyperv-tlfs.h | 7 +++++
>> include/asm-generic/mshyperv.h | 6 +++++
>> 5 files changed, 56 insertions(+), 41 deletions(-)
>>
>> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
>> index 17a71e92a343..c350fa05ee59 100644
>> --- a/arch/x86/hyperv/hv_init.c
>> +++ b/arch/x86/hyperv/hv_init.c
>> @@ -413,40 +413,6 @@ static void __init hv_get_partition_id(void)
>> local_irq_restore(flags);
>> }
>>
>> -#if IS_ENABLED(CONFIG_HYPERV_VTL_MODE)
>> -static u8 __init get_vtl(void)
>> -{
>> - u64 control = HV_HYPERCALL_REP_COMP_1 | HVCALL_GET_VP_REGISTERS;
>> - struct hv_get_vp_registers_input *input;
>> - struct hv_get_vp_registers_output *output;
>> - unsigned long flags;
>> - u64 ret;
>> -
>> - local_irq_save(flags);
>> - input = *this_cpu_ptr(hyperv_pcpu_input_arg);
>> - output = (struct hv_get_vp_registers_output *)input;
>> -
>> - memset(input, 0, struct_size(input, element, 1));
>> - input->header.partitionid = HV_PARTITION_ID_SELF;
>> - input->header.vpindex = HV_VP_INDEX_SELF;
>> - input->header.inputvtl = 0;
>> - input->element[0].name0 = HV_X64_REGISTER_VSM_VP_STATUS;
>> -
>> - ret = hv_do_hypercall(control, input, output);
>> - if (hv_result_success(ret)) {
>> - ret = output->as64.low & HV_X64_VTL_MASK;
>> - } else {
>> - pr_err("Failed to get VTL(error: %lld) exiting...\n", ret);
>> - BUG();
>> - }
>> -
>> - local_irq_restore(flags);
>> - return ret;
>> -}
>> -#else
>> -static inline u8 get_vtl(void) { return 0; }
>> -#endif
>> -
>> /*
>> * This function is to be invoked early in the boot sequence after the
>> * hypervisor has been detected.
>> diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
>> index 3787d26810c1..9ee68eb8e6ff 100644
>> --- a/arch/x86/include/asm/hyperv-tlfs.h
>> +++ b/arch/x86/include/asm/hyperv-tlfs.h
>> @@ -309,13 +309,6 @@ enum hv_isolation_type {
>> #define HV_MSR_STIMER0_CONFIG (HV_X64_MSR_STIMER0_CONFIG)
>> #define HV_MSR_STIMER0_COUNT (HV_X64_MSR_STIMER0_COUNT)
>>
>> -/*
>> - * Registers are only accessible via HVCALL_GET_VP_REGISTERS hvcall and
>> - * there is not associated MSR address.
>> - */
>> -#define HV_X64_REGISTER_VSM_VP_STATUS 0x000D0003
>> -#define HV_X64_VTL_MASK GENMASK(3, 0)
>> -
>> /* Hyper-V memory host visibility */
>> enum hv_mem_host_visibility {
>> VMBUS_PAGE_NOT_VISIBLE = 0,
>> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
>> index dde3f9b6871a..d4cf477a4d0c 100644
>> --- a/drivers/hv/hv_common.c
>> +++ b/drivers/hv/hv_common.c
>> @@ -660,3 +660,46 @@ u64 __weak hv_tdx_hypercall(u64 control, u64 param1, u64
>> param2)
>> return HV_STATUS_INVALID_PARAMETER;
>> }
>> EXPORT_SYMBOL_GPL(hv_tdx_hypercall);
>> +
>> +#if IS_ENABLED(CONFIG_HYPERV_VTL_MODE)
>> +u8 __init get_vtl(void)
>> +{
>> + u64 control = HV_HYPERCALL_REP_COMP_1 | HVCALL_GET_VP_REGISTERS;
>> + struct hv_get_vp_registers_input *input;
>> + struct hv_get_vp_registers_output *output;
>> + unsigned long flags;
>> + u64 ret;
>> +
>> + local_irq_save(flags);
>> + input = *this_cpu_ptr(hyperv_pcpu_input_arg);
>> + output = (struct hv_get_vp_registers_output *)input;
>
> I gave some bad advice on the original version of the above code. Rather
> than allocating a separate hypercall output area, I had suggested just setting
> the hypercall output to be the same as the input area, which is done
> above. While the overlap doesn't cause any problems for a hypercall returning
> the value of single register, the TLFS says to never set up such an overlap.
>
> Since this code is being moved anyway, there's an opportunity to fix
> this by setting output to "input" + "struct_size(input, element, 1)". Or
> put the output at the start of the second half of the page (note that the
> size is HY_HYP_PAGE_SIZE, not PAGE_SIZE). The input and output for 1
> register are relatively small and both easily fit with either approach. They
> just shouldn't overlap.
>
> Some might argue this tweak should be made in a separate patch, but
> in my judgment, it's OK to do it here if you note it in the commit
> message. Your call. Of course, if you make this change, my previous
> comment about "No functional changes" no longer applies. :-)
I'll update the code to adhere to the TLFS requirements, thank you :)
>
> Michael
>
>> +
>> + memset(input, 0, struct_size(input, element, 1));
>> + input->header.partitionid = HV_PARTITION_ID_SELF;
>> + input->header.vpindex = HV_VP_INDEX_SELF;
>> + input->header.inputvtl = 0;
>> + input->element[0].name0 = HV_REGISTER_VSM_VP_STATUS;
>> +
>> + ret = hv_do_hypercall(control, input, output);
>> + if (hv_result_success(ret)) {
>> + ret = output->as64.low & HV_VTL_MASK;
>> + } else {
>> + pr_err("Failed to get VTL(error: %lld) exiting...\n", ret);
>> +
>> + /*
>> + * This is a dead end, something fundamental is broken.
>> + *
>> + * There is no sensible way of continuing as the Hyper-V drivers
>> + * transitively depend via the vmbus driver on knowing which VTL
>> + * they run in to establish communication with the host. The kernel
>> + * is going to be worse off if continued booting than a panicked one,
>> + * just hung and stuck, producing second-order failures, with neither
>> + * a way to recover nor to provide expected services.
>> + */
>> + BUG();
>> + }
>> +
>> + local_irq_restore(flags);
>> + return ret;
>> +}
>> +#endif
>> diff --git a/include/asm-generic/hyperv-tlfs.h b/include/asm-generic/hyperv-tlfs.h
>> index 87e3d49a4e29..682bcda3124f 100644
>> --- a/include/asm-generic/hyperv-tlfs.h
>> +++ b/include/asm-generic/hyperv-tlfs.h
>> @@ -75,6 +75,13 @@
>> /* AccessTscInvariantControls privilege */
>> #define HV_ACCESS_TSC_INVARIANT BIT(15)
>>
>> +/*
>> + * This synthetic register is only accessible via the HVCALL_GET_VP_REGISTERS
>> + * hvcall, and there is no an associated MSR on x86.
>> + */
>> +#define HV_REGISTER_VSM_VP_STATUS 0x000D0003
>> +#define HV_VTL_MASK GENMASK(3, 0)
>> +
>> /*
>> * Group B features.
>> */
>> diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
>> index 99935779682d..ea434186d765 100644
>> --- a/include/asm-generic/mshyperv.h
>> +++ b/include/asm-generic/mshyperv.h
>> @@ -301,4 +301,10 @@ static inline enum hv_isolation_type
>> hv_get_isolation_type(void)
>> }
>> #endif /* CONFIG_HYPERV */
>>
>> +#if IS_ENABLED(CONFIG_HYPERV_VTL_MODE)
>> +u8 __init get_vtl(void);
>> +#else
>> +static inline u8 get_vtl(void) { return 0; }
>> +#endif
>> +
>> #endif
>> --
>> 2.45.0
>>
--
Thank you,
Roman
Powered by blists - more mailing lists