Re: [PATCH] target/arm: Fix handling of LPAE block descriptors

[Richard Henderson <richard.henderson@linaro.org>]

On 3/4/22 08:56, Peter Maydell wrote:
> LPAE descriptors come in three forms:
> 
>   * table descriptors, giving the address of the next level page table
>   * page descriptors, which occur only at level 3 and describe the
>     mapping of one page (which might be 4K, 16K or 64K)
>   * block descriptors, which occur at higher page table levels, and
>     describe the mapping of huge pages
> 
> QEMU's page-table-walk code treats block and page entries
> identically, simply ORing in a number of bits from the input virtual
> address that depends on the level of the page table that we stopped
> at; we depend on the previous masking of descaddr with descaddrmask
> to have already cleared out the low bits of the descriptor word.
> 
> This is not quite right: the address field in a block descriptor is
> smaller, and so there are bits which are valid address bits in a page
> descriptor or a table descriptor but which are not supposed to be
> part of the address in a block descriptor, and descaddrmask does not
> clear them.  We previously mostly got away with this because those
> descriptor bits are RES0; however with FEAT_BBM (part of Armv8.4)
> block descriptor bit 16 is defined to be the nT bit.  No emulated
> QEMU CPU has FEAT_BBM yet, but if the host CPU has it then we might
> see it when using KVM or hvf.
> 
> Explicitly zero out all the descaddr bits we're about to OR vaddr
> bits into.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/790
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/helper.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 088956eecf0..b5c8caafe84 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -11706,11 +11706,17 @@ static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address,
>               indexmask = indexmask_grainsize;
>               continue;
>           }
> -        /* Block entry at level 1 or 2, or page entry at level 3.
> +        /*
> +         * Block entry at level 1 or 2, or page entry at level 3.
>            * These are basically the same thing, although the number
> -         * of bits we pull in from the vaddr varies.
> +         * of bits we pull in from the vaddr varies. Note that although
> +         * descaddrmask masks enough of the low bits of the descriptor
> +         * to give a correct page or table address, the address field
> +         * in a block descriptor is smaller; so we need to explicitly
> +         * clear the lower bits here before ORing in the low vaddr bits.
>            */
>           page_size = (1ULL << ((stride * (4 - level)) + 3));
> +        descaddr &= ~(page_size - 1);
>           descaddr |= (address & (page_size - 1));

As is,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

though I wonder if it reads better as

     page_bits = stride * (4 - level) + 3;
     descaddr = deposit64(descaddr, 0, page_bits, address);


r~