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
