Zhuoying Cai <[email protected]> writes:
> Introduce a new `boot-certs` machine type option for the s390-ccw-virtio
> machine. This allows users to specify one or more certificate file paths
> or directories to be used during secure boot.
>
> Each entry is specified using the syntax:
> boot-certs.<index>.path=/path/to/cert.pem
>
> Multiple paths can be specify using array properties:
> boot-certs.0.path=/path/to/cert.pem,
> boot-certs.1.path=/path/to/cert-dir,
> boot-certs.2.path=/path/to/another-dir...
>
> Signed-off-by: Zhuoying Cai <[email protected]>
> ---
> docs/system/s390x/secure-ipl.rst | 20 ++++++++++++++++++++
> hw/s390x/s390-virtio-ccw.c | 30 ++++++++++++++++++++++++++++++
> include/hw/s390x/s390-virtio-ccw.h | 2 ++
> qapi/machine-s390x.json | 24 ++++++++++++++++++++++++
> qemu-options.hx | 6 +++++-
> 5 files changed, 81 insertions(+), 1 deletion(-)
> create mode 100644 docs/system/s390x/secure-ipl.rst
>
> diff --git a/docs/system/s390x/secure-ipl.rst
> b/docs/system/s390x/secure-ipl.rst
> new file mode 100644
> index 0000000000..9b3fd25cc4
> --- /dev/null
> +++ b/docs/system/s390x/secure-ipl.rst
> @@ -0,0 +1,20 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Secure IPL Command Line Options
> +===============================
> +
> +New parameters have been introduced to s390-ccw-virtio machine type option
> +to support secure IPL. These parameters allow users to provide certificates
> +and enable secure IPL directly via the command line.
All too soon these parameters will no longer be new. Consider something
like "The s390-ccw-virtio machine type supports secure TPL. To enable
it, you need to provide certificates."
> +
> +Providing Certificates
> +----------------------
> +
> +The certificate store can be populated by supplying a list of certificate
> file
> +paths or directories on the command-line:
File is clear enough (use the certificate found in the file). What does
directory do?
> +
> +.. code-block:: shell
> +
> + qemu-system-s390x -machine s390-ccw-virtio, \
> + boot-certs.0.path=/.../qemu/certs, \
> + boot-certs.1.path=/another/path/cert.pem ...
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index c294106a74..9ac425c695 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -45,6 +45,7 @@
> #include "target/s390x/kvm/pv.h"
> #include "migration/blocker.h"
> #include "qapi/visitor.h"
> +#include "qapi/qapi-visit-machine-s390x.h"
> #include "hw/s390x/cpu-topology.h"
> #include "kvm/kvm_s390x.h"
> #include "hw/virtio/virtio-md-pci.h"
> @@ -798,6 +799,30 @@ static void machine_set_loadparm(Object *obj, Visitor *v,
> g_free(val);
> }
>
> +static void machine_get_boot_certs(Object *obj, Visitor *v,
> + const char *name, void *opaque,
> + Error **errp)
> +{
> + S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
> + BootCertPathList **certs = &ms->boot_certs;
> +
> + visit_type_BootCertPathList(v, name, certs, errp);
> +}
> +
> +static void machine_set_boot_certs(Object *obj, Visitor *v, const char *name,
> + void *opaque, Error **errp)
> +{
> + S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
> + BootCertPathList *cert_list = NULL;
> +
> + visit_type_BootCertPathList(v, name, &cert_list, errp);
> + if (!cert_list) {
> + return;
> + }
> +
> + ms->boot_certs = cert_list;
> +}
> +
> static void ccw_machine_class_init(ObjectClass *oc, const void *data)
> {
> MachineClass *mc = MACHINE_CLASS(oc);
> @@ -851,6 +876,11 @@ static void ccw_machine_class_init(ObjectClass *oc,
> const void *data)
> "Up to 8 chars in set of [A-Za-z0-9. ] (lower case chars
> converted"
> " to upper case) to pass to machine loader, boot manager,"
> " and guest kernel");
> +
> + object_class_property_add(oc, "boot-certs", "BootCertPath",
Isn't this a BootCertPathList, not a BootCertPath?
> + machine_get_boot_certs,
> machine_set_boot_certs, NULL, NULL);
> + object_class_property_set_description(oc, "boot-certs",
> + "provide paths to a directory and/or a certificate file for
> secure boot");
> }
>
> static inline void s390_machine_initfn(Object *obj)
> diff --git a/include/hw/s390x/s390-virtio-ccw.h
> b/include/hw/s390x/s390-virtio-ccw.h
> index 526078a4e2..b90949355c 100644
> --- a/include/hw/s390x/s390-virtio-ccw.h
> +++ b/include/hw/s390x/s390-virtio-ccw.h
> @@ -14,6 +14,7 @@
> #include "hw/boards.h"
> #include "qom/object.h"
> #include "hw/s390x/sclp.h"
> +#include "qapi/qapi-types-machine-s390x.h"
>
> #define TYPE_S390_CCW_MACHINE "s390-ccw-machine"
>
> @@ -31,6 +32,7 @@ struct S390CcwMachineState {
> uint8_t loadparm[8];
> uint64_t memory_limit;
> uint64_t max_pagesize;
> + BootCertPathList *boot_certs;
>
> SCLPDevice *sclp;
> };
> diff --git a/qapi/machine-s390x.json b/qapi/machine-s390x.json
> index 966dbd61d2..3e89ef8320 100644
> --- a/qapi/machine-s390x.json
> +++ b/qapi/machine-s390x.json
> @@ -119,3 +119,27 @@
> { 'command': 'query-s390x-cpu-polarization', 'returns':
> 'CpuPolarizationInfo',
> 'features': [ 'unstable' ]
> }
> +
> +##
> +# @BootCertPath:
> +#
> +# Boot certificate path.
> +#
> +# @path: path of certificate(s)
> +#
> +# Since: 10.1
> +##
No mention of file vs. directory.
Why do you wrap the file name in a struct? One possible reason is
providing for future extensions. Can you think of any?
If you extend it, the name BootCertPath could become suboptimal.
BootCertificate?
> +{ 'struct': 'BootCertPath',
> + 'data': {'path': 'str'} }
> +
> +##
> +# @BootCerts:
> +#
> +# List of boot certificate paths.
> +#
> +# @boot-certs: List of BootCertPath
Anti-pattern: the description text restates the type.
> +#
> +# Since: 10.1
> +##
> +{ 'struct': 'BootCerts',
> + 'data': {'boot-certs': ['BootCertPath'] } }
Where is this type used?
> diff --git a/qemu-options.hx b/qemu-options.hx
> index ab23f14d21..ac497eb3a0 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -44,7 +44,8 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
> #endif
> " memory-backend='backend-id' specifies explicitly
> provided backend for main RAM (default=none)\n"
> "
> cxl-fmw.0.targets.0=firsttarget,cxl-fmw.0.targets.1=secondtarget,cxl-fmw.0.size=size[,cxl-fmw.0.interleave-granularity=granularity]\n"
> - "
> smp-cache.0.cache=cachename,smp-cache.0.topology=topologylevel\n",
> + "
> smp-cache.0.cache=cachename,smp-cache.0.topology=topologylevel\n"
> + "
> boot-certs.0.path=/path/directory,boot-certs.1.path=/path/file provides paths
> to a directory and/or a certificate file\n",
> QEMU_ARCH_ALL)
> SRST
> ``-machine [type=]name[,prop=value[,...]]``
> @@ -205,6 +206,9 @@ SRST
> ::
>
> -machine
> smp-cache.0.cache=l1d,smp-cache.0.topology=core,smp-cache.1.cache=l1i,smp-cache.1.topology=core
> +
> + ``boot-certs.0.path=/path/directory,boot-certs.1.path=/path/file``
> + Provide paths to a directory and/or a certificate file on the host
> [s390x only].
> ERST
>
> DEF("M", HAS_ARG, QEMU_OPTION_M,
