Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 11 Nov 2020 13:03:21 +0000
From:   David Brazdil <dbrazdil@google.com>
To:     Marc Zyngier <maz@kernel.org>
Cc:     kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, James Morse <james.morse@arm.com>,
        Julien Thierry <julien.thierry.kdev@gmail.com>,
        Suzuki K Poulose <suzuki.poulose@arm.com>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>, Dennis Zhou <dennis@kernel.org>,
        Tejun Heo <tj@kernel.org>, Christoph Lameter <cl@linux.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Quentin Perret <qperret@google.com>,
        Andrew Scull <ascull@google.com>,
        Andrew Walbran <qwandor@google.com>, kernel-team@android.com
Subject: Re: [PATCH v1 07/24] kvm: arm64: Create nVHE copy of cpu_logical_map
Message-ID: <20201111130321.qalrzfabdonrwvsz@google.com>
References: <20201109113233.9012-1-dbrazdil@google.com>
 <20201109113233.9012-8-dbrazdil@google.com>
 <d473fd26e5314f2407b70242488f33de@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <d473fd26e5314f2407b70242488f33de@kernel.org>
Precedence: bulk

> > +/*
> > + * nVHE copy of data structures tracking available CPU cores.
> > + * Only entries for CPUs that were online at KVM init are populated.
> > + * Other CPUs should not be allowed to boot because their features were
> > + * not checked against the finalized system capabilities.
> > + */
> > +u64 __ro_after_init __cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1]
> > = INVALID_HWID };
> 
> I'm not sure what __ro_after_init means once we get S2 isolation.

It is stretching the definition of 'init' a bit, I know, but I don't see what
your worry is about S2? The intention is to mark this read-only for .hyp.text
at runtime. With S2, the host won't be able to write to it after KVM init.
Obviously that's currently not the case.

One thing we might change in the future is marking it RW for some initial
setup in a HVC handler, then marking it RO for the rest of uptime.

> 
> > +
> > +u64 cpu_logical_map(int cpu)
> 
> nit: is there any reason why "cpu" cannot be unsigned? The thought
> of a negative CPU number makes me shiver...

Same here. That's how it's defined in kernel proper, so I went with that.

> 
> > +{
> > +	if (cpu < 0 || cpu >= ARRAY_SIZE(__cpu_logical_map))
> > +		hyp_panic();
> > +
> > +	return __cpu_logical_map[cpu];
> > +}
> > +
> >  unsigned long __hyp_per_cpu_offset(unsigned int cpu)
> >  {
> >  	unsigned long *cpu_base_array;
> 
> Overall, this patch would make more sense closer it its use case
> (in patch 19). I also don't understand why this lives in percpu.c...

I didn't think it called for adding another C file for this. How about we
rename this file to smp.c? Would that make sense for both?