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

[Thomas Huth]

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.

However, if you respin this patch series anyway, then maybe better add
the ULL for clarity.

 Thomas

signature.asc
Description: OpenPGP digital signature