Re: [RFC v2 04/17] x86/apic: Initialize APIC ID for Secure AVIC

[Thomas Gleixner <tglx@linutronix.de>]

On Wed, Feb 26 2025 at 14:35, Neeraj Upadhyay wrote:
> Initialize the APIC ID in the Secure AVIC APIC backing page with
> the APIC_ID msr value read from Hypervisor. Maintain a hashmap to
> check and report same APIC_ID value returned by Hypervisor for two
> different vCPUs.

What for?
 
> +struct apic_id_node {
> +	 struct llist_node node;
> +	 u32 apic_id;
> +	 int cpu;
> +};

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers

and please read the rest of the document too.

> +static void init_backing_page(void *backing_page)
> +{
> +	struct apic_id_node *next_node, *this_cpu_node;
> +	unsigned int apic_map_slot;
> +	u32 apic_id;
> +	int cpu;
> +
> +	/*
> +	 * Before Secure AVIC is enabled, APIC msr reads are
> +	 * intercepted. APIC_ID msr read returns the value
> +	 * from hv.

Can you please write things out? i.e. s/hv/hypervisor/ This is not twatter.

> +	 */
> +	apic_id = native_apic_msr_read(APIC_ID);
> +	set_reg(backing_page, APIC_ID, apic_id);
> +
> +	if (!apic_id_map)
> +		return;
> +
> +	cpu = smp_processor_id();
> +	this_cpu_node = &per_cpu(apic_id_node, cpu);
> +	this_cpu_node->apic_id = apic_id;
> +	this_cpu_node->cpu = cpu;
> +	/*
> +	 * In common case, apic_ids for CPUs are sequentially numbered.
> +	 * So, each CPU should hash to a different slot in the apic id
> +	 * map.
> +	 */
> +	apic_map_slot = apic_id % nr_cpu_ids;
> +	llist_add(&this_cpu_node->node, &apic_id_map[apic_map_slot]);

Why does this need to be a llist? What's wrong about a trivial hlist?

> +	/* Each CPU checks only its next nodes for duplicates. */
> +	llist_for_each_entry(next_node, this_cpu_node->node.next, node) {
> +		if (WARN_ONCE(next_node->apic_id == apic_id,
> +			      "Duplicate APIC %u for cpu %d and cpu %d. IPI handling will suffer!",
> +			      apic_id, cpu, next_node->cpu))
> +			break;
> +	}

This does not make any sense at all. The warning is completely useless
because two milliseconds later the topology evaluation code will yell
about mismatch of APIC IDs and catch the duplicate.

So what is this overengineered thing buying you? Just more
incomprehensible security voodoo for no value.

Thanks,

        tglx