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

[Julien Grall <julien.grall@xxxxxxx>]

On 23/11/16 10:38, Bhupinder Thakur wrote:
> Hi,
Hi Bhupinder,

Can you configure your e-mail client to use ">" to quote the previous 
answer? Using tab makes the mail quite confusing. Thank you.
>          static DECLARE_BITMAP(vmid_mask, MAX_VMID);
>
>          void p2m_vmid_allocator_init(void)
>          {
>         +    unsigned int cpu;
>         +
>              set_bit(INVALID_VMID, vmid_mask);
>         +
>         +    max_vmid = MAX_VMID;
>
>
>     max_vmid is only declared for ARM64 and will break compilation for
>     ARM32. Please try to build each patch for both architecture before
>     sending to the ML.
>
> I will compile for ARM32.
>
>
>     However, in this case. It would make more sense to keep the maximum
>     vmid static for ARM32 as it will never change.
>
>
>     I quite like what has been done for P2M_ROOT_{LEVEL,ORDER} where the
>     define is a constant on ARM32 and point to a variable on ARM64.
>
> I will keep the bitmap statically defined for ARM32. For ARM64 bitmap
> will be allocated dynamically.
I am not asking the bitmap to be static but the number of vmid to be a 
constant (not a variable) for ARM32.
I prefer to have the bitmap dynamic on both architecture because it is 
will be easier to maintain the code in long term.
What I was asking for is:

#ifdef CONFIG_ARM_64
/* The VMID is encoded on 8-bit minimum. */
static unsigned int __read_mostly max_vmid = MAX_VMID_8BIT;
#define MAX_VMID max_vmid
#else
/* The VMID is always encoded on 8-bit */
#define MAX_VMID MAX_VMID_8BIT
#endif

The code would use MAX_VMID and the variable max_vmid would only be 
defined for arm64 breaking the compilation directly if used in the 
common code.
[...]

>          #endif
>
>          #define VTCR_RES1       (_AC(1,UL)<<31)
>         @@ -269,6 +270,11 @@
>          /* FSR long format */
>          #define FSRL_STATUS_DEBUG       (_AC(0x22,UL)<<0)
>
>         +#ifdef CONFIG_ARM_64
>         +#define VMID_8_BITS_SUPPORT         0x0
>         +#define VMID_16_BITS_SUPPORT        0x2
>         +#endif
>
>
>     I would prefix the 2 define with MM64_ so we know how the values
>     will be matched.
>
> You mean I define them like this ?
> #define MM64_VMID_8_BITS_SUPPORT   0x0
> #define MM64_VMID_16_BITS_SUPPORT 0x2
Yes, or any other name mentioning the register name in the define. This 
is how are defined all the register constant within this file.
Regards,

--
Julien Grall

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