4:31 a.m.
These address Andrea's suggestion:
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/E...
I know we've just entered freeze, but pstore is a new feature and it'd
be nice to have these in. The first one is technically a feature, but
the second one can be viewed as a bug fix.
Michal Prívozník (2):
qemu: Autofill pstore path if missing
qemu: Pre-create pstore device file
src/qemu/qemu_domain.c | 27 +++++++++++++++++++++++-
src/qemu/qemu_process.c | 46 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 72 insertions(+), 1 deletion(-)
--
2.44.2
4:31 a.m.
New subject: [PATCH 1/2] qemu: Autofill pstore path if missing
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(a)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
8:55 a.m.
New subject: [PATCH 1/2] qemu: Autofill pstore path if missing
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(a)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
10:23 a.m.
New subject: [PATCH 1/2] qemu: Autofill pstore path if missing
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(a)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
4:31 a.m.
New subject: [PATCH 2/2] qemu: Pre-create pstore device file
So far we are relying on QEMU or sysadmin to create the file for
pstore. This is suboptimal as in the case of the former we can
not set proper seclabels (there's nothing to set seclabels on
until QEMU is started).
Therefore, make sure the file is created before launching QEMU
and that it has the correct size.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_process.c | 46 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 46 insertions(+)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 6255ba92e7..7e8fdca43e 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6835,6 +6835,49 @@ qemuProcessPrepareLaunchSecurityGuestInput(virDomainObj *vm)
}
+static int
+qemuProcessPreparePstore(virDomainObj *vm)
+{
+ virDomainPstoreDef *pstore = vm->def->pstore;
+ VIR_AUTOCLOSE fd = -1;
+
+ if (!pstore)
+ return 0;
+
+ switch (pstore->backend) {
+ case VIR_DOMAIN_PSTORE_BACKEND_ACPI_ERST:
+ if ((fd = open(pstore->path, O_WRONLY | O_CREAT, 0600)) < 0) {
+ virReportSystemError(errno,
+ _("cannot create file '%1$s'"),
+ pstore->path);
+ return -1;
+ }
+
+ if (ftruncate(fd, pstore->size) < 0) {
+ virReportSystemError(errno,
+ _("Failed to truncate file '%1$s'"),
+ pstore->path);
+ return -1;
+ }
+
+ if (VIR_CLOSE(fd) < 0) {
+ virReportSystemError(errno,
+ _("Unable to save '%1$s'"),
+ pstore->path);
+ return -1;
+ }
+
+
+ break;
+
+ case VIR_DOMAIN_PSTORE_BACKEND_LAST:
+ break;
+ }
+
+ return 0;
+}
+
+
static int
qemuProcessPrepareHostStorageSourceVDPA(virStorageSource *src,
qemuDomainObjPrivate *priv)
@@ -7333,6 +7376,9 @@ qemuProcessPrepareHost(virQEMUDriver *driver,
if (qemuProcessPrepareLaunchSecurityGuestInput(vm) < 0)
return -1;
+ if (qemuProcessPreparePstore(vm) < 0)
+ return -1;
+
return 0;
}
--
2.44.2
9:04 a.m.
New subject: [PATCH 2/2] qemu: Pre-create pstore device file
On Mon, Jul 29, 2024 at 11:31:36AM GMT, Michal Privoznik wrote:
+static int
+qemuProcessPreparePstore(virDomainObj *vm)
+{
+ virDomainPstoreDef *pstore = vm->def->pstore;
+ VIR_AUTOCLOSE fd = -1;
+
+ if (!pstore)
+ return 0;
+
+ switch (pstore->backend) {
+ case VIR_DOMAIN_PSTORE_BACKEND_ACPI_ERST:
+ if ((fd = open(pstore->path, O_WRONLY | O_CREAT, 0600)) < 0) {
+ virReportSystemError(errno,
+ _("cannot create file '%1$s'"),
+ pstore->path);
+ return -1;
+ }
+
+ if (ftruncate(fd, pstore->size) < 0) {
+ virReportSystemError(errno,
+ _("Failed to truncate file '%1$s'"),
+ pstore->path);
+ return -1;
+ }
The size is stored in KiB but ftruncate(2) expect it in bytes, so you
need to multiply it by 1024 or QEMU will (correctly) complain about
the size mismatch on startup.
+ if (VIR_CLOSE(fd) < 0) {
+ virReportSystemError(errno,
+ _("Unable to save '%1$s'"),
+ pstore->path);
+ return -1;
+ }
+
+
+ break;
Unnecessary empty line.
Even with the fix mentioned above applied, I still get an error on
startup on my SELinux-enabled system. Running `setenforce 0` makes it
go away, but clearly that's not acceptable.
The diff below seems to do the trick, but I'm not entirely confident
that it's the right fix rather than a mere kludge.
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index ba0ce8fb9d..31df4d22db 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -3341,7 +3341,7 @@ virSecuritySELinuxSetAllLabel(virSecurityManager *mgr,
if (def->pstore &&
virSecuritySELinuxSetFilecon(mgr, def->pstore->path,
- data->content_context, true) < 0)
+ secdef->imagelabel, true) < 0)
return -1;
return 0;
--
Andrea Bolognani / Red Hat / Virtualization
10:20 a.m.
New subject: [PATCH 2/2] qemu: Pre-create pstore device file
On 7/30/24 16:04, Andrea Bolognani wrote:
On Mon, Jul 29, 2024 at 11:31:36AM GMT, Michal Privoznik wrote:
> +static int
> +qemuProcessPreparePstore(virDomainObj *vm)
> +{
> + virDomainPstoreDef *pstore = vm->def->pstore;
> + VIR_AUTOCLOSE fd = -1;
> +
> + if (!pstore)
> + return 0;
> +
> + switch (pstore->backend) {
> + case VIR_DOMAIN_PSTORE_BACKEND_ACPI_ERST:
> + if ((fd = open(pstore->path, O_WRONLY | O_CREAT, 0600)) < 0) {
> + virReportSystemError(errno,
> + _("cannot create file '%1$s'"),
> + pstore->path);
> + return -1;
> + }
> +
> + if (ftruncate(fd, pstore->size) < 0) {
> + virReportSystemError(errno,
> + _("Failed to truncate file
'%1$s'"),
> + pstore->path);
> + return -1;
> + }
The size is stored in KiB but ftruncate(2) expect it in bytes, so you
need to multiply it by 1024 or QEMU will (correctly) complain about
the size mismatch on startup.
> + if (VIR_CLOSE(fd) < 0) {
> + virReportSystemError(errno,
> + _("Unable to save '%1$s'"),
> + pstore->path);
> + return -1;
> + }
> +
> +
> + break;
Unnecessary empty line.
Even with the fix mentioned above applied, I still get an error on
startup on my SELinux-enabled system. Running `setenforce 0` makes it
go away, but clearly that's not acceptable.
The diff below seems to do the trick, but I'm not entirely confident
that it's the right fix rather than a mere kludge.
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index ba0ce8fb9d..31df4d22db 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -3341,7 +3341,7 @@ virSecuritySELinuxSetAllLabel(virSecurityManager *mgr,
if (def->pstore &&
virSecuritySELinuxSetFilecon(mgr, def->pstore->path,
- data->content_context, true) < 0)
+ secdef->imagelabel, true) < 0)
Ah, correct. I don't run SELinux enabled system, hence I did not catch
this in my testing. The thing is, pstore is somewhat similar to NVRAM
store - QEMU must be able to not just read it but also write to it.
Hence, it's different to other files around this line (kernel image,
initrd, DTB, SLIC table) - those are RO. Looking into what NVRAM would
use - it in fact boils down to secdef->imagelabel.
I'll post this as a separate patch.
Thanks for catching this and prompt review!
Michal
115
days inactive
116
days old
6 comments
3 participants
participants (3)
-
Andrea Bolognani -
Michal Privoznik -
Michal Prívozník