Re: [Xen-devel] [PATCH V2 2/2] Xen ACPI memory hotplug

["Liu, Jinsong" <jinsong.liu@xxxxxxxxx>]

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;
>> +}
>> +

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel