[libvirt] [PATCH v2] docs: formatdomain: Document the CPU feature 'name' attribute

Kashyap Chamarthy posted 1 patch 6 years, 2 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20180124152549.29130-1-kchamart@redhat.com
There is a newer version of this series
docs/formatdomain.html.in | 14 ++++++++++++++
1 file changed, 14 insertions(+)
[libvirt] [PATCH v2] docs: formatdomain: Document the CPU feature 'name' attribute
Posted by Kashyap Chamarthy 6 years, 2 months ago
Currently, the CPU feature 'name' XML attribute, as in:

    [...]
    <cpu match='exact'>
      <model fallback='forbid'>IvyBridge</model>
      <vendor>Intel</vendor>
      <feature policy='require' name='pcid'/>
    </cpu>
    [...]

isn't explicitly documented in formatdomain.html.

Document it now.

Signed-off-by: Kashyap Chamarthy <kchamart@redhat.com>
---
v2:
 - Remove redundant line [Eduardo Habkost]

 docs/formatdomain.html.in | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index d272cc1ba..a6d40411c 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -1454,6 +1454,20 @@
 
         <span class="since">Since 0.8.5</span> the <code>policy</code>
         attribute can be omitted and will default to <code>require</code>.
+
+        Individual CPU feature names can be specified as part of the
+        <code>name</code> attribute. For example, to explicitly specify
+        the 'pcid' feature with Intel IvyBridge CPU model:
+
+<pre>
+...
+&lt;cpu match='exact'&gt;
+  &lt;model fallback='forbid'&gt;IvyBridge&lt;/model&gt;
+  &lt;vendor&gt;Intel&lt;/vendor&gt;
+  &lt;feature policy='require' name='pcid'/&gt;
+&lt;/cpu&gt;
+...</pre>
+
       </dd>
 
       <dt><code>cache</code></dt>
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] docs: formatdomain: Document the CPU feature 'name' attribute
Posted by Kashyap Chamarthy 6 years, 2 months ago
On Wed, Jan 24, 2018 at 04:25:49PM +0100, Kashyap Chamarthy wrote:

[...]

> ---
> v2:
>  - Remove redundant line [Eduardo Habkost]
> 
>  docs/formatdomain.html.in | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index d272cc1ba..a6d40411c 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -1454,6 +1454,20 @@
>  
>          <span class="since">Since 0.8.5</span> the <code>policy</code>
>          attribute can be omitted and will default to <code>require</code>.
> +
> +        Individual CPU feature names can be specified as part of the

Eduardo also asked: s/can/should/.  Forgot to do that.

Whoever applying this patch can touch up that?

[...]

-- 
/kashyap

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] docs: formatdomain: Document the CPU feature 'name' attribute
Posted by John Ferlan 6 years, 2 months ago

On 01/24/2018 10:25 AM, Kashyap Chamarthy wrote:
> Currently, the CPU feature 'name' XML attribute, as in:
> 
>     [...]
>     <cpu match='exact'>
>       <model fallback='forbid'>IvyBridge</model>
>       <vendor>Intel</vendor>
>       <feature policy='require' name='pcid'/>
>     </cpu>
>     [...]
> 
> isn't explicitly documented in formatdomain.html.
> 
> Document it now.
> 
> Signed-off-by: Kashyap Chamarthy <kchamart@redhat.com>
> ---
> v2:
>  - Remove redundant line [Eduardo Habkost]
> 
>  docs/formatdomain.html.in | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 

When built, formatted, rendered, and read in a browser one gets in one
long line:

Since 0.8.5 the policy attribute can be omitted and will default to
require. Individual CPU feature names should be specified as part of the
name attribute. For example, to explicitly specify the 'pcid' feature
with Intel IvyBridge CPU model:
...

So I think wrapping your change with <p> ... </p> will at least make it
look like a separate paragraph within the <feature> element description.

> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index d272cc1ba..a6d40411c 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -1454,6 +1454,20 @@
>  
>          <span class="since">Since 0.8.5</span> the <code>policy</code>
>          attribute can be omitted and will default to <code>require</code>.
> +
> +        Individual CPU feature names can be specified as part of the

Since the name attribute is required, rather than "can be" or "should
be" (as Eduardo suggested), I think perhaps "features names are
specified using the required <code>name</name> attribute."

or

"The required <code>name</code> attribute is used specify each desired
CPU feature."

(because we state initially "The cpu element can contain zero or more
elements...").

> +        <code>name</code> attribute. For example, to explicitly specify

s/specify/require


Thoughts?  I can make the adjustment before pushing if desired.

John



> +        the 'pcid' feature with Intel IvyBridge CPU model:
> +
> +<pre>
> +...
> +&lt;cpu match='exact'&gt;
> +  &lt;model fallback='forbid'&gt;IvyBridge&lt;/model&gt;
> +  &lt;vendor&gt;Intel&lt;/vendor&gt;
> +  &lt;feature policy='require' name='pcid'/&gt;
> +&lt;/cpu&gt;
> +...</pre>
> +
>        </dd>
>  
>        <dt><code>cache</code></dt>
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] docs: formatdomain: Document the CPU feature 'name' attribute
Posted by Kashyap Chamarthy 6 years, 2 months ago
On Wed, Jan 24, 2018 at 03:31:31PM -0500, John Ferlan wrote:
> On 01/24/2018 10:25 AM, Kashyap Chamarthy wrote:

[...]

> >  docs/formatdomain.html.in | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> 
> When built, formatted, rendered, and read in a browser one gets in one
> long line:

[...]

> So I think wrapping your change with <p> ... </p> will at least make it
> look like a separate paragraph within the <feature> element description.

Ah, good catch.  Fixed in v3.

[...]

> > +        Individual CPU feature names can be specified as part of the
> 
> Since the name attribute is required, rather than "can be" or "should
> be" (as Eduardo suggested), I think perhaps "features names are
> specified using the required <code>name</name> attribute."

I went with your above wording in v3.

> or
> 
> "The required <code>name</code> attribute is used specify each desired
> CPU feature."
> 
> (because we state initially "The cpu element can contain zero or more
> elements...").
> 
> > +        <code>name</code> attribute. For example, to explicitly specify
> 
> s/specify/require

I used the verb 'specify' to indicate that there is an _action_ to be
taken.  To my non-native ears: "to explicitly require" sounds slightly
odd when asking to take an action.

But I'll defer to your native tounge intuition.

> Thoughts?  I can make the adjustment before pushing if desired.

Thanks for the review.  Sending a v3; feel free to adjust it as you see
fit.

-- 
/kashyap

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] docs: formatdomain: Document the CPU feature 'name' attribute
Posted by John Ferlan 6 years, 2 months ago
[...]

>>
>>> +        <code>name</code> attribute. For example, to explicitly specify
>>
>> s/specify/require
> 
> I used the verb 'specify' to indicate that there is an _action_ to be
> taken.  To my non-native ears: "to explicitly require" sounds slightly
> odd when asking to take an action.
> 
> But I'll defer to your native tounge intuition.
> 

FWIW: I noted require because the generated XML in the example is:

 <feature policy='require' name='pcid'/>

I'm OK with the change from v3, but the XML is what I was keying off.
Essentially the line (to me) says, this domain requires a CPU that is
required to have the 'pcid' feature. Perhaps just being too literal though.

John

>> Thoughts?  I can make the adjustment before pushing if desired.
> 
> Thanks for the review.  Sending a v3; feel free to adjust it as you see
> fit.
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] docs: formatdomain: Document the CPU feature 'name' attribute
Posted by Kashyap Chamarthy 6 years, 2 months ago
On Thu, Jan 25, 2018 at 09:17:22AM -0500, John Ferlan wrote:
> [...]
> 
> >>
> >>> +        <code>name</code> attribute. For example, to explicitly specify
> >>
> >> s/specify/require
> > 
> > I used the verb 'specify' to indicate that there is an _action_ to be
> > taken.  To my non-native ears: "to explicitly require" sounds slightly
> > odd when asking to take an action.
> > 
> > But I'll defer to your native tounge intuition.
> > 
> 
> FWIW: I noted require because the generated XML in the example is:
> 
>  <feature policy='require' name='pcid'/>
> 
> I'm OK with the change from v3, but the XML is what I was keying off.
> Essentially the line (to me) says, this domain requires a CPU that is
> required to have the 'pcid' feature. Perhaps just being too literal though.

Ah, I see where you're coming from now.  

If you're merging v3, feel free to tweak it.

Thanks.

[...]

-- 
/kashyap

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] docs: formatdomain: Document the CPU feature 'name' attribute
Posted by Eduardo Habkost 6 years, 2 months ago
On Thu, Jan 25, 2018 at 09:17:22AM -0500, John Ferlan wrote:
> [...]
> >>
> >>> +        <code>name</code> attribute. For example, to explicitly specify
> >>
> >> s/specify/require
> > 
> > I used the verb 'specify' to indicate that there is an _action_ to be
> > taken.  To my non-native ears: "to explicitly require" sounds slightly
> > odd when asking to take an action.
> > 
> > But I'll defer to your native tounge intuition.
> > 
> 
> FWIW: I noted require because the generated XML in the example is:
> 
>  <feature policy='require' name='pcid'/>
> 
> I'm OK with the change from v3, but the XML is what I was keying off.
> Essentially the line (to me) says, this domain requires a CPU that is
> required to have the 'pcid' feature. Perhaps just being too literal though.

IMO, "requiring a CPU" is not the important part.  The most
important consequence of <feature/> is changing what exactly the
guest will see on the virtual CPU.  On most cases, this requires
a host CPU that provides the feature, but it's just a
consequence of how it works, not the main goal.

-- 
Eduardo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list