[PATCH 2/3] conf,qemu: implement RISC-V 'aia' virt domain feature

Daniel Henrique Barboza posted 3 patches 1 year, 3 months ago
There is a newer version of this series
[PATCH 2/3] conf,qemu: implement RISC-V 'aia' virt domain feature
Posted by Daniel Henrique Barboza 1 year, 3 months ago
This feature is implemented as a string that can range from "none",
"aplic" and "aplic-imsic".

If the feature isn't present in the domain XML the hypervisor default
will be used. For QEMU, at least up to 9.2, the default is "none".

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 docs/formatdomain.rst             |  8 +++++++
 src/conf/domain_conf.c            | 39 +++++++++++++++++++++++++++++++
 src/conf/domain_conf.h            | 11 +++++++++
 src/conf/schemas/domaincommon.rng | 15 ++++++++++++
 src/libvirt_private.syms          |  2 ++
 src/qemu/qemu_validate.c          | 15 ++++++++++++
 6 files changed, 90 insertions(+)

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 3253a28e5a..780cea9aba 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -2028,6 +2028,7 @@ Hypervisors may allow certain CPU / machine features to be toggled on/off.
      <async-teardown enabled='yes'/>
      <ras state='on'/>
      <ps2 state='on'/>
+     <aia value='aplic-imsic'/>
    </features>
    ...
 
@@ -2275,6 +2276,13 @@ are:
    disable the emulation of a PS/2 controller used by ``ps2`` bus input devices.
    If the attribute is not defined, the hypervisor default will be used.
    :since:`Since 10.7.0` (QEMU only)
+``aia``
+   Configure aia (Advanced Interrupt Architecture) for RISC-V 'virt'
+   guests. Possible values for the ``value`` attribute are ``aplic`` (one
+   emulated APLIC device present per socket), ``aplic-imsic`` (one APLIC and
+   one IMSIC device present per core), or ``none`` (no support for AIA).
+   If the attribute is not defined, the hypervisor default
+   will be used. :since:`Since 10.8.0` (QEMU/KVM and RISC-V guests only)
 
 Time keeping
 ------------
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 6d7dee7956..1a677c47d7 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -186,6 +186,7 @@ VIR_ENUM_IMPL(virDomainFeature,
               "async-teardown",
               "ras",
               "ps2",
+              "aia",
 );
 
 VIR_ENUM_IMPL(virDomainCapabilitiesPolicy,
@@ -1522,6 +1523,13 @@ VIR_ENUM_IMPL(virDomainPstoreBackend,
               "acpi-erst",
 );
 
+VIR_ENUM_IMPL(virDomainAIA,
+              VIR_DOMAIN_AIA_LAST,
+              "none",
+              "aplic",
+              "aplic-imsic",
+);
+
 typedef enum {
     VIR_DOMAIN_NET_VHOSTUSER_MODE_NONE,
     VIR_DOMAIN_NET_VHOSTUSER_MODE_CLIENT,
@@ -17040,6 +17048,18 @@ virDomainFeaturesDefParse(virDomainDef *def,
             break;
         }
 
+        case VIR_DOMAIN_FEATURE_AIA: {
+            virDomainAIA value;
+
+            if (virXMLPropEnumDefault(nodes[i], "value", virDomainAIATypeFromString,
+                                      VIR_XML_PROP_NONZERO, &value,
+                                      VIR_DOMAIN_AIA_NONE) < 0)
+                return -1;
+
+            def->features[val] = value;
+            break;
+        }
+
         case VIR_DOMAIN_FEATURE_TCG:
             if (virDomainFeaturesTCGDefParse(def, ctxt, nodes[i]) < 0)
                 return -1;
@@ -21007,6 +21027,17 @@ virDomainDefFeaturesCheckABIStability(virDomainDef *src,
             }
             break;
 
+        case VIR_DOMAIN_FEATURE_AIA:
+            if (src->features[i] != dst->features[i]) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                               _("State of feature '%1$s' differs: source: '%2$s=%3$s', destination: '%4$s=%5$s'"),
+                               featureName,
+                               "value", virDomainAIATypeToString(src->features[i]),
+                               "value", virDomainAIATypeToString(dst->features[i]));
+                return false;
+            }
+            break;
+
         case VIR_DOMAIN_FEATURE_MSRS:
         case VIR_DOMAIN_FEATURE_TCG:
         case VIR_DOMAIN_FEATURE_ASYNC_TEARDOWN:
@@ -28001,6 +28032,14 @@ virDomainDefFormatFeatures(virBuffer *buf,
                                   virTristateBoolTypeToString(def->features[i]));
             break;
 
+        case VIR_DOMAIN_FEATURE_AIA:
+            if (def->features[i] == VIR_DOMAIN_AIA_NONE)
+                break;
+
+            virBufferAsprintf(&childBuf, "<aia value='%s'/>\n",
+                              virDomainAIATypeToString(def->features[i]));
+            break;
+
         case VIR_DOMAIN_FEATURE_LAST:
             break;
         }
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index a15af4fae3..3cc019eb90 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2182,6 +2182,7 @@ typedef enum {
     VIR_DOMAIN_FEATURE_ASYNC_TEARDOWN,
     VIR_DOMAIN_FEATURE_RAS,
     VIR_DOMAIN_FEATURE_PS2,
+    VIR_DOMAIN_FEATURE_AIA,
 
     VIR_DOMAIN_FEATURE_LAST
 } virDomainFeature;
@@ -2398,6 +2399,16 @@ typedef enum {
 
 VIR_ENUM_DECL(virDomainIBS);
 
+typedef enum {
+    VIR_DOMAIN_AIA_NONE = 0,
+    VIR_DOMAIN_AIA_APLIC,
+    VIR_DOMAIN_AIA_APLIC_IMSIC,
+
+    VIR_DOMAIN_AIA_LAST
+} virDomainAIA;
+
+VIR_ENUM_DECL(virDomainAIA);
+
 typedef struct _virDomainFeatureKVM virDomainFeatureKVM;
 struct _virDomainFeatureKVM {
     int features[VIR_DOMAIN_KVM_LAST];
diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng
index efb5f00d77..203b2ca979 100644
--- a/src/conf/schemas/domaincommon.rng
+++ b/src/conf/schemas/domaincommon.rng
@@ -6922,6 +6922,9 @@
               <ref name="featurestate"/>
             </element>
           </optional>
+          <optional>
+            <ref name="aia"/>
+          </optional>
         </interleave>
       </element>
     </optional>
@@ -7220,6 +7223,18 @@
     </element>
   </define>
 
+  <define name="aia">
+    <element name="aia">
+      <attribute name="value">
+        <choice>
+          <value>none</value>
+          <value>aplic</value>
+          <value>aplic-imsic</value>
+        </choice>
+      </attribute>
+    </element>
+  </define>
+
   <define name="address">
     <element name="address">
       <choice>
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 5fb4df3513..deb4ef5ba6 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -229,6 +229,8 @@ virDiskNameParse;
 virDiskNameToBusDeviceIndex;
 virDiskNameToIndex;
 virDomainActualNetDefFree;
+virDomainAIATypeFromString;
+virDomainAIATypeToString;
 virDomainAudioDefFree;
 virDomainAudioFormatTypeFromString;
 virDomainAudioFormatTypeToString;
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index fa23c5f973..8f44205667 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -265,6 +265,21 @@ qemuValidateDomainDefFeatures(const virDomainDef *def,
             }
             break;
 
+        case VIR_DOMAIN_FEATURE_AIA:
+            if (def->features[i] != VIR_TRISTATE_SWITCH_ABSENT &&
+                !qemuDomainIsRISCVVirt(def)) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                               _("aia feature is only supported with RISC-V Virt machines"));
+                return -1;
+            }
+            if (def->features[i] != VIR_TRISTATE_SWITCH_ABSENT &&
+                !virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_VIRT_AIA)) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                              _("aia feature is not available with this QEMU binary"));
+                return -1;
+            }
+            break;
+
         case VIR_DOMAIN_FEATURE_SMM:
         case VIR_DOMAIN_FEATURE_KVM:
         case VIR_DOMAIN_FEATURE_XEN:
-- 
2.45.2
Re: [PATCH 2/3] conf,qemu: implement RISC-V 'aia' virt domain feature
Posted by Martin Kletzander 1 year ago
On Thu, Oct 24, 2024 at 11:08:19AM -0300, Daniel Henrique Barboza wrote:
>This feature is implemented as a string that can range from "none",
>"aplic" and "aplic-imsic".
>
>If the feature isn't present in the domain XML the hypervisor default
>will be used. For QEMU, at least up to 9.2, the default is "none".
>
>Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>---
> docs/formatdomain.rst             |  8 +++++++
> src/conf/domain_conf.c            | 39 +++++++++++++++++++++++++++++++
> src/conf/domain_conf.h            | 11 +++++++++
> src/conf/schemas/domaincommon.rng | 15 ++++++++++++
> src/libvirt_private.syms          |  2 ++
> src/qemu/qemu_validate.c          | 15 ++++++++++++
> 6 files changed, 90 insertions(+)
>
>diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
>index 3253a28e5a..780cea9aba 100644
>--- a/docs/formatdomain.rst
>+++ b/docs/formatdomain.rst
>@@ -2028,6 +2028,7 @@ Hypervisors may allow certain CPU / machine features to be toggled on/off.
>      <async-teardown enabled='yes'/>
>      <ras state='on'/>
>      <ps2 state='on'/>
>+     <aia value='aplic-imsic'/>
>    </features>
>    ...
>
>@@ -2275,6 +2276,13 @@ are:
>    disable the emulation of a PS/2 controller used by ``ps2`` bus input devices.
>    If the attribute is not defined, the hypervisor default will be used.
>    :since:`Since 10.7.0` (QEMU only)
>+``aia``
>+   Configure aia (Advanced Interrupt Architecture) for RISC-V 'virt'
>+   guests. Possible values for the ``value`` attribute are ``aplic`` (one
>+   emulated APLIC device present per socket), ``aplic-imsic`` (one APLIC and
>+   one IMSIC device present per core), or ``none`` (no support for AIA).
>+   If the attribute is not defined, the hypervisor default
>+   will be used. :since:`Since 10.8.0` (QEMU/KVM and RISC-V guests only)
>

This needs to be changed to 11.1.0, sorry for missing this patch for so
long.

Also, the hypervisor default will be used even the user specifies
aia='none' based on the code in patch 3/3.  So either we need to add an
absent value for this attribute or change the wording.

Other than that these patches are all fine and good to go, the only
thing above needs to be figured out.
Re: [PATCH 2/3] conf,qemu: implement RISC-V 'aia' virt domain feature
Posted by Daniel Henrique Barboza 1 year ago

On 1/22/25 8:43 AM, Martin Kletzander wrote:
> On Thu, Oct 24, 2024 at 11:08:19AM -0300, Daniel Henrique Barboza wrote:
>> This feature is implemented as a string that can range from "none",
>> "aplic" and "aplic-imsic".
>>
>> If the feature isn't present in the domain XML the hypervisor default
>> will be used. For QEMU, at least up to 9.2, the default is "none".
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>> docs/formatdomain.rst             |  8 +++++++
>> src/conf/domain_conf.c            | 39 +++++++++++++++++++++++++++++++
>> src/conf/domain_conf.h            | 11 +++++++++
>> src/conf/schemas/domaincommon.rng | 15 ++++++++++++
>> src/libvirt_private.syms          |  2 ++
>> src/qemu/qemu_validate.c          | 15 ++++++++++++
>> 6 files changed, 90 insertions(+)
>>
>> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
>> index 3253a28e5a..780cea9aba 100644
>> --- a/docs/formatdomain.rst
>> +++ b/docs/formatdomain.rst
>> @@ -2028,6 +2028,7 @@ Hypervisors may allow certain CPU / machine features to be toggled on/off.
>>      <async-teardown enabled='yes'/>
>>      <ras state='on'/>
>>      <ps2 state='on'/>
>> +     <aia value='aplic-imsic'/>
>>    </features>
>>    ...
>>
>> @@ -2275,6 +2276,13 @@ are:
>>    disable the emulation of a PS/2 controller used by ``ps2`` bus input devices.
>>    If the attribute is not defined, the hypervisor default will be used.
>>    :since:`Since 10.7.0` (QEMU only)
>> +``aia``
>> +   Configure aia (Advanced Interrupt Architecture) for RISC-V 'virt'
>> +   guests. Possible values for the ``value`` attribute are ``aplic`` (one
>> +   emulated APLIC device present per socket), ``aplic-imsic`` (one APLIC and
>> +   one IMSIC device present per core), or ``none`` (no support for AIA).
>> +   If the attribute is not defined, the hypervisor default
>> +   will be used. :since:`Since 10.8.0` (QEMU/KVM and RISC-V guests only)
>>
> 
> This needs to be changed to 11.1.0, sorry for missing this patch for so
> long.
> 
> Also, the hypervisor default will be used even the user specifies
> aia='none' based on the code in patch 3/3.  So either we need to add an
> absent value for this attribute or change the wording.

You're right. I forgot about the 'none' meaning here as 'this option wasn't
specified' when writing this doc.

I'll add an "off" (or "disabled" - I'll see what libvirt tends to use in these
cases) value for it that can be translated to 'none' when building the QEMU
command line.


Thanks,

Daniel


> 
> Other than that these patches are all fine and good to go, the only
> thing above needs to be figured out.
Re: [PATCH 2/3] conf,qemu: implement RISC-V 'aia' virt domain feature
Posted by Martin Kletzander 1 year ago
On Wed, Jan 22, 2025 at 09:40:06AM -0300, Daniel Henrique Barboza wrote:
>
>
>On 1/22/25 8:43 AM, Martin Kletzander wrote:
>> On Thu, Oct 24, 2024 at 11:08:19AM -0300, Daniel Henrique Barboza wrote:
>>> This feature is implemented as a string that can range from "none",
>>> "aplic" and "aplic-imsic".
>>>
>>> If the feature isn't present in the domain XML the hypervisor default
>>> will be used. For QEMU, at least up to 9.2, the default is "none".
>>>
>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>> ---
>>> docs/formatdomain.rst             |  8 +++++++
>>> src/conf/domain_conf.c            | 39 +++++++++++++++++++++++++++++++
>>> src/conf/domain_conf.h            | 11 +++++++++
>>> src/conf/schemas/domaincommon.rng | 15 ++++++++++++
>>> src/libvirt_private.syms          |  2 ++
>>> src/qemu/qemu_validate.c          | 15 ++++++++++++
>>> 6 files changed, 90 insertions(+)
>>>
>>> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
>>> index 3253a28e5a..780cea9aba 100644
>>> --- a/docs/formatdomain.rst
>>> +++ b/docs/formatdomain.rst
>>> @@ -2028,6 +2028,7 @@ Hypervisors may allow certain CPU / machine features to be toggled on/off.
>>>      <async-teardown enabled='yes'/>
>>>      <ras state='on'/>
>>>      <ps2 state='on'/>
>>> +     <aia value='aplic-imsic'/>
>>>    </features>
>>>    ...
>>>
>>> @@ -2275,6 +2276,13 @@ are:
>>>    disable the emulation of a PS/2 controller used by ``ps2`` bus input devices.
>>>    If the attribute is not defined, the hypervisor default will be used.
>>>    :since:`Since 10.7.0` (QEMU only)
>>> +``aia``
>>> +   Configure aia (Advanced Interrupt Architecture) for RISC-V 'virt'
>>> +   guests. Possible values for the ``value`` attribute are ``aplic`` (one
>>> +   emulated APLIC device present per socket), ``aplic-imsic`` (one APLIC and
>>> +   one IMSIC device present per core), or ``none`` (no support for AIA).
>>> +   If the attribute is not defined, the hypervisor default
>>> +   will be used. :since:`Since 10.8.0` (QEMU/KVM and RISC-V guests only)
>>>
>>
>> This needs to be changed to 11.1.0, sorry for missing this patch for so
>> long.
>>
>> Also, the hypervisor default will be used even the user specifies
>> aia='none' based on the code in patch 3/3.  So either we need to add an
>> absent value for this attribute or change the wording.
>
>You're right. I forgot about the 'none' meaning here as 'this option wasn't
>specified' when writing this doc.
>
>I'll add an "off" (or "disabled" - I'll see what libvirt tends to use in these
>cases) value for it that can be translated to 'none' when building the QEMU
>command line.
>

You can keep the current options the way they are (especially if qemu
accepts "none") and just add "default" which will not be documented.

>
>Thanks,
>
>Daniel
>
>
>>
>> Other than that these patches are all fine and good to go, the only
>> thing above needs to be figured out.
>
Re: [PATCH 2/3] conf,qemu: implement RISC-V 'aia' virt domain feature
Posted by Daniel Henrique Barboza 1 year ago

On 1/22/25 10:11 AM, Martin Kletzander wrote:
> On Wed, Jan 22, 2025 at 09:40:06AM -0300, Daniel Henrique Barboza wrote:
>>
>>
>> On 1/22/25 8:43 AM, Martin Kletzander wrote:
>>> On Thu, Oct 24, 2024 at 11:08:19AM -0300, Daniel Henrique Barboza wrote:
>>>> This feature is implemented as a string that can range from "none",
>>>> "aplic" and "aplic-imsic".
>>>>
>>>> If the feature isn't present in the domain XML the hypervisor default
>>>> will be used. For QEMU, at least up to 9.2, the default is "none".
>>>>
>>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>>> ---
>>>> docs/formatdomain.rst             |  8 +++++++
>>>> src/conf/domain_conf.c            | 39 +++++++++++++++++++++++++++++++
>>>> src/conf/domain_conf.h            | 11 +++++++++
>>>> src/conf/schemas/domaincommon.rng | 15 ++++++++++++
>>>> src/libvirt_private.syms          |  2 ++
>>>> src/qemu/qemu_validate.c          | 15 ++++++++++++
>>>> 6 files changed, 90 insertions(+)
>>>>
>>>> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
>>>> index 3253a28e5a..780cea9aba 100644
>>>> --- a/docs/formatdomain.rst
>>>> +++ b/docs/formatdomain.rst
>>>> @@ -2028,6 +2028,7 @@ Hypervisors may allow certain CPU / machine features to be toggled on/off.
>>>>      <async-teardown enabled='yes'/>
>>>>      <ras state='on'/>
>>>>      <ps2 state='on'/>
>>>> +     <aia value='aplic-imsic'/>
>>>>    </features>
>>>>    ...
>>>>
>>>> @@ -2275,6 +2276,13 @@ are:
>>>>    disable the emulation of a PS/2 controller used by ``ps2`` bus input devices.
>>>>    If the attribute is not defined, the hypervisor default will be used.
>>>>    :since:`Since 10.7.0` (QEMU only)
>>>> +``aia``
>>>> +   Configure aia (Advanced Interrupt Architecture) for RISC-V 'virt'
>>>> +   guests. Possible values for the ``value`` attribute are ``aplic`` (one
>>>> +   emulated APLIC device present per socket), ``aplic-imsic`` (one APLIC and
>>>> +   one IMSIC device present per core), or ``none`` (no support for AIA).
>>>> +   If the attribute is not defined, the hypervisor default
>>>> +   will be used. :since:`Since 10.8.0` (QEMU/KVM and RISC-V guests only)
>>>>
>>>
>>> This needs to be changed to 11.1.0, sorry for missing this patch for so
>>> long.
>>>
>>> Also, the hypervisor default will be used even the user specifies
>>> aia='none' based on the code in patch 3/3.  So either we need to add an
>>> absent value for this attribute or change the wording.
>>
>> You're right. I forgot about the 'none' meaning here as 'this option wasn't
>> specified' when writing this doc.
>>
>> I'll add an "off" (or "disabled" - I'll see what libvirt tends to use in these
>> cases) value for it that can be translated to 'none' when building the QEMU
>> command line.
>>
> 
> You can keep the current options the way they are (especially if qemu
> accepts "none") and just add "default" which will not be documented.

That's even better. In fact it seems like libvirt already does that with
other features as well.

I'll send a v2 shortly. Thanks,


Daniel

> 
>>
>> Thanks,
>>
>> Daniel
>>
>>
>>>
>>> Other than that these patches are all fine and good to go, the only
>>> thing above needs to be figured out.
>>