Re: [PATCH 1/4] apic: add support for x2APIC mode

[Bui Quang Minh <minhquangbui99@gmail.com>]

On 2/28/23 23:39, Igor Mammedov wrote:
> On Tue, 28 Feb 2023 21:34:33 +0700
> Bui Quang Minh <minhquangbui99@gmail.com> wrote:
> 
>> On 2/27/23 23:07, Igor Mammedov wrote:
>>> On Sat, 25 Feb 2023 17:15:17 +0700
>>> Bui Quang Minh <minhquangbui99@gmail.com> wrote:
>>>    
>>>> On 2/24/23 21:29, Igor Mammedov wrote:
>>>>> On Tue, 21 Feb 2023 23:04:57 +0700
>>>>> Bui Quang Minh <minhquangbui99@gmail.com> wrote:
>>>>>       
>>>>>> This commit refactors APIC registers read/write function to support both
>>>>>> MMIO read/write in xAPIC mode and MSR read/write in x2APIC mode. Also,
>>>>>> support larger APIC ID, self IPI, new IPI destination determination in
>>>>>> x2APIC mode.
>>>>>>
>>>>>> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
>>>>>> ---
>>>>>>     hw/intc/apic.c                  | 211 +++++++++++++++++++++++++-------
>>>>>>     hw/intc/apic_common.c           |   2 +-
>>>>>>     include/hw/i386/apic.h          |   5 +-
>>>>>>     include/hw/i386/apic_internal.h |   2 +-
>>>>>>     4 files changed, 172 insertions(+), 48 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/intc/apic.c b/hw/intc/apic.c
>>>>>> index 2d3e55f4e2..205d5923ec 100644
>>>>>> --- a/hw/intc/apic.c
>>>>>> +++ b/hw/intc/apic.c
>>>>>> @@ -30,6 +30,7 @@
>>>>>>     #include "hw/i386/apic-msidef.h"
>>>>>>     #include "qapi/error.h"
>>>>>>     #include "qom/object.h"
>>>>>> +#include "tcg/helper-tcg.h"
>>>>>>     
>>>>>>     #define MAX_APICS 255
>>>>>
>>>>> I'm curious how does it work without increasing ^^^?
>>>>
>>>> Hmm, my commit message is not entirely correct. In this series, some
>>>> operations (send IPI, IPI destination determination) have been updated
>>>> to support x2APIC mode. However, the emulated APIC still doesn't support
>>>> APIC ID larger than 255 because currently, we use a fixed length (255 +
>>>> 1) array to manage local APICs. So to support larger APIC ID, I think we
>>>> need to find any way to manage those, as the possible allocated APIC ID
>>>> range is large and maybe the allocated APIC ID is sparse which makes
>>>> fixed length array so wasteful.
>>> how much sparse it is?
>>
>> As far as I know, QEMU allows to set CPU's APIC ID, so user can pass a
>> very sparse APIC ID array.
> 
> I don't think that it does permit this (if it does it's a bug that should be fixed).
> 
> As far as I'm aware QEMU derives apic_id from '-smp' and possibly cpu type
> (there was some differences between Intel and AMD in how apic id was encoded
> notably AMD having threads or cores that lead to sparse apic id), though I don't
> remember current state of affairs in x86 cpu topo code.
> 
>>> benefits of simple static array is simplicity in management and O(1) access time.
>>> QEMU does know in advance max apic id so we can size array by dynamically
>>> allocating it when 1st apic is created. Or if IDs are too sparse
>>> switch to another structure to keep mapping.
>>
>> I totally agree with this.
>>
>> I admit that my main focus on this series is to make x2APIC mode
>> function correctly with TCG accelerator, so I skip the part of extending
>> the support for higher APIC ID.
> the tricky part in such half approach is making sure that the code is
> 'correct' and won't lead to exploits.
> It would be easier to review if it was completed solution instead of partial.

I looked around and found the way to dynamically allocate local_apics array

	void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version)
	{
		if (!kvm_irqchip_in_kernel()) {
         		apic_set_max_apic_id(x86ms->apic_id_limit);
     		}

	}

We already calculated apic_id_limit before creating CPU and local APIC 
so we can use that number to dynamically allocated local_apics.

However, there are still problems while trying to extending support to 
APIC ID larger than 255 because there are many places assume APIC ID is 
8-bit long. One of that is interrupt remapping which returns 32-bit 
destination ID but uses MSI (which has 8-bit destination) to send to 
APIC. I will look more into this.

Thanks,
Quang Minh.