Eduardo Habkost <ehabk...@redhat.com> writes: > On Wed, Jul 13, 2016 at 06:47:20PM -0400, Bandan Das wrote: >> Igor Mammedov <imamm...@redhat.com> writes: >> >> > MAX_APICS is only used by child 'apic' class and not >> > by its parent TYPE_APIC_COMMON or any other derived >> > class. >> > Move check into end user 'apic' class so it won't >> > get in the way of other APIC implementations >> > if they support more then MAX_APICS. >> > >> > Signed-off-by: Igor Mammedov <imamm...@redhat.com> >> > --- >> > hw/intc/apic.c | 10 ++++++++++ >> > hw/intc/apic_common.c | 8 -------- >> > include/hw/i386/apic_internal.h | 4 +--- >> > 3 files changed, 11 insertions(+), 11 deletions(-) >> > >> > diff --git a/hw/intc/apic.c b/hw/intc/apic.c >> > index e1ab935..b0d237b 100644 >> > --- a/hw/intc/apic.c >> > +++ b/hw/intc/apic.c >> > @@ -28,7 +28,9 @@ >> > #include "trace.h" >> > #include "hw/i386/pc.h" >> > #include "hw/i386/apic-msidef.h" >> > +#include "qapi/error.h" >> > >> > +#define MAX_APICS 255 >> > #define MAX_APIC_WORDS 8 >> > >> > #define SYNC_FROM_VAPIC 0x1 >> > @@ -869,6 +871,14 @@ static const MemoryRegionOps apic_io_ops = { >> > static void apic_realize(DeviceState *dev, Error **errp) >> > { >> > APICCommonState *s = APIC_COMMON(dev); >> > + static int apic_no; >> >> Can this be a global ? I understand there are no other users but >> maybe it fits alongside local_apics. > > The variable is removed by the next patch. :) > >> >> > + if (apic_no >= MAX_APICS) { >> > + error_setg(errp, "%s initialization failed.", >> > + object_get_typename(OBJECT(dev))); >> > + return; >> > + } >> > + s->idx = apic_no++; >> >> Is there a possibility of a race here with the apic removal path ? > > If there's that possibility: 1) it was already in > apic_common_realize(); 2) it is removed by patch 13/19.
Ugh! Stupid me :) Yep, thanks for pointing that out. >> >> Bandan >> >> > memory_region_init_io(&s->io_memory, OBJECT(s), &apic_io_ops, s, >> > "apic-msi", >> > APIC_SPACE_SIZE); >> > diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c >> > index e6eb694..fd425d1 100644 >> > --- a/hw/intc/apic_common.c >> > +++ b/hw/intc/apic_common.c >> > @@ -299,14 +299,6 @@ static void apic_common_realize(DeviceState *dev, >> > Error **errp) >> > APICCommonState *s = APIC_COMMON(dev); >> > APICCommonClass *info; >> > static DeviceState *vapic; >> > - static int apic_no; >> > - >> > - if (apic_no >= MAX_APICS) { >> > - error_setg(errp, "%s initialization failed.", >> > - object_get_typename(OBJECT(dev))); >> > - return; >> > - } >> > - s->idx = apic_no++; >> > >> > info = APIC_COMMON_GET_CLASS(s); >> > info->realize(dev, errp); >> > diff --git a/include/hw/i386/apic_internal.h >> > b/include/hw/i386/apic_internal.h >> > index 74fe935..5d3be9a 100644 >> > --- a/include/hw/i386/apic_internal.h >> > +++ b/include/hw/i386/apic_internal.h >> > @@ -120,8 +120,6 @@ >> > #define VAPIC_ENABLE_BIT 0 >> > #define VAPIC_ENABLE_MASK (1 << VAPIC_ENABLE_BIT) >> > >> > -#define MAX_APICS 255 >> > - >> > typedef struct APICCommonState APICCommonState; >> > >> > #define TYPE_APIC_COMMON "apic-common" >> > @@ -175,7 +173,7 @@ struct APICCommonState { >> > uint32_t initial_count; >> > int64_t initial_count_load_time; >> > int64_t next_time; >> > - int idx; >> > + int idx; /* not actually common, used only by 'apic' derived class */ >> > QEMUTimer *timer; >> > int64_t timer_expiry; >> > int sipi_vector;