Re: [Qemu-devel] [Qemu-ppc] [PATCH 11/13] pseries: Fixes and enhancements to L1 cache properties

[David Gibson]

On Wed, Dec 19, 2012 at 11:40:09PM +0100, Alexander Graf wrote:
> 
> On 19.12.2012, at 05:34, David Gibson wrote:
> 
> > On Tue, Dec 18, 2012 at 12:49:02AM +0100, Alexander Graf wrote:
> >> 
> >> On 18.12.2012, at 00:00, David Gibson wrote:
> >> 
> >>> On Mon, Dec 17, 2012 at 11:10:12AM +0100, Alexander Graf wrote:
> >>>> 
> >>>> 
> >>>> On 17.12.2012, at 03:32, David Gibson <address@hidden> wrote:
> >>>> 
> >>>>> On Thu, Dec 13, 2012 at 01:50:25PM +0100, Alexander Graf wrote:
> >>>>>> 
> >>>>>> On 04.12.2012, at 03:42, David Gibson wrote:
> >>>>>> 
> >>>>>>> PAPR requires that the device tree's CPU nodes have several properties
> >>>>>>> with information about the L1 cache.  We created two of these
> >>>>>>> properties, but with incorrect names - "[id]cache-block-size" instead
> >>>>>>> of "[id]-cache-block-size" (note the extra hyphen).
> >>>>>>> 
> >>>>>>> We were also missing some of the required cache properties.  This
> >>>>>>> patch adds the [id]-cache-line-size properties (which have the same
> >>>>>>> values as the block size properties in all current cases).  We also
> >>>>>>> add the [id]-cache-size properties.  The latter requires some extra
> >>>>>>> infrastructure in the general target-ppc code to (optionally) set the
> >>>>>>> cache sizes for various CPUs.  We obtain the published values either
> >>>>>>> from there, or from the host when KVM is in use.
> >>>>>>> 
> >>>>>>> Signed-off-by: David Gibson <address@hidden>
> >>>>>>> ---
> >>>>>>> hw/spapr.c                  |   20 ++++++++++++++++++--
> >>>>>>> target-ppc/cpu.h            |    1 +
> >>>>>>> target-ppc/kvm.c            |   10 ++++++++++
> >>>>>>> target-ppc/kvm_ppc.h        |   12 ++++++++++++
> >>>>>>> target-ppc/translate_init.c |    4 ++++
> >>>>>>> 5 files changed, 45 insertions(+), 2 deletions(-)
> >>>>>>> 
> >>>>>>> diff --git a/hw/spapr.c b/hw/spapr.c
> >>>>>>> index d23aa9d..3bacf2f 100644
> >>>>>>> --- a/hw/spapr.c
> >>>>>>> +++ b/hw/spapr.c
> >>>>>>> @@ -315,6 +315,10 @@ static void *spapr_create_fdt_skel(const char 
> >>>>>>> *cpu_model,
> >>>>>>>                         0xffffffff, 0xffffffff};
> >>>>>>>      uint32_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq() : 
> >>>>>>> TIMEBASE_FREQ;
> >>>>>>>      uint32_t cpufreq = kvm_enabled() ? kvmppc_get_clockfreq() : 
> >>>>>>> 1000000000;
> >>>>>>> +        int dcache_size = kvm_enabled() ? kvmppc_get_dcache_size()
> >>>>>>> +            : env->l1_dcache_size;
> >>>>>>> +        int icache_size = kvm_enabled() ? kvmppc_get_icache_size()
> >>>>>>> +            : env->l1_icache_size;
> >>>>>> 
> >>>>>> By default with KVM we use -cpu host, right? So we already should
> >>>>>> get the correct cache sizes for the CPU you're on.
> >>>>> 
> >>>>> Um.. sort of.  The first problem with that is that I only just added
> >>>>> the cache size information to qemu, so only a few CPUs currently
> >>>>> populate that information.  Using the host info means we can get the
> >>>>> right information even for CPUs that don't yet have cache info in
> >>>>> qemu.
> >>>>> 
> >>>>>> Imagine we would support the compatibility feature where you could
> >>>>>> run with -cpu POWER6 on a POWER7 machine. Would exposing the POWER6
> >>>>>> cache size rather than the host's make any real difference to the
> >>>>>> guest? Or would it work nevertheless?
> >>>>> 
> >>>>> The second problem is that there may be circumstances where the
> >>>>> cache size is altered from the normal size for the cpu.  Running in
> >>>>> POWER6 compat mode
> >>>> 
> >>>> Well, either we want to be compatible or we don't :). If we run with
> >>>> -cpu POWER6 we want to generate the same dt as we did on a POWER6
> >>>> system itself.
> >>> 
> >>> Hrm.  Ok.
> >>> 
> >>> So, the remaining difficulty I have with that is that for -cpu HOST we
> >>> should still take the cache sizes from the host, but that can't easily
> >>> be done because they're only stored in the env, not the cpu_def.
> >> 
> >> Can we set a bit somewhere that allows us to do a sanity check
> >> later? After all, the values coming from the host and the values in
> >> the populated env really should just be identical for -cpu
> >> host. Every time they're not, it's simply a bug that needs to be
> >> reported.
> > 
> > That works.  Although it's not obvious where to put the check and
> > fixup.  kvmppc_fixup_cpu() seems like the obvious place, but that's no
> > good because it's called before the per-cpu-type init function, which
> > is what populates the expected cachesize values.
> 
> That's a real shame. Any reason we don't run it after the init
> function? Fixup indicates that it fixes things up after they
> happened, not before :).

Well, the very first thing that kvmppc_fixup_cpu() did, when I first
implemented it is to adjust the cpu numbers so  we get the right SMT
behaviour under KVM.  I was concerned that later parts of the
initialization might use the cpu_index.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: Digital signature