[Xen-devel] xenconsole: Define and use a macro INVALID_XEN_PFN instead of -1

Message ID 1508258793-5690-5-git-send-email-bhupinder.thakur@linaro.org
State New
Headers show
Series
  • [Xen-devel] xenconsole: Define and use a macro INVALID_XEN_PFN instead of -1
Related show

Commit Message

Bhupinder Thakur Oct. 17, 2017, 4:46 p.m.
xenconsole will use a new macro INVALID_XEN_PFN instead of -1 for initializing ring-ref.

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: Andrew Cooper <andrew.cooper3@citrix.com>
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>

 tools/console/daemon/io.c | 10 +++++-----
 xen/include/public/xen.h  |  2 ++
 2 files changed, 7 insertions(+), 5 deletions(-)

Comments

Jan Beulich Oct. 18, 2017, 10:02 a.m. | #1
>>> On 17.10.17 at 18:46, <bhupinder.thakur@linaro.org> wrote:
> --- a/tools/console/daemon/io.c
> +++ b/tools/console/daemon/io.c
> @@ -658,12 +658,12 @@ static void console_unmap_interface(struct console *con)
>  {
>  	if (con->interface == NULL)
>  		return;
> -	if (xgt_handle && con->ring_ref == -1)
> +	if (xgt_handle && con->ring_ref == INVALID_XEN_PFN)
>  		xengnttab_unmap(xgt_handle, con->interface, 1);
>  	else
>  		munmap(con->interface, XC_PAGE_SIZE);
>  	con->interface = NULL;
> -	con->ring_ref = -1;
> +	con->ring_ref = INVALID_XEN_PFN;
>  }
>   
>  static int console_create_ring(struct console *con)
> @@ -698,7 +698,7 @@ static int console_create_ring(struct console *con)
>  	free(type);
>  
>  	/* If using ring_ref and it has changed, remap */
> -	if (ring_ref != con->ring_ref && con->ring_ref != -1)
> +	if (ring_ref != con->ring_ref && con->ring_ref != INVALID_XEN_PFN)
>  		console_unmap_interface(con);
>  
>  	if (!con->interface && xgt_handle && con->use_gnttab) {
> @@ -706,7 +706,7 @@ static int console_create_ring(struct console *con)
>  		con->interface = xengnttab_map_grant_ref(xgt_handle,
>  			dom->domid, GNTTAB_RESERVED_CONSOLE,
>  			PROT_READ|PROT_WRITE);
> -		con->ring_ref = -1;
> +		con->ring_ref = INVALID_XEN_PFN;
>  	}
>  	if (!con->interface) {
>  		/* Fall back to xc_map_foreign_range */
> @@ -812,7 +812,7 @@ static int console_init(struct console *con, struct domain *dom, void **data)
>  	con->master_pollfd_idx = -1;
>  	con->slave_fd = -1;
>  	con->log_fd = -1;
> -	con->ring_ref = -1;
> +	con->ring_ref = INVALID_XEN_PFN;
>  	con->local_port = -1;
>  	con->remote_port = -1;
>  	con->xce_pollfd_idx = -1;
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -37,6 +37,8 @@
>  #error "Unsupported architecture"
>  #endif
>  
> +#define INVALID_XEN_PFN (~(xen_pfn_t)0)

As said before, the uses of this which you introduce don't warrant
this addition to the public interface (which, if it was added, also
should start with XEN_). I'm not going to NAK such a (corrected)
addition to the public interface, but given the users I'm also not
going to ACK it (but perhaps another REST maintainer would).

Jan
Wei Liu Oct. 18, 2017, 11:53 a.m. | #2
On Wed, Oct 18, 2017 at 04:02:45AM -0600, Jan Beulich wrote:
> >>> On 17.10.17 at 18:46, <bhupinder.thakur@linaro.org> wrote:
> > --- a/tools/console/daemon/io.c
> > +++ b/tools/console/daemon/io.c
> > @@ -658,12 +658,12 @@ static void console_unmap_interface(struct console *con)
> >  {
> >  	if (con->interface == NULL)
> >  		return;
> > -	if (xgt_handle && con->ring_ref == -1)
> > +	if (xgt_handle && con->ring_ref == INVALID_XEN_PFN)
> >  		xengnttab_unmap(xgt_handle, con->interface, 1);
> >  	else
> >  		munmap(con->interface, XC_PAGE_SIZE);
> >  	con->interface = NULL;
> > -	con->ring_ref = -1;
> > +	con->ring_ref = INVALID_XEN_PFN;
> >  }
> >   
> >  static int console_create_ring(struct console *con)
> > @@ -698,7 +698,7 @@ static int console_create_ring(struct console *con)
> >  	free(type);
> >  
> >  	/* If using ring_ref and it has changed, remap */
> > -	if (ring_ref != con->ring_ref && con->ring_ref != -1)
> > +	if (ring_ref != con->ring_ref && con->ring_ref != INVALID_XEN_PFN)
> >  		console_unmap_interface(con);
> >  
> >  	if (!con->interface && xgt_handle && con->use_gnttab) {
> > @@ -706,7 +706,7 @@ static int console_create_ring(struct console *con)
> >  		con->interface = xengnttab_map_grant_ref(xgt_handle,
> >  			dom->domid, GNTTAB_RESERVED_CONSOLE,
> >  			PROT_READ|PROT_WRITE);
> > -		con->ring_ref = -1;
> > +		con->ring_ref = INVALID_XEN_PFN;
> >  	}
> >  	if (!con->interface) {
> >  		/* Fall back to xc_map_foreign_range */
> > @@ -812,7 +812,7 @@ static int console_init(struct console *con, struct domain *dom, void **data)
> >  	con->master_pollfd_idx = -1;
> >  	con->slave_fd = -1;
> >  	con->log_fd = -1;
> > -	con->ring_ref = -1;
> > +	con->ring_ref = INVALID_XEN_PFN;
> >  	con->local_port = -1;
> >  	con->remote_port = -1;
> >  	con->xce_pollfd_idx = -1;
> > --- a/xen/include/public/xen.h
> > +++ b/xen/include/public/xen.h
> > @@ -37,6 +37,8 @@
> >  #error "Unsupported architecture"
> >  #endif
> >  
> > +#define INVALID_XEN_PFN (~(xen_pfn_t)0)
> 
> As said before, the uses of this which you introduce don't warrant
> this addition to the public interface (which, if it was added, also
> should start with XEN_). I'm not going to NAK such a (corrected)
> addition to the public interface, but given the users I'm also not
> going to ACK it (but perhaps another REST maintainer would).
> 

I agree with you here. We don't need this in public interface yet.
Julien Grall Oct. 18, 2017, 1:05 p.m. | #3
Hi Wei,

On 10/18/2017 12:53 PM, Wei Liu wrote:
> On Wed, Oct 18, 2017 at 04:02:45AM -0600, Jan Beulich wrote:
>>>>> On 17.10.17 at 18:46, <bhupinder.thakur@linaro.org> wrote:
>>> --- a/xen/include/public/xen.h
>>> +++ b/xen/include/public/xen.h
>>> @@ -37,6 +37,8 @@
>>>   #error "Unsupported architecture"
>>>   #endif
>>>   
>>> +#define INVALID_XEN_PFN (~(xen_pfn_t)0)
>>
>> As said before, the uses of this which you introduce don't warrant
>> this addition to the public interface (which, if it was added, also
>> should start with XEN_). I'm not going to NAK such a (corrected)
>> addition to the public interface, but given the users I'm also not
>> going to ACK it (but perhaps another REST maintainer would).
>>
> 
> I agree with you here. We don't need this in public interface yet.

Couldn't this new define be used in place like xc_mem_access.c (they 
have a plain ~0UL for invalid GFN) or even LIBXL_INVALID_GFN?

Cheers,
Jan Beulich Oct. 18, 2017, 1:16 p.m. | #4
>>> On 18.10.17 at 15:05, <julien.grall@linaro.org> wrote:
> On 10/18/2017 12:53 PM, Wei Liu wrote:
>> On Wed, Oct 18, 2017 at 04:02:45AM -0600, Jan Beulich wrote:
>>>>>> On 17.10.17 at 18:46, <bhupinder.thakur@linaro.org> wrote:
>>>> --- a/xen/include/public/xen.h
>>>> +++ b/xen/include/public/xen.h
>>>> @@ -37,6 +37,8 @@
>>>>   #error "Unsupported architecture"
>>>>   #endif
>>>>   
>>>> +#define INVALID_XEN_PFN (~(xen_pfn_t)0)
>>>
>>> As said before, the uses of this which you introduce don't warrant
>>> this addition to the public interface (which, if it was added, also
>>> should start with XEN_). I'm not going to NAK such a (corrected)
>>> addition to the public interface, but given the users I'm also not
>>> going to ACK it (but perhaps another REST maintainer would).
>>>
>> 
>> I agree with you here. We don't need this in public interface yet.
> 
> Couldn't this new define be used in place like xc_mem_access.c (they 
> have a plain ~0UL for invalid GFN) or even LIBXL_INVALID_GFN?

Sure it could be, but it should be introduced to the public header
when a hypercall producer and/or consumer appears, not for the
internal purposes of xenconsole.

Jan

Patch

diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index 1839973..9129f5a 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
@@ -658,12 +658,12 @@  static void console_unmap_interface(struct console *con)
 {
 	if (con->interface == NULL)
 		return;
-	if (xgt_handle && con->ring_ref == -1)
+	if (xgt_handle && con->ring_ref == INVALID_XEN_PFN)
 		xengnttab_unmap(xgt_handle, con->interface, 1);
 	else
 		munmap(con->interface, XC_PAGE_SIZE);
 	con->interface = NULL;
-	con->ring_ref = -1;
+	con->ring_ref = INVALID_XEN_PFN;
 }
  
 static int console_create_ring(struct console *con)
@@ -698,7 +698,7 @@  static int console_create_ring(struct console *con)
 	free(type);
 
 	/* If using ring_ref and it has changed, remap */
-	if (ring_ref != con->ring_ref && con->ring_ref != -1)
+	if (ring_ref != con->ring_ref && con->ring_ref != INVALID_XEN_PFN)
 		console_unmap_interface(con);
 
 	if (!con->interface && xgt_handle && con->use_gnttab) {
@@ -706,7 +706,7 @@  static int console_create_ring(struct console *con)
 		con->interface = xengnttab_map_grant_ref(xgt_handle,
 			dom->domid, GNTTAB_RESERVED_CONSOLE,
 			PROT_READ|PROT_WRITE);
-		con->ring_ref = -1;
+		con->ring_ref = INVALID_XEN_PFN;
 	}
 	if (!con->interface) {
 		/* Fall back to xc_map_foreign_range */
@@ -812,7 +812,7 @@  static int console_init(struct console *con, struct domain *dom, void **data)
 	con->master_pollfd_idx = -1;
 	con->slave_fd = -1;
 	con->log_fd = -1;
-	con->ring_ref = -1;
+	con->ring_ref = INVALID_XEN_PFN;
 	con->local_port = -1;
 	con->remote_port = -1;
 	con->xce_pollfd_idx = -1;
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index 308109f..fc383ca 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -37,6 +37,8 @@ 
 #error "Unsupported architecture"
 #endif
 
+#define INVALID_XEN_PFN (~(xen_pfn_t)0)
+
 #ifndef __ASSEMBLY__
 /* Guest handles for primitive C types. */
 DEFINE_XEN_GUEST_HANDLE(char);