[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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, <sstabellini@xxxxxxxxxx> 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-<switch_char> switches input direction between Xen and DOM0. */
+/*
+ * CTRL-<switch_char> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.