On Fri, 12 Mar 2021 11:29:43 +0000 Alexandru Elisei <[email protected]> wrote:
> Hi Andre, > > On 2/25/21 12:59 AM, Andre Przywara wrote: > > With the planned retirement of the special ioport emulation code, we > > need to provide an emulation function compatible with the MMIO prototype. > > > > Adjust the trap handler to use that new function, and provide shims to > > implement the old ioport interface, for now. > > > > We drop the usage of ioport__read8/write8 entirely, as this would only > > be applicable for I/O port accesses, and does nothing for 8-bit wide > > accesses anyway. > > > > Signed-off-by: Andre Przywara <[email protected]> > > --- > > hw/serial.c | 93 +++++++++++++++++++++++++++++++++-------------------- > > 1 file changed, 58 insertions(+), 35 deletions(-) > > > > diff --git a/hw/serial.c b/hw/serial.c > > index b0465d99..c495eac1 100644 > > --- a/hw/serial.c > > +++ b/hw/serial.c > > @@ -242,36 +242,31 @@ void serial8250__inject_sysrq(struct kvm *kvm, char > > sysrq) > > sysrq_pending = sysrq; > > } > > > > -static bool serial8250_out(struct ioport *ioport, struct kvm_cpu *vcpu, > > u16 port, > > - void *data, int size) > > +static bool serial8250_out(struct serial8250_device *dev, struct kvm_cpu > > *vcpu, > > + u16 offset, u8 data) > > { > > - struct serial8250_device *dev = ioport->priv; > > - u16 offset; > > bool ret = true; > > - char *addr = data; > > > > mutex_lock(&dev->mutex); > > > > - offset = port - dev->iobase; > > - > > switch (offset) { > > case UART_TX: > > if (dev->lcr & UART_LCR_DLAB) { > > - dev->dll = ioport__read8(data); > > + dev->dll = data; > > break; > > } > > > > /* Loopback mode */ > > if (dev->mcr & UART_MCR_LOOP) { > > if (dev->rxcnt < FIFO_LEN) { > > - dev->rxbuf[dev->rxcnt++] = *addr; > > + dev->rxbuf[dev->rxcnt++] = data; > > dev->lsr |= UART_LSR_DR; > > } > > break; > > } > > > > if (dev->txcnt < FIFO_LEN) { > > - dev->txbuf[dev->txcnt++] = *addr; > > + dev->txbuf[dev->txcnt++] = data; > > dev->lsr &= ~UART_LSR_TEMT; > > if (dev->txcnt == FIFO_LEN / 2) > > dev->lsr &= ~UART_LSR_THRE; > > @@ -283,18 +278,18 @@ static bool serial8250_out(struct ioport *ioport, > > struct kvm_cpu *vcpu, u16 port > > break; > > case UART_IER: > > if (!(dev->lcr & UART_LCR_DLAB)) > > - dev->ier = ioport__read8(data) & 0x0f; > > + dev->ier = data & 0x0f; > > else > > - dev->dlm = ioport__read8(data); > > + dev->dlm = data; > > break; > > case UART_FCR: > > - dev->fcr = ioport__read8(data); > > + dev->fcr = data; > > break; > > case UART_LCR: > > - dev->lcr = ioport__read8(data); > > + dev->lcr = data; > > break; > > case UART_MCR: > > - dev->mcr = ioport__read8(data); > > + dev->mcr = data; > > break; > > case UART_LSR: > > /* Factory test */ > > @@ -303,7 +298,7 @@ static bool serial8250_out(struct ioport *ioport, > > struct kvm_cpu *vcpu, u16 port > > /* Not used */ > > break; > > case UART_SCR: > > - dev->scr = ioport__read8(data); > > + dev->scr = data; > > break; > > default: > > ret = false; > > @@ -317,7 +312,7 @@ static bool serial8250_out(struct ioport *ioport, > > struct kvm_cpu *vcpu, u16 port > > return ret; > > } > > > > -static void serial8250_rx(struct serial8250_device *dev, void *data) > > +static void serial8250_rx(struct serial8250_device *dev, u8 *data) > > { > > if (dev->rxdone == dev->rxcnt) > > return; > > @@ -325,57 +320,54 @@ static void serial8250_rx(struct serial8250_device > > *dev, void *data) > > /* Break issued ? */ > > if (dev->lsr & UART_LSR_BI) { > > dev->lsr &= ~UART_LSR_BI; > > - ioport__write8(data, 0); > > + *data = 0; > > return; > > } > > > > - ioport__write8(data, dev->rxbuf[dev->rxdone++]); > > + *data = dev->rxbuf[dev->rxdone++]; > > if (dev->rxcnt == dev->rxdone) { > > dev->lsr &= ~UART_LSR_DR; > > dev->rxcnt = dev->rxdone = 0; > > } > > } > > > > -static bool serial8250_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 > > port, void *data, int size) > > +static bool serial8250_in(struct serial8250_device *dev, struct kvm_cpu > > *vcpu, > > + u16 offset, u8 *data) > > { > > - struct serial8250_device *dev = ioport->priv; > > - u16 offset; > > bool ret = true; > > > > mutex_lock(&dev->mutex); > > > > - offset = port - dev->iobase; > > - > > switch (offset) { > > case UART_RX: > > if (dev->lcr & UART_LCR_DLAB) > > - ioport__write8(data, dev->dll); > > + *data = dev->dll; > > else > > serial8250_rx(dev, data); > > break; > > case UART_IER: > > if (dev->lcr & UART_LCR_DLAB) > > - ioport__write8(data, dev->dlm); > > + *data = dev->dlm; > > else > > - ioport__write8(data, dev->ier); > > + *data = dev->ier; > > break; > > case UART_IIR: > > - ioport__write8(data, dev->iir | UART_IIR_TYPE_BITS); > > + *data = dev->iir | UART_IIR_TYPE_BITS; > > break; > > case UART_LCR: > > - ioport__write8(data, dev->lcr); > > + *data = dev->lcr; > > break; > > case UART_MCR: > > - ioport__write8(data, dev->mcr); > > + *data = dev->mcr; > > break; > > case UART_LSR: > > - ioport__write8(data, dev->lsr); > > + *data = dev->lsr; > > break; > > case UART_MSR: > > - ioport__write8(data, dev->msr); > > + *data = dev->msr; > > break; > > case UART_SCR: > > - ioport__write8(data, dev->scr); > > + *data = dev->scr; > > break; > > default: > > ret = false; > > @@ -389,6 +381,37 @@ static bool serial8250_in(struct ioport *ioport, > > struct kvm_cpu *vcpu, u16 port, > > return ret; > > } > > I'm not sure about replacing ioport_{read,write}8 with pointer dereferencing. > They > are functionally equivalent because big-endian and little-endian are about the > order of bytes in memory, not the order of bits, but I don't think the change > makes much sense because: > > 1. All the other emulated devices use the ioport accessors, even in the case > of 8 > bit accesses. For example, the RTC device uses ioport accessors and it too is > converted in the end to a MMIO device, just like serial. > > 2. The patch is about refactoring the emulation callbacks, other (non-trivial) > changes should be a separate patch. > > Please let me know if it's something that I'm missing. Other than this > change, the > patch looks good. Well, I can see merits in both approaches: keeping the wrappers and ditching them. Don't really have a strong opinion, but I think eventually we should have the endian conversion in one central place (in kvm__emulate_io()), where we can easily differentiate between port I/O, MMIO and PCI accesses. Then all devices just need to dereference pointers. But this is indeed something for another time, and having wrappers should not hurt either way, so I will keep them. Makes the patch smaller anyway. Cheers, Andre > > > > +static void serial8250_mmio(struct kvm_cpu *vcpu, u64 addr, u8 *data, u32 > > len, > > + u8 is_write, void *ptr) > > +{ > > + struct serial8250_device *dev = ptr; > > + > > + if (is_write) > > + serial8250_out(dev, vcpu, addr - dev->iobase, *data); > > + else > > + serial8250_in(dev, vcpu, addr - dev->iobase, data); > > +} > > + > > +static bool serial8250_ioport_out(struct ioport *ioport, struct kvm_cpu > > *vcpu, > > + u16 port, void *data, int size) > > +{ > > + struct serial8250_device *dev = ioport->priv; > > + > > + serial8250_mmio(vcpu, port, data, 1, true, dev); > > + > > + return true; > > +} > > + > > +static bool serial8250_ioport_in(struct ioport *ioport, struct kvm_cpu > > *vcpu, > > + u16 port, void *data, int size) > > +{ > > + struct serial8250_device *dev = ioport->priv; > > + > > + serial8250_mmio(vcpu, port, data, 1, false, dev); > > + > > + return true; > > +} > > + > > #ifdef CONFIG_HAS_LIBFDT > > > > char *fdt_stdout_path = NULL; > > @@ -427,8 +450,8 @@ void serial8250_generate_fdt_node(void *fdt, struct > > device_header *dev_hdr, > > #endif > > > > static struct ioport_operations serial8250_ops = { > > - .io_in = serial8250_in, > > - .io_out = serial8250_out, > > + .io_in = serial8250_ioport_in, > > + .io_out = serial8250_ioport_out, > > }; > > > > static int serial8250__device_init(struct kvm *kvm, _______________________________________________ kvmarm mailing list [email protected] https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
