[QEMU,v2,10/11] hw/arm: introduce xenpv machine

Add a new machine xenpv 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 xenpv QEMU to connect to swtpm:
    -chardev socket,id=chrtpm,path=/tmp/myvtpm2/swtpm-sock \
    -tpmdev emulator,id=tpm0,chardev=chrtpm \

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 &

/* Comment about machine name. Will be removed in next version*/
Please reply with the machine name you agree. Below are two possible names:
1. xenpv
2. xenpvh

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
---
 hw/arm/meson.build            |   2 +
 hw/arm/xen_arm.c              | 175 ++++++++++++++++++++++++++++++++++
 include/hw/arm/xen_arch_hvm.h |   9 ++
 include/hw/xen/arch_hvm.h     |   2 +
 4 files changed, 188 insertions(+)
 create mode 100644 hw/arm/xen_arm.c
 create mode 100644 include/hw/arm/xen_arch_hvm.h

On 02.12.22 04:00, Vikram Garhwal wrote:
> Add a new machine xenpv 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 xenpv QEMU to connect to swtpm:
>      -chardev socket,id=chrtpm,path=/tmp/myvtpm2/swtpm-sock \
>      -tpmdev emulator,id=tpm0,chardev=chrtpm \
> 
> 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 &
> 
> /* Comment about machine name. Will be removed in next version*/
> Please reply with the machine name you agree. Below are two possible names:
> 1. xenpv
> 2. xenpvh

Please us xenpvh, as on Arm the guests are more like PVH guests.

This will be needed on x86 to support e.g. virtio for PVH guests, and xenpv
is already taken on x86 for PV guests.


Juergen

On 2/12/22 04:00, Vikram Garhwal wrote:
> Add a new machine xenpv 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 xenpv QEMU to connect to swtpm:
>      -chardev socket,id=chrtpm,path=/tmp/myvtpm2/swtpm-sock \
>      -tpmdev emulator,id=tpm0,chardev=chrtpm \
> 
> 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 &
> 
> /* Comment about machine name. Will be removed in next version*/
> Please reply with the machine name you agree. Below are two possible names:
> 1. xenpv
> 2. xenpvh
> 
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> ---
>   hw/arm/meson.build            |   2 +
>   hw/arm/xen_arm.c              | 175 ++++++++++++++++++++++++++++++++++
>   include/hw/arm/xen_arch_hvm.h |   9 ++
>   include/hw/xen/arch_hvm.h     |   2 +
>   4 files changed, 188 insertions(+)
>   create mode 100644 hw/arm/xen_arm.c
>   create mode 100644 include/hw/arm/xen_arch_hvm.h


> 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

Do we really want to maintain a 32-bit PV machine?

Vikram Garhwal <vikram.garhwal@amd.com> writes:

> Add a new machine xenpv 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 xenpv QEMU to connect to swtpm:
>     -chardev socket,id=chrtpm,path=/tmp/myvtpm2/swtpm-sock \
>     -tpmdev emulator,id=tpm0,chardev=chrtpm \
>
> 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 &
>
> /* Comment about machine name. Will be removed in next version*/
> Please reply with the machine name you agree. Below are two possible names:
> 1. xenpv
> 2. xenpvh
>
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> ---
>  hw/arm/meson.build            |   2 +
>  hw/arm/xen_arm.c              | 175 ++++++++++++++++++++++++++++++++++
>  include/hw/arm/xen_arch_hvm.h |   9 ++
>  include/hw/xen/arch_hvm.h     |   2 +
>  4 files changed, 188 insertions(+)
>  create mode 100644 hw/arm/xen_arm.c
>  create mode 100644 include/hw/arm/xen_arch_hvm.h
>
> diff --git a/hw/arm/meson.build b/hw/arm/meson.build
> index 92f9f6e000..0cae024374 100644
> --- a/hw/arm/meson.build
> +++ b/hw/arm/meson.build
> @@ -62,5 +62,7 @@ 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('smmu-common.c', '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)
>  
>  hw_arch += {'arm': arm_ss}
> diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c
> new file mode 100644
> index 0000000000..bcb8e95f2c
> --- /dev/null
> +++ b/hw/arm/xen_arm.c
> @@ -0,0 +1,175 @@
> +/*
> + * QEMU ARM Xen PV Machine
> + *
> + *
> + * 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 "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("xenpv")
> +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;
> +};
> +
> +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)
> +{
> +}
> +

This function is only used under CONFIG_TPM so without it you get:

  ../../hw/arm/xen_arm.c:78:12: error: ‘xen_init_ioreq’ defined but not used [-Werror=unused-function]
     78 | static int xen_init_ioreq(XenIOState *state, unsigned int max_cpus)
        |            ^~~~~~~~~~~~~~
  cc1: all warnings being treated as errors

I think I reported this on v1 as well.

> +static int xen_init_ioreq(XenIOState *state, unsigned int max_cpus)
> +{
> +    xen_dmod = xendevicemodel_open(0, 0);
> +    if (xen_dmod == NULL) {
> +        perror("xen: can't open xen device model interface\n");
> +        return -1;
> +    }
> +
> +    xen_xc = xc_interface_open(0, 0, 0);
> +    if (xen_xc == NULL) {
> +        perror("xen: can't open xen interface\n");
> +        return -1;
> +    }
> +
> +    xen_fmem = xenforeignmemory_open(0, 0);
> +    if (xen_fmem == NULL) {
> +        perror("xen: can't open xen fmem interface\n");
> +        xc_interface_close(xen_xc);
> +        return -1;
> +    }
> +
> +    xen_register_ioreq(state, max_cpus, xen_memory_listener);
> +
> +    xen_register_backend(state);
> +
> +    xenstore_record_dm_state(state->xenstore, "running");
> +
> +    return 0;
> +}
> +
> +static void xen_enable_tpm(void)
> +{
> +/* qemu_find_tpm_be is only available when CONFIG_TPM is enabled. */
> +#ifdef CONFIG_TPM
> +    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, GUEST_TPM_BASE);
> +
> +    DPRINTF("Connected tpmdev at address 0x%lx\n", GUEST_TPM_BASE);
> +#endif
> +}
> +
> +static void xen_arm_init(MachineState *machine)
> +{
> +    XenArmState *xam = XEN_ARM(machine);
> +
> +    xam->state =  g_new0(XenIOState, 1);
> +
> +    /* For now enable IOREQ only for CONFIG_TPM. */
> +#ifdef CONFIG_TPM
> +    if (xen_init_ioreq(xam->state, machine->smp.cpus)) {
> +        return;
> +    }
> +#endif
> +
> +    xen_enable_tpm();

In fact this just looks weird - you guard the call to init_ioreq with
ifdefs guarding it but nest those ifdefs in xen_enable_tpm. Surely you
want the IOREQ server support even if you don't have TPM. Otherwise it
should be a hard configure requirement.

> +
> +    return;
> +}
> +
> +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;
> +
> +#ifdef CONFIG_TPM
> +    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

Vikram Garhwal <vikram.garhwal@amd.com> writes:

> Add a new machine xenpv 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 xenpv QEMU to connect to swtpm:
>     -chardev socket,id=chrtpm,path=/tmp/myvtpm2/swtpm-sock \
>     -tpmdev emulator,id=tpm0,chardev=chrtpm \
>
> 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 &
<snip>
> +
> +static void xen_enable_tpm(void)
> +{
> +/* qemu_find_tpm_be is only available when CONFIG_TPM is enabled. */
> +#ifdef CONFIG_TPM
> +    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, GUEST_TPM_BASE);

Still fails on my aarch64 Debian machine:

  FAILED: libqemu-aarch64-softmmu.fa.p/hw_arm_xen_arm.c.o 
  cc -Ilibqemu-aarch64-softmmu.fa.p -I. -I../.. -Itarget/arm -I../../target/arm -Iqapi -Itrace -Iui -Iui/shader -I/usr/include/pixman-1 -I/usr/local/include -I/usr/include/capstone -I/usr/include/spice-server -I/usr/include/spice-1 -I/usr/include/glib-2.0 -I/usr/lib/aarch64-linux-gnu/glib-2.0/include -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -isystem /home/alex/lsrc/qemu.git/linux-headers -isystem linux-headers -iquote . -iquote /home/alex/lsrc/qemu.git -iquote /home/alex/lsrc/qemu.git/include -iquote /home/alex/lsrc/qemu.git/tcg/aarch64 -pthread -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wold-style-declaration -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi -fstack-protector-strong -fPIE -isystem../../linux-headers -isystemlinux-headers -DNEED_CPU_H '-DCONFIG_TARGET="aarch64-softmmu-config-target.h"' '-DCONFIG_DEVICES="aarch64-softmmu-config-devices.h"' -MD -MQ libqemu-aarch64-softmmu.fa.p/hw_arm_xen_arm.c.o -MF libqemu-aarch64-softmmu.fa.p/hw_arm_xen_arm.c.o.d -o libqemu-aarch64-softmmu.fa.p/hw_arm_xen_arm.c.o -c ../../hw/arm/xen_arm.c
  ../../hw/arm/xen_arm.c: In function ‘xen_enable_tpm’:
  ../../hw/arm/xen_arm.c:126:32: error: ‘GUEST_TPM_BASE’ undeclared (first use in this function); did you mean ‘GUEST_RAM_BASE’?
    126 |     sysbus_mmio_map(busdev, 0, GUEST_TPM_BASE);
        |                                ^~~~~~~~~~~~~~
        |                                GUEST_RAM_BASE
  ../../hw/arm/xen_arm.c:126:32: note: each undeclared identifier is reported only once for each function it appears in
  [2082/3246] Compiling C object libqemu-aarch64-softmmu.fa.p/hw_xen_xen-mapcache.c.o
  [2083/3246] Compiling C object libqemu-aarch64-softmmu.fa.p/hw_xen_xen-hvm-common.c.o
  ninja: build stopped: subcommand failed.
  make: *** [Makefile:165: run-ninja] Error 1

> +
> +    DPRINTF("Connected tpmdev at address 0x%lx\n", GUEST_TPM_BASE);
> +#endif
> +}

If there is a minimum required version for TPM then it needs to be
picked up by configure.

Hi Alex,

On 12/2/22 6:52 AM, Alex Bennée wrote:
> Vikram Garhwal <vikram.garhwal@amd.com> writes:
>
>> Add a new machine xenpv 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 xenpv QEMU to connect to swtpm:
>>      -chardev socket,id=chrtpm,path=/tmp/myvtpm2/swtpm-sock \
>>      -tpmdev emulator,id=tpm0,chardev=chrtpm \
>>
>> 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 &
> <snip>
>> +
>> +static void xen_enable_tpm(void)
>> +{
>> +/* qemu_find_tpm_be is only available when CONFIG_TPM is enabled. */
>> +#ifdef CONFIG_TPM
>> +    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, GUEST_TPM_BASE);
> Still fails on my aarch64 Debian machine:
>
>    FAILED: libqemu-aarch64-softmmu.fa.p/hw_arm_xen_arm.c.o
>    cc -Ilibqemu-aarch64-softmmu.fa.p -I. -I../.. -Itarget/arm -I../../target/arm -Iqapi -Itrace -Iui -Iui/shader -I/usr/include/pixman-1 -I/usr/local/include -I/usr/include/capstone -I/usr/include/spice-server -I/usr/include/spice-1 -I/usr/include/glib-2.0 -I/usr/lib/aarch64-linux-gnu/glib-2.0/include -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -isystem /home/alex/lsrc/qemu.git/linux-headers -isystem linux-headers -iquote . -iquote /home/alex/lsrc/qemu.git -iquote /home/alex/lsrc/qemu.git/include -iquote /home/alex/lsrc/qemu.git/tcg/aarch64 -pthread -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wold-style-declaration -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi -fstack-protector-strong -fPIE -isystem../../linux-headers -isystemlinux-headers -DNEED_CPU_H '-DCONFIG_TARGET="aarch64-softmmu-config-target.h"' '-DCONFIG_DEVICES="aarch64-softmmu-config-devices.h"' -MD -MQ libqemu-aarch64-softmmu.fa.p/hw_arm_xen_arm.c.o -MF libqemu-aarch64-softmmu.fa.p/hw_arm_xen_arm.c.o.d -o libqemu-aarch64-softmmu.fa.p/hw_arm_xen_arm.c.o -c ../../hw/arm/xen_arm.c
>    ../../hw/arm/xen_arm.c: In function ‘xen_enable_tpm’:
>    ../../hw/arm/xen_arm.c:126:32: error: ‘GUEST_TPM_BASE’ undeclared (first use in this function); did you mean ‘GUEST_RAM_BASE’?
>      126 |     sysbus_mmio_map(busdev, 0, GUEST_TPM_BASE);
>          |                                ^~~~~~~~~~~~~~
>          |                                GUEST_RAM_BASE
>    ../../hw/arm/xen_arm.c:126:32: note: each undeclared identifier is reported only once for each function it appears in
>    [2082/3246] Compiling C object libqemu-aarch64-softmmu.fa.p/hw_xen_xen-mapcache.c.o
>    [2083/3246] Compiling C object libqemu-aarch64-softmmu.fa.p/hw_xen_xen-hvm-common.c.o
>    ninja: build stopped: subcommand failed.
>    make: *** [Makefile:165: run-ninja] Error 1
>
Do you know what Xen version your build env has?

Another way to fix this(as Julien suggested) is by setting this 
GUEST_TPM_BASE value via a property or something and user can set it via 
command line.

@sstabellini@kernel.org, do you think of any other fix?
>> +
>> +    DPRINTF("Connected tpmdev at address 0x%lx\n", GUEST_TPM_BASE);
>> +#endif
>> +}
> If there is a minimum required version for TPM then it needs to be
> picked up by configure.
>

On Fri, 2 Dec 2022, Vikram Garhwal wrote:
> On 12/2/22 6:52 AM, Alex Bennée wrote:
> > Vikram Garhwal <vikram.garhwal@amd.com> writes:
> > 
> > > Add a new machine xenpv 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 xenpv QEMU to connect to swtpm:
> > >      -chardev socket,id=chrtpm,path=/tmp/myvtpm2/swtpm-sock \
> > >      -tpmdev emulator,id=tpm0,chardev=chrtpm \
> > > 
> > > 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 &
> > <snip>
> > > +
> > > +static void xen_enable_tpm(void)
> > > +{
> > > +/* qemu_find_tpm_be is only available when CONFIG_TPM is enabled. */
> > > +#ifdef CONFIG_TPM
> > > +    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, GUEST_TPM_BASE);
> > Still fails on my aarch64 Debian machine:
> > 
> >    FAILED: libqemu-aarch64-softmmu.fa.p/hw_arm_xen_arm.c.o
> >    cc -Ilibqemu-aarch64-softmmu.fa.p -I. -I../.. -Itarget/arm
> > -I../../target/arm -Iqapi -Itrace -Iui -Iui/shader -I/usr/include/pixman-1
> > -I/usr/local/include -I/usr/include/capstone -I/usr/include/spice-server
> > -I/usr/include/spice-1 -I/usr/include/glib-2.0
> > -I/usr/lib/aarch64-linux-gnu/glib-2.0/include -fdiagnostics-color=auto -Wall
> > -Winvalid-pch -Werror -std=gnu11 -O2 -g -isystem
> > /home/alex/lsrc/qemu.git/linux-headers -isystem linux-headers -iquote .
> > -iquote /home/alex/lsrc/qemu.git -iquote /home/alex/lsrc/qemu.git/include
> > -iquote /home/alex/lsrc/qemu.git/tcg/aarch64 -pthread -U_FORTIFY_SOURCE
> > -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
> > -Wstrict-prototypes -Wredundant-decls -Wundef -Wwrite-strings
> > -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv
> > -Wold-style-declaration -Wold-style-definition -Wtype-limits
> > -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body
> > -Wnested-externs -Wendif-labels -Wexpansion-to-defined
> > -Wimplicit-fallthrough=2 -Wno-missing-include-dirs -Wno-shift-negative-value
> > -Wno-psabi -fstack-protector-strong -fPIE -isystem../../linux-headers
> > -isystemlinux-headers -DNEED_CPU_H
> > '-DCONFIG_TARGET="aarch64-softmmu-config-target.h"'
> > '-DCONFIG_DEVICES="aarch64-softmmu-config-devices.h"' -MD -MQ
> > libqemu-aarch64-softmmu.fa.p/hw_arm_xen_arm.c.o -MF
> > libqemu-aarch64-softmmu.fa.p/hw_arm_xen_arm.c.o.d -o
> > libqemu-aarch64-softmmu.fa.p/hw_arm_xen_arm.c.o -c ../../hw/arm/xen_arm.c
> >    ../../hw/arm/xen_arm.c: In function ‘xen_enable_tpm’:
> >    ../../hw/arm/xen_arm.c:126:32: error: ‘GUEST_TPM_BASE’ undeclared (first
> > use in this function); did you mean ‘GUEST_RAM_BASE’?
> >      126 |     sysbus_mmio_map(busdev, 0, GUEST_TPM_BASE);
> >          |                                ^~~~~~~~~~~~~~
> >          |                                GUEST_RAM_BASE
> >    ../../hw/arm/xen_arm.c:126:32: note: each undeclared identifier is
> > reported only once for each function it appears in
> >    [2082/3246] Compiling C object
> > libqemu-aarch64-softmmu.fa.p/hw_xen_xen-mapcache.c.o
> >    [2083/3246] Compiling C object
> > libqemu-aarch64-softmmu.fa.p/hw_xen_xen-hvm-common.c.o
> >    ninja: build stopped: subcommand failed.
> >    make: *** [Makefile:165: run-ninja] Error 1
> > 
> Do you know what Xen version your build env has?

I think Alex is just building against upstream Xen. GUEST_TPM_BASE is
not defined there yet. I think we would need to introduce in
xen_common.h something like:

#ifndef GUEST_TPM_BASE
#define GUEST_TPM_BASE 0x0c000000
#endif

We already have similar code in xen_common.h for other things.  Also, it
would be best to get GUEST_TPM_BASE defined upstream in Xen first.


> Another way to fix this(as Julien suggested) is by setting this GUEST_TPM_BASE
> value via a property or something and user can set it via command line.
> 
> @sstabellini@kernel.org, do you think of any other fix?

Setting the TPM address from the command line is nice and preferable to
hardcoding the value in xen_common.h. It comes with the challenge that
it is not very scalable (imagine we have a dozen emulated devices) but
for now it is fine and a good way to start if you can arrange it.

Hi,

On 02/12/2022 22:36, Stefano Stabellini wrote:
>> Do you know what Xen version your build env has?
> 
> I think Alex is just building against upstream Xen. GUEST_TPM_BASE is
> not defined there yet. I think we would need to introduce in
> xen_common.h something like:
> 
> #ifndef GUEST_TPM_BASE
> #define GUEST_TPM_BASE 0x0c000000
> #endif

I think this would be a big mistake to add the two lines above in QEMU.

Libxl is responsible for creating the domain and generating the firwmare 
tables. Any mismatch of values will be a real pain to debug.

Even if...

> 
> We already have similar code in xen_common.h for other things.  Also, it
> would be best to get GUEST_TPM_BASE defined upstream in Xen first.

... we introduce upstream first, the guest layout is not part of the 
stable ABI and therefore could change from release to release.

> 
> 
>> Another way to fix this(as Julien suggested) is by setting this GUEST_TPM_BASE
>> value via a property or something and user can set it via command line.
>>
>> @sstabellini@kernel.org, do you think of any other fix?
> 
> Setting the TPM address from the command line is nice and preferable to
> hardcoding the value in xen_common.h. It comes with the challenge that
> it is not very scalable (imagine we have a dozen emulated devices) but
> for now it is fine and a good way to start if you can arrange it.

It is not clear which one you think is not scalable. If this is the 
command line option approach, then I think this is unrealistic to ask 
every user to rebuild there QEMU just because the guest layout has changed.

Today the rebuild may only be necessary when switching to a new release. 
But in the future we may imagine a per-domain layout (e.g. for legacy 
purpose). So you will now need to request the user to have one QEMU 
built per domain.

How is that scalable?

Cheers,

Julien Grall <julien@xen.org> writes:

> Hi,
>
> On 02/12/2022 22:36, Stefano Stabellini wrote:
>>> Do you know what Xen version your build env has?
>> I think Alex is just building against upstream Xen. GUEST_TPM_BASE
>> is
>> not defined there yet. I think we would need to introduce in
>> xen_common.h something like:
>> #ifndef GUEST_TPM_BASE
>> #define GUEST_TPM_BASE 0x0c000000
>> #endif
>
> I think this would be a big mistake to add the two lines above in QEMU.
>
> Libxl is responsible for creating the domain and generating the
> firwmare tables. Any mismatch of values will be a real pain to debug.
>
> Even if...
>
>> We already have similar code in xen_common.h for other things.
>> Also, it
>> would be best to get GUEST_TPM_BASE defined upstream in Xen first.
>
> ... we introduce upstream first, the guest layout is not part of the
> stable ABI and therefore could change from release to release.
>
>> 
>>> Another way to fix this(as Julien suggested) is by setting this GUEST_TPM_BASE
>>> value via a property or something and user can set it via command line.
>>>
>>> @sstabellini@kernel.org, do you think of any other fix?
>> Setting the TPM address from the command line is nice and preferable
>> to
>> hardcoding the value in xen_common.h. It comes with the challenge that
>> it is not very scalable (imagine we have a dozen emulated devices) but
>> for now it is fine and a good way to start if you can arrange it.
>
> It is not clear which one you think is not scalable. If this is the
> command line option approach, then I think this is unrealistic to ask
> every user to rebuild there QEMU just because the guest layout has
> changed.
>
> Today the rebuild may only be necessary when switching to a new
> release. But in the future we may imagine a per-domain layout (e.g.
> for legacy purpose). So you will now need to request the user to have
> one QEMU built per domain.

I agree if this is something that can change it needs to be configured
from the command line or somehow otherwise gleaned from the source of
truth. Isn't this information available via XenStore? Isn't that what
Viresh has to do for all the VirtIO devices he's adding to libxl?

A default value for the option could be done I guess.

>
> How is that scalable?
>
> Cheers,