[PATCH for v10.6.0 0/2] qemu: Two pstore improvements
These address Andrea's suggestion: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/EEIM... 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
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
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@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
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
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
participants (3)
-
Andrea Bolognani -
Michal Privoznik -
Michal Prívozník