diff mbox series

[RFC,5/9] target/arm: introduce CPU feature dependency mechanism

Message ID 20200813102657.2588720-6-liangpeng10@huawei.com (mailing list archive)
State New, archived
Headers show
Series Support disable/enable CPU features for AArch64 | expand

Commit Message

Peng Liang Aug. 13, 2020, 10:26 a.m. UTC 
Some CPU features are dependent on other CPU features.  For example,
ID_AA64PFR0_EL1.FP field and ID_AA64PFR0_EL1.AdvSIMD must have the same
value, which means FP and ADVSIMD are dependent on each other, FPHP and
ADVSIMDHP are dependent on each other.

This commit introduces a mechanism for CPU feature dependency in
AArch64.  We build a directed graph from the CPU feature dependency
relationship, each edge from->to means the `to` CPU feature is dependent
on the `from` CPU feature.  And we will automatically enable/disable CPU
feature according to the directed graph.

For example, a, b, and c CPU features are in relationship a->b->c, which
means c is dependent on b and b is dependent on a.  If c is enabled by
user, then a and b is enabled automatically.  And if a is disabled by
user, then b and c is disabled automatically.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Peng Liang <liangpeng10@huawei.com>
---
 target/arm/cpu.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 129 insertions(+)

Comments

Andrew Jones Aug. 13, 2020, 12:48 p.m. UTC | #1 
On Thu, Aug 13, 2020 at 06:26:53PM +0800, Peng Liang wrote:
> Some CPU features are dependent on other CPU features.  For example,
> ID_AA64PFR0_EL1.FP field and ID_AA64PFR0_EL1.AdvSIMD must have the same
> value, which means FP and ADVSIMD are dependent on each other, FPHP and
> ADVSIMDHP are dependent on each other.
> 
> This commit introduces a mechanism for CPU feature dependency in
> AArch64.  We build a directed graph from the CPU feature dependency
> relationship, each edge from->to means the `to` CPU feature is dependent
> on the `from` CPU feature.  And we will automatically enable/disable CPU
> feature according to the directed graph.
> 
> For example, a, b, and c CPU features are in relationship a->b->c, which
> means c is dependent on b and b is dependent on a.  If c is enabled by
> user, then a and b is enabled automatically.  And if a is disabled by
> user, then b and c is disabled automatically.

And what if a is mutually exclusive with b? I.e. a and b can both be
disabled, but only a or b may be enabled.

Thanks,
drew
Peng Liang Aug. 15, 2020, 2:19 a.m. UTC | #2 
On 8/13/2020 8:48 PM, Andrew Jones wrote:
> On Thu, Aug 13, 2020 at 06:26:53PM +0800, Peng Liang wrote:
>> Some CPU features are dependent on other CPU features.  For example,
>> ID_AA64PFR0_EL1.FP field and ID_AA64PFR0_EL1.AdvSIMD must have the same
>> value, which means FP and ADVSIMD are dependent on each other, FPHP and
>> ADVSIMDHP are dependent on each other.
>>
>> This commit introduces a mechanism for CPU feature dependency in
>> AArch64.  We build a directed graph from the CPU feature dependency
>> relationship, each edge from->to means the `to` CPU feature is dependent
>> on the `from` CPU feature.  And we will automatically enable/disable CPU
>> feature according to the directed graph.
>>
>> For example, a, b, and c CPU features are in relationship a->b->c, which
>> means c is dependent on b and b is dependent on a.  If c is enabled by
>> user, then a and b is enabled automatically.  And if a is disabled by
>> user, then b and c is disabled automatically.
> 
> And what if a is mutually exclusive with b? I.e. a and b can both be
> disabled, but only a or b may be enabled.
> 
> Thanks,
> drew
> 
> .
> 

Currently, a and b will be both enabled or disabled.  For example, a and b are
in relationship a->b, which means b is dependent on a.  If -cpu host,a=off,b=on,
then both a and b are enabled.  If -cpu host,b=on,a=off, then both a and b are
disabled.  Maybe we should report an error to user in this scenario?

Thanks,
Peng
Andrew Jones Aug. 15, 2020, 6:59 a.m. UTC | #3 
On Sat, Aug 15, 2020 at 10:19:27AM +0800, Peng Liang wrote:
> On 8/13/2020 8:48 PM, Andrew Jones wrote:
> > On Thu, Aug 13, 2020 at 06:26:53PM +0800, Peng Liang wrote:
> >> Some CPU features are dependent on other CPU features.  For example,
> >> ID_AA64PFR0_EL1.FP field and ID_AA64PFR0_EL1.AdvSIMD must have the same
> >> value, which means FP and ADVSIMD are dependent on each other, FPHP and
> >> ADVSIMDHP are dependent on each other.
> >>
> >> This commit introduces a mechanism for CPU feature dependency in
> >> AArch64.  We build a directed graph from the CPU feature dependency
> >> relationship, each edge from->to means the `to` CPU feature is dependent
> >> on the `from` CPU feature.  And we will automatically enable/disable CPU
> >> feature according to the directed graph.
> >>
> >> For example, a, b, and c CPU features are in relationship a->b->c, which
> >> means c is dependent on b and b is dependent on a.  If c is enabled by
> >> user, then a and b is enabled automatically.  And if a is disabled by
> >> user, then b and c is disabled automatically.
> > 
> > And what if a is mutually exclusive with b? I.e. a and b can both be
> > disabled, but only a or b may be enabled.
> > 
> > Thanks,
> > drew
> > 
> > .
> > 
> 
> Currently, a and b will be both enabled or disabled.  For example, a and b are
> in relationship a->b, which means b is dependent on a.  If -cpu host,a=off,b=on,
> then both a and b are enabled.  If -cpu host,b=on,a=off, then both a and b are
> disabled.  Maybe we should report an error to user in this scenario?
>

Right. There are more relationships between features than "depends on",
such as "only if not" or "only if value is". The last one points out
that just setting the minimum feature value may not be sufficient to
control all the features. Also, there could be relationships involving
more than two features, such as 'a iff b and c', or 'a iff b and !c'.

We really have to take each feature of each ID register one at a time to
make sure we handle them appropriately. Exposing them all like this
without any checks just pushes all the pain onto the user to figure
everything out, and if there's not even errors generated, then how will
the user know when they got something wrong until their guest breaks
in some mysterious way?

Thanks,
drew
diff mbox series

Patch

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 113cf4a9e7..4e67b8f22c 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1418,6 +1418,103 @@  static struct CPUFeatureInfo cpu_features[] = {
     },
 };
 
+typedef struct CPUFeatureDep {
+    CPUFeatureInfo from, to;
+} CPUFeatureDep;
+
+static const CPUFeatureDep feature_dependencies[] = {
+    {
+        .from = FIELD_INFO(ID_AA64PFR0, FP, true, 0, 0xf, false),
+        .to = FIELD_INFO(ID_AA64PFR0, ADVSIMD, true, 0, 0xf, false),
+    },
+    {
+        .from = FIELD_INFO(ID_AA64PFR0, ADVSIMD, true, 0, 0xf, false),
+        .to = FIELD_INFO(ID_AA64PFR0, FP, true, 0, 0xf, false),
+    },
+    {
+        .from = {
+            .reg = ID_AA64PFR0, .length = R_ID_AA64PFR0_FP_LENGTH,
+            .shift = R_ID_AA64PFR0_FP_SHIFT, .sign = true, .min_value = 1,
+            .ni_value = 0, .name = "FPHP", .is_32bit = false,
+        },
+        .to = {
+            .reg = ID_AA64PFR0, .length = R_ID_AA64PFR0_ADVSIMD_LENGTH,
+            .shift = R_ID_AA64PFR0_ADVSIMD_SHIFT, .sign = true, .min_value = 1,
+            .ni_value = 0, .name = "ADVSIMDHP", .is_32bit = false,
+        },
+    },
+    {
+        .from = {
+            .reg = ID_AA64PFR0, .length = R_ID_AA64PFR0_ADVSIMD_LENGTH,
+            .shift = R_ID_AA64PFR0_ADVSIMD_SHIFT, .sign = true, .min_value = 1,
+            .ni_value = 0, .name = "ADVSIMDHP", .is_32bit = false,
+        },
+        .to = {
+            .reg = ID_AA64PFR0, .length = R_ID_AA64PFR0_FP_LENGTH,
+            .shift = R_ID_AA64PFR0_FP_SHIFT, .sign = true, .min_value = 1,
+            .ni_value = 0, .name = "FPHP", .is_32bit = false,
+        },
+    },
+    {
+
+        .from = FIELD_INFO(ID_AA64ISAR0, AES, false, 1, 0, false),
+        .to = {
+            .reg = ID_AA64ISAR0, .length = R_ID_AA64ISAR0_AES_LENGTH,
+            .shift = R_ID_AA64ISAR0_AES_SHIFT, .sign = false, .min_value = 2,
+            .ni_value = 1, .name = "PMULL", .is_32bit = false,
+        },
+    },
+    {
+
+        .from = FIELD_INFO(ID_AA64ISAR0, SHA2, false, 1, 0, false),
+        .to = {
+            .reg = ID_AA64ISAR0, .length = R_ID_AA64ISAR0_SHA2_LENGTH,
+            .shift = R_ID_AA64ISAR0_SHA2_SHIFT, .sign = false, .min_value = 2,
+            .ni_value = 1, .name = "SHA512", .is_32bit = false,
+        },
+    },
+    {
+        .from = FIELD_INFO(ID_AA64ISAR1, LRCPC, false, 1, 0, false),
+        .to = {
+            .reg = ID_AA64ISAR1, .length = R_ID_AA64ISAR1_LRCPC_LENGTH,
+            .shift = R_ID_AA64ISAR1_LRCPC_SHIFT, .sign = false, .min_value = 2,
+            .ni_value = 1, .name = "ILRCPC", .is_32bit = false,
+        },
+    },
+    {
+        .from = FIELD_INFO(ID_AA64ISAR0, SM3, false, 1, 0, false),
+        .to = FIELD_INFO(ID_AA64ISAR0, SM4, false, 1, 0, false),
+    },
+    {
+        .from = FIELD_INFO(ID_AA64ISAR0, SM4, false, 1, 0, false),
+        .to = FIELD_INFO(ID_AA64ISAR0, SM3, false, 1, 0, false),
+    },
+    {
+        .from = FIELD_INFO(ID_AA64ISAR0, SHA1, false, 1, 0, false),
+        .to = FIELD_INFO(ID_AA64ISAR0, SHA2, false, 1, 0, false),
+    },
+    {
+        .from = FIELD_INFO(ID_AA64ISAR0, SHA1, false, 1, 0, false),
+        .to = FIELD_INFO(ID_AA64ISAR0, SHA3, false, 1, 0, false),
+    },
+    {
+        .from = FIELD_INFO(ID_AA64ISAR0, SHA3, false, 1, 0, false),
+        .to = {
+            .reg = ID_AA64ISAR0, .length = R_ID_AA64ISAR0_SHA2_LENGTH,
+            .shift = R_ID_AA64ISAR0_SHA2_SHIFT, .sign = false, .min_value = 2,
+            .ni_value = 1, .name = "SHA512", .is_32bit = false,
+        },
+    },
+    {
+        .from = {
+            .reg = ID_AA64ISAR0, .length = R_ID_AA64ISAR0_SHA2_LENGTH,
+            .shift = R_ID_AA64ISAR0_SHA2_SHIFT, .sign = false, .min_value = 2,
+            .ni_value = 1, .name = "SHA512", .is_32bit = false,
+        },
+        .to = FIELD_INFO(ID_AA64ISAR0, SHA3, false, 1, 0, false),
+    },
+};
+
 static void arm_cpu_get_bit_prop(Object *obj, Visitor *v, const char *name,
                                  void *opaque, Error **errp)
 {
@@ -1454,13 +1551,45 @@  static void arm_cpu_set_bit_prop(Object *obj, Visitor *v, const char *name,
     }
 
     if (value) {
+        if (object_property_get_bool(obj, feat->name, NULL)) {
+            return;
+        }
         isar->regs[feat->reg] = deposit64(isar->regs[feat->reg],
                                           feat->shift, feat->length,
                                           feat->min_value);
+        /* Auto enable the features which current feature is dependent on. */
+        for (int i = 0; i < ARRAY_SIZE(feature_dependencies); ++i) {
+            const CPUFeatureDep *d = &feature_dependencies[i];
+            if (strcmp(d->to.name, feat->name) != 0) {
+                continue;
+            }
+
+            object_property_set_bool(obj, d->from.name, true, &local_err);
+            if (local_err) {
+                error_propagate(errp, local_err);
+                return;
+            }
+        }
     } else {
+        if (!object_property_get_bool(obj, feat->name, NULL)) {
+            return;
+        }
         isar->regs[feat->reg] = deposit64(isar->regs[feat->reg],
                                           feat->shift, feat->length,
                                           feat->ni_value);
+        /* Auto disable the features which are dependent on current feature. */
+        for (int i = 0; i < ARRAY_SIZE(feature_dependencies); ++i) {
+            const CPUFeatureDep *d = &feature_dependencies[i];
+            if (strcmp(d->from.name, feat->name) != 0) {
+                continue;
+            }
+
+            object_property_set_bool(obj, d->to.name, false, &local_err);
+            if (local_err) {
+                error_propagate(errp, local_err);
+                return;
+            }
+        }
     }
 }