RE: [Xen-devel] SMMU permission fault on Dom0 when init vpu_decoder

[Peng Fan <peng.fan@xxxxxxx>]

> Subject: Re: [Xen-devel] SMMU permission fault on Dom0 when init
> vpu_decoder
> 
> (+ Stefano)
> 
> On 30/05/2022 16:21, Oleksii Moisieiev wrote:
> > Hello,
> 
> Hi Oleksii,
> 
> > I'm getting permission fault from SMMU when trying to init
> > VPU_Encoder/Decoder in Dom0 on IMX8QM board:
> > (XEN) smmu: /iommu@51400000: Unhandled context fault: fsr=0x408,
> > iova=0x86000a60, fsynr=0x1c0062, cb=0 This error appears when
> > vpu_encoder/decoder tries to memcpy firmware image to
> > 0x86000000 address, which is defined in reserved-memory node in xen
> > device-tree as encoder_boot/decoder_boot region.
> 
> It is not clear to me who is executing the memcpy(). Is it the device or your
> domain? If the former, where was the instruction fetch from?
> 
> The reason I am asking that is, from what you wrote, mempcy() will write to
> 0x86000000. So the write should not result to a instruction abort.
> Only an instruction fetch would lead to such abort.
> 
> >
> > I'm using xen from branch xen-project/staging-4.16 + imx related
> > patches, which were taken from
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.c
> odeaurora.org%2Fexternal%2Fimx%2Fimx-xen&data=05%7C01%7Cpeng.f
> an%40nxp.com%7C726bfc45b45747a655bc08da425351d4%7C686ea1d3bc2b
> 4c6fa92cd99c5c301635%7C0%7C0%7C637895222868692087%7CUnknown%
> 7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL
> CJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=KcbEjN80VJS5yq4KMp2g
> NjQuVtx95jKHH5T32ZCj3Do%3D&reserved=0.
> >
> > After some investigation I found that this issue was fixed by Peng Fan
> > in
> > commit: 46b3dd3718144ca6ac2c12a3b106e57fb7156554 (Hash from
> > codeaurora), but only for the Guest domains.
> > It introduces new p2m_type p2m_mmio_direct_nc_x, which differs from
> > p2m_mmio_direct_nc by XN = 0. This type is set to the reserved memory
> > region in map_mmio_regions function.
> >
> > I was able to fix issue in Dom0 by setting p2m_mmio_direct_nc_x type
> > for the reserved memory in map_regions_p2mt, which is used to map
> memory during Dom0 creation.
> > Patch can be found below.
> >
> > Based on initial discussions on IRC channel - XN bit did the trick
> > because looks like vpu decoder is executing some code from this memory.
> 
> This was a surprise to me that device could also execute memory. From the
> SMMU spec, this looks a legit things. Before relaxing the type, I would like 
> to
> confirm this is what's happenning in your case.

It is not device, VPU could execute code, it has firmware. Just like use
SMMU to isolate M4-core, M4 could execute code.

Regards,
Peng.

> 
> [...]
> 
> > ---
> > arm: Set p2m_type to p2m_mmio_direct_nc_x for reserved memory regions
> >
> > This is the enhancement of the
> 46b3dd3718144ca6ac2c12a3b106e57fb7156554.
> > Those patch introduces p2m_mmio_direct_nc_x p2m type which sets the
> > e->p2m.xn = 0 for the reserved-memory, such as vpu encoder/decoder.
> >
> > Set p2m_mmio_direct_nc_x in map_regions_p2mt for reserved-memory the
> > same way it does in map_mmio_regions. This change is for the case when
> > vpu encoder/decoder works in DomO and not passed-through to the Guest
> > Domains.
> >
> > Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx>
> > ---
> >   xen/arch/arm/p2m.c | 7 +++++++
> >   1 file changed, 7 insertions(+)
> >
> > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index
> > e9568dab88..bb1f681b71 100644
> > --- a/xen/arch/arm/p2m.c
> > +++ b/xen/arch/arm/p2m.c
> > @@ -1333,6 +1333,13 @@ int map_regions_p2mt(struct domain *d,
> >                        mfn_t mfn,
> >                        p2m_type_t p2mt)
> >   {
> > +    if (((long)gfn_x(gfn) >= (GUEST_RAM0_BASE >> PAGE_SHIFT)) &&
> > +        (((long)gfn_x(gfn) + nr) <=
> > +        ((GUEST_RAM0_BASE + GUEST_RAM0_SIZE)>> PAGE_SHIFT)))
> 
> I am afraid I don't understand what this check is for. In a normal setup, we 
> don't
> know where the reserved regions are mapped. Only the caller may know that.
> 
> For dom0, this decision could be taken in map_range_to_domain(). For the
> domU, we would need to let the toolstack to chose the memory attribute.
> Stefano attempted to do that a few years ago (see [1]). Maybe we should revive
> it?
> 
> > +    {
> > +        p2m_remove_mapping(d, gfn, nr, mfn);
> > +        return p2m_insert_mapping(d, gfn, nr, mfn,
> p2m_mmio_direct_nc_x);
> > +    }
> >       return p2m_insert_mapping(d, gfn, nr, mfn, p2mt);
> >   }
> >
> 
> Cheers,
> 
> [1]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.ker
> nel.org%2Fxen-devel%2Falpine.DEB.2.10.1902261501020.20689%40sstabellini
> -ThinkPad-X260%2F&amp;data=05%7C01%7Cpeng.fan%40nxp.com%7C726bfc
> 45b45747a655bc08da425351d4%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> 7C0%7C0%7C637895222868692087%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi
> MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000
> %7C%7C%7C&amp;sdata=Y%2B%2Fxslm6wc38jM6zZWshRnTM4tx%2BMpyXaI
> r5BLV1R9Q%3D&amp;reserved=0
> 
> --
> Julien Gral