Re: [Qemu-devel] Re: [PATCH 12/15] Support 1T segments on ppc

[Alexander Graf]

On 13.02.2011, at 10:34, David Gibson wrote:

> On Sat, Feb 12, 2011 at 04:57:39PM +0100, Alexander Graf wrote:
>> On 12.02.2011, at 15:54, David Gibson wrote:
> [snip]
>>> +    if (rb & (0x1000 - env->slb_nr))
>> 
>> Braces...
> 
> Oops, yeah.  These later patches in the series I haven't really
> audited for coding style adequately yet.  I'll fix these before the
> next version.
> 
> [snip]
>>> +   return -1; /* 1T segment on MMU that doesn't support it */
>>> + 
>>> +    /* We stuff a copy of the B field into slb->esid to simplify
>>> +     * lookup later */
>>> +    slb->esid = (rb & (SLB_ESID_ESID | SLB_ESID_V)) |
>>> +        (rs >> SLB_VSID_SSIZE_SHIFT);
>> 
>> Wouldn't it be easier to add another field?
> 
> Easier for what?  The reason I put these bits in here is that the rest
> of the things slb_lookup() needs to scan for are all in the esid
> field, so putting B in there means slb_lookup() needs only one
> comparison per-slot, per segment size.

Hrm - but it also needs random & ~3 masking in other code which is very 
unpretty. Comparing two numbers really shouldn't hurt performance too much, but 
makes the code better maintainable.

struct slb_entry {
    uint64_t esid;
    uint64_t vsid;
    int b;
}

or so :).

Alex