Re: [QEMU][PATCH v4 09/10] hw/arm: introduce xenpvh machine

[Vikram Garhwal <vikram.garhwal@amd.com>]

Hi Stefano,

On 1/25/23 2:20 PM, Stefano Stabellini wrote:
> On Wed, 25 Jan 2023, Vikram Garhwal wrote:
>> Add a new machine xenpvh which creates a IOREQ server to register/connect with
>> Xen Hypervisor.
>>
>> Optional: When CONFIG_TPM is enabled, it also creates a tpm-tis-device, adds a
>> TPM emulator and connects to swtpm running on host machine via chardev socket
>> and support TPM functionalities for a guest domain.
>>
>> Extra command line for aarch64 xenpvh QEMU to connect to swtpm:
>>      -chardev socket,id=chrtpm,path=/tmp/myvtpm2/swtpm-sock \
>>      -tpmdev emulator,id=tpm0,chardev=chrtpm \
>>      -machine tpm-base-addr=0x0c000000 \
>>
>> swtpm implements a TPM software emulator(TPM 1.2 & TPM 2) built on libtpms and
>> provides access to TPM functionality over socket, chardev and CUSE interface.
>> Github repo: https://github.com/stefanberger/swtpm
>> Example for starting swtpm on host machine:
>>      mkdir /tmp/vtpm2
>>      swtpm socket --tpmstate dir=/tmp/vtpm2 \
>>      --ctrl type=unixio,path=/tmp/vtpm2/swtpm-sock &
>>
>> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
>> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
>> ---
>>   docs/system/arm/xenpvh.rst    |  34 +++++++
>>   docs/system/target-arm.rst    |   1 +
>>   hw/arm/meson.build            |   2 +
>>   hw/arm/xen_arm.c              | 184 ++++++++++++++++++++++++++++++++++
>>   include/hw/arm/xen_arch_hvm.h |   9 ++
>>   include/hw/xen/arch_hvm.h     |   2 +
>>   6 files changed, 232 insertions(+)
>>   create mode 100644 docs/system/arm/xenpvh.rst
>>   create mode 100644 hw/arm/xen_arm.c
>>   create mode 100644 include/hw/arm/xen_arch_hvm.h
>>
>> diff --git a/docs/system/arm/xenpvh.rst b/docs/system/arm/xenpvh.rst
>> new file mode 100644
>> index 0000000000..e1655c7ab8
>> --- /dev/null
>> +++ b/docs/system/arm/xenpvh.rst
>> @@ -0,0 +1,34 @@
>> +XENPVH (``xenpvh``)
>> +=========================================
>> +This machine creates a IOREQ server to register/connect with Xen Hypervisor.
>> +
>> +When TPM is enabled, this machine also creates a tpm-tis-device at a user input
>> +tpm base address, adds a TPM emulator and connects to a swtpm application
>> +running on host machine via chardev socket. This enables xenpvh to support TPM
>> +functionalities for a guest domain.
>> +
>> +More information about TPM use and installing swtpm linux application can be
>> +found at: docs/specs/tpm.rst.
>> +
>> +Example for starting swtpm on host machine:
>> +.. code-block:: console
>> +
>> +    mkdir /tmp/vtpm2
>> +    swtpm socket --tpmstate dir=/tmp/vtpm2 \
>> +    --ctrl type=unixio,path=/tmp/vtpm2/swtpm-sock &
>> +
>> +Sample QEMU xenpvh commands for running and connecting with Xen:
>> +.. code-block:: console
>> +
>> +    qemu-system-aarch64 -xen-domid 1 \
>> +    -chardev socket,id=libxl-cmd,path=qmp-libxl-1,server=on,wait=off \
>> +    -mon chardev=libxl-cmd,mode=control \
>> +    -chardev socket,id=libxenstat-cmd,path=qmp-libxenstat-1,server=on,wait=off \
>> +    -mon chardev=libxenstat-cmd,mode=control \
>> +    -xen-attach -name guest0 -vnc none -display none -nographic \
>> +    -machine xenpvh -m 1301 \
>> +    -chardev socket,id=chrtpm,path=tmp/vtpm2/swtpm-sock \
>> +    -tpmdev emulator,id=tpm0,chardev=chrtpm -machine tpm-base-addr=0x0C000000
>> +
>> +In above QEMU command, last two lines are for connecting xenpvh QEMU to swtpm
>> +via chardev socket.
>> diff --git a/docs/system/target-arm.rst b/docs/system/target-arm.rst
>> index 91ebc26c6d..af8d7c77d6 100644
>> --- a/docs/system/target-arm.rst
>> +++ b/docs/system/target-arm.rst
>> @@ -106,6 +106,7 @@ undocumented; you can get a complete list by running
>>      arm/stm32
>>      arm/virt
>>      arm/xlnx-versal-virt
>> +   arm/xenpvh
>>   
>>   Emulated CPU architecture support
>>   =================================
>> diff --git a/hw/arm/meson.build b/hw/arm/meson.build
>> index b036045603..06bddbfbb8 100644
>> --- a/hw/arm/meson.build
>> +++ b/hw/arm/meson.build
>> @@ -61,6 +61,8 @@ arm_ss.add(when: 'CONFIG_FSL_IMX7', if_true: files('fsl-imx7.c', 'mcimx7d-sabre.
>>   arm_ss.add(when: 'CONFIG_ARM_SMMUV3', if_true: files('smmuv3.c'))
>>   arm_ss.add(when: 'CONFIG_FSL_IMX6UL', if_true: files('fsl-imx6ul.c', 'mcimx6ul-evk.c'))
>>   arm_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('nrf51_soc.c'))
>> +arm_ss.add(when: 'CONFIG_XEN', if_true: files('xen_arm.c'))
>> +arm_ss.add_all(xen_ss)
>>   
>>   softmmu_ss.add(when: 'CONFIG_ARM_SMMUV3', if_true: files('smmu-common.c'))
>>   softmmu_ss.add(when: 'CONFIG_EXYNOS4', if_true: files('exynos4_boards.c'))
>> diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c
>> new file mode 100644
>> index 0000000000..12b19e3609
>> --- /dev/null
>> +++ b/hw/arm/xen_arm.c
>> @@ -0,0 +1,184 @@
>> +/*
>> + * QEMU ARM Xen PV Machine
>                     ^ PVH
>
>
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/error-report.h"
>> +#include "qapi/qapi-commands-migration.h"
>> +#include "qapi/visitor.h"
>> +#include "hw/boards.h"
>> +#include "hw/sysbus.h"
>> +#include "sysemu/block-backend.h"
>> +#include "sysemu/tpm_backend.h"
>> +#include "sysemu/sysemu.h"
>> +#include "hw/xen/xen-legacy-backend.h"
>> +#include "hw/xen/xen-hvm-common.h"
>> +#include "sysemu/tpm.h"
>> +#include "hw/xen/arch_hvm.h"
>> +
>> +#define TYPE_XEN_ARM  MACHINE_TYPE_NAME("xenpvh")
>> +OBJECT_DECLARE_SIMPLE_TYPE(XenArmState, XEN_ARM)
>> +
>> +static MemoryListener xen_memory_listener = {
>> +    .region_add = xen_region_add,
>> +    .region_del = xen_region_del,
>> +    .log_start = NULL,
>> +    .log_stop = NULL,
>> +    .log_sync = NULL,
>> +    .log_global_start = NULL,
>> +    .log_global_stop = NULL,
>> +    .priority = 10,
>> +};
>> +
>> +struct XenArmState {
>> +    /*< private >*/
>> +    MachineState parent;
>> +
>> +    XenIOState *state;
>> +
>> +    struct {
>> +        uint64_t tpm_base_addr;
>> +    } cfg;
>> +};
>> +
>> +void arch_handle_ioreq(XenIOState *state, ioreq_t *req)
>> +{
>> +    hw_error("Invalid ioreq type 0x%x\n", req->type);
>> +
>> +    return;
>> +}
>> +
>> +void arch_xen_set_memory(XenIOState *state, MemoryRegionSection *section,
>> +                         bool add)
>> +{
>> +}
>> +
>> +void xen_hvm_modified_memory(ram_addr_t start, ram_addr_t length)
>> +{
>> +}
>> +
>> +void qmp_xen_set_global_dirty_log(bool enable, Error **errp)
>> +{
>> +}
>> +
>> +#ifdef CONFIG_TPM
>> +static void xen_enable_tpm(XenArmState *xam)
>> +{
>> +    Error *errp = NULL;
>> +    DeviceState *dev;
>> +    SysBusDevice *busdev;
>> +
>> +    TPMBackend *be = qemu_find_tpm_be("tpm0");
>> +    if (be == NULL) {
>> +        DPRINTF("Couldn't fine the backend for tpm0\n");
>> +        return;
>> +    }
>> +    dev = qdev_new(TYPE_TPM_TIS_SYSBUS);
>> +    object_property_set_link(OBJECT(dev), "tpmdev", OBJECT(be), &errp);
>> +    object_property_set_str(OBJECT(dev), "tpmdev", be->id, &errp);
>> +    busdev = SYS_BUS_DEVICE(dev);
>> +    sysbus_realize_and_unref(busdev, &error_fatal);
>> +    sysbus_mmio_map(busdev, 0, xam->cfg.tpm_base_addr);
>> +
>> +    DPRINTF("Connected tpmdev at address 0x%lx\n", xam->cfg.tpm_base_addr);
>> +}
>> +#endif
>> +
>> +static void xen_arm_init(MachineState *machine)
>> +{
>> +    XenArmState *xam = XEN_ARM(machine);
>> +
>> +    xam->state =  g_new0(XenIOState, 1);
>> +
>> +    xen_register_ioreq(xam->state, machine->smp.cpus, xen_memory_listener);
>> +
>> +#ifdef CONFIG_TPM
>> +    if (xam->cfg.tpm_base_addr) {
>> +        xen_enable_tpm(xam);
>> +    } else {
>> +        DPRINTF("tpm-base-addr is not provided. TPM will not be enabled\n");
>> +    }
> I would remove the "else", we already have a DPRINTF at the end of
> xen_enable_tpm.

This print is bit different than the one in /xen_enable_tpm/. I added it 
because now user needs to provide "tpm_base_addr=0x0C00_0000" from 
command line. If no /tpm_base_addr/ then /cfg.tpm_base_addr /value is 
0x0 and we don't wanna create tpm device at 0x0.

Perhaps instead of debug print, I print a warning here?

>
>
>> +#endif
>> +
>> +    return;
> the return is unnecessary
>
>
>> +}
>> +
>> +#ifdef CONFIG_TPM
>> +static void xen_arm_get_tpm_base_addr(Object *obj, Visitor *v,
>> +                                      const char *name, void *opaque,
>> +                                      Error **errp)
>> +{
>> +    XenArmState *xam = XEN_ARM(obj);
>> +    uint64_t value = xam->cfg.tpm_base_addr;
>> +
>> +    visit_type_uint64(v, name, &value, errp);
>> +}
>> +
>> +static void xen_arm_set_tpm_base_addr(Object *obj, Visitor *v,
>> +                                      const char *name, void *opaque,
>> +                                      Error **errp)
>> +{
>> +    XenArmState *xam = XEN_ARM(obj);
>> +    uint64_t value;
>> +
>> +    if (!visit_type_uint64(v, name, &value, errp)) {
>> +        return;
>> +    }
>> +
>> +    xam->cfg.tpm_base_addr = value;
>> +}
>> +#endif
>> +
>> +static void xen_arm_machine_class_init(ObjectClass *oc, void *data)
>> +{
>> +
>> +    MachineClass *mc = MACHINE_CLASS(oc);
>> +    mc->desc = "Xen Para-virtualized PC";
>> +    mc->init = xen_arm_init;
>> +    mc->max_cpus = 1;
>> +    mc->default_machine_opts = "accel=xen";
>> +
>> +#ifdef CONFIG_TPM
>> +    object_class_property_add(oc, "tpm-base-addr", "uint64_t",
>> +                              xen_arm_get_tpm_base_addr,
>> +                              xen_arm_set_tpm_base_addr,
>> +                              NULL, NULL);
>> +    object_class_property_set_description(oc, "tpm-base-addr",
>> +                                          "Set Base address for TPM device.");
>> +
>> +    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_TIS_SYSBUS);
>> +#endif
>> +}
>> +
>> +static const TypeInfo xen_arm_machine_type = {
>> +    .name = TYPE_XEN_ARM,
>> +    .parent = TYPE_MACHINE,
>> +    .class_init = xen_arm_machine_class_init,
>> +    .instance_size = sizeof(XenArmState),
>> +};
>> +
>> +static void xen_arm_machine_register_types(void)
>> +{
>> +    type_register_static(&xen_arm_machine_type);
>> +}
>> +
>> +type_init(xen_arm_machine_register_types)
>> diff --git a/include/hw/arm/xen_arch_hvm.h b/include/hw/arm/xen_arch_hvm.h
>> new file mode 100644
>> index 0000000000..8fd645e723
>> --- /dev/null
>> +++ b/include/hw/arm/xen_arch_hvm.h
>> @@ -0,0 +1,9 @@
>> +#ifndef HW_XEN_ARCH_ARM_HVM_H
>> +#define HW_XEN_ARCH_ARM_HVM_H
>> +
>> +#include <xen/hvm/ioreq.h>
>> +void arch_handle_ioreq(XenIOState *state, ioreq_t *req);
>> +void arch_xen_set_memory(XenIOState *state,
>> +                         MemoryRegionSection *section,
>> +                         bool add);
>> +#endif
>> diff --git a/include/hw/xen/arch_hvm.h b/include/hw/xen/arch_hvm.h
>> index 26674648d8..c7c515220d 100644
>> --- a/include/hw/xen/arch_hvm.h
>> +++ b/include/hw/xen/arch_hvm.h
>> @@ -1,3 +1,5 @@
>>   #if defined(TARGET_I386) || defined(TARGET_X86_64)
>>   #include "hw/i386/xen_arch_hvm.h"
>> +#elif defined(TARGET_ARM) || defined(TARGET_ARM_64)
>> +#include "hw/arm/xen_arch_hvm.h"
>>   #endif
>> -- 
>> 2.17.0
>>