Re: [Qemu-devel] [PATCH 03/10] target-ppc: Rework ppc_store_slb

[David Gibson]

On Wed, Jan 27, 2016 at 08:32:25AM +0100, Thomas Huth wrote:
> On 27.01.2016 01:04, David Gibson wrote:
> > On Mon, Jan 25, 2016 at 08:22:51PM +0100, Alexander Graf wrote:
> >>
> >>
> >> On 01/25/2016 06:15 AM, David Gibson wrote:
> >>> ppc_store_slb updates the SLB for PPC cpus with 64-bit hash MMUs.
> >>> Currently it takes two parameters, which contain values encoded as the
> >>> register arguments to the slbmte instruction, one register contains the
> >>> ESID portion of the SLBE and also the slot number, the other contains the
> >>> VSID portion of the SLBE.
> >>>
> >>> We're shortly going to want to do some SLB updates from other code where
> >>> it is more convenient to supply the slot number and ESID separately, so
> >>> rework this function and its callers to work this way.
> >>>
> >>> As a bonus, this slightly simplifies the emulation of segment registers 
> >>> for
> >>> when running a 32-bit OS on a 64-bit CPU.
> >>>
> >>> Signed-off-by: David Gibson <address@hidden>
> >>> ---
> >>>  target-ppc/kvm.c        |  2 +-
> >>>  target-ppc/mmu-hash64.c | 24 +++++++++++++-----------
> >>>  target-ppc/mmu-hash64.h |  3 ++-
> >>>  target-ppc/mmu_helper.c | 14 +++++---------
> >>>  4 files changed, 21 insertions(+), 22 deletions(-)
> ...
> >>> @@ -196,7 +198,7 @@ void helper_store_slb(CPUPPCState *env, target_ulong 
> >>> rb, target_ulong rs)
> >>>  {
> >>>      PowerPCCPU *cpu = ppc_env_get_cpu(env);
> >>> -    if (ppc_store_slb(cpu, rb, rs) < 0) {
> >>> +    if (ppc_store_slb(cpu, rb & 0xfff, rb & ~0xfff, rs) < 0) {
> >>
> >> This might truncate the esid to 32bits on 32bits hosts, no? Should be
> >> 0xfffULL instead.
> > 
> > Good point, nice catch.
> 
> Are you sure that it is really needed? If I run the following test
> program on my 64-bit system:
> 
> int main()
> {
>       unsigned long long ll = -1ULL;
>       printf("%llx %llx\n", ll, ll & ~0xfff);
>       return 0;
> }
> 
> Then I get this output:
> 
> ffffffffffffffff fffffffffffff000
> 
> So it sounds like the value is sign-extended when it is cast to
> 64-bit.

Ah, yes.  It would be promoted to s64 and sign extended before being
promoted to u64.  Still that's a pretty subtle detail to be relying on.
 
> However, if you respin this patch series anyway, then maybe better add
> the ULL for clarity.

Already done.

-- 
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: PGP signature