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

[Julien Grall <julien@xxxxxxx>]

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@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");
>          }
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