From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35224) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bwUxy-0000LT-Ue for qemu-devel@nongnu.org; Tue, 18 Oct 2016 10:02:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bwUxu-0000bp-28 for qemu-devel@nongnu.org; Tue, 18 Oct 2016 10:02:07 -0400 Received: from mx1.redhat.com ([209.132.183.28]:54438) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bwUxt-0000bZ-RY for qemu-devel@nongnu.org; Tue, 18 Oct 2016 10:02:02 -0400 Date: Tue, 18 Oct 2016 16:01:56 +0200 From: Igor Mammedov Message-ID: <20161018160156.28470db3@nial.brq.redhat.com> In-Reply-To: <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> <20161018125917.GO3275@thinpad.lan.raisama.net> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit 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: Eduardo Habkost 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, 18 Oct 2016 10:59:17 -0200 Eduardo Habkost wrote: > 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. Static property won't work here as it should show APIC ID depending on current CPU mode. I could make it readonly property on respin, and set apic.id/initial_apic_id directly from CPU. Change would be - apic_common_set_id() + apic_common::set_apic_id() callback It won't get us less LOC, more likely it will take even more code to do so. As it's in this patch, it's at least consistent in a way values are get/set. And effectively it's readonly due to check: + if (dev->realized) { + qdev_prop_set_after_realize(dev, name, errp); + return; + } as external user can see only realized apic device.