[PATCH 0/3] target/i386/cpu: Misc Cleanup on host-cache-info

[PATCH 0/3] target/i386/cpu: Misc Cleanup on host-cache-info

[Zhao Liu]

Hi,

This series is mainly to addresss Igor's comment about if one check in
host-cache-info could be removed [1], i.e., whether Guest's cache
topology should be self-consistent (able to correspond to Guest's CPU
topology level, as we currently do with the Guest cache topo).

I originally thought (in the mail thread with Igor) that host-cache-info
should allow Guest and Host to have the same topology level information,
e.g. if Host shares cache on core level, then via host-cache-info, Guest
should also share on core level.

But in practice, I gave up on this idea, because in the cache info
passthrough case, it should be possible for Guest to get the original
Host cache info (including the original threads sharing cache) without
further modifying the info to Guest.

Therefore, I simply added the comment in PATCH 3 to hopefully illustrate
the need for such a check.

Hope my explanation is clear enough so that my poor English doesn't
bother you!

[1]: https://lore.kernel.org/qemu-devel/20240527170317.14520a2f@imammedo.users.ipa.redhat.com/

Thanks and Best Regards,
Zhao
---
Zhao Liu (3):
  target/i386/cpu: Use hex mask to check for valid cache CPUID leaf
  target/i386/cpu: Check guest_thread_ids_per_pkg for host-cache-info
    case
  target/i386/cpu: Add comment about adjusting the Guest cache topo for
    host-cache-info

 target/i386/cpu.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

-- 
2.34.1

[PATCH 1/3] target/i386/cpu: Use hex mask to check for valid cache CPUID leaf

[Zhao Liu]

Hexadecimal mask is more intuitive comparing to decimal.

Therefore convert the mask of bits 00-04 to hexadecimal value.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 target/i386/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 365852cb99e1..c4d4048ec32a 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6452,7 +6452,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
              * QEMU has its own number of cores/logical cpus,
              * set 24..14, 31..26 bit to configured values
              */
-            if (*eax & 31) {
+            if (*eax & 0x1f) {
                 int host_vcpus_per_cache = 1 + ((*eax & 0x3FFC000) >> 14);
 
                 *eax &= ~0xFC000000;
-- 
2.34.1

[PATCH 2/3] target/i386/cpu: Check guest_thread_ids_per_pkg for host-cache-info case

[Zhao Liu]

The CPUID[4].EAX[bits 25:14] encodes the "maximum number of addressable
IDs for logical processors", which value may be different with the
actual number of threads.

For example, there's a Guest with the topology like: 3 threads per core
and 3 cores per package. Its maximum ids for package level is 15 (0xf),
but it has 9 threads per package.

Therefore, using "threads_per_pkg" to check sharing threads overflow (out
of package scope) is not sufficient.

Use Guest's maximum ids for package level information to compare with
Host's.

Note the original check is stricter, but it can be mathematically proven
that the original check does not contain redundant case (e.g.
guest_thread_ids_per_pkg >= host_thread_ids_per_cache > threads_per_pkg,
which is impossible for the current QEMU APIC ID encoding rule).

Therefore, the behavior of this feature is consistent before and after
the change.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 target/i386/cpu.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index c4d4048ec32a..c20ff69b7b65 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6453,16 +6453,22 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
              * set 24..14, 31..26 bit to configured values
              */
             if (*eax & 0x1f) {
-                int host_vcpus_per_cache = 1 + ((*eax & 0x3FFC000) >> 14);
+                int host_thread_ids_per_cache;
+                int guest_thread_ids_per_pkg;
 
                 *eax &= ~0xFC000000;
                 *eax |= max_core_ids_in_package(&topo_info) << 26;
-                if (host_vcpus_per_cache > threads_per_pkg) {
+
+                host_thread_ids_per_cache = 1 + ((*eax & 0x3FFC000) >> 14);
+                guest_thread_ids_per_pkg =
+                    max_thread_ids_for_cache(&topo_info,
+                                             CPU_TOPO_LEVEL_PACKAGE);
+
+                if (host_thread_ids_per_cache > guest_thread_ids_per_pkg) {
                     *eax &= ~0x3FFC000;
 
                     /* Share the cache at package level. */
-                    *eax |= max_thread_ids_for_cache(&topo_info,
-                                CPU_TOPO_LEVEL_PACKAGE) << 14;
+                    *eax |= guest_thread_ids_per_pkg << 14;
                 }
             }
         } else if (cpu->vendor_cpuid_only && IS_AMD_CPU(env)) {
-- 
2.34.1

[PATCH 3/3] target/i386/cpu: Add comment about adjusting the Guest cache topo for host-cache-info

[Zhao Liu]

The host-cache-info needs the check to ensure the valid maximum
addressable thread IDs.

We don't need to adjust the information in this one field for all cache
topology cases by default, even though Host's cache topology may not
correspond to Guest's CPU topology level.

For example, when a Geust (3 threads per core) runs on a Host with 1
threads per core, the L2 cache topo (L2 per core on Host) obtained by
Guest does not correspond to the Guest's core level. So for the case
where the topology of Guest and Host are very inconsistent, it is not
possible to do a perfect job, so we try to let the Guest have similar
cache topo info as Host, at least in the case of an even distribution
of vCPUs, which can benefit the Guest internal scheduling.

To this end, add a comment to explain why we need to care for this check
and why we don't need to adjust the topology for all cache cases.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 target/i386/cpu.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index c20ff69b7b65..71300ac6d197 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6463,7 +6463,15 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
                 guest_thread_ids_per_pkg =
                     max_thread_ids_for_cache(&topo_info,
                                              CPU_TOPO_LEVEL_PACKAGE);
-
+                /*
+                 * We handle this case because it causes sharing threads to
+                 * overflow out of the package scope. In other cases, there
+                 * is no need to adjust the cache topology info for the Guest,
+                 * as the Host's maximum addressable thread IDs are not out of
+                 * bounds in the Guest's APIC ID scope, and are always valid,
+                 * even though Host's cache topology may not correspond to
+                 * Guest's CPU topology level.
+                 */
                 if (host_thread_ids_per_cache > guest_thread_ids_per_pkg) {
                     *eax &= ~0x3FFC000;
 
-- 
2.34.1

Re: [PATCH 0/3] target/i386/cpu: Misc Cleanup on host-cache-info

[Zhao Liu]

Hi Igor,

Just a gentle poke and what do you think about this minor series?

Thanks,
Zhao

On Wed, Jun 19, 2024 at 10:42:12PM +0800, Zhao Liu wrote:
> Date: Wed, 19 Jun 2024 22:42:12 +0800
> From: Zhao Liu <zhao1.liu@intel.com>
> Subject: [PATCH 0/3] target/i386/cpu: Misc Cleanup on host-cache-info
> X-Mailer: git-send-email 2.34.1
> 
> Hi,
> 
> This series is mainly to addresss Igor's comment about if one check in
> host-cache-info could be removed [1], i.e., whether Guest's cache
> topology should be self-consistent (able to correspond to Guest's CPU
> topology level, as we currently do with the Guest cache topo).
> 
> I originally thought (in the mail thread with Igor) that host-cache-info
> should allow Guest and Host to have the same topology level information,
> e.g. if Host shares cache on core level, then via host-cache-info, Guest
> should also share on core level.
> 
> But in practice, I gave up on this idea, because in the cache info
> passthrough case, it should be possible for Guest to get the original
> Host cache info (including the original threads sharing cache) without
> further modifying the info to Guest.
> 
> Therefore, I simply added the comment in PATCH 3 to hopefully illustrate
> the need for such a check.
> 
> Hope my explanation is clear enough so that my poor English doesn't
> bother you!
> 
> [1]: https://lore.kernel.org/qemu-devel/20240527170317.14520a2f@imammedo.users.ipa.redhat.com/
> 
> Thanks and Best Regards,
> Zhao
> ---
> Zhao Liu (3):
>   target/i386/cpu: Use hex mask to check for valid cache CPUID leaf
>   target/i386/cpu: Check guest_thread_ids_per_pkg for host-cache-info
>     case
>   target/i386/cpu: Add comment about adjusting the Guest cache topo for
>     host-cache-info
> 
>  target/i386/cpu.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> -- 
> 2.34.1
> 
> 

Re: [PATCH 0/3] target/i386/cpu: Misc Cleanup on host-cache-info

[Zhao Liu]

Hi Igor,

Just a friendly ping :-)

May I ask if you are satisfied with the clarification in this series?

Thanks,
Zhao

On Mon, Jul 08, 2024 at 10:23:26AM +0800, Zhao Liu wrote:
> Date: Mon, 8 Jul 2024 10:23:26 +0800
> From: Zhao Liu <zhao1.liu@intel.com>
> Subject: Re: [PATCH 0/3] target/i386/cpu: Misc Cleanup on host-cache-info
> 
> Hi Igor,
> 
> Just a gentle poke and what do you think about this minor series?
> 
> Thanks,
> Zhao
> 
> On Wed, Jun 19, 2024 at 10:42:12PM +0800, Zhao Liu wrote:
> > Date: Wed, 19 Jun 2024 22:42:12 +0800
> > From: Zhao Liu <zhao1.liu@intel.com>
> > Subject: [PATCH 0/3] target/i386/cpu: Misc Cleanup on host-cache-info
> > X-Mailer: git-send-email 2.34.1
> > 
> > Hi,
> > 
> > This series is mainly to addresss Igor's comment about if one check in
> > host-cache-info could be removed [1], i.e., whether Guest's cache
> > topology should be self-consistent (able to correspond to Guest's CPU
> > topology level, as we currently do with the Guest cache topo).
> > 
> > I originally thought (in the mail thread with Igor) that host-cache-info
> > should allow Guest and Host to have the same topology level information,
> > e.g. if Host shares cache on core level, then via host-cache-info, Guest
> > should also share on core level.
> > 
> > But in practice, I gave up on this idea, because in the cache info
> > passthrough case, it should be possible for Guest to get the original
> > Host cache info (including the original threads sharing cache) without
> > further modifying the info to Guest.
> > 
> > Therefore, I simply added the comment in PATCH 3 to hopefully illustrate
> > the need for such a check.
> > 
> > Hope my explanation is clear enough so that my poor English doesn't
> > bother you!
> > 
> > [1]: https://lore.kernel.org/qemu-devel/20240527170317.14520a2f@imammedo.users.ipa.redhat.com/
> > 
> > Thanks and Best Regards,
> > Zhao
> > ---
> > Zhao Liu (3):
> >   target/i386/cpu: Use hex mask to check for valid cache CPUID leaf
> >   target/i386/cpu: Check guest_thread_ids_per_pkg for host-cache-info
> >     case
> >   target/i386/cpu: Add comment about adjusting the Guest cache topo for
> >     host-cache-info
> > 
> >  target/i386/cpu.c | 24 +++++++++++++++++++-----
> >  1 file changed, 19 insertions(+), 5 deletions(-)
> > 
> > -- 
> > 2.34.1
> > 
> > 
>