From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47249) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bwTzJ-0001cu-GD for qemu-devel@nongnu.org; Tue, 18 Oct 2016 08:59:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bwTzE-0007KC-ER for qemu-devel@nongnu.org; Tue, 18 Oct 2016 08:59:25 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34142) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bwTzE-0007Jf-7D for qemu-devel@nongnu.org; Tue, 18 Oct 2016 08:59:20 -0400 Date: Tue, 18 Oct 2016 10:59:17 -0200 From: Eduardo Habkost Message-ID: <20161018125917.GO3275@thinpad.lan.raisama.net> References: <1476352367-69400-1-git-send-email-imammedo@redhat.com> <1476352367-69400-7-git-send-email-imammedo@redhat.com> <20161018105628.GJ3275@thinpad.lan.raisama.net> <20161018143610.035ee7ad@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161018143610.035ee7ad@nial.brq.redhat.com> Subject: Re: [Qemu-devel] [PATCH v3 06/13] pc: apic_common: extend APIC ID property to 32bit List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: qemu-devel@nongnu.org, kraxel@redhat.com, liuxiaojian6@huawei.com, mst@redhat.com, rkrcmar@redhat.com, peterx@redhat.com, kevin@koconnor.net, pbonzini@redhat.com, lersek@redhat.com, chao.gao@intel.com On Tue, Oct 18, 2016 at 02:36:10PM +0200, Igor Mammedov wrote: > On Tue, 18 Oct 2016 08:56:28 -0200 > Eduardo Habkost wrote: > > > On Thu, Oct 13, 2016 at 11:52:40AM +0200, Igor Mammedov wrote: > > > ACPI ID is 32 bit wide on CPUs with x2APIC support. > > > Extend 'id' property to support it. > > > > > > Signed-off-by: Igor Mammedov > > > --- > > > v3: > > > keep original behaviour where 'id' is readonly after > > > object is realized (pbonzini) > > > --- > > [...] > > > diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c > > > index 8d01c9c..30f2af0 100644 > > > --- a/hw/intc/apic_common.c > > > +++ b/hw/intc/apic_common.c > > > @@ -428,7 +429,6 @@ static const VMStateDescription vmstate_apic_common = { > > > }; > > > > > > static Property apic_properties_common[] = { > > > - DEFINE_PROP_UINT8("id", APICCommonState, id, -1), > > > DEFINE_PROP_UINT8("version", APICCommonState, version, 0x14), > > > DEFINE_PROP_BIT("vapic", APICCommonState, vapic_control, VAPIC_ENABLE_BIT, > > > true), > > > @@ -437,6 +437,49 @@ static Property apic_properties_common[] = { > > > DEFINE_PROP_END_OF_LIST(), > > > }; > > > > > > +static void apic_common_get_id(Object *obj, Visitor *v, const char *name, > > > + void *opaque, Error **errp) > > > +{ > > > + APICCommonState *s = APIC_COMMON(obj); > > > + int64_t value; > > > + > > > + value = s->apicbase & MSR_IA32_APICBASE_EXTD ? s->initial_apic_id : s->id; > > > + visit_type_int(v, name, &value, errp); > > > +} > > > > Who exactly is going to read this property and require this logic > > to be in the property getter? > As it's set/read only from CPU we don't actually have to expose it > as property. > However, I've kept it as read/write property because it has already > been this way and been exposed to external users as some magic property. > Not sure is anyone cares. > > > > Do we really need to expose this to the outside as a magic > > property that changes depending on hardware state? Returning > > initial_apic_id sounds much simpler. > Well that's what it is now, so I've kept current behavior. > If we decide to change property behavior or drop it altogether > I can do it on top. > I agree to make them static properties as follow-up patch. This way, if the change breaks anything we can revert only that patch. -- Eduardo