Re: [Xen-devel] [PATCH v3 20/25] xen/arm: introduce a union in vpl011

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 01/08/18 00:28, Stefano Stabellini wrote:
> Introduce a union in struct vpl011 to contain the console ring members.
> A later patch will add another member of the union for the case where
> the backend is in Xen.
>
> Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> ---
> Changes in v3:
> - rename ring field to dom
>
> Changes in v2:
> - new patch
> ---
>   xen/arch/arm/vpl011.c        | 20 ++++++++++----------
>   xen/include/asm-arm/vpl011.h |  8 ++++++--
>   2 files changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> index a281eab..e70c5ec 100644
> --- a/xen/arch/arm/vpl011.c
> +++ b/xen/arch/arm/vpl011.c
> @@ -82,7 +82,7 @@ static uint8_t vpl011_read_data(struct domain *d)
>       unsigned long flags;
>       uint8_t data = 0;
>       struct vpl011 *vpl011 = &d->arch.vpl011;
> -    struct xencons_interface *intf = vpl011->ring_buf;
> +    struct xencons_interface *intf = vpl011->dom.ring_buf;
>       XENCONS_RING_IDX in_cons, in_prod;
>   
>       VPL011_LOCK(d, flags);
> @@ -145,7 +145,7 @@ static uint8_t vpl011_read_data(struct domain *d)
>   static void vpl011_update_tx_fifo_status(struct vpl011 *vpl011,
>                                            unsigned int fifo_level)
>   {
> -    struct xencons_interface *intf = vpl011->ring_buf;
> +    struct xencons_interface *intf = vpl011->dom.ring_buf;
>       unsigned int fifo_threshold = sizeof(intf->out) - SBSA_UART_FIFO_LEVEL;
>   
>       BUILD_BUG_ON(sizeof(intf->out) < SBSA_UART_FIFO_SIZE);
> @@ -164,7 +164,7 @@ static void vpl011_write_data(struct domain *d, uint8_t 
> data)
>   {
>       unsigned long flags;
>       struct vpl011 *vpl011 = &d->arch.vpl011;
> -    struct xencons_interface *intf = vpl011->ring_buf;
> +    struct xencons_interface *intf = vpl011->dom.ring_buf;
>       XENCONS_RING_IDX out_cons, out_prod;
>   
>       VPL011_LOCK(d, flags);
> @@ -382,7 +382,7 @@ static void vpl011_data_avail(struct domain *d)
>   {
>       unsigned long flags;
>       struct vpl011 *vpl011 = &d->arch.vpl011;
> -    struct xencons_interface *intf = vpl011->ring_buf;
> +    struct xencons_interface *intf = vpl011->dom.ring_buf;
>       XENCONS_RING_IDX in_cons, in_prod, out_cons, out_prod;
>       XENCONS_RING_IDX in_fifo_level, out_fifo_level;
>   
> @@ -459,14 +459,14 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
>       int rc;
>       struct vpl011 *vpl011 = &d->arch.vpl011;
>   
> -    if ( vpl011->ring_buf )
> +    if ( vpl011->dom.ring_buf )
>           return -EINVAL;
>   
>       /* Map the guest PFN to Xen address space. */
>       rc =  prepare_ring_for_helper(d,
>                                     gfn_x(info->gfn),
> -                                  &vpl011->ring_page,
> -                                  &vpl011->ring_buf);
> +                                  &vpl011->dom.ring_page,
> +                                  &vpl011->dom.ring_buf);
>       if ( rc < 0 )
>           goto out;
>   
> @@ -495,7 +495,7 @@ out2:
>       vgic_free_virq(d, GUEST_VPL011_SPI);
>   
>   out1:
> -    destroy_ring_for_helper(&vpl011->ring_buf, vpl011->ring_page);
> +    destroy_ring_for_helper(&vpl011->dom.ring_buf, vpl011->dom.ring_page);
>   
>   out:
>       return rc;
> @@ -505,11 +505,11 @@ void domain_vpl011_deinit(struct domain *d)
>   {
>       struct vpl011 *vpl011 = &d->arch.vpl011;
>   
> -    if ( !vpl011->ring_buf )
> +    if ( !vpl011->dom.ring_buf )
>           return;
>   
>       free_xen_event_channel(d, vpl011->evtchn);
> -    destroy_ring_for_helper(&vpl011->ring_buf, vpl011->ring_page);
> +    destroy_ring_for_helper(&vpl011->dom.ring_buf, vpl011->dom.ring_page);
>   }
>   
>   /*
> diff --git a/xen/include/asm-arm/vpl011.h b/xen/include/asm-arm/vpl011.h
> index db95ff8..b873a29 100644
> --- a/xen/include/asm-arm/vpl011.h
> +++ b/xen/include/asm-arm/vpl011.h
> @@ -31,8 +31,12 @@
>   #define SBSA_UART_FIFO_SIZE 32
>   
>   struct vpl011 {
> -    void *ring_buf;
> -    struct page_info *ring_page;
> +    union {
If you name the union vpl011_backend it would be clearer that this deal 
with backend information. Or even just avoiding the anonymous union by 
naming it "backend".
> +        struct {
> +            void *ring_buf;
> +            struct page_info *ring_page;
> +        } dom;
> +    };
>       uint32_t    uartfr;         /* Flag register */
>       uint32_t    uartcr;         /* Control register */
>       uint32_t    uartimsc;       /* Interrupt mask register*/
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel