[PATCH] tools/libxl: Fix build following c/s 74fd984ae

[PATCH] tools/libxl: Fix build following c/s 74fd984ae

[Andrew Cooper]

c/s 74fd984ae "tools/libxl: Drop xc_domain_configuration_t from
libxl__domain_build_state" removed state->config completely, but the GIC
version is available in info.  Use the up-to-date version.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: Juergen Gross <jgross@suse.com>

Completely untested.  I don't even have a compile environment to hand, which
is how this got missed before.  Sorry.
---
 tools/libxl/libxl_arm.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index fbe8786..177c6b7 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -846,9 +846,6 @@ static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_build_info *info,
     const libxl_version_info *vers;
     const struct arch_info *ainfo;
 
-    /* convenience aliases */
-    xc_domain_configuration_t *xc_config = &state->config;
-
     vers = libxl_get_version_info(CTX);
     if (vers == NULL) return ERROR_FAIL;
 
@@ -857,7 +854,8 @@ static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_build_info *info,
 
     LOG(DEBUG, "constructing DTB for Xen version %d.%d guest",
         vers->xen_version_major, vers->xen_version_minor);
-    LOG(DEBUG, " - vGIC version: %s", gicv_to_string(xc_config->gic_version));
+    LOG(DEBUG, " - vGIC version: %s",
+        gicv_to_string(info->arch_arm.gic_version));
 
     if (info->device_tree) {
         LOG(DEBUG, " - Partial device tree provided: %s", info->device_tree);
-- 
2.1.4


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

Re: [PATCH] tools/libxl: Fix build following c/s 74fd984ae

[Wei Liu]

On Thu, Apr 05, 2018 at 07:54:26PM +0100, Andrew Cooper wrote:
> c/s 74fd984ae "tools/libxl: Drop xc_domain_configuration_t from
> libxl__domain_build_state" removed state->config completely, but the GIC
> version is available in info.  Use the up-to-date version.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> CC: Juergen Gross <jgross@suse.com>
> 
> Completely untested.  I don't even have a compile environment to hand, which
> is how this got missed before.  Sorry.

Looks correct to me:

Acked-by: Wei Liu <wei.liu2@citrix.com>

> ---
>  tools/libxl/libxl_arm.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index fbe8786..177c6b7 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -846,9 +846,6 @@ static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_build_info *info,
>      const libxl_version_info *vers;
>      const struct arch_info *ainfo;
>  
> -    /* convenience aliases */
> -    xc_domain_configuration_t *xc_config = &state->config;
> -
>      vers = libxl_get_version_info(CTX);
>      if (vers == NULL) return ERROR_FAIL;
>  
> @@ -857,7 +854,8 @@ static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_build_info *info,
>  
>      LOG(DEBUG, "constructing DTB for Xen version %d.%d guest",
>          vers->xen_version_major, vers->xen_version_minor);
> -    LOG(DEBUG, " - vGIC version: %s", gicv_to_string(xc_config->gic_version));
> +    LOG(DEBUG, " - vGIC version: %s",
> +        gicv_to_string(info->arch_arm.gic_version));
>  
>      if (info->device_tree) {
>          LOG(DEBUG, " - Partial device tree provided: %s", info->device_tree);
> -- 
> 2.1.4
> 

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

Re: [PATCH] tools/libxl: Fix build following c/s 74fd984ae

[Juergen Gross]

On 06/04/18 10:00, Wei Liu wrote:
> On Thu, Apr 05, 2018 at 07:54:26PM +0100, Andrew Cooper wrote:
>> c/s 74fd984ae "tools/libxl: Drop xc_domain_configuration_t from
>> libxl__domain_build_state" removed state->config completely, but the GIC
>> version is available in info.  Use the up-to-date version.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien.grall@arm.com>
>> CC: Juergen Gross <jgross@suse.com>
>>
>> Completely untested.  I don't even have a compile environment to hand, which
>> is how this got missed before.  Sorry.
> 
> Looks correct to me:
> 
> Acked-by: Wei Liu <wei.liu2@citrix.com>

Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen

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

Re: [PATCH] tools/libxl: Fix build following c/s 74fd984ae

[Julien Grall]

Hi,

On 04/06/2018 09:00 AM, Wei Liu wrote:
> On Thu, Apr 05, 2018 at 07:54:26PM +0100, Andrew Cooper wrote:
>> c/s 74fd984ae "tools/libxl: Drop xc_domain_configuration_t from
>> libxl__domain_build_state" removed state->config completely, but the GIC
>> version is available in info.  Use the up-to-date version.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien.grall@arm.com>
>> CC: Juergen Gross <jgross@suse.com>
>>
>> Completely untested.  I don't even have a compile environment to hand, which
>> is how this got missed before.  Sorry.
> 
> Looks correct to me:
> 
> Acked-by: Wei Liu <wei.liu2@citrix.com>

Sadly libxl still does not compile on ARM:

libxl_arm.c: In function 'libxl__prepare_dtb':
libxl_arm.c:923:17: error: 'xc_config' undeclared (first use in this function); did you mean 'yajl_config'?
         switch (xc_config->gic_version) {
                 ^~~~~~~~~
                 yajl_config
libxl_arm.c:923:17: note: each undeclared identifier is reported only once for each function it appears in

> 
>> ---
>>   tools/libxl/libxl_arm.c | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
>> index fbe8786..177c6b7 100644
>> --- a/tools/libxl/libxl_arm.c
>> +++ b/tools/libxl/libxl_arm.c
>> @@ -846,9 +846,6 @@ static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_build_info *info,
>>       const libxl_version_info *vers;
>>       const struct arch_info *ainfo;
>>   
>> -    /* convenience aliases */
>> -    xc_domain_configuration_t *xc_config = &state->config;
>> -
>>       vers = libxl_get_version_info(CTX);
>>       if (vers == NULL) return ERROR_FAIL;
>>   
>> @@ -857,7 +854,8 @@ static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_build_info *info,
>>   
>>       LOG(DEBUG, "constructing DTB for Xen version %d.%d guest",
>>           vers->xen_version_major, vers->xen_version_minor);
>> -    LOG(DEBUG, " - vGIC version: %s", gicv_to_string(xc_config->gic_version));
>> +    LOG(DEBUG, " - vGIC version: %s",
>> +        gicv_to_string(info->arch_arm.gic_version));

This is wrong. gicv_to_string works on XEN_DOMCTL_* define and not the LIBXL_GIC_*.
So this will not give the right output.

I would suggest to revert that patch and I will send one that actually fix the compilation.
Not sure I would be able to do it today thought.

Cheers,

-- 
Julien Grall

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

Re: [PATCH] tools/libxl: Fix build following c/s 74fd984ae

[Juergen Gross]

On 06/04/18 11:03, Julien Grall wrote:
> Hi,
> 
> On 04/06/2018 09:00 AM, Wei Liu wrote:
>> On Thu, Apr 05, 2018 at 07:54:26PM +0100, Andrew Cooper wrote:
>>> c/s 74fd984ae "tools/libxl: Drop xc_domain_configuration_t from
>>> libxl__domain_build_state" removed state->config completely, but the GIC
>>> version is available in info.  Use the up-to-date version.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>>> CC: Wei Liu <wei.liu2@citrix.com>
>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>> CC: Julien Grall <julien.grall@arm.com>
>>> CC: Juergen Gross <jgross@suse.com>
>>>
>>> Completely untested.  I don't even have a compile environment to hand, which
>>> is how this got missed before.  Sorry.
>>
>> Looks correct to me:
>>
>> Acked-by: Wei Liu <wei.liu2@citrix.com>
> 
> Sadly libxl still does not compile on ARM:
> 
> libxl_arm.c: In function 'libxl__prepare_dtb':
> libxl_arm.c:923:17: error: 'xc_config' undeclared (first use in this function); did you mean 'yajl_config'?
>          switch (xc_config->gic_version) {
>                  ^~~~~~~~~
>                  yajl_config
> libxl_arm.c:923:17: note: each undeclared identifier is reported only once for each function it appears in
> 
>>
>>> ---
>>>   tools/libxl/libxl_arm.c | 6 ++----
>>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
>>> index fbe8786..177c6b7 100644
>>> --- a/tools/libxl/libxl_arm.c
>>> +++ b/tools/libxl/libxl_arm.c
>>> @@ -846,9 +846,6 @@ static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_build_info *info,
>>>       const libxl_version_info *vers;
>>>       const struct arch_info *ainfo;
>>>   
>>> -    /* convenience aliases */
>>> -    xc_domain_configuration_t *xc_config = &state->config;
>>> -
>>>       vers = libxl_get_version_info(CTX);
>>>       if (vers == NULL) return ERROR_FAIL;
>>>   
>>> @@ -857,7 +854,8 @@ static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_build_info *info,
>>>   
>>>       LOG(DEBUG, "constructing DTB for Xen version %d.%d guest",
>>>           vers->xen_version_major, vers->xen_version_minor);
>>> -    LOG(DEBUG, " - vGIC version: %s", gicv_to_string(xc_config->gic_version));
>>> +    LOG(DEBUG, " - vGIC version: %s",
>>> +        gicv_to_string(info->arch_arm.gic_version));
> 
> This is wrong. gicv_to_string works on XEN_DOMCTL_* define and not the LIBXL_GIC_*.
> So this will not give the right output.
> 
> I would suggest to revert that patch and I will send one that actually fix the compilation.
> Not sure I would be able to do it today thought.

Fine with me.


Juergen


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

Re: [PATCH] tools/libxl: Fix build following c/s 74fd984ae

[Wei Liu]

On Fri, Apr 06, 2018 at 10:03:14AM +0100, Julien Grall wrote:
> This is wrong. gicv_to_string works on XEN_DOMCTL_* define and not the LIBXL_GIC_*.
> So this will not give the right output.
> 
> I would suggest to revert that patch and I will send one that actually fix the compilation.
> Not sure I would be able to do it today thought.
> 

OK, I will revert this patch.

> Cheers,
> 
> -- 
> Julien Grall

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

Re: [PATCH] tools/libxl: Fix build following c/s 74fd984ae

[Julien Grall]

Hi,

On 06/04/18 10:33, Wei Liu wrote:
> On Fri, Apr 06, 2018 at 10:03:14AM +0100, Julien Grall wrote:
>> This is wrong. gicv_to_string works on XEN_DOMCTL_* define and not the LIBXL_GIC_*.
>> So this will not give the right output.
>>
>> I would suggest to revert that patch and I will send one that actually fix the compilation.
>> Not sure I would be able to do it today thought.
>>
> 
> OK, I will revert this patch.

I have looked at a potential way to fix it. The original patch 
(74fd984ae6) assumption is incorrect. Some of information from 
xc_domain_configuration_t is not written back ton libxl__domain_build_state.

For instance, this is the case of the clock frequency. That field is 
used to workaround bootloader/firmware that didn't configure correct 
CNTFRQ. If we detect such platform, we will read the host clock 
frequency from the host Device-Tree and write it to the guest 
Device-Tree. This should never be exposed to the guest.

Arguably, this field should not belong to xc_domain_configuration_t. So 
I can see two solutions:
	1) Store the frequency in libxl__domain_build_state
	2) Introduce a different hypercall to get the system frequency. I guess 
we could piggy-back on Andrew's idea to provide 
XEN_DOMCTL_{get,set}_arch_settings.

The latter will require some rework in the code and define a new API. 
Not sure if that would be acceptable for Xen 4.11.

Any opinions?

Cheers,

-- 
Julien Grall

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

Re: [PATCH] tools/libxl: Fix build following c/s 74fd984ae

[Wei Liu]

On Mon, Apr 09, 2018 at 11:29:28AM +0100, Julien Grall wrote:
> Hi,
> 
> On 06/04/18 10:33, Wei Liu wrote:
> > On Fri, Apr 06, 2018 at 10:03:14AM +0100, Julien Grall wrote:
> > > This is wrong. gicv_to_string works on XEN_DOMCTL_* define and not the LIBXL_GIC_*.
> > > So this will not give the right output.
> > > 
> > > I would suggest to revert that patch and I will send one that actually fix the compilation.
> > > Not sure I would be able to do it today thought.
> > > 
> > 
> > OK, I will revert this patch.
> 
> I have looked at a potential way to fix it. The original patch (74fd984ae6)
> assumption is incorrect. Some of information from xc_domain_configuration_t
> is not written back ton libxl__domain_build_state.
> 
> For instance, this is the case of the clock frequency. That field is used to
> workaround bootloader/firmware that didn't configure correct CNTFRQ. If we
> detect such platform, we will read the host clock frequency from the host
> Device-Tree and write it to the guest Device-Tree. This should never be
> exposed to the guest.

Not sure I follow. If you write that value to guest DT, guest should be
able to see it?

> 
> Arguably, this field should not belong to xc_domain_configuration_t. So I
> can see two solutions:
> 	1) Store the frequency in libxl__domain_build_state
> 	2) Introduce a different hypercall to get the system frequency. I guess we
> could piggy-back on Andrew's idea to provide
> XEN_DOMCTL_{get,set}_arch_settings.
> 
> The latter will require some rework in the code and define a new API. Not
> sure if that would be acceptable for Xen 4.11.
> 
> Any opinions?

#1 seems simpler to me, especially since we already have vuart (ARM
only) information in libxl__domain_build_state.

Wei.

> 
> Cheers,
> 
> -- 
> Julien Grall

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

Re: [PATCH] tools/libxl: Fix build following c/s 74fd984ae

[Julien Grall]

Hi,

On 09/04/18 12:07, Wei Liu wrote:
> On Mon, Apr 09, 2018 at 11:29:28AM +0100, Julien Grall wrote:
>> Hi,
>>
>> On 06/04/18 10:33, Wei Liu wrote:
>>> On Fri, Apr 06, 2018 at 10:03:14AM +0100, Julien Grall wrote:
>>>> This is wrong. gicv_to_string works on XEN_DOMCTL_* define and not the LIBXL_GIC_*.
>>>> So this will not give the right output.
>>>>
>>>> I would suggest to revert that patch and I will send one that actually fix the compilation.
>>>> Not sure I would be able to do it today thought.
>>>>
>>>
>>> OK, I will revert this patch.
>>
>> I have looked at a potential way to fix it. The original patch (74fd984ae6)
>> assumption is incorrect. Some of information from xc_domain_configuration_t
>> is not written back ton libxl__domain_build_state.
>>
>> For instance, this is the case of the clock frequency. That field is used to
>> workaround bootloader/firmware that didn't configure correct CNTFRQ. If we
>> detect such platform, we will read the host clock frequency from the host
>> Device-Tree and write it to the guest Device-Tree. This should never be
>> exposed to the guest.
> 
> Not sure I follow. If you write that value to guest DT, guest should be
> able to see it?

On normal system the clock frequency can be read by using the system 
register CNTFRQ. This register is usually initialized by the 
firmware/bootloader. However, on some platforms the firmware does not do 
its job and therefore CNTFRQ is configured incorrectly. To workaround 
the problem, a property exists in the host DT with the correct frequency.

On the broken platform, the field clock_frequency in 
xc_domain_configuration_t will be written by the hypervisor with the 
correct frequency.

> 
>>
>> Arguably, this field should not belong to xc_domain_configuration_t. So I
>> can see two solutions:
>> 	1) Store the frequency in libxl__domain_build_state
>> 	2) Introduce a different hypercall to get the system frequency. I guess we
>> could piggy-back on Andrew's idea to provide
>> XEN_DOMCTL_{get,set}_arch_settings.
>>
>> The latter will require some rework in the code and define a new API. Not
>> sure if that would be acceptable for Xen 4.11.
>>
>> Any opinions?
> 
> #1 seems simpler to me, especially since we already have vuart (ARM
> only) information in libxl__domain_build_state.

I will have a look at that.

Cheers,

-- 
Julien Grall

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