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

[David Wang <DavidWang@xxxxxxxxxxx>]

Hi Jan,
    Thanks for your reply. Answer as following.
________________________________________
发件人: Jan Beulich <JBeulich@xxxxxxxx>
发送时间: 2018年4月30日 22:15
收件人: David Wang
抄送: xen-devel; Fiona Li(BJ-RD)
主题: Re: [PATCH v2] x86/cpu: Add supports for zhaoxin x86 platform

>>> 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.
[David]: Concerned the dead code in init_intel_cacheinfo(), I rewrite it. 
Thanks for your patch, i will re-using init_intel_cacheinfo() in next version.

> --- /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).
[David]: Sorry for my mistake.

> --- 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?
[David]: Yes, there are numerous occurrences of X86_VENDOR_INTEL, such as 
supporting idle_nops in arch_init_ideal_nops() or vpmu in 
vpmu_arch_initialise(). Some of them perfect function, others improve 
performance.   Can we perfect those by submitting separate patches? To support 
the iommu,  we need to re-use intel_iommu_ops() and intel_vtd_setup(). 

David


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