From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51085) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YUyx0-0008HJ-1G for qemu-devel@nongnu.org; Mon, 09 Mar 2015 10:46:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YUywv-0007iL-VQ for qemu-devel@nongnu.org; Mon, 09 Mar 2015 10:46:33 -0400 Date: Mon, 9 Mar 2015 15:46:00 +0100 From: "Michael S. Tsirkin" Message-ID: <20150309153749-mutt-send-email-mst@redhat.com> References: <1425813387-31231-1-git-send-email-marcel@redhat.com> <1425813387-31231-2-git-send-email-marcel@redhat.com> <20150309112822.1aba6a0d@nial.brq.redhat.com> <20150309120234-mutt-send-email-mst@redhat.com> <20150309132640.7fd91c28@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150309132640.7fd91c28@nial.brq.redhat.com> Subject: Re: [Qemu-devel] [PATCH v4 for-2.3 01/25] acpi: fix aml_equal term implementation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: kraxel@redhat.com, quintela@redhat.com, seabios@seabios.org, qemu-devel@nongnu.org, agraf@suse.de, amit.shah@redhat.com, alex.williamson@redhat.com, kevin@koconnor.net, qemu-ppc@nongnu.org, hare@suse.de, Marcel Apfelbaum , pbonzini@redhat.com, leon.alrae@imgtec.com, aurelien@aurel32.net, rth@twiddle.net On Mon, Mar 09, 2015 at 01:26:40PM +0100, Igor Mammedov wrote: > On Mon, 9 Mar 2015 12:04:51 +0100 > "Michael S. Tsirkin" wrote: > > > On Mon, Mar 09, 2015 at 11:28:22AM +0100, Igor Mammedov wrote: > > > On Sun, 8 Mar 2015 13:16:03 +0200 > > > Marcel Apfelbaum wrote: > > > > > > > The DefLEqual op does not have a target operand. Remove it. > > > > > > > > Signed-off-by: Marcel Apfelbaum > > > Reviewed-by: Igor Mammedov > > > > > > > --- > > > > hw/acpi/aml-build.c | 1 - > > > > 1 file changed, 1 deletion(-) > > > > > > > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c > > > > index 876cada..0d14561 100644 > > > > --- a/hw/acpi/aml-build.c > > > > +++ b/hw/acpi/aml-build.c > > > > @@ -542,7 +542,6 @@ Aml *aml_equal(Aml *arg1, Aml *arg2) > > > > Aml *var = aml_opcode(0x93 /* LequalOp */); > > > > aml_append(var, arg1); > > > > aml_append(var, arg2); > > > > - build_append_int(var->buf, 0x00); /* NullNameOp */ > > > It's just happens to work in case CPU and PCI hotplug because > > > it LEqual was the only predicate in if block and NullNameOp > > > was considered as part of inner code-block, like: > > > if (LEqual(a, b)) { > > > NullName; // nop > > > ... > > > } > > > > So - maybe aml_if should get 3rd parameter - the command? > it's not only one command it's block of AML code inside of 'if' scope. > Adding 3rd argument would mean inventing another not defined by spec element > like aml_block() where you could put AML items that are in block, > I'd like to keep non spec items to minimum and not add them unless we have to. In fact, it's TermList in spec, isn't it? But I don't insist. > > Then for consistence purposes we would add this 'aml_block' argument > to other block constructs like 'device, scope, package, ...' > So I think current way of defining context and then putting items in it > is pretty clean way as opposed to doing it backwards, first defining > elements somewhere and then passing that somewhere as argument to > a AML block construct. I think it's just a question of adding a convenience wrapper for the most common case. /* Convenience wrapper for when there's a single * term in TermList */ Aml *aml_if_then_1term(Aml *predicate, Aml *term) { Aml *if_ctx = aml_if(predicate); aml_append(if_ctx, term); return if_ctx; }