Re: [Xen-devel] [PATCH v3 23/25] xen: support console_switching between Dom0 and DomUs on ARM

2018-10-05 Thread Stefano Stabellini 
On Fri, 5 Oct 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 10/04/2018 10:52 PM, Stefano Stabellini wrote:
> > On Wed, 1 Aug 2018, Jan Beulich wrote:
> > > > > > On 01.08.18 at 01:28,  wrote:
> > > > Today Ctrl-AAA is used to switch between Xen and Dom0. Extend the
> > > > mechanism to allow for switching between Xen, Dom0, and any of the
> > > > initial DomU created from Xen alongside Dom0 out of information provided
> > > > via device tree.
> > > > 
> > > > Rename xen_rx to console_rx to match the new behavior.
> > > > 
> > > > Clarify existing comment about "notify the guest", making it clear that
> > > > it is only about the hardware domain.
> > > > 
> > > > Export a function named console_input_domain() to allow others to know
> > > > which domains has input at a given time.
> > > 
> > > As always in such cases I don't think such functions should be added
> > > without any caller.
> > 
> > I'll add console_input_domain within an #if 0 and remove the #if 0 in
> > the following patch. If you are OK with it, the two patches can be
> > merged on commit (Julien already agreed to it.) They are separate only
> > to make them easier to review.
> 
> Which two patches? I agreed to merge #24 and #22. Not #23. Merging the 3 of
> them is going to make a massive patch which is not going to help understand
> patches after they get merged.

No, you are right, I got confused. That's correct #22 and #24 are the
ones to be merged, I'll add a note about this to the patches. Sorry
about that.


> > 
> > > > @@ -389,30 +392,72 @@ static void dump_console_ring_key(unsigned char
> > > > key)
> > > >   free_xenheap_pages(buf, order);
> > > >   }
> > > >   -/* CTRL- switches input direction between Xen and DOM0.
> > > > */
> > > > +/*
> > > > + * CTRL- switches input direction between Xen, Dom0 and
> > > > + * DomUs.
> > > > + */
> > > >   #define switch_code (opt_conswitch[0]-'a'+1)
> > > > -static int __read_mostly xen_rx = 1; /* FALSE => input passed to domain
> > > > 0. */
> > > > +/*
> > > > + * console_rx=0 => input to xen
> > > > + * console_rx=1 => input to dom0
> > > > + * console_rx=N => input dom(N-1)
> > > > + */
> > > > +static int __read_mostly console_rx = 0;
> > > 
> > > Originally this was supposed to be bool, but didn't get switched yet.
> > > With your current scheme it can't go negative and hence should be
> > > unsigned int. However, I still wonder whether it wouldn't be better
> > > (especially for readers of the code) is this was an actual domid_t.
> > > But as clarified before, I'm not meaning to make this a requirement.
> > 
> > I'll use unsigned int
> > 
> > 
> > > > +struct domain *console_input_domain(void)
> > > > +{
> > > > +return get_domain_by_id(console_rx - 1);
> > > > +}
> > > 
> > > And this is exactly the reason for the above remark: This is (or at
> > > least looks) broken for console_rx == 0.
> > 
> > I'll fix
> > 
> > 
> > > >   static void switch_serial_input(void)
> > > >   {
> > > > -static char *input_str[2] = { "DOM0", "Xen" };
> > > > -xen_rx = !xen_rx;
> > > > -printk("*** Serial input -> %s", input_str[xen_rx]);
> > > > +console_rx++;
> > > > +if ( console_rx == max_init_domid + 2 )
> > > > +console_rx = 0;
> > > 
> > > Same here - the literal 2 at least raises questions. I think it would
> > > at least be a little less confusing if you had
> > > 
> > >  if ( console_rx++ == max_init_domid + 1 )
> > >  console_rx = 0;
> > 
> > I'll do
> > 
> > 
> > > >   static void __serial_rx(char c, struct cpu_user_regs *regs)
> > > >   {
> > > > -if ( xen_rx )
> > > > +if ( console_rx == 0 )
> > > >   return handle_keypress(c, regs);
> > > >   -/* Deliver input to guest buffer, unless it is already full. */
> > > > -if ( (serial_rx_prod-serial_rx_cons) != SERIAL_RX_SIZE )
> > > > -serial_rx_ring[SERIAL_RX_MASK(serial_rx_prod++)] = c;
> > > > -/* Always notify the guest: prevents receive path from getting
> > > > stuck. */
> > > 
> > > Just like you adjust "guest" in this comment, I think you'd better ...
> > > 
> > > > +if ( console_rx == 1 )
> > > > +{
> > > > +/* Deliver input to guest buffer, unless it is already full. */
> > > 
> > > ... adjust it here too.
> > 
> > I'll fix
> > 
> > 
> > > > +if ( (serial_rx_prod - serial_rx_cons) != SERIAL_RX_SIZE )
> > > > +serial_rx_ring[SERIAL_RX_MASK(serial_rx_prod++)] = c;
> > > > +}
> > > > +#ifdef CONFIG_SBSA_VUART_CONSOLE
> > > > +else
> > > > +{
> > > > +struct domain *d = get_domain_by_id(console_rx - 1);
> > > > +
> > > > +/*
> > > > + * If we have a properly initialized vpl011 console for the
> > > > + * domain, without a full PV ring to Dom0 (in that case input
> > > > + * comes from the PV ring), then send the character to it.
> > > > + */
> > > > +if ( !d->arch.vpl011.backend_in_domain && d->arch.vpl011.xen !=
> > > > NULL )
> > > > +vpl011

Re: [Xen-devel] [PATCH v3 23/25] xen: support console_switching between Dom0 and DomUs on ARM

2018-10-05 Thread Stefano Stabellini 
On Fri, 5 Oct 2018, Julien Grall wrote:
> On 10/05/2018 10:25 AM, Julien Grall wrote:
> > On 10/04/2018 10:52 PM, Stefano Stabellini wrote:
> > > On Wed, 1 Aug 2018, Jan Beulich wrote:
> > > > > > > On 01.08.18 at 01:28,  wrote:
> > > > > Today Ctrl-AAA is used to switch between Xen and Dom0. Extend the
> > > > > mechanism to allow for switching between Xen, Dom0, and any of the
> > > > > initial DomU created from Xen alongside Dom0 out of information
> > > > > provided
> > > > > via device tree.
> > > > > 
> > > > > Rename xen_rx to console_rx to match the new behavior.
> > > > > 
> > > > > Clarify existing comment about "notify the guest", making it clear
> > > > > that
> > > > > it is only about the hardware domain.
> > > > > 
> > > > > Export a function named console_input_domain() to allow others to know
> > > > > which domains has input at a given time.
> > > > 
> > > > As always in such cases I don't think such functions should be added
> > > > without any caller.
> > > 
> > > I'll add console_input_domain within an #if 0 and remove the #if 0 in
> > > the following patch. If you are OK with it, the two patches can be
> > > merged on commit (Julien already agreed to it.) They are separate only
> > > to make them easier to review.
> > 
> > Which two patches? I agreed to merge #24 and #22. Not #23. Merging the 3 of
> > them is going to make a massive patch which is not going to help understand
> > patches after they get merged.
> 
> Thinking a bit more. Why does it need to be under #if 0 and then merging the 2
> patches? There are nothing prevent a 2 line function to be moved from one
> patch to another.

I'll move console_input_domain to the following patch (#24).

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

Re: [Xen-devel] [PATCH v3 23/25] xen: support console_switching between Dom0 and DomUs on ARM

2018-10-05 Thread Julien Grall 

On 10/05/2018 10:25 AM, Julien Grall wrote:

On 10/04/2018 10:52 PM, Stefano Stabellini wrote:

On Wed, 1 Aug 2018, Jan Beulich wrote:

On 01.08.18 at 01:28,  wrote:

Today Ctrl-AAA is used to switch between Xen and Dom0. Extend the
mechanism to allow for switching between Xen, Dom0, and any of the
initial DomU created from Xen alongside Dom0 out of information 
provided

via device tree.

Rename xen_rx to console_rx to match the new behavior.

Clarify existing comment about "notify the guest", making it clear that
it is only about the hardware domain.

Export a function named console_input_domain() to allow others to know
which domains has input at a given time.


As always in such cases I don't think such functions should be added
without any caller.


I'll add console_input_domain within an #if 0 and remove the #if 0 in
the following patch. If you are OK with it, the two patches can be
merged on commit (Julien already agreed to it.) They are separate only
to make them easier to review.


Which two patches? I agreed to merge #24 and #22. Not #23. Merging the 3 
of them is going to make a massive patch which is not going to help 
understand patches after they get merged.


Thinking a bit more. Why does it need to be under #if 0 and then merging 
the 2 patches? There are nothing prevent a 2 line function to be moved 
from one patch to another.


Cheers.

--
Julien Grall

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

Re: [Xen-devel] [PATCH v3 23/25] xen: support console_switching between Dom0 and DomUs on ARM

2018-10-05 Thread Julien Grall 

Hi Stefano,

On 10/04/2018 10:52 PM, Stefano Stabellini wrote:

On Wed, 1 Aug 2018, Jan Beulich wrote:

On 01.08.18 at 01:28,  wrote:

Today Ctrl-AAA is used to switch between Xen and Dom0. Extend the
mechanism to allow for switching between Xen, Dom0, and any of the
initial DomU created from Xen alongside Dom0 out of information provided
via device tree.

Rename xen_rx to console_rx to match the new behavior.

Clarify existing comment about "notify the guest", making it clear that
it is only about the hardware domain.

Export a function named console_input_domain() to allow others to know
which domains has input at a given time.


As always in such cases I don't think such functions should be added
without any caller.


I'll add console_input_domain within an #if 0 and remove the #if 0 in
the following patch. If you are OK with it, the two patches can be
merged on commit (Julien already agreed to it.) They are separate only
to make them easier to review.


Which two patches? I agreed to merge #24 and #22. Not #23. Merging the 3 
of them is going to make a massive patch which is not going to help 
understand patches after they get merged.


Cheers,





@@ -389,30 +392,72 @@ static void dump_console_ring_key(unsigned char key)
  free_xenheap_pages(buf, order);
  }
  
-/* CTRL- switches input direction between Xen and DOM0. */

+/*
+ * CTRL- switches input direction between Xen, Dom0 and
+ * DomUs.
+ */
  #define switch_code (opt_conswitch[0]-'a'+1)
-static int __read_mostly xen_rx = 1; /* FALSE => input passed to domain 0. */
+/*
+ * console_rx=0 => input to xen
+ * console_rx=1 => input to dom0
+ * console_rx=N => input dom(N-1)
+ */
+static int __read_mostly console_rx = 0;


Originally this was supposed to be bool, but didn't get switched yet.
With your current scheme it can't go negative and hence should be
unsigned int. However, I still wonder whether it wouldn't be better
(especially for readers of the code) is this was an actual domid_t.
But as clarified before, I'm not meaning to make this a requirement.


I'll use unsigned int



+struct domain *console_input_domain(void)
+{
+return get_domain_by_id(console_rx - 1);
+}


And this is exactly the reason for the above remark: This is (or at
least looks) broken for console_rx == 0.


I'll fix



  static void switch_serial_input(void)
  {
-static char *input_str[2] = { "DOM0", "Xen" };
-xen_rx = !xen_rx;
-printk("*** Serial input -> %s", input_str[xen_rx]);
+console_rx++;
+if ( console_rx == max_init_domid + 2 )
+console_rx = 0;


Same here - the literal 2 at least raises questions. I think it would
at least be a little less confusing if you had

 if ( console_rx++ == max_init_domid + 1 )
 console_rx = 0;


I'll do



  static void __serial_rx(char c, struct cpu_user_regs *regs)
  {
-if ( xen_rx )
+if ( console_rx == 0 )
  return handle_keypress(c, regs);
  
-/* Deliver input to guest buffer, unless it is already full. */

-if ( (serial_rx_prod-serial_rx_cons) != SERIAL_RX_SIZE )
-serial_rx_ring[SERIAL_RX_MASK(serial_rx_prod++)] = c;
-/* Always notify the guest: prevents receive path from getting stuck. */


Just like you adjust "guest" in this comment, I think you'd better ...


+if ( console_rx == 1 )
+{
+/* Deliver input to guest buffer, unless it is already full. */


... adjust it here too.


I'll fix



+if ( (serial_rx_prod - serial_rx_cons) != SERIAL_RX_SIZE )
+serial_rx_ring[SERIAL_RX_MASK(serial_rx_prod++)] = c;
+}
+#ifdef CONFIG_SBSA_VUART_CONSOLE
+else
+{
+struct domain *d = get_domain_by_id(console_rx - 1);
+
+/*
+ * If we have a properly initialized vpl011 console for the
+ * domain, without a full PV ring to Dom0 (in that case input
+ * comes from the PV ring), then send the character to it.
+ */
+if ( !d->arch.vpl011.backend_in_domain && d->arch.vpl011.xen != NULL )
+vpl011_rx_char_xen(d, c);
+else
+printk("Cannot send chars to Dom%d: no UART available\n",
+   d->domain_id);
+}
+#endif
+/*
+ * Always notify the hardware domain: prevents receive path from
+ * getting stuck.
+ */
  send_global_virq(VIRQ_CONSOLE);


Why does this not move into the if() body above?


That was a mistake, I fixed it



@@ -923,7 +968,7 @@ void __init console_endboot(void)
   * a useful 'how to switch' message.
   */
  if ( opt_conswitch[1] == 'x' )
-xen_rx = !xen_rx;
+console_rx = 0;


According to the comment I think you need to store
max_init_domid + 1 here, so that the switch_serial_input() a few
lines down would actually switch to Xen.


I'll fix

Thanks for the comments!



--
Julien Grall

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

Re: [Xen-devel] [PATCH v3 23/25] xen: support console_switching between Dom0 and DomUs on ARM

2018-10-04 Thread Stefano Stabellini 
On Wed, 1 Aug 2018, Jan Beulich wrote:
> >>> On 01.08.18 at 01:28,  wrote:
> > Today Ctrl-AAA is used to switch between Xen and Dom0. Extend the
> > mechanism to allow for switching between Xen, Dom0, and any of the
> > initial DomU created from Xen alongside Dom0 out of information provided
> > via device tree.
> > 
> > Rename xen_rx to console_rx to match the new behavior.
> > 
> > Clarify existing comment about "notify the guest", making it clear that
> > it is only about the hardware domain.
> > 
> > Export a function named console_input_domain() to allow others to know
> > which domains has input at a given time.
> 
> As always in such cases I don't think such functions should be added
> without any caller.

I'll add console_input_domain within an #if 0 and remove the #if 0 in
the following patch. If you are OK with it, the two patches can be
merged on commit (Julien already agreed to it.) They are separate only
to make them easier to review.


> > @@ -389,30 +392,72 @@ static void dump_console_ring_key(unsigned char key)
> >  free_xenheap_pages(buf, order);
> >  }
> >  
> > -/* CTRL- switches input direction between Xen and DOM0. */
> > +/*
> > + * CTRL- switches input direction between Xen, Dom0 and
> > + * DomUs.
> > + */
> >  #define switch_code (opt_conswitch[0]-'a'+1)
> > -static int __read_mostly xen_rx = 1; /* FALSE => input passed to domain 0. 
> > */
> > +/*
> > + * console_rx=0 => input to xen
> > + * console_rx=1 => input to dom0
> > + * console_rx=N => input dom(N-1)
> > + */
> > +static int __read_mostly console_rx = 0;
> 
> Originally this was supposed to be bool, but didn't get switched yet.
> With your current scheme it can't go negative and hence should be
> unsigned int. However, I still wonder whether it wouldn't be better
> (especially for readers of the code) is this was an actual domid_t.
> But as clarified before, I'm not meaning to make this a requirement.

I'll use unsigned int


> > +struct domain *console_input_domain(void)
> > +{
> > +return get_domain_by_id(console_rx - 1);
> > +}
> 
> And this is exactly the reason for the above remark: This is (or at
> least looks) broken for console_rx == 0.

I'll fix


> >  static void switch_serial_input(void)
> >  {
> > -static char *input_str[2] = { "DOM0", "Xen" };
> > -xen_rx = !xen_rx;
> > -printk("*** Serial input -> %s", input_str[xen_rx]);
> > +console_rx++;
> > +if ( console_rx == max_init_domid + 2 )
> > +console_rx = 0;
> 
> Same here - the literal 2 at least raises questions. I think it would
> at least be a little less confusing if you had
> 
> if ( console_rx++ == max_init_domid + 1 )
> console_rx = 0;

I'll do


> >  static void __serial_rx(char c, struct cpu_user_regs *regs)
> >  {
> > -if ( xen_rx )
> > +if ( console_rx == 0 )
> >  return handle_keypress(c, regs);
> >  
> > -/* Deliver input to guest buffer, unless it is already full. */
> > -if ( (serial_rx_prod-serial_rx_cons) != SERIAL_RX_SIZE )
> > -serial_rx_ring[SERIAL_RX_MASK(serial_rx_prod++)] = c;
> > -/* Always notify the guest: prevents receive path from getting stuck. 
> > */
> 
> Just like you adjust "guest" in this comment, I think you'd better ...
> 
> > +if ( console_rx == 1 )
> > +{
> > +/* Deliver input to guest buffer, unless it is already full. */
> 
> ... adjust it here too.

I'll fix


> > +if ( (serial_rx_prod - serial_rx_cons) != SERIAL_RX_SIZE )
> > +serial_rx_ring[SERIAL_RX_MASK(serial_rx_prod++)] = c;
> > +}
> > +#ifdef CONFIG_SBSA_VUART_CONSOLE
> > +else
> > +{
> > +struct domain *d = get_domain_by_id(console_rx - 1);
> > +
> > +/*
> > + * If we have a properly initialized vpl011 console for the
> > + * domain, without a full PV ring to Dom0 (in that case input
> > + * comes from the PV ring), then send the character to it.
> > + */
> > +if ( !d->arch.vpl011.backend_in_domain && d->arch.vpl011.xen != 
> > NULL )
> > +vpl011_rx_char_xen(d, c);
> > +else
> > +printk("Cannot send chars to Dom%d: no UART available\n",
> > +   d->domain_id);
> > +}
> > +#endif
> > +/*
> > + * Always notify the hardware domain: prevents receive path from
> > + * getting stuck.
> > + */
> >  send_global_virq(VIRQ_CONSOLE);
> 
> Why does this not move into the if() body above?

That was a mistake, I fixed it


> > @@ -923,7 +968,7 @@ void __init console_endboot(void)
> >   * a useful 'how to switch' message.
> >   */
> >  if ( opt_conswitch[1] == 'x' )
> > -xen_rx = !xen_rx;
> > +console_rx = 0;
> 
> According to the comment I think you need to store
> max_init_domid + 1 here, so that the switch_serial_input() a few
> lines down would actually switch to Xen.

I'll fix

Thanks for the comments!

___
Xen-devel mailing list
Xen-devel@lists.x

Re: [Xen-devel] [PATCH v3 23/25] xen: support console_switching between Dom0 and DomUs on ARM

2018-08-16 Thread Stefano Stabellini 
On Mon, 13 Aug 2018, Julien Grall wrote:
> Hi Stefano,
> 
> OOI, on the previous version you said you will explore the CTRL-x N solution
> (where N is the domID console to switch too). What was the result here?

I meant I'll explore it as a follow-up to this series. I haven't looked
into it yet, but it is in my todo.


> On 01/08/18 00:28, Stefano Stabellini wrote:
> > Today Ctrl-AAA is used to switch between Xen and Dom0. Extend the
> > mechanism to allow for switching between Xen, Dom0, and any of the
> > initial DomU created from Xen alongside Dom0 out of information provided
> > via device tree.
> > 
> > Rename xen_rx to console_rx to match the new behavior.
> > 
> > Clarify existing comment about "notify the guest", making it clear that
> > it is only about the hardware domain.
> > 
> > Export a function named console_input_domain() to allow others to know
> > which domains has input at a given time.
> > 
> > Signed-off-by: Stefano Stabellini 
> > CC: andrew.coop...@citrix.com
> > CC: george.dun...@eu.citrix.com
> > CC: ian.jack...@eu.citrix.com
> > CC: jbeul...@suse.com
> > CC: konrad.w...@oracle.com
> > CC: t...@xen.org
> > CC: wei.l...@citrix.com
> > ---
> > Changes in v3:
> > - only call vpl011 functions ifdef CONFIG_SBSA_VUART_CONSOLE
> > - add blank line and spaces
> > - remove xen_rx from print messages
> > - rename xen_rx to console_rx
> > - keep switch_serial_input() at boot
> > - add better comments
> > - fix existing comment
> > - add warning if no guests console/uart is available
> > - add console_input_domain function
> > 
> > Changes in v2:
> > - only call vpl011_rx_char if the vpl011 has been initialized
> > ---
> >   xen/drivers/char/console.c | 71
> > +-
> >   xen/include/xen/console.h  |  2 ++
> >   2 files changed, 60 insertions(+), 13 deletions(-)
> > 
> > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> > index 0f05369..cd4dfb1 100644
> > --- a/xen/drivers/char/console.c
> > +++ b/xen/drivers/char/console.c
> > @@ -31,10 +31,13 @@
> >   #include 
> >   #include 
> >   #include 
> > +#include 
> > #ifdef CONFIG_X86
> >   #include 
> >   #include 
> > +#else
> > +#include 
> >   #endif
> > /* console: comma-separated list of console outputs. */
> > @@ -389,30 +392,72 @@ static void dump_console_ring_key(unsigned char key)
> >   free_xenheap_pages(buf, order);
> >   }
> >   -/* CTRL- switches input direction between Xen and DOM0. */
> > +/*
> > + * CTRL- switches input direction between Xen, Dom0 and
> > + * DomUs.
> > + */
> >   #define switch_code (opt_conswitch[0]-'a'+1)
> > -static int __read_mostly xen_rx = 1; /* FALSE => input passed to domain 0.
> > */
> > +/*
> > + * console_rx=0 => input to xen
> > + * console_rx=1 => input to dom0
> > + * console_rx=N => input dom(N-1)
> > + */
> > +static int __read_mostly console_rx = 0;
> > +
> > +struct domain *console_input_domain(void)
> > +{
> > +return get_domain_by_id(console_rx - 1);
> 
> Please take care of the case where console_rx == 0.

I'll do


> > +}
> > static void switch_serial_input(void)
> >   {
> > -static char *input_str[2] = { "DOM0", "Xen" };
> > -xen_rx = !xen_rx;
> > -printk("*** Serial input -> %s", input_str[xen_rx]);
> > +console_rx++;
> > +if ( console_rx == max_init_domid + 2 )
> > +console_rx = 0;
> > +
> > +if ( !console_rx )
> > +printk("*** Serial input to Xen");
> > +else
> > +printk("*** Serial input to DOM%d", console_rx - 1);
> > +
> >   if ( switch_code )
> > -printk(" (type 'CTRL-%c' three times to switch input to %s)",
> > -   opt_conswitch[0], input_str[!xen_rx]);
> > +printk(" (type 'CTRL-%c' three times to switch input)",
> > +   opt_conswitch[0]);
> >   printk("\n");
> >   }
> > static void __serial_rx(char c, struct cpu_user_regs *regs)
> >   {
> > -if ( xen_rx )
> > +if ( console_rx == 0 )
> >   return handle_keypress(c, regs);
> >   -/* Deliver input to guest buffer, unless it is already full. */
> > -if ( (serial_rx_prod-serial_rx_cons) != SERIAL_RX_SIZE )
> > -serial_rx_ring[SERIAL_RX_MASK(serial_rx_prod++)] = c;
> > -/* Always notify the guest: prevents receive path from getting stuck.
> > */
> > +if ( console_rx == 1 )
> > +{
> > +/* Deliver input to guest buffer, unless it is already full. */
> > +if ( (serial_rx_prod - serial_rx_cons) != SERIAL_RX_SIZE )
> > +serial_rx_ring[SERIAL_RX_MASK(serial_rx_prod++)] = c;
> > +}
> > +#ifdef CONFIG_SBSA_VUART_CONSOLE
> > +else
> > +{
> > +struct domain *d = get_domain_by_id(console_rx - 1);
> 
> I don't think this is correct to assume the domain will always be present.
> With this series, it would be possible to retire a domain and therefore
> get_domain_by_id() would return NULL here. This would result to a data abort
> below.

Well, spotted! I'll fix.


> Also, I thin

Re: [Xen-devel] [PATCH v3 23/25] xen: support console_switching between Dom0 and DomUs on ARM

2018-08-13 Thread Julien Grall 

Hi Stefano,

OOI, on the previous version you said you will explore the CTRL-x N 
solution (where N is the domID console to switch too). What was the 
result here?


On 01/08/18 00:28, Stefano Stabellini wrote:

Today Ctrl-AAA is used to switch between Xen and Dom0. Extend the
mechanism to allow for switching between Xen, Dom0, and any of the
initial DomU created from Xen alongside Dom0 out of information provided
via device tree.

Rename xen_rx to console_rx to match the new behavior.

Clarify existing comment about "notify the guest", making it clear that
it is only about the hardware domain.

Export a function named console_input_domain() to allow others to know
which domains has input at a given time.

Signed-off-by: Stefano Stabellini 
CC: andrew.coop...@citrix.com
CC: george.dun...@eu.citrix.com
CC: ian.jack...@eu.citrix.com
CC: jbeul...@suse.com
CC: konrad.w...@oracle.com
CC: t...@xen.org
CC: wei.l...@citrix.com
---
Changes in v3:
- only call vpl011 functions ifdef CONFIG_SBSA_VUART_CONSOLE
- add blank line and spaces
- remove xen_rx from print messages
- rename xen_rx to console_rx
- keep switch_serial_input() at boot
- add better comments
- fix existing comment
- add warning if no guests console/uart is available
- add console_input_domain function

Changes in v2:
- only call vpl011_rx_char if the vpl011 has been initialized
---
  xen/drivers/char/console.c | 71 +-
  xen/include/xen/console.h  |  2 ++
  2 files changed, 60 insertions(+), 13 deletions(-)

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 0f05369..cd4dfb1 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -31,10 +31,13 @@
  #include 
  #include 
  #include 
+#include 
  
  #ifdef CONFIG_X86

  #include 
  #include 
+#else
+#include 
  #endif
  
  /* console: comma-separated list of console outputs. */

@@ -389,30 +392,72 @@ static void dump_console_ring_key(unsigned char key)
  free_xenheap_pages(buf, order);
  }
  
-/* CTRL- switches input direction between Xen and DOM0. */

+/*
+ * CTRL- switches input direction between Xen, Dom0 and
+ * DomUs.
+ */
  #define switch_code (opt_conswitch[0]-'a'+1)
-static int __read_mostly xen_rx = 1; /* FALSE => input passed to domain 0. */
+/*
+ * console_rx=0 => input to xen
+ * console_rx=1 => input to dom0
+ * console_rx=N => input dom(N-1)
+ */
+static int __read_mostly console_rx = 0;
+
+struct domain *console_input_domain(void)
+{
+return get_domain_by_id(console_rx - 1);


Please take care of the case where console_rx == 0.


+}
  
  static void switch_serial_input(void)

  {
-static char *input_str[2] = { "DOM0", "Xen" };
-xen_rx = !xen_rx;
-printk("*** Serial input -> %s", input_str[xen_rx]);
+console_rx++;
+if ( console_rx == max_init_domid + 2 )
+console_rx = 0;
+
+if ( !console_rx )
+printk("*** Serial input to Xen");
+else
+printk("*** Serial input to DOM%d", console_rx - 1);
+
  if ( switch_code )
-printk(" (type 'CTRL-%c' three times to switch input to %s)",
-   opt_conswitch[0], input_str[!xen_rx]);
+printk(" (type 'CTRL-%c' three times to switch input)",
+   opt_conswitch[0]);
  printk("\n");
  }
  
  static void __serial_rx(char c, struct cpu_user_regs *regs)

  {
-if ( xen_rx )
+if ( console_rx == 0 )
  return handle_keypress(c, regs);
  
-/* Deliver input to guest buffer, unless it is already full. */

-if ( (serial_rx_prod-serial_rx_cons) != SERIAL_RX_SIZE )
-serial_rx_ring[SERIAL_RX_MASK(serial_rx_prod++)] = c;
-/* Always notify the guest: prevents receive path from getting stuck. */
+if ( console_rx == 1 )
+{
+/* Deliver input to guest buffer, unless it is already full. */
+if ( (serial_rx_prod - serial_rx_cons) != SERIAL_RX_SIZE )
+serial_rx_ring[SERIAL_RX_MASK(serial_rx_prod++)] = c;
+}
+#ifdef CONFIG_SBSA_VUART_CONSOLE
+else
+{
+struct domain *d = get_domain_by_id(console_rx - 1);


I don't think this is correct to assume the domain will always be 
present. With this series, it would be possible to retire a domain and 
therefore get_domain_by_id() would return NULL here. This would result 
to a data abort below.


Also, I think you want to use rcu_lock_by_domain here (I am not 100% 
sure though).



+
+/*
+ * If we have a properly initialized vpl011 console for the
+ * domain, without a full PV ring to Dom0 (in that case input
+ * comes from the PV ring), then send the character to it.
+ */
+if ( !d->arch.vpl011.backend_in_domain && d->arch.vpl011.xen != NULL )
+vpl011_rx_char_xen(d, c);
+else
+printk("Cannot send chars to Dom%d: no UART available\n",
+   d->domain_id);
+}
+#endif
+/*
+ * Always notify the hardware domain: prevents receive path from


That's not true. If you l

Re: [Xen-devel] [PATCH v3 23/25] xen: support console_switching between Dom0 and DomUs on ARM

2018-08-01 Thread Jan Beulich 
>>> On 01.08.18 at 01:28,  wrote:
> Today Ctrl-AAA is used to switch between Xen and Dom0. Extend the
> mechanism to allow for switching between Xen, Dom0, and any of the
> initial DomU created from Xen alongside Dom0 out of information provided
> via device tree.
> 
> Rename xen_rx to console_rx to match the new behavior.
> 
> Clarify existing comment about "notify the guest", making it clear that
> it is only about the hardware domain.
> 
> Export a function named console_input_domain() to allow others to know
> which domains has input at a given time.

As always in such cases I don't think such functions should be added
without any caller.

> @@ -389,30 +392,72 @@ static void dump_console_ring_key(unsigned char key)
>  free_xenheap_pages(buf, order);
>  }
>  
> -/* CTRL- switches input direction between Xen and DOM0. */
> +/*
> + * CTRL- switches input direction between Xen, Dom0 and
> + * DomUs.
> + */
>  #define switch_code (opt_conswitch[0]-'a'+1)
> -static int __read_mostly xen_rx = 1; /* FALSE => input passed to domain 0. */
> +/*
> + * console_rx=0 => input to xen
> + * console_rx=1 => input to dom0
> + * console_rx=N => input dom(N-1)
> + */
> +static int __read_mostly console_rx = 0;

Originally this was supposed to be bool, but didn't get switched yet.
With your current scheme it can't go negative and hence should be
unsigned int. However, I still wonder whether it wouldn't be better
(especially for readers of the code) is this was an actual domid_t.
But as clarified before, I'm not meaning to make this a requirement.

> +struct domain *console_input_domain(void)
> +{
> +return get_domain_by_id(console_rx - 1);
> +}

And this is exactly the reason for the above remark: This is (or at
least looks) broken for console_rx == 0.

>  static void switch_serial_input(void)
>  {
> -static char *input_str[2] = { "DOM0", "Xen" };
> -xen_rx = !xen_rx;
> -printk("*** Serial input -> %s", input_str[xen_rx]);
> +console_rx++;
> +if ( console_rx == max_init_domid + 2 )
> +console_rx = 0;

Same here - the literal 2 at least raises questions. I think it would
at least be a little less confusing if you had

if ( console_rx++ == max_init_domid + 1 )
console_rx = 0;

>  static void __serial_rx(char c, struct cpu_user_regs *regs)
>  {
> -if ( xen_rx )
> +if ( console_rx == 0 )
>  return handle_keypress(c, regs);
>  
> -/* Deliver input to guest buffer, unless it is already full. */
> -if ( (serial_rx_prod-serial_rx_cons) != SERIAL_RX_SIZE )
> -serial_rx_ring[SERIAL_RX_MASK(serial_rx_prod++)] = c;
> -/* Always notify the guest: prevents receive path from getting stuck. */

Just like you adjust "guest" in this comment, I think you'd better ...

> +if ( console_rx == 1 )
> +{
> +/* Deliver input to guest buffer, unless it is already full. */

... adjust it here too.

> +if ( (serial_rx_prod - serial_rx_cons) != SERIAL_RX_SIZE )
> +serial_rx_ring[SERIAL_RX_MASK(serial_rx_prod++)] = c;
> +}
> +#ifdef CONFIG_SBSA_VUART_CONSOLE
> +else
> +{
> +struct domain *d = get_domain_by_id(console_rx - 1);
> +
> +/*
> + * If we have a properly initialized vpl011 console for the
> + * domain, without a full PV ring to Dom0 (in that case input
> + * comes from the PV ring), then send the character to it.
> + */
> +if ( !d->arch.vpl011.backend_in_domain && d->arch.vpl011.xen != NULL 
> )
> +vpl011_rx_char_xen(d, c);
> +else
> +printk("Cannot send chars to Dom%d: no UART available\n",
> +   d->domain_id);
> +}
> +#endif
> +/*
> + * Always notify the hardware domain: prevents receive path from
> + * getting stuck.
> + */
>  send_global_virq(VIRQ_CONSOLE);

Why does this not move into the if() body above?

> @@ -923,7 +968,7 @@ void __init console_endboot(void)
>   * a useful 'how to switch' message.
>   */
>  if ( opt_conswitch[1] == 'x' )
> -xen_rx = !xen_rx;
> +console_rx = 0;

According to the comment I think you need to store
max_init_domid + 1 here, so that the switch_serial_input() a few
lines down would actually switch to Xen.

Jan


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

[Xen-devel] [PATCH v3 23/25] xen: support console_switching between Dom0 and DomUs on ARM

2018-07-31 Thread Stefano Stabellini 
Today Ctrl-AAA is used to switch between Xen and Dom0. Extend the
mechanism to allow for switching between Xen, Dom0, and any of the
initial DomU created from Xen alongside Dom0 out of information provided
via device tree.

Rename xen_rx to console_rx to match the new behavior.

Clarify existing comment about "notify the guest", making it clear that
it is only about the hardware domain.

Export a function named console_input_domain() to allow others to know
which domains has input at a given time.

Signed-off-by: Stefano Stabellini 
CC: andrew.coop...@citrix.com
CC: george.dun...@eu.citrix.com
CC: ian.jack...@eu.citrix.com
CC: jbeul...@suse.com
CC: konrad.w...@oracle.com
CC: t...@xen.org
CC: wei.l...@citrix.com
---
Changes in v3:
- only call vpl011 functions ifdef CONFIG_SBSA_VUART_CONSOLE
- add blank line and spaces
- remove xen_rx from print messages
- rename xen_rx to console_rx
- keep switch_serial_input() at boot
- add better comments
- fix existing comment
- add warning if no guests console/uart is available
- add console_input_domain function

Changes in v2:
- only call vpl011_rx_char if the vpl011 has been initialized
---
 xen/drivers/char/console.c | 71 +-
 xen/include/xen/console.h  |  2 ++
 2 files changed, 60 insertions(+), 13 deletions(-)

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 0f05369..cd4dfb1 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -31,10 +31,13 @@
 #include 
 #include 
 #include 
+#include 
 
 #ifdef CONFIG_X86
 #include 
 #include 
+#else
+#include 
 #endif
 
 /* console: comma-separated list of console outputs. */
@@ -389,30 +392,72 @@ static void dump_console_ring_key(unsigned char key)
 free_xenheap_pages(buf, order);
 }
 
-/* CTRL- switches input direction between Xen and DOM0. */
+/*
+ * CTRL- switches input direction between Xen, Dom0 and
+ * DomUs.
+ */
 #define switch_code (opt_conswitch[0]-'a'+1)
-static int __read_mostly xen_rx = 1; /* FALSE => input passed to domain 0. */
+/*
+ * console_rx=0 => input to xen
+ * console_rx=1 => input to dom0
+ * console_rx=N => input dom(N-1)
+ */
+static int __read_mostly console_rx = 0;
+
+struct domain *console_input_domain(void)
+{
+return get_domain_by_id(console_rx - 1);
+}
 
 static void switch_serial_input(void)
 {
-static char *input_str[2] = { "DOM0", "Xen" };
-xen_rx = !xen_rx;
-printk("*** Serial input -> %s", input_str[xen_rx]);
+console_rx++;
+if ( console_rx == max_init_domid + 2 )
+console_rx = 0;
+
+if ( !console_rx )
+printk("*** Serial input to Xen");
+else
+printk("*** Serial input to DOM%d", console_rx - 1);
+
 if ( switch_code )
-printk(" (type 'CTRL-%c' three times to switch input to %s)",
-   opt_conswitch[0], input_str[!xen_rx]);
+printk(" (type 'CTRL-%c' three times to switch input)",
+   opt_conswitch[0]);
 printk("\n");
 }
 
 static void __serial_rx(char c, struct cpu_user_regs *regs)
 {
-if ( xen_rx )
+if ( console_rx == 0 )
 return handle_keypress(c, regs);
 
-/* Deliver input to guest buffer, unless it is already full. */
-if ( (serial_rx_prod-serial_rx_cons) != SERIAL_RX_SIZE )
-serial_rx_ring[SERIAL_RX_MASK(serial_rx_prod++)] = c;
-/* Always notify the guest: prevents receive path from getting stuck. */
+if ( console_rx == 1 )
+{
+/* Deliver input to guest buffer, unless it is already full. */
+if ( (serial_rx_prod - serial_rx_cons) != SERIAL_RX_SIZE )
+serial_rx_ring[SERIAL_RX_MASK(serial_rx_prod++)] = c;
+}
+#ifdef CONFIG_SBSA_VUART_CONSOLE
+else
+{
+struct domain *d = get_domain_by_id(console_rx - 1);
+
+/*
+ * If we have a properly initialized vpl011 console for the
+ * domain, without a full PV ring to Dom0 (in that case input
+ * comes from the PV ring), then send the character to it.
+ */
+if ( !d->arch.vpl011.backend_in_domain && d->arch.vpl011.xen != NULL )
+vpl011_rx_char_xen(d, c);
+else
+printk("Cannot send chars to Dom%d: no UART available\n",
+   d->domain_id);
+}
+#endif
+/*
+ * Always notify the hardware domain: prevents receive path from
+ * getting stuck.
+ */
 send_global_virq(VIRQ_CONSOLE);
 
 #ifdef CONFIG_X86
@@ -923,7 +968,7 @@ void __init console_endboot(void)
  * a useful 'how to switch' message.
  */
 if ( opt_conswitch[1] == 'x' )
-xen_rx = !xen_rx;
+console_rx = 0;
 
 register_keyhandler('w', dump_console_ring_key,
 "synchronously dump console ring buffer (dmesg)", 0);
diff --git a/xen/include/xen/console.h b/xen/include/xen/console.h
index ea06fd8..2fe3912 100644
--- a/xen/include/xen/console.h
+++ b/xen/include/xen/console.h
@@ -31,6 +31,8 @@ void console_end_sync(void);
 void console_start_log_everyt