RE: [PATCH 10/11] xen/arm: device assignment on 1:1 direct-map domain

[Penny Zheng <Penny.Zheng@xxxxxxx>]

> -----Original Message-----
> From: Penny Zheng
> Sent: Wednesday, October 13, 2021 3:44 PM
> To: Julien Grall <julien@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> sstabellini@xxxxxxxxxx
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Wei Chen
> <Wei.Chen@xxxxxxx>
> Subject: RE: [PATCH 10/11] xen/arm: device assignment on 1:1 direct-map
> domain
> 
> Hi Julien
> 
> > -----Original Message-----
> > From: Julien Grall <julien@xxxxxxx>
> > Sent: Monday, October 11, 2021 7:14 PM
> > To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> > sstabellini@xxxxxxxxxx
> > Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Wei Chen
> > <Wei.Chen@xxxxxxx>
> > 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@xxxxxxx>
> > >> Sent: Thursday, September 23, 2021 7:27 PM
> > >> To: Penny Zheng <Penny.Zheng@xxxxxxx>;
> > >> xen-devel@xxxxxxxxxxxxxxxxxxxx; sstabellini@xxxxxxxxxx
> > >> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Wei Chen
> > >> <Wei.Chen@xxxxxxx>
> > >> 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@xxxxxxx>
> > >>> ---
> > >>>    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