Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <54cf3e98-49d3-81f5-58e6-ca62671ab457@intel.com>
Date:   Wed, 3 Aug 2022 07:20:52 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH v5 07/22] x86/virt/tdx: Implement SEAMCALL function
Content-Language: en-US
To:     Kai Huang <kai.huang@intel.com>, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org
Cc:     seanjc@google.com, pbonzini@redhat.com, len.brown@intel.com,
        tony.luck@intel.com, rafael.j.wysocki@intel.com,
        reinette.chatre@intel.com, dan.j.williams@intel.com,
        peterz@infradead.org, ak@linux.intel.com,
        kirill.shutemov@linux.intel.com,
        sathyanarayanan.kuppuswamy@linux.intel.com,
        isaku.yamahata@intel.com
References: <cover.1655894131.git.kai.huang@intel.com>
 <095e6bbc57b4470e1e9a9104059a5238c9775f00.1655894131.git.kai.huang@intel.com>
 <069a062e-a4a6-09af-7b74-7f4929f2ec0b@intel.com>
 <5ce7ebfe54160ea35e432bf50207ebed32db31fc.camel@intel.com>
 <84e93539-a2f9-f68e-416a-ea3d8fc725af@intel.com>
 <6bef368ccc68676e4acaecc4b6dc52f598ea7f2f.camel@intel.com>
 <ea03e55499f556388c0a5f9ed565e72e213c276f.camel@intel.com>
 <978c3d37-97c9-79b9-426a-2c27db34c38a@intel.com>
 <0b20f1878d31658a9e3cd3edaf3826fe8731346e.camel@intel.com>
 <c96a78c6a8caf25b01e450f139c934688d1735b0.camel@intel.com>
From:   Dave Hansen <dave.hansen@intel.com>
In-Reply-To: <c96a78c6a8caf25b01e450f139c934688d1735b0.camel@intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 8/2/22 19:37, Kai Huang wrote:
> On Thu, 2022-07-21 at 13:52 +1200, Kai Huang wrote:
>> Also, if I understand correctly above, your suggestion is we want to prevent any
>> CMR memory going offline so it won't be hot-removed (assuming we can get CMRs
>> during boot).  This looks contradicts to the requirement of being able to allow
>> moving memory from core-mm to driver.  When we offline the memory, we cannot
>> know whether the memory will be used by driver, or later hot-removed.
> Hi Dave,
> 
> The high level flow of device hot-removal is:
> 
> acpi_scan_hot_remove()
> 	-> acpi_scan_try_to_offline()
> 		-> acpi_bus_offline()
> 			-> device_offline()
> 				-> memory_subsys_offline()
> 	-> acpi_bus_trim()
> 		-> acpi_memory_device_remove()
> 
> 
> And memory_subsys_offline() can also be triggered via /sysfs:
> 
> 	echo 0 > /sys/devices/system/memory/memory30/online
> 
> After the memory block is offline, my understanding is kernel can theoretically
> move it to, i.e. ZONE_DEVICE via memremap_pages().
> 
> As you can see memory_subsys_offline() is the entry point of memory device
> offline (before it the code is generic for all ACPI device), and it cannot
> distinguish whether the removal is from ACPI event, or from /sysfs, so it seems
> we are unable to refuse to offline memory in  memory_subsys_offline() when it is
> called from ACPI event.
> 
> Any comments?

I suggest refactoring the code in a way that makes it possible to
distinguish the two cases.

It's not like you have some binary kernel.  You have the source code for
the whole thing and can propose changes *ANYWHERE* you need.  Even better:

$ grep -A2 ^ACPI\$ MAINTAINERS
ACPI
M:	"Rafael J. Wysocki" <rafael@kernel.org>
R:	Len Brown <lenb@kernel.org>

The maintainer of ACPI works for our employer.  Plus, he's a nice
helpful guy that you can go ask how you might refactor this or
approaches you might take.  Have you talked to Rafael about this issue?

Also, from a two-minute grepping session, I noticed this:

> static acpi_status acpi_bus_offline(acpi_handle handle, u32 lvl, void *data,
>                                     void **ret_p)
> {
...
>         if (device->handler && !device->handler->hotplug.enabled) {
>                 *ret_p = &device->dev;
>                 return AE_SUPPORT;
>         }

It looks to me like if you simply set:

	memory_device_handler->hotplug.enabled = false;

you'll get most of the behavior you want.  ACPI memory hotplug would not
work and the changes would be confined to the ACPI world.  The
"lower-level" bus-based hotplug would be unaffected.

Now, I don't know what kind of locking would be needed to muck with a
global structure like that.  But, it's a start.