[PATCH] arch/arm-common: Simplify bit field extractions using GET_FIELD macro

[Adeel Ahmad]

In file jailhouse/hypervisor/arch/arm-common/paging.c
the GET_FIELD macro is utilized for the sake of
code simplification.

Signed-off-by: Adeel Ahmad <adeelahm...@gmail.com>
---
hypervisor/arch/arm-common/paging.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hypervisor/arch/arm-common/paging.c b/hypervisor/arch/arm-common/paging.c
index 2ba7da6..7d919ac 100644
--- a/hypervisor/arch/arm-common/paging.c
+++ b/hypervisor/arch/arm-common/paging.c
@@ -45,7 +45,7 @@ static bool arm_page_table_empty(page_table_t page_table)
#if MAX_PAGE_TABLE_LEVELS > 3
static pt_entry_t arm_get_l0_entry(page_table_t page_table, unsigned long virt)
{
- return &page_table[(virt & L0_VADDR_MASK) >> 39];
+ return &page_table[GET_FIELD((virt & L0_VADDR_MASK), 39, 39)];
}

static unsigned long arm_get_l0_phys(pt_entry_t pte, unsigned long virt)
@@ -59,7 +59,7 @@ static unsigned long arm_get_l0_phys(pt_entry_t pte, unsigned long virt)
#if MAX_PAGE_TABLE_LEVELS > 2
static pt_entry_t arm_get_l1_entry(page_table_t page_table, unsigned long virt)
{
- return &page_table[(virt & L1_VADDR_MASK) >> 30];
+ return &page_table[GET_FIELD((virt & L1_VADDR_MASK), 30, 30)];
}

static void arm_set_l1_block(pt_entry_t pte, unsigned long phys, unsigned long flags)
@@ -77,24 +77,24 @@ static unsigned long arm_get_l1_phys(pt_entry_t pte, unsigned long virt)

static pt_entry_t arm_get_l1_alt_entry(page_table_t page_table, unsigned long virt)
{
- return &page_table[(virt & BIT_MASK(48,30)) >> 30];
+ return &page_table[GET_FIELD((virt), 48, 30) >> 30];
}

static unsigned long arm_get_l1_alt_phys(pt_entry_t pte, unsigned long virt)
{
if ((*pte & PTE_TABLE_FLAGS) == PTE_TABLE_FLAGS)
return INVALID_PHYS_ADDR;
- return (*pte & BIT_MASK(48,30)) | (virt & BIT_MASK(29,0));
+ return GET_FIELD((*pte), 48, 30) | GET_FIELD((virt), 29, 0);
}

static pt_entry_t arm_get_l2_entry(page_table_t page_table, unsigned long virt)
{
- return &page_table[(virt & L2_VADDR_MASK) >> 21];
+ return &page_table[GET_FIELD((virt & L2_VADDR_MASK), 21, 21)];
}

static pt_entry_t arm_get_l3_entry(page_table_t page_table, unsigned long virt)
{
- return &page_table[(virt & L3_VADDR_MASK) >> 12];
+ return &page_table[GET_FIELD((virt & L3_VADDR_MASK), 12, 12)];
}

static void arm_set_l2_block(pt_entry_t pte, unsigned long phys, unsigned long flags)
--
1.9.1

[Jan Kiszka]

On 2018-03-07 19:20, Adeel Ahmad wrote:
> In file jailhouse/hypervisor/arch/arm-common/paging.c
> the GET_FIELD macro is utilized for the sake of
> code simplification.
>
> Signed-off-by: Adeel Ahmad <adeelahm...@gmail.com>
> ---
> hypervisor/arch/arm-common/paging.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/hypervisor/arch/arm-common/paging.c b/hypervisor/arch/arm-common/paging.c
> index 2ba7da6..7d919ac 100644
> --- a/hypervisor/arch/arm-common/paging.c
> +++ b/hypervisor/arch/arm-common/paging.c
> @@ -45,7 +45,7 @@ static bool arm_page_table_empty(page_table_t page_table)
> #if MAX_PAGE_TABLE_LEVELS > 3
> static pt_entry_t arm_get_l0_entry(page_table_t page_table, unsigned long virt)
> {
> - return &page_table[(virt & L0_VADDR_MASK) >> 39];
> + return &page_table[GET_FIELD((virt & L0_VADDR_MASK), 39, 39)];

That and similar cases below are actually not correct (you would now
return a single bit rather than the whole field), nor is it the
intention of the GET_FIELD macro. The idea is that GET_FIELD alone is
enough, no further masking nor shifting should be needed.

We rather want something like

#define L0_INDEX(virt) GET_FIELD(virt, 47, 39)

The virtual address bits 47:39 are used here to derive an index into the
page table at level 0, the topmost level.

> }
>
> static unsigned long arm_get_l0_phys(pt_entry_t pte, unsigned long virt)
> @@ -59,7 +59,7 @@ static unsigned long arm_get_l0_phys(pt_entry_t pte, unsigned long virt)
> #if MAX_PAGE_TABLE_LEVELS > 2
> static pt_entry_t arm_get_l1_entry(page_table_t page_table, unsigned long virt)
> {
> - return &page_table[(virt & L1_VADDR_MASK) >> 30];
> + return &page_table[GET_FIELD((virt & L1_VADDR_MASK), 30, 30)];

See above.

> }
>
> static void arm_set_l1_block(pt_entry_t pte, unsigned long phys, unsigned long flags)
> @@ -77,24 +77,24 @@ static unsigned long arm_get_l1_phys(pt_entry_t pte, unsigned long virt)
>
> static pt_entry_t arm_get_l1_alt_entry(page_table_t page_table, unsigned long virt)
> {
> - return &page_table[(virt & BIT_MASK(48,30)) >> 30];
> + return &page_table[GET_FIELD((virt), 48, 30) >> 30];

Nope, just a bit differently incorrect.

> }
>
> static unsigned long arm_get_l1_alt_phys(pt_entry_t pte, unsigned long virt)
> {
> if ((*pte & PTE_TABLE_FLAGS) == PTE_TABLE_FLAGS)
> return INVALID_PHYS_ADDR;
> - return (*pte & BIT_MASK(48,30)) | (virt & BIT_MASK(29,0));
> + return GET_FIELD((*pte), 48, 30) | GET_FIELD((virt), 29, 0);

This is also not equivalent, more precisely the first half. Look again
what the macro does.

In this case, it's clearer to keep the code as is.

> }
>
> static pt_entry_t arm_get_l2_entry(page_table_t page_table, unsigned long virt)
> {
> - return &page_table[(virt & L2_VADDR_MASK) >> 21];
> + return &page_table[GET_FIELD((virt & L2_VADDR_MASK), 21, 21)];
> }
>
> static pt_entry_t arm_get_l3_entry(page_table_t page_table, unsigned long virt)
> {
> - return &page_table[(virt & L3_VADDR_MASK) >> 12];
> + return &page_table[GET_FIELD((virt & L3_VADDR_MASK), 12, 12)];
> }
>
> static void arm_set_l2_block(pt_entry_t pte, unsigned long phys, unsigned long flags)
>

And the other two fall into the first category again.

Please re-check the logic of the macros (GET_FIELD, BIT_FIELD) and
compare carefully what the original code tries to achieve.

Jan

--
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

[Adeel Ahmad]

Thanks for the review!

I just submitted a patch which tries to fix the issues pointed out above. For now, I have used the GET_FIELD macro directly for computing the index e.g. &page_table[GET_FIELD(virt, 47, 39)];

I did try defining separate macros for this in the hypervisor/arch/arm64/include/asm/paging.h file:

#define L0_INDEX(virt) GET_FIELD(virt, 47, 39)

#define L1_INDEX(virt) GET_FIELD(virt, 38, 30)
#define L1_ALT_INDEX(virt) GET_FIELD(virt, 48, 30)
#define L2_INDEX(virt) GET_FIELD(virt, 29, 21)
#define L3_INDEX(virt) GET_FIELD(virt, 20, 12)

But, this produced the error: implicit declaration of function ‘L1_ALT_INDEX‘. The function is defined before its being used, so I'm not sure why this error occurs.

The patch hasn't shown up on the mailing list again. I tried to send from my Gmail account. It probably got marked as spam again.

Please let me know if I need to further improve upon the latest patch.

Thank you,
Adeel