*** BLURB HERE ***

Michal Prívozník (4):
  conf: Introduce MTE domain feature
  qemu:: Introduce QEMU_CAPS_MACHINE_VIRT_MTE capability
  qemu: Validate MTE feature
  qemu: Generate command line for MTE feature

 docs/formatdomain.rst                         |  7 +++++
 src/conf/domain_conf.c                        |  6 ++++-
 src/conf/domain_conf.h                        |  1 +
 src/conf/schemas/domaincommon.rng             |  5 ++++
 src/qemu/qemu_capabilities.c                  |  2 ++
 src/qemu/qemu_capabilities.h                  |  1 +
 src/qemu/qemu_command.c                       |  6 +++++
 src/qemu/qemu_validate.c                      | 26 +++++++++++++------
 .../caps_5.2.0_aarch64.xml                    |  1 +
 .../caps_6.0.0_aarch64.xml                    |  1 +
 .../caps_6.2.0_aarch64.xml                    |  1 +
 .../caps_7.0.0_aarch64+hvf.xml                |  1 +
 .../caps_7.0.0_aarch64.xml                    |  1 +
 tests/qemuxml2argvdata/aarch64-gic-v3.args    |  2 +-
 tests/qemuxml2argvdata/aarch64-gic-v3.xml     |  1 +
 .../aarch64-gic-v3.aarch64-latest.xml         |  1 +
 16 files changed, 53 insertions(+), 10 deletions(-)

-- 
2.39.3

On Tue, May 16, 2023 at 12:54:12PM +0200, Michal Privoznik wrote:
> Michal Prívozník (4):
>   conf: Introduce MTE domain feature
>   qemu:: Introduce QEMU_CAPS_MACHINE_VIRT_MTE capability
>   qemu: Validate MTE feature
>   qemu: Generate command line for MTE feature

I wish I'd managed to see this before it got reviewed and merged :/

For context, I have been following the development of the MTE feature
in QEMU for a while, and was planning to work on the libvirt part
later down the line. The main reason why I have not done so yet is
that there are still some open questions about the interface.

Specifically, MTE is not just a single thing: there are at least two
versions that I'm aware of, MTE and MTE3.

Right now, mte=on gives you MTE3 with TCG and whatever the host
supports on KVM. Of course the latter is problematic when it comes to
guaranteeing a stable guest ABI... I think a reasonable interface
would be similar to what we have for GIC, with a 'version' attribute
used to explicitly choose between MTE and MTE3, and some logic to
fill in a reasonable value for the host by default.

But there's also the question of whether MTE should be a machine
property in the first place, rather than a CPU feature?

Committing to any specific interface in libvirt at this point in time
feels premature, as it's pretty much guaranteed that it will no
longer fit once the questions above have been answered.

Last but not least, the way detection has been implemented is not
accurate: as of today, QEMU does *not* support enabling MTE with KVM.
Patches adding this feature have been posted[1] and are going to be
merged soon, but even then just looking at the machine type property
is not going to be enough to determine whether MTE can actually be
used.

CC'ing Connie so that she can point out any mistakes I might have
made above :)


[1] https://lists.gnu.org/archive/html/qemu-devel/2023-04/msg05452.html
-- 
Andrea Bolognani / Red Hat / Virtualization

On 5/16/23 18:32, Andrea Bolognani wrote:
> On Tue, May 16, 2023 at 12:54:12PM +0200, Michal Privoznik wrote:
>> Michal Prívozník (4):
>>   conf: Introduce MTE domain feature
>>   qemu:: Introduce QEMU_CAPS_MACHINE_VIRT_MTE capability
>>   qemu: Validate MTE feature
>>   qemu: Generate command line for MTE feature
> 
> I wish I'd managed to see this before it got reviewed and merged :/

We can still revert the patches, if needed.

> 
> For context, I have been following the development of the MTE feature
> in QEMU for a while, and was planning to work on the libvirt part
> later down the line. The main reason why I have not done so yet is
> that there are still some open questions about the interface.
> 
> Specifically, MTE is not just a single thing: there are at least two
> versions that I'm aware of, MTE and MTE3.
> 
> Right now, mte=on gives you MTE3 with TCG and whatever the host
> supports on KVM. Of course the latter is problematic when it comes to
> guaranteeing a stable guest ABI... I think a reasonable interface
> would be similar to what we have for GIC, with a 'version' attribute
> used to explicitly choose between MTE and MTE3, and some logic to
> fill in a reasonable value for the host by default.
> 
> But there's also the question of whether MTE should be a machine
> property in the first place, rather than a CPU feature?

I admit that my QEMU code base understanding is limited, but the patch
you've linked doesn't really expose any CPU feature that libvirt could
set. Instead, it enables MTE for KVM under the same API, i.e. -machine
virt,mte=on.

> 
> Committing to any specific interface in libvirt at this point in time
> feels premature, as it's pretty much guaranteed that it will no
> longer fit once the questions above have been answered.

Fair enough, feel free to revert my patches.

> 
> Last but not least, the way detection has been implemented is not
> accurate: as of today, QEMU does *not* support enabling MTE with KVM.
> Patches adding this feature have been posted[1] and are going to be
> merged soon, but even then just looking at the machine type property
> is not going to be enough to determine whether MTE can actually be
> used.

Then it's a matter of:

diff --git i/hw/arm/virt.c w/hw/arm/virt.c
index b99ae18501..ead3555dfa 100644
--- i/hw/arm/virt.c
+++ w/hw/arm/virt.c
@@ -3111,11 +3111,13 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
                                           "Set on/off to enable/disable reporting host memory errors "
                                           "to a KVM guest using ACPI and guest external abort exceptions");
 
-    object_class_property_add_bool(oc, "mte", virt_get_mte, virt_set_mte);
-    object_class_property_set_description(oc, "mte",
-                                          "Set on/off to enable/disable emulating a "
-                                          "guest CPU which implements the ARM "
-                                          "Memory Tagging Extension");
+    if (kvm_arm_mte_supported()) {
+        object_class_property_add_bool(oc, "mte", virt_get_mte, virt_set_mte);
+        object_class_property_set_description(oc, "mte",
+                                              "Set on/off to enable/disable emulating a "
+                                              "guest CPU which implements the ARM "
+                                              "Memory Tagging Extension");
+    }
 
     object_class_property_add_bool(oc, "its", virt_get_its,
                                    virt_set_its);

Or querying KVM extensions in libvirt (we already do that for some features).

Michal

On Wed, May 17, 2023 at 09:14:02AM +0200, Michal Prívozník wrote:
> On 5/16/23 18:32, Andrea Bolognani wrote:
> > Last but not least, the way detection has been implemented is not
> > accurate: as of today, QEMU does *not* support enabling MTE with KVM.
> > Patches adding this feature have been posted[1] and are going to be
> > merged soon, but even then just looking at the machine type property
> > is not going to be enough to determine whether MTE can actually be
> > used.
>
> Then it's a matter of:
>
> +    if (kvm_arm_mte_supported()) {
> +        object_class_property_add_bool(oc, "mte", virt_get_mte, virt_set_mte);
> +        object_class_property_set_description(oc, "mte",
> +                                              "Set on/off to enable/disable emulating a "
> +                                              "guest CPU which implements the ARM "
> +                                              "Memory Tagging Extension");
> +    }

I don't think this would work: even if KVM doesn't support MTE, TCG
can still emulate it, so the property still needs to show up.

> Or querying KVM extensions in libvirt (we already do that for some features).

That would tell us whether KVM itself is MTE-capable, but not whether
the QEMU binary can make use of that feature.

> > Committing to any specific interface in libvirt at this point in time
> > feels premature, as it's pretty much guaranteed that it will no
> > longer fit once the questions above have been answered.
>
> Fair enough, feel free to revert my patches.

Let's keep the discussion going for now, but if we get very close to
the freeze without a clear consensus on the one you have implemented
being the interface that we want I'll probably post a revert.

-- 
Andrea Bolognani / Red Hat / Virtualization

On Wed, May 17 2023, Michal Prívozník <mprivozn@redhat.com> wrote:

> On 5/16/23 18:32, Andrea Bolognani wrote:
>> On Tue, May 16, 2023 at 12:54:12PM +0200, Michal Privoznik wrote:
>>> Michal Prívozník (4):
>>>   conf: Introduce MTE domain feature
>>>   qemu:: Introduce QEMU_CAPS_MACHINE_VIRT_MTE capability
>>>   qemu: Validate MTE feature
>>>   qemu: Generate command line for MTE feature
>> 
>> I wish I'd managed to see this before it got reviewed and merged :/
>
> We can still revert the patches, if needed.
>
>> 
>> For context, I have been following the development of the MTE feature
>> in QEMU for a while, and was planning to work on the libvirt part
>> later down the line. The main reason why I have not done so yet is
>> that there are still some open questions about the interface.
>> 
>> Specifically, MTE is not just a single thing: there are at least two
>> versions that I'm aware of, MTE and MTE3.
>> 
>> Right now, mte=on gives you MTE3 with TCG and whatever the host
>> supports on KVM. Of course the latter is problematic when it comes to
>> guaranteeing a stable guest ABI... I think a reasonable interface
>> would be similar to what we have for GIC, with a 'version' attribute
>> used to explicitly choose between MTE and MTE3, and some logic to
>> fill in a reasonable value for the host by default.
>> 
>> But there's also the question of whether MTE should be a machine
>> property in the first place, rather than a CPU feature?
>
> I admit that my QEMU code base understanding is limited, but the patch
> you've linked doesn't really expose any CPU feature that libvirt could
> set. Instead, it enables MTE for KVM under the same API, i.e. -machine
> virt,mte=on.

This has been through some iterations... we (as in people working on
this in QEMU) need to decide on where to go with cpu features, cpu
models, etc. on Arm, but for now, it's a virt machine property.

I have considered doing a GIC-like configuration, but for starters, the
kernel doesn't support configuring the MTE version yet... and I'm not
sure if configuring the MTE version (vs enabling/disabling MTE) should
not be modeled as a cpu property instead.

Note that my patch adds a migration blocker when enabling MTE, so (1)
nothing bad regarding migration compatibility should happen yet, and (2)
libvirt should probably not turn it on by default (I haven't checked
what the patches actually do ;)

[Also, I don't think there is any readily available hardware supporting
MTE yet; I have been testing my code on the simulator...]

On Wed, May 17, 2023 at 11:19:17AM +0200, Cornelia Huck wrote:
> This has been through some iterations... we (as in people working on
> this in QEMU) need to decide on where to go with cpu features, cpu
> models, etc. on Arm, but for now, it's a virt machine property.
>
> I have considered doing a GIC-like configuration, but for starters, the
> kernel doesn't support configuring the MTE version yet... and I'm not
> sure if configuring the MTE version (vs enabling/disabling MTE) should
> not be modeled as a cpu property instead.
>
> Note that my patch adds a migration blocker when enabling MTE, so (1)
> nothing bad regarding migration compatibility should happen yet

Migration is of course the most obvious failure scenario, but one of
the critical features offered by libvirt is guest ABI compatibility.

If the user needs MTE3 specifically, rather than any MTE version, to
be available to the guest OS, they'll configure the domain with
something like

  <mte version='3'/>

and we need to be able to prevent such a VM from running on a host
that doesn't have MTE3 support.

So the fundamental question is, does the current libvirt interface
paint us into a corner when it comes to implementing a more granular
one later?

Remember that, unlike QEMU, we don't have the luxury of erasing
mistakes from our public interfaces, ever :)

> libvirt should probably not turn it on by default (I haven't checked
> what the patches actually do ;)
>
> [Also, I don't think there is any readily available hardware supporting
> MTE yet; I have been testing my code on the simulator...]

Agreed on not enabling it by default, especially considering the
current hardware support situation. The feature remains disabled by
default after the patches that have been merged.

-- 
Andrea Bolognani / Red Hat / Virtualization

On Wed, May 17 2023, Andrea Bolognani <abologna@redhat.com> wrote:

> On Wed, May 17, 2023 at 11:19:17AM +0200, Cornelia Huck wrote:
>> This has been through some iterations... we (as in people working on
>> this in QEMU) need to decide on where to go with cpu features, cpu
>> models, etc. on Arm, but for now, it's a virt machine property.
>>
>> I have considered doing a GIC-like configuration, but for starters, the
>> kernel doesn't support configuring the MTE version yet... and I'm not
>> sure if configuring the MTE version (vs enabling/disabling MTE) should
>> not be modeled as a cpu property instead.
>>
>> Note that my patch adds a migration blocker when enabling MTE, so (1)
>> nothing bad regarding migration compatibility should happen yet
>
> Migration is of course the most obvious failure scenario, but one of
> the critical features offered by libvirt is guest ABI compatibility.
>
> If the user needs MTE3 specifically, rather than any MTE version, to
> be available to the guest OS, they'll configure the domain with
> something like
>
>   <mte version='3'/>
>
> and we need to be able to prevent such a VM from running on a host
> that doesn't have MTE3 support.
>
> So the fundamental question is, does the current libvirt interface
> paint us into a corner when it comes to implementing a more granular
> one later?

Given that an interface allowing to actually control the exposed version
is not likely to pop up in the next days, does it make sense to even try
to wire it up in libvirt right now?

>
> Remember that, unlike QEMU, we don't have the luxury of erasing
> mistakes from our public interfaces, ever :)

On Mon, May 22, 2023 at 11:55:15AM +0200, Cornelia Huck wrote:
> On Wed, May 17 2023, Andrea Bolognani <abologna@redhat.com> wrote:
> > Migration is of course the most obvious failure scenario, but one of
> > the critical features offered by libvirt is guest ABI compatibility.
> >
> > If the user needs MTE3 specifically, rather than any MTE version, to
> > be available to the guest OS, they'll configure the domain with
> > something like
> >
> >   <mte version='3'/>
> >
> > and we need to be able to prevent such a VM from running on a host
> > that doesn't have MTE3 support.
> >
> > So the fundamental question is, does the current libvirt interface
> > paint us into a corner when it comes to implementing a more granular
> > one later?
>
> Given that an interface allowing to actually control the exposed version
> is not likely to pop up in the next days, does it make sense to even try
> to wire it up in libvirt right now?

I believe it does not.

Until the situation has cleared up

  <qemu:commandline>
    <qemu:arg value='-machine'/>
    <qemu:arg value='virt,mte=on'/>
  </qemu:commandline>

while far from an optimal solution, will work in a pinch.

-- 
Andrea Bolognani / Red Hat / Virtualization

On Tue, May 16, 2023 at 12:54:12PM +0200, Michal Privoznik wrote:
>*** BLURB HERE ***
>
>Michal Prívozník (4):
>  conf: Introduce MTE domain feature
>  qemu:: Introduce QEMU_CAPS_MACHINE_VIRT_MTE capability
>  qemu: Validate MTE feature
>  qemu: Generate command line for MTE feature

With the two points pointed out in 3/4 and 4/4 fixed

Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

>
> docs/formatdomain.rst                         |  7 +++++
> src/conf/domain_conf.c                        |  6 ++++-
> src/conf/domain_conf.h                        |  1 +
> src/conf/schemas/domaincommon.rng             |  5 ++++
> src/qemu/qemu_capabilities.c                  |  2 ++
> src/qemu/qemu_capabilities.h                  |  1 +
> src/qemu/qemu_command.c                       |  6 +++++
> src/qemu/qemu_validate.c                      | 26 +++++++++++++------
> .../caps_5.2.0_aarch64.xml                    |  1 +
> .../caps_6.0.0_aarch64.xml                    |  1 +
> .../caps_6.2.0_aarch64.xml                    |  1 +
> .../caps_7.0.0_aarch64+hvf.xml                |  1 +
> .../caps_7.0.0_aarch64.xml                    |  1 +
> tests/qemuxml2argvdata/aarch64-gic-v3.args    |  2 +-
> tests/qemuxml2argvdata/aarch64-gic-v3.xml     |  1 +
> .../aarch64-gic-v3.aarch64-latest.xml         |  1 +
> 16 files changed, 53 insertions(+), 10 deletions(-)
>
>-- 
>2.39.3
>