On 20.12.2012, at 04:38, David Gibson <da...@gibson.dropbear.id.au> wrote:
> 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 <da...@gibson.dropbear.id.au> >>>>>> 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 <da...@gibson.dropbear.id.au> >>>>>>>>> --- >>>>>>>>> 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. Then create 2 fixups. One before and one after init ;). Alex > > -- > 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
