[Xen-devel,2/5,v2] libxl: Change the type of console_mfn to xen_pfn_t

Message ID 1508923628-26446-2-git-send-email-bhupinder.thakur@linaro.org
State New
Headers show
Series
  • [Xen-devel,1/5,v2] libxl: Fix the bug introduced in commit "libxl: use correct type modifier for vuart_gfn"
Related show

Commit Message

Bhupinder Thakur Oct. 25, 2017, 9:27 a.m.
Currently the type of console mfn is unsigned long in libxl. This may be
an issue for 32-bit toolstack running on 64-bit Xen, where the pfn are
64 bit. To ensure that console_mfn can hold any valid 64-bit pfn, the
type of console_mfn is changed to xen_pfn_t.

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---
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>

This patch is as per the review of commit fa1f157
    libxl: Fix the bug introduced in commit "libxl: use correct type

 tools/libxl/libxl_console.c  | 2 +-
 tools/libxl/libxl_dom.c      | 2 +-
 tools/libxl/libxl_internal.h | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

Comments

Wei Liu Oct. 26, 2017, 11:05 a.m. | #1
On Wed, Oct 25, 2017 at 02:57:05PM +0530, Bhupinder Thakur wrote:
> Currently the type of console mfn is unsigned long in libxl. This may be
> an issue for 32-bit toolstack running on 64-bit Xen, where the pfn are
> 64 bit. To ensure that console_mfn can hold any valid 64-bit pfn, the
> type of console_mfn is changed to xen_pfn_t.
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>

Acked-by: Wei Liu <wei.liu2@citrix.com>
Wei Liu Oct. 26, 2017, 11:13 a.m. | #2
On Wed, Oct 25, 2017 at 02:57:05PM +0530, Bhupinder Thakur wrote:
> Currently the type of console mfn is unsigned long in libxl. This may be
> an issue for 32-bit toolstack running on 64-bit Xen, where the pfn are
> 64 bit. To ensure that console_mfn can hold any valid 64-bit pfn, the
> type of console_mfn is changed to xen_pfn_t.
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
> 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>
> 
> This patch is as per the review of commit fa1f157
>     libxl: Fix the bug introduced in commit "libxl: use correct type
> 
>  tools/libxl/libxl_console.c  | 2 +-
>  tools/libxl/libxl_dom.c      | 2 +-
>  tools/libxl/libxl_internal.h | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/libxl_console.c b/tools/libxl/libxl_console.c
> index 6bfc0e5..f2ca689 100644
> --- a/tools/libxl/libxl_console.c
> +++ b/tools/libxl/libxl_console.c
> @@ -329,7 +329,7 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
>          flexarray_append(ro_front, "port");
>          flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->console_port));
>          flexarray_append(ro_front, "ring-ref");
> -        flexarray_append(ro_front, GCSPRINTF("%lu", state->console_mfn));
> +        flexarray_append(ro_front, GCSPRINTF("%"PRIu_xen_pfn, state->console_mfn));

Actually, please consider changing console_mfn to console_pfn.

You can keep my ack if you make such change.
Andrew Cooper Oct. 26, 2017, 11:17 a.m. | #3
On 26/10/17 12:13, Wei Liu wrote:
> On Wed, Oct 25, 2017 at 02:57:05PM +0530, Bhupinder Thakur wrote:
>> Currently the type of console mfn is unsigned long in libxl. This may be
>> an issue for 32-bit toolstack running on 64-bit Xen, where the pfn are
>> 64 bit. To ensure that console_mfn can hold any valid 64-bit pfn, the
>> type of console_mfn is changed to xen_pfn_t.
>>
>> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
>> ---
>> 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>
>>
>> This patch is as per the review of commit fa1f157
>>     libxl: Fix the bug introduced in commit "libxl: use correct type
>>
>>  tools/libxl/libxl_console.c  | 2 +-
>>  tools/libxl/libxl_dom.c      | 2 +-
>>  tools/libxl/libxl_internal.h | 2 +-
>>  3 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_console.c b/tools/libxl/libxl_console.c
>> index 6bfc0e5..f2ca689 100644
>> --- a/tools/libxl/libxl_console.c
>> +++ b/tools/libxl/libxl_console.c
>> @@ -329,7 +329,7 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
>>          flexarray_append(ro_front, "port");
>>          flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->console_port));
>>          flexarray_append(ro_front, "ring-ref");
>> -        flexarray_append(ro_front, GCSPRINTF("%lu", state->console_mfn));
>> +        flexarray_append(ro_front, GCSPRINTF("%"PRIu_xen_pfn, state->console_mfn));
> Actually, please consider changing console_mfn to console_pfn.

If you are going to make this change, then it is a gfn, not a pfn. 
(console_pfn would be as equally wrong for PV guests as console_mfn is
currently wrong for HVM guest.)

~Andrew
Bhupinder Thakur Oct. 30, 2017, 9:12 a.m. | #4
Hi,

On 26 October 2017 at 16:47, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 26/10/17 12:13, Wei Liu wrote:
>> On Wed, Oct 25, 2017 at 02:57:05PM +0530, Bhupinder Thakur wrote:
>>> Currently the type of console mfn is unsigned long in libxl. This may be
>>> an issue for 32-bit toolstack running on 64-bit Xen, where the pfn are
>>> 64 bit. To ensure that console_mfn can hold any valid 64-bit pfn, the
>>> type of console_mfn is changed to xen_pfn_t.
>>>
>>> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
>>> ---
>>> 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>
>>>
>>> This patch is as per the review of commit fa1f157
>>>     libxl: Fix the bug introduced in commit "libxl: use correct type
>>>
>>>  tools/libxl/libxl_console.c  | 2 +-
>>>  tools/libxl/libxl_dom.c      | 2 +-
>>>  tools/libxl/libxl_internal.h | 2 +-
>>>  3 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tools/libxl/libxl_console.c b/tools/libxl/libxl_console.c
>>> index 6bfc0e5..f2ca689 100644
>>> --- a/tools/libxl/libxl_console.c
>>> +++ b/tools/libxl/libxl_console.c
>>> @@ -329,7 +329,7 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
>>>          flexarray_append(ro_front, "port");
>>>          flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->console_port));
>>>          flexarray_append(ro_front, "ring-ref");
>>> -        flexarray_append(ro_front, GCSPRINTF("%lu", state->console_mfn));
>>> +        flexarray_append(ro_front, GCSPRINTF("%"PRIu_xen_pfn, state->console_mfn));
>> Actually, please consider changing console_mfn to console_pfn.
>
> If you are going to make this change, then it is a gfn, not a pfn.
> (console_pfn would be as equally wrong for PV guests as console_mfn is
> currently wrong for HVM guest.)

Changing console_mfn to console_gfn will require changes in many
files. Should I go ahead and change all the files?

Regards,
Bhupinder
Wei Liu Oct. 30, 2017, 10:33 a.m. | #5
On Mon, Oct 30, 2017 at 02:42:57PM +0530, Bhupinder Thakur wrote:
> Hi,
> 
> On 26 October 2017 at 16:47, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> > On 26/10/17 12:13, Wei Liu wrote:
> >> On Wed, Oct 25, 2017 at 02:57:05PM +0530, Bhupinder Thakur wrote:
> >>> Currently the type of console mfn is unsigned long in libxl. This may be
> >>> an issue for 32-bit toolstack running on 64-bit Xen, where the pfn are
> >>> 64 bit. To ensure that console_mfn can hold any valid 64-bit pfn, the
> >>> type of console_mfn is changed to xen_pfn_t.
> >>>
> >>> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> >>> ---
> >>> 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>
> >>>
> >>> This patch is as per the review of commit fa1f157
> >>>     libxl: Fix the bug introduced in commit "libxl: use correct type
> >>>
> >>>  tools/libxl/libxl_console.c  | 2 +-
> >>>  tools/libxl/libxl_dom.c      | 2 +-
> >>>  tools/libxl/libxl_internal.h | 2 +-
> >>>  3 files changed, 3 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/tools/libxl/libxl_console.c b/tools/libxl/libxl_console.c
> >>> index 6bfc0e5..f2ca689 100644
> >>> --- a/tools/libxl/libxl_console.c
> >>> +++ b/tools/libxl/libxl_console.c
> >>> @@ -329,7 +329,7 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
> >>>          flexarray_append(ro_front, "port");
> >>>          flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->console_port));
> >>>          flexarray_append(ro_front, "ring-ref");
> >>> -        flexarray_append(ro_front, GCSPRINTF("%lu", state->console_mfn));
> >>> +        flexarray_append(ro_front, GCSPRINTF("%"PRIu_xen_pfn, state->console_mfn));
> >> Actually, please consider changing console_mfn to console_pfn.
> >
> > If you are going to make this change, then it is a gfn, not a pfn.
> > (console_pfn would be as equally wrong for PV guests as console_mfn is
> > currently wrong for HVM guest.)
> 
> Changing console_mfn to console_gfn will require changes in many
> files. Should I go ahead and change all the files?
> 

$ cd libxl && git grep \\bconsole_mfn | wc -l
14

Not too bad, so please go ahead.

Patch

diff --git a/tools/libxl/libxl_console.c b/tools/libxl/libxl_console.c
index 6bfc0e5..f2ca689 100644
--- a/tools/libxl/libxl_console.c
+++ b/tools/libxl/libxl_console.c
@@ -329,7 +329,7 @@  int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
         flexarray_append(ro_front, "port");
         flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->console_port));
         flexarray_append(ro_front, "ring-ref");
-        flexarray_append(ro_front, GCSPRINTF("%lu", state->console_mfn));
+        flexarray_append(ro_front, GCSPRINTF("%"PRIu_xen_pfn, state->console_mfn));
     } else {
         flexarray_append(front, "state");
         flexarray_append(front, GCSPRINTF("%d", XenbusStateInitialising));
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index ef834e6..a58e74f 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -869,7 +869,7 @@  out:
 static int hvm_build_set_params(xc_interface *handle, uint32_t domid,
                                 libxl_domain_build_info *info,
                                 int store_evtchn, unsigned long *store_mfn,
-                                int console_evtchn, unsigned long *console_mfn,
+                                int console_evtchn, xen_pfn_t *console_mfn,
                                 domid_t store_domid, domid_t console_domid)
 {
     struct hvm_info_table *va_hvm;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 45e6df6..f52aeb3 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1128,7 +1128,7 @@  typedef struct {
 
     uint32_t console_port;
     uint32_t console_domid;
-    unsigned long console_mfn;
+    xen_pfn_t console_mfn;
     char *console_tty;
 
     char *saved_state;