Re: [PATCH v2 3/4] target/i386: Introduce Zhaoxin Shijidadao-Client CPU model

[Zhao Liu <zhao1.liu@intel.com>]

On Tue, Nov 25, 2025 at 04:57:04PM +0800, Ewan Hai wrote:
> Date: Tue, 25 Nov 2025 16:57:04 +0800
> From: Ewan Hai <ewanhai-oc@zhaoxin.com>
> Subject: Re: [PATCH v2 3/4] target/i386: Introduce Zhaoxin
>  Shijidadao-Client CPU model
> 
> On 11/25/25 3:35 PM, Zhao Liu wrote:
> > 
> > 
> >> +        /*
> >> +         * TODO: When the Linux kernel introduces other existing definitions
> >> +         * for this leaf, remember to update the definitions here.
> >> +         */
> > 
> > This TODO seems a bit vague; it's best to explicitly list the existing
> > features that are currently missing. Otherwise, maintainers won't be
> > able to understand or clean up this TODO either.
> > 
> 
> I agree. The same problem also exists in the YongFeng vCPU model. For this
> series, I can drop the vague TODO and instead add a more explicit comment that
> documents which CPUID.C000_0001.EDX bits are intentionally missing today. In
> addition, I can post a small follow-up cleanup patch to fix the YongFeng model
> in the same way, so the two Zhaoxin models stay consistent. If you prefer, I can
> also fold the YongFeng comment update into this series as an extra patch.

Yes, it's good to make everything clear and I think it's better to
include your extra patch into this series to help maintainer review/pick
in one goes.

> As background, current Zhaoxin CPUs implement several CPUID.(EAX=0xC0000001,
> ECX=0):EDX feature bits that are not yet defined in the Linux kernel, for
> example SM2/SM2_EN, SM3/SM4 and their enable bits, PARALLAX/PARALLAX_EN,
> TM3/TM3_EN, RNG2/RNG2_EN, PHE2/PHE2_EN, and RSA/RSA_EN.
> 
> We previously tried to upstream all these extra feature bits in one
> patch(https://lore.kernel.org/all/20230414095334.8743-1-TonyWWang-oc@zhaoxin.com/),
> but the maintainer rejected it because there was no in-tree code using these
> features yet. So our current plan is to add the CPUID bits together with real
> kernel users step by step.

I see. I think it's enough to document missing CPUIDs in comment.

...

> > (Based on my personal experience, the absence of SMAP seems a bit
> > odd. Could it be a hardware bug in a specific stepping?)
> > 
> 
> This is not a stepping-specific silicon bug. For this product family, SMAP
> support was intentionally not enabled in the final product because our internal
> performance evaluation showed an unacceptable performance impact in certain
> workloads. The v2 CPU model therefore keeps "smap" off to reflect the actual
> shipped behavior, while the v1 definition with SMAP enabled is kept for
> customers who need to model early v1 silicon where SMAP is still available.

v1 is not the final product, then I think it's not necessary to upstream
it. For example, these Intel CPU models are basically all targeted at
the final products. But unluckily, engineering samples may have bugs so
we have to add or remove features based on what the final products
support. So if the final product is clear from the beginning, there's no
need to take intermediate steps.

BTW, even with v2, user can still enable smap by +smap.

> >> +                .props = (PropValue[]) {
> >> +                    { "xsavec", "on" },
> >> +                    { "xgetbv1", "on" },
> >> +                    { "xsaves", "on"},
> >> +                    { "vmx-xsaves", "on"},
> >> +                    { "smap", "off" },
> >> +                    { /* end of list */ }
> >> +                },
> >> +            },
> > 
> > BTW, if the differences aren't too significant, is it possible to merge
> > the server and client models? :)
> > 
> 
> From the user point of view, I slightly prefer keeping separate
> Shijidadao-Client and Shijidadao-Server models.
> 
> The main reason is that customers who want a "full-feature" vCPU that behaves
> very close to a specific physical product can simply pick the corresponding
> model name, without having to remember a set of extra "-cpu ..., +-feature"
> overrides. If we merge everything into a single Shijidadao model that
> corresponds to a more restricted baseline, users who want the full configuration
> would need to explicitly enable multiple features (such as the additional XSAVE
> bits) on the command line, which is easier to get wrong and less user-friendly.

Could we make Shijidadao-Client as a v2 of Shijidadao-Server, and create an
alias for this v2?

.alias = "Shijidadao-Client"

Then we could rename Shijidadao-Server to Shijidadao, and its v2 is for
client.

> This is also aligned with how QEMU models other vendors' micro-architectures
> where client and server products have slightly different feature sets.

The main use case for CPU models is to easy migration across mixed CPU
clusters [*]. So, IMO, not all products require a model.

[*]: https://lore.kernel.org/qemu-devel/ZJrJYG1dICHjKx09@redhat.com/

> Thanks again for the review!

Thanks for your patience and sorry for my late reply.

Regards,
Zhao