Re: [Xen-devel] [RFC PATCH 9/9] xen: Add use_iommu flag to createdomain domctl

[Oleksandr Tyshchenko]

Hi, Ian

On Wed, Apr 26, 2017 at 1:05 PM, Ian Jackson  wrote:
> Oleksandr Tyshchenko writes ("Re: [RFC PATCH 9/9] xen: Add use_iommu flag to 
> createdomain domctl"):
>> On Tue, Apr 25, 2017 at 6:23 PM, Wei Liu  wrote:
>> > Let me explain where I'm coming from:
>> >
>> > 1. if not populating the iommu page table causes Xen to malfunction
>> >(crash?), it is a bug. It should be fixed without involvement
>> >from toolstack.
>> > 2. if this is an optimasation, can't we not always populate iommu pt
>> >hence no new flag is needed?
>> >
>> > Overall I'm not too convinced a new flag is needed.
>>
>> We don't want to always populate IOMMU page table since it might be
>> just wasting CPU and memory resources if no devices are assigned to
>> domain. That's why we need this hint.
>
> I thought we already had a way to tell libxl that pci passthrough is
> expected for this domain.  There are other things that want to know
> this in advance.  I can't seem to find it now at least in the docs,
> though.
Did mean a way different from the guest configuration file?

>
> Ian.



-- 
Regards,

Oleksandr Tyshchenko

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

Re: [Xen-devel] [RFC PATCH 9/9] xen: Add use_iommu flag to createdomain domctl

[Ian Jackson]

Oleksandr Tyshchenko writes ("Re: [RFC PATCH 9/9] xen: Add use_iommu flag to 
createdomain domctl"):
> On Tue, Apr 25, 2017 at 6:23 PM, Wei Liu  wrote:
> > Let me explain where I'm coming from:
> >
> > 1. if not populating the iommu page table causes Xen to malfunction
> >(crash?), it is a bug. It should be fixed without involvement
> >from toolstack.
> > 2. if this is an optimasation, can't we not always populate iommu pt
> >hence no new flag is needed?
> >
> > Overall I'm not too convinced a new flag is needed.
>
> We don't want to always populate IOMMU page table since it might be
> just wasting CPU and memory resources if no devices are assigned to
> domain. That's why we need this hint.

I thought we already had a way to tell libxl that pci passthrough is
expected for this domain.  There are other things that want to know
this in advance.  I can't seem to find it now at least in the docs,
though.

Ian.

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

Re: [Xen-devel] [RFC PATCH 9/9] xen: Add use_iommu flag to createdomain domctl

[Oleksandr Tyshchenko]

Hi, Wei

On Tue, Apr 25, 2017 at 6:23 PM, Wei Liu  wrote:
> On Wed, Apr 19, 2017 at 07:26:44PM +0100, Julien Grall wrote:
>> Hi Oleksandr,
>>
>> Please CC the appropriate maintainers for all the components you modify.
>>
>> On 15/03/17 20:05, Oleksandr Tyshchenko wrote:
>> > From: Oleksandr Tyshchenko 
>> >
>> > This flag is intended to let Xen know that the guest has devices
>> > which will most likely be used for passthrough.
>> > The primary aim of this knowledge is to help the IOMMUs that don't
>> > share page tables with the CPU be ready before P2M code starts
>> > updating IOMMU mapping.
>> > So, if this flag is set the unshared IOMMUs will populate their
>> > page tables at the domain creation time and thereby will be able
>> > to handle IOMMU mapping updates from *the very beginning*.
>> >
>
> If it is not able to do so since the very beginning, what will happen?
IOMMU page faults will occur if passthroughed/protected devices try to
do DMA since
the corresponding memory mapping won't be present in IOMMU page table.
If we don't populate IOMMU page table (with setting need_iommu flag)
during domain creation time we will lose memory mapping (at least
domain RAM).
Unfortunately, we can't restore this memory mapping on ARM unlike x86.
I would like to say that it is true for non-shared IOMMUs only. For
shared IOMMU (SMMU) we don't have such problem.

>
> Let me explain where I'm coming from:
>
> 1. if not populating the iommu page table causes Xen to malfunction
>(crash?), it is a bug. It should be fixed without involvement
>from toolstack.
> 2. if this is an optimasation, can't we not always populate iommu pt
>hence no new flag is needed?
>
> Overall I'm not too convinced a new flag is needed.
We don't want to always populate IOMMU page table since it might be
just wasting CPU and memory resources if no devices are assigned to
domain. That's why we need this hint.

>
> Wei.



-- 
Regards,

Oleksandr Tyshchenko

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

Re: [Xen-devel] [RFC PATCH 9/9] xen: Add use_iommu flag to createdomain domctl

[Wei Liu]

On Wed, Apr 19, 2017 at 07:26:44PM +0100, Julien Grall wrote:
> Hi Oleksandr,
> 
> Please CC the appropriate maintainers for all the components you modify.
> 
> On 15/03/17 20:05, Oleksandr Tyshchenko wrote:
> > From: Oleksandr Tyshchenko 
> > 
> > This flag is intended to let Xen know that the guest has devices
> > which will most likely be used for passthrough.
> > The primary aim of this knowledge is to help the IOMMUs that don't
> > share page tables with the CPU be ready before P2M code starts
> > updating IOMMU mapping.
> > So, if this flag is set the unshared IOMMUs will populate their
> > page tables at the domain creation time and thereby will be able
> > to handle IOMMU mapping updates from *the very beginning*.
> > 

If it is not able to do so since the very beginning, what will happen?

Let me explain where I'm coming from:

1. if not populating the iommu page table causes Xen to malfunction
   (crash?), it is a bug. It should be fixed without involvement
   from toolstack.
2. if this is an optimasation, can't we not always populate iommu pt
   hence no new flag is needed?

Overall I'm not too convinced a new flag is needed.

Wei.

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

Re: [Xen-devel] [RFC PATCH 9/9] xen: Add use_iommu flag to createdomain domctl

[Oleksandr Tyshchenko]

On Wed, Apr 19, 2017 at 9:26 PM, Julien Grall  wrote:
> Hi Oleksandr,
Hi, Julien

>
> Please CC the appropriate maintainers for all the components you modify.
Sorry, sure.

>
>
> On 15/03/17 20:05, Oleksandr Tyshchenko wrote:
>>
>> From: Oleksandr Tyshchenko 
>>
>> This flag is intended to let Xen know that the guest has devices
>> which will most likely be used for passthrough.
>> The primary aim of this knowledge is to help the IOMMUs that don't
>> share page tables with the CPU be ready before P2M code starts
>> updating IOMMU mapping.
>> So, if this flag is set the unshared IOMMUs will populate their
>> page tables at the domain creation time and thereby will be able
>> to handle IOMMU mapping updates from *the very beginning*.
>>
>> Signed-off-by: Oleksandr Tyshchenko 
>> ---
>>  tools/libxl/libxl_create.c  | 5 +
>>  xen/arch/arm/domain.c   | 4 +++-
>>  xen/arch/x86/domain.c   | 4 +++-
>>  xen/common/domctl.c | 5 -
>>  xen/include/public/domctl.h | 3 +++
>>  xen/include/xen/sched.h | 3 +++
>>  6 files changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
>> index e741b9a..4393fa2 100644
>> --- a/tools/libxl/libxl_create.c
>> +++ b/tools/libxl/libxl_create.c
>> @@ -546,6 +546,11 @@ int libxl__domain_make(libxl__gc *gc,
>> libxl_domain_config *d_config,
>>  flags |= XEN_DOMCTL_CDF_hap;
>>  }
>>
>> +/* TODO Are these assumptions enough to make decision about using
>> IOMMU? */
>> +if ((d_config->num_dtdevs && d_config->dtdevs) ||
>> +(d_config->num_pcidevs && d_config->pcidevs))
>> +flags |= XEN_DOMCTL_CDF_use_iommu;
>
>
> Regardless Jan's comment about the flag, I believe we still want to keep the
> current behavior for x86 (e.g allocating the page table on-demand).
>
> So I think this should be per architecture decision rather than a common
> change. Maybe in, libxl__arch_domain_prepare_config. This also means we
> would switch to xen_arch_domainconfig.
Agree. Will do. So, the use_iommu flag will be included to xen_arch_domainconfig
and passed to iommu_domain_init() on ARM. On x86 we will always pass
the "false" value to
iommu_domain_init(). Right?

>
>> +
>>  /* Ultimately, handle is an array of 16 uint8_t, same as uuid */
>>  libxl_uuid_copy(ctx, (libxl_uuid *)handle, >uuid);
>>
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index bab62ee..940bb98 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -539,6 +539,7 @@ int arch_domain_create(struct domain *d, unsigned int
>> domcr_flags,
>> struct xen_arch_domainconfig *config)
>>  {
>>  int rc, count = 0;
>> +bool_t use_iommu;
>
>
> s/bool_t/bool/
ok

>
>>
>>  BUILD_BUG_ON(GUEST_MAX_VCPUS < MAX_VIRT_CPUS);
>>  d->arch.relmem = RELMEM_not_started;
>> @@ -550,7 +551,8 @@ int arch_domain_create(struct domain *d, unsigned int
>> domcr_flags,
>>  ASSERT(config != NULL);
>>
>>  /* p2m_init relies on some value initialized by the IOMMU subsystem
>> */
>> -if ( (rc = iommu_domain_init(d, false)) != 0 )
>> +use_iommu = !!(domcr_flags & DOMCRF_use_iommu);
>> +if ( (rc = iommu_domain_init(d, use_iommu)) != 0 )
>>  goto fail;
>>
>>  if ( (rc = p2m_init(d)) != 0 )
>> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
>> index 8ef4160..7d634ff 100644
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -525,6 +525,7 @@ int arch_domain_create(struct domain *d, unsigned int
>> domcr_flags,
>>  {
>>  bool paging_initialised = false;
>>  int rc = -ENOMEM;
>> +bool_t use_iommu;
>
>
> Ditto.
ok

>
>
>>
>>  if ( config == NULL && !is_idle_domain(d) )
>>  return -EINVAL;
>> @@ -646,7 +647,8 @@ int arch_domain_create(struct domain *d, unsigned int
>> domcr_flags,
>>  if ( (rc = init_domain_irq_mapping(d)) != 0 )
>>  goto fail;
>>
>> -if ( (rc = iommu_domain_init(d, false)) != 0 )
>> +use_iommu = !!(domcr_flags & DOMCRF_use_iommu);
>> +if ( (rc = iommu_domain_init(d, use_iommu)) != 0 )
>>  goto fail;
>>  }
>>  spin_lock_init(>arch.e820_lock);
>> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
>> index 93e3029..56c4d38 100644
>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c
>> @@ -505,7 +505,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t)
>> u_domctl)
>> | XEN_DOMCTL_CDF_hap
>> | XEN_DOMCTL_CDF_s3_integrity
>> | XEN_DOMCTL_CDF_oos_off
>> -   | XEN_DOMCTL_CDF_xs_domain)) )
>> +   | XEN_DOMCTL_CDF_xs_domain
>> +   | XEN_DOMCTL_CDF_use_iommu)) )
>>  break;
>>
>>  dom = op->domain;
>> @@ -549,6 +550,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t)
>> u_domctl)
>>  domcr_flags |= DOMCRF_oos_off;
>>

Re: [Xen-devel] [RFC PATCH 9/9] xen: Add use_iommu flag to createdomain domctl

[Julien Grall]

Hi Oleksandr,

Please CC the appropriate maintainers for all the components you modify.

On 15/03/17 20:05, Oleksandr Tyshchenko wrote:

From: Oleksandr Tyshchenko 

This flag is intended to let Xen know that the guest has devices
which will most likely be used for passthrough.
The primary aim of this knowledge is to help the IOMMUs that don't
share page tables with the CPU be ready before P2M code starts
updating IOMMU mapping.
So, if this flag is set the unshared IOMMUs will populate their
page tables at the domain creation time and thereby will be able
to handle IOMMU mapping updates from *the very beginning*.

Signed-off-by: Oleksandr Tyshchenko 
---
 tools/libxl/libxl_create.c  | 5 +
 xen/arch/arm/domain.c   | 4 +++-
 xen/arch/x86/domain.c   | 4 +++-
 xen/common/domctl.c | 5 -
 xen/include/public/domctl.h | 3 +++
 xen/include/xen/sched.h | 3 +++
 6 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index e741b9a..4393fa2 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -546,6 +546,11 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config 
*d_config,
 flags |= XEN_DOMCTL_CDF_hap;
 }

+/* TODO Are these assumptions enough to make decision about using IOMMU? */
+if ((d_config->num_dtdevs && d_config->dtdevs) ||
+(d_config->num_pcidevs && d_config->pcidevs))
+flags |= XEN_DOMCTL_CDF_use_iommu;


Regardless Jan's comment about the flag, I believe we still want to keep 
the current behavior for x86 (e.g allocating the page table on-demand).


So I think this should be per architecture decision rather than a common 
change. Maybe in, libxl__arch_domain_prepare_config. This also means we 
would switch to xen_arch_domainconfig.



+
 /* Ultimately, handle is an array of 16 uint8_t, same as uuid */
 libxl_uuid_copy(ctx, (libxl_uuid *)handle, >uuid);

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index bab62ee..940bb98 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -539,6 +539,7 @@ int arch_domain_create(struct domain *d, unsigned int 
domcr_flags,
struct xen_arch_domainconfig *config)
 {
 int rc, count = 0;
+bool_t use_iommu;


s/bool_t/bool/



 BUILD_BUG_ON(GUEST_MAX_VCPUS < MAX_VIRT_CPUS);
 d->arch.relmem = RELMEM_not_started;
@@ -550,7 +551,8 @@ int arch_domain_create(struct domain *d, unsigned int 
domcr_flags,
 ASSERT(config != NULL);

 /* p2m_init relies on some value initialized by the IOMMU subsystem */
-if ( (rc = iommu_domain_init(d, false)) != 0 )
+use_iommu = !!(domcr_flags & DOMCRF_use_iommu);
+if ( (rc = iommu_domain_init(d, use_iommu)) != 0 )
 goto fail;

 if ( (rc = p2m_init(d)) != 0 )
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 8ef4160..7d634ff 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -525,6 +525,7 @@ int arch_domain_create(struct domain *d, unsigned int 
domcr_flags,
 {
 bool paging_initialised = false;
 int rc = -ENOMEM;
+bool_t use_iommu;


Ditto.



 if ( config == NULL && !is_idle_domain(d) )
 return -EINVAL;
@@ -646,7 +647,8 @@ int arch_domain_create(struct domain *d, unsigned int 
domcr_flags,
 if ( (rc = init_domain_irq_mapping(d)) != 0 )
 goto fail;

-if ( (rc = iommu_domain_init(d, false)) != 0 )
+use_iommu = !!(domcr_flags & DOMCRF_use_iommu);
+if ( (rc = iommu_domain_init(d, use_iommu)) != 0 )
 goto fail;
 }
 spin_lock_init(>arch.e820_lock);
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 93e3029..56c4d38 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -505,7 +505,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
u_domctl)
| XEN_DOMCTL_CDF_hap
| XEN_DOMCTL_CDF_s3_integrity
| XEN_DOMCTL_CDF_oos_off
-   | XEN_DOMCTL_CDF_xs_domain)) )
+   | XEN_DOMCTL_CDF_xs_domain
+   | XEN_DOMCTL_CDF_use_iommu)) )
 break;

 dom = op->domain;
@@ -549,6 +550,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
u_domctl)
 domcr_flags |= DOMCRF_oos_off;
 if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_xs_domain )
 domcr_flags |= DOMCRF_xs_domain;
+if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_use_iommu )
+domcr_flags |= DOMCRF_use_iommu;

 d = domain_create(dom, domcr_flags, op->u.createdomain.ssidref,
   >u.createdomain.config);
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 85cbb7c..a37a566 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -66,6 +66,9 @@ struct xen_domctl_createdomain {
  /* Is this a xenstore domain? */
 #define _XEN_DOMCTL_CDF_xs_domain  

Re: [Xen-devel] [RFC PATCH 9/9] xen: Add use_iommu flag to createdomain domctl

[Oleksandr Tyshchenko]

On Fri, Mar 24, 2017 at 1:38 PM, Jan Beulich  wrote:
 On 24.03.17 at 12:19,  wrote:
>> Hi Jan
>>
>> On Thu, Mar 23, 2017 at 7:05 PM, Jan Beulich  wrote:
>> On 23.03.17 at 17:36,  wrote:
 On Wed, Mar 22, 2017 at 5:56 PM, Jan Beulich  wrote:
 On 15.03.17 at 21:05,  wrote:
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -66,6 +66,9 @@ struct xen_domctl_createdomain {
>>   /* Is this a xenstore domain? */
>>  #define _XEN_DOMCTL_CDF_xs_domain 5
>>  #define XEN_DOMCTL_CDF_xs_domain  (1U<<_XEN_DOMCTL_CDF_xs_domain)
>> + /* Should IOMMU page tables be populated at the domain creation time? 
>> */
>> +#define _XEN_DOMCTL_CDF_use_iommu 6
>> +#define XEN_DOMCTL_CDF_use_iommu  (1U<<_XEN_DOMCTL_CDF_use_iommu)
>>  uint32_t flags;
>
> The need for this to be done via domain creation flag (rather than
> as a separate, later step) needs to be well explained. Generally
> what to add here should only be things which can't be done later
> in a reasonable way.

 Well, the non-shared IOMMU should populate its page table by the time
 the P2M code will have started update mappings. Theoretically, it
 might happen right after p2m_init has been completed,
 that is, even during createdomain domctl execution. For example, I see
 that domain_vgic_init() inserts mapping to P2M table, because of
 map_mmio_regions() is being called during VGIC initialization.
 Not sure that GIC mmio ranges must be present in the IOMMU page table,
 but anyway, it might be the per-domain initialization of other IPs,
 co-processors that mapping mustn't be skipped because of IOMMU is not
 ready.
 So, as both iommu_domain_init() and p2m_init() are called from
 arch_domain_create(), i.e. during createdomain domctl execution, we
 have to know exactly should we use IOMMU for this domain to pass
 proper value to iommu_domain_init().
>>>
>>> Well, no, iommu_domain_init() is not supposed to do any table
>>> setup, so it shouldn't need to know.
>>
>> So, does this mean that you disagree to what this patch does as well
>> as the preceding patch
>> [RFC PATCH 6/9] iommu: Pass additional use_iommu argument to
>> iommu_domain_init().
>
> Yes.
>
>> As we need this use_iommu flag on ARM only, the possible solution
>> might be to hide it in struct xen_arch_domainconfig for ARM.
>> In such case we would always call iommu_domain_init() with use_iommu
>> forced to false on x86.
>
> If that won't involve a domain creation flag addition anymore,
> I'd be fine and leave this to the ARM maintainers.

OK.

>
 If don't take into account everything I wrote above, yes, it is
 possible to introduce new domctl for this purpose that will be called
 later, but before any operations with guest_physmap.
>>>
>>> Exactly. Any other things needing syncing, but being done during
>>> domain creation may then need syncing over. One might question
>>> whether some of those things then actually are being done too
>>> early (and quite possibly have been done that way just for simplicity).
>>
>> Sorry, I'm afraid I don't entirely understand you here.
>
> You had mentioned a couple of things where you think you need
> to know ahead of the time whether IOMMU use will actually be
> needed. In turn I question whether these things can't be done
> later, i.e. whether they're being done in their current ordering
> just for convenience of the original code authors.

Now I got it, but I don't have the right answer at the moment.

-- 
Regards,

Oleksandr Tyshchenko

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

Re: [Xen-devel] [RFC PATCH 9/9] xen: Add use_iommu flag to createdomain domctl

[Jan Beulich]

>>> On 24.03.17 at 12:19,  wrote:
> Hi Jan
> 
> On Thu, Mar 23, 2017 at 7:05 PM, Jan Beulich  wrote:
> On 23.03.17 at 17:36,  wrote:
>>> On Wed, Mar 22, 2017 at 5:56 PM, Jan Beulich  wrote:
>>> On 15.03.17 at 21:05,  wrote:
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -66,6 +66,9 @@ struct xen_domctl_createdomain {
>   /* Is this a xenstore domain? */
>  #define _XEN_DOMCTL_CDF_xs_domain 5
>  #define XEN_DOMCTL_CDF_xs_domain  (1U<<_XEN_DOMCTL_CDF_xs_domain)
> + /* Should IOMMU page tables be populated at the domain creation time? */
> +#define _XEN_DOMCTL_CDF_use_iommu 6
> +#define XEN_DOMCTL_CDF_use_iommu  (1U<<_XEN_DOMCTL_CDF_use_iommu)
>  uint32_t flags;

 The need for this to be done via domain creation flag (rather than
 as a separate, later step) needs to be well explained. Generally
 what to add here should only be things which can't be done later
 in a reasonable way.
>>>
>>> Well, the non-shared IOMMU should populate its page table by the time
>>> the P2M code will have started update mappings. Theoretically, it
>>> might happen right after p2m_init has been completed,
>>> that is, even during createdomain domctl execution. For example, I see
>>> that domain_vgic_init() inserts mapping to P2M table, because of
>>> map_mmio_regions() is being called during VGIC initialization.
>>> Not sure that GIC mmio ranges must be present in the IOMMU page table,
>>> but anyway, it might be the per-domain initialization of other IPs,
>>> co-processors that mapping mustn't be skipped because of IOMMU is not
>>> ready.
>>> So, as both iommu_domain_init() and p2m_init() are called from
>>> arch_domain_create(), i.e. during createdomain domctl execution, we
>>> have to know exactly should we use IOMMU for this domain to pass
>>> proper value to iommu_domain_init().
>>
>> Well, no, iommu_domain_init() is not supposed to do any table
>> setup, so it shouldn't need to know.
> 
> So, does this mean that you disagree to what this patch does as well
> as the preceding patch
> [RFC PATCH 6/9] iommu: Pass additional use_iommu argument to
> iommu_domain_init().

Yes.

> As we need this use_iommu flag on ARM only, the possible solution
> might be to hide it in struct xen_arch_domainconfig for ARM.
> In such case we would always call iommu_domain_init() with use_iommu
> forced to false on x86.

If that won't involve a domain creation flag addition anymore,
I'd be fine and leave this to the ARM maintainers.

>>> If don't take into account everything I wrote above, yes, it is
>>> possible to introduce new domctl for this purpose that will be called
>>> later, but before any operations with guest_physmap.
>>
>> Exactly. Any other things needing syncing, but being done during
>> domain creation may then need syncing over. One might question
>> whether some of those things then actually are being done too
>> early (and quite possibly have been done that way just for simplicity).
> 
> Sorry, I'm afraid I don't entirely understand you here.

You had mentioned a couple of things where you think you need
to know ahead of the time whether IOMMU use will actually be
needed. In turn I question whether these things can't be done
later, i.e. whether they're being done in their current ordering
just for convenience of the original code authors.

Jan


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

Re: [Xen-devel] [RFC PATCH 9/9] xen: Add use_iommu flag to createdomain domctl

[Oleksandr Tyshchenko]

Hi Jan

On Thu, Mar 23, 2017 at 7:05 PM, Jan Beulich  wrote:
 On 23.03.17 at 17:36,  wrote:
>> On Wed, Mar 22, 2017 at 5:56 PM, Jan Beulich  wrote:
>> On 15.03.17 at 21:05,  wrote:
 --- a/xen/include/public/domctl.h
 +++ b/xen/include/public/domctl.h
 @@ -66,6 +66,9 @@ struct xen_domctl_createdomain {
   /* Is this a xenstore domain? */
  #define _XEN_DOMCTL_CDF_xs_domain 5
  #define XEN_DOMCTL_CDF_xs_domain  (1U<<_XEN_DOMCTL_CDF_xs_domain)
 + /* Should IOMMU page tables be populated at the domain creation time? */
 +#define _XEN_DOMCTL_CDF_use_iommu 6
 +#define XEN_DOMCTL_CDF_use_iommu  (1U<<_XEN_DOMCTL_CDF_use_iommu)
  uint32_t flags;
>>>
>>> The need for this to be done via domain creation flag (rather than
>>> as a separate, later step) needs to be well explained. Generally
>>> what to add here should only be things which can't be done later
>>> in a reasonable way.
>>
>> Well, the non-shared IOMMU should populate its page table by the time
>> the P2M code will have started update mappings. Theoretically, it
>> might happen right after p2m_init has been completed,
>> that is, even during createdomain domctl execution. For example, I see
>> that domain_vgic_init() inserts mapping to P2M table, because of
>> map_mmio_regions() is being called during VGIC initialization.
>> Not sure that GIC mmio ranges must be present in the IOMMU page table,
>> but anyway, it might be the per-domain initialization of other IPs,
>> co-processors that mapping mustn't be skipped because of IOMMU is not
>> ready.
>> So, as both iommu_domain_init() and p2m_init() are called from
>> arch_domain_create(), i.e. during createdomain domctl execution, we
>> have to know exactly should we use IOMMU for this domain to pass
>> proper value to iommu_domain_init().
>
> Well, no, iommu_domain_init() is not supposed to do any table
> setup, so it shouldn't need to know.

So, does this mean that you disagree to what this patch does as well
as the preceding patch
[RFC PATCH 6/9] iommu: Pass additional use_iommu argument to
iommu_domain_init().
Am I right?

As we need this use_iommu flag on ARM only, the possible solution
might be to hide it in struct xen_arch_domainconfig for ARM.
In such case we would always call iommu_domain_init() with use_iommu
forced to false on x86.

>
>> If don't take into account everything I wrote above, yes, it is
>> possible to introduce new domctl for this purpose that will be called
>> later, but before any operations with guest_physmap.
>
> Exactly. Any other things needing syncing, but being done during
> domain creation may then need syncing over. One might question
> whether some of those things then actually are being done too
> early (and quite possibly have been done that way just for simplicity).

Sorry, I'm afraid I don't entirely understand you here.

-- 
Regards,

Oleksandr Tyshchenko

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

Re: [Xen-devel] [RFC PATCH 9/9] xen: Add use_iommu flag to createdomain domctl

[Jan Beulich]

>>> On 23.03.17 at 17:36,  wrote:
> On Wed, Mar 22, 2017 at 5:56 PM, Jan Beulich  wrote:
> On 15.03.17 at 21:05,  wrote:
>>> --- a/xen/include/public/domctl.h
>>> +++ b/xen/include/public/domctl.h
>>> @@ -66,6 +66,9 @@ struct xen_domctl_createdomain {
>>>   /* Is this a xenstore domain? */
>>>  #define _XEN_DOMCTL_CDF_xs_domain 5
>>>  #define XEN_DOMCTL_CDF_xs_domain  (1U<<_XEN_DOMCTL_CDF_xs_domain)
>>> + /* Should IOMMU page tables be populated at the domain creation time? */
>>> +#define _XEN_DOMCTL_CDF_use_iommu 6
>>> +#define XEN_DOMCTL_CDF_use_iommu  (1U<<_XEN_DOMCTL_CDF_use_iommu)
>>>  uint32_t flags;
>>
>> The need for this to be done via domain creation flag (rather than
>> as a separate, later step) needs to be well explained. Generally
>> what to add here should only be things which can't be done later
>> in a reasonable way.
> 
> Well, the non-shared IOMMU should populate its page table by the time
> the P2M code will have started update mappings. Theoretically, it
> might happen right after p2m_init has been completed,
> that is, even during createdomain domctl execution. For example, I see
> that domain_vgic_init() inserts mapping to P2M table, because of
> map_mmio_regions() is being called during VGIC initialization.
> Not sure that GIC mmio ranges must be present in the IOMMU page table,
> but anyway, it might be the per-domain initialization of other IPs,
> co-processors that mapping mustn't be skipped because of IOMMU is not
> ready.
> So, as both iommu_domain_init() and p2m_init() are called from
> arch_domain_create(), i.e. during createdomain domctl execution, we
> have to know exactly should we use IOMMU for this domain to pass
> proper value to iommu_domain_init().

Well, no, iommu_domain_init() is not supposed to do any table
setup, so it shouldn't need to know.

> If don't take into account everything I wrote above, yes, it is
> possible to introduce new domctl for this purpose that will be called
> later, but before any operations with guest_physmap.

Exactly. Any other things needing syncing, but being done during
domain creation may then need syncing over. One might question
whether some of those things then actually are being done too
early (and quite possibly have been done that way just for simplicity).

Jan


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

Re: [Xen-devel] [RFC PATCH 9/9] xen: Add use_iommu flag to createdomain domctl

[Oleksandr Tyshchenko]

On Wed, Mar 22, 2017 at 5:56 PM, Jan Beulich  wrote:
 On 15.03.17 at 21:05,  wrote:
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -66,6 +66,9 @@ struct xen_domctl_createdomain {
>>   /* Is this a xenstore domain? */
>>  #define _XEN_DOMCTL_CDF_xs_domain 5
>>  #define XEN_DOMCTL_CDF_xs_domain  (1U<<_XEN_DOMCTL_CDF_xs_domain)
>> + /* Should IOMMU page tables be populated at the domain creation time? */
>> +#define _XEN_DOMCTL_CDF_use_iommu 6
>> +#define XEN_DOMCTL_CDF_use_iommu  (1U<<_XEN_DOMCTL_CDF_use_iommu)
>>  uint32_t flags;
>
> The need for this to be done via domain creation flag (rather than
> as a separate, later step) needs to be well explained. Generally
> what to add here should only be things which can't be done later
> in a reasonable way.

Well, the non-shared IOMMU should populate its page table by the time
the P2M code will have started update mappings. Theoretically, it
might happen right after p2m_init has been completed,
that is, even during createdomain domctl execution. For example, I see
that domain_vgic_init() inserts mapping to P2M table, because of
map_mmio_regions() is being called during VGIC initialization.
Not sure that GIC mmio ranges must be present in the IOMMU page table,
but anyway, it might be the per-domain initialization of other IPs,
co-processors that mapping mustn't be skipped because of IOMMU is not
ready.
So, as both iommu_domain_init() and p2m_init() are called from
arch_domain_create(), i.e. during createdomain domctl execution, we
have to know exactly should we use IOMMU for this domain to pass
proper value to iommu_domain_init().

If don't take into account everything I wrote above, yes, it is
possible to introduce new domctl for this purpose that will be called
later, but before any operations with guest_physmap.

-- 
Regards,

Oleksandr Tyshchenko

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

Re: [Xen-devel] [RFC PATCH 9/9] xen: Add use_iommu flag to createdomain domctl

[Jan Beulich]

>>> On 15.03.17 at 21:05,  wrote:
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -66,6 +66,9 @@ struct xen_domctl_createdomain {
>   /* Is this a xenstore domain? */
>  #define _XEN_DOMCTL_CDF_xs_domain 5
>  #define XEN_DOMCTL_CDF_xs_domain  (1U<<_XEN_DOMCTL_CDF_xs_domain)
> + /* Should IOMMU page tables be populated at the domain creation time? */
> +#define _XEN_DOMCTL_CDF_use_iommu 6
> +#define XEN_DOMCTL_CDF_use_iommu  (1U<<_XEN_DOMCTL_CDF_use_iommu)
>  uint32_t flags;

The need for this to be done via domain creation flag (rather than
as a separate, later step) needs to be well explained. Generally
what to add here should only be things which can't be done later
in a reasonable way.

Jan


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

[Xen-devel] [RFC PATCH 9/9] xen: Add use_iommu flag to createdomain domctl

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko 

This flag is intended to let Xen know that the guest has devices
which will most likely be used for passthrough.
The primary aim of this knowledge is to help the IOMMUs that don't
share page tables with the CPU be ready before P2M code starts
updating IOMMU mapping.
So, if this flag is set the unshared IOMMUs will populate their
page tables at the domain creation time and thereby will be able
to handle IOMMU mapping updates from *the very beginning*.

Signed-off-by: Oleksandr Tyshchenko 
---
 tools/libxl/libxl_create.c  | 5 +
 xen/arch/arm/domain.c   | 4 +++-
 xen/arch/x86/domain.c   | 4 +++-
 xen/common/domctl.c | 5 -
 xen/include/public/domctl.h | 3 +++
 xen/include/xen/sched.h | 3 +++
 6 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index e741b9a..4393fa2 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -546,6 +546,11 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config 
*d_config,
 flags |= XEN_DOMCTL_CDF_hap;
 }
 
+/* TODO Are these assumptions enough to make decision about using IOMMU? */
+if ((d_config->num_dtdevs && d_config->dtdevs) ||
+(d_config->num_pcidevs && d_config->pcidevs))
+flags |= XEN_DOMCTL_CDF_use_iommu;
+
 /* Ultimately, handle is an array of 16 uint8_t, same as uuid */
 libxl_uuid_copy(ctx, (libxl_uuid *)handle, >uuid);
 
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index bab62ee..940bb98 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -539,6 +539,7 @@ int arch_domain_create(struct domain *d, unsigned int 
domcr_flags,
struct xen_arch_domainconfig *config)
 {
 int rc, count = 0;
+bool_t use_iommu;
 
 BUILD_BUG_ON(GUEST_MAX_VCPUS < MAX_VIRT_CPUS);
 d->arch.relmem = RELMEM_not_started;
@@ -550,7 +551,8 @@ int arch_domain_create(struct domain *d, unsigned int 
domcr_flags,
 ASSERT(config != NULL);
 
 /* p2m_init relies on some value initialized by the IOMMU subsystem */
-if ( (rc = iommu_domain_init(d, false)) != 0 )
+use_iommu = !!(domcr_flags & DOMCRF_use_iommu);
+if ( (rc = iommu_domain_init(d, use_iommu)) != 0 )
 goto fail;
 
 if ( (rc = p2m_init(d)) != 0 )
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 8ef4160..7d634ff 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -525,6 +525,7 @@ int arch_domain_create(struct domain *d, unsigned int 
domcr_flags,
 {
 bool paging_initialised = false;
 int rc = -ENOMEM;
+bool_t use_iommu;
 
 if ( config == NULL && !is_idle_domain(d) )
 return -EINVAL;
@@ -646,7 +647,8 @@ int arch_domain_create(struct domain *d, unsigned int 
domcr_flags,
 if ( (rc = init_domain_irq_mapping(d)) != 0 )
 goto fail;
 
-if ( (rc = iommu_domain_init(d, false)) != 0 )
+use_iommu = !!(domcr_flags & DOMCRF_use_iommu);
+if ( (rc = iommu_domain_init(d, use_iommu)) != 0 )
 goto fail;
 }
 spin_lock_init(>arch.e820_lock);
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 93e3029..56c4d38 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -505,7 +505,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
u_domctl)
| XEN_DOMCTL_CDF_hap
| XEN_DOMCTL_CDF_s3_integrity
| XEN_DOMCTL_CDF_oos_off
-   | XEN_DOMCTL_CDF_xs_domain)) )
+   | XEN_DOMCTL_CDF_xs_domain
+   | XEN_DOMCTL_CDF_use_iommu)) )
 break;
 
 dom = op->domain;
@@ -549,6 +550,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
u_domctl)
 domcr_flags |= DOMCRF_oos_off;
 if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_xs_domain )
 domcr_flags |= DOMCRF_xs_domain;
+if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_use_iommu )
+domcr_flags |= DOMCRF_use_iommu;
 
 d = domain_create(dom, domcr_flags, op->u.createdomain.ssidref,
   >u.createdomain.config);
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 85cbb7c..a37a566 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -66,6 +66,9 @@ struct xen_domctl_createdomain {
  /* Is this a xenstore domain? */
 #define _XEN_DOMCTL_CDF_xs_domain 5
 #define XEN_DOMCTL_CDF_xs_domain  (1U<<_XEN_DOMCTL_CDF_xs_domain)
+ /* Should IOMMU page tables be populated at the domain creation time? */
+#define _XEN_DOMCTL_CDF_use_iommu 6
+#define XEN_DOMCTL_CDF_use_iommu  (1U<<_XEN_DOMCTL_CDF_use_iommu)
 uint32_t flags;
 struct xen_arch_domainconfig config;
 };
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 0929c0b..80e6fdc 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -561,6