2:47 a.m.
On 09.06.2016 19:06, Jim Fehlig wrote:
Michal Privoznik wrote:
> On 09.06.2016 16:46, Jim Fehlig wrote:
>> Add support to xenconfig for conversion of xl.cfg(5) bios config
>> to/from libvirt domXml <loader> config. SeaBIOS is the default
>> for HVM guests using upstream QEMU. ROMBIOS is the default when
>> using the old qemu-dm. This patch allows specifying OVMF as an
>> alternate firmware.
>>
>> Example xl.cfg:
>> bios = "ovmf"
>>
>> Example domXML:
>> <os>
>> ...
>> <loader readonly='yes'
type='pflash'>/usr/lib/xen/boot/ovmf.bin</loader>
>> </os>
>>
>> An alternate OVMF firmware (from the one advertised in
>> domaincapabilities) can be specified with
>>
>> bios = "ovmf"
>> bios_override = "/path/to/my/ovmf.bin"
>>
>> Note that currently, Xen does not support a separate nvram for
>> non-volatile variables.
>>
>> Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
>> ---
>> src/Makefile.am | 2 +-
>> src/xenconfig/xen_xl.c | 50 ++++++++++++++++---
>> tests/xlconfigdata/test-fullvirt-ovmf-override.cfg | 27 ++++++++++
>> tests/xlconfigdata/test-fullvirt-ovmf-override.xml | 58 ++++++++++++++++++++++
>> tests/xlconfigdata/test-fullvirt-ovmf.cfg | 26 ++++++++++
>> tests/xlconfigdata/test-fullvirt-ovmf.xml | 58 ++++++++++++++++++++++
>> tests/xlconfigtest.c | 2 +
>> 7 files changed, 216 insertions(+), 7 deletions(-)
>>
>
> Looking good, but just one small nit.
>
>> diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
>> index 9b8306f..bbe53ee 100644
>> --- a/src/xenconfig/xen_xl.c
>> +++ b/src/xenconfig/xen_xl.c
>
>
>> @@ -535,6 +561,18 @@ xenFormatXLOS(virConfPtr conf, virDomainDefPtr def)
>> if (xenConfigSetString(conf, "builder", "hvm") <
0)
>> return -1;
>>
>> + if (def->os.loader &&
>> + def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH) {
>> + if (xenConfigSetString(conf, "bios", "ovmf")
< 0)
>> + return -1;
>> +
>> + if (STRNEQ(def->os.loader->path, LIBXL_FIRMWARE_DIR
"/ovmf.bin")) {
>
> On my system LIBXL_FIRMWARE_DIR is:
>
> $ pkg-config --variable=xenfirmwaredir xenlight
> /usr/libexec/xen/boot
>
>> + if (xenConfigSetString(conf, "bios_override",
>> + def->os.loader->path) < 0)
>
> and since "bios_override" is set only if it points somewhere else ...
>
>> + return -1;
>> + }
>> + }
>> +
>> #ifdef LIBXL_HAVE_BUILDINFO_KERNEL
>> if (def->os.kernel &&
>> xenConfigSetString(conf, "kernel", def->os.kernel) <
0)
>> diff --git a/tests/xlconfigdata/test-fullvirt-ovmf-override.cfg
b/tests/xlconfigdata/test-fullvirt-ovmf-override.cfg
>> new file mode 100644
>> index 0000000..46bd684
>> --- /dev/null
>> +++ b/tests/xlconfigdata/test-fullvirt-ovmf-override.cfg
>> @@ -0,0 +1,27 @@
>> +name = "XenGuest2"
>> +uuid = "c7a5fdb2-cdaf-9455-926a-d65c16db1809"
>> +maxmem = 579
>> +memory = 394
>> +vcpus = 1
>> +pae = 1
>> +acpi = 1
>> +apic = 1
>> +viridian = 0
>> +rtc_timeoffset = 0
>> +localtime = 0
>> +on_poweroff = "destroy"
>> +on_reboot = "restart"
>> +on_crash = "restart"
>> +device_model = "/usr/lib/xen/bin/qemu-dm"
>> +sdl = 0
>> +vnc = 1
>> +vncunused = 1
>> +vnclisten = "127.0.0.1"
>> +vif = [
"mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000" ]
>> +parallel = "none"
>> +serial = "none"
>> +builder = "hvm"
>> +bios = "ovmf"
>> +bios_override = "/usr/share/qemu/ovmf-x86_64.bin"
>> +boot = "d"
>> +disk = [
"format=raw,vdev=hda,access=rw,backendtype=phy,target=/dev/HostVG/XenGuest2",
"format=qcow2,vdev=hdb,access=rw,backendtype=qdisk,target=/var/lib/libvirt/images/XenGuest2-home",
"format=raw,vdev=hdc,access=ro,backendtype=qdisk,devtype=cdrom,target=/root/boot.iso"
]
>> diff --git a/tests/xlconfigdata/test-fullvirt-ovmf-override.xml
b/tests/xlconfigdata/test-fullvirt-ovmf-override.xml
>> new file mode 100644
>> index 0000000..435a791
>> --- /dev/null
>> +++ b/tests/xlconfigdata/test-fullvirt-ovmf-override.xml
>> @@ -0,0 +1,58 @@
>> +<domain type='xen'>
>> + <name>XenGuest2</name>
>> + <uuid>c7a5fdb2-cdaf-9455-926a-d65c16db1809</uuid>
>> + <memory unit='KiB'>592896</memory>
>> + <currentMemory unit='KiB'>403456</currentMemory>
>> + <vcpu placement='static'>1</vcpu>
>> + <os>
>> + <type arch='x86_64' machine='xenfv'>hvm</type>
>> + <loader readonly='yes'
type='pflash'>/usr/share/qemu/ovmf-x86_64.bin</loader>
>> + <boot dev='cdrom'/>
>> + </os>
>> + <features>
>> + <acpi/>
>> + <apic/>
>> + <pae/>
>> + </features>
>> + <clock offset='variable' adjustment='0'
basis='utc'/>
>> + <on_poweroff>destroy</on_poweroff>
>> + <on_reboot>restart</on_reboot>
>> + <on_crash>restart</on_crash>
>> + <devices>
>> + <emulator>/usr/lib/xen/bin/qemu-dm</emulator>
>> + <disk type='block' device='disk'>
>> + <driver name='phy' type='raw'/>
>> + <source dev='/dev/HostVG/XenGuest2'/>
>> + <target dev='hda' bus='ide'/>
>> + <address type='drive' controller='0' bus='0'
target='0' unit='0'/>
>> + </disk>
>> + <disk type='file' device='disk'>
>> + <driver name='qemu' type='qcow2'/>
>> + <source file='/var/lib/libvirt/images/XenGuest2-home'/>
>> + <target dev='hdb' bus='ide'/>
>> + <address type='drive' controller='0' bus='0'
target='0' unit='1'/>
>> + </disk>
>> + <disk type='file' device='cdrom'>
>> + <driver name='qemu' type='raw'/>
>> + <source file='/root/boot.iso'/>
>> + <target dev='hdc' bus='ide'/>
>> + <readonly/>
>> + <address type='drive' controller='0' bus='1'
target='0' unit='0'/>
>> + </disk>
>> + <controller type='ide' index='0'/>
>> + <interface type='bridge'>
>> + <mac address='00:16:3e:66:92:9c'/>
>> + <source bridge='xenbr1'/>
>> + <script path='vif-bridge'/>
>> + <model type='e1000'/>
>> + </interface>
>> + <input type='mouse' bus='ps2'/>
>> + <input type='keyboard' bus='ps2'/>
>> + <graphics type='vnc' port='-1' autoport='yes'
listen='127.0.0.1'>
>> + <listen type='address' address='127.0.0.1'/>
>> + </graphics>
>> + <video>
>> + <model type='cirrus' vram='8192' heads='1'
primary='yes'/>
>> + </video>
>> + </devices>
>> +</domain>
>> diff --git a/tests/xlconfigdata/test-fullvirt-ovmf.cfg
b/tests/xlconfigdata/test-fullvirt-ovmf.cfg
>> new file mode 100644
>> index 0000000..af0735e
>> --- /dev/null
>> +++ b/tests/xlconfigdata/test-fullvirt-ovmf.cfg
>> @@ -0,0 +1,26 @@
>> +name = "XenGuest2"
>> +uuid = "c7a5fdb2-cdaf-9455-926a-d65c16db1809"
>> +maxmem = 579
>> +memory = 394
>> +vcpus = 1
>> +pae = 1
>> +acpi = 1
>> +apic = 1
>> +viridian = 0
>> +rtc_timeoffset = 0
>> +localtime = 0
>> +on_poweroff = "destroy"
>> +on_reboot = "restart"
>> +on_crash = "restart"
>> +device_model = "/usr/lib/xen/bin/qemu-dm"
>> +sdl = 0
>> +vnc = 1
>> +vncunused = 1
>> +vnclisten = "127.0.0.1"
>> +vif = [
"mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000" ]
>> +parallel = "none"
>> +serial = "none"
>> +builder = "hvm"
>> +bios = "ovmf"
>
> I have got:
>
> bios_override = "/usr/lib/xen/boot/ovmf.bin"
Actually, I need to remove handling of bios_override since that config item is
part of Anthony's "Load BIOS via toolstack instead of been embedded in
hvmloader" series that is not yet committed to xen.git. Specifically, it is
introduced in 5/14 here
http://lists.xen.org/archives/html/xen-devel/2016-03/msg01628.html
>
> here. And thus the test fails for me. I'm not sure how test what you're
> trying to test here though.
You bring up a point here that I'm not quite sure how to handle.
LIBXL_FIRMWARE_DIR can certainly be different between Xen installations,
depending on configuration options selected when building Xen. But currently all
the various data files under tests/ are "hard-coded". E.g.
tests/xlconfigdata/test-fullvirt-ovmf.xml introduced in this patch contains
<loader readonly='yes'
type='pflash'>/usr/lib/xen/boot/ovmf.bin</loader>
but it should really contain
<loader readonly='yes'
type='pflash'>LIBXL_FIRMWARE_DIR/ovmf.bin</loader>
Yes. this is what I had in mind, thank you for writing it down.
How should test data files which depend on values discovered at build time be
handled?
Good question. I don't think we have such case in our test suite yet. So
it's up to us to walk the path for others :)
Should they be created at build time from e.g.
tests/xlconfigdata/test-fullvirt-ovmf.xml.in?
Not a bad idea. This will definitely work thus it can be our fall back
if other approaches fail.
Is there a better trick for
substituting values discovered at build time in test data files?
Well, I'm thinking of fixing the path in the testsuite. In xlconfigtest
depending on mode either testCompareParseXML or testCompareFormatXML is
called. For the simplicity of explanation assume just the former one,
the latter would be similar.
In the testCompareParseXML, the domain XML is parsed and xl config is
then produced from it. What if we introduced a boolean that if set would
mangle^Wfix the boot loader path just before we try to produce xl config
from it? Then, testCompareHelper would merely just pass the boolean from
testInfo struct. Thus we would need new macro, say DO_TEST_FIX_LOADER (I
know, terrible name) which would set the boolean in the struct. All
other macros would then leave the boolean unset (=false).
I know this is not a terrific idea, because if one would open the XML,
the corresponding config would not be its exact copy.
Or even better, in the XML there would be
<loader>LIBXL_FIRMWARE_DIR/ovmf.bin</loader> and if the boolean is set,
it would just replace LIBXL_FIRMWARE_DIR with the actual value of the
macro (virStringReplace).
This way it would be obvious even in the XML that some post processing
is happening in the test.
Michal