User could do device passthrough, with "xen,force-assign-without-iommu" in
the device tree snippet, on trusted guest through 1:1 direct-map,
if IOMMU absent or disabled on hardware.

In order to achieve that, this patch adds 1:1 direct-map check and disables
iommu-related action.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
 xen/arch/arm/domain_build.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index c92e510ae7..9a9d2522b7 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2070,14 +2070,18 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo,
     if ( res < 0 )
         return res;
 
+    /*
+     * If xen_force, we allow assignment of devices without IOMMU protection.
+     * And if IOMMU is disabled or absent, 1:1 direct-map is necessary
+     */
+    if ( xen_force && is_domain_direct_mapped(kinfo->d) &&
+         !dt_device_is_protected(node) )
+        return 0;
+
     res = iommu_add_dt_device(node);
     if ( res < 0 )
         return res;
 
-    /* If xen_force, we allow assignment of devices without IOMMU protection. */
-    if ( xen_force && !dt_device_is_protected(node) )
-        return 0;
-
     return iommu_assign_dt_device(kinfo->d, node);
 }
 
-- 
2.25.1

Hi,

On 23/09/2021 08:11, Penny Zheng wrote:
> User could do device passthrough, with "xen,force-assign-without-iommu" in
> the device tree snippet, on trusted guest through 1:1 direct-map,
> if IOMMU absent or disabled on hardware.

At the moment, it would be possible to passthrough a non-DMA capable 
device with direct-mapping. After this patch, this is going to be forbidden.

> 
> In order to achieve that, this patch adds 1:1 direct-map check and disables
> iommu-related action.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
>   xen/arch/arm/domain_build.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index c92e510ae7..9a9d2522b7 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2070,14 +2070,18 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo,
>       if ( res < 0 )
>           return res;
>   
> +    /*
> +     * If xen_force, we allow assignment of devices without IOMMU protection.
> +     * And if IOMMU is disabled or absent, 1:1 direct-map is necessary > +     */
> +    if ( xen_force && is_domain_direct_mapped(kinfo->d) &&
> +         !dt_device_is_protected(node) )

dt_device_is_protected() will be always false unless the device is 
protected behing an SMMU using the legacy binding. So I don't think this 
is correct to move this check ahead. In fact..

> +        return 0;
> +
>       res = iommu_add_dt_device(node);

... the call should already be a NOP when the IOMMU is disabled or the 
device is not behind an IOMMU. So can you explain what you are trying to 
prevent here?

>       if ( res < 0 )
>           return res;
>   
> -    /* If xen_force, we allow assignment of devices without IOMMU protection. */
> -    if ( xen_force && !dt_device_is_protected(node) )
> -        return 0;
> -
>       return iommu_assign_dt_device(kinfo->d, node);
>   }
>   
> 

Cheers,

-- 
Julien Grall

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Thursday, September 23, 2021 7:27 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>
> Subject: Re: [PATCH 10/11] xen/arm: device assignment on 1:1 direct-map
> domain
> 
> Hi,
> 
> On 23/09/2021 08:11, Penny Zheng wrote:
> > User could do device passthrough, with
> > "xen,force-assign-without-iommu" in the device tree snippet, on
> > trusted guest through 1:1 direct-map, if IOMMU absent or disabled on
> hardware.
> 
> At the moment, it would be possible to passthrough a non-DMA capable
> device with direct-mapping. After this patch, this is going to be forbidden.
> 
> >
> > In order to achieve that, this patch adds 1:1 direct-map check and
> > disables iommu-related action.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> >   xen/arch/arm/domain_build.c | 12 ++++++++----
> >   1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index c92e510ae7..9a9d2522b7 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -2070,14 +2070,18 @@ static int __init
> handle_passthrough_prop(struct kernel_info *kinfo,
> >       if ( res < 0 )
> >           return res;
> >
> > +    /*
> > +     * If xen_force, we allow assignment of devices without IOMMU
> protection.
> > +     * And if IOMMU is disabled or absent, 1:1 direct-map is necessary > +
> */
> > +    if ( xen_force && is_domain_direct_mapped(kinfo->d) &&
> > +         !dt_device_is_protected(node) )
> 
> dt_device_is_protected() will be always false unless the device is protected
> behing an SMMU using the legacy binding. So I don't think this is correct to
> move this check ahead. In fact..
> 
> > +        return 0;
> > +
> >       res = iommu_add_dt_device(node);
> 
> ... the call should already be a NOP when the IOMMU is disabled or the
> device is not behind an IOMMU. So can you explain what you are trying to
> prevent here?
> 

If the IOMMU is disabled, iommu_add_dt_device will return 1 as errno. 
So we could not make it to the xen_force check...

So I tried to move all IOMMU action behind xen_force check.

Now, device assignment without IOMMU protection is only
applicable on direct-map domains, so this commit also adds
is_domain_direct_mapped check together with xen_force check.

> >       if ( res < 0 )
> >           return res;
> >
> > -    /* If xen_force, we allow assignment of devices without IOMMU
> protection. */
> > -    if ( xen_force && !dt_device_is_protected(node) )
> > -        return 0;
> > -
> >       return iommu_assign_dt_device(kinfo->d, node);
> >   }
> >
> >
> 
> Cheers,
> 
> --
> Julien Grall

On 09/10/2021 10:40, Penny Zheng wrote:
> Hi Julien

Hi Penny,

> 
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: Thursday, September 23, 2021 7:27 PM
>> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
>> sstabellini@kernel.org
>> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
>> <Wei.Chen@arm.com>
>> Subject: Re: [PATCH 10/11] xen/arm: device assignment on 1:1 direct-map
>> domain
>>
>> Hi,
>>
>> On 23/09/2021 08:11, Penny Zheng wrote:
>>> User could do device passthrough, with
>>> "xen,force-assign-without-iommu" in the device tree snippet, on
>>> trusted guest through 1:1 direct-map, if IOMMU absent or disabled on
>> hardware.
>>
>> At the moment, it would be possible to passthrough a non-DMA capable
>> device with direct-mapping. After this patch, this is going to be forbidden.
>>
>>>
>>> In order to achieve that, this patch adds 1:1 direct-map check and
>>> disables iommu-related action.
>>>
>>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
>>> ---
>>>    xen/arch/arm/domain_build.c | 12 ++++++++----
>>>    1 file changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index c92e510ae7..9a9d2522b7 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -2070,14 +2070,18 @@ static int __init
>> handle_passthrough_prop(struct kernel_info *kinfo,
>>>        if ( res < 0 )
>>>            return res;
>>>
>>> +    /*
>>> +     * If xen_force, we allow assignment of devices without IOMMU
>> protection.
>>> +     * And if IOMMU is disabled or absent, 1:1 direct-map is necessary > +
>> */
>>> +    if ( xen_force && is_domain_direct_mapped(kinfo->d) &&
>>> +         !dt_device_is_protected(node) )
>>
>> dt_device_is_protected() will be always false unless the device is protected
>> behing an SMMU using the legacy binding. So I don't think this is correct to
>> move this check ahead. In fact..
>>
>>> +        return 0;
>>> +
>>>        res = iommu_add_dt_device(node);
>>
>> ... the call should already be a NOP when the IOMMU is disabled or the
>> device is not behind an IOMMU. So can you explain what you are trying to
>> prevent here?
>>
> 
> If the IOMMU is disabled, iommu_add_dt_device will return 1 as errno.
> So we could not make it to the xen_force check...

I disagree. The check is:

if ( res < 0 )
   return res;

Given that res is 1, we wouldn't return and move to check whether the 
assignment can be done.

> 
> So I tried to move all IOMMU action behind xen_force check.
> 
> Now, device assignment without IOMMU protection is only
> applicable on direct-map domains,

It is fine to assign a non-DMA capable device without direct-mapping. So 
why do you want to add this restriction?

Cheers,

-- 
Julien Grall

Hi julien

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Monday, October 11, 2021 7:14 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>
> Subject: Re: [PATCH 10/11] xen/arm: device assignment on 1:1 direct-map
> domain
> 
> 
> 
> On 09/10/2021 10:40, Penny Zheng wrote:
> > Hi Julien
> 
> Hi Penny,
> 
> >
> >> -----Original Message-----
> >> From: Julien Grall <julien@xen.org>
> >> Sent: Thursday, September 23, 2021 7:27 PM
> >> To: Penny Zheng <Penny.Zheng@arm.com>;
> >> xen-devel@lists.xenproject.org; sstabellini@kernel.org
> >> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> >> <Wei.Chen@arm.com>
> >> Subject: Re: [PATCH 10/11] xen/arm: device assignment on 1:1
> >> direct-map domain
> >>
> >> Hi,
> >>
> >> On 23/09/2021 08:11, Penny Zheng wrote:
> >>> User could do device passthrough, with
> >>> "xen,force-assign-without-iommu" in the device tree snippet, on
> >>> trusted guest through 1:1 direct-map, if IOMMU absent or disabled on
> >> hardware.
> >>
> >> At the moment, it would be possible to passthrough a non-DMA capable
> >> device with direct-mapping. After this patch, this is going to be forbidden.
> >>
> >>>
> >>> In order to achieve that, this patch adds 1:1 direct-map check and
> >>> disables iommu-related action.
> >>>
> >>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> >>> ---
> >>>    xen/arch/arm/domain_build.c | 12 ++++++++----
> >>>    1 file changed, 8 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/xen/arch/arm/domain_build.c
> >>> b/xen/arch/arm/domain_build.c index c92e510ae7..9a9d2522b7 100644
> >>> --- a/xen/arch/arm/domain_build.c
> >>> +++ b/xen/arch/arm/domain_build.c
> >>> @@ -2070,14 +2070,18 @@ static int __init
> >> handle_passthrough_prop(struct kernel_info *kinfo,
> >>>        if ( res < 0 )
> >>>            return res;
> >>>
> >>> +    /*
> >>> +     * If xen_force, we allow assignment of devices without IOMMU
> >> protection.
> >>> +     * And if IOMMU is disabled or absent, 1:1 direct-map is
> >>> + necessary > +
> >> */
> >>> +    if ( xen_force && is_domain_direct_mapped(kinfo->d) &&
> >>> +         !dt_device_is_protected(node) )
> >>
> >> dt_device_is_protected() will be always false unless the device is
> >> protected behing an SMMU using the legacy binding. So I don't think
> >> this is correct to move this check ahead. In fact..
> >>
> >>> +        return 0;
> >>> +
> >>>        res = iommu_add_dt_device(node);
> >>
> >> ... the call should already be a NOP when the IOMMU is disabled or
> >> the device is not behind an IOMMU. So can you explain what you are
> >> trying to prevent here?
> >>
> >
> > If the IOMMU is disabled, iommu_add_dt_device will return 1 as errno.
> > So we could not make it to the xen_force check...
> 
> I disagree. The check is:
> 
> if ( res < 0 )
>    return res;
> 
> Given that res is 1, we wouldn't return and move to check whether the
> assignment can be done.
> 
> >
> > So I tried to move all IOMMU action behind xen_force check.
> >
> > Now, device assignment without IOMMU protection is only applicable on
> > direct-map domains,
> 
> It is fine to assign a non-DMA capable device without direct-mapping. So why
> do you want to add this restriction?

I understand.
If it is fine to assign a non-DMA capable device without direct-mapping, the former
commit containing the following changes needs fix.

         if ( dt_find_compatible_node(node, NULL, "multiboot,device-tree") )
        {
            if ( iommu_enabled )
                d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
            else if ( d_cfg.flags & XEN_DOMCTL_INTERNAL_directmap )
                warning_add("Please be sure of having trusted guests, when doing device assignment without IOMMU protection\n");
            else
                panic("Assign a device but IOMMU and direct-map are all disabled\n");
        }

Definitely no panic when IOMMU and direct-map are all disabled.

> 
> Cheers,
> 
> --
> Julien Grall

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Monday, October 11, 2021 7:14 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>
> Subject: Re: [PATCH 10/11] xen/arm: device assignment on 1:1 direct-map
> domain
> 
> 
> 
> On 09/10/2021 10:40, Penny Zheng wrote:
> > Hi Julien
> 
> Hi Penny,
> 
> >
> >> -----Original Message-----
> >> From: Julien Grall <julien@xen.org>
> >> Sent: Thursday, September 23, 2021 7:27 PM
> >> To: Penny Zheng <Penny.Zheng@arm.com>;
> >> xen-devel@lists.xenproject.org; sstabellini@kernel.org
> >> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> >> <Wei.Chen@arm.com>
> >> Subject: Re: [PATCH 10/11] xen/arm: device assignment on 1:1
> >> direct-map domain
> >>
> >> Hi,
> >>
> >> On 23/09/2021 08:11, Penny Zheng wrote:
> >>> User could do device passthrough, with
> >>> "xen,force-assign-without-iommu" in the device tree snippet, on
> >>> trusted guest through 1:1 direct-map, if IOMMU absent or disabled on
> >> hardware.
> >>
> >> At the moment, it would be possible to passthrough a non-DMA capable
> >> device with direct-mapping. After this patch, this is going to be forbidden.
> >>
> >>>
> >>> In order to achieve that, this patch adds 1:1 direct-map check and
> >>> disables iommu-related action.
> >>>
> >>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> >>> ---
> >>>    xen/arch/arm/domain_build.c | 12 ++++++++----
> >>>    1 file changed, 8 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/xen/arch/arm/domain_build.c
> >>> b/xen/arch/arm/domain_build.c index c92e510ae7..9a9d2522b7 100644
> >>> --- a/xen/arch/arm/domain_build.c
> >>> +++ b/xen/arch/arm/domain_build.c
> >>> @@ -2070,14 +2070,18 @@ static int __init
> >> handle_passthrough_prop(struct kernel_info *kinfo,
> >>>        if ( res < 0 )
> >>>            return res;
> >>>
> >>> +    /*
> >>> +     * If xen_force, we allow assignment of devices without IOMMU
> >> protection.
> >>> +     * And if IOMMU is disabled or absent, 1:1 direct-map is
> >>> + necessary > +
> >> */
> >>> +    if ( xen_force && is_domain_direct_mapped(kinfo->d) &&
> >>> +         !dt_device_is_protected(node) )
> >>
> >> dt_device_is_protected() will be always false unless the device is
> >> protected behing an SMMU using the legacy binding. So I don't think
> >> this is correct to move this check ahead. In fact..
> >>
> >>> +        return 0;
> >>> +
> >>>        res = iommu_add_dt_device(node);
> >>
> >> ... the call should already be a NOP when the IOMMU is disabled or
> >> the device is not behind an IOMMU. So can you explain what you are
> >> trying to prevent here?
> >>
> >
> > If the IOMMU is disabled, iommu_add_dt_device will return 1 as errno.
> > So we could not make it to the xen_force check...
> 
> I disagree. The check is:
> 
> if ( res < 0 )
>    return res;
> 
> Given that res is 1, we wouldn't return and move to check whether the
> assignment can be done.
> 
> >
> > So I tried to move all IOMMU action behind xen_force check.
> >
> > Now, device assignment without IOMMU protection is only applicable on
> > direct-map domains,
> 
> It is fine to assign a non-DMA capable device without direct-mapping. So why
> do you want to add this restriction?
> 

When constructing direct-map-v2, found that, in xen/arch/arm/domain_build.c

if ( dt_find_compatible_node(node, NULL, "multiboot,device-tree") )
    d_cfg.flags |= XEN_DOMCTL_CDF_iommu;

And this flag XEN_DOMCTL_CDF_iommu determines whether iommu is enabled.

In ./xen/include/xen/sched.h

static always_inline bool is_iommu_enabled(const struct domain *d)
{
    return evaluate_nospec(d->options & XEN_DOMCTL_CDF_iommu);
}

That is, even if we assign a non-DMA capable device, we request the platform to be
iommu enabled.

> Cheers,
> 
> --
> Julien Grall

> -----Original Message-----
> From: Penny Zheng
> Sent: Wednesday, October 13, 2021 3:44 PM
> To: Julien Grall <julien@xen.org>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>
> Subject: RE: [PATCH 10/11] xen/arm: device assignment on 1:1 direct-map
> domain
> 
> Hi Julien
> 
> > -----Original Message-----
> > From: Julien Grall <julien@xen.org>
> > Sent: Monday, October 11, 2021 7:14 PM
> > To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> > sstabellini@kernel.org
> > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> > <Wei.Chen@arm.com>
> > Subject: Re: [PATCH 10/11] xen/arm: device assignment on 1:1
> > direct-map domain
> >
> >
> >
> > On 09/10/2021 10:40, Penny Zheng wrote:
> > > Hi Julien
> >
> > Hi Penny,
> >
> > >
> > >> -----Original Message-----
> > >> From: Julien Grall <julien@xen.org>
> > >> Sent: Thursday, September 23, 2021 7:27 PM
> > >> To: Penny Zheng <Penny.Zheng@arm.com>;
> > >> xen-devel@lists.xenproject.org; sstabellini@kernel.org
> > >> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> > >> <Wei.Chen@arm.com>
> > >> Subject: Re: [PATCH 10/11] xen/arm: device assignment on 1:1
> > >> direct-map domain
> > >>
> > >> Hi,
> > >>
> > >> On 23/09/2021 08:11, Penny Zheng wrote:
> > >>> User could do device passthrough, with
> > >>> "xen,force-assign-without-iommu" in the device tree snippet, on
> > >>> trusted guest through 1:1 direct-map, if IOMMU absent or disabled
> > >>> on
> > >> hardware.
> > >>
> > >> At the moment, it would be possible to passthrough a non-DMA
> > >> capable device with direct-mapping. After this patch, this is going to be
> forbidden.
> > >>
> > >>>
> > >>> In order to achieve that, this patch adds 1:1 direct-map check and
> > >>> disables iommu-related action.
> > >>>
> > >>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > >>> ---
> > >>>    xen/arch/arm/domain_build.c | 12 ++++++++----
> > >>>    1 file changed, 8 insertions(+), 4 deletions(-)
> > >>>
> > >>> diff --git a/xen/arch/arm/domain_build.c
> > >>> b/xen/arch/arm/domain_build.c index c92e510ae7..9a9d2522b7 100644
> > >>> --- a/xen/arch/arm/domain_build.c
> > >>> +++ b/xen/arch/arm/domain_build.c
> > >>> @@ -2070,14 +2070,18 @@ static int __init
> > >> handle_passthrough_prop(struct kernel_info *kinfo,
> > >>>        if ( res < 0 )
> > >>>            return res;
> > >>>
> > >>> +    /*
> > >>> +     * If xen_force, we allow assignment of devices without IOMMU
> > >> protection.
> > >>> +     * And if IOMMU is disabled or absent, 1:1 direct-map is
> > >>> + necessary > +
> > >> */
> > >>> +    if ( xen_force && is_domain_direct_mapped(kinfo->d) &&
> > >>> +         !dt_device_is_protected(node) )
> > >>
> > >> dt_device_is_protected() will be always false unless the device is
> > >> protected behing an SMMU using the legacy binding. So I don't think
> > >> this is correct to move this check ahead. In fact..
> > >>
> > >>> +        return 0;
> > >>> +
> > >>>        res = iommu_add_dt_device(node);
> > >>
> > >> ... the call should already be a NOP when the IOMMU is disabled or
> > >> the device is not behind an IOMMU. So can you explain what you are
> > >> trying to prevent here?
> > >>
> > >
> > > If the IOMMU is disabled, iommu_add_dt_device will return 1 as errno.
> > > So we could not make it to the xen_force check...
> >
> > I disagree. The check is:
> >
> > if ( res < 0 )
> >    return res;
> >
> > Given that res is 1, we wouldn't return and move to check whether the
> > assignment can be done.
> >
> > >
> > > So I tried to move all IOMMU action behind xen_force check.
> > >
> > > Now, device assignment without IOMMU protection is only applicable
> > > on direct-map domains,
> >
> > It is fine to assign a non-DMA capable device without direct-mapping.
> > So why do you want to add this restriction?
> >
> 
> When constructing direct-map-v2, found that, in
> xen/arch/arm/domain_build.c
> 
> if ( dt_find_compatible_node(node, NULL, "multiboot,device-tree") )
>     d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
> 
> And this flag XEN_DOMCTL_CDF_iommu determines whether iommu is
> enabled.
> 
> In ./xen/include/xen/sched.h
> 
> static always_inline bool is_iommu_enabled(const struct domain *d) {
>     return evaluate_nospec(d->options & XEN_DOMCTL_CDF_iommu); }
> 
> That is, even if we assign a non-DMA capable device, we request the platform
> to be iommu enabled.
>

I intend to change it to

        if ( dt_find_compatible_node(node, NULL, "multiboot,device-tree") )
        {
            if ( iommu_enabled )
                d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
            else if ( d_cfg.flags & XEN_DOMCTL_CDF_directmap )
                warning_add("Please be sure of having trusted guests, when doing device assignment without IOMMU protection\n");
        }

> > Cheers,
> >
> > --
> > Julien Grall

Hi Penny,

On 13/10/2021 08:51, Penny Zheng wrote:
> 
>> -----Original Message-----
>> From: Penny Zheng
>> Sent: Wednesday, October 13, 2021 3:44 PM
>> To: Julien Grall <julien@xen.org>; xen-devel@lists.xenproject.org;
>> sstabellini@kernel.org
>> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
>> <Wei.Chen@arm.com>
>> Subject: RE: [PATCH 10/11] xen/arm: device assignment on 1:1 direct-map
>> domain
>>
>> Hi Julien
>>
>>> -----Original Message-----
>>> From: Julien Grall <julien@xen.org>
>>> Sent: Monday, October 11, 2021 7:14 PM
>>> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
>>> sstabellini@kernel.org
>>> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
>>> <Wei.Chen@arm.com>
>>> Subject: Re: [PATCH 10/11] xen/arm: device assignment on 1:1
>>> direct-map domain
>>>
>>>
>>>
>>> On 09/10/2021 10:40, Penny Zheng wrote:
>>>> Hi Julien
>>>
>>> Hi Penny,
>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Julien Grall <julien@xen.org>
>>>>> Sent: Thursday, September 23, 2021 7:27 PM
>>>>> To: Penny Zheng <Penny.Zheng@arm.com>;
>>>>> xen-devel@lists.xenproject.org; sstabellini@kernel.org
>>>>> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
>>>>> <Wei.Chen@arm.com>
>>>>> Subject: Re: [PATCH 10/11] xen/arm: device assignment on 1:1
>>>>> direct-map domain
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 23/09/2021 08:11, Penny Zheng wrote:
>>>>>> User could do device passthrough, with
>>>>>> "xen,force-assign-without-iommu" in the device tree snippet, on
>>>>>> trusted guest through 1:1 direct-map, if IOMMU absent or disabled
>>>>>> on
>>>>> hardware.
>>>>>
>>>>> At the moment, it would be possible to passthrough a non-DMA
>>>>> capable device with direct-mapping. After this patch, this is going to be
>> forbidden.
>>>>>
>>>>>>
>>>>>> In order to achieve that, this patch adds 1:1 direct-map check and
>>>>>> disables iommu-related action.
>>>>>>
>>>>>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
>>>>>> ---
>>>>>>     xen/arch/arm/domain_build.c | 12 ++++++++----
>>>>>>     1 file changed, 8 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/xen/arch/arm/domain_build.c
>>>>>> b/xen/arch/arm/domain_build.c index c92e510ae7..9a9d2522b7 100644
>>>>>> --- a/xen/arch/arm/domain_build.c
>>>>>> +++ b/xen/arch/arm/domain_build.c
>>>>>> @@ -2070,14 +2070,18 @@ static int __init
>>>>> handle_passthrough_prop(struct kernel_info *kinfo,
>>>>>>         if ( res < 0 )
>>>>>>             return res;
>>>>>>
>>>>>> +    /*
>>>>>> +     * If xen_force, we allow assignment of devices without IOMMU
>>>>> protection.
>>>>>> +     * And if IOMMU is disabled or absent, 1:1 direct-map is
>>>>>> + necessary > +
>>>>> */
>>>>>> +    if ( xen_force && is_domain_direct_mapped(kinfo->d) &&
>>>>>> +         !dt_device_is_protected(node) )
>>>>>
>>>>> dt_device_is_protected() will be always false unless the device is
>>>>> protected behing an SMMU using the legacy binding. So I don't think
>>>>> this is correct to move this check ahead. In fact..
>>>>>
>>>>>> +        return 0;
>>>>>> +
>>>>>>         res = iommu_add_dt_device(node);
>>>>>
>>>>> ... the call should already be a NOP when the IOMMU is disabled or
>>>>> the device is not behind an IOMMU. So can you explain what you are
>>>>> trying to prevent here?
>>>>>
>>>>
>>>> If the IOMMU is disabled, iommu_add_dt_device will return 1 as errno.
>>>> So we could not make it to the xen_force check...
>>>
>>> I disagree. The check is:
>>>
>>> if ( res < 0 )
>>>     return res;
>>>
>>> Given that res is 1, we wouldn't return and move to check whether the
>>> assignment can be done.
>>>
>>>>
>>>> So I tried to move all IOMMU action behind xen_force check.
>>>>
>>>> Now, device assignment without IOMMU protection is only applicable
>>>> on direct-map domains,
>>>
>>> It is fine to assign a non-DMA capable device without direct-mapping.
>>> So why do you want to add this restriction?
>>>
>>
>> When constructing direct-map-v2, found that, in
>> xen/arch/arm/domain_build.c
>>
>> if ( dt_find_compatible_node(node, NULL, "multiboot,device-tree") )
>>      d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>>
>> And this flag XEN_DOMCTL_CDF_iommu determines whether iommu is
>> enabled.
>>
>> In ./xen/include/xen/sched.h
>>
>> static always_inline bool is_iommu_enabled(const struct domain *d) {
>>      return evaluate_nospec(d->options & XEN_DOMCTL_CDF_iommu); }
>>
>> That is, even if we assign a non-DMA capable device, we request the platform
>> to be iommu enabled.
>>
> 
> I intend to change it to
> 
>          if ( dt_find_compatible_node(node, NULL, "multiboot,device-tree") )
>          {
>              if ( iommu_enabled )
>                  d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>              else if ( d_cfg.flags & XEN_DOMCTL_CDF_directmap )
>                  warning_add("Please be sure of having trusted guests, when doing device assignment without IOMMU protection\n");
>          }

I think the warning is misleading. You don't need to trust a guest in 
order to assign non-DMA capable device.

But, I think the message is not necessary because from my understanding, 
an admin would need to add the property xen,force-assign-without-iommu 
in order to passthrough. So they should be fully aware of the 
consequence to do the passthrough.

Cheers,

-- 
Julien Grall