Re: [PATCHv2 02/13] kernel/cpu: Add support for declaring CPU offlining not supported

[Huang, Kai]

On Mon, 2023-10-23 at 18:31 +0300, kirill.shutemov@xxxxxxxxxxxxxxx wrote:
> On Mon, Oct 23, 2023 at 09:30:59AM +0000, Huang, Kai wrote:
> > IMHO it's a little bit odd to have two mechanisms in place, even in this middle
> > state patch.  Is it better to completely replace CC_ATTR_HOTPLUG_DISABLED with
> > the new cpu_hotplug_offline_disabled in this patch? You can explicitly call
> > cpu_hotplug_disable_offlining() in tdx_early_init() so no functional change is
> > done.
> 
> I can. But I don't see how it makes a difference.

Personally I think this is better because it is odd to have two mechanisms in
place even temporarily especially when we can avoid it.  But no hard opinion. 
Up to you.

> 
> > Or I am wondering why cannot just merge this and the next patch together, with a
> > proper justification?
> 
> Because the very next thing reviewers would ask is to split them :P
> 
> > Btw, IMHO the changelog (this and next patch's) seems didn't explain the true
> > reason to replace CC_ATTR_HOTPLUG_DISABLED.
> > 
> > 	Currently hotplug prevented based on the confidential computing
> > 	attribute which is set for Intel TDX. But TDX is not the only possible
> > 	user of the wake up method.
> > 
> > "TDX is not the only possible user of the wake up method" doesn't mean we need
> > to replace CC_ATTR_HOTPLUG_DISABLED.  E.g., other CoCo VM type can also select
> > CC_ATTR_HOTPLUG_DISABLED if it uses MADT wake up method.
> > 
> > To me the true reason is the new MADT wake up version actually brings the
> > support of offlining cpu, thus it's more suitable to decide whether the CoCo VM
> > needs to disable CPU offline based on the MADT wake up version, but not the CC_*
> > attributes that is determined by CoCo VM type.
> 
> No. MADT is orthogonal to CoCo. It can be implemented outside of CoCo
> environment and CoCo platform can implement other wake up methods. It is
> not up to TDX/SEV/whatever to decide if offlining is supported. It is
> property of the wakeup method implemented on the platform.
> 

Yeah sure.  Can we put this to changelog to make it clearer? :-)