Re: [Xen-devel] [PATCH] x86/cpu: Support a new cpu vendor, which is Shanghai

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.03.18 at 12:28, <lifang110@xxxxxxx> wrote:
> From: FionaLi <FionaLi@xxxxxxxxxxx>
> 
> Signed-off-by: Fiona Li<fionali@xxxxxxxxxxx>

First of all, please shorten the subject and put a fair part of what
you had there in the description.

Then you talk about a VT-d compatible IOMMU, but not about VMX
or some other CPU side hardware virtualization. Is that really not
available?

Further it would help if the mail address you send from was in
sync (or at least allow some matching with) the one in the From
and S-o-b.

> --- a/xen/arch/x86/cpu/Makefile
> +++ b/xen/arch/x86/cpu/Makefile
> @@ -5,6 +5,7 @@ obj-y += amd.o
>  obj-y += centaur.o
>  obj-y += common.o
>  obj-y += intel.o
> +obj-y += shanghai.o

Please put where it belongs alphabetically.

> --- /dev/null
> +++ b/xen/arch/x86/cpu/shanghai.c
> @@ -0,0 +1,61 @@
> +#include <xen/lib.h>
> +#include <xen/init.h>
> +#include <xen/bitops.h>
> +#include <asm/processor.h>
> +#include <asm/msr.h>
> +#include <asm/e820.h>
> +#include "cpu.h"
> +
> +#define ACE_PRESENT(x)  ((x)&(1U<<6))
> +#define ACE_ENABLED(x)  ((x)&(1U<<7))
> +#define ACE_FCR              (1U << 28)      /* MSR_ZX_ACE Advanced 
> Cryprography Engine */
> +
> +#define RNG_PRESENT(x)  ((x)&(1U<<6))
> +#define RNG_ENABLED(x)  ((x)&(1U<<7))
> +#define RNG_ENABLE   (1U << 6)       /* MSR_ZX_RNG Random Number Generator */

Style: Blanks around binary operators please.

> +
> +
> +

Please don't put multiple consecutive blank lines anywhere.

> +static void init_shanghai(struct cpuinfo_x86 *c)
> +{
> +     uint64_t msr_ace,msr_rng;
> +     /* Test for Shanghai Extended CPUID information */
> +     if (cpuid_eax(0xC0000000) >= 0xC0000001) {
> +             /*Get Shanghai Extended function number */
> +             u32 extented_feature_flags = cpuid_edx(0xC0000001);
> +
> +             /* enable ACE,if support ACE unit */
> +             if(ACE_PRESENT(extented_feature_flags) && 
> !ACE_ENABLED(extented_feature_flags)) {
> +                     rdmsrl(MSR_ZX_ACE, msr_ace);
> +                     /* enable ACE  */
> +                     wrmsrl(MSR_ZX_ACE, (msr_ace | ACE_FCR));
> +                     printk(KERN_INFO "CPU: Enabled ACE h/w crypto\n");
> +             }
> +             /* enable RNG,if support RNG unit */
> +             if (RNG_PRESENT(extented_feature_flags) && 
> !RNG_ENABLED(extented_feature_flags)) {
> +                     rdmsrl(MSR_ZX_RNG, msr_rng);
> +                     /* enable RNG  */
> +                     wrmsrl(MSR_ZX_RNG, msr_rng | RNG_ENABLE);
> +                     printk(KERN_INFO "CPU: Enabled h/w RNG\n");
> +             }
> +     }
> +
> +     if (c->x86 == 0x6 && c->x86_model >= 0xf) {
> +             c->x86_cache_alignment = c->x86_clflush_size * 2;
> +             __set_bit(X86_FEATURE_CONSTANT_TSC, c->x86_capability);
> +     }

Is there a specification available anywhere for all of the above?

What about guests? How would they know these extensions are
available for their use?

> --- a/xen/include/asm-x86/iommu.h
> +++ b/xen/include/asm-x86/iommu.h
> @@ -53,6 +53,7 @@ static inline const struct iommu_ops *iommu_get_ops(void)
>  {
>      switch ( boot_cpu_data.x86_vendor )
>      {
> +    case X86_VENDOR_SHANGHAI:
>      case X86_VENDOR_INTEL:
>          return &intel_iommu_ops;
>      case X86_VENDOR_AMD:
> @@ -68,6 +69,7 @@ static inline int iommu_hardware_setup(void)
>  {
>      switch ( boot_cpu_data.x86_vendor )
>      {
> +    case X86_VENDOR_SHANGHAI:
>      case X86_VENDOR_INTEL:
>          return intel_vtd_setup();
>      case X86_VENDOR_AMD:

Please don't put new entries first.

> --- a/xen/include/asm-x86/msr-index.h
> +++ b/xen/include/asm-x86/msr-index.h
> @@ -293,6 +293,10 @@
>  #define MSR_TMTA_LRTI_READOUT                0x80868018
>  #define MSR_TMTA_LRTI_VOLT_MHZ               0x8086801a
>  
> +/* Shanghai ZhaoXin defined MSRs*/
> +#define MSR_ZX_ACE                   0x00001107
> +#define MSR_ZX_RNG                   0x0000110b

As Wei has already indicated, we'd prefer consistent names.
Either ZX / ZhaoXin everywhere, or Shanghai. If one of them is
just a code name, the permanent one would obviously better.
This extends to ...

> --- a/xen/include/asm-x86/setup.h
> +++ b/xen/include/asm-x86/setup.h
> @@ -22,6 +22,7 @@ int amd_init_cpu(void);
>  int cyrix_init_cpu(void);
>  int nsc_init_cpu(void);
>  int centaur_init_cpu(void);
> +int shanghai_init_cpu(void);

... this and ...

> --- a/xen/include/asm-x86/x86-vendors.h
> +++ b/xen/include/asm-x86/x86-vendors.h
> @@ -7,7 +7,8 @@
>  #define X86_VENDOR_INTEL 0
>  #define X86_VENDOR_AMD 1
>  #define X86_VENDOR_CENTAUR 2
> -#define X86_VENDOR_NUM 3
> +#define X86_VENDOR_SHANGHAI 3

... this (alongside the file name chosen).

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel