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

[Igor Mammedov <imammedo@redhat.com>]

On Sat, 4 Mar 2023 21:10:54 +0700
Bui Quang Minh <minhquangbui99@gmail.com> wrote:

> 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.
that's what I was concerned about (i.e. just enabling x2apic without fixing
with all code that just assumes 8bit apicid).

> 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!

> 
> Thanks,
> Quang Minh.
>