Re: [PATCH v6 09/18] target/ppc: Streamline calculation of RMA limit from LPCR[RMLS]

From: Greg Kurz <groug@kaod.org>
To: David Gibson <david@gibson.dropbear.id.au>
Cc: lvivier@redhat.com, Thomas Huth <thuth@redhat.com>,
	Xiao Guangrong <xiaoguangrong.eric@gmail.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	aik@ozlabs.ru, farosas@linux.ibm.com,
	Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>,
	qemu-devel@nongnu.org, Igor Mammedov <imammedo@redhat.com>,
	qemu-ppc@nongnu.org, clg@kaod.org,
	Paolo Bonzini <pbonzini@redhat.com>,
	"Edgar E. Iglesias" <edgar.iglesias@gmail.com>,
	paulus@samba.org
Subject: Re: [PATCH v6 09/18] target/ppc: Streamline calculation of RMA limit from LPCR[RMLS]
Date: Wed, 26 Feb 2020 08:56:40 +0100	[thread overview]
Message-ID: <20200226085640.6320d9de@bahia.home> (raw)
In-Reply-To: <20200226010413.GH41629@umbus.fritz.box>

[-- Attachment #1: Type: text/plain, Size: 5149 bytes --]

On Wed, 26 Feb 2020 12:04:13 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Feb 25, 2020 at 11:47:25PM +0100, Greg Kurz wrote:
> > On Tue, 25 Feb 2020 18:05:31 +0100
> > Greg Kurz <groug@kaod.org> wrote:
> > 
> > > On Tue, 25 Feb 2020 10:37:15 +1100
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > 
> > > > Currently we use a big switch statement in ppc_hash64_update_rmls() to work
> > > > out what the right RMA limit is based on the LPCR[RMLS] field.  There's no
> > > > formula for this - it's just an arbitrary mapping defined by the existing
> > > > CPU implementations - but we can make it a bit more readable by using a
> > > > lookup table rather than a switch.  In addition we can use the MiB/GiB
> > > > symbols to make it a bit clearer.
> > > > 
> > > > While there we add a bit of clarity and rationale to the comment about
> > > > what happens if the LPCR[RMLS] doesn't contain a valid value.
> > > > 
> > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > Reviewed-by: Cédric Le Goater <clg@kaod.org>
> > > > ---
> > > >  target/ppc/mmu-hash64.c | 71 ++++++++++++++++++++---------------------
> > > >  1 file changed, 35 insertions(+), 36 deletions(-)
> > > > 
> > > > diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> > > > index 0ef330a614..4f082d775d 100644
> > > > --- a/target/ppc/mmu-hash64.c
> > > > +++ b/target/ppc/mmu-hash64.c
> > > > @@ -18,6 +18,7 @@
> > > >   * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> > > >   */
> > > >  #include "qemu/osdep.h"
> > > > +#include "qemu/units.h"
> > > >  #include "cpu.h"
> > > >  #include "exec/exec-all.h"
> > > >  #include "exec/helper-proto.h"This tool was originally developed to fix Linux CPU throttling issues affecting Lenovo T480 / T480s / X1C6 as described here.
> > > > @@ -757,6 +758,39 @@ static void ppc_hash64_set_c(PowerPCCPU *cpu, hwaddr ptex, uint64_t pte1)
> > > >      stb_phys(CPU(cpu)->as, base + offset, (pte1 & 0xff) | 0x80);
> > > >  }
> > > >  
> > > > +static target_ulong rmls_limit(PowerPCCPU *cpu)
> > > > +{
> > > > +    CPUPPCState *env = &cpu->env;
> > > > +    /*
> > > > +     * This is the full 4 bits encoding of POWER8. Previous
> > > > +     * CPUs only support a subset of these but the filtering
> > > > +     * is done when writing LPCR
> > > > +     */
> > > > +    const target_ulong rma_sizes[] = {
> > > > +        [0] = 0,
> > > > +        [1] = 16 * GiB,
> > > > +        [2] = 1 * GiB,
> > > > +        [3] = 64 * MiB,
> > > > +        [4] = 256 * MiB,
> > > > +        [5] = 0,
> > > > +        [6] = 0,
> > > > +        [7] = 128 * MiB,
> > > > +        [8] = 32 * MiB,
> > > > +    };
> > > > +    target_ulong rmls = (env->spr[SPR_LPCR] & LPCR_RMLS) >> LPCR_RMLS_SHIFT;
> > > > +
> > > > +    if (rmls < ARRAY_SIZE(rma_sizes)) {
> > > 
> > > This condition is always true since the RMLS field is 4-bit long... 
> > 
> > Oops my mistake, I was already thinking about the suggestion I have
> > for something that was puzzling me. See below.
> > 
> > > I guess you want to check that RMLS encodes a valid RMA size instead.
> > > 
> > >     if (rma_sizes[rmls]) {
> > > 
> > > > +        return rma_sizes[rmls];
> > > > +    } else {
> > > > +        /*
> > > > +         * Bad value, so the OS has shot itself in the foot.  Return a
> > > > +         * 0-sized RMA which we expect to trigger an immediate DSI or
> > > > +         * ISI
> > > > +         */
> > 
> > It seems a bit weird to differentiate the case where the value is bad
> > because it happens to be bigger than the highest supported one, compared
> > to values that are declared bad in rma_sizes[], like 0, 5 or 6. They're
> > all basically the same case of values not used to encode a valid
> > size...
> 
> Right, but the result is the same either way - the function returns
> 0.  This is basically just a small space optimization.
> 
> > 
> > What about :
> > 
> >     static const target_ulong rma_sizes[16] = {
> >         [1] = 16 * GiB,
> >         [2] = 1 * GiB,
> >         [3] = 64 * MiB,
> >         [4] = 256 * MiB,
> >         [7] = 128 * MiB,
> >         [8] = 32 * MiB,
> >     };
> 
> Eh, I guess?  I don't see much to pick between them.
> 

This is what I had in mind actually.

static target_ulong rmls_limit(PowerPCCPU *cpu)
{
    CPUPPCState *env = &cpu->env;
    /*
     * This is the full 4 bits encoding of POWER8. Previous
     * CPUs only support a subset of these but the filtering
     * is done when writing LPCR.
     *
     * Unsupported values mean the OS has shot itself in the
     * foot. Return a 0-sized RMA in this case, which we expect
     * to trigger an immediate DSI or ISI
     */
    static const target_ulong rma_sizes[16] = {
        [1] = 16 * GiB,
        [2] = 1 * GiB,
        [3] = 64 * MiB,
        [4] = 256 * MiB,
        [7] = 128 * MiB,
        [8] = 32 * MiB,
    };
    target_ulong rmls = (env->spr[SPR_LPCR] & LPCR_RMLS) >> LPCR_RMLS_SHIFT;

    return rma_sizes[rmls];
}

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]