Re: [Xen-devel] [RFC PATCH] arm64: vgic-v3: Add ITS doorbell region in Dom0 stage-2

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Manish,

On 18/01/18 06:15, mjaggi@xxxxxxxxxxxxxxxxxx wrote:
> From: Manish Jaggi <mjaggi@xxxxxxxxxxxxxxxxxx>
>
> This patch introduces a function vgic_map_translation_space for mapping
> ITS 64K translater space for doorbells in dom0 stage-2.
> It is required as dom0 PCI devices behined SMMU  wont be able to write MSIs.
s/behined/behind/

also there a two spaces after SMMU. One should be enough.

> This function is called from vgic_v3_its_init_domain only for hardware domain.
This is really confusing. You mix dom0 and hardware domain in the same 
commit. Please use only "hardware domain".
> This patch is also required for testing
> [RFC 00/11] acpi: arm: IORT Support for Xen [1]
> + [2]  [RFC v4 0/8] SMMUv3 driver
>   
> [1] https://lists.xenproject.org/archives/html/xen-devel/2018-01/msg00007.html
> [2] https://lists.xen.org/archives/html/xen-devel/2017-12/msg01294.html
Anything after "This patch..." does not seem to be relevant in the 
commit. You might want to add that after --- so it get stripped after 
committing.
> Signed-off-by: Manish Jaggi <manish.jaggi@xxxxxxxxxx>
> ---
>   xen/arch/arm/vgic-v3-its.c | 26 ++++++++++++++++++++++++++
>   1 file changed, 26 insertions(+)
>
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index 72a5c70656..a8f8079149 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -1511,6 +1511,30 @@ unsigned int vgic_v3_its_count(const struct domain *d)
>       return ret;
>   }
>   
> +static int vgic_map_translation_space(struct domain *d, paddr_t guest_addr)
This function is quite confusing. What is guest_addr? It looks like it 
is the base address of the ITS but it is not very clear here.
> +{
> +    u64 addr, size;
This should be paddr_t.

> +    int ret;
Newline here please.

> +    addr = guest_addr + SZ_64K;
> +    size = SZ_64K;
> +
> +    ret = map_mmio_regions(d,
> +                           paddr_to_pfn(addr & PAGE_MASK),
> +                           DIV_ROUND_UP(size, PAGE_SIZE),
> +                           paddr_to_pfn(addr & PAGE_MASK));
You are assuming a 1:1 mapping in the domain. Nowhere you explain that 
assumption nor that it will not work for guest. At least you need an 
ASSERT(is_domain_direct_mapped(d)).
Furthermore, as we discussed yesterday I would expect a big fat comment 
that this need to be revisited when DomU support will be added as it may 
not be safe to map the Doorbell in page-tables used by the processor.
> +
> +    if ( ret )
> +    {
> +        printk(XENLOG_ERR "Unable to map to dom%d access to"
> +               " 0x%"PRIx64" - 0x%"PRIx64"\n",
> +               d->domain_id,
> +               addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
> +    }
> +
> +    return ret;
> +
> +}
> +
>   /*
>    * For a hardware domain, this will iterate over the host ITSes
>    * and map one virtual ITS per host ITS at the same address.
> @@ -1541,6 +1565,8 @@ int vgic_v3_its_init_domain(struct domain *d)
>                   return ret;
>               else
>                   d->arch.vgic.has_its = true;
> +
> +            vgic_map_translation_space(d, hw_its->addr);
Your new function may return an error. Therefore you should test it.

>           }
>       }
>   
Cheers,

--
Julien Grall

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