Re: [Xen-devel] [PATCH V2 2/2] svm: iommu: Only call guest_iommu_init() after initialized HVM domain

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: suravee.suthikulpanit@xxxxxxx
> [mailto:suravee.suthikulpanit@xxxxxxx]
> Sent: 13 May 2016 20:37
> To: xen-devel@xxxxxxxxxxxxx; George Dunlap; jbeulich@xxxxxxxx
> Cc: Paul Durrant; Suravee Suthikulpanit; Suravee Suthikulpanit
> Subject: [PATCH V2 2/2] svm: iommu: Only call guest_iommu_init() after
> initialized HVM domain
> 
> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@xxxxxxx>
> 
> The guest_iommu_init() is currently called by the following code path:
> 
>     arch/x86/domain.c: arch_domain_create()
>       ]- drivers/passthrough/iommu.c: iommu_domain_init()
>         |- drivers/passthrough/amd/pci_amd_iommu.c:
> amd_iommu_domain_init();
>           |- drivers/passthrough/amd/iommu_guest.c: guest_iommu_init()
> 
> At this point, the hvm_domain_initialised() has not been
> called. So register_mmio_handler(), in guest_iommu_init(), silently fails.
> This patch moves the call to guest_iommu_init/destroy() into
> the svm_domain_intialise/_destroy() instead.
> 

That seems wrong. You're taking a call that currently comes via a jump table, 
i.e. an abstraction layer, and calling it directly. Is it possible, instead, to 
move the call to iommu_domain_init() later in arch_domain_create()? It seems 
odd, to me at least, that it's done before hvm_domain_initialise() anyway.

  Paul

> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> ---
>  xen/arch/x86/hvm/svm/svm.c                  | 10 ++++++++++
>  xen/drivers/passthrough/amd/iommu_guest.c   |  6 ++++++
>  xen/drivers/passthrough/amd/pci_amd_iommu.c |  4 ----
>  3 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index e62dfa1..0c4affc 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -45,6 +45,7 @@
>  #include <asm/hvm/support.h>
>  #include <asm/hvm/io.h>
>  #include <asm/hvm/emulate.h>
> +#include <asm/hvm/svm/amd-iommu-proto.h>
>  #include <asm/hvm/svm/asid.h>
>  #include <asm/hvm/svm/svm.h>
>  #include <asm/hvm/svm/vmcb.h>
> @@ -1176,11 +1177,20 @@ void svm_host_osvw_init()
> 
>  static int svm_domain_initialise(struct domain *d)
>  {
> +    if ( is_hvm_domain(d) )
> +        /*
> +         * This requires the hvm domain to be
> +         * initialized first.
> +         */
> +        return guest_iommu_init(d);
> +
>      return 0;
>  }
> 
>  static void svm_domain_destroy(struct domain *d)
>  {
> +    if ( is_hvm_domain(d) )
> +        guest_iommu_destroy(d);
>  }
> 
>  static int svm_vcpu_initialise(struct vcpu *v)
> diff --git a/xen/drivers/passthrough/amd/iommu_guest.c
> b/xen/drivers/passthrough/amd/iommu_guest.c
> index b4e75ac..9f26765 100644
> --- a/xen/drivers/passthrough/amd/iommu_guest.c
> +++ b/xen/drivers/passthrough/amd/iommu_guest.c
> @@ -891,6 +891,12 @@ int guest_iommu_init(struct domain* d)
>           !has_viommu(d) )
>          return 0;
> 
> +    if ( d->arch.hvm_domain.io_handler == NULL )
> +    {
> +        AMD_IOMMU_DEBUG("Error: uninitalized hvm io handler\n");
> +        return 1;
> +    }
> +
>      iommu = xzalloc(struct guest_iommu);
>      if ( !iommu )
>      {
> diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> index c1c0b6b..f791618 100644
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -274,9 +274,6 @@ static int amd_iommu_domain_init(struct domain *d)
>      hd->arch.paging_mode = is_hvm_domain(d) ?
>                        IOMMU_PAGING_MODE_LEVEL_2 :
>                        get_paging_mode(max_page);
> -
> -    guest_iommu_init(d);
> -
>      return 0;
>  }
> 
> @@ -476,7 +473,6 @@ static void deallocate_iommu_page_tables(struct
> domain *d)
> 
>  static void amd_iommu_domain_destroy(struct domain *d)
>  {
> -    guest_iommu_destroy(d);
>      deallocate_iommu_page_tables(d);
>      amd_iommu_flush_all_pages(d);
>  }
> --
> 1.9.1


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