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 <adeelahmadadl1...@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
--
You received this message because you are subscribed to the Google Groups
"Jailhouse" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to jailhouse-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
