Re: [PATCH 12/12] xen/arm: call iomem_permit_access for passthrough devices

From: Stefano Stabellini <sstabellini@kernel.org>
To: Julien Grall <julien@xen.org>
Cc: xen-devel@lists.xenproject.org,
	Stefano Stabellini <sstabellini@kernel.org>,
	Volodymyr_Babchuk@epam.com,
	Stefano Stabellini <stefano.stabellini@xilinx.com>
Subject: Re: [PATCH 12/12] xen/arm: call iomem_permit_access for passthrough devices
Date: Wed, 29 Apr 2020 13:47:25 -0700 (PDT)	[thread overview]
Message-ID: <alpine.DEB.2.21.2004291318270.28941@sstabellini-ThinkPad-T480s> (raw)
In-Reply-To: <521c8e55-73e8-950f-2d94-70b0c664bd3d@xen.org>

On Wed, 15 Apr 2020, Julien Grall wrote:
> On 15/04/2020 02:02, Stefano Stabellini wrote:
> > iomem_permit_access should be called for MMIO regions of devices
> > assigned to a domain. Currently it is not called for MMIO regions of
> > passthrough devices of Dom0less guests. This patch fixes it.
> 
> You can already have cases today where the MMIO regions are mapped to the
> guest but the guest doesn't have permission on them.
> 
> Can you explain why this is a problem here?

I don't remember the original problem that prompted me into doing this
investigation. It came up while I was developing and testing this
series: I noticed the discrepancy compared to the regular (not dom0less)
device assignment code path
(tools/libxl/libxl_create.c:domcreate_launch_dm).

Now I removed this patch from the series, went back to test it, and it
still works fine. Oops, it is not needed after all :-)

I think it makes sense to remove this patch from this series, I'll do
that.

But doesn't it make sense to give domU permission if it is going to get
the memory mapped? But admittedly I can't think of something that would
break because of the lack of the iomem_permit_access call in this code
path.

> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > ---
> >   xen/arch/arm/domain_build.c | 11 +++++++++++
> >   1 file changed, 11 insertions(+)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index d0fc497d07..d3d42eef5d 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -1846,6 +1846,17 @@ static int __init handle_passthrough_prop(struct
> > kernel_info *kinfo,
> >               return -EINVAL;
> >           }
> >   +        res = iomem_permit_access(kinfo->d, paddr_to_pfn(mstart),
> > +                                  paddr_to_pfn(PAGE_ALIGN(mstart + size -
> > 1)));
> > +        if ( res )
> > +        {
> > +            printk(XENLOG_ERR "Unable to permit to dom%d access to"
> > +                   " 0x%"PRIx64" - 0x%"PRIx64"\n",
> > +                   kinfo->d->domain_id,
> > +                   mstart & PAGE_MASK, PAGE_ALIGN(mstart + size) - 1);
> > +            return res;
> > +        }
> > +
> >           res = map_regions_p2mt(kinfo->d,
> >                                  gaddr_to_gfn(gstart),
> >                                  PFN_DOWN(size),
> > 
> 
> -- 
> Julien Grall
> 