Re: [Xen-devel] [PATCH 14/15] xen/arm: guest_walk_tables: Switch the return to bool
On Mon, 16 Jul 2018, Julien Grall wrote:
> At the moment, guest_walk_tables can either return 0, -EFAULT, -EINVAL.
> The use of the last 2 are not clearly defined and used inconsistently in
> the code. The current only caller does not care about the return
> value and the value of it seems very limited (no way to differentiate
> between the 15ish error paths).
>
> So switch to bool to simplify the return and make the developper life a
^ developer
> bit easier.
>
> Signed-off-by: Julien Grall
Aside from the minor typo:
Reviewed-by: Stefano Stabellini
> ---
> xen/arch/arm/guest_walk.c| 50
>
> xen/arch/arm/mem_access.c| 2 +-
> xen/include/asm-arm/guest_walk.h | 8 +++
> 3 files changed, 30 insertions(+), 30 deletions(-)
>
> diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
> index 4a1b4cf2c8..7db7a7321b 100644
> --- a/xen/arch/arm/guest_walk.c
> +++ b/xen/arch/arm/guest_walk.c
> @@ -28,9 +28,9 @@
> * page table on a different vCPU, the following registers would need to be
> * loaded: TCR_EL1, TTBR0_EL1, TTBR1_EL1, and SCTLR_EL1.
> */
> -static int guest_walk_sd(const struct vcpu *v,
> - vaddr_t gva, paddr_t *ipa,
> - unsigned int *perms)
> +static bool guest_walk_sd(const struct vcpu *v,
> + vaddr_t gva, paddr_t *ipa,
> + unsigned int *perms)
> {
> int ret;
> bool disabled = true;
> @@ -79,7 +79,7 @@ static int guest_walk_sd(const struct vcpu *v,
> }
>
> if ( disabled )
> -return -EFAULT;
> +return false;
>
> /*
> * The address of the L1 descriptor for the initial lookup has the
> @@ -97,12 +97,12 @@ static int guest_walk_sd(const struct vcpu *v,
> /* Access the guest's memory to read only one PTE. */
> ret = access_guest_memory_by_ipa(d, paddr, , sizeof(short_desc_t),
> false);
> if ( ret )
> -return -EINVAL;
> +return false;
>
> switch ( pte.walk.dt )
> {
> case L1DESC_INVALID:
> -return -EFAULT;
> +return false;
>
> case L1DESC_PAGE_TABLE:
> /*
> @@ -122,10 +122,10 @@ static int guest_walk_sd(const struct vcpu *v,
> /* Access the guest's memory to read only one PTE. */
> ret = access_guest_memory_by_ipa(d, paddr, ,
> sizeof(short_desc_t), false);
> if ( ret )
> -return -EINVAL;
> +return false;
>
> if ( pte.walk.dt == L2DESC_INVALID )
> -return -EFAULT;
> +return false;
>
> if ( pte.pg.page ) /* Small page. */
> {
> @@ -175,7 +175,7 @@ static int guest_walk_sd(const struct vcpu *v,
> *perms |= GV2M_EXEC;
> }
>
> -return 0;
> +return true;
> }
>
> /*
> @@ -355,9 +355,9 @@ static bool check_base_size(unsigned int output_size,
> uint64_t base)
> * page table on a different vCPU, the following registers would need to be
> * loaded: TCR_EL1, TTBR0_EL1, TTBR1_EL1, and SCTLR_EL1.
> */
> -static int guest_walk_ld(const struct vcpu *v,
> - vaddr_t gva, paddr_t *ipa,
> - unsigned int *perms)
> +static bool guest_walk_ld(const struct vcpu *v,
> + vaddr_t gva, paddr_t *ipa,
> + unsigned int *perms)
> {
> int ret;
> bool disabled = true;
> @@ -442,7 +442,7 @@ static int guest_walk_ld(const struct vcpu *v,
> */
> if ( (input_size > TCR_EL1_IPS_48_BIT_VAL) ||
> (input_size < TCR_EL1_IPS_MIN_VAL) )
> -return -EFAULT;
> +return false;
> }
> else
> {
> @@ -487,7 +487,7 @@ static int guest_walk_ld(const struct vcpu *v,
> }
>
> if ( disabled )
> -return -EFAULT;
> +return false;
>
> /*
> * The starting level is the number of strides (grainsizes[gran] - 3)
> @@ -498,12 +498,12 @@ static int guest_walk_ld(const struct vcpu *v,
> /* Get the IPA output_size. */
> ret = get_ipa_output_size(d, tcr, _size);
> if ( ret )
> -return -EFAULT;
> +return false;
>
> /* Make sure the base address does not exceed its configured size. */
> ret = check_base_size(output_size, ttbr);
> if ( !ret )
> -return -EFAULT;
> +return false;
>
> /*
> * Compute the base address of the first level translation table that is
> @@ -523,12 +523,12 @@ static int guest_walk_ld(const struct vcpu *v,
> /* Access the guest's memory to read only one PTE. */
> ret = access_guest_memory_by_ipa(d, paddr, , sizeof(lpae_t),
> false);
> if ( ret )
> -return -EFAULT;
> +return false;
>
> /* Make sure the base address does not exceed its configured size. */
> ret =
[Xen-devel] [PATCH 14/15] xen/arm: guest_walk_tables: Switch the return to bool
At the moment, guest_walk_tables can either return 0, -EFAULT, -EINVAL.
The use of the last 2 are not clearly defined and used inconsistently in
the code. The current only caller does not care about the return
value and the value of it seems very limited (no way to differentiate
between the 15ish error paths).
So switch to bool to simplify the return and make the developper life a
bit easier.
Signed-off-by: Julien Grall
---
xen/arch/arm/guest_walk.c| 50
xen/arch/arm/mem_access.c| 2 +-
xen/include/asm-arm/guest_walk.h | 8 +++
3 files changed, 30 insertions(+), 30 deletions(-)
diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
index 4a1b4cf2c8..7db7a7321b 100644
--- a/xen/arch/arm/guest_walk.c
+++ b/xen/arch/arm/guest_walk.c
@@ -28,9 +28,9 @@
* page table on a different vCPU, the following registers would need to be
* loaded: TCR_EL1, TTBR0_EL1, TTBR1_EL1, and SCTLR_EL1.
*/
-static int guest_walk_sd(const struct vcpu *v,
- vaddr_t gva, paddr_t *ipa,
- unsigned int *perms)
+static bool guest_walk_sd(const struct vcpu *v,
+ vaddr_t gva, paddr_t *ipa,
+ unsigned int *perms)
{
int ret;
bool disabled = true;
@@ -79,7 +79,7 @@ static int guest_walk_sd(const struct vcpu *v,
}
if ( disabled )
-return -EFAULT;
+return false;
/*
* The address of the L1 descriptor for the initial lookup has the
@@ -97,12 +97,12 @@ static int guest_walk_sd(const struct vcpu *v,
/* Access the guest's memory to read only one PTE. */
ret = access_guest_memory_by_ipa(d, paddr, , sizeof(short_desc_t),
false);
if ( ret )
-return -EINVAL;
+return false;
switch ( pte.walk.dt )
{
case L1DESC_INVALID:
-return -EFAULT;
+return false;
case L1DESC_PAGE_TABLE:
/*
@@ -122,10 +122,10 @@ static int guest_walk_sd(const struct vcpu *v,
/* Access the guest's memory to read only one PTE. */
ret = access_guest_memory_by_ipa(d, paddr, , sizeof(short_desc_t),
false);
if ( ret )
-return -EINVAL;
+return false;
if ( pte.walk.dt == L2DESC_INVALID )
-return -EFAULT;
+return false;
if ( pte.pg.page ) /* Small page. */
{
@@ -175,7 +175,7 @@ static int guest_walk_sd(const struct vcpu *v,
*perms |= GV2M_EXEC;
}
-return 0;
+return true;
}
/*
@@ -355,9 +355,9 @@ static bool check_base_size(unsigned int output_size,
uint64_t base)
* page table on a different vCPU, the following registers would need to be
* loaded: TCR_EL1, TTBR0_EL1, TTBR1_EL1, and SCTLR_EL1.
*/
-static int guest_walk_ld(const struct vcpu *v,
- vaddr_t gva, paddr_t *ipa,
- unsigned int *perms)
+static bool guest_walk_ld(const struct vcpu *v,
+ vaddr_t gva, paddr_t *ipa,
+ unsigned int *perms)
{
int ret;
bool disabled = true;
@@ -442,7 +442,7 @@ static int guest_walk_ld(const struct vcpu *v,
*/
if ( (input_size > TCR_EL1_IPS_48_BIT_VAL) ||
(input_size < TCR_EL1_IPS_MIN_VAL) )
-return -EFAULT;
+return false;
}
else
{
@@ -487,7 +487,7 @@ static int guest_walk_ld(const struct vcpu *v,
}
if ( disabled )
-return -EFAULT;
+return false;
/*
* The starting level is the number of strides (grainsizes[gran] - 3)
@@ -498,12 +498,12 @@ static int guest_walk_ld(const struct vcpu *v,
/* Get the IPA output_size. */
ret = get_ipa_output_size(d, tcr, _size);
if ( ret )
-return -EFAULT;
+return false;
/* Make sure the base address does not exceed its configured size. */
ret = check_base_size(output_size, ttbr);
if ( !ret )
-return -EFAULT;
+return false;
/*
* Compute the base address of the first level translation table that is
@@ -523,12 +523,12 @@ static int guest_walk_ld(const struct vcpu *v,
/* Access the guest's memory to read only one PTE. */
ret = access_guest_memory_by_ipa(d, paddr, , sizeof(lpae_t),
false);
if ( ret )
-return -EFAULT;
+return false;
/* Make sure the base address does not exceed its configured size. */
ret = check_base_size(output_size, pfn_to_paddr(pte.walk.base));
if ( !ret )
-return -EFAULT;
+return false;
/*
* If page granularity is 64K, make sure the address is aligned
@@ -537,7 +537,7 @@ static int guest_walk_ld(const struct vcpu *v,
if ( (output_size < TCR_EL1_IPS_52_BIT_VAL) &&
(gran == GRANULE_SIZE_INDEX_64K) &&
(pte.walk.base & 0xf) )
-return -EFAULT;
+
