Re: [Xen-devel] [PATCH v5 10/12] arm/mem_access: Add long-descriptor based gpt

[Julien Grall <julien.grall@xxxxxxx>]

Hi Sergej,

On 06/27/2017 12:52 PM, Sergej Proskurin wrote:
>      Also, create a macro CHECK_BASE_SIZE which simply reduces the code
>      size and simplifies readability.
No, it makes more confusing because you have the return within the 
macro. It is not that bad too have an helper checking the base and do
if ( check_base_size(....) )
  return -EINVAL;

[...]

> +/* Make sure the base address does not exceed its configured size. */
> +#define CHECK_BASE_SIZE(output_size, base)                                  \
> +{                                                                           \
> +    paddr_t mask = GENMASK_ULL((TCR_EL1_IPS_48_BIT_VAL - 1), output_size);  \
> +    if ( output_size < TCR_EL1_IPS_48_BIT_VAL && (base & mask) )            \
> +        return -EFAULT;                                                     \
> +}
See my comment in the changelog about this macro.

[...]

> +    for ( ; ; level++ )
> +    {
> +        /*
> +         * Add offset given by the GVA to the translation table base address.
> +         * Shift the offset by 3 as it is 8-byte aligned.
> +         */
> +        paddr |= offsets[gran][level] << 3;
> +
> +        /* Access the guest's memory to read only one PTE. */
> +        ret = vgic_access_guest_memory(d, paddr, &pte, sizeof(lpae_t), false);
It really doesn't make sense to call a function vgic_* in guest page 
table walk code. I wasn't expected that I needed to explicitly say that 
vgic_access_* should be moved in ARM generic code and be renamed.
[...]

> +    /*
> +     * According to to ARM DDI 0487B.a J1-5927, we return an error if the found
> +     * PTE is invalid or holds a reserved entry (PTE<1:0> == x0)) or if the PTE
> +     * maps a memory block at level 3 (PTE<1:0> == 01).
> +     */
> +    if ( !lpae_valid(pte) || ((level == 3) && !lpae_page(pte, level)) )
NIT: What you want to check here is either the entry is a superpage or a 
page. So the below check would be easier to parse:
if ( !lpae_is_superpage(pte, level) || !lpae_is_page(pte, level) )

> +        return -EFAULT;
> +
> +    *ipa = pfn_to_paddr(pte.walk.base) | (gva & masks[gran][level]);
> +
> +    /*
> +     * Set permissions so that the caller can check the flags by herself. Note
> +     * that stage 1 translations also inherit attributes from the tables
> +     * (ARM DDI 0487B.a J1-5928).
> +     */
> +    if ( !pte.pt.ro && !ro_table )
> +        *perms |= GV2M_WRITE;
> +    if ( !pte.pt.xn && !xn_table )
> +        *perms |= GV2M_EXEC;
> +
> +    return 0;
> +}
> +
> +#undef CHECK_BASE_SIZE
> +
>   int guest_walk_tables(const struct vcpu *v, vaddr_t gva,
>                         paddr_t *ipa, unsigned int *perms)
>   {
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel