Bug 1425058

A couple of super-quick questions:

  * Is there a problem with having

      romfile=

    as opposed to

      romfile='' or romfile=""

    on the QEMU command line, or are the two notations
    completely equivalent?

  * libvirt currently supports the <rom/> element (and
    consequently a way for the user to pick the values
    of the 'rombar' and 'romfile' properties) only for
    network interfaces, either emulated (<interface>
    element) or assigned from the host (<hostdev>
    element). Is the empty string a valid value for
    the 'romfile' property in both cases?

Thanks :)

Essentially QEMU has overloaded 2 separate properties (ROM on/off and file path) onto a single property. We shouldn't follow that in libvirt as this is not portable semantics wrt other hypervisors. We should have an explicit attribute to enable/disable ROMs and not rely on a magic filepath.

>   * Is there a problem with having
> 
>       romfile=

That is the correct one.

>       romfile='' or romfile=""

That'll only work of you run qemu from the shell.
The "" and '' are interpreted by the shell and qemu will not set them.

(In reply to Daniel Berrange from comment #2)
> Essentially QEMU has overloaded 2 separate properties (ROM on/off and file
> path) onto a single property. We shouldn't follow that in libvirt as this is
> not portable semantics wrt other hypervisors. We should have an explicit
> attribute to enable/disable ROMs and not rely on a magic filepath.

So you would propose leaving the current requirements on
the value of 'file' alone, and add something like

  <rom enable='no'/>

to disable ROM loading entirely, which would then be
mapped to

  romfile=

on the QEMU command line? Sound sensible to me.

Note that

  <rom file=''/>

is already handled by libvirt, eg. the schema will forbid
it, but you can ignore the XML validation error and keep it
around. Should we make an attempt to convert such guests to
the new property, or is it just not worth the effort?

If we changed existing guests, that might cause migration fun for old libvirt,

(In reply to Daniel Berrange from comment #5)
> If we changed existing guests, that might cause migration fun for old
> libvirt,

Yeah, that was my guess too :)

(In reply to Daniel Berrange from comment #2)
> Essentially QEMU has overloaded 2 separate properties (ROM on/off and file
> path) onto a single property.

QEMU distinguishes the following cases:

(1) romfile=<empty_string>, with rombar=[01] irrelevant -- this disables PCI
    oprom loading for the device entirely
(2) romfile=<pathname>, rombar=1 -- the PCI oprom is loaded into the PCI
    device's ROM BAR
(3) romfile=<pathname>, rombar=0 -- the PCI oprom is loaded into a dedicated
    fw_cfg file called "genroms/%s", where "%s" is replaced by the basename
    of the host filesystem file that the oprom is loaded from.

For a long time, the domain XML schema had accepted <rom file=''/>, mapping it to case (1). At some point it started rejecting <rom file=''/>.

For UEFI guests, this was no problem, because for them, (1) and (3) behave identically (UEFI ignores "genroms/*" in fw_cfg). So it wasn't hard to flip these guests from <rom file=''/> to <rom bar='off'/>; any nonempty QEMU default for romfile=... would be ignored by the UEFI guest anyway, because the PCI ROM BAR would not be present.

However, for SeaBIOS guests, (1) and (3) are different, thus some way to re-expose (1) in the domain XML would be good.

Also, under (3), on the QEMU level, if the default <pathname> cannot be resolved on the filesystem (for example because the default option ROMs are not installed), QEMU will whine on stderr (although it will start). In this aspect, (1) and (3) are not equivalent even for UEFI guests.

> We shouldn't follow that in libvirt as this is
> not portable semantics wrt other hypervisors. We should have an explicit
> attribute to enable/disable ROMs and not rely on a magic filepath.

If that's feasible, it's likely the best solution.

(In reply to Andrea Bolognani from comment #1)
> A couple of super-quick questions:
> 
>   * Is there a problem with having
> 
>       romfile=
> 
>     as opposed to
> 
>       romfile='' or romfile=""
> 
>     on the QEMU command line, or are the two notations
>     completely equivalent?


Gerd answered this in comment 3.

>   * libvirt currently supports the <rom/> element (and
>     consequently a way for the user to pick the values
>     of the 'rombar' and 'romfile' properties) only for
>     network interfaces, either emulated (<interface>
>     element) or assigned from the host (<hostdev>
>     element). Is the empty string a valid value for
>     the 'romfile' property in both cases?

romfile=<empty_string> is suitable for all PCI devices. The property is defined in QEMU's "hw/pci/pci.c", for TYPE_PCI_DEVICE ("pci-device"). All classes derived from TYPE_PCI_DEVICE inherit the property, and the property is handled in common code (pci_add_option_rom() [hw/pci/pci.c]).

Additional info:

Currently it's not possible to use virsh attach-interface on rombar-less installations. QEMU errors out with, e.g.

 error: Failed to attach interface
 error: internal error: unable to execute QEMU command 'device_add': failed to find romfile "pxe-virtio.rom"

Attempting virsh attach-device instead with the following xml

 <interface type='bridge'>
   <source bridge='br0'/>
   <model type='virtio'/>
   <rom bar='off'/>
 </interface>

fails with

 error: Failed to attach device from interface.xml
 error: internal error: unable to execute QEMU command 'device_add': Hot-plugged device without ROM bar can't have an option ROM

Changing the xml to

 <interface type='bridge'>
   <source bridge='br0'/>
   <model type='virtio'/>
   <rom file=''/>
 </interface>

works.

Cross-filed ticket 01880358 on the Red Hat customer portal.

Not sure how much relevance this exactly has here, but virt-manager is
currently also not able to handle disabling ROM loading for a NIC.

(In reply to Robert Scheck from comment #17)
> Not sure how much relevance this exactly has here, but virt-manager is
> currently also not able to handle disabling ROM loading for a NIC.

The virt-manager part would need a separate bug report.

It's unclear to me whether such a feature would be
appropriate for virt-manager though, as it's an exceedingly
narrow use case and, much like this very bug, mostly
irrelevant now that Bug 1337510 has been addressed.

I think I may have found another (general) issue with the <rom bar='off'/> workaround. It affects UEFI guests primarily. From comment #7:

> (3) romfile=<pathname>, rombar=0 -- the PCI oprom is loaded into a dedicated
>     fw_cfg file called "genroms/%s", where "%s" is replaced by the basename
>     of the host filesystem file that the oprom is loaded from.
>
> [...]
>
> [...] under (3), on the QEMU level, if the default <pathname> cannot be
> resolved on the filesystem (for example because the default option ROMs are
> not installed), QEMU will whine on stderr (although it will start).

It gets worse than whining if:
- we use multiple instances of a device model that comes with a default
  romfile pathname,
- and the romfile *is* installed on the system,
- and we try to use rombar=0 (possibly via libvirt's <rom bar='off'/>) to
  keep the romfile out of the PCI ROM BAR.

Namely, we'll see a "genroms/%s" conflict, and QEMU will refuse to start:

qemu-system-x86_64 \
  -device virtio-net-pci,id=vnet1,rombar=0 \
  -device virtio-net-pci,id=vnet2,rombar=0

qemu-system-x86_64: -device virtio-net-pci,id=vnet2,rombar=0: duplicate fw_cfg file name: genroms/efi-virtio.rom

This is quite easy to trigger with a VM that is hooked into two subnets (with two separate virtio NICs):

    <interface type='network'>
      <source network='net-1'/>
      <model type='virtio'/>
      <rom bar='off'/>
    </interface>
    <interface type='network'>
      <source network='net-2'/>
      <model type='virtio'/>
      <rom bar='off'/>
    </interface>

I doubt, but adding Michal.

nope, RHV doesn't need that

Patches posted upstream.

  https://www.redhat.com/archives/libvir-list/2018-April/msg02077.html

Merged upstream.

commit 4d11d9a2922bf3272a697cffe396d05cae373e12
Author: Andrea Bolognani <abologna>
Date:   Fri Apr 20 17:17:11 2018 +0200

    qemu: Format rom.enabled attribute for PCI devices
    
    The attribute can be used to disable ROM loading completely
    for a device.
    
    This might be needed because, even when the guest is configured
    such that the PCI ROM will not be loaded in the PCI BAR, some
    hypervisors (eg. QEMU) might still make it available to the
    guest in a form (eg. fw_cfg) that some firmwares (eg. SeaBIOS)
    will consume, thus not achieving the desired result.
    
    Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1425058
    
    Signed-off-by: Andrea Bolognani <abologna>

v4.2.0-451-g4d11d9a292

Test on libvirt-4.3.0-1.el7.x86_64:
1. It fails to disable the pxe boot when try uefi pxe boot with virtio(see below details), please help to check;

2. The issues in comment 9 and comment 19 are not fixed.
comment 9: hotplug interface with <rom bar='off'/> fail
comment 19: vm with 2 interface both has <rom bar='off'/> fails to start(vm with seabios)

3. If the comment 0 is a issue and need to be fixed that <rom file=''> fail to validate but can be ignored?
# virsh edit q  
====> add <rom file=''/> into interface xml

error: XML document failed to validate against schema: Unable to validate doc against /usr/share/libvirt/schemas/domain.rng
Extra element devices in interleave
Element domain failed to validate content

Failed. Try again? [y,n,i,f,?]: ===>  press 'i'
Domain q XML configuration edited.



#####################  Details for 1 #######################
1). Prepare a pxe server for UEFI on one host, and prepare a Q35 vm on anther host, connect the 2 host in P-2-P connection. 
# virsh dumpxml q 
...
  <os>
    <type arch='x86_64' machine='pc-q35-rhel7.5.0'>hvm</type>
    <loader readonly='yes' secure='yes' type='pflash'>/usr/share/OVMF/OVMF_CODE.secboot.fd</loader>
    <nvram>/var/lib/libvirt/qemu/nvram/q_VARS.fd</nvram>
  </os>
...
<interface type='direct'>
      <mac address='52:54:00:58:b8:7a'/>
      <source dev='eno1' mode='bridge'/>
      <model type='virtio'/>
      <boot order='1'/>
      <rom enabled='no'/>
      <address type='pci' domain='0x0000' bus='0x02' slot='0x00' function='0x0'/>
    </interface>
...

2). Start the vm, and check the vm can pxe boot by network interface.


Other scenarios & configs tested as below, and it works as expected.
1. direct type + legacy bios + virtio/rtl8139/e1000/e1000e:
2. direct type + OVMF + rtl8139/e1000/e1000e:

config 1: <rom enabled='no'/>  ---> PASS, can not pxe boot
config 2: <rom enabled='yes'/> ---> PASS, can pxe boot
config 3: no rom setting  ---> PASS, can pxe boot

3. vf passthrough(hostdev interface) + legacy bios
4. vf passthrough(hostdev device) + legacy bios
5. pf passthrough(hostdev device) + legacy bios

config 1: <rom enabled='no'/>  ---> PASS, can not pxe boot
config 2: <rom enabled='yes' file='/usr/share/ipxe/808610ca.rom'/> ---> PASS, can pxe boot
config 3: <rom file='/usr/share/ipxe/808610ca.rom'/>  ---> PASS, can pxe boot
config 4: set "rom enabled='no'" together with "rom bar" or "rom file" will report error:
# virsh edit rhel
...
 <rom enabled='no' bar='off'/>

error: unsupported configuration: ROM tuning is not supported when ROM is disabled
Failed. Try again? [y,n,i,f,?]: 

It makes sense.

Have not test vf/pf passthrouth with UEFI because the oprom you used had no UEFI image(refer bug 1278421 #21)

(In reply to yalzhang from comment #36)
> Test on libvirt-4.3.0-1.el7.x86_64:
> 1. It fails to disable the pxe boot when try uefi pxe boot with virtio(see
> below details), please help to check;

I'm not sure I quite get what you're trying to say: the report below
seems to mark all cases as PASS. In any case, I'll leave looking
further into this up to Laszlo and others, since AFAIU libvirt has
generated the expected command line (let me know if that is not
actually the case) but the guest is still not behaving as intended.

> 2. The issues in comment 9 and comment 19 are not fixed.
> comment 9: hotplug interface with <rom bar='off'/> fail

That's weird, it should not happen with Bug 1337510 having been
addressed.

But I see you're testing on x86_64 instead of aarch64, so that
could be muddying the water somewhat. Can you please report the
actual error message in this case?

Either way, you should also try using <rom enabled='no'/>
instead of <rom bar='off'/> and see whether it still fails.

> comment 19: vm with 2 interface both has <rom bar='off'/> fails to start(vm
> with seabios)

That's expected: nothing's changed in the handling of
<rom bar='off'/> so you're still hitting the clash Laszlo described
in Comment 19.

Using <rom enabled='no'/> instead of <rom bar='off'/> should allow
the guest to start.

> 3. If the comment 0 is a issue and need to be fixed that <rom file=''> fail
> to validate but can be ignored?
> # virsh edit q  
> ====> add <rom file=''/> into interface xml
> 
> error: XML document failed to validate against schema: Unable to validate
> doc against /usr/share/libvirt/schemas/domain.rng
> Extra element devices in interleave
> Element domain failed to validate content
> 
> Failed. Try again? [y,n,i,f,?]: ===>  press 'i'
> Domain q XML configuration edited.

It was agreed that such an XML should indeed generate a validation
error, and that's exactly why a new attribute was introduced. So
libvirt is working as expected here.

(In reply to Andrea Bolognani from comment #37)
> (In reply to yalzhang from comment #36)
> > Test on libvirt-4.3.0-1.el7.x86_64:
> > 1. It fails to disable the pxe boot when try uefi pxe boot with virtio(see
> > below details), please help to check;
> 
> I'm not sure I quite get what you're trying to say: the report below
> seems to mark all cases as PASS. In any case, I'll leave looking
> further into this up to Laszlo and others, since AFAIU libvirt has
> generated the expected command line (let me know if that is not
> actually the case) but the guest is still not behaving as intended.

This is expected (in fact, intended) behavior with virtio-net NICs -- OVMF and AAVMF contain a built-in virtio-net driver called "VirtioNetDxe".

The UEFI driver for virtio-net that is normally exposed to the guest in the NIC's option ROM BAR comes from the iPXE project, and normally it takes priority over the built-in VirtioNetDxe driver. If you prevent the loading of the iPXE oprom (either by disabling the ROM BAR, or by setting the ROM file pathname to the empty string, or by having arch/board-specific QEMU quirks such as bug 1337510), then the firmware falls back to VirtioNetDxe. This is 100% intended behavior.

Scenario 1: live update "rom enabled" setting ---> FAIL 
(report succeed but no change, should report error like "error: Operation not supported: cannot modify network device rom enabled setting")

Scenario 2: hotplug/unplug with "rom enabled" setting ---> PASS

Details:
Scenario 1: live update
# virsh dumpxml rhel | grep /interface -B9
    <interface type='direct'>
      <mac address='52:54:00:78:36:26'/>
      <source dev='eno1' mode='bridge'/>
      <target dev='macvtap0'/>
      <model type='rtl8139'/>
      <boot order='1'/>
      <alias name='net0'/>
      <rom enabled='no'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0'/>
    </interface>

# cat direct.xml
<interface type='direct'>
      <mac address='52:54:00:78:36:26'/>
      <source dev='eno1' mode='bridge'/>
      <target dev='macvtap0'/>
      <model type='rtl8139'/>
      <boot order='1'/>
      <alias name='net0'/>
  *** <rom enabled='yes'/> ***
      <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0'/>
    </interface>

# virsh update-device rhel direct.xml
Device updated successfully

# virsh dumpxml rhel | grep /interface -B9
    <interface type='direct'>
      <mac address='52:54:00:78:36:26'/>
      <source dev='eno1' mode='bridge'/>
      <target dev='macvtap0'/>
      <model type='rtl8139'/>
      <boot order='1'/>
      <alias name='net0'/>
      <rom enabled='no'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0'/>
    </interface>

When live update file path:
# virsh update-device rhel direct_file.xml
error: Failed to update device from direct.xml
error: Operation not supported: cannot modify network rom file

# virsh update-device rhel direct_bar.xml
error: Failed to update device from direct.xml
error: Operation not supported: cannot modify network device rom bar setting

Hi Andrea, Thank you for your feedback, please check comment 40.

For the issue in comment 9, I have not tried on aarch64, only on x86_64. And the bug 1337510 is for qemu-kvm-rhev. So I would not report the error on that qemu case. I found it happens only for qemu emulated nics, not for hostdev, see below:
# rpm -q libvirt qemu-kvm-rhev
libvirt-4.3.0-1.el7.x86_64
qemu-kvm-rhev-2.12.0-3.el7.x86_64

1. Start vm with <rom bar='off'/>, succeed:
# virsh dumpxml rhel |grep /interface -B6
    <interface type='network'>
      <mac address='52:54:00:e7:11:12'/>
      <source network='default'/>
      <model type='rtl8139'/>
      <rom bar='off'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x0c' function='0x0'/>
    </interface>

# virsh start rhel
Domain rhel started

2. Hotplug interface with <rom bar='off'/> with emulated model types, failed:
# cat interface.xml
<interface type='network'>
      <source network='default'/>
      <rom bar='off'/>
<model type='rtl8139/virtio/e1000/e1000e'/> *** each type both failed
</interface>

# virsh attach-device rhel interface.xml
error: Failed to attach device from interface.xml
error: internal error: unable to execute QEMU command 'device_add': Hot-plugged device without ROM bar can't have an option ROM

3. hotplug hostdev interface with <rom bar='off'/>, succeed:
# cat  hostdev.xml
<interface type='hostdev' managed='yes'>
  <source>
    <address type='pci' domain='0x0000' bus='0x04' slot='0x10' function='0x0'/>
  </source>
<rom bar='off'/>
</interface>

# virsh attach-device rhel hostdev.xml
Device attached successfully

Do you think it should be fixed?

And also for comment 19, maybe we need open another bug for these 2 as they are not related with the customer request in this case.

(1) Updating the rom/@enable attribute for a running domain:

    According to the virsh documentation, the "update-device" sub-command
    updates the current domain state, if no flags are given. (I.e., modify
    runtime state only if the domain is running, and modify the persistent
    configuration if the domain is shut down.)

    Similarly, the "dumpxml" sub-command, when invoked for a running domain
    without "--inactive", seems to be documented to dump the runtime state.

    So the issue report seems valid to me, either "update-device" should
    fail on a running domain for rom/@enable (like it apparently does for
    rom/@bar and rom/@file), or else its success should be reflected by
    dumpxml.

    (I hope my comments are not unwelcome, this is partly to educate
    myself.)

(2) Regarding comment 9, I don't think "attach-interface" should be tested
    with <rom bar='off'/> *at all*.

    We've established that <rom bar='off'/> is not what we're after; that's
    why <rom enable='...'/> has been introduced. In other words,
    "attach-interface" should be tested with <rom enable='no'/> instead.

    The difference at the QEMU level is that <rom bar='off'/> sets the
    "rombar" property to false, but it does not touch the "romfile"
    property, which is a non-empty pathname for (a) virtual devices on (b)
    the x86_64 target. QEMU is right to complain about this configuration.
    The fix is to ignore the "rombar" property completely and to set the
    "romfile" property to the empty string. Which is exactly what <rom
    enable='no'/> does.

    (a) QEMU doesn't complain about hostdev hotplug with <rom bar='off'/> on
        x86_64 because the "romfile" property is by default "" for hostdev.

    (b) QEMU doesn't complain about virtio-net hotplug with <rom bar='off'/>
        on aarch64 because we changed the "romfile" default to "" on
        aarch64, in bug 1337510.

(3) Regarding comment 19, no new BZ is necessary. That case is also solved
    by <rom enable='no'/>.

    That's because, with "romfile" set to "", no ROM file is loaded into
    "genroms/%s"-style fw_cfg files either, so they cannot conflict between
    each other.

Summary:

- IMO, "update-device" should indeed either reject modifying rom/@enable for
  a running domain, or if it reports success, the result should be reflected
  by a subsequent "dumpxml" (for the active domain).

- The "attach-interface" tests with <rom bar='off'/> are invalid and out of
  scope, they should be repeated with <rom enable='no'/> instead.

- <rom enable='no'/> can be specified for multiple <interface> elements at
  the same time; it should be easy to test. Again, <rom bar='...'/> is
  simply irrelevant here.

Thank Laszlo for the detailed and clear feedback on comment 42.

So the '(1) Updating the rom/@enable attribute for a running domain' need to be fixed here, details see comment 40

(In reply to yalzhang from comment #43)
> Thank Laszlo for the detailed and clear feedback on comment 42.
> 
> So the '(1) Updating the rom/@enable attribute for a running domain' need to
> be fixed here, details see comment 40

Agreed, we should give better feedback if the user is trying to update
the device and we are unable to comply with the request.

Can you please file a separate bug for that?

(In reply to Andrea Bolognani from comment #44)
> (In reply to yalzhang from comment #43)
> > Thank Laszlo for the detailed and clear feedback on comment 42.
> > 
> > So the '(1) Updating the rom/@enable attribute for a running domain' need to
> > be fixed here, details see comment 40
> 
> Agreed, we should give better feedback if the user is trying to update
> the device and we are unable to comply with the request.
> 
> Can you please file a separate bug for that?

Thank you Andrea, file one Bug 1599513, and move this bug to be verified.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHSA-2018:3113