Re: [PATCH v6 05/11] tools/hvmloader: Retrieve (x2)APIC IDs from the APs themselves

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.10.2024 19:19, Alejandro Vallejo wrote:
> On Wed Oct 9, 2024 at 3:03 PM BST, Jan Beulich wrote:
>> On 01.10.2024 14:38, Alejandro Vallejo wrote:
>>> Make it so the APs expose their own APIC IDs in a LUT. We can use that
>>> LUT to populate the MADT, decoupling the algorithm that relates CPU IDs
>>> and APIC IDs from hvmloader.
>>>
>>> While at this also remove ap_callin, as writing the APIC ID may serve
>>> the same purpose.
>>
>> ... on the assumption that no AP will have an APIC ID of zero.
>>
>>> @@ -341,11 +341,11 @@ int main(void)
>>>  
>>>      printf("CPU speed is %u MHz\n", get_cpu_mhz());
>>>  
>>> +    smp_initialise();
>>> +
>>>      apic_setup();
>>>      pci_setup();
>>>  
>>> -    smp_initialise();
>>
>> I can see that you may want cpu_setup(0) to run ahead of apic_setup().
> 
> Not only that. This hunk ensures CPU_TO_X2APICID is populated ASAP for every
> CPU. Reading zeroes where a non-zero APIC ID should be is fatal and tricky to
> debug later. I tripped on enough "used the LUT before being set up" bugs to
> really prefer initialising it before anyone has a chance to misuse it.
> 
>> Yet is it really appropriate to run boot_cpu() ahead of apic_setup() as well?
> 
> I would've agreed before the patches that went in to replace INIT-SIPI-SIPI
> with hypercalls, but now that hvmloader is enlightened it has no real need for
> the APIC to be configured. If feels weird because you wouldn't use this order
> on bare metal. But it's fine under virt.
> 
>> At the very least it feels logically wrong, even if at the moment there
>> may not be any direct dependency (which might change, however, down the
>> road).
> 
> I suspect it feels wrong because you can't boot CPUs ahead of configuring your
> APIC in real hardware. But hvmloader is always virtualized so that point is
> moot. If anything, I'd be scared of adding code ahead of smp_initialise() that
> relies on CPU_TO_X2APICID being set when it hasn't yet.
> 
> If you have a strong view on the matter I can remove this hunk and call
> read_apic_id() from apic_setup(). But it wouldn't be my preference to do so.

No, with the explanations above it's fine to stay as is. Just that some of
what you say above needs to move into the patch description.

Jan