On Tue, Jan 17 2023, Richard Henderson <richard.henderson@xxxxxxxxxx> wrote:
> On 1/11/23 06:13, Cornelia Huck wrote:
>> @@ -2136,7 +2136,7 @@ static void machvirt_init(MachineState *machine)
>>
>> if (vms->mte && (kvm_enabled() || hvf_enabled())) {
>> error_report("mach-virt: %s does not support providing "
>> - "MTE to the guest CPU",
>> + "emulated MTE to the guest CPU",
>> kvm_enabled() ? "KVM" : "HVF");
>
> Not your bug, but noticing this should use current_accel_name().
I can fix it as I'm touching the code anyway.
(Hm... two more of these right above. Maybe better in a separate patch.)
>
>> +static inline bool arm_machine_has_tag_memory(void)
>> +{
>> +#ifndef CONFIG_USER_ONLY
>> + Object *obj = object_dynamic_cast(qdev_get_machine(), TYPE_VIRT_MACHINE);
>> +
>> + /* so far, only the virt machine has support for tag memory */
>> + if (obj) {
>> + VirtMachineState *vms = VIRT_MACHINE(obj);
>> +
>> + return vms->mte;
>> + }
>> +#endif
>> + return false;
>> +}
>
> True for CONFIG_USER_ONLY, via page_get_target_data().
> You should have seen check-tcg test failures...
Weird, let me check my setup...
>
>> +void arm_cpu_mte_finalize(ARMCPU *cpu, Error **errp)
>> +{
>> + bool enable_mte;
>> +
>> + switch (cpu->prop_mte) {
>> + case ON_OFF_AUTO_OFF:
>> + enable_mte = false;
>> + break;
>> + case ON_OFF_AUTO_ON:
>> + if (!kvm_enabled()) {
>
> tcg_enabled(), here and everywhere else you test for !kvm.
>
>> +#ifdef CONFIG_KVM
>> + if (kvm_enabled() && !kvm_arm_mte_supported()) {
>
> kvm_arm.h should get a stub inline returning false, so that the ifdef is removed.
> See e.g. kvm_arm_sve_supported().
Oh, I actually did add it already...
>
>> + default: /* AUTO */
>> + if (!kvm_enabled()) {
>
> tcg_enabled.
>
>> + /* accelerator-specific enablement */
>> + if (kvm_enabled()) {
>> +#ifdef CONFIG_KVM
>> + if (kvm_vm_enable_cap(kvm_state, KVM_CAP_ARM_MTE, 0)) {
>> + error_setg(errp, "Failed to enable KVM_CAP_ARM_MTE");
>
> Ideally this ifdef could go away as well.
>
>> + } else {
>> + /* TODO: add proper migration support with MTE enabled */
>> + if (!mte_migration_blocker) {
>
> Move the global variable here, as a local static?
>
> I guess this check is to avoid adding one blocker per cpu?
> I would guess the cap doesn't need enabling more than once either?
Indeed, it's a VM cap. (I only tested this on a single-cpu FVP setup.)
>
>
>> + error_setg(&mte_migration_blocker,
>> + "Live migration disabled due to MTE enabled");
>> + if (migrate_add_blocker(mte_migration_blocker, NULL)) {
>
> You pass NULL to the migrate_add_blocker errp argument...
>
>> + error_setg(errp, "Failed to add MTE migration blocker");
>
> ... then make up your own generic reason for why it failed.
> In this case it seems only related to another command-line option: --only-migratable.
>
>
> Anyway, I wonder about hiding all of this in target/arm/kvm.c:
>
> bool kvm_arm_enable_mte(Error *errp)
> {
> static bool once = false;
> Error *blocker;
>
> if (once) {
> return;
> }
> once = true;
>
> if (kvm_vm_enable_cap(kvm_state, KVM_CAP_ARM_MTE, 0)) {
> error_setg_errno(errp, "Failed to enable KVM_CAP_ARM_MTE");
> return false;
> }
>
> blocker = g_new0(Error);
> error_setg(blocker, "Live migration disabled....");
> return !migrate_add_blocker(blocker, errp);
> }
>
> with
>
> static inline bool kvm_arm_enable_mte(Error *errp)
> {
> g_assert_not_reached();
> }
>
> in the !CONFIG_KVM block in kvm_arm.h.
Good suggestion, I'll give that one a try.
Thanks for the feedback!
