Re: [Xen-devel] [XEN VMID PATCH 2/2 v4] xen/arm: Add support for 16 bit VMIDs

[Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>]

Hi Julien,


>> -#define MAX_VMID 256
>> -#define INVALID_VMID 0 /* VMID 0 is reserved */
>>
>>  static spinlock_t vmid_alloc_lock = SPIN_LOCK_UNLOCKED;
>>
>>  /*
>> - * VTTBR_EL2 VMID field is 8 bits. Using a bitmap here limits us to
>> - * 256 concurrent domains.
>> + * VTTBR_EL2 VMID field is 8 or 16 bits. Aarch64 supports 16-bit VMID.
>
>
> s/Aarch64/AArch64/

ok.

>
> Also I would say "may support" because this is not true for all AArch64
> platform.
>
>> + * Using a bitmap here limits us to 256 or 65536 (for Aarch64) concurrent
>
>
> s/Aarch64/AArch64/
>
ok.

>> + * domains. The bitmap space will be allocated dynamically based on
>> + * whether 8 or 16 bit VMIDs are supported.
>
>
> I am wondering if it would be better to comment #define MAX_VMID instead.
> E.g
>
> /* VMID is by default 8 bit width on AArch64 */
> static unsigned int max_vmid = ....;
> #define MAX_VMID        max_vmid
>
> /* VMID is always 8 bit width on AArch32 */
> #define MAX_VMID        MAX_VMID_8_BIT
>
> What do you think?
Looks fine. I will modify.

>
>>   */
>> -static DECLARE_BITMAP(vmid_mask, MAX_VMID);
>> +static unsigned long *vmid_mask;
>>
>>  static void p2m_vmid_allocator_init(void)
>>  {
>> +    /*
>> +     * allocate space for vmid_mask based on MAX_VMID
>> +     */
>> +    vmid_mask = xzalloc_array(unsigned long, BITS_TO_LONGS(MAX_VMID));
>> +
>> +    if ( !vmid_mask )
>> +        panic("Could not allocate VMID bitmap space");
>> +
>>      set_bit(INVALID_VMID, vmid_mask);
>>  }
>>
>> @@ -1632,20 +1649,36 @@ void __init setup_virt_paging(void)
>>
>>      unsigned int cpu;
>>      unsigned int pa_range = 0x10; /* Larger than any possible value */
>> +    bool     vmid_8_bit = false;
>
>
> Only one space between bool and vmid_8_bit please.
>
ok.

>>
>>      for_each_online_cpu ( cpu )
>>      {
>>          const struct cpuinfo_arm *info = &cpu_data[cpu];
>>          if ( info->mm64.pa_range < pa_range )
>>              pa_range = info->mm64.pa_range;
>> +
>> +        /* set a flag if the current cpu does not suppot 16 bit VMIDs */
>
>
> s/set/Set/
> s/suppot/support/
>
> Please also add a full stop at the end of the sentence.
>
ok.

>
>> +        if ( info->mm64.vmid_bits != MM64_VMID_16_BITS_SUPPORT )
>> +            vmid_8_bit = true;
>>      }
>>
>> +    /*
>> +     * if the flag is not set then it means all CPUs support 16-bit
>
>
> s/if/If/
>
ok.

>> +     * VMIDs.
>> +     */
>> +    if ( !vmid_8_bit )
>> +        max_vmid = MAX_VMID_16_BIT;
>> +
>>      /* pa_range is 4 bits, but the defined encodings are only 3 bits */
>>      if ( pa_range&0x8 || !pa_range_info[pa_range].pabits )
>>          panic("Unknown encoding of ID_AA64MMFR0_EL1.PARange %x\n",
>> pa_range);
>
>
> This code has changed in xen upstream and the platform will unlikely apply.
> Please rebase your code on staging.
>
I checked with the staging branch. This code is same. I re-based my
code on staging branch but there is no change in this code fragment.

>>
>>      val |= VTCR_PS(pa_range);
>>      val |= VTCR_TG0_4K;
>> +
>> +    /* set the VS bit only if 16 bit VMID is supported */
>
>
> s/set/Set/ + full stop
ok.

>
>> +    if ( MAX_VMID == MAX_VMID_16_BIT )
>> +        val |= VTCR_VS;
>
>
> Sorry, I have only spotted until now. Can you print a message to advertise
> the width of VMID?
>
> See the printk("P2M: %d-bit levels...");
>
ok. I will add the VMID size in this printk.

>>      val |= VTCR_SL0(pa_range_info[pa_range].sl0);
>>      val |= VTCR_T0SZ(pa_range_info[pa_range].t0sz);
>
>
> Cheers,
>
> --
> Julien Grall

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