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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@xxxxxxxxx>
> ---
>  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.

>  
> -        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...

> +            push_block("Device", CPU_NAME_FMT, cpu);
> +        else

... IMHO an assert(cpu < 255); should be added here.

> +            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.

>  
> +        stmt("Name", "_HID, \"ACPI0007\"");
>          stmt("Name", "_UID, %d", cpu);
>  #ifdef CONFIG_ARM_64
>          pop_block();
> @@ -268,15 +274,15 @@ int main(int argc, char **argv)
>          /* Extract current CPU's status: 0=offline; 1=online. */
>          stmt("And", "Local1, 1, Local2");
>          /* Check if status is up-to-date in the relevant MADT LAPIC entry... 
> */
> -        push_block("If", "LNotEqual(Local2, \\_SB.PR%02X.FLG)", cpu);
> +        push_block("If", "LNotEqual(Local2, \\_SB."CPU_NAME_FMT ".FLG)", 
> cpu);
>          /* ...If not, update it and the MADT checksum, and notify OSPM. */
> -        stmt("Store", "Local2, \\_SB.PR%02X.FLG", cpu);
> +        stmt("Store", "Local2, \\_SB."CPU_NAME_FMT".FLG", cpu);
>          push_block("If", "LEqual(Local2, 1)");
> -        stmt("Notify", "PR%02X, 1", cpu); /* Notify: Device Check */
> +        stmt("Notify", CPU_NAME_FMT", 1", cpu); /* Notify: Device Check */
>          stmt("Subtract", "\\_SB.MSU, 1, \\_SB.MSU"); /* Adjust MADT csum */
>          pop_block();
>          push_block("Else", NULL);
> -        stmt("Notify", "PR%02X, 3", cpu); /* Notify: Eject Request */
> +        stmt("Notify", CPU_NAME_FMT", 3", cpu); /* Notify: Eject Request */
>          stmt("Add", "\\_SB.MSU, 1, \\_SB.MSU"); /* Adjust MADT csum */
>          pop_block();
>          pop_block();
> -- 
> 1.8.3.1
> 

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