[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] target-arm: Fix descriptor address masking in A

From: Peter Maydell
Subject: Re: [Qemu-devel] [PATCH] target-arm: Fix descriptor address masking in ARM address translation
Date: Sat, 12 Mar 2016 07:18:30 +0700

On 12 March 2016 at 06:44, Sergey Sorokin <address@hidden> wrote:
> 11.03.2016, 11:41, "Peter Maydell" <address@hidden>:
>>On 4 March 2016 at 23:04, Sergey Sorokin <address@hidden> wrote:
>>> There is a bug in ARM address translation regime with a long-descriptor
>>> format. On the descriptor reading its address is formed from an index
>>> which is a part of the input address. And on the first iteration this index
>>> is incorrectly masked with 'grainsize' mask. But it can be wider according
>>> to pseudo-code.
>>> On the other hand on the iterations other than first the descriptor address
>>> is formed from the previous level descriptor by masking with 'descaddrmask'
>>> value. It always clears just 12 lower bits, but it must clear 'grainsize'
>>> lower bits instead according to pseudo-code.
>>> The patch fixes both cases.
>>This is pretty confusing to understand -- it might help if you
>>could give an example.
> According to documentation (ARMv8 ARM DDI 0487A.i J1.1.5:
> aarch64/translation/walk/AArch64.TranslationTableWalk):
> bits(48) index = ZeroExtend(inputaddr<addrselecttop:addrselectbottom>:'000');
> descaddr.paddress.physicaladdress = baseaddress OR index;
> For a first iteration of the descriptor reading:
> addrselecttop = inputsize - 1;
> addrselectbottom = (3-level)*stride + grainsize;
> Let's assume grainsize == 12 (so stride == 9), level == 1, inputsize == 43.
> Then index is
> inputaddr<42:30>:'000';

...which is more than 9 bits, so when does this happen?
I think this can only happen for the Stage-2 only
concatenated translation-tables case...

(I agree we have a bug here, I'm just trying to work out when it
can trigger; if it's only possible for S2 page tables then it's
not a visible bug yet because no CPUs have EL2 support enabled.)

>>> -    /* The address field in the descriptor goes up to bit 39 for ARMv7
>>> -     * but up to bit 47 for ARMv8.
>>> +    /* The address field in the descriptor goes up to bit 39 for AArch32
>>> +     * but up to bit 47 for AArch64.
>>>       */
>>This is not correct -- the descriptor field widths are as the comment
>>states before your patch:
>> * up to bit 39 for ARMv7
>> * up to bit 47 for ARMv8 (whether AArch32 or AArch64)
>>See the v8 ARM ARM AArch32.TranslationTableWalkLD pseudocode and in
>>particular note the width which it uses for AddressSizeFault checks.
> I see in ARMv8 ARM DDI 0487A.i J1.2.4
> aarch32/translation/walk/AArch32.TranslationTableWalkLD:
> Before 'repeat' cycle:
> baseaddress = baseregister<39:baselowerbound>:Zeros(baselowerbound);
> Inside the cycle:
> baseaddress = desc<39:grainsize>:Zeros(grainsize);

Yes, but this happens only after we have done the check:

  if !IsZero(desc<47:40>) then
     [take the AddressSize fault]

which tells us that the descriptor field really is up to bit 48.
We just haven't yet implemented the check in QEMU which will
generate the AddressSize fault if the top bits are nonzero.
(In contrast, in ARMv7 there really are only 40 bits there.)

If you want to implement the AddressSize checks that's fine,
but otherwise please leave this bit of the code alone.

-- PMM

reply via email to

[Prev in Thread] Current Thread [Next in Thread]