[libvirt] [PATCH v4 1/8] docs: schema: Add basic documentation for the virtual
John Ferlan
jferlan at redhat.com
Thu Aug 3 21:33:18 UTC 2017
On 07/07/2017 04:07 AM, Longpeng(Mike) wrote:
> This patch documents XML elements used for support of virtual
> crypto devices.
>
> In the devices section in the domain XML users may specify:
> <crypto model='virtio'>
> <backend type='builtin' queues='1'/>
Add an example <address... > too.
> </crypto>
> to enable the crypto device for guests.
>
> Signed-off-by: Longpeng(Mike) <longpeng2 at huawei.com>
Is this the "legal name" that would be used for a commit? Generally we
prefer to see a more legal name rather than someone's email name.
There's plenty of examples in git history.
> ---
> docs/formatdomain.html.in | 61 +++++++++++++++++++++++++++++++++++++++++++
> docs/schemas/domaincommon.rng | 30 +++++++++++++++++++++
> 2 files changed, 91 insertions(+)
>
For some reason I'm only seeing this patch from the series come through.
Whether that's something specific to the RH email or in general, I'm not
sure. Similarly for your v3 series, just the first patch came through.
Since they were close together - I have to wonder if the RH email system
was having one it's clogged or senior moments and the patches are still
stuck in some queue somewhere. It's happened before, but usually
everything gets backed up, not just one series from one submittor.
I see from the archive you pinged on 7/25 looking for a review on the
series, but even that didn't come through. It's very strange. Still I
think you need to repost and adjust anyway.
Here's some thoughts looking just at the archives though...
Patches 1 & 3 have a "relationship" insomuch as as you're documenting in
patch 1 before the domain_conf code exists. I think it's best to combine
them.
* For both, will the default of MODEL_VIRTIO and BACKEND_BUILTIN live
for perpetuity? Or is it possible that at some point a "default" or
"unknown" would be required? I ask only since both would be equal to
zero for the enum and VIR_ALLOC means default to zero. So sometimes
adding a "default" or "none" type entry ensures that something does get
set and it's not some default as a result of the allocation algorithm
that takes over.
* When you add the XML parsing code, you should add the xml2xml tests.
That means grabbing qemuxml2xmltest.c and xml from patches 7 & 8 and
moving them into here.
* For new functions, make sure there's 2 blank lines before and after
the function... virDomainCryptoDefFree only has 1 before.
* For the queues parse, use virStrToLong_uip to ensure no negative is
supplied (per the rng below using positiveInteger)
Patch 2 should be the last patch as news is always last.
Patch 4 is going to need some merge conflict resolution. There is also
now some tests/qemucapabilitiesdata/*ppc* replies/xml that exist -
whether that relates here or not I'm not sure, but something that I
think may have been added since you last posted...
Patch 5...
* There's an error message that has "faile" instead of "failed".
* There's a switch for dev->data.crypto->model that uses
VIR_DOMAIN_RNG_MODEL_LAST for a case.
* Should the alias include the "virtio" in some way. Would it ever be
reasonable for a domain to use two different types for different
devices? Maybe virtio is supplied today and becomes legacy and who
knows what is the new sleek thing next year, but both are allowed so you
have to change the alias then.
* You may way to create an accessor that prints the "obj%s" alias since
it's formatted twice. It'll be useful if you support hotplug as well.
* What about hotplug? You either should support or explicitly deny. I'm
kind of surprised you didn't get build warnings because
VIR_DOMAIN_DEVICE_CRYPTO wasn't added to qemu_driver.c and
qemu_hotplug.c since the switch ((virDomainDeviceType) def->type) is there.
* This is when the qemuxml2argvtest should be adjusted.
Patch 6... Put the comma on the AddLit rather than the next
virBufferAsprintf.... although since PPC and CCW are supported from the
start, I'd say add them both at the same time. Although I do understand
and appreciate why they're separate. Still it's not "new" functionality
for CCW support, so just do it all at once.
Patch 7... Tests are usually added at the time the command is adjusted.
This looks merge-able with patches 3 and 5
Patch 8... Looks merge-able with patches 3 and 6
Couple more comments below...
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 36bea67..7c27ae7 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -7547,6 +7547,67 @@ qemu-kvm -net nic,model=? /dev/null
> </dd>
> </dl>
>
> + <h4><a name="elementCrypto">Crypto device</a></h4>
> +
> + <p>
> + The virtual crypto device is a virtual crypto accelerator
> + card(provides crypto services, such as CIPHER, MAC, HASH,
s/card(provides/card (provides)
> + and AEAD) for virtual machines and it can be added to the
> + guest via the <code>crypto</code> element.
> + <span class="since">Since 3.1.0, QEMU and KVM only</span>
It'd be 3.7.0 at the earliest
> + </p>
> +
> + <p>
> + Example: usage of the crypto device:
> + </p>
> +<pre>
> + ...
> + <devices>
> + <crypto model='virtio'>
> + <backend type='builtin' queues='1'/>
> + </crypto>
> + </devices>
> + ...
> +</pre>
> + <dl>
> + <dt><code>model</code></dt>
> + <dd>
> + <p>
> + The required <code>model</code> attribute specifies what
> + type of crypto device is provide.
either "is provided" or "to provide"
> + Currently only 'virtio' is supported and it needs virtio-crypto
> + guest driver.
> + </p>
> + </dd>
> + <dt><code>backend</code></dt>
> + <dd>
> + <p>
> + The <code>backend</code> element specifies the type and
> + number of queues of the crypto device to be used for the
s/of the crypto/for the crypto/
> + domain.
> + </p>
> + <dl>
> + <dt><code>type</code></dt>
> + <dd>
> + <p>
> + The required <code>type</code> element specifies the
> + type of the crypto device.
> + Currently only supports 'builtin' which uses QEMU's
> + crypto APIs to complete the crypto operations.
> + </p>
> + </dd>
> + <dt><code>queues</code></dt>
> + <dd>
> + <p>
> + The optional <code>queues</code> element specifies the
> + number of queues of the crypto device, the default number
> + of queues is 1.
Again for the crypto device reads better to me, but it's a bit redundant
with the first sentence.
This makes me wonder what happens if someone uses 100 or 1000 or ...
queues. Is there some maximum (I didn't check the qemu code). Beyond
that what use does increasing the number of queues have?
> + </p>
> + </dd>
> + </dl>
> + </dd>
> + </dl>
> +
There's also an <address> that is required to be "pci" or "ccw" - that
should be mentioned here. You can use a link from here to the device
address section IIRC.
Hopefully the next time posted, the series will show up for me too!
John
> <h3><a name="seclabel">Security label</a></h3>
>
> <p>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index bdf7103..6e3b0fd 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -4506,6 +4506,7 @@
> <ref name="tpm"/>
> <ref name="shmem"/>
> <ref name="memorydev"/>
> + <ref name="crypto"/>
> </choice>
> </zeroOrMore>
> <optional>
> @@ -5052,6 +5053,35 @@
> </optional>
> </define>
>
> + <define name="crypto">
> + <element name="crypto">
> + <attribute name="model">
> + <choice>
> + <value>virtio</value>
> + </choice>
> + </attribute>
> + <ref name="crypto-backend"/>
> + <optional>
> + <ref name="address"/>
> + </optional>
> + </element>
> + </define>
> +
> + <define name="crypto-backend">
> + <element name="backend">
> + <attribute name="type">
> + <choice>
> + <value>builtin</value>
> + </choice>
> + </attribute>
> + <optional>
> + <attribute name="queues">
> + <ref name="positiveInteger"/>
> + </attribute>
> + </optional>
> + </element>
> + </define>
> +
> <define name="usbmaster">
> <element name="master">
> <attribute name="startport">
>
More information about the libvir-list
mailing list