Re: [Xen-devel] [RFC PATCH v2 6/8] arm/mem_access: Add long-descriptor based gpt

[Sergej Proskurin <proskurin@xxxxxxxxxxxxx>]

Hi Julien,

[...]

>
>> +        {
>> +            input_size = REGISTER_WIDTH_64_BIT - t0_sz;
>> +
>> +            if ( input_size > IPS_MAX )
>> +                /* We limit the input_size to be max 48 bit. */
>> +                input_size = IPS_MAX;
>> +            else if ( input_size < IPS_MIN )
>> +                /* We limit the input_size to be max 25 bit. */
>> +                input_size = IPS_MIN;
>
> like this could be simplified by using min/max. But I think we should
> bail out here. Likely something in the page table is wrong and
> ignoring is the worst thing to do.
>
> For instance ARMv8.2 has extended the input size to 52 bits. It would
> be difficult to catch what is missing because of that, not mentioning
> that the only caller today will be memaccess that is not enabled by
> default.
>

Agreed.

>> +
>> +            /* Normalize granule size. */
>
> I think 0, 1, 2 is more confusing to read. It would be better to use
> directly TCR_TG0_*.
>

I agree, however the ARM architecture uses different granularity
encodigs for TG0 and TG1. That is the values for (tcr & TCR_TG0_MASK) >>
TCR_TG0_SHIFT are different for the same granularity (e.g. shifted
TCR_TG0_4K == 0x0 vs. TCR_EL1_TG1_4K == 0x2).


Because of this, we won't be able to use TCR_TG0_* and TCR_TG1_*
directly. It would be probably easier to read/review the code if a part
of this functionality would be in a separate function (e.g.
get_granularity()), though. I will see what I can do at this point.

>> +            switch ( tcr & TCR_TG0_MASK ) {
>> +            case TCR_TG0_16K:
>> +                gran = 1;
>> +                break;
>> +            case TCR_TG0_64K:
>> +                gran = 2;
>> +                break;
>> +            default:
>> +                gran = 0;
>> +            } > +
>> +            /* Use TTBR0 for GVA to IPA translation. */
>> +            ttbr = READ_SYSREG64(TTBR0_EL1);
>> +
>> +            /* If TCR.EPD0 is set, translations using TTBR0 are
>> disabled. */
>> +            disabled = ( tcr & TCR_EPD0 ) ? 1 : 0;
>> +        }
>> +        else
>> +        {
>> +            input_size = REGISTER_WIDTH_64_BIT - t1_sz;
>> +
>> +            if ( input_size > IPS_MAX )
>> +                /* We limit the input_size to be max 48 bit. */
>> +                input_size = IPS_MAX;
>> +            else if ( input_size < IPS_MIN )
>> +                /* We limit the input_size to be max 25 bit. */
>> +                input_size = IPS_MIN;
>> +
>> +            /* Normalize granule size. */
>> +            switch ( tcr & TCR_TG1_MASK ) {
>> +            case TCR_TG1_16K:
> If you shift your tcr by TCR_TG1_SHIFT then all this code can become
> generic. Avoiding duplication, reviewing twice similar code and
> potential bug.

Please see my comment above.

[...]

Cheers,
~Sergej


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