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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 11/02/2020 17:47, Ian Jackson wrote:
> 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.
>>
>> Leave a rather better explaination of why only HVM guests have a choice in 
>> PAE
>> setting.
> I am inclined believe you that this is right (since you are evidently
> familiar with this whole area and I'm not), but the explanations leave
> me confused.
>
>>  int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
>> -                          const uint32_t *featureset, unsigned int 
>> nr_features)
>> +                          const uint32_t *featureset, unsigned int 
>> nr_features,
>> +                          bool pae)
>>  {
>>      int rc;
>>      xc_dominfo_t di;
>> @@ -579,8 +580,6 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t 
>> domid,
>>      }
>>      else
>>      {
>> -        uint64_t val;
>> -
>>          /*
>>           * Topology for HVM guests is entirely controlled by Xen.  For now, 
>> we
>>           * hardcode APIC_ID = vcpu_id * 2 to give the illusion of no SMT.
>> @@ -635,14 +634,10 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t 
>> domid,
>>          }
>>  
>>          /*
>> -         * 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.
> In particular, I don't understand what these comments mean by
> "HVM_PARAM_PAE_ENABLED is a parameter to this function" and "PAE used
> to be a parameter passed to this function by HVM_PARAM_PAE_ENABLED".
>
> Maybe this is some loose use of the term "parameter" ?
>
> If you could explain more clearly (ideally, explain the meaning of the
> old comment in the commit message, and make the new comment
> unambiguous) then that would be great.

HVM_PARAM_PAE_ENABLED encapsulates a boolean meaning "should I advertise
the PAE feature to the guest?".

It has only ever been used in a way which should have been "bool pae"
passed into xc_cpuid_apply_policy().  This patch tries to do just that.


I think there might be confusion as to which comment the commit message
referred to.

In xc_cpuid_apply_policy(), I want a comment explaining why we have this
weird pae parameter.  It will disappear from the new way of doing CPUID
at boot, but will have to remain for the pre-4.14 compatibility.

The comment I was referring to in the commit message was actually the
libxl comment, explaining why PV and PVH guests don't get a choice to
hide the PAE feature.

~Andrew

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