linux-kernel - RE: [PATCH v8 4/6] arm64: hyperv: Initialize hypervisor on boot

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <MWHPR21MB1593960D8B1FFCEC510CD991D7969@MWHPR21MB1593.namprd21.prod.outlook.com>
Date:   Fri, 5 Mar 2021 21:03:24 +0000
From:   Michael Kelley <mikelley@...rosoft.com>
To:     Sunil Muthuswamy <sunilmut@...rosoft.com>
CC:     "will@...nel.org" <will@...nel.org>,
        "catalin.marinas@....com" <catalin.marinas@....com>,
        Mark Rutland <Mark.Rutland@....com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "linux-efi@...r.kernel.org" <linux-efi@...r.kernel.org>,
        "arnd@...db.de" <arnd@...db.de>,
        "wei.liu@...nel.org" <wei.liu@...nel.org>,
        "ardb@...nel.org" <ardb@...nel.org>,
        "daniel.lezcano@...aro.org" <daniel.lezcano@...aro.org>,
        KY Srinivasan <kys@...rosoft.com>
Subject: RE: [PATCH v8 4/6] arm64: hyperv: Initialize hypervisor on boot

From: Sunil Muthuswamy <sunilmut@...rosoft.com> Sent: Wednesday, March 3, 2021 12:21 PM
> 
> > +static u64 hypercall_output __initdata;
> > +
> > +static int __init hyperv_init(void)
> > +{
> > +	struct hv_get_vpindex_from_apicid_input *input;
> > +	u64	status;
> > +	int	i;
> nit: both, tabs & spaces are being used to indent variable names. Can we stick
> to one?
> 

Agreed.  Will make this consistent in the next version.

> > +
> > +	/*
> > +	 * Hypercall inputs must not cross a page boundary, so allocate
> > +	 * power of 2 size, which will be aligned to that size.
> > +	 */
> > +	input = kzalloc(roundup_pow_of_two(sizeof(input->header) +
> > +				sizeof(input->element[0])), GFP_KERNEL);
> > +	if (!input)
> > +		return -ENOMEM;
> Similar to the comment on the other patch, can this be done through a
> per-cpu hypercall input page?

Per comments on other previous patch, these allocations can be eliminated
and the problem avoided entirely.

> 
> > +		if ((status & HV_HYPERCALL_RESULT_MASK) == HV_STATUS_SUCCESS)
> > +			hv_vp_index[i] = hypercall_output;
> > +		else {
> CNF suggests using braces even for single line 'if' statements if the 'else' is a multi-line
> statement

Will do.

> 
> > +			pr_warn("Hyper-V: No VP index for CPU %d MPIDR %llx status
> %llx\n",
> > +				i, cpu_logical_map(i), status);
> > +			hv_vp_index[i] = VP_INVAL;
> > +		}
> 
> > +bool hv_is_hyperv_initialized(void)
> > +{
> > +	return hyperv_initialized;
> > +}
> > +EXPORT_SYMBOL_GPL(hv_is_hyperv_initialized);
> 
> This to me seems like more of an indication that whether the hypervisor is Hyper-V or
> something else, rather than if necessary Hyper-V pieces have been initialized. If that is
> the case, suggest renaming. Maybe just call it 'hv_is_hyperv'?

This function also exists on the x86/x64 side and is used in architecture independent
code.  It's meaning is somewhere between "is Hyper-V" and "is Hyper-V initialized".
I'm not sure there's much value in changing the name everywhere.

Michael