Re: [PATCH v6 04/15] emul/ns16x50: implement DLL/DLM registers

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Denis,

On Sat, Sep 6, 2025 at 3:11 AM <dmukhin@xxxxxxx> wrote:
>
> From: Denis Mukhin <dmukhin@xxxxxxxx>
>
> Add DLL/DLM registers emulation.
>
> DLL/DLM registers report hardcoded 115200 baud rate to the guest OS.
>
> Add stub for ns16x50_dlab_get() helper.
>
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> ---
> Changes since v5:
> - dropped ns16x50_dlab_get() hunk (moved to emulator stub)
> - Link to v5: 
> https://lore.kernel.org/xen-devel/20250828235409.2835815-5-dmukhin@xxxxxxxx/
> ---
>  xen/common/emul/vuart/ns16x50.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>
> diff --git a/xen/common/emul/vuart/ns16x50.c b/xen/common/emul/vuart/ns16x50.c
> index 0462a961e785..7f479a5be4a2 100644
> --- a/xen/common/emul/vuart/ns16x50.c
> +++ b/xen/common/emul/vuart/ns16x50.c
> @@ -97,8 +97,13 @@ static uint8_t ns16x50_dlab_get(const struct vuart_ns16x50 
> *vdev)
>  static int ns16x50_io_write8(
>      struct vuart_ns16x50 *vdev, uint32_t reg, uint8_t *data)
>  {
> +    uint8_t *regs = vdev->regs;
> +    uint8_t val = *data;
>      int rc = 0;
>
> +    if ( ns16x50_dlab_get(vdev) && (reg == UART_DLL || reg == UART_DLM) )
> +        regs[NS16X50_REGS_NUM + reg] = val;

Some documentation (e.g., DesignWare DW_apb_uart Databook, v3.04a)
notes that if the Divisor Latch Registers (DLL and DLH) are set to
zero, the baud clock is disabled and no serial communications occur.

Should we handle the zero-divisor case to emulate this behavior more
accurately?

> +
>      return rc;
>  }
>
> @@ -109,8 +114,16 @@ static int ns16x50_io_write8(
>  static int ns16x50_io_write16(
>      struct vuart_ns16x50 *vdev, uint32_t reg, uint16_t *data)
>  {
> +    uint16_t val = *data;
>      int rc = -EINVAL;
>
> +    if ( ns16x50_dlab_get(vdev) && reg == UART_DLL )
> +    {
> +        vdev->regs[NS16X50_REGS_NUM + UART_DLL] = val & 0xff;
> +        vdev->regs[NS16X50_REGS_NUM + UART_DLM] = (val >> 8) & 0xff;

Instead of hardcoding 0xff here (and in similar lines below), consider
using UINT8_MAX. This makes it explicit that the value is the maximum
for a uint8_t and avoids magic numbers.

> +        rc = 0;
> +    }
> +
>      return rc;
>  }
>
> @@ -146,9 +159,13 @@ static int ns16x50_io_write(
>  static int ns16x50_io_read8(
>      struct vuart_ns16x50 *vdev, uint32_t reg, uint8_t *data)
>  {
> +    uint8_t *regs = vdev->regs;
>      uint8_t val = 0xff;
>      int rc = 0;
>
> +    if ( ns16x50_dlab_get(vdev) && (reg == UART_DLL || reg == UART_DLM) )
> +        val = regs[NS16X50_REGS_NUM + reg];
> +
>      *data = val;
>
>      return rc;
> @@ -163,6 +180,13 @@ static int ns16x50_io_read16(
>      uint16_t val = 0xffff;
>      int rc = -EINVAL;
>
> +    if ( ns16x50_dlab_get(vdev) && reg == UART_DLL )
> +    {
> +        val = vdev->regs[NS16X50_REGS_NUM + UART_DLM] << 8 |
> +              vdev->regs[NS16X50_REGS_NUM + UART_DLL];
> +        rc = 0;
> +    }
> +
>      *data = val;
>
>      return rc;
> @@ -280,12 +304,17 @@ out:
>
>  static int ns16x50_init(void *arg)
>  {
> +    const uint16_t divisor = (UART_CLOCK_HZ / 115200) >> 4;
>      struct vuart_ns16x50 *vdev = arg;
>      const struct vuart_info *info = vdev->info;
>      struct domain *d = vdev->owner;
>
>      ASSERT(vdev);
>
> +    /* NB: report 115200 baud rate. */
> +    vdev->regs[NS16X50_REGS_NUM + UART_DLL] = divisor & 0xff;
> +    vdev->regs[NS16X50_REGS_NUM + UART_DLM] = (divisor >> 8) & 0xff;
> +
>      register_portio_handler(d, info->base_addr, info->size, 
> ns16x50_io_handle);
>
>      return 0;
> --
> 2.51.0
>
>

Best regards,
Mykola