> > -   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.
> 

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

> 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.

Reply via email to