Re: [PATCH] target/ppc: Validate HTABMASK and reserved bits in SDR1 for 32-bit mode

[Chinmay Rath <rathc@linux.ibm.com>]

On 8/11/26 18:47, Chinmay Rath wrote:
> Hi Minhang,
> Thanks for fixing this. Took me a while to get hold of the docs for 32 
> bit mmu, hence the late response.
>
> On 8/7/26 14:47, sesame_h@qq.com wrote:
>> From: Minhang Zhang <zhangminhang@kylinos.cn>
>>
>> ppc_store_sdr1() had validation for 64-bit SDR1 values but lacked
>> corresponding checks for the 32-bit case.  According to the Power ISA,
>> in 32-bit mode SDR1 bits 16-22 are reserved (must be zero) and
>> HTABMASK (bits 23-31) must consist of a consecutive string of
>> 1-bits starting from the LSB, i.e., be of the form 2^n-1.
>>
>> Add checks to reject invalid HTABMASK values and log a guest error
>> for non-zero reserved bits, following the same pattern used by the
>> existing 64-bit validation.
>>
>> Signed-off-by: Minhang Zhang <zhangminhang@kylinos.cn>
>> ---
>>   target/ppc/mmu_common.c | 18 ++++++++++++++++--
>>   1 file changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c
>> index 2499e61..59e8324 100644
>> --- a/target/ppc/mmu_common.c
>> +++ b/target/ppc/mmu_common.c
>> @@ -57,9 +57,23 @@ void ppc_store_sdr1(CPUPPCState *env, target_ulong 
>> value)
>>                        " stored in SDR1", htabsize);
>>               return;
>>           }
>> -    }
>> +    } else
>>   #endif /* defined(TARGET_PPC64) */
>> -    /* FIXME: Should check for valid HTABMASK values in 32-bit case */
>> +    {
>> +        target_ulong htabmask = value & SDR_32_HTABMASK;
>> +        if (value & 0x007F0000UL) {
> From what I see in the programming manual, shouldn't the bits of this 
> flag be the other way around : 0x0000FE00 ?
"bits of this mask", is what I meant. Not "flag", my bad.
> Nevertheless, it would be better to form the mask by ORing the 
> SDR_32_HTABORG and SDR_32_HTABMASK fields from mmu-hash32.h, instead 
> of hardcoding it; the way it is already done for 64 bits in the same 
> function.
>
> Regards,
> Chinmay
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> +                          "Invalid SDR1: reserved bits 0x" 
>> TARGET_FMT_lx
>> +                          " set\n", value & 0x007F0000UL);
>> +            value &= ~0x007F0000UL;
>> +        }
>> +        if ((htabmask & (htabmask + 1)) != 0) {
>> +            qemu_log_mask(LOG_GUEST_ERROR,
>> +                          "Invalid HTABMASK 0x" TARGET_FMT_lx
>> +                          " in SDR1 (must be of form 2^n-1)\n", 
>> htabmask);
>> +            return;
>> +        }
>> +    }
>>       env->spr[SPR_SDR1] = value;
>>   }