Re: [RFC PATCH V2 2/4] Tool/ACPI: DSDT extension to support more vcpus

[Lan Tianyu <tianyu.lan@intel.com>]

On 2017年08月31日 23:38, Roger Pau Monné wrote:
> On Thu, Aug 31, 2017 at 01:01:47AM -0400, Lan Tianyu wrote:
>> This patch is to change DSDT table for processor object to support >128 vcpus
>> accroding to ACPI spec 8.4 Declaring Processors
>>
>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>> ---
>>  tools/libacpi/mk_dsdt.c | 18 ++++++++++++------
>>  1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/libacpi/mk_dsdt.c b/tools/libacpi/mk_dsdt.c
>> index 2daf32c..6c4c325 100644
>> --- a/tools/libacpi/mk_dsdt.c
>> +++ b/tools/libacpi/mk_dsdt.c
>> @@ -24,6 +24,8 @@
>>  #include <xen/arch-arm.h>
>>  #endif
>>  
>> +#define CPU_NAME_FMT      "P%.03X"
>> +
>>  static unsigned int indent_level;
>>  static bool debug = false;
>>  
>> @@ -196,10 +198,14 @@ int main(int argc, char **argv)
>>      /* Define processor objects and control methods. */
>>      for ( cpu = 0; cpu < max_cpus; cpu++)
>>      {
>> -        push_block("Processor", "PR%02X, %d, 0x0000b010, 0x06", cpu, cpu);
>> +        unsigned int apic_id = cpu * 2;
> 
> This is fragile, ideally there should be a single point where the APIC
> ID is calculated. Although there are already two places where the APIC
> ID is calculated, in hvmloader and libxl.
> 
> And I'm not sure how to use any of those here in order to avoid
> introducing a third one.

The mk_dsdt is independent tool to build dsdt table. It wasn't linked
with libxl and hvmloader. We can't reuse old function to do that.

But I think we may introduce a new LAPIC_ID(vcpu) in the arch head
file(i.e, #include <xen/arch-x86/xen.h>) and replace old ones.

> 
>>  
>> -        stmt("Name", "_HID, \"ACPI0007\"");
>> +        if ( apic_id > 255 )
> 
> We need to be careful with this. This is not a problem ATM because the
> ACPI ID is the CPU ID, but care should be taken to not create a
> Processor object with ACPI ID 255, because that's the broadcast ACPI
> ID...

Yes.

> 
>> +            push_block("Device", CPU_NAME_FMT, cpu);
>> +        else
> 
> ... IMHO an assert(cpu < 255); should be added here.

OK.

> 
>> +            push_block("Processor", CPU_NAME_FMT", %d, 0x0000b010, 0x06", cpu, cpu);
>                                                    ^ space (here and below)
> 
> Please leave a space between the string literals and the defines, it
> makes it easier to read. And this line needs to be split.
> 

OK. Will update.


-- 
Best regards
Tianyu Lan

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