Re: [PATCH v7 04/10] tools/hvmloader: Retrieve (x2)APIC IDs from the APs themselves

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 21/10/2024 4:45 pm, Alejandro Vallejo wrote:
> diff --git a/tools/firmware/hvmloader/config.h 
> b/tools/firmware/hvmloader/config.h
> index cd716bf39245..04cab1e59f08 100644
> --- a/tools/firmware/hvmloader/config.h
> +++ b/tools/firmware/hvmloader/config.h
> @@ -4,6 +4,8 @@
>  #include <stdint.h>
>  #include <stdbool.h>
>  
> +#include <xen/hvm/hvm_info_table.h>
> +
>  enum virtual_vga { VGA_none, VGA_std, VGA_cirrus, VGA_pt };
>  extern enum virtual_vga virtual_vga;
>  
> @@ -48,8 +50,9 @@ extern uint8_t ioapic_version;
>  
>  #define IOAPIC_ID           0x01
>  
> +extern uint32_t cpu_to_x2apicid[HVM_MAX_VCPUS];

Just cpu_to_apic_id[] please.   The distinction between x or x2 isn't
interesting here.

HVM_MAX_VCPUS is a constant that should never have existed in the first
place, *and* its the limit we're looking to finally break when this
series is accepted.

This array needs to be hvm_info->nr_vcpus entries long, and will want to
be more than 128 entries very soon.  Just scratch_alloc() the array. 
Then you can avoid the include.

> diff --git a/tools/firmware/hvmloader/mp_tables.c 
> b/tools/firmware/hvmloader/mp_tables.c
> index 77d3010406d0..539260365e1e 100644
> --- a/tools/firmware/hvmloader/mp_tables.c
> +++ b/tools/firmware/hvmloader/mp_tables.c
> @@ -198,8 +198,10 @@ static void fill_mp_config_table(struct mp_config_table 
> *mpct, int length)
>  /* fills in an MP processor entry for VCPU 'vcpu_id' */
>  static void fill_mp_proc_entry(struct mp_proc_entry *mppe, int vcpu_id)
>  {
> +    ASSERT(cpu_to_x2apicid[vcpu_id] < 0xFF );

This is just going to break when we hit 256 vCPUs in a VM.

What do real systems do?

They'll either wrap around 255 like the CPUID xAPIC_ID does, or they'll
not write out MP tables at all.

> diff --git a/tools/firmware/hvmloader/smp.c b/tools/firmware/hvmloader/smp.c
> index 1b940cefd071..d63536f14f00 100644
> --- a/tools/firmware/hvmloader/smp.c
> +++ b/tools/firmware/hvmloader/smp.c
> @@ -90,10 +120,11 @@ static void boot_cpu(unsigned int cpu)
>          BUG();
>  
>      /*
> -     * Wait for the secondary processor to complete initialisation.
> +     * Wait for the secondary processor to complete initialisation,
> +     * which is signaled by its x2APIC ID being written to the LUT.

Technically all arrays are a lookup table, but I'm not sure LUT is a
common enough term to be used unqualified like this.

Just say "... signalled by writing its APIC_ID out."  The where is very
apparent by the code.

> @@ -104,6 +135,12 @@ static void boot_cpu(unsigned int cpu)
>  void smp_initialise(void)
>  {
>      unsigned int i, nr_cpus = hvm_info->nr_vcpus;
> +    uint32_t ecx;
> +
> +    cpuid(1, NULL, NULL, &ecx, NULL);
> +    has_x2apic = (ecx >> 21) & 1;
> +    if ( has_x2apic )
> +        printf("x2APIC supported\n");

You need to check max_leaf >= 0xb too.  Remember Xen might not give you
leave 0xb yet, and then you'll hit the assert for finding 0.

And has_x2apic wants to be a simple boolean.  Nothing good can come from
confusing -1 with "x2apic available".


I recommend splitting this patch into three.  Several aspects are quite
subtle.

1) Collect the APIC_IDs on APs
2) Change how callin is signalled.
3) Replace LAPIC_ID() with the collected apic_id.

but AFAICT, it can be done as a standalone series, independently of the
other Xen/toolstack work.

~Andrew