[{"id":1765899,"web_url":"http://patchwork.ozlabs.org/comment/1765899/","msgid":"<20170910034141.GC2735@umbus.fritz.box>","list_archive_url":null,"date":"2017-09-10T03:41:41","subject":"Re: [Qemu-devel] [PATCH 2/4] spapr: introduce a helper to compute\n\tthe address of the HPT","submitter":{"id":47,"url":"http://patchwork.ozlabs.org/api/people/47/","name":"David Gibson","email":"david@gibson.dropbear.id.au"},"content":"On Mon, Sep 04, 2017 at 11:47:05PM +0200, Greg Kurz wrote:\n> The formula used to compute the address of the HPT allocated by QEMU is\n> open-coded in several places. This patch moves the magic to a dedicated\n> helper. While here, we also patch the callers to only pass the address\n> to KVM if we indeed have a userland HPT (ie, KVM PR).\n\nThe helper function seems reasonable, though I'm not sure about the\nname (a. it's not just a pointer, since it includes the encoded size\nand b. the name doesn't indicate this is basically KVM PR specific).\n\nTHe \"only pass the address to KVM if we indeed have a userland HPT\n(ie, KVM PR)\" bit doesn't really work.  You're doing it by testing for\n(sdr1 != 0), but that can only be true if the HPT is minimum size,\nwhich doesn't have much to do with anything meaningful.\n\n> Signed-off-by: Greg Kurz <groug@kaod.org>\n> ---\n>  hw/ppc/spapr.c          |    9 +++++++++\n>  hw/ppc/spapr_cpu_core.c |   12 +++++++-----\n>  hw/ppc/spapr_hcall.c    |   14 ++++++++------\n>  include/hw/ppc/spapr.h  |    1 +\n>  4 files changed, 25 insertions(+), 11 deletions(-)\n> \n> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c\n> index caffa1276328..bf24c26b756d 100644\n> --- a/hw/ppc/spapr.c\n> +++ b/hw/ppc/spapr.c\n> @@ -1290,6 +1290,15 @@ static void spapr_store_hpte(PPCVirtualHypervisor *vhyp, hwaddr ptex,\n>      }\n>  }\n>  \n> +target_ulong spapr_get_hpt_pointer(sPAPRMachineState *spapr)\n> +{\n> +    if (!spapr->htab) {\n> +        return 0;\n> +    }\n> +\n> +    return (target_ulong)(uintptr_t)spapr->htab | (spapr->htab_shift - 18);\n> +}\n> +\n>  int spapr_hpt_shift_for_ramsize(uint64_t ramsize)\n>  {\n>      int shift;\n> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c\n> index 85037ef71e27..581eb4d92de9 100644\n> --- a/hw/ppc/spapr_cpu_core.c\n> +++ b/hw/ppc/spapr_cpu_core.c\n> @@ -93,11 +93,13 @@ static void spapr_cpu_reset(void *opaque)\n>       * HPT\n>       */\n>      if (kvm_enabled()) {\n> -        env->spr[SPR_SDR1] = (target_ulong)(uintptr_t)spapr->htab\n> -            | (spapr->htab_shift - 18);\n> -        if (kvmppc_put_books_sregs(cpu) < 0) {\n> -            error_report(\"Unable to update SDR1 in KVM\");\n> -            exit(1);\n> +        target_ulong sdr1 = spapr_get_hpt_pointer(spapr);\n> +        if (sdr1) {\n> +            env->spr[SPR_SDR1] = sdr1;\n> +            if (kvmppc_put_books_sregs(cpu) < 0) {\n> +                error_report(\"Unable to update SDR1 in KVM\");\n> +                exit(1);\n> +            }\n>          }\n>      }\n>  }\n> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c\n> index 6ab8c188f381..06059b44ab40 100644\n> --- a/hw/ppc/spapr_hcall.c\n> +++ b/hw/ppc/spapr_hcall.c\n> @@ -735,9 +735,10 @@ static target_ulong h_resize_hpt_commit(PowerPCCPU *cpu,\n>  \n>          if (kvm_enabled()) {\n>              /* For KVM PR, update the HPT pointer */\n> -            target_ulong sdr1 = (target_ulong)(uintptr_t)spapr->htab\n> -                | (spapr->htab_shift - 18);\n> -            kvmppc_update_sdr1(sdr1);\n> +            target_ulong sdr1 = spapr_get_hpt_pointer(spapr);\n> +            if (sdr1) {\n> +                kvmppc_update_sdr1(sdr1);\n> +            }\n>          }\n>  \n>          pending->hpt = NULL; /* so it's not free()d */\n> @@ -1566,9 +1567,10 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,\n>              spapr_reallocate_hpt(spapr, maxshift, &error_fatal);\n>              if (kvm_enabled()) {\n>                  /* For KVM PR, update the HPT pointer */\n> -                target_ulong sdr1 = (target_ulong)(uintptr_t)spapr->htab\n> -                    | (spapr->htab_shift - 18);\n> -                kvmppc_update_sdr1(sdr1);\n> +                target_ulong sdr1 = spapr_get_hpt_pointer(spapr);\n> +                if (sdr1) {\n> +                    kvmppc_update_sdr1(sdr1);\n> +                }\n>              }\n>          }\n>      }\n> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h\n> index c1b365f56431..a1f5edc15018 100644\n> --- a/include/hw/ppc/spapr.h\n> +++ b/include/hw/ppc/spapr.h\n> @@ -709,4 +709,5 @@ void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg);\n>  int spapr_vcpu_id(PowerPCCPU *cpu);\n>  PowerPCCPU *spapr_find_cpu(int vcpu_id);\n>  \n> +target_ulong spapr_get_hpt_pointer(sPAPRMachineState *spapr);\n>  #endif /* HW_SPAPR_H */\n>","headers":{"Return-Path":"<qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=<UNKNOWN>)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=gibson.dropbear.id.au\n\theader.i=@gibson.dropbear.id.au header.b=\"m8+0Vg7U\"; \n\tdkim-atps=neutral"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xqcM90Bl0z9sPk\n\tfor <incoming@patchwork.ozlabs.org>;\n\tSun, 10 Sep 2017 13:42:29 +1000 (AEST)","from localhost ([::1]:51633 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t<qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>)\n\tid 1dqt8d-0004Yp-60\n\tfor incoming@patchwork.ozlabs.org; Sat, 09 Sep 2017 23:42:27 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:52798)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from <dgibson@ozlabs.org>) id 1dqt8B-0004S9-MV\n\tfor qemu-devel@nongnu.org; Sat, 09 Sep 2017 23:42:00 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from <dgibson@ozlabs.org>) id 1dqt88-0002xB-JY\n\tfor qemu-devel@nongnu.org; Sat, 09 Sep 2017 23:41:59 -0400","from ozlabs.org ([103.22.144.67]:38645)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from <dgibson@ozlabs.org>)\n\tid 1dqt87-0002wQ-Uk; Sat, 09 Sep 2017 23:41:56 -0400","by ozlabs.org (Postfix, from userid 1007)\n\tid 3xqcLQ2p8Sz9sRV; Sun, 10 Sep 2017 13:41:50 +1000 (AEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple;\n\td=gibson.dropbear.id.au; s=201602; t=1505014910;\n\tbh=+3GWF9HiKkzg4tGtYNCdyEo97oBbTzF3lukHRhc877k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=m8+0Vg7UwUcT8rhYe7F/TukBMzn5c9emhN0q+QKEFE+fnxTpQ7gQ4fL3EBW+egAyV\n\tyX2OtNqcheNgP6vnulYgOqxM5kMjWylgsWXJfGBMFDFCdL0ROaRsjwr2HkJaF3mvMD\n\tIUGkfKeAsNNrjcovB3V8vEFf9sH3cK6iRSbhpDE8=","Date":"Sun, 10 Sep 2017 13:41:41 +1000","From":"David Gibson <david@gibson.dropbear.id.au>","To":"Greg Kurz <groug@kaod.org>","Message-ID":"<20170910034141.GC2735@umbus.fritz.box>","References":"<150456160452.17000.3290192176290246589.stgit@bahia.lan>\n\t<150456162500.17000.8195671755736683016.stgit@bahia.lan>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"chE8DYtGH5bd9Y2b\"","Content-Disposition":"inline","In-Reply-To":"<150456162500.17000.8195671755736683016.stgit@bahia.lan>","User-Agent":"Mutt/1.8.3 (2017-05-23)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"103.22.144.67","Subject":"Re: [Qemu-devel] [PATCH 2/4] spapr: introduce a helper to compute\n\tthe address of the HPT","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"<qemu-devel.nongnu.org>","List-Unsubscribe":"<https://lists.nongnu.org/mailman/options/qemu-devel>,\n\t<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>","List-Archive":"<http://lists.nongnu.org/archive/html/qemu-devel/>","List-Post":"<mailto:qemu-devel@nongnu.org>","List-Help":"<mailto:qemu-devel-request@nongnu.org?subject=help>","List-Subscribe":"<https://lists.nongnu.org/mailman/listinfo/qemu-devel>,\n\t<mailto:qemu-devel-request@nongnu.org?subject=subscribe>","Cc":"Thomas Huth <thuth@redhat.com>, qemu-ppc@nongnu.org,\n\tqemu-devel@nongnu.org, Suraj Jitindar Singh <sjitindarsingh@gmail.com>","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t<qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>"}},{"id":1766265,"web_url":"http://patchwork.ozlabs.org/comment/1766265/","msgid":"<20170911140437.1124ddfe@bahia.lan>","list_archive_url":null,"date":"2017-09-11T12:04:37","subject":"Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/4] spapr: introduce a helper\n\tto compute the address of the HPT","submitter":{"id":69178,"url":"http://patchwork.ozlabs.org/api/people/69178/","name":"Greg Kurz","email":"groug@kaod.org"},"content":"On Sun, 10 Sep 2017 13:41:41 +1000\nDavid Gibson <david@gibson.dropbear.id.au> wrote:\n\n> On Mon, Sep 04, 2017 at 11:47:05PM +0200, Greg Kurz wrote:\n> > The formula used to compute the address of the HPT allocated by QEMU is\n> > open-coded in several places. This patch moves the magic to a dedicated\n> > helper. While here, we also patch the callers to only pass the address\n> > to KVM if we indeed have a userland HPT (ie, KVM PR).  \n> \n> The helper function seems reasonable, though I'm not sure about the\n> name (a. it's not just a pointer, since it includes the encoded size\n> and b. the name doesn't indicate this is basically KVM PR specific).\n> \n\nSure, I'll come up with a better name.\n\n> THe \"only pass the address to KVM if we indeed have a userland HPT\n> (ie, KVM PR)\" bit doesn't really work.  You're doing it by testing for\n> (sdr1 != 0), but that can only be true if the HPT is minimum size,\n> which doesn't have much to do with anything meaningful.\n> \n\nHmmm... if QEMU has allocated an HPT in userspace then the helper\nnecessarily returns a non-null value, no matter the HPT size. Am\nI missing something ?\n\n> > Signed-off-by: Greg Kurz <groug@kaod.org>\n> > ---\n> >  hw/ppc/spapr.c          |    9 +++++++++\n> >  hw/ppc/spapr_cpu_core.c |   12 +++++++-----\n> >  hw/ppc/spapr_hcall.c    |   14 ++++++++------\n> >  include/hw/ppc/spapr.h  |    1 +\n> >  4 files changed, 25 insertions(+), 11 deletions(-)\n> > \n> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c\n> > index caffa1276328..bf24c26b756d 100644\n> > --- a/hw/ppc/spapr.c\n> > +++ b/hw/ppc/spapr.c\n> > @@ -1290,6 +1290,15 @@ static void spapr_store_hpte(PPCVirtualHypervisor *vhyp, hwaddr ptex,\n> >      }\n> >  }\n> >  \n> > +target_ulong spapr_get_hpt_pointer(sPAPRMachineState *spapr)\n> > +{\n> > +    if (!spapr->htab) {\n> > +        return 0;\n> > +    }\n> > +\n> > +    return (target_ulong)(uintptr_t)spapr->htab | (spapr->htab_shift - 18);\n> > +}\n> > +\n> >  int spapr_hpt_shift_for_ramsize(uint64_t ramsize)\n> >  {\n> >      int shift;\n> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c\n> > index 85037ef71e27..581eb4d92de9 100644\n> > --- a/hw/ppc/spapr_cpu_core.c\n> > +++ b/hw/ppc/spapr_cpu_core.c\n> > @@ -93,11 +93,13 @@ static void spapr_cpu_reset(void *opaque)\n> >       * HPT\n> >       */\n> >      if (kvm_enabled()) {\n> > -        env->spr[SPR_SDR1] = (target_ulong)(uintptr_t)spapr->htab\n> > -            | (spapr->htab_shift - 18);\n> > -        if (kvmppc_put_books_sregs(cpu) < 0) {\n> > -            error_report(\"Unable to update SDR1 in KVM\");\n> > -            exit(1);\n> > +        target_ulong sdr1 = spapr_get_hpt_pointer(spapr);\n> > +        if (sdr1) {\n> > +            env->spr[SPR_SDR1] = sdr1;\n> > +            if (kvmppc_put_books_sregs(cpu) < 0) {\n> > +                error_report(\"Unable to update SDR1 in KVM\");\n> > +                exit(1);\n> > +            }\n> >          }\n> >      }\n> >  }\n> > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c\n> > index 6ab8c188f381..06059b44ab40 100644\n> > --- a/hw/ppc/spapr_hcall.c\n> > +++ b/hw/ppc/spapr_hcall.c\n> > @@ -735,9 +735,10 @@ static target_ulong h_resize_hpt_commit(PowerPCCPU *cpu,\n> >  \n> >          if (kvm_enabled()) {\n> >              /* For KVM PR, update the HPT pointer */\n> > -            target_ulong sdr1 = (target_ulong)(uintptr_t)spapr->htab\n> > -                | (spapr->htab_shift - 18);\n> > -            kvmppc_update_sdr1(sdr1);\n> > +            target_ulong sdr1 = spapr_get_hpt_pointer(spapr);\n> > +            if (sdr1) {\n> > +                kvmppc_update_sdr1(sdr1);\n> > +            }\n> >          }\n> >  \n> >          pending->hpt = NULL; /* so it's not free()d */\n> > @@ -1566,9 +1567,10 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,\n> >              spapr_reallocate_hpt(spapr, maxshift, &error_fatal);\n> >              if (kvm_enabled()) {\n> >                  /* For KVM PR, update the HPT pointer */\n> > -                target_ulong sdr1 = (target_ulong)(uintptr_t)spapr->htab\n> > -                    | (spapr->htab_shift - 18);\n> > -                kvmppc_update_sdr1(sdr1);\n> > +                target_ulong sdr1 = spapr_get_hpt_pointer(spapr);\n> > +                if (sdr1) {\n> > +                    kvmppc_update_sdr1(sdr1);\n> > +                }\n> >              }\n> >          }\n> >      }\n> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h\n> > index c1b365f56431..a1f5edc15018 100644\n> > --- a/include/hw/ppc/spapr.h\n> > +++ b/include/hw/ppc/spapr.h\n> > @@ -709,4 +709,5 @@ void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg);\n> >  int spapr_vcpu_id(PowerPCCPU *cpu);\n> >  PowerPCCPU *spapr_find_cpu(int vcpu_id);\n> >  \n> > +target_ulong spapr_get_hpt_pointer(sPAPRMachineState *spapr);\n> >  #endif /* HW_SPAPR_H */\n> >   \n>","headers":{"Return-Path":"<qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=<UNKNOWN>)","Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xrRw5714vz9s71\n\tfor <incoming@patchwork.ozlabs.org>;\n\tMon, 11 Sep 2017 22:25:25 +1000 (AEST)","from localhost ([::1]:57307 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t<qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>)\n\tid 1drNUp-0005PM-BB\n\tfor incoming@patchwork.ozlabs.org; Mon, 11 Sep 2017 08:07:23 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:36399)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from <groug@kaod.org>) id 1drNSO-0003I0-AJ\n\tfor qemu-devel@nongnu.org; Mon, 11 Sep 2017 08:04:58 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from <groug@kaod.org>) id 1drNSJ-0006m8-Tl\n\tfor qemu-devel@nongnu.org; Mon, 11 Sep 2017 08:04:52 -0400","from 6.mo1.mail-out.ovh.net ([46.105.43.205]:52010)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from <groug@kaod.org>) id 1drNSJ-0006lI-KA\n\tfor qemu-devel@nongnu.org; Mon, 11 Sep 2017 08:04:47 -0400","from player169.ha.ovh.net (b9.ovh.net [213.186.33.59])\n\tby mo1.mail-out.ovh.net (Postfix) with ESMTP id 0BC33932E2\n\tfor <qemu-devel@nongnu.org>; Mon, 11 Sep 2017 14:04:45 +0200 (CEST)","from bahia.lan (gar31-1-82-66-74-139.fbx.proxad.net [82.66.74.139])\n\t(Authenticated sender: groug@kaod.org)\n\tby player169.ha.ovh.net (Postfix) with ESMTPSA id 4F871580082;\n\tMon, 11 Sep 2017 14:04:39 +0200 (CEST)"],"Date":"Mon, 11 Sep 2017 14:04:37 +0200","From":"Greg Kurz <groug@kaod.org>","To":"David Gibson <david@gibson.dropbear.id.au>","Message-ID":"<20170911140437.1124ddfe@bahia.lan>","In-Reply-To":"<20170910034141.GC2735@umbus.fritz.box>","References":"<150456160452.17000.3290192176290246589.stgit@bahia.lan>\n\t<150456162500.17000.8195671755736683016.stgit@bahia.lan>\n\t<20170910034141.GC2735@umbus.fritz.box>","X-Mailer":"Claws Mail 3.14.1 (GTK+ 2.24.31; x86_64-redhat-linux-gnu)","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha1;\n\tboundary=\"Sig_//llnXYSxNnd08YaK1L_1l8l\";\n\tprotocol=\"application/pgp-signature\"","X-Ovh-Tracer-Id":"8925290039733754342","X-VR-SPAMSTATE":"OK","X-VR-SPAMSCORE":"-100","X-VR-SPAMCAUSE":"gggruggvucftvghtrhhoucdtuddrfeelledrgedtgdeglecutefuodetggdotefrodftvfcurfhrohhfihhlvgemucfqggfjpdevjffgvefmvefgnecuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"46.105.43.205","Subject":"Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/4] spapr: introduce a helper\n\tto compute the address of the HPT","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"<qemu-devel.nongnu.org>","List-Unsubscribe":"<https://lists.nongnu.org/mailman/options/qemu-devel>,\n\t<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>","List-Archive":"<http://lists.nongnu.org/archive/html/qemu-devel/>","List-Post":"<mailto:qemu-devel@nongnu.org>","List-Help":"<mailto:qemu-devel-request@nongnu.org?subject=help>","List-Subscribe":"<https://lists.nongnu.org/mailman/listinfo/qemu-devel>,\n\t<mailto:qemu-devel-request@nongnu.org?subject=subscribe>","Cc":"Thomas Huth <thuth@redhat.com>, qemu-ppc@nongnu.org,\n\tqemu-devel@nongnu.org, Suraj Jitindar Singh <sjitindarsingh@gmail.com>","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t<qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>"}},{"id":1766296,"web_url":"http://patchwork.ozlabs.org/comment/1766296/","msgid":"<20170911125045.GD2784@umbus.fritz.box>","list_archive_url":null,"date":"2017-09-11T12:50:45","subject":"Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/4] spapr: introduce a helper\n\tto compute the address of the HPT","submitter":{"id":47,"url":"http://patchwork.ozlabs.org/api/people/47/","name":"David Gibson","email":"david@gibson.dropbear.id.au"},"content":"On Mon, Sep 11, 2017 at 02:04:37PM +0200, Greg Kurz wrote:\n> On Sun, 10 Sep 2017 13:41:41 +1000\n> David Gibson <david@gibson.dropbear.id.au> wrote:\n> \n> > On Mon, Sep 04, 2017 at 11:47:05PM +0200, Greg Kurz wrote:\n> > > The formula used to compute the address of the HPT allocated by QEMU is\n> > > open-coded in several places. This patch moves the magic to a dedicated\n> > > helper. While here, we also patch the callers to only pass the address\n> > > to KVM if we indeed have a userland HPT (ie, KVM PR).  \n> > \n> > The helper function seems reasonable, though I'm not sure about the\n> > name (a. it's not just a pointer, since it includes the encoded size\n> > and b. the name doesn't indicate this is basically KVM PR specific).\n> > \n> \n> Sure, I'll come up with a better name.\n> \n> > THe \"only pass the address to KVM if we indeed have a userland HPT\n> > (ie, KVM PR)\" bit doesn't really work.  You're doing it by testing for\n> > (sdr1 != 0), but that can only be true if the HPT is minimum size,\n> > which doesn't have much to do with anything meaningful.\n> > \n> \n> Hmmm... if QEMU has allocated an HPT in userspace then the helper\n> necessarily returns a non-null value, no matter the HPT size. Am\n> I missing something ?\n\nYes, but the reverse is not true.  Even if qemu *hasn't* allocated an\nHPT in userspace, it will usually return a non-zero value - the only\ncase it won't is when the HPT is minimum size.  That makes the test\npretty pointless.\n\n> \n> > > Signed-off-by: Greg Kurz <groug@kaod.org>\n> > > ---\n> > >  hw/ppc/spapr.c          |    9 +++++++++\n> > >  hw/ppc/spapr_cpu_core.c |   12 +++++++-----\n> > >  hw/ppc/spapr_hcall.c    |   14 ++++++++------\n> > >  include/hw/ppc/spapr.h  |    1 +\n> > >  4 files changed, 25 insertions(+), 11 deletions(-)\n> > > \n> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c\n> > > index caffa1276328..bf24c26b756d 100644\n> > > --- a/hw/ppc/spapr.c\n> > > +++ b/hw/ppc/spapr.c\n> > > @@ -1290,6 +1290,15 @@ static void spapr_store_hpte(PPCVirtualHypervisor *vhyp, hwaddr ptex,\n> > >      }\n> > >  }\n> > >  \n> > > +target_ulong spapr_get_hpt_pointer(sPAPRMachineState *spapr)\n> > > +{\n> > > +    if (!spapr->htab) {\n> > > +        return 0;\n> > > +    }\n> > > +\n> > > +    return (target_ulong)(uintptr_t)spapr->htab | (spapr->htab_shift - 18);\n> > > +}\n> > > +\n> > >  int spapr_hpt_shift_for_ramsize(uint64_t ramsize)\n> > >  {\n> > >      int shift;\n> > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c\n> > > index 85037ef71e27..581eb4d92de9 100644\n> > > --- a/hw/ppc/spapr_cpu_core.c\n> > > +++ b/hw/ppc/spapr_cpu_core.c\n> > > @@ -93,11 +93,13 @@ static void spapr_cpu_reset(void *opaque)\n> > >       * HPT\n> > >       */\n> > >      if (kvm_enabled()) {\n> > > -        env->spr[SPR_SDR1] = (target_ulong)(uintptr_t)spapr->htab\n> > > -            | (spapr->htab_shift - 18);\n> > > -        if (kvmppc_put_books_sregs(cpu) < 0) {\n> > > -            error_report(\"Unable to update SDR1 in KVM\");\n> > > -            exit(1);\n> > > +        target_ulong sdr1 = spapr_get_hpt_pointer(spapr);\n> > > +        if (sdr1) {\n> > > +            env->spr[SPR_SDR1] = sdr1;\n> > > +            if (kvmppc_put_books_sregs(cpu) < 0) {\n> > > +                error_report(\"Unable to update SDR1 in KVM\");\n> > > +                exit(1);\n> > > +            }\n> > >          }\n> > >      }\n> > >  }\n> > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c\n> > > index 6ab8c188f381..06059b44ab40 100644\n> > > --- a/hw/ppc/spapr_hcall.c\n> > > +++ b/hw/ppc/spapr_hcall.c\n> > > @@ -735,9 +735,10 @@ static target_ulong h_resize_hpt_commit(PowerPCCPU *cpu,\n> > >  \n> > >          if (kvm_enabled()) {\n> > >              /* For KVM PR, update the HPT pointer */\n> > > -            target_ulong sdr1 = (target_ulong)(uintptr_t)spapr->htab\n> > > -                | (spapr->htab_shift - 18);\n> > > -            kvmppc_update_sdr1(sdr1);\n> > > +            target_ulong sdr1 = spapr_get_hpt_pointer(spapr);\n> > > +            if (sdr1) {\n> > > +                kvmppc_update_sdr1(sdr1);\n> > > +            }\n> > >          }\n> > >  \n> > >          pending->hpt = NULL; /* so it's not free()d */\n> > > @@ -1566,9 +1567,10 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,\n> > >              spapr_reallocate_hpt(spapr, maxshift, &error_fatal);\n> > >              if (kvm_enabled()) {\n> > >                  /* For KVM PR, update the HPT pointer */\n> > > -                target_ulong sdr1 = (target_ulong)(uintptr_t)spapr->htab\n> > > -                    | (spapr->htab_shift - 18);\n> > > -                kvmppc_update_sdr1(sdr1);\n> > > +                target_ulong sdr1 = spapr_get_hpt_pointer(spapr);\n> > > +                if (sdr1) {\n> > > +                    kvmppc_update_sdr1(sdr1);\n> > > +                }\n> > >              }\n> > >          }\n> > >      }\n> > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h\n> > > index c1b365f56431..a1f5edc15018 100644\n> > > --- a/include/hw/ppc/spapr.h\n> > > +++ b/include/hw/ppc/spapr.h\n> > > @@ -709,4 +709,5 @@ void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg);\n> > >  int spapr_vcpu_id(PowerPCCPU *cpu);\n> > >  PowerPCCPU *spapr_find_cpu(int vcpu_id);\n> > >  \n> > > +target_ulong spapr_get_hpt_pointer(sPAPRMachineState *spapr);\n> > >  #endif /* HW_SPAPR_H */\n> > >   \n> > \n>","headers":{"Return-Path":"<qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=<UNKNOWN>)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=gibson.dropbear.id.au\n\theader.i=@gibson.dropbear.id.au header.b=\"bFWLo/j5\"; \n\tdkim-atps=neutral"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xrT4d6ppRz9s7F\n\tfor <incoming@patchwork.ozlabs.org>;\n\tMon, 11 Sep 2017 23:17:53 +1000 (AEST)","from localhost ([::1]:57688 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t<qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>)\n\tid 1drOb2-00044k-2h\n\tfor incoming@patchwork.ozlabs.org; Mon, 11 Sep 2017 09:17:52 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:56153)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from <dgibson@ozlabs.org>) id 1drOaL-00040g-Nz\n\tfor qemu-devel@nongnu.org; Mon, 11 Sep 2017 09:17:16 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from <dgibson@ozlabs.org>) id 1drOaH-0008SB-5i\n\tfor qemu-devel@nongnu.org; Mon, 11 Sep 2017 09:17:09 -0400","from ozlabs.org ([2401:3900:2:1::2]:48719)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from <dgibson@ozlabs.org>)\n\tid 1drOaG-0008QE-IB; Mon, 11 Sep 2017 09:17:05 -0400","by ozlabs.org (Postfix, from userid 1007)\n\tid 3xrT3d64K8z9s83; Mon, 11 Sep 2017 23:17:01 +1000 (AEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple;\n\td=gibson.dropbear.id.au; s=201602; t=1505135821;\n\tbh=J3mcxIuVqbbuYEXTLC0xDIa22tbhQ3p+Bn5PlpkaGz4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bFWLo/j5nkRwV4Ore+spE0WTFfpNQTSzcVLzZ2PlSkSXj3jvp/O/rJd8MbWtfBm7D\n\t6I+giDXrDCr/nvySibN9Zsz4c3FT1AXPhyWY0WvhchVgI8asSTDbqPa4WasUbQrQhq\n\tDemP3gwb3HKgqzmBcDilOn2WvoPBMOkcuc5hycxE=","Date":"Mon, 11 Sep 2017 22:50:45 +1000","From":"David Gibson <david@gibson.dropbear.id.au>","To":"Greg Kurz <groug@kaod.org>","Message-ID":"<20170911125045.GD2784@umbus.fritz.box>","References":"<150456160452.17000.3290192176290246589.stgit@bahia.lan>\n\t<150456162500.17000.8195671755736683016.stgit@bahia.lan>\n\t<20170910034141.GC2735@umbus.fritz.box>\n\t<20170911140437.1124ddfe@bahia.lan>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"VV4b6MQE+OnNyhkM\"","Content-Disposition":"inline","In-Reply-To":"<20170911140437.1124ddfe@bahia.lan>","User-Agent":"Mutt/1.8.3 (2017-05-23)","X-detected-operating-system":"by eggs.gnu.org: Genre and OS details not\n\trecognized.","X-Received-From":"2401:3900:2:1::2","Subject":"Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/4] spapr: introduce a helper\n\tto compute the address of the HPT","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"<qemu-devel.nongnu.org>","List-Unsubscribe":"<https://lists.nongnu.org/mailman/options/qemu-devel>,\n\t<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>","List-Archive":"<http://lists.nongnu.org/archive/html/qemu-devel/>","List-Post":"<mailto:qemu-devel@nongnu.org>","List-Help":"<mailto:qemu-devel-request@nongnu.org?subject=help>","List-Subscribe":"<https://lists.nongnu.org/mailman/listinfo/qemu-devel>,\n\t<mailto:qemu-devel-request@nongnu.org?subject=subscribe>","Cc":"Thomas Huth <thuth@redhat.com>, qemu-ppc@nongnu.org,\n\tqemu-devel@nongnu.org, Suraj Jitindar Singh <sjitindarsingh@gmail.com>","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t<qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>"}},{"id":1766317,"web_url":"http://patchwork.ozlabs.org/comment/1766317/","msgid":"<20170911155416.0e4596b8@bahia.lan>","list_archive_url":null,"date":"2017-09-11T13:54:16","subject":"Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/4] spapr: introduce a helper\n\tto compute the address of the HPT","submitter":{"id":69178,"url":"http://patchwork.ozlabs.org/api/people/69178/","name":"Greg Kurz","email":"groug@kaod.org"},"content":"On Mon, 11 Sep 2017 22:50:45 +1000\nDavid Gibson <david@gibson.dropbear.id.au> wrote:\n\n> On Mon, Sep 11, 2017 at 02:04:37PM +0200, Greg Kurz wrote:\n> > On Sun, 10 Sep 2017 13:41:41 +1000\n> > David Gibson <david@gibson.dropbear.id.au> wrote:\n> >   \n> > > On Mon, Sep 04, 2017 at 11:47:05PM +0200, Greg Kurz wrote:  \n> > > > The formula used to compute the address of the HPT allocated by QEMU is\n> > > > open-coded in several places. This patch moves the magic to a dedicated\n> > > > helper. While here, we also patch the callers to only pass the address\n> > > > to KVM if we indeed have a userland HPT (ie, KVM PR).    \n> > > \n> > > The helper function seems reasonable, though I'm not sure about the\n> > > name (a. it's not just a pointer, since it includes the encoded size\n> > > and b. the name doesn't indicate this is basically KVM PR specific).\n> > >   \n> > \n> > Sure, I'll come up with a better name.\n> >   \n> > > THe \"only pass the address to KVM if we indeed have a userland HPT\n> > > (ie, KVM PR)\" bit doesn't really work.  You're doing it by testing for\n> > > (sdr1 != 0), but that can only be true if the HPT is minimum size,\n> > > which doesn't have much to do with anything meaningful.\n> > >   \n> > \n> > Hmmm... if QEMU has allocated an HPT in userspace then the helper\n> > necessarily returns a non-null value, no matter the HPT size. Am\n> > I missing something ?  \n> \n> Yes, but the reverse is not true.  Even if qemu *hasn't* allocated an\n> HPT in userspace, it will usually return a non-zero value - the only\n> case it won't is when the HPT is minimum size.  That makes the test\n> pretty pointless.\n> \n\nThe helper does return 0 if QEMU hasn't allocated an HPT in userspace.\n\n[...]\n> > > > +target_ulong spapr_get_hpt_pointer(sPAPRMachineState *spapr)\n> > > > +{\n> > > > +    if (!spapr->htab) {\n> > > > +        return 0;\n> > > > +    }\n> > > > +\n> > > > +    return (target_ulong)(uintptr_t)spapr->htab | (spapr->htab_shift - 18);\n> > > > +}\n> > > > +\n\nbut I agree the patch isn't good... for the comprehension at least. I'll\nrework the patchset.\n\nAlso this causes a behavior change: we won't update SDR1 anymore with KVM HV,\nwhich already handles it BTW.\n\nvoid kvmppc_set_hpt(struct kvm *kvm, struct kvm_hpt_info *info)\n{\n\tatomic64_set(&kvm->arch.mmio_update, 0);\n\tkvm->arch.hpt = *info;\n\tkvm->arch.sdr1 = __pa(info->virt) | (info->order - 18);\n\nMaybe I should push this behavior change to a separate patch anyway...","headers":{"Return-Path":"<qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=<UNKNOWN>)","Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xrV1M1ZSfz9s0Z\n\tfor <incoming@patchwork.ozlabs.org>;\n\tTue, 12 Sep 2017 00:00:07 +1000 (AEST)","from localhost ([::1]:57951 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t<qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>)\n\tid 1drPFt-0008RN-95\n\tfor incoming@patchwork.ozlabs.org; Mon, 11 Sep 2017 10:00:05 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:43095)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from <groug@kaod.org>) id 1drPAV-0004Cy-Uf\n\tfor qemu-devel@nongnu.org; Mon, 11 Sep 2017 09:54:33 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from <groug@kaod.org>) id 1drPAR-0004Dr-UH\n\tfor qemu-devel@nongnu.org; Mon, 11 Sep 2017 09:54:32 -0400","from 16.mo1.mail-out.ovh.net ([178.33.104.224]:38209)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from <groug@kaod.org>) id 1drPAR-0004Cz-NJ\n\tfor qemu-devel@nongnu.org; Mon, 11 Sep 2017 09:54:27 -0400","from player169.ha.ovh.net (b9.ovh.net [213.186.33.59])\n\tby mo1.mail-out.ovh.net (Postfix) with ESMTP id CA3C39359C\n\tfor <qemu-devel@nongnu.org>; Mon, 11 Sep 2017 15:54:25 +0200 (CEST)","from bahia.lan (gar31-1-82-66-74-139.fbx.proxad.net [82.66.74.139])\n\t(Authenticated sender: groug@kaod.org)\n\tby player169.ha.ovh.net (Postfix) with ESMTPSA id ED07958009D;\n\tMon, 11 Sep 2017 15:54:17 +0200 (CEST)"],"Date":"Mon, 11 Sep 2017 15:54:16 +0200","From":"Greg Kurz <groug@kaod.org>","To":"David Gibson <david@gibson.dropbear.id.au>","Message-ID":"<20170911155416.0e4596b8@bahia.lan>","In-Reply-To":"<20170911125045.GD2784@umbus.fritz.box>","References":"<150456160452.17000.3290192176290246589.stgit@bahia.lan>\n\t<150456162500.17000.8195671755736683016.stgit@bahia.lan>\n\t<20170910034141.GC2735@umbus.fritz.box>\n\t<20170911140437.1124ddfe@bahia.lan>\n\t<20170911125045.GD2784@umbus.fritz.box>","X-Mailer":"Claws Mail 3.14.1 (GTK+ 2.24.31; x86_64-redhat-linux-gnu)","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha1;\n\tboundary=\"Sig_/NXmNHS6lA5gKBdnCL5.oeKc\";\n\tprotocol=\"application/pgp-signature\"","X-Ovh-Tracer-Id":"10777113912296053222","X-VR-SPAMSTATE":"OK","X-VR-SPAMSCORE":"-100","X-VR-SPAMCAUSE":"gggruggvucftvghtrhhoucdtuddrfeelledrgedtgdejvdcutefuodetggdotefrodftvfcurfhrohhfihhlvgemucfqggfjpdevjffgvefmvefgnecuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"178.33.104.224","Subject":"Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/4] spapr: introduce a helper\n\tto compute the address of the HPT","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"<qemu-devel.nongnu.org>","List-Unsubscribe":"<https://lists.nongnu.org/mailman/options/qemu-devel>,\n\t<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>","List-Archive":"<http://lists.nongnu.org/archive/html/qemu-devel/>","List-Post":"<mailto:qemu-devel@nongnu.org>","List-Help":"<mailto:qemu-devel-request@nongnu.org?subject=help>","List-Subscribe":"<https://lists.nongnu.org/mailman/listinfo/qemu-devel>,\n\t<mailto:qemu-devel-request@nongnu.org?subject=subscribe>","Cc":"Thomas Huth <thuth@redhat.com>, qemu-ppc@nongnu.org,\n\tqemu-devel@nongnu.org, Suraj Jitindar Singh <sjitindarsingh@gmail.com>","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t<qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>"}}]