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

[Julien Grall <julien@xxxxxxx>]

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?
Cheers,

--
Julien Grall