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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 13 Aug 2018, Julien Grall wrote:
> 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".

OK

> > +        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