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

From: Stefano Stabellini <sstabellini@kernel.org>
To: Vikram Garhwal <vikram.garhwal@amd.com>
Cc: "Alex Bennée" <alex.bennee@linaro.org>,
	qemu-dev@xilinx.com, stefano.stabellini@amd.com,
	xen-devel@lists.xenproject.org,
	"Peter Maydell" <peter.maydell@linaro.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Anthony Perard" <anthony.perard@citrix.com>,
	"Paul Durrant" <paul@xen.org>,
	"open list:ARM TCG CPUs" <qemu-arm@nongnu.org>,
	"open list:All patches CC here" <qemu-devel@nongnu.org>
Subject: Re: [QEMU][PATCH v2 10/11] hw/arm: introduce xenpv machine
Date: Fri, 2 Dec 2022 14:36:14 -0800 (PST)	[thread overview]
Message-ID: <alpine.DEB.2.22.394.2212021429220.4039@ubuntu-linux-20-04-desktop> (raw)
In-Reply-To: <ade61d47-f8c0-09cc-1a44-faaaff87d76a@amd.com>

[-- Attachment #1: Type: text/plain, Size: 5352 bytes --]

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.