Re: [Xen-devel] [PATCH 5/6] tools/libx[cl]: Don't use HVM_PARAM_PAE_ENABLED as a function parameter

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Andrew Cooper writes ("[PATCH 5/6] tools/libx[cl]: Don't use 
HVM_PARAM_PAE_ENABLED as a function parameter"):
> The sole use of HVM_PARAM_PAE_ENABLED is as a non-standard calling
> convention for xc_cpuid_apply_policy().  Pass PAE as a regular
> parameter instead.

Following our conversation on irc, I have tried to write an
explanation in my own words of what this commit is doing.

  The value of HVM_PARAM_PAE_ENABLED is set by the toolstack.  And the
  only place that reads it is also in the toolstack, in
  xc_cpuid_apply_policy.  Effectively, this hypervisor domain
  parameter is used solely as a way to pass this boolean parameter
  from one part of the toolstack to another.

  This is not sensible.

  Replace its use in xc_cpuid_apply_policy with a plain boolean
  parameter, passed directly by the one (in-tree) caller.
  The now-redundant code to set the value in the hypervisor will be
  deleted in the next patch.

> Leave a rather better explaination of why only HVM guests have a
> choice in PAE setting.

I approve of this part of the commit message.

> No functional change.

I would write

   No overall functional change.  The new code fior calculating the
   `pae' value (in libxl__cpuid_legacy) is isomorphic to the
   obselescent code (in libxl_x86.c).

I had a look to see whether this was true and it seemed to be.

>          /*
> -         * HVM_PARAM_PAE_ENABLED is a parameter to this function, stashed in
> -         * Xen.  Nothing else has ever taken notice of the value.
> +         * PAE used to be a parameter passed to this function by
> +         * HVM_PARAM_PAE_ENABLED.  It is now passed normally.
>           */

I find this phrasing confusing, particularly this very loose use of
the word `parameter'.  I would drop this comment entirely and let the
commit message stand as the historical documentation.

>      char *cpuid_res[4];
> +    bool pae = true;
> +
> +    /*
> +     * PAE is a Xen-controlled for PV guests (it is the 'p' that causes the
> +     * difference between the xen-3.0-x86_32 and xen-3.0-x86_32p ABIs).  It\
 is
> +     * mandatory as Xen is running in 64bit mode.
> +     *
> +     * PVH guests don't have a top-level PAE control, and is treated as
> +     * available.
> +     */

I approve of putting a new comment here with an explanation.  However,
it should be wrapped rather more tightly (eg, in my MUA it is now
suffering from wrap damage as I demonstrate above) and it seems to
have some problems with the grammar ?  And I think the 2nd sentence
"It is mandatory" could usefully be re-qualified "for PV guests".  Or
you could write something like this.

   PV guests: PAE is Xen-controlled (it is the 'p' that causes the
   difference between the xen-3.0-x86_32 and xen-3.0-x86_32p ABIs);
   Xen is in 64-bit mode so PAE is mandatory.

   PVH guests: there is no top-level PAE control in the libx domain
   config; we always make it available.

   So only this test only applies to HVM guests:

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel