Re: [Qemu-devel] [PATCH v3 12/19] apic: move MAX_APICS check to 'apic' class

[Eduardo Habkost]

On Wed, Jul 13, 2016 at 06:47:20PM -0400, Bandan Das wrote:
> Igor Mammedov <address@hidden> 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 <address@hidden>
> > ---
> >  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.

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

-- 
Eduardo