Re: [PATCHv3 08/11] arm64: Check for selected granule support

[Ard Biesheuvel]

On 15 October 2015 at 14:58, Suzuki K. Poulose <Suzuki.Poulose@xxxxxxx> wrote:
> On 15/10/15 13:37, Mark Rutland wrote:
>>
>> On Thu, Oct 15, 2015 at 12:25:33PM +0100, Suzuki K. Poulose wrote:
>>>
>>> On Thu, Oct 15, 2015 at 11:45:15AM +0100, Mark Rutland wrote:
>>>>
>>>> On Wed, Oct 14, 2015 at 04:13:47PM -0500, Jeremy Linton wrote:
>>>>>
>>>>> On 10/14/2015 06:20 AM, Suzuki K. Poulose wrote:
>
>
>
>>>
>>> ----8>----
>>>
>>> Author: Suzuki K. Poulose <suzuki.poulose@xxxxxxx>
>>> Date:   Wed Oct 14 11:25:16 2015 +0100
>>>
>>>      arm64: Check for selected granule support
>>>
>>>      Ensure that the selected page size is supported by the CPU(s). If it
>>> isn't
>>>      park the CPU. A check is added to the EFI stub to detect if the boot
>>> CPU
>>>      supports the page size, failing which, we fail the boot gracefully,
>>> with
>>>      an error message.
>>>
>>>      Signed-off-by: Suzuki K. Poulose <suzuki.poulose@xxxxxxx>
>>>      [ Added a check to EFI stub ]
>>>      Signed-off-by: Jeremy Linton <jeremy.linton@xxxxxxx>
>>
>>
>> Your sign-off should be last, given you are taking resposibility for
>> Jeremy's patch.
>
>
> OK, I was a bit confused about how it should look like. I kept it on top
> so that I could add [ ] for Jeremy's contribution.
>
>>
>> However, I would prefer that the EFI stub addition were a separate/later
>> patch.
>
>
> OK, that makes sense.
>
>>
>>> diff --git a/arch/arm64/include/asm/sysreg.h
>>> b/arch/arm64/include/asm/sysreg.h
>>> index a7f3d4b..72d814c 100644
>>> --- a/arch/arm64/include/asm/sysreg.h
>>> +++ b/arch/arm64/include/asm/sysreg.h
>
>
>>> +#define ID_AA64MMFR0_TGRAN4_NI         0xf
>>> +#define ID_AA64MMFR0_TGRAN4_ON         0x0
>>> +#define ID_AA64MMFR0_TGRAN64_NI                0xf
>>> +#define ID_AA64MMFR0_TGRAN64_ON                0x0
>>> +#define ID_AA64MMFR0_TGRAN16_NI                0x0
>>> +#define ID_AA64MMFR0_TGRAN16_ON                0x1
>>
>>
>> I still don't like "ON" here -- I thought these would also be changed
>> s/ON/SUPPORTED/.
>
>
> I know and I expected that. I have "_ON" in my 'CPU feature' series, which
> will/can be reused here. Hence kept it _ON. I can change it everywhere to
> _SUPPORTED, since I may need to spin another version for that.
>
>
>>>   #include <linux/efi.h>
>>>   #include <asm/efi.h>
>>> +#include <asm/sysreg.h>
>>>   #include <asm/sections.h>
>>
>>
>> Nit: include order.
>
>
> OK
>
>>
>>>
>>> +#if defined(CONFIG_ARM64_4K_PAGES)
>>> +#define PAGE_SIZE_STR  "4K"
>>> +#elif defined(CONFIG_ARM64_64K_PAGES)
>>> +#define PAGE_SIZE_STR  "64K"
>>> +#endif
>>> +

4k can be dropped (since UEFI support implies support for 4k pages)
16k is missing here

>>>   efi_status_t __init handle_kernel_image(efi_system_table_t
>>> *sys_table_arg,
>>>                                         unsigned long *image_addr,
>>>                                         unsigned long *image_size,
>>> @@ -25,6 +32,17 @@ efi_status_t __init
>>> handle_kernel_image(efi_system_table_t *sys_table_arg,
>>>         unsigned long kernel_size, kernel_memsize = 0;
>>>         unsigned long nr_pages;
>>>         void *old_image_addr = (void *)*image_addr;
>>> +       u64 aa64mmfr0_el1;
>>> +
>>> +       /*
>>> +        * Check to see if the CPU supports the requested pagesize
>>> +        */

... so here you would need a #ifndef CONFIG_ARM_4K_PAGES as well.

>>> +       asm volatile("mrs %0, ID_AA64MMFR0_EL1" : "=r" (aa64mmfr0_el1));
>>
>>
>> Can we not use read_cpuid() or similar here?
>
>
> Yes, I will try that out. I didn't want to include additional header-files
> in efi-stub.c.
>
>
>>> +       aa64mmfr0_el1 >>= ID_AA64MMFR0_TGRAN_SHIFT;
>>
>>
>> ... and can we not do the shift and mask in one go?
>>
>
>>> +       if ((aa64mmfr0_el1 & 0xf) != ID_AA64MMFR0_TGRAN_SUPPORTED) {
>>> +               pr_efi_err(sys_table_arg, PAGE_SIZE_STR" granule not
>>> supported by the CPU\n");
>>
>>
>> Nit: space before the first quote, please.
>
>
> Will do
>
> Thanks
> Suzuki
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/