Hi Connie,
On 6/14/22 10:40, Cornelia Huck wrote:
> On Fri, Jun 10 2022, Eric Auger <eauger@xxxxxxxxxx> wrote:
>
>> Hi Connie,
>> On 5/12/22 15:11, Cornelia Huck wrote:
>>> We need to disable migration, as we do not yet have a way to migrate
>>> the tags as well.
>>
>> This patch does much more than adding a migration blocker ;-) you may
>> describe the new cpu option and how it works.
>
> I admit this is a bit terse ;) The idea is to control mte at the cpu
> level directly (and not indirectly via tag memory at the machine
> level). I.e. the user gets whatever is available given the constraints
> (host support etc.) if they don't specify anything, and they can
> explicitly turn it off/on.
Could the OnOffAuto property value be helpful?
>
>>>
>>> Signed-off-by: Cornelia Huck <cohuck@xxxxxxxxxx>
>>> ---
>>> target/arm/cpu.c | 18 ++++------
>>> target/arm/cpu.h | 4 +++
>>> target/arm/cpu64.c | 78 ++++++++++++++++++++++++++++++++++++++++++++
>>> target/arm/kvm64.c | 5 +++
>>> target/arm/kvm_arm.h | 12 +++++++
>>> target/arm/monitor.c | 1 +
>>> 6 files changed, 106 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>>> index 029f644768b1..f0505815b1e7 100644
>>> --- a/target/arm/cpu.c
>>> +++ b/target/arm/cpu.c
>>> @@ -1435,6 +1435,11 @@ void arm_cpu_finalize_features(ARMCPU *cpu, Error **errp)
>>> error_propagate(errp, local_err);
>>> return;
>>> }
>>> + arm_cpu_mte_finalize(cpu, &local_err);
>>> + if (local_err != NULL) {
>>> + error_propagate(errp, local_err);
>>> + return;
>>> + }
>>> }
>>>
>>> if (kvm_enabled()) {
>>> @@ -1504,7 +1509,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>> }
>>> if (cpu->tag_memory) {
>>> error_setg(errp,
>>> - "Cannot enable KVM when guest CPUs has MTE enabled");
>>> + "Cannot enable KVM when guest CPUs has tag memory enabled");
>> before this series, tag_memory was used to detect MTE was enabled at
>> machine level. And this was not compatible with KVM.
>>
>> Hasn't it changed now with this series? Sorry I don't know much about
>> that tag_memory along with the KVM use case? Can you describe it as well
>> in the cover letter.
>
> IIU the current code correctly, the purpose of tag_memory is twofold:
> - control whether mte should be available in the first place
> - provide a place where a memory region used by the tcg implemtation can
> be linked
OK I now understand the tag memory is a pure TCG thingy. So its setting
along with KVM shall be invalid indeed.
>
> The latter part (extra memory region) is not compatible with
> kvm. "Presence of extra memory for the implementation" as the knob to
> configure mte for tcg makes sense, but it didn't seem right to me to use
> it for kvm while controlling something which is basically a cpu property.
>
>>> return;
>>> }
>>> }
>
> (...)
>
>>> +void aarch64_add_mte_properties(Object *obj)
>>> +{
>>> + ARMCPU *cpu = ARM_CPU(obj);
>>> +
>>> + /*
>>> + * For tcg, the machine type may provide tag memory for MTE emulation.
>> s/machine type/machine?
>
> Either, I guess, as only the virt machine type provides tag memory in
> the first place.
yeah that's what just a nit about the terminology.
>
>>> + * We do not know whether that is the case at this point in time, so
>>> + * default MTE to on and check later.
>>> + * This preserves pre-existing behaviour, but is really a bit awkward.
>>> + */
>>> + qdev_property_add_static(DEVICE(obj), &arm_cpu_mte_property);
>>> + if (kvm_enabled()) {
>>> + /*
>>> + * Default MTE to off, as long as migration support is not
>>> + * yet implemented.
>>> + * TODO: implement migration support for kvm
>>> + */
>>> + cpu->prop_mte = false;
>>> + }
>>> +}
>>> +
>>> +void arm_cpu_mte_finalize(ARMCPU *cpu, Error **errp)
>>> +{
>>> + if (!cpu->prop_mte) {
>>> + /* Disable MTE feature bits. */
>>> + cpu->isar.id_aa64pfr1 =
>>> + FIELD_DP64(cpu->isar.id_aa64pfr1, ID_AA64PFR1, MTE, 0);
>>> + return;
>>> + }
>>> +#ifndef CONFIG_USER_ONLY
>>> + if (!kvm_enabled()) {
>>> + if (cpu_isar_feature(aa64_mte, cpu) && !cpu->tag_memory) {
>>> + /*
>>> + * Disable the MTE feature bits, unless we have tag-memory
>>> + * provided by the machine.
>>> + * This silent downgrade is not really nice if the user had
>>> + * explicitly requested MTE to be enabled by the cpu, but it
>>> + * preserves pre-existing behaviour. In an ideal world, we
>>
>>
>> Can't we "simply" prevent the end-user from using the prop_mte option
>> with a TCG CPU? and have something like
>>
>> For TCG, MTE depends on the CPU feature availability + machine tag memory
>> For KVM, MTE depends on the user opt-in + CPU feature avail (if
>> relevant) + host VM capability (?)
>
> I don't like kvm and tcg cpus behaving too differently... but then, tcg
> is already different as it needs tag_memory.
>
> Thinking about it, maybe we could repurpose tag_memory in the kvm case
> (e.g. for a temporary buffer for migration purposes) and require it in
> all cases (making kvm fail if the user specified tag memory, but the
> host doesn't support it). A cpu feature still looks more natural to me,
> but I'm not yet quite used to how things are done in arm :)
If the tag memory is an implementation "detail" for TCG then I agree
with you that a CPU property would be more adapted for KVM.
>
> The big elefant in the room is how migration will end up
> working... after reading the disscussions in
> https://lore.kernel.org/all/CAJc+Z1FZxSYB_zJit4+0uTR-88VqQL+-01XNMSEfua-dXDy6Wg@xxxxxxxxxxxxxx/
> I don't think it will be as "easy" as I thought, and we probably require
> some further fiddling on the kernel side.
Yes maybe the MTE migration process shall be documented and discussed
separately on the ML? Is Haibu Xu's address bouncing?
Eric
>
>>
>> But maybe I miss the point ...
>>> + * would fail if MTE was requested, but no tag memory has
>>> + * been provided.
>>> + */
>>> + cpu->isar.id_aa64pfr1 =
>>> + FIELD_DP64(cpu->isar.id_aa64pfr1, ID_AA64PFR1, MTE, 0);
>>> + }
>>> + if (!cpu_isar_feature(aa64_mte, cpu)) {
>>> + cpu->prop_mte = false;
>>> + }
>>> + return;
>>> + }
>>> + if (kvm_arm_mte_supported()) {
>>> +#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");
>>> + } else {
>>> + /* TODO: add proper migration support with MTE enabled */
>>> + if (!mte_migration_blocker) {
>>> + error_setg(&mte_migration_blocker,
>>> + "Live migration disabled due to MTE enabled");
>>> + if (migrate_add_blocker(mte_migration_blocker, NULL)) {
>> error_free(mte_migration_blocker);
>> mte_migration_blocker = NULL;
>
> Ah, I missed that, thanks.
>
>>> + error_setg(errp, "Failed to add MTE migration blocker");
>>> + }
>>> + }
>>> + }
>>> +#endif
>>> + }
>>> + /* When HVF provides support for MTE, add it here */
>>> +#endif
>>> +}
>>> +
>
