From: Denis Mukhin <dmukhin@ford.com>

There are few places which check pv_shim console under CONFIG_PV_SHIM or
CONFIG_X86 in xen console driver.

Instead of inconsistent #ifdef-ing, introduce and use consoled_is_enabled() in
switch_serial_input() and __serial_rx().

PV shim case is fixed in __serial_rx() - should be under 'pv_shim &&
pv_console' check.

Signature of consoled_guest_{rx,tx} has changed so the errors can be logged
on the callsites.

Also, move get_initial_domain_id() to arch-independent header since it is now
required by console driver.

Lastly, add missing SPDX-License-Identifier to xen/consoled.h

Signed-off-by: Denis Mukhin <dmukhin@ford.com>
---
Changes v4->v5:
- Dropped consoled_guest_tx() stub under !CONFIG_PV_SHIM case
- Link to CI: https://gitlab.com/xen-project/people/dmukhin/xen/-/pipelines/1702411426
---
 xen/arch/x86/include/asm/pv/shim.h |  5 +++--
 xen/arch/x86/pv/shim.c             |  8 ++------
 xen/common/domain.c                |  9 +++++++++
 xen/drivers/char/console.c         | 15 ++++++---------
 xen/drivers/char/consoled.c        | 17 +++++++++++++----
 xen/include/xen/consoled.h         | 15 +++++++++++++--
 xen/include/xen/domain.h           |  2 ++
 7 files changed, 48 insertions(+), 23 deletions(-)

diff --git a/xen/arch/x86/include/asm/pv/shim.h b/xen/arch/x86/include/asm/pv/shim.h
index 6153e27005..27053d4f6f 100644
--- a/xen/arch/x86/include/asm/pv/shim.h
+++ b/xen/arch/x86/include/asm/pv/shim.h
@@ -31,7 +31,7 @@ long cf_check pv_shim_cpu_up(void *data);
 long cf_check pv_shim_cpu_down(void *data);
 void pv_shim_online_memory(unsigned int nr, unsigned int order);
 void pv_shim_offline_memory(unsigned int nr, unsigned int order);
-domid_t get_initial_domain_id(void);
+domid_t pv_shim_get_initial_domain_id(void);
 uint64_t pv_shim_mem(uint64_t avail);
 void pv_shim_fixup_e820(void);
 const struct platform_bad_page *pv_shim_reserved_pages(unsigned int *size);
@@ -76,8 +76,9 @@ static inline void pv_shim_offline_memory(unsigned int nr, unsigned int order)
 {
     ASSERT_UNREACHABLE();
 }
-static inline domid_t get_initial_domain_id(void)
+static inline domid_t pv_shim_get_initial_domain_id(void)
 {
+    ASSERT_UNREACHABLE();
     return 0;
 }
 static inline uint64_t pv_shim_mem(uint64_t avail)
diff --git a/xen/arch/x86/pv/shim.c b/xen/arch/x86/pv/shim.c
index 81e4a0516d..c506cc0bec 100644
--- a/xen/arch/x86/pv/shim.c
+++ b/xen/arch/x86/pv/shim.c
@@ -603,8 +603,7 @@ long pv_shim_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 
         if ( pv_console && send.port == pv_console_evtchn() )
         {
-            consoled_guest_rx();
-            rc = 0;
+            rc = consoled_guest_rx();
         }
         else
             rc = xen_hypercall_event_channel_op(EVTCHNOP_send, &send);
@@ -1016,13 +1015,10 @@ void pv_shim_offline_memory(unsigned int nr, unsigned int order)
     }
 }
 
-domid_t get_initial_domain_id(void)
+domid_t pv_shim_get_initial_domain_id(void)
 {
     uint32_t eax, ebx, ecx, edx;
 
-    if ( !pv_shim )
-        return 0;
-
     cpuid(xen_cpuid_base + 4, &eax, &ebx, &ecx, &edx);
 
     return (eax & XEN_HVM_CPUID_DOMID_PRESENT) ? ecx : 1;
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 0c4cc77111..163663a051 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -2259,6 +2259,15 @@ int continue_hypercall_on_cpu(
     return 0;
 }
 
+domid_t get_initial_domain_id(void)
+{
+#ifdef CONFIG_X86
+    if ( pv_shim )
+        return pv_shim_get_initial_domain_id();
+#endif
+    return hardware_domid;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 2f028c5d44..ba428199d2 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -32,9 +32,9 @@
 #include <xen/pv_console.h>
 #include <asm/setup.h>
 #include <xen/sections.h>
+#include <xen/consoled.h>
 
 #ifdef CONFIG_X86
-#include <xen/consoled.h>
 #include <asm/guest.h>
 #endif
 #ifdef CONFIG_SBSA_VUART_CONSOLE
@@ -507,11 +507,9 @@ static void switch_serial_input(void)
             break;
         }
 
-#ifdef CONFIG_PV_SHIM
-        if ( next_rx == 1 )
+        if ( consoled_is_enabled() && next_rx == 1 )
             domid = get_initial_domain_id();
         else
-#endif
             domid = next_rx - 1;
         d = rcu_lock_domain_by_id(domid);
         if ( d )
@@ -562,16 +560,15 @@ static void __serial_rx(char c)
         rc = vpl011_rx_char_xen(d, c);
 #endif
 
+    if ( consoled_is_enabled() )
+        /* Deliver input to the PV shim console. */
+        rc = consoled_guest_tx(c);
+
     if ( rc )
         guest_printk(d,
                      XENLOG_WARNING "failed to process console input: %d\n",
                      rc);
 
-#ifdef CONFIG_X86
-    if ( pv_shim && pv_console )
-        consoled_guest_tx(c);
-#endif
-
     console_put_domain(d);
 }
 
diff --git a/xen/drivers/char/consoled.c b/xen/drivers/char/consoled.c
index b415b632ce..8704ec251e 100644
--- a/xen/drivers/char/consoled.c
+++ b/xen/drivers/char/consoled.c
@@ -43,13 +43,13 @@ struct xencons_interface *consoled_get_ring_addr(void)
 static char buf[BUF_SZ + 1];
 
 /* Receives characters from a domain's PV console */
-void consoled_guest_rx(void)
+int consoled_guest_rx(void)
 {
     size_t idx = 0;
     XENCONS_RING_IDX cons, prod;
 
     if ( !cons_ring )
-        return;
+        return -ENODEV;
 
     spin_lock(&rx_lock);
 
@@ -91,15 +91,17 @@ void consoled_guest_rx(void)
 
  out:
     spin_unlock(&rx_lock);
+
+    return 0;
 }
 
 /* Sends a character into a domain's PV console */
-void consoled_guest_tx(char c)
+int consoled_guest_tx(char c)
 {
     XENCONS_RING_IDX cons, prod;
 
     if ( !cons_ring )
-        return;
+        return -ENODEV;
 
     cons = ACCESS_ONCE(cons_ring->in_cons);
     prod = cons_ring->in_prod;
@@ -125,6 +127,13 @@ void consoled_guest_tx(char c)
  notify:
     /* Always notify the guest: prevents receive path from getting stuck. */
     pv_shim_inject_evtchn(pv_console_evtchn());
+
+    return 0;
+}
+
+bool consoled_is_enabled(void)
+{
+    return pv_shim && pv_console;
 }
 
 /*
diff --git a/xen/include/xen/consoled.h b/xen/include/xen/consoled.h
index bd7ab6329e..195b9a6c23 100644
--- a/xen/include/xen/consoled.h
+++ b/xen/include/xen/consoled.h
@@ -1,12 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
 #ifndef __XEN_CONSOLED_H__
 #define __XEN_CONSOLED_H__
 
 #include <public/io/console.h>
 
+int consoled_guest_tx(char c);
+
+#ifdef CONFIG_PV_SHIM
+
 void consoled_set_ring_addr(struct xencons_interface *ring);
 struct xencons_interface *consoled_get_ring_addr(void);
-void consoled_guest_rx(void);
-void consoled_guest_tx(char c);
+int consoled_guest_rx(void);
+bool consoled_is_enabled(void);
+
+#else
+
+#define consoled_is_enabled()   (false)
+
+#endif /* CONFIG_PV_SHIM */
 
 #endif /* __XEN_CONSOLED_H__ */
 /*
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index 3de5635291..83069de501 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -35,6 +35,8 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info);
 void arch_get_domain_info(const struct domain *d,
                           struct xen_domctl_getdomaininfo *info);
 
+domid_t get_initial_domain_id(void);
+
 /* CDF_* constant. Internal flags for domain creation. */
 /* Is this a privileged domain? */
 #define CDF_privileged           (1U << 0)
-- 
2.34.1

On 06.03.2025 08:59, dmkhn@proton.me wrote:> --- a/xen/include/xen/consoled.h
> +++ b/xen/include/xen/consoled.h
> @@ -1,12 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
>  #ifndef __XEN_CONSOLED_H__
>  #define __XEN_CONSOLED_H__
>  
>  #include <public/io/console.h>
>  
> +int consoled_guest_tx(char c);
> +
> +#ifdef CONFIG_PV_SHIM
> +
>  void consoled_set_ring_addr(struct xencons_interface *ring);
>  struct xencons_interface *consoled_get_ring_addr(void);
> -void consoled_guest_rx(void);
> -void consoled_guest_tx(char c);
> +int consoled_guest_rx(void);
> +bool consoled_is_enabled(void);
> +
> +#else
> +
> +#define consoled_is_enabled()   (false)
> +
> +#endif /* CONFIG_PV_SHIM */

I'm sorry to be picky, but why did you move just consoled_guest_tx() out
of the conditional? Once something needs moving out, imo everything that
doesn't strictly need to be there wants to move out as well. Which would
leave just consoled_is_enabled() there. Then
Acked-by: Jan Beulich <jbeulich@suse.com>
I can certainly make the adjustment while committing, if no other need
for a v6 arises.

Jan

On Thursday, March 6th, 2025 at 8:35 AM, Jan Beulich <jbeulich@suse.com> wrote:

> 
> 
> On 06.03.2025 08:59, dmkhn@proton.me wrote:> --- a/xen/include/xen/consoled.h
> 
> > +++ b/xen/include/xen/consoled.h
> > @@ -1,12 +1,23 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > #ifndef XEN_CONSOLED_H
> > #define XEN_CONSOLED_H
> > 
> > #include <public/io/console.h>
> > 
> > +int consoled_guest_tx(char c);
> > +
> > +#ifdef CONFIG_PV_SHIM
> > +
> > void consoled_set_ring_addr(struct xencons_interface *ring);
> > struct xencons_interface consoled_get_ring_addr(void);
> > -void consoled_guest_rx(void);
> > -void consoled_guest_tx(char c);
> > +int consoled_guest_rx(void);
> > +bool consoled_is_enabled(void);
> > +
> > +#else
> > +
> > +#define consoled_is_enabled() (false)
> > +
> > +#endif / CONFIG_PV_SHIM */
> 
> 
> I'm sorry to be picky, but why did you move just consoled_guest_tx() out
> of the conditional? Once something needs moving out, imo everything that
> doesn't strictly need to be there wants to move out as well. Which would
> leave just consoled_is_enabled() there. Then
> Acked-by: Jan Beulich jbeulich@suse.com
> 
> I can certainly make the adjustment while committing, if no other need
> for a v6 arises.

I will appreciate help!
Thanks

> 
> Jan