Re: [Xen-devel] [PATCH v5 3/5] ARM: ITS: Deny hardware domain access to ITS

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Manish,

I just spotted an issue in this patch.

On 10/10/17 07:16, mjaggi@xxxxxxxxxxxxxxxxxx wrote:
> From: Manish Jaggi <mjaggi@xxxxxxxxxx>
>
> This patch extends the gicv3_iomem_deny_access functionality by adding
> support for ITS region as well. Add function gicv3_its_deny_access.
>
> Reviewed-by: Andre Przywara <andre.przywara@xxxxxxx>
> Acked-by: Julien Grall <julien.grall@xxxxxxx>
> Signed-off-by: Manish Jaggi <mjaggi@xxxxxxxxxx>
> ---
>   xen/arch/arm/gic-v3-its.c        | 22 ++++++++++++++++++++++
>   xen/arch/arm/gic-v3.c            |  3 +++
>   xen/include/asm-arm/gic_v3_its.h |  9 +++++++++
>   3 files changed, 34 insertions(+)
>
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 3023ee5..7746ae8 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -21,6 +21,7 @@
>   #include <xen/acpi.h>
>   #include <xen/lib.h>
>   #include <xen/delay.h>
> +#include <xen/iocap.h>
>   #include <xen/libfdt/libfdt.h>
>   #include <xen/mm.h>
>   #include <xen/rbtree.h>
> @@ -905,6 +906,27 @@ struct pending_irq *gicv3_assign_guest_event(struct domain 
> *d,
>       return pirq;
>   }
>   
> +int gicv3_its_deny_access(const struct domain *d)
> +{
> +    int rc = 0;
> +    unsigned long mfn, nr;
> +    const struct host_its *its_data;
> +
> +    list_for_each_entry( its_data, &host_its_list, entry )
> +    {
> +        mfn = paddr_to_pfn(its_data->addr);
> +        nr = PFN_UP(GICV3_ITS_SIZE);
While you fix the bug (see below), please use its_data->size here.

> +        rc = iomem_deny_access(d, mfn, mfn + nr);
> +        if ( rc )
> +        {
> +            printk( "iomem_deny_access failed for %lx:%lx \r\n", mfn, nr);
And here the spurious space between ( and ".

> +            break;
> +        }
> +    }
> +
> +    return rc;
> +}
> +
>   /*
>    * Create the respective guest DT nodes from a list of host ITSes.
>    * This copies the reg property, so the guest sees the ITS at the same address
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 6f562f4..b3d605d 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1308,6 +1308,9 @@ static int gicv3_iomem_deny_access(const struct domain *d)
>       if ( rc )
>           return rc;
>   
> +    if ( gicv3_its_deny_access(d) )
> +        return rc;
you return rc here in case of an error, however you don't set it before. 
So this will always return 0 and Xen will continue to boot as nothing 
happen.
The best way to fix it would be:

rc = gicv3_its_deny_access(d);
if ( rc )
  return rc;

> +
>       for ( i = 0; i < gicv3.rdist_count; i++ )
>       {
>           mfn = gicv3.rdist_regions[i].base >> PAGE_SHIFT;
> diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h
> index e1be33c..31fca66 100644
> --- a/xen/include/asm-arm/gic_v3_its.h
> +++ b/xen/include/asm-arm/gic_v3_its.h
> @@ -139,6 +139,10 @@ void gicv3_its_dt_init(const struct dt_device_node *node);
>   #ifdef CONFIG_ACPI
>   void gicv3_its_acpi_init(void);
>   #endif
> +
> +/* Deny iomem access for its */
> +int gicv3_its_deny_access(const struct domain *d);
> +
>   bool gicv3_its_host_has_its(void);
>   
>   unsigned int vgic_v3_its_count(const struct domain *d);
> @@ -206,6 +210,11 @@ static inline void gicv3_its_acpi_init(void)
>   }
>   #endif
>   
> +static inline int gicv3_its_deny_access(const struct domain *d)
> +{
> +    return 0;
> +}
> +
>   static inline bool gicv3_its_host_has_its(void)
>   {
>       return false;
Cheers,

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel