On Sat, 4 Mar 2023 21:10:54 +0700
Bui Quang Minh <minhquangbu...@gmail.com> wrote:
> On 2/28/23 23:39, Igor Mammedov wrote:
> > On Tue, 28 Feb 2023 21:34:33 +0700
> > Bui Quang Minh <minhquangbu...@gmail.com> wrote:
> >
> >> On 2/27/23 23:07, Igor Mammedov wrote:
> >>> On Sat, 25 Feb 2023 17:15:17 +0700
> >>> Bui Quang Minh <minhquangbu...@gmail.com> wrote:
> >>>
> >>>> On 2/24/23 21:29, Igor Mammedov wrote:
> >>>>> On Tue, 21 Feb 2023 23:04:57 +0700
> >>>>> Bui Quang Minh <minhquangbu...@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 <minhquangbu...@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.
>
