On 18/04/13 19:59, Laine Stump wrote:
On 04/18/2013 07:27 AM, Osier Yang wrote:
On 18/04/13 19:16, Laine Stump wrote:
On 04/18/2013 05:41 AM, Martin Kletzander wrote:
On 04/18/2013 11:05 AM, Osier Yang wrote:
On 18/04/13 17:00, Martin Kletzander wrote:
On 04/18/2013 10:54 AM, Osier Yang wrote:
On 18/04/13 16:42, Martin Kletzander wrote:
On 04/18/2013 06:36 AM, Laine Stump wrote:
The rng schema for <controller> had been non-specific about
which
types of controllers allowed which models, and also allowed the
num_queues attribute (since that hasn't been released yet,
should we
rename it to "numQueues"?)
Since there's still time (the commit with that is
v1.0.4-65-gd4bf0a9), I
think we should rename it ASAP since we are using camelCase for
all the
attribute names.
Apart from that, the RNG with this patch is precise according to
the
documentation, so ACK. I'll try to send the numQueues patch
to see
what
others think.
I guess you mean multiple queues support for virtio network?
Regardless of which style we will use finally, FYI,
"num_queues" is
used for disk.. Personally I'm fine with either, because we
already
use both across.
Yes, I meant the virtio-scsi num_queues. As we're trying not to
use
underscores in XML, I hope we can still switch it. I haven't
found any
other num_queues anywhere in the code. Could you point me to the
commit
that uses that? I'm sending the previously discussed patch in the
meantime.
Except the virtio-scsi num_queues, there is no other tag for
multiple
queue yet, we will need a patch to support multiple queue for the
network,
but it's not committed yet.
It's fine to convert it now, 1.0.5 is not released yet. But is it
deserved to
do, we already have many tags with underscore, which can't be
changed
for back-compat.
I believe those attributes [1] were created by mistake, and kept only
because of backward compatibility. I'm trying to be open-minded,
though, so I'm not forcing my patch in, but seeing it just as a
proposal. Others may have different opinions and I'm willing to
discuss
that. My first feeling, though, was that we should try to keep the
same
policy for as many of them as possible. OTOH, I've mistaken the
underscore with a hyphen when I remembered what Daniel told me about
attributes [2].
I had recalled DV saying something about underscores in the names a
long
time ago, and I recently asked about underscore vs. camelCase, and
danpb
said the same thing. (Personally I don't have a preference one way or
the other, but if we really are trying to avoid them, now is our
chance).
I'm fine with either keeping it or changing num_queues. For long
term consistence, I agreed with having a consistent naming style
is nice.
In the meantime, in other device types, we've tried to keep backend
details like this pushed into a <driver> subelement when possible, to
avoid polluting the main element (e.g. see the <driver> subelement of
<interface>). Is it worth putting this numQueues attribute in a
<driver>
subelement too? Or am I just playing XML God?
Not sure if you mean the upcoming numQueues for interface. But for the
existing num_queues, it's for the virtio-scsi controller, putting it
in <driver>
doesn't reflect the purpose.
But isn't it a backend implementation detail of the specific SCSI
controller? In <interface> and <disk>, information that is specific to a
particular backend (and isn't generally applicable to that type of
device) is in the <driver> subelement.
This is the QEMU command line for a virtio-scsi disk: ("-device
virtio-scsi-pci"
is mapped to virtio-scsi controller in libvirt XML, with num_queues set):
<...>
-device virtio-scsi-pci,id=scsi0,num_queues=8,bus=pci.0,addr=0x3 \
-usb \
-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-scsi0-0-0-0 \
-device
scsi-disk,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0
\
</...>
And this is the QEMU command line for a virtio disk (with event_idx set):
<...>
-drive
file=/var/lib/libvirt/images/f14.img,if=none,id=drive-virtio-disk0 \
-device
virtio-blk-pci,event_idx=on,scsi=off,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0
\
</...>
This is the properties the QEMU device "scsi-disk" supports:
% ./x86_64-softmmu/qemu-system-x86_64 -device scsi-disk,?
scsi-disk.drive=drive
scsi-disk.logical_block_size=blocksize
scsi-disk.physical_block_size=blocksize
scsi-disk.min_io_size=uint16
scsi-disk.opt_io_size=uint32
scsi-disk.bootindex=int32
scsi-disk.discard_granularity=uint32
scsi-disk.ver=string
scsi-disk.serial=string
scsi-disk.vendor=string
scsi-disk.product=string
scsi-disk.removable=on/off
scsi-disk.dpofua=on/off
scsi-disk.wwn=hex64
scsi-disk.channel=uint32
scsi-disk.scsi-id=uint32
scsi-disk.lun=uint32
And the properties "virtio-blk-pci" device supports:
% ./x86_64-softmmu/qemu-system-x86_64 -device virtio-blk-pci,?
virtio-blk-pci.class=hex32
virtio-blk-pci.ioeventfd=on/off
virtio-blk-pci.vectors=uint32
virtio-blk-pci.indirect_desc=on/off
virtio-blk-pci.event_idx=on/off
virtio-blk-pci.drive=drive
virtio-blk-pci.logical_block_size=blocksize
virtio-blk-pci.physical_block_size=blocksize
virtio-blk-pci.min_io_size=uint16
virtio-blk-pci.opt_io_size=uint32
virtio-blk-pci.bootindex=int32
virtio-blk-pci.discard_granularity=uint32
virtio-blk-pci.cyls=uint32
virtio-blk-pci.heads=uint32
virtio-blk-pci.secs=uint32
virtio-blk-pci.serial=string
virtio-blk-pci.config-wce=on/off
virtio-blk-pci.scsi=on/off
virtio-blk-pci.addr=pci-devfn
virtio-blk-pci.romfile=string
virtio-blk-pci.rombar=uint32
virtio-blk-pci.multifunction=on/off
virtio-blk-pci.command_serr_enable=on/off
And the properties "virtio-scsi-pci" device supports:
% ./x86_64-softmmu/qemu-system-x86_64 -device virtio-scsi-pci,?
virtio-scsi-pci.ioeventfd=on/off
virtio-scsi-pci.vectors=uint32
virtio-scsi-pci.indirect_desc=on/off
virtio-scsi-pci.event_idx=on/off
virtio-scsi-pci.hotplug=on/off
virtio-scsi-pci.param_change=on/off
virtio-scsi-pci.num_queues=uint32
virtio-scsi-pci.max_sectors=uint32
virtio-scsi-pci.cmd_per_lun=uint32
virtio-scsi-pci.addr=pci-devfn
virtio-scsi-pci.romfile=string
virtio-scsi-pci.rombar=uint32
virtio-scsi-pci.multifunction=on/off
virtio-scsi-pci.command_serr_enable=on/off
We can put things like "ioeventfd", "event_idx" in the <driver>
subelement, is
because of the QEMU device used for disk supports it. But for a
virtio-scsi disk,
"num_queues" is supported in a separate device "virtio-scsi-pci"
instead.. That
means, from libvirt p.o.v, things like "ioevent_idx" are for disk,
"num_queues"
is for the disk controller.
Assuming that we put "num_queues" or "numQueues" in <driver>, then we
need
to find out the controller for disk when building QEMU command line,
and check
if it's virtio-scsi model, if not, error out, otherwise tell the
function to build the
controller device string that "num_queues" is specified, and what its
value is. Or
something similar but reversely (find out the disk associated with the
virtio-scsi
controller, check if num_queues is specified). This might be not the
exact process,
but it can show putting "num_queues" in <driver> is just a straight
wrong way to go...
