[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v1 10/12] hw/arm: introduce xenpv machine
Hi,
There seem to be some missing patches on xen-devel (including the cover
letter). Is that expected?
On 15/10/2022 06:07, Vikram Garhwal wrote:
Add a new machine xenpv which creates a IOREQ server to register/connect with
Xen Hypervisor.
I don't like the name 'xenpv' because it doesn't convey the fact that
some of the HW may be emulated rather than para-virtualized. In fact one
may only want to use for emulating devices.
Potential name would be 'xen-arm' or re-using 'virt' but with
'accel=xen' to select a Xen layout.
Xen IOREQ connection expect the TARGET_PAGE_SIZE to 4096, and the xenpv machine
on ARM will have no CPU definitions. We need to define TARGET_PAGE_SIZE
appropriately ourselves.
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 &
I see patches for QEMU but not Xen. How can this be tested with existing
Xen? Will libxl ever create QEMU?
[...]
+static int xen_init_ioreq(XenIOState *state, unsigned int max_cpus)
+{
+ xen_dmod = xendevicemodel_open(0, 0);
+ xen_xc = xc_interface_open(0, 0, 0);
+
+ if (xen_xc == NULL) {
You are checking xen_xc but not xen_dmod. Why?
+ 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);
+
+ xenstore_record_dm_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);
I can't find where GUEST_TPM_BASE is defined. But then the guest memory
layout is not expected to be stable. With your current approach, it
means QEMU would need to be rebuilt for every Xen version. Is it what we
want?
+
+ 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);
+
+ if (xen_init_ioreq(xam->state, machine->smp.cpus)) {
+ return;
In another patch, you said the IOREQ would be optional. IHMO, I think
this is a bad idea to register it by default because one may only want
to use PV drivers. Registering IOREQ will add unnecessary overhead in Xen.
Furthermore, it means that someone selecting TPM but Xen is not built
with CONFIG_IOREQ=y (BTW This is still a tech preview but there are
security holes on Arm...) will not get an error. Instead, the OS will
until it crashes when trying to access the TPM.
Overall I think it would be better if IOREQ is only registered when a
device requires (like TPM) it *and* throw an error if there is a problem
during the initialization.
+ } > +
+ xen_enable_tpm();
+
+ 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;
+ machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_TIS_SYSBUS);
Shouldn't this be protected with #ifdef CONFIG_TPM?
+}
+
+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..f645dfec28
--- /dev/null
+++ b/include/hw/arm/xen_arch_hvm.h
@@ -0,0 +1,12 @@
+#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);
+
+#undef TARGET_PAGE_SIZE
I am a bit puzzled with this #undef. In the commit message you said that
there will be no CPU definition. So the implications is this should not
be defined.
If it is defined, then what guarantees that all the source will use the
correct value?
+#define TARGET_PAGE_SIZE 4096
It would be better to use XC_PAGE_SIZE (or similar) rather than
hardcoding it.
+#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
Cheers,
--
Julien Grall
|
