On 05/14/13 18:01, Jordan Justen wrote: > On Tue, May 14, 2013 at 3:10 AM, Laszlo Ersek <[email protected]> wrote: >> On 04/29/13 03:14, Laszlo Ersek wrote: >>> On 04/28/13 02:28, Jordan Justen wrote: >>>> On Tue, Apr 23, 2013 at 3:18 AM, Laszlo Ersek <[email protected]> wrote: >>>>> @@ -199,13 +198,9 @@ VirtioPrepare ( >>>>> >>>>> In *Indices: >>>>> >>>>> - @param [in] HeadIdx The index identifying the head buffer >>>>> (first >>>>> - buffer appended) belonging to this same >>>>> - request. >>>>> - >>>>> - @param [in out] NextAvailIdx On input, the index identifying the next >>>>> - descriptor available to carry the >>>>> buffer. On >>>>> - output, incremented by one, modulo 2^16. >>>>> + @param [in out] NextDescIdx On input, the index identifying the next >>>>> + descriptor to carry the buffer. On output, >>>>> + incremented by one, modulo 2^16. >>>> >>>> Can you use @param just with the actual parameters? >>> >>> Should I just move the "@param [in out]" string from NextDescIdx to >>> Indices, or should I also drop NextDescIdx completely? > > Use @param[in], @param[out], and @param[in,out], and only for > parameters listed in the C code. For a parameter that is a structure, > you can document fields within the area for that parameter. At least, > this seems to be what our coding style wants.
I'll post a patch for this. >>> Also, may I follow up with a separate patch for this? To me it would >>> seem a bit more logical to keep these changes separate, and (for >>> some reason) it also feels safer. >>> >>>> Do you think an addition to OvmfPkg/README for Virtio usage would >>>> be useful? >>> >>> I don't know. People will (or will not) be aware how to configure >>> virtio devices in qemu, independently of OVMF. What do you have in >>> mind? > > I guess I what I had in mind was a quick one liner that I could use to > test it. I trust that you test things, but I always feel better > committing something if I can at least minimally test it. I always test my patches. I agree that it's best practice for you as a maintainer to test stuff before committing. Unfortunately, I personally find the qemu command line an abomination (*) and use libvirt exclusively. I do need functionality from qemu that my RHEL-6.4 libvirt version doesn't support -- for this reason I have a qemu wrapper script (executed by libvirt) that munges command line options. (*) I don't "blame" qemu for its command line, it's the result of organic growth. Nonetheless, right now qemu has such a huge set of command line options (for common usage, nothing exceptional) that it would really need a config file. Libvirt gives me that. > I found our previous discussion about this, and managed to construct > this command line, which is able to at least start booting an ISO > image: > qemu-system-x86_64 -L path/to/ovmf \ > -device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x5 \ > -drive > file=path/to/cd.iso,if=none,media=cdrom,id=drive-scsi0-0-0-0,readonly=on,format=raw > \ > -device > scsi-disk,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0,bootindex=1 > > Is all this really required? Can it be simplified any? The way I create a brand new OVMF guest is described in <http://www.linux-kvm.org/page/OVMF#Making_guests_use_it>. Basically I create a guest XML with virt-install, customize it before first run, then start it from virt-manager. (Of course my qemu wrapper script sits in the middle still.) During this first run of the VM I install the guest OS from the ISO image. Back to your question, this is all really required *for virtio-scsi*: - You need "-device virtio-scsi-pci" for the virtio-scsi controller (the HBA) -- although maybe not the "bus=pci.0,addr=0x5" sub-options, - you need the "-drive file=..." option for the host-side configuration of the guest, - you need "-device scsi-disk" for the guest-side appearance of the disk. "bus=scsi0.0" is important because that connects the disk to the virtio-scsi HBA (which has "id=scsi0"). "drive=drive-scsi0-0-0-0" is needed to connect the "scsi-disk frontend" to the "drive backend" (which has "id=drive-scsi0-0-0-0"). The linkage goes like: -device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x5 ^ | -device scsi-disk,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0,bootindex=1 | v -drive file=path/to/cd.iso,if=none,media=cdrom,id=drive-scsi0-0-0-0,readonly=on,format=raw You could *maybe* omit: - bus & addr -- from "-device virtio-scsi-pci", - channel, scsi-id, lun, id -- from "-device scsi-disk", - most likely nothing from the -drive option. For virtio-blk the following could suffice (no extra controller needed): -device virtio-blk-pci,scsi=off,drive=drive-virtio-disk1 | v -drive file=/var/lib/libvirt/images/Edk2-AppPkg.img,if=none,id=drive-virtio-disk1,format=qcow2,cache=none (Virtio-blk is no good for CD-ROMs, only hard disks. Anyhow the patch in this thread affects both VirtioBlkDxe and VirtioScsiDxe.) > By the way, do you know any live-CD's that support fully booting > virtio via uefi? Yes, Fedora 18 and Fedora 19 LiveCD's should support booting from virtio-scsi CD-ROMs. For example I reported <https://bugzilla.redhat.com/show_bug.cgi?id=864012> and it has been fixed; I tested it. > Another thing I'd like to see documented in the README is redirecting > the default debug messages to a log file. I can send a patch for this too. AFAICS I'll have to modify the "RUNNING OVMF on QEMU" section. For reference, the current version of my wrapper script (specified in the /domain/devices/emulator element of the libvirt XML) is: #!/bin/bash set -e -C -u NEW_ARGS=() append() { for I in "$@"; do NEW_ARGS[${#NEW_ARGS[@]}]=$I done } NAME= LAST= for ARG in "$@"; do if [ _"$LAST" = _-device ] && [[ _"$ARG" = _e1000* ]]; then append "$ARG,romfile=/home/virt-images/efi-roms/efi-e1000.rom" elif [ _"$LAST" = _-device ] && [[ _"$ARG" = _ne2k_pci* ]]; then append "$ARG,romfile=/home/virt-images/efi-roms/efi-ne2k_pci.rom" elif [ _"$LAST" = _-device ] && [[ _"$ARG" = _pcnet* ]]; then append "$ARG,romfile=/home/virt-images/efi-roms/efi-pcnet.rom" elif [ _"$LAST" = _-device ] && [[ _"$ARG" = _rtl8139* ]]; then append "$ARG,romfile=/home/virt-images/efi-roms/efi-rtl8139.rom" elif [ _"$LAST" = _-device ] && [[ _"$ARG" = _virtio-net-pci* ]]; then append "$ARG,romfile=/home/virt-images/efi-roms/efi-virtio.rom" else append "$ARG" fi if [ _"$LAST" = _-name ]; then NAME=$ARG fi LAST=$ARG done if [ -n "$NAME" ]; then append -debugcon file:/tmp/"$NAME".debug -global isa-debugcon.iobase=0x402 \ -global PIIX4_PM.disable_s3=0 -global PIIX4_PM.disable_s4=0 fi exec /usr/libexec/qemu-kvm "${NEW_ARGS[@]}" (An earlier version of the script is visible under <http://www.linux-kvm.org/page/OVMF#Saving_OVMF_debug_messages>.) The script - adds the ",romfile=..." stuff for the iPXE UEFI NIC drivers, - distinguishes invocations with -name from invocations without -name (libvirt invokes the emulator without -name right after virsh edit for verification purposes), - if -name was specified (ie. starting the emulator for real), it captures debug messages in a file, and ensures S3/S4 support (some qemu versions disable it by default). >> Ping... this fix is important. Please tell me in direct terms what I >> must do; I'd like to see this committed. > > I committed it. I'd still like to see the parameter comments changed, > but I wanted to (at least minimally) test the change. Thank you very much, patches for the comment and the README (debug messages) coming up. Laszlo ------------------------------------------------------------------------------ AlienVault Unified Security Management (USM) platform delivers complete security visibility with the essential security capabilities. Easily and efficiently configure, manage, and operate all of your security controls from a single console and one unified framework. Download a free trial. http://p.sf.net/sfu/alienvault_d2d _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
