Re: [Xen-devel] [PATCH v2] x86/cpu: Add supports for zhaoxin x86 platform

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.04.18 at 11:51, <Davidwang@xxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/cpu/intel_cacheinfo.c
> +++ b/xen/arch/x86/cpu/intel_cacheinfo.c
> @@ -103,7 +103,7 @@ int cpuid4_cache_lookup(int index, struct cpuid4_info 
> *this_leaf)
>       return 0;
>  }
>  
> -static int find_num_cache_leaves(void)
> +int find_num_cache_leaves(void)

Instead of making this function non-static, please consider re-using
init_intel_cacheinfo(): All you want is skip the CPUID leaf 2 handling,
and you'd better to this by altering the single if() controlling it in that
function than by effectively introducing a clone. If you're concerned
of some other dead code in that function, attached you'll find a patch
deleting at least some of that.

> --- /dev/null
> +++ b/xen/arch/x86/cpu/shanghai.c
> @@ -0,0 +1,90 @@
> +#include <xen/bitops.h>
> +#include <xen/init.h>
> +#include <asm/processor.h>
> +#include "cpu.h"
> +
> +void init_shanghai_cache(struct cpuinfo_x86 *c)
> +{
> +     unsigned int i = 0, l1d = 0, l1i = 0, l2 = 0, l3 = 0;
> +    struct cpuid4_info leaf;
> +     static bool is_initialized = false;
> +     static unsigned int cache_leaves = 0;
> +
> +     if ( (!is_initialized) && (c->cpuid_level > 0x00000003) )
> +    {

If there was a convincing argument that this clone of the original
function was really needed, then you'd need to go through here
and clean up style (various aspects of it are broken, most notably
the mix of space and tab indentation).

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

There are numerous further occurrences of X86_VENDOR_INTEL throughout
the code base - is it really the case that no single one of them needs similar
amendment?

Jan

Attachment: x86-init-Intel-cacheinfo.patch
Description: Text document

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