8:47 a.m.
On Sun, Mar 26, 2017 at 03:21:04PM -0400, Laine Stump wrote:
On 03/22/2017 11:27 AM, Erik Skultety wrote:
> Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
> ---
> docs/formatdomain.html.in | 46 ++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 40 insertions(+), 6 deletions(-)
I always like to put the docs changes in the same patch that modifies
the schema and XML parsing functions, but that's not a hard rule, so I'm
fine with this.
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 4a3123e989..1eb6c44b6f 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -3886,6 +3886,19 @@
> </devices>
> ...</pre>
>
> + <p>or:</p>
> +
> +<pre>
> + ...
> + <devices>
> + <hostdev mode='subsystem' type='mdev'
model='vfio-pci'>
> + <source>
> + <address uuid='c2177883-f1bb-47f0-914d-32a22e3a8804'>
> + </source>
> + </hostdev>
> + </devices>
> + ...</pre>
> +
> <dl>
> <dt><code>hostdev</code></dt>
> <dd>The <code>hostdev</code> element is the main container
for describing
> @@ -3930,12 +3943,22 @@
> <code>type</code> passes all LUNs presented by a single HBA
to
> the guest.
> </dd>
> + <dt><code>mdev</code></dt>
> + <dd>For mediated devices (<span class="since">Since
3.2.0</span>)
> + the <code>model</code> attribute specifies the device API
which
> + determines how the host's vfio driver will expose the device to the
> + guest. Currently, only <code>vfio-pci</code> model is
supported.
either "only the vfio-pci model is supported" or "only
model='vfio-pci'
is supported".
> + There are also some implications on the usage of guest's address
type
> + depending on the <code>model</code> attribute, see the
> + <code>address</code> element below.</dd>
I'm not really comfortable with this - it means that the code that
parses <address> has to have information from a higher level rather than
being self contained. Remind me why you decided to remove
"type='mdev'"
(or whatever it was) from the <address> syntax?
Because we're kind of abusing the <address> element when using it within
source. If you look at our docs [1] we define it only for guest addresses where
we actually require the type, but when we use it under source, we define what
kind of attributes are expected. Internally, we have an enum for each address
type we know (which we also check only when assigning guest addresses) which
means that I would have to add the enum for mdev and also document it under the
'Device Addresses' paragraph (which I did before), but the only thing I
documented then was that it's not supposed to be used as a guest address,
except that the paragraph headline indicates it's indeed about guest addresses.
Do you see the confusion in that, i.e. so do we or do we actually
not support mdev as a guest address?? (that was a rhetorical question...)
Also, besides our schema not being happy about that, you can happily define a
domain with an source address element containing some additional rubbish
attributes which will be silently ignored by the parser.
[1] http://libvirt.org/formatdomain.html#elementsAddress
> </dl>
> <p>
> - Note: The <code>managed</code> attribute is only used with
PCI devices
s/PCI/type='pci'/
> - and is ignored by all the other device types, thus setting
> - <code>managed</code> explicitly with other than PCI device
has the same
> - effect as omitting it.
> + Note: The <code>managed</code> attribute is only used with
PCI and is
> + ignored by all the other device types, thus setting
> + <code>managed</code> explicitly with other than a PCI device
has the
> + same effect as omitting it. Similarly, <code>model</code>
attribute is
> + only supported by mediated devices and ignored by all other device
> + types.
Do we want to ignore it, or log an error if someone specifies it when
they shouldn't? Doing the latter would prevent someone from thinking
they've done "A" when actually they've done "B".
As I wrote above, our parser is quite permissive in semantics and since usually
the case is that we ignore everything that does not make sense for us and only
error out if we got a value different from what we expected or we're missing a
mandatory attribute in general or there is a conflict in attributes, etc. If
someone then later complains that they have a different hostdev and expected
the model to do something, we can still confront them with the documentation
which states that it will be ignored for unsupported device types.
I'm still unsure about having no type specifier in <address>. I know
that's already the case for PCI addresses in <source>, but I actually
think that was an incorrect decision - it's cleaner if we can
parse/format <address> without reaching up into higher levels. Maybe I
can be convinced though :-)
See my response above
In the meantime, I think it would be better to get this all in and get
some solid testing outside of the few people directly involved, so ACK
to this patch too (which makes the entire series ACKed (with a few small
changes requested), and we can have one last debate about <address>
during the next couple days.
Thanks a lot for review, hopefully I didn't forget to adjust any of the patches
and pushed the series.
Erik