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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Ian Jackson <ian.jackson@citrix.com>
Cc: Anthony Perard <anthony.perard@citrix.com>,
	Xen-devel <xen-devel@lists.xenproject.org>, Wei Liu <wl@xen.org>
Subject: Re: [Xen-devel] [PATCH 5/6] tools/libx[cl]: Don't use HVM_PARAM_PAE_ENABLED as a function parameter
Date: Tue, 11 Feb 2020 17:55:08 +0000	[thread overview]
Message-ID: <dcac4b7b-c790-981f-2d9c-3277c46a9a38@citrix.com> (raw)
In-Reply-To: <24130.59555.850210.126017@mariner.uk.xensource.com>

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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel