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

[Julien Grall <julien.grall@xxxxxxx>]

Hi Sergej,

On 06/07/17 12:50, Sergej Proskurin wrote:
> +/*
> + * Get the MSB number of the GVA, according to "AddrTop" pseudocode
> + * implementation in ARM DDI 0487B.a J1-6066.
> + */
> +static unsigned int get_top_bit(struct domain *d, vaddr_t gva, register_t tcr)
> +{
> +    unsigned int topbit;
> +
> +    /*
> +     * IF EL1 is using AArch64 then addresses from EL0 using AArch32 are
NIT: s/IF/If/

> +     * zero-extended to 64 bits (ARM DDI 0487B.a J1-6066).
> +     */
> +    if ( is_32bit_domain(d) )
> +        topbit = 31;
> +    else if ( is_64bit_domain(d) )
> +    {
> +        if ( ((gva & BIT_ULL(55)) && (tcr & TCR_EL1_TBI1)) ||
> +             (!(gva & BIT_ULL(55)) && (tcr & TCR_EL1_TBI0)) )
> +            topbit = 55;
> +        else
> +            topbit = 63;
> +    }
> +
> +    return topbit;
> +}
> +
> +/* Make sure the base address does not exceed its configured size. */
> +static int check_base_size(unsigned int output_size, uint64_t 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;
> +
> +    return 0;
This function only return 0 or -EFAULT and the caller doesn't care of 
the exact value. I would prefer if you return a boolean here.
[...]

> +    /*
> +     * 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_is_page(pte, level) && !lpae_is_superpage(pte, level) )
> +        return -EFAULT;
> +
> +    *ipa = pfn_to_paddr(pte.walk.base) | (gva & masks[gran][level]);
I haven't noticed it until now. When using 16KB and 64KB, you rely on 
the bottom bits to be zeroed. Although, the guest could purposefully put 
wrong value here. So you want to mask it as you do just above.
Furthermore, as other part of the Xen ARM you rely on the page size of 
Xen to always be 4KB. This is not really true and this code will break 
as soon as we introduce 16KB/64KB page granularity support in Xen. I 
will have a look on what to do here. No need to worry about that for now.
> +
> +    /*
> +     * 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;
>  }
>
>  int guest_walk_tables(const struct vcpu *v, vaddr_t gva,
Cheers,

--
Julien Grall

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