Re: [PATCH v1 05/16] arm/vpl011: use void pointer in domain struct

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 24/06/2025 05:55, dmkhn@xxxxxxxxx wrote:
> From: Denis Mukhin <dmukhin@xxxxxxxx> 
> 
> Switch to using void pointer in domain struct to reduce compile-time
> dependencies for PL011 emulator.
I don't understand the rationale very well. Could you provide more details?
Why can't we keep struct vpl011 in domain struct given? I would understand the
need for void if we used a single member that could map to different vUARTs
depending on selection. That's clearly not the case. If it is just to avoid the
header, then I don't think we need such churn.

> 
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> ---
>  xen/arch/arm/include/asm/domain.h |   3 +-
>  xen/arch/arm/vpl011.c             | 139 +++++++++++++++++-------------
>  2 files changed, 79 insertions(+), 63 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/domain.h 
> b/xen/arch/arm/include/asm/domain.h
> index 746ea687d523..2ee9976b55a8 100644
> --- a/xen/arch/arm/include/asm/domain.h
> +++ b/xen/arch/arm/include/asm/domain.h
> @@ -9,7 +9,6 @@
>  #include <asm/mmio.h>
>  #include <asm/gic.h>
>  #include <asm/vgic.h>
> -#include <asm/vpl011.h>
>  #include <public/hvm/params.h>
>  
>  struct hvm_domain
> @@ -114,7 +113,7 @@ struct arch_domain
>      } monitor;
>  
>  #ifdef CONFIG_HAS_VUART_PL011
> -    struct vpl011 vpl011;
> +    void *vpl011;
>  #endif
>  
>  #ifdef CONFIG_TEE
> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> index a97c3b74208c..3c027ccf0b4e 100644
> --- a/xen/arch/arm/vpl011.c
> +++ b/xen/arch/arm/vpl011.c
> @@ -22,6 +22,7 @@
>  #include <xen/console.h>
>  #include <xen/serial.h>
>  #include <xen/vuart.h>
> +#include <xen/xvmalloc.h>
>  #include <public/domctl.h>
>  #include <public/io/console.h>
>  #include <asm/domain_build.h>
> @@ -31,6 +32,43 @@
>  #include <asm/vpl011.h>
>  #include <asm/vreg.h>
>  
> +static void __vpl011_exit(struct vpl011 *vpl011, struct domain *d
Names starting with '__' are reserved and forbidden by MISRA C rule that AFAIR
we accepted (don't remember what rule exactly).

> +{
> +    if ( vpl011->virq )
> +    {
> +        vgic_free_virq(d, vpl011->virq);
> +
> +        /*
> +         * Set to invalid irq (we use SPI) to prevent extra free and to avoid
> +         * freeing irq that could have already been reserved by someone else.
> +         */
> +        vpl011->virq = 0;
> +    }
> +
> +    if ( vpl011->backend_in_domain )
> +    {
> +        if ( vpl011->backend.dom.ring_buf )
> +            destroy_ring_for_helper(&vpl011->backend.dom.ring_buf,
> +                                    vpl011->backend.dom.ring_page);
> +
> +        if ( vpl011->evtchn )
> +        {
> +            free_xen_event_channel(d, vpl011->evtchn);
> +
> +            /*
> +             * Set to invalid event channel port to prevent extra free and to
> +             * avoid freeing port that could have already been allocated for
> +             * other purposes.
> +             */
> +            vpl011->evtchn = 0;
> +        }
> +    }
> +    else
> +        XFREE(vpl011->backend.xen);
> +
> +    xfree(vpl011);
> +}
> +
>  /*
>   * Since pl011 registers are 32-bit registers, all registers
>   * are handled similarly allowing 8-bit, 16-bit and 32-bit
> @@ -43,7 +81,7 @@ static bool vpl011_reg32_check_access(struct hsr_dabt dabt)
>  
>  static void vpl011_update_interrupt_status(struct domain *d)
>  {
> -    struct vpl011 *vpl011 = &d->arch.vpl011;
> +    struct vpl011 *vpl011 = d->arch.vpl011;
>      uint32_t uartmis = vpl011->uartris & vpl011->uartimsc;
>  
>      /*
> @@ -81,7 +119,7 @@ static void vpl011_update_interrupt_status(struct domain 
> *d)
>  static void vpl011_write_data_xen(struct domain *d, uint8_t data)
>  {
>      unsigned long flags;
> -    struct vpl011 *vpl011 = &d->arch.vpl011;
> +    struct vpl011 *vpl011 = d->arch.vpl011;
>      struct vpl011_xen_backend *intf = vpl011->backend.xen;
>      struct domain *input = console_get_domain();
>  
> @@ -140,7 +178,7 @@ static uint8_t vpl011_read_data_xen(struct domain *d)
>  {
>      unsigned long flags;
>      uint8_t data = 0;
> -    struct vpl011 *vpl011 = &d->arch.vpl011;
> +    struct vpl011 *vpl011 = d->arch.vpl011;
>      struct vpl011_xen_backend *intf = vpl011->backend.xen;
>      XENCONS_RING_IDX in_cons, in_prod;
>  
> @@ -199,7 +237,7 @@ static uint8_t vpl011_read_data(struct domain *d)
>  {
>      unsigned long flags;
>      uint8_t data = 0;
> -    struct vpl011 *vpl011 = &d->arch.vpl011;
> +    struct vpl011 *vpl011 = d->arch.vpl011;
>      struct xencons_interface *intf = vpl011->backend.dom.ring_buf;
>      XENCONS_RING_IDX in_cons, in_prod;
>  
> @@ -284,7 +322,7 @@ static void vpl011_update_tx_fifo_status(struct vpl011 
> *vpl011,
>  static void vpl011_write_data(struct domain *d, uint8_t data)
>  {
>      unsigned long flags;
> -    struct vpl011 *vpl011 = &d->arch.vpl011;
> +    struct vpl011 *vpl011 = d->arch.vpl011;
>      struct xencons_interface *intf = vpl011->backend.dom.ring_buf;
>      XENCONS_RING_IDX out_cons, out_prod;
>  
> @@ -350,10 +388,9 @@ static int vpl011_mmio_read(struct vcpu *v,
>                              register_t *r,
>                              void *priv)
>  {
> +    struct vpl011 *vpl011 = v->domain->arch.vpl011;
>      struct hsr_dabt dabt = info->dabt;
> -    uint32_t vpl011_reg = (uint32_t)(info->gpa -
> -                                     v->domain->arch.vpl011.base_addr);
> -    struct vpl011 *vpl011 = &v->domain->arch.vpl011;
> +    uint32_t vpl011_reg = (uint32_t)(info->gpa - vpl011->base_addr);
>      struct domain *d = v->domain;
>      unsigned long flags;
>  
> @@ -439,10 +476,9 @@ static int vpl011_mmio_write(struct vcpu *v,
>                               register_t r,
>                               void *priv)
>  {
> +    struct vpl011 *vpl011 = v->domain->arch.vpl011;
>      struct hsr_dabt dabt = info->dabt;
> -    uint32_t vpl011_reg = (uint32_t)(info->gpa -
> -                                     v->domain->arch.vpl011.base_addr);
> -    struct vpl011 *vpl011 = &v->domain->arch.vpl011;
> +    uint32_t vpl011_reg = (uint32_t)(info->gpa - vpl011->base_addr);
>      struct domain *d = v->domain;
>      unsigned long flags;
>  
> @@ -518,7 +554,7 @@ static void vpl011_data_avail(struct domain *d,
>                                XENCONS_RING_IDX out_fifo_level,
>                                XENCONS_RING_IDX out_size)
>  {
> -    struct vpl011 *vpl011 = &d->arch.vpl011;
> +    struct vpl011 *vpl011 = d->arch.vpl011;
>  
>      /**** Update the UART RX state ****/
>  
> @@ -576,7 +612,7 @@ static void vpl011_data_avail(struct domain *d,
>  int vuart_putchar(struct domain *d, char c)
>  {
>      unsigned long flags;
> -    struct vpl011 *vpl011 = &d->arch.vpl011;
> +    struct vpl011 *vpl011 = d->arch.vpl011;
>      struct vpl011_xen_backend *intf = vpl011->backend.xen;
>      XENCONS_RING_IDX in_cons, in_prod, in_fifo_level;
>  
> @@ -614,7 +650,7 @@ static void vpl011_notification(struct vcpu *v, unsigned 
> int port)
>  {
>      unsigned long flags;
>      struct domain *d = v->domain;
> -    struct vpl011 *vpl011 = &d->arch.vpl011;
> +    struct vpl011 *vpl011 = d->arch.vpl011;
>      struct xencons_interface *intf = vpl011->backend.dom.ring_buf;
>      XENCONS_RING_IDX in_cons, in_prod, out_cons, out_prod;
>      XENCONS_RING_IDX in_fifo_level, out_fifo_level;
> @@ -644,11 +680,14 @@ static void vpl011_notification(struct vcpu *v, 
> unsigned int port)
>  
>  int vuart_init(struct domain *d, struct vuart_params *params)
>  {
> +    struct vpl011 *vpl011;
>      int rc;
> -    struct vpl011 *vpl011 = &d->arch.vpl011;
>  
> -    if ( vpl011->backend.dom.ring_buf )
> -        return -EINVAL;
> +    BUG_ON(d->arch.vpl011);
> +
> +    vpl011 = xvzalloc(typeof(*vpl011));
> +    if ( !vpl011 )
> +        return -ENOMEM;
>  
>      /*
>       * The VPL011 virq is GUEST_VPL011_SPI, except for direct-map domains
> @@ -670,7 +709,8 @@ int vuart_init(struct domain *d, struct vuart_params 
> *params)
>          {
>              printk(XENLOG_ERR
>                     "vpl011: Unable to re-use the Xen UART information.\n");
> -            return -EINVAL;
> +            rc = -EINVAL;
> +            goto err_out;
>          }
>  
>          /*
> @@ -684,7 +724,8 @@ int vuart_init(struct domain *d, struct vuart_params 
> *params)
>          {
>              printk(XENLOG_ERR
>                     "vpl011: Can't re-use the Xen UART MMIO region as it is 
> too small.\n");
> -            return -EINVAL;
> +            rc = -EINVAL;
> +            goto err_out;
>          }
>      }
>      else
> @@ -707,12 +748,12 @@ int vuart_init(struct domain *d, struct vuart_params 
> *params)
>                                        &vpl011->backend.dom.ring_page,
>                                        &vpl011->backend.dom.ring_buf);
>          if ( rc < 0 )
> -            goto out;
> +            goto err_out;
>  
>          rc = alloc_unbound_xen_event_channel(d, 0, params->console_domid,
>                                               vpl011_notification);
>          if ( rc < 0 )
> -            goto out1;
> +            goto err_out;
>  
>          vpl011->evtchn = params->evtchn = rc;
>      }
> @@ -725,7 +766,7 @@ int vuart_init(struct domain *d, struct vuart_params 
> *params)
>          if ( vpl011->backend.xen == NULL )
>          {
>              rc = -ENOMEM;
> -            goto out;
> +            goto err_out;
>          }
>      }
>  
> @@ -733,7 +774,7 @@ int vuart_init(struct domain *d, struct vuart_params 
> *params)
>      if ( !rc )
>      {
>          rc = -EINVAL;
> -        goto out1;
> +        goto err_out;
>      }
>  
>      vpl011->uartfr = TXFE | RXFE;
> @@ -743,50 +784,22 @@ int vuart_init(struct domain *d, struct vuart_params 
> *params)
>      register_mmio_handler(d, &vpl011_mmio_handler,
>                            vpl011->base_addr, GUEST_PL011_SIZE, NULL);
>  
> +    d->arch.vpl011 = vpl011;
> +
>      return 0;
>  
> -out1:
> -    vuart_exit(d);
> -
> -out:
> +err_out:
Labels should be indented by one space.

~Michal