Re: [PATCH v5 01/29] Add boot-certs to s390-ccw-virtio machine type option

[Zhuoying Cai <zycai@linux.ibm.com>]

On 9/12/25 2:42 AM, Markus Armbruster wrote:
> Zhuoying Cai <zycai@linux.ibm.com> writes:
> 
>> Thanks for the feedback.
>>
>> On 9/11/25 3:24 AM, Markus Armbruster wrote:
>>> Zhuoying Cai <zycai@linux.ibm.com> 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 <zycai@linux.ibm.com>
>>>> ---
>>>>  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?
>>
>> A directory contains a list of certificate files, and allowing both
>> files and directories could make the CLI more flexible.
> 
> I figure when @path names a file, it's an error when the file doesn't
> contain a valid cetificate.
> 
> What is @path names a directory, and one of the directory's files
> doesn't contain a valid certificate?
> 
> Can a single file contain multiple certificates?
> 

A certificate file path is expected to contain exactly one certificate.

Certificates provided through the CLI, whether as individual files or
within a directory, are validated before use. If a certificate is
invalid (e.g., unsupported format), it will be skipped and not added to
the S390 certificate store.

When iterating through the provided paths, the program will terminate on
fatal configuration errors, such as when a specified path is neither a
file nor a directory.

>>>> +
>>>> +.. 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?
>>
>> If I understand correctly, would BootCerts also be the correct option to
>> use here?
> 
> This is object_class_property_add()'s @type argument.  It's an arbitrary
> string, not checked in any way.  When the property's actual type is a
> QAPI type, like it is here, then the @type argument should be the name
> of the QAPI type.
> 
> Questions?
> 
>>>> +                              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?
>>>
>>
>> I wrapped the path in a struct to follow the array-style structure used
>> by cxl-fmw and smp-cache options (as Daniel suggested).
>>
>> ```
>>   It would be something like this on the CLI:
>>
>>
>> boot-certs.0.path=/path/to/dir,boot-certs.1.path=/to/other/dir,boot-certs.2.path=/some/...
>> ```
>>
>> This could potentially leave room for future extensions, such as
>> including details about the certificate (e.g., issuer, hashing
>> algorithm, etc).
> 
> No objections to the wrapping.  I'd prefer naming the struct
> BootCertificates.
> 

I'll update this in the next version.

>>>> +{ '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?
>>
>> Please correct me if I'm wrong, but from what I've seen, it is not used
>> directly in the implementation. It provides a list property for
>> BootCertPath, which makes the BootCertPathList definition valid and able
>> to accept multiple paths. If BootCerts is removed, then BootCertPathList
>> becomes underfined and results in compilation errors.
> 
> Aha!
> 
> Your QOM property setter and getter need visit_type_BootCertPathList().
> 
> The QAPI generator generates code for BootCertPathList, including
> visit_type_BootCertPathList(), only when it knows it's actually used.
> It can only see uses within the QAPI schema.  So, when ['BootCertPath']
> doesn't occur there, visit_type_BootCertPathList() won't exist, and your
> QOM code won't compile.
> 
> Since you don't actually need BootCertPathList in the schema, you need
> to add an artifical use just to get it generated.  The common way to do
> that is using it in a dummy type like this:
> 
>     ##
>     # @DummyForceS390Arrays:
>     #
>     # Not used by QMP; hack to let us use BootCertPathList internally
>     #
>     # Since: 10.2
>     ##
>     { 'struct': 'DummyForceArrays',
>       'data': { 'unused': ['BootCertPath'] } }
> 
> You also need to add it to pragme documentation-exceptions is
> pragma.json.
> 
> Yes, this is clunky.  Sorry :)
> 

Thanks for all the explanations! I'll make the changes in the next version.

> [...]
>