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 via email to