From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40384) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YUfS0-0007HS-9t for qemu-devel@nongnu.org; Sun, 08 Mar 2015 13:57:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YUfRz-0005Qd-0z for qemu-devel@nongnu.org; Sun, 08 Mar 2015 13:57:16 -0400 Message-ID: <54FC8D5D.2050104@redhat.com> Date: Sun, 08 Mar 2015 19:56:45 +0200 From: Marcel Apfelbaum MIME-Version: 1.0 References: <1425567322-8337-1-git-send-email-marcel@redhat.com> <1425567322-8337-11-git-send-email-marcel@redhat.com> <20150308104634.GA4714@redhat.com> <54FC2CA7.8080408@redhat.com> <20150308144754.GB29959@redhat.com> <54FC6943.3050201@redhat.com> <20150308152716.GD29959@redhat.com> <54FC6CB5.7060901@redhat.com> <20150308160320.GA31757@redhat.com> In-Reply-To: <20150308160320.GA31757@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 for-2.3 10/24] hw/apci: add _PRT method for extra PCI root busses List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: kraxel@redhat.com, quintela@redhat.com, seabios@seabios.org, qemu-devel@nongnu.org, agraf@suse.de, alex.williamson@redhat.com, kevin@koconnor.net, qemu-ppc@nongnu.org, hare@suse.de, imammedo@redhat.com, amit.shah@redhat.com, pbonzini@redhat.com, leon.alrae@imgtec.com, aurelien@aurel32.net, rth@twiddle.net On 03/08/2015 06:03 PM, Michael S. Tsirkin wrote: > On Sun, Mar 08, 2015 at 05:37:25PM +0200, Marcel Apfelbaum wrote: >> On 03/08/2015 05:27 PM, Michael S. Tsirkin wrote: >>> On Sun, Mar 08, 2015 at 05:22:43PM +0200, Marcel Apfelbaum wrote: >>>> On 03/08/2015 04:47 PM, Michael S. Tsirkin wrote: >>>>> On Sun, Mar 08, 2015 at 01:04:07PM +0200, Marcel Apfelbaum wrote: >>>>>> On 03/08/2015 12:46 PM, Michael S. Tsirkin wrote: >>>>>>> On Thu, Mar 05, 2015 at 04:55:08PM +0200, Marcel Apfelbaum wrote: >>>>>>>> Signed-off-by: Marcel Apfelbaum >>>>>>> >>>>>>> some ideas for cleaning this up. >>>>>>> there's more here btw. >>>>>>> >>>>>>>> --- >>>>>>>> hw/i386/acpi-build.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>>>>>> 1 file changed, 78 insertions(+) >>>>>>>> >>>>>>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c >>>>>>>> index e5709e8..f0401d2 100644 >>>>>>>> --- a/hw/i386/acpi-build.c >>>>>>>> +++ b/hw/i386/acpi-build.c >>>>>>>> @@ -664,6 +664,83 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, >>>>>>>> aml_append(parent_scope, method); >>>>>>>> } >>>>>>>> >>>>>>>> +static Aml *build_prt(void) >>>>>>>> +{ >>>>>>>> + Aml *method, *pkg, *if_ctx, *while_ctx; >>>>>>>> + >>>>>>>> + method = aml_method("_PRT", 0); >>>>>>>> + >>>>>>>> + aml_append(method, aml_store(aml_package(128), aml_local(0))); >>>>>>>> + aml_append(method, aml_store(aml_int(0), aml_local(1))); >>>>>>>> + while_ctx = aml_while(aml_lless(aml_local(1), aml_int(128))); >>>>>>>> + { >>>>>>>> + aml_append(while_ctx, >>>>>>>> + aml_store(aml_shiftright(aml_local(1), aml_int(2)), aml_local(2))); >>>>>>>> + aml_append(while_ctx, >>>>>>>> + aml_store(aml_and(aml_add(aml_local(1), aml_local(2)), aml_int(3)), >>>>>>>> + aml_local(3))); >>>>>>> >>>>>>> >>>>>>> As an example, you can have >>>>>>> >>>>>>> Aml *i = aml_local(1); >>>>>>> Aml *mask = aml_local(2); >>>>>> Hi Michael, >>>>>> >>>>>> Thank you for the tip, the implementation is ready >>>>>> and I'll submit it right away. >>>>> >>>>> So, one thing that's problematic here is that >>>>> expected files need to be examined manually. >>>> Only if pxb-device is added, otherwise they remain the same. >>>> So by default, make check remains unchanged. >>>>> How about a three-stage approach: >>>>> >>>>> 1. move _PRT out from DSDT - e.g. to a separate ssdt, >>>>> update expected files. >>>>> One way to do this is to first revert >>>>> commit 4ec8d2b3f54dd1dcd9e2a80e529feff4e2603288 >>>>> Author: Igor Mammedov >>>>> pc: acpi-build: drop remaining ssdt_misc template >>>>> 2. rewrite the new SSDT in C, >>>>> produce an otherwise identical code. >>>>> make check will catch errors >>>>> 3. reuse code from (2) for extra roots. >>>> >>>> I am aware of this and I was planning to do that on top of this series. >>>> Four reasons for this: >>>> 1. As stated before, it does not affect make check because this code >>>> affects the ACPI table only if pxb-device is present. >>>> 2. The _PRT is not *exactly* the same, bus 0's _PRT has an extra "if" for >>>> the power-management device than needs needs SCI >>>> 3. The series is already big, I prefer attacking this as a new enhancement: >>>> "Dynamically create bus 0 _PRT" >>>> 4. QEMU 2.3 is approaching, I don't want o diverge now >>>> >>>> >>>> Thanks, >>>> Marcel >>> >>> Meanwhile 2.3 will have two almost identical copies of _PRT? >>> I don't think that's a good idea. >> The are 2 *different* _PRTs for 2 different hw components. >> The look the same only because I *preferred* them to look almost the same, >> I could implement it differently. >> There is actually nothing common about >> 1. PIIX host bridge _PRT implemented statically >> 2. pcb-device _PRT implemented in code. >> >> What is true is the series makes easier to implement dynamic _PRT >> for any device, including PIIX host bridge. It doesn't mean we have two. >> >> Actually we need to think about using the same code, because changing it >> would influence both devices. >> >> I really don't think they are related. >> Thanks, >> Marcel > > > Come on, the same argument would make us duplicate almost > any block of code. Copy-pasting always seems safer and faster > than reusing, until you have to fix a bug in multiple places. Completely agree, but this is not the case. Here we have symmetric ways to solve this: 1. As I do: _PRT for PXB, then reuse it for piix host-bridge PXB code. 2. First the _PRT of piix host-bridge, then reuse it for PXB _PRT And we still make the series longer and more complex. Thanks, Marcel > >> >>> >>>>> >> [...]