Re: [Xen-devel] [PATCH 01/15] x86/cpu: Create Hygon Dhyana architecture support file

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Dec 26, 2018 at 07:42:14PM +0800, Pu Wen wrote:
> On 2018/12/21 18:20, Andrew Cooper wrote:
> >> +  /* Hygon CPUs do not support SYSENTER outside of legacy mode. */
> >> +  __clear_bit(X86_FEATURE_SEP, c->x86_capability);
> >> +
> >> +  /* Hygon processors have APIC timer running in deep C states. */
> >> +  if ( opt_arat )
> >> +          __set_bit(X86_FEATURE_ARAT, c->x86_capability);
> >> +
> >> +  if (cpu_has(c, X86_FEATURE_EFRO)) {
> >> +          rdmsr(MSR_K7_HWCR, l, h);
> >> +          l |= (1 << 27); /* Enable read-only APERF/MPERF bit */
> >> +          wrmsr(MSR_K7_HWCR, l, h);
> >> +  }
> >
> > Is there anything which is actually unique to Hygon here?  I ask,
> > because this looks like a lot of duplicate code, considering that the
> > processor base is the same.
> 
> Right now these codes are necessary for Hygon Dhyana processor even though
> they are duplicated. As Hygon Dhyana support many CPU features such as ITSC
> and EFRO, so I think the "if cpu_has" determine should be removed to make
> the code clear and essential.

What if you later add new CPUs that have a different set of features?
You will have to change this code to use cpu_has again in order to
cope with different models having different features.

Using cpu_has is IMO the correct approach, even tough if you are
targeting a single CPU model ATM.

> Keeping the codes into a separate compilation unit(hygon.c) at least has
> two advantages:
> 1) Make the code flow more clear. Hygon is a new joint venture which has no
>    historical old architectures, so I'm afraid that there are sufficient
>    motivations to keep a clear new processor init flow.

AFAICT the code that you add is mostly a reduced copy of AMD's code,
so from a code PoV it doesn't matter much whether Hygon is a new
company or not, if the code is the same it should be shared.

Adding a new compilation unit is fine, but you should make the AMD
functions that you need to use global and call them from your hygon.c
file.

> 2) Beneficial for the future maintaining. AMD and Hygon may maintain their
>    respective architecture related codes with no interaction with each
>    other.

But likely bugs in code or erratum that affect AMD's family 17h are
going to affect Hygon CPU, in which case you want to share the code so
that fixes done by AMD or Hygon will be shared.

Thanks, Roger.

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