RE: [PATCH V2 2/2] Xen ACPI memory hotplug

Liu, Jinsong
2013-01-14T06:00:55Z

Agree, w/ minor comments below.

Konrad Rzeszutek Wilk wrote:
>> +
>> + /*
>> + * Early boot code has recognized memory area by EFI/E820.
>> + * If DSDT shows these memory devices on boot, hotplug is not
>> necessary + * for them. So, it just returns until completion of
>> this driver's + * start up.
>
> "So it just returns until completion of this drivers's start up."
>
> Can you change that to be:
> "Return OK until this driver starts up."
>
> But then.. how can this function be called with
> acpi_hotmem_initialized=false? Is it b/c of the acpi_walk_namespace
> call? How about you state that:
>
> "This can be done via the acpi_walk_namespace which is called during
> early boot and acpi_hotmem_initialized is set _after_ that call
> has completed."
>

The action for booting existed memory and hot-added memory is different.
For booting existed memory devices, we don't need do xen_hotadd_memory.
For hot-added memory devices, we need do xen_hotadd_memory, hypercall to hypervisor to add memory.

I've updated comments as below to be more clear:
/*
* For booting existed memory devices, early boot code has recognized
* memory area by EFI/E820. If DSDT shows these memory devices on boot,
* hotplug is not necessary for them.
* For hot-added memory devices during runtime, it need hypercall to
* Xen hypervisor to add memory.
*/

>> +static void __exit xen_acpi_memory_device_exit(void) +{
>> + acpi_status status;
>> +
>> + if (!xen_initial_domain())
>> + return;
>> +
>> + status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
>> + ACPI_UINT32_MAX, +
>> acpi_memory_deregister_notify_handler, + NULL, NULL, NULL);
>> + if (ACPI_FAILURE(status))
>> + pr_warn(PREFIX "walk_namespace failed\n");
>> +
>> + acpi_bus_unregister_driver(&xen_acpi_memory_device_driver); +
>> + /*
>> + * stub reserve space again to prevent any chance of native
>> + * driver loading, though not much meaning in real life
>
> not much meaning in real life? What does that mean?
>

Hmm, what I want to say here is, in real life module driver seldomly unloaded.
Anyway, the comments is ambiguous, I've dropped the word 'though not much meaning in real life'.

Thanks,
Jinsong

>> + */
>> + acpi_bus_register_driver(&xen_stub_memory_device_driver); + return;
>> +}
>> +