vmd(8): slight NS8250 fix

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

vmd(8): slight NS8250 fix

Luigi30
Hi,

Adjusted NS8250 behavior in vmd(8) so it gets detected as an 8250 and not a 16450 by OpenBSD’s boot process. Also generalized some of the COM1-specific I/O address definitions to support adding COM2 (and COM3, and COM4…) in the future.

Tested by logging into my VM with the virtual serial console and reinstalling 6.5, everything was fine. The boot process detected an NS8250.

Index: ns8250.c
===================================================================
RCS file: /cvs/src/usr.sbin/vmd/ns8250.c,v
retrieving revision 1.20
diff -u -p -u -r1.20 ns8250.c
--- ns8250.c 11 Mar 2019 17:08:52 -0000 1.20
+++ ns8250.c 23 May 2019 00:52:15 -0000
@@ -74,6 +74,7 @@ ns8250_init(int fd, uint32_t vmid)
  }
  com1_dev.fd = fd;
  com1_dev.irq = 4;
+ com1_dev.portid = NS8250_COM1;
  com1_dev.rcv_pending = 0;
  com1_dev.vmid = vmid;
  com1_dev.byte_out = 0;
@@ -509,10 +510,10 @@ vcpu_process_com_scr(struct vm_exit *vei
  /*
  * vei_dir == VEI_DIR_OUT : out instruction
  *
- * Write to SCR
+ * The 8250 does not have a scratch register.
  */
  if (vei->vei.vei_dir == VEI_DIR_OUT) {
- com1_dev.regs.scr = vei->vei.vei_data;
+ com1_dev.regs.scr = 0xFF;
  } else {
  /*
  * vei_dir == VEI_DIR_IN : in instruction
@@ -647,6 +648,7 @@ ns8250_restore(int fd, int con_fd, uint3
  }
  com1_dev.fd = con_fd;
  com1_dev.irq = 4;
+ com1_dev.portid = NS8250_COM1;
  com1_dev.rcv_pending = 0;
  com1_dev.vmid = vmid;
  com1_dev.byte_out = 0;
Index: ns8250.h
===================================================================
RCS file: /cvs/src/usr.sbin/vmd/ns8250.h,v
retrieving revision 1.7
diff -u -p -u -r1.7 ns8250.h
--- ns8250.h 11 Mar 2019 17:08:52 -0000 1.7
+++ ns8250.h 23 May 2019 00:52:15 -0000
@@ -18,14 +18,30 @@
 /*
  * Emulated 8250 UART
  */
-#define COM1_DATA 0x3f8
-#define COM1_IER 0x3f9
-#define COM1_IIR 0x3fa
-#define COM1_LCR 0x3fb
-#define COM1_MCR 0x3fc
-#define COM1_LSR 0x3fd
-#define COM1_MSR 0x3fe
-#define COM1_SCR 0x3ff
+#define COM1_BASE       0x3f8
+#define COM1_DATA COM1_BASE+COM_OFFSET_DATA
+#define COM1_IER COM1_BASE+COM_OFFSET_IER
+#define COM1_IIR COM1_BASE+COM_OFFSET_IIR
+#define COM1_LCR COM1_BASE+COM_OFFSET_LCR
+#define COM1_MCR COM1_BASE+COM_OFFSET_MCR
+#define COM1_LSR COM1_BASE+COM_OFFSET_LSR
+#define COM1_MSR COM1_BASE+COM_OFFSET_MSR
+#define COM1_SCR COM1_BASE+COM_OFFSET_SCR
+
+#define COM_OFFSET_DATA 0
+#define COM_OFFSET_IER  1
+#define COM_OFFSET_IIR  2
+#define COM_OFFSET_LCR  3
+#define COM_OFFSET_MCR  4
+#define COM_OFFSET_LSR  5
+#define COM_OFFSET_MSR  6
+#define COM_OFFSET_SCR  7
+
+/* ns8250 port identifier */
+enum ns8250_portid {
+ NS8250_COM1,
+ NS8250_COM2,
+};
 
 /* ns8250 UART registers */
 struct ns8250_regs {
@@ -50,6 +66,7 @@ struct ns8250_dev {
  struct event rate;
  struct event wake;
  struct timeval rate_tv;
+ enum ns8250_portid portid;
  int fd;
  int irq;
  int rcv_pending;

Reply | Threaded
Open this post in threaded view
|

Re: vmd(8): slight NS8250 fix

Mike Larkin-2
On Wed, May 22, 2019 at 08:05:50PM -0500, Katherine Rohl wrote:
> Hi,
>
> Adjusted NS8250 behavior in vmd(8) so it gets detected as an 8250 and not a 16450 by OpenBSD’s boot process. Also generalized some of the COM1-specific I/O address definitions to support adding COM2 (and COM3, and COM4…) in the future.
>
> Tested by logging into my VM with the virtual serial console and reinstalling 6.5, everything was fine. The boot process detected an NS8250.
>

Thanks for the vmd diff.

Please test with various Linux guests also. I added the scratch reg to make
this like an 8250A, IIRC, to support Linux guests. And you'll need to test
with a few, Alpine, Ubuntu, Centos, etc, as they all had different
behaviour. If those all work then we can get this in.

-ml

> Index: ns8250.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/ns8250.c,v
> retrieving revision 1.20
> diff -u -p -u -r1.20 ns8250.c
> --- ns8250.c 11 Mar 2019 17:08:52 -0000 1.20
> +++ ns8250.c 23 May 2019 00:52:15 -0000
> @@ -74,6 +74,7 @@ ns8250_init(int fd, uint32_t vmid)
>   }
>   com1_dev.fd = fd;
>   com1_dev.irq = 4;
> + com1_dev.portid = NS8250_COM1;
>   com1_dev.rcv_pending = 0;
>   com1_dev.vmid = vmid;
>   com1_dev.byte_out = 0;
> @@ -509,10 +510,10 @@ vcpu_process_com_scr(struct vm_exit *vei
>   /*
>   * vei_dir == VEI_DIR_OUT : out instruction
>   *
> - * Write to SCR
> + * The 8250 does not have a scratch register.
>   */
>   if (vei->vei.vei_dir == VEI_DIR_OUT) {
> - com1_dev.regs.scr = vei->vei.vei_data;
> + com1_dev.regs.scr = 0xFF;
>   } else {
>   /*
>   * vei_dir == VEI_DIR_IN : in instruction
> @@ -647,6 +648,7 @@ ns8250_restore(int fd, int con_fd, uint3
>   }
>   com1_dev.fd = con_fd;
>   com1_dev.irq = 4;
> + com1_dev.portid = NS8250_COM1;
>   com1_dev.rcv_pending = 0;
>   com1_dev.vmid = vmid;
>   com1_dev.byte_out = 0;
> Index: ns8250.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/ns8250.h,v
> retrieving revision 1.7
> diff -u -p -u -r1.7 ns8250.h
> --- ns8250.h 11 Mar 2019 17:08:52 -0000 1.7
> +++ ns8250.h 23 May 2019 00:52:15 -0000
> @@ -18,14 +18,30 @@
>  /*
>   * Emulated 8250 UART
>   */
> -#define COM1_DATA 0x3f8
> -#define COM1_IER 0x3f9
> -#define COM1_IIR 0x3fa
> -#define COM1_LCR 0x3fb
> -#define COM1_MCR 0x3fc
> -#define COM1_LSR 0x3fd
> -#define COM1_MSR 0x3fe
> -#define COM1_SCR 0x3ff
> +#define COM1_BASE       0x3f8
> +#define COM1_DATA COM1_BASE+COM_OFFSET_DATA
> +#define COM1_IER COM1_BASE+COM_OFFSET_IER
> +#define COM1_IIR COM1_BASE+COM_OFFSET_IIR
> +#define COM1_LCR COM1_BASE+COM_OFFSET_LCR
> +#define COM1_MCR COM1_BASE+COM_OFFSET_MCR
> +#define COM1_LSR COM1_BASE+COM_OFFSET_LSR
> +#define COM1_MSR COM1_BASE+COM_OFFSET_MSR
> +#define COM1_SCR COM1_BASE+COM_OFFSET_SCR
> +
> +#define COM_OFFSET_DATA 0
> +#define COM_OFFSET_IER  1
> +#define COM_OFFSET_IIR  2
> +#define COM_OFFSET_LCR  3
> +#define COM_OFFSET_MCR  4
> +#define COM_OFFSET_LSR  5
> +#define COM_OFFSET_MSR  6
> +#define COM_OFFSET_SCR  7
> +
> +/* ns8250 port identifier */
> +enum ns8250_portid {
> + NS8250_COM1,
> + NS8250_COM2,
> +};
>  
>  /* ns8250 UART registers */
>  struct ns8250_regs {
> @@ -50,6 +66,7 @@ struct ns8250_dev {
>   struct event rate;
>   struct event wake;
>   struct timeval rate_tv;
> + enum ns8250_portid portid;
>   int fd;
>   int irq;
>   int rcv_pending;
>

Reply | Threaded
Open this post in threaded view
|

Re: vmd(8): slight NS8250 fix

Luigi30
Hi,

I was able to install Centos and boot
Alpine and Ubuntu without problems using the
serial interface, using vmd(8) built with my diff.

(Side note, what console email program is
the best for submitting these patches anyway?)

Katherine

> On May 23, 2019, at 12:22 AM, Mike Larkin <[hidden email]> wrote:
>
>

Reply | Threaded
Open this post in threaded view
|

Re: vmd(8): slight NS8250 fix

Mike Larkin-2
On Sat, May 25, 2019 at 01:43:43AM -0500, Katherine Rohl wrote:

> Hi,
>
> I was able to install Centos and boot
> Alpine and Ubuntu without problems using the
> serial interface, using vmd(8) built with my diff.
>
> (Side note, what console email program is
> the best for submitting these patches anyway?)
>
> Katherine
>
> > On May 23, 2019, at 12:22 AM, Mike Larkin <[hidden email]> wrote:
> >
> >
>

I will do a quick test and try to commit this weekend. Thanks!

-ml

Reply | Threaded
Open this post in threaded view
|

Re: vmd(8): slight NS8250 fix

Mike Larkin-2
In reply to this post by Luigi30
On Wed, May 22, 2019 at 08:05:50PM -0500, Katherine Rohl wrote:
> Hi,
>
> Adjusted NS8250 behavior in vmd(8) so it gets detected as an 8250 and not a 16450 by OpenBSD’s boot process. Also generalized some of the COM1-specific I/O address definitions to support adding COM2 (and COM3, and COM4…) in the future.
>
> Tested by logging into my VM with the virtual serial console and reinstalling 6.5, everything was fine. The boot process detected an NS8250.
>

It occurred to me that by always returning 0xFF, a guest might write 0xFF to
scratch, then read it back, and we'd always return 0xFF. They might then
think that the scratch register worked. That's pretty bizarre, but I've seen
a lot of bizarre behaviour in various guests.

The slightly revised version below just inverts whatever was written, ensuring
that whatever is read is not what was written. This should make anyone doing
such calculation think the scratch doesn't work.

Tested on Linux and OpenBSD guests.

Thoughts?

-ml

Index: ns8250.c
===================================================================
RCS file: /cvs/src/usr.sbin/vmd/ns8250.c,v
retrieving revision 1.20
diff -u -p -a -u -r1.20 ns8250.c
--- ns8250.c 11 Mar 2019 17:08:52 -0000 1.20
+++ ns8250.c 27 May 2019 06:34:38 -0000
@@ -74,6 +74,7 @@ ns8250_init(int fd, uint32_t vmid)
  }
  com1_dev.fd = fd;
  com1_dev.irq = 4;
+ com1_dev.portid = NS8250_COM1;
  com1_dev.rcv_pending = 0;
  com1_dev.vmid = vmid;
  com1_dev.byte_out = 0;
@@ -509,7 +510,8 @@ vcpu_process_com_scr(struct vm_exit *vei
  /*
  * vei_dir == VEI_DIR_OUT : out instruction
  *
- * Write to SCR
+ * The 8250 does not have a scratch register. Make sure that whatever
+ * was written is not what gets read back.
  */
  if (vei->vei.vei_dir == VEI_DIR_OUT) {
  com1_dev.regs.scr = vei->vei.vei_data;
@@ -517,9 +519,11 @@ vcpu_process_com_scr(struct vm_exit *vei
  /*
  * vei_dir == VEI_DIR_IN : in instruction
  *
- * Read from SCR
+ * Read from SCR - invert whatever was previously written,
+ * to ensure the guest doesn't think the scratch register
+ * works.
  */
- set_return_data(vei, com1_dev.regs.scr);
+ set_return_data(vei, ~com1_dev.regs.scr);
  }
 }
 
@@ -647,6 +651,7 @@ ns8250_restore(int fd, int con_fd, uint3
  }
  com1_dev.fd = con_fd;
  com1_dev.irq = 4;
+ com1_dev.portid = NS8250_COM1;
  com1_dev.rcv_pending = 0;
  com1_dev.vmid = vmid;
  com1_dev.byte_out = 0;
Index: ns8250.h
===================================================================
RCS file: /cvs/src/usr.sbin/vmd/ns8250.h,v
retrieving revision 1.7
diff -u -p -a -u -r1.7 ns8250.h
--- ns8250.h 11 Mar 2019 17:08:52 -0000 1.7
+++ ns8250.h 27 May 2019 06:31:13 -0000
@@ -18,14 +18,30 @@
 /*
  * Emulated 8250 UART
  */
-#define COM1_DATA 0x3f8
-#define COM1_IER 0x3f9
-#define COM1_IIR 0x3fa
-#define COM1_LCR 0x3fb
-#define COM1_MCR 0x3fc
-#define COM1_LSR 0x3fd
-#define COM1_MSR 0x3fe
-#define COM1_SCR 0x3ff
+#define COM1_BASE       0x3f8
+#define COM1_DATA COM1_BASE+COM_OFFSET_DATA
+#define COM1_IER COM1_BASE+COM_OFFSET_IER
+#define COM1_IIR COM1_BASE+COM_OFFSET_IIR
+#define COM1_LCR COM1_BASE+COM_OFFSET_LCR
+#define COM1_MCR COM1_BASE+COM_OFFSET_MCR
+#define COM1_LSR COM1_BASE+COM_OFFSET_LSR
+#define COM1_MSR COM1_BASE+COM_OFFSET_MSR
+#define COM1_SCR COM1_BASE+COM_OFFSET_SCR
+
+#define COM_OFFSET_DATA 0
+#define COM_OFFSET_IER  1
+#define COM_OFFSET_IIR  2
+#define COM_OFFSET_LCR  3
+#define COM_OFFSET_MCR  4
+#define COM_OFFSET_LSR  5
+#define COM_OFFSET_MSR  6
+#define COM_OFFSET_SCR  7
+
+/* ns8250 port identifier */
+enum ns8250_portid {
+ NS8250_COM1,
+ NS8250_COM2,
+};
 
 /* ns8250 UART registers */
 struct ns8250_regs {
@@ -50,6 +66,7 @@ struct ns8250_dev {
  struct event rate;
  struct event wake;
  struct timeval rate_tv;
+ enum ns8250_portid portid;
  int fd;
  int irq;
  int rcv_pending;

Reply | Threaded
Open this post in threaded view
|

mail program for patches (was: Re: vmd(8): slight NS8250 fix)

Stefan Sperling-5
In reply to this post by Luigi30
On Sat, May 25, 2019 at 01:43:43AM -0500, Katherine Rohl wrote:
> (Side note, what console email program is
> the best for submitting these patches anyway?)

It doesn't really matter what tooling you use as long as it does not
mangle patches; typically, whitespace reformatting and line-wrapping
pose a problem. Copy-pasting patches into a browser window is quite
likely to break them so that should be avoided. Reading patch files
directly into an editor buffer will generally work well.

One neat trick is sending a large patch to yourself and trying to apply
it yourself, and tweaking your tooling / workflow until this works.

Reply | Threaded
Open this post in threaded view
|

Re: vmd(8): slight NS8250 fix

Mike Larkin-2
In reply to this post by Mike Larkin-2
On Sun, May 26, 2019 at 11:38:37PM -0700, Mike Larkin wrote:

> On Wed, May 22, 2019 at 08:05:50PM -0500, Katherine Rohl wrote:
> > Hi,
> >
> > Adjusted NS8250 behavior in vmd(8) so it gets detected as an 8250 and not a 16450 by OpenBSD’s boot process. Also generalized some of the COM1-specific I/O address definitions to support adding COM2 (and COM3, and COM4…) in the future.
> >
> > Tested by logging into my VM with the virtual serial console and reinstalling 6.5, everything was fine. The boot process detected an NS8250.
> >
>
> It occurred to me that by always returning 0xFF, a guest might write 0xFF to
> scratch, then read it back, and we'd always return 0xFF. They might then
> think that the scratch register worked. That's pretty bizarre, but I've seen
> a lot of bizarre behaviour in various guests.
>

Theo pointed out that this is probably the right way (always return 0xFF), so
I'll commit the original version.

Thanks.

-ml

> The slightly revised version below just inverts whatever was written, ensuring
> that whatever is read is not what was written. This should make anyone doing
> such calculation think the scratch doesn't work.
>
> Tested on Linux and OpenBSD guests.
>
> Thoughts?
>
> -ml
>
> Index: ns8250.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/ns8250.c,v
> retrieving revision 1.20
> diff -u -p -a -u -r1.20 ns8250.c
> --- ns8250.c 11 Mar 2019 17:08:52 -0000 1.20
> +++ ns8250.c 27 May 2019 06:34:38 -0000
> @@ -74,6 +74,7 @@ ns8250_init(int fd, uint32_t vmid)
>   }
>   com1_dev.fd = fd;
>   com1_dev.irq = 4;
> + com1_dev.portid = NS8250_COM1;
>   com1_dev.rcv_pending = 0;
>   com1_dev.vmid = vmid;
>   com1_dev.byte_out = 0;
> @@ -509,7 +510,8 @@ vcpu_process_com_scr(struct vm_exit *vei
>   /*
>   * vei_dir == VEI_DIR_OUT : out instruction
>   *
> - * Write to SCR
> + * The 8250 does not have a scratch register. Make sure that whatever
> + * was written is not what gets read back.
>   */
>   if (vei->vei.vei_dir == VEI_DIR_OUT) {
>   com1_dev.regs.scr = vei->vei.vei_data;
> @@ -517,9 +519,11 @@ vcpu_process_com_scr(struct vm_exit *vei
>   /*
>   * vei_dir == VEI_DIR_IN : in instruction
>   *
> - * Read from SCR
> + * Read from SCR - invert whatever was previously written,
> + * to ensure the guest doesn't think the scratch register
> + * works.
>   */
> - set_return_data(vei, com1_dev.regs.scr);
> + set_return_data(vei, ~com1_dev.regs.scr);
>   }
>  }
>  
> @@ -647,6 +651,7 @@ ns8250_restore(int fd, int con_fd, uint3
>   }
>   com1_dev.fd = con_fd;
>   com1_dev.irq = 4;
> + com1_dev.portid = NS8250_COM1;
>   com1_dev.rcv_pending = 0;
>   com1_dev.vmid = vmid;
>   com1_dev.byte_out = 0;
> Index: ns8250.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/ns8250.h,v
> retrieving revision 1.7
> diff -u -p -a -u -r1.7 ns8250.h
> --- ns8250.h 11 Mar 2019 17:08:52 -0000 1.7
> +++ ns8250.h 27 May 2019 06:31:13 -0000
> @@ -18,14 +18,30 @@
>  /*
>   * Emulated 8250 UART
>   */
> -#define COM1_DATA 0x3f8
> -#define COM1_IER 0x3f9
> -#define COM1_IIR 0x3fa
> -#define COM1_LCR 0x3fb
> -#define COM1_MCR 0x3fc
> -#define COM1_LSR 0x3fd
> -#define COM1_MSR 0x3fe
> -#define COM1_SCR 0x3ff
> +#define COM1_BASE       0x3f8
> +#define COM1_DATA COM1_BASE+COM_OFFSET_DATA
> +#define COM1_IER COM1_BASE+COM_OFFSET_IER
> +#define COM1_IIR COM1_BASE+COM_OFFSET_IIR
> +#define COM1_LCR COM1_BASE+COM_OFFSET_LCR
> +#define COM1_MCR COM1_BASE+COM_OFFSET_MCR
> +#define COM1_LSR COM1_BASE+COM_OFFSET_LSR
> +#define COM1_MSR COM1_BASE+COM_OFFSET_MSR
> +#define COM1_SCR COM1_BASE+COM_OFFSET_SCR
> +
> +#define COM_OFFSET_DATA 0
> +#define COM_OFFSET_IER  1
> +#define COM_OFFSET_IIR  2
> +#define COM_OFFSET_LCR  3
> +#define COM_OFFSET_MCR  4
> +#define COM_OFFSET_LSR  5
> +#define COM_OFFSET_MSR  6
> +#define COM_OFFSET_SCR  7
> +
> +/* ns8250 port identifier */
> +enum ns8250_portid {
> + NS8250_COM1,
> + NS8250_COM2,
> +};
>  
>  /* ns8250 UART registers */
>  struct ns8250_regs {
> @@ -50,6 +66,7 @@ struct ns8250_dev {
>   struct event rate;
>   struct event wake;
>   struct timeval rate_tv;
> + enum ns8250_portid portid;
>   int fd;
>   int irq;
>   int rcv_pending;