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

[Neeraj Upadhyay <Neeraj.Upadhyay@amd.com>]

On 3/21/2025 7:22 PM, Thomas Gleixner wrote:
> 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?
> 

Guest CPU reads the APIC ID from Hypervisor's vCPU (emulated) APIC state
and updates it in the APIC backing page which is used by Secure AVIC
hardware. As Hypervisor in untrusted, guest does a check for duplicate
APIC IDs. Guest and hypervisor's APIC ID for a vCPU need to match for
IPI flow to work.

IPI flow:

1. Guest source CPU updates the IRR bit in the destination vCPU's APIC backing page.
2. Guest source vCPU takes an exit to hypervisor (icr write msr vmexit). The
   destination APIC ID in ICR data contains guest APIC ID for the destination vCPU.
3. Hypervisor uses the apic id in the icr data provided by guest, to either kick
   the corresponding destination vCPU (if not running) or write to AVIC doorbell
   to notify the running destination vCPU about the new interrupt.


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

Ok.

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

Ok.

>> +	 */
>> +	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?
> 

I was not sure if the setup() can run in parallel on 2 CPUs. So,
used an atomic list.

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

I see. I was not aware of the mismatch check in topology code. I will
remove this code.


- Neeraj

> Thanks,
> 
>         tglx