From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59020) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bNTjo-00031c-Rv for qemu-devel@nongnu.org; Wed, 13 Jul 2016 19:38:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bNTjk-0004wt-MF for qemu-devel@nongnu.org; Wed, 13 Jul 2016 19:38:43 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50192) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bNTjk-0004wl-E5 for qemu-devel@nongnu.org; Wed, 13 Jul 2016 19:38:40 -0400 Date: Wed, 13 Jul 2016 20:38:37 -0300 From: Eduardo Habkost Message-ID: <20160713233837.GS3727@thinpad.lan.raisama.net> References: <1467786055-85835-1-git-send-email-imammedo@redhat.com> <1467786055-85835-13-git-send-email-imammedo@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v3 12/19] apic: move MAX_APICS check to 'apic' class List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Bandan Das Cc: Igor Mammedov , qemu-devel@nongnu.org, pkrempa@redhat.com, mst@redhat.com, armbru@redhat.com, eduardo.otubo@profitbricks.com, marcel@redhat.com, pbonzini@redhat.com, rth@twiddle.net On Wed, Jul 13, 2016 at 06:47:20PM -0400, Bandan Das wrote: > Igor Mammedov 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 > > --- > > 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