diff mbox series

[v3,2/2] xen/arm: vpl011: drop redundancy in mmio_write/read

Message ID 20221129023935.1576133-3-jiamei.xie@arm.com (mailing list archive)
State Superseded
Headers show
Series xen/arm: refine vpl011 | expand

Commit Message

Jiamei Xie Nov. 29, 2022, 2:39 a.m. UTC 
This commit is to drop redundancy in the function vpl011_mmio_write
and vpl011_mmio_read:
- In vpl011_mmio_read switch block, all cases have a return. So the
  outside one can be removed.
- Each switch case checks access by the same if statments. So we can
  just use one and put it before the switch block.
- The goto label bad_width and read_as_zero is used only once, put the
  code directly

Signed-off-by: Jiamei Xie <jiamei.xie@arm.com>
---
 xen/arch/arm/vpl011.c | 66 +++++++++++++------------------------------
 1 file changed, 19 insertions(+), 47 deletions(-)

Comments

Julien Grall Nov. 29, 2022, 8:54 a.m. UTC | #1 
Hi,

On 29/11/2022 03:39, Jiamei Xie wrote:
> This commit is to drop redundancy in the function vpl011_mmio_write
> and vpl011_mmio_read:
> - In vpl011_mmio_read switch block, all cases have a return. So the
>    outside one can be removed.
> - Each switch case checks access by the same if statments. So we can
>    just use one and put it before the switch block.
> - The goto label bad_width and read_as_zero is used only once, put the
>    code directly
> 
> Signed-off-by: Jiamei Xie <jiamei.xie@arm.com>
> ---
>   xen/arch/arm/vpl011.c | 66 +++++++++++++------------------------------
>   1 file changed, 19 insertions(+), 47 deletions(-)
> 
> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> index 1bf803fc1f..80b859baed 100644
> --- a/xen/arch/arm/vpl011.c
> +++ b/xen/arch/arm/vpl011.c
> @@ -354,11 +354,15 @@ static int vpl011_mmio_read(struct vcpu *v,
>       struct domain *d = v->domain;
>       unsigned long flags;
>   
> +    if ( !vpl011_reg32_check_access(dabt) ) {

As I pointed out in the previous version, we don't know the size of the 
registers for the one not described in the SBSA UART. So I don't think 
this check should be consolidated.

Also, coding style:

if ( .... )
{

> +        gprintk(XENLOG_ERR, "vpl011: bad read width %d r%d offset %#08x\n",
> +                dabt.size, dabt.reg, vpl011_reg);
> +        return 0;
> +    }
> +
>       switch ( vpl011_reg )
>       {
>       case DR:
> -        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
> -
>           if ( vpl011->backend_in_domain )
>               *r = vreg_reg32_extract(vpl011_read_data(d), info);
>           else
> @@ -366,31 +370,23 @@ static int vpl011_mmio_read(struct vcpu *v,
>           return 1;
>   
>       case RSR:
> -        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
> -
>           /* It always returns 0 as there are no physical errors. */
>           *r = 0;
>           return 1;
>   
>       case FR:
> -        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
> -
>           VPL011_LOCK(d, flags);
>           *r = vreg_reg32_extract(vpl011->uartfr, info);
>           VPL011_UNLOCK(d, flags);
>           return 1;
>   
>       case RIS:
> -        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
> -
>           VPL011_LOCK(d, flags);
>           *r = vreg_reg32_extract(vpl011->uartris, info);
>           VPL011_UNLOCK(d, flags);
>           return 1;
>   
>       case MIS:
> -        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
> -
>           VPL011_LOCK(d, flags);
>           *r = vreg_reg32_extract(vpl011->uartris & vpl011->uartimsc,
>                                   info);
> @@ -398,40 +394,25 @@ static int vpl011_mmio_read(struct vcpu *v,
>           return 1;
>   
>       case IMSC:
> -        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
> -
>           VPL011_LOCK(d, flags);
>           *r = vreg_reg32_extract(vpl011->uartimsc, info);
>           VPL011_UNLOCK(d, flags);
>           return 1;
>   
>       case ICR:
> -        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
> -
>           /* Only write is valid. */
>           return 0;
>   
>       default:
>           gprintk(XENLOG_ERR, "vpl011: unhandled read r%d offset %#08x\n",
>                   dabt.reg, vpl011_reg);
> -        goto read_as_zero;
> -    }
> -
> -    return 1;
> -
> -read_as_zero:

In general, we don't want to introduce and remove the same code within a 
series. If you don't want to keep read_as_zero, then you should not 
introduce it.

However... I think using the read_as_zero label could still be 
beneficial to reduce the numbers of lines where the registers are RAZ 
(e.g. default, RSR...).

> -    if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
> -
> -    VPL011_LOCK(d, flags);
> -    *r = 0;
> -    VPL011_UNLOCK(d, flags);
> -    return 1;
> -
> -bad_width:
> -    gprintk(XENLOG_ERR, "vpl011: bad read width %d r%d offset %#08x\n",
> -            dabt.size, dabt.reg, vpl011_reg);
> -    return 0;
>   
> +        /* Read as zero */
> +        VPL011_LOCK(d, flags);
> +        *r = 0;
> +        VPL011_UNLOCK(d, flags);
> +        return 1;
> +    }
>   }
>   
>   static int vpl011_mmio_write(struct vcpu *v,
> @@ -446,14 +427,18 @@ static int vpl011_mmio_write(struct vcpu *v,
>       struct domain *d = v->domain;
>       unsigned long flags;
>   
> -    switch ( vpl011_reg )
> +   if ( !vpl011_reg32_check_access(dabt) ) {
> +       gprintk(XENLOG_ERR, "vpl011: bad write width %d r%d offset %#08x\n",
> +               dabt.size, dabt.reg, vpl011_reg);
> +       return 0;
> +   }

Same remarks as for the read part.

> +
> +   switch ( vpl011_reg )
>       {
>       case DR:
>       {
>           uint32_t data = 0;
>   
> -        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
> -
>           vreg_reg32_update(&data, r, info);
>           data &= 0xFF;
>           if ( vpl011->backend_in_domain )
> @@ -464,8 +449,6 @@ static int vpl011_mmio_write(struct vcpu *v,
>       }
>   
>       case RSR: /* Nothing to clear. */
> -        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
> -
>           return 1;
>   
>       case FR:
> @@ -474,8 +457,6 @@ static int vpl011_mmio_write(struct vcpu *v,
>           goto write_ignore;
>   
>       case IMSC:
> -        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
> -
>           VPL011_LOCK(d, flags);
>           vreg_reg32_update(&vpl011->uartimsc, r, info);
>           vpl011_update_interrupt_status(v->domain);
> @@ -483,8 +464,6 @@ static int vpl011_mmio_write(struct vcpu *v,
>           return 1;
>   
>       case ICR:
> -        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
> -
>           VPL011_LOCK(d, flags);
>           vreg_reg32_clearbits(&vpl011->uartris, r, info);
>           vpl011_update_interrupt_status(d);
> @@ -498,14 +477,7 @@ static int vpl011_mmio_write(struct vcpu *v,
>       }
>   
>   write_ignore:
> -    if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
>       return 1;
> -
> -bad_width:
> -    gprintk(XENLOG_ERR, "vpl011: bad write width %d r%d offset %#08x\n",
> -            dabt.size, dabt.reg, vpl011_reg);
> -    return 0;
> -
>   }
>   
>   static const struct mmio_handler_ops vpl011_mmio_handler = {
diff mbox series

Patch

diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
index 1bf803fc1f..80b859baed 100644
--- a/xen/arch/arm/vpl011.c
+++ b/xen/arch/arm/vpl011.c
@@ -354,11 +354,15 @@  static int vpl011_mmio_read(struct vcpu *v,
     struct domain *d = v->domain;
     unsigned long flags;
 
+    if ( !vpl011_reg32_check_access(dabt) ) {
+        gprintk(XENLOG_ERR, "vpl011: bad read width %d r%d offset %#08x\n",
+                dabt.size, dabt.reg, vpl011_reg);
+        return 0;
+    }
+
     switch ( vpl011_reg )
     {
     case DR:
-        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
-
         if ( vpl011->backend_in_domain )
             *r = vreg_reg32_extract(vpl011_read_data(d), info);
         else
@@ -366,31 +370,23 @@  static int vpl011_mmio_read(struct vcpu *v,
         return 1;
 
     case RSR:
-        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
-
         /* It always returns 0 as there are no physical errors. */
         *r = 0;
         return 1;
 
     case FR:
-        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
-
         VPL011_LOCK(d, flags);
         *r = vreg_reg32_extract(vpl011->uartfr, info);
         VPL011_UNLOCK(d, flags);
         return 1;
 
     case RIS:
-        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
-
         VPL011_LOCK(d, flags);
         *r = vreg_reg32_extract(vpl011->uartris, info);
         VPL011_UNLOCK(d, flags);
         return 1;
 
     case MIS:
-        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
-
         VPL011_LOCK(d, flags);
         *r = vreg_reg32_extract(vpl011->uartris & vpl011->uartimsc,
                                 info);
@@ -398,40 +394,25 @@  static int vpl011_mmio_read(struct vcpu *v,
         return 1;
 
     case IMSC:
-        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
-
         VPL011_LOCK(d, flags);
         *r = vreg_reg32_extract(vpl011->uartimsc, info);
         VPL011_UNLOCK(d, flags);
         return 1;
 
     case ICR:
-        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
-
         /* Only write is valid. */
         return 0;
 
     default:
         gprintk(XENLOG_ERR, "vpl011: unhandled read r%d offset %#08x\n",
                 dabt.reg, vpl011_reg);
-        goto read_as_zero;
-    }
-
-    return 1;
-
-read_as_zero:
-    if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
-
-    VPL011_LOCK(d, flags);
-    *r = 0;
-    VPL011_UNLOCK(d, flags);
-    return 1;
-
-bad_width:
-    gprintk(XENLOG_ERR, "vpl011: bad read width %d r%d offset %#08x\n",
-            dabt.size, dabt.reg, vpl011_reg);
-    return 0;
 
+        /* Read as zero */
+        VPL011_LOCK(d, flags);
+        *r = 0;
+        VPL011_UNLOCK(d, flags);
+        return 1;
+    }
 }
 
 static int vpl011_mmio_write(struct vcpu *v,
@@ -446,14 +427,18 @@  static int vpl011_mmio_write(struct vcpu *v,
     struct domain *d = v->domain;
     unsigned long flags;
 
-    switch ( vpl011_reg )
+   if ( !vpl011_reg32_check_access(dabt) ) {
+       gprintk(XENLOG_ERR, "vpl011: bad write width %d r%d offset %#08x\n",
+               dabt.size, dabt.reg, vpl011_reg);
+       return 0;
+   }
+
+   switch ( vpl011_reg )
     {
     case DR:
     {
         uint32_t data = 0;
 
-        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
-
         vreg_reg32_update(&data, r, info);
         data &= 0xFF;
         if ( vpl011->backend_in_domain )
@@ -464,8 +449,6 @@  static int vpl011_mmio_write(struct vcpu *v,
     }
 
     case RSR: /* Nothing to clear. */
-        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
-
         return 1;
 
     case FR:
@@ -474,8 +457,6 @@  static int vpl011_mmio_write(struct vcpu *v,
         goto write_ignore;
 
     case IMSC:
-        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
-
         VPL011_LOCK(d, flags);
         vreg_reg32_update(&vpl011->uartimsc, r, info);
         vpl011_update_interrupt_status(v->domain);
@@ -483,8 +464,6 @@  static int vpl011_mmio_write(struct vcpu *v,
         return 1;
 
     case ICR:
-        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
-
         VPL011_LOCK(d, flags);
         vreg_reg32_clearbits(&vpl011->uartris, r, info);
         vpl011_update_interrupt_status(d);
@@ -498,14 +477,7 @@  static int vpl011_mmio_write(struct vcpu *v,
     }
 
 write_ignore:
-    if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
     return 1;
-
-bad_width:
-    gprintk(XENLOG_ERR, "vpl011: bad write width %d r%d offset %#08x\n",
-            dabt.size, dabt.reg, vpl011_reg);
-    return 0;
-
 }
 
 static const struct mmio_handler_ops vpl011_mmio_handler = {