On 24/06/2025 05:55, dm...@proton.me wrote:
> From: Denis Mukhin <dmuk...@ford.com>
>
> 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 <dmuk...@ford.com>
> ---
> 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
