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.. 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...
Osier