On 12/06/2025 12:37, Ayan Kumar Halder wrote:
>
> On 12/06/2025 10:35, Luca Fancellu wrote:
>> Hi Ayan,
> Hi Luca,
>>
>>> On 11 Jun 2025, at 15:35, Ayan Kumar Halder <ayan.kumar.hal...@amd.com>
>>> wrote:
>>>
>>> Define prepare_selector(), read_protection_region() and
>>> write_protection_region() for arm32. Also, define
>>> GENERATE_{READ/WRITE}_PR_REG_OTHERS to access MPU regions from 32 to 255.
>>>
>>> Enable pr_{get/set}_{base/limit}(), region_is_valid() for arm32.
>>> Enable pr_of_addr() for arm32.
>>>
>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.hal...@amd.com>
>>> ---
>> Based on your v2
>> (https://patchwork.kernel.org/project/xen-devel/patch/20250606164854.1551148-4-ayan.kumar.hal...@amd.com/)
>> I was imaging something like this:
>>
>> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
>> index 74e96ca57137..5d324b2d4ca5 100644
>> --- a/xen/arch/arm/mpu/mm.c
>> +++ b/xen/arch/arm/mpu/mm.c
>> @@ -87,20 +87,28 @@ static void __init __maybe_unused build_assertions(void)
>> */
>> static void prepare_selector(uint8_t *sel)
>> {
>> -#ifdef CONFIG_ARM_64
>> uint8_t cur_sel = *sel;
>>
>> +#ifdef CONFIG_ARM_64
>> /*
>> - * {read,write}_protection_region works using the direct access to the
>> 0..15
>> - * regions, so in order to save the isb() overhead, change the
>> PRSELR_EL2
>> - * only when needed, so when the upper 4 bits of the selector will
>> change.
>> + * {read,write}_protection_region works using the Arm64 direct access
>> to the
>> + * 0..15 regions, so in order to save the isb() overhead, change the
>> + * PRSELR_EL2 only when needed, so when the upper 4 bits of the selector
>> + * will change.
>> */
>> cur_sel &= 0xF0U;
>> +#else
>> + /* Arm32 MPU can use direct access for 0-31 */
>> + if ( cur_sel > 31 )
>> + cur_sel = 0;
>> +#endif
>> if ( READ_SYSREG(PRSELR_EL2) != cur_sel )
>> {
>> WRITE_SYSREG(cur_sel, PRSELR_EL2);
>> isb();
>> }
>> +
>> +#ifdef CONFIG_ARM_64
>> *sel = *sel & 0xFU;
>> #endif
>> }
>> @@ -144,6 +152,12 @@ void read_protection_region(pr_t *pr_read, uint8_t sel)
>> GENERATE_READ_PR_REG_CASE(29, pr_read);
>> GENERATE_READ_PR_REG_CASE(30, pr_read);
>> GENERATE_READ_PR_REG_CASE(31, pr_read);
>> + case 32 ... 255:
>> + {
>> + pr->prbar.bits = READ_SYSREG(PRBAR_EL2);
>> + pr->prlar.bits = READ_SYSREG(PRLAR_EL2);
>> + break;
>> + }
>> #endif
>> default:
>> BUG(); /* Can't happen */
>> @@ -190,6 +204,12 @@ void write_protection_region(const pr_t *pr_write,
>> uint8_t sel)
>> GENERATE_WRITE_PR_REG_CASE(29, pr_write);
>> GENERATE_WRITE_PR_REG_CASE(30, pr_write);
>> GENERATE_WRITE_PR_REG_CASE(31, pr_write);
>> + case 32 ... 255:
>> + {
>> + WRITE_SYSREG(pr->prbar.bits & ~MPU_REGION_RES0, PRBAR_EL2);
>> + WRITE_SYSREG(pr->prlar.bits & ~MPU_REGION_RES0, PRLAR_EL2);
>> + break;
>> + }
>> #endif
>> default:
>> BUG(); /* Can't happen */
>>
>>
>> Is it using too ifdefs in your opinion that would benefit the split you do
>> in v3?
>
> Yes. However, I understand that this is subjective. I need your and
> Michal/Julien to have an opinion here whether to go with the split
> (which means some amount of code duplication) or introduce if-defs. I
> will be happy to proceed as per your opinions.
I don't have a strong opinion here. Maybe I slightly prefer the split to avoid
ifdefery.
~Michal
