Introduced only a couple of commits ago (in
v10.5.0-84-g90e50e67c6) the pstore device acts as a nonvolatile
storage, where guest kernel can store information about crashes.
This device, however, expects a file in the host from which the
crash data is read. So far, we expected users to provide a path,
but we can autogenerate one if missing. Just put it next to
per-domain's _NVRAM stores.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_domain.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 298f4bfb9e..198ab99aef 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6289,6 +6289,28 @@ qemuDomainMemoryDefPostParse(virDomainMemoryDef *mem, virArch arch,
 }
 
 
+static int
+qemuDomainPstoreDefPostParse(virDomainPstoreDef *pstore,
+                             const virDomainDef *def,
+                             virQEMUDriver *driver)
+{
+    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
+
+    switch (pstore->backend) {
+    case VIR_DOMAIN_PSTORE_BACKEND_ACPI_ERST:
+        if (!pstore->path)
+            pstore->path = g_strdup_printf("%s/%s_PSTORE.raw",
+                                           cfg->nvramDir, def->name);
+        break;
+
+    case VIR_DOMAIN_PSTORE_BACKEND_LAST:
+        break;
+    }
+
+    return 0;
+}
+
+
 static int
 qemuDomainDeviceDefPostParse(virDomainDeviceDef *dev,
                              const virDomainDef *def,
@@ -6350,6 +6372,10 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDef *dev,
                                            parseFlags);
         break;
 
+    case VIR_DOMAIN_DEVICE_PSTORE:
+        ret = qemuDomainPstoreDefPostParse(dev->data.pstore, def, driver);
+        break;
+
     case VIR_DOMAIN_DEVICE_LEASE:
     case VIR_DOMAIN_DEVICE_FS:
     case VIR_DOMAIN_DEVICE_INPUT:
@@ -6365,7 +6391,6 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDef *dev,
     case VIR_DOMAIN_DEVICE_IOMMU:
     case VIR_DOMAIN_DEVICE_AUDIO:
     case VIR_DOMAIN_DEVICE_CRYPTO:
-    case VIR_DOMAIN_DEVICE_PSTORE:
         ret = 0;
         break;
 
-- 
2.44.2

On Mon, Jul 29, 2024 at 11:31:35AM GMT, Michal Privoznik wrote:
> Introduced only a couple of commits ago (in
> v10.5.0-84-g90e50e67c6) the pstore device acts as a nonvolatile
> storage, where guest kernel can store information about crashes.
> This device, however, expects a file in the host from which the
> crash data is read. So far, we expected users to provide a path,
> but we can autogenerate one if missing. Just put it next to
> per-domain's _NVRAM stores.

Either s/_NVRAM/_VARS/ or lose the leading underscore.

You also need to squash in the diff below.

With the latter taken care of,

  Reviewed-by: Andrea Bolognani <abologna@redhat.com>


diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 860ef17d7b..c56b739b23 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -8683,8 +8683,7 @@ desired backend (only ``acpi-erst`` is accepted
for now). Then it has the
 following child elements:

 ``path``
-  Represents a path in the host that backs the pstore device in the guest. It
-  is mandatory.
+  Represents a path in the host that backs the pstore device in the guest.

 ``size``
   Configures the size of the persistent storage available to the guest. It is
diff --git a/src/conf/schemas/domaincommon.rng
b/src/conf/schemas/domaincommon.rng
index 6fcee2a70c..7d58dce465 100644
--- a/src/conf/schemas/domaincommon.rng
+++ b/src/conf/schemas/domaincommon.rng
@@ -6261,9 +6261,11 @@
         <value>acpi-erst</value>
       </attribute>
       <interleave>
-        <element name="path">
-          <ref name="absFilePath"/>
-        </element>
+        <optional>
+          <element name="path">
+            <ref name="absFilePath"/>
+          </element>
+        </optional>
         <element name="size">
           <ref name="scaledInteger"/>
         </element>
-- 
Andrea Bolognani / Red Hat / Virtualization

On 7/30/24 15:55, Andrea Bolognani wrote:
> On Mon, Jul 29, 2024 at 11:31:35AM GMT, Michal Privoznik wrote:
>> Introduced only a couple of commits ago (in
>> v10.5.0-84-g90e50e67c6) the pstore device acts as a nonvolatile
>> storage, where guest kernel can store information about crashes.
>> This device, however, expects a file in the host from which the
>> crash data is read. So far, we expected users to provide a path,
>> but we can autogenerate one if missing. Just put it next to
>> per-domain's _NVRAM stores.
> 
> Either s/_NVRAM/_VARS/ or lose the leading underscore.
> 
> You also need to squash in the diff below.
> 
> With the latter taken care of,
> 
>   Reviewed-by: Andrea Bolognani <abologna@redhat.com>
> 
> 
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index 860ef17d7b..c56b739b23 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
> @@ -8683,8 +8683,7 @@ desired backend (only ``acpi-erst`` is accepted
> for now). Then it has the
>  following child elements:
> 
>  ``path``
> -  Represents a path in the host that backs the pstore device in the guest. It
> -  is mandatory.
> +  Represents a path in the host that backs the pstore device in the guest.
> 
>  ``size``
>    Configures the size of the persistent storage available to the guest. It is
> diff --git a/src/conf/schemas/domaincommon.rng
> b/src/conf/schemas/domaincommon.rng
> index 6fcee2a70c..7d58dce465 100644
> --- a/src/conf/schemas/domaincommon.rng
> +++ b/src/conf/schemas/domaincommon.rng
> @@ -6261,9 +6261,11 @@
>          <value>acpi-erst</value>
>        </attribute>
>        <interleave>
> -        <element name="path">
> -          <ref name="absFilePath"/>
> -        </element>
> +        <optional>
> +          <element name="path">
> +            <ref name="absFilePath"/>
> +          </element>
> +        </optional>
>          <element name="size">
>            <ref name="scaledInteger"/>
>          </element>

I remember thinking - do not forget the schema, when writing this patch.
But then I forgot. That's what you get for writing patches before
morning coffee kicks in.

Thanks!

Michal