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

[Bui Quang Minh]

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.