Re: [PATCH v2] xen/arm: fix gnttab_need_iommu_mapping

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 9 Feb 2021, Stefano Stabellini wrote:
> On Tue, 9 Feb 2021, Rahul Singh wrote:
> > > On 8 Feb 2021, at 6:49 pm, Stefano Stabellini <sstabellini@xxxxxxxxxx> 
> > > wrote:
> > > 
> > > Commit 91d4eca7add broke gnttab_need_iommu_mapping on ARM.
> > > The offending chunk is:
> > > 
> > > #define gnttab_need_iommu_mapping(d)                    \
> > > -    (is_domain_direct_mapped(d) && need_iommu(d))
> > > +    (is_domain_direct_mapped(d) && need_iommu_pt_sync(d))
> > > 
> > > On ARM we need gnttab_need_iommu_mapping to be true for dom0 when it is
> > > directly mapped and IOMMU is enabled for the domain, like the old check
> > > did, but the new check is always false.
> > > 
> > > In fact, need_iommu_pt_sync is defined as dom_iommu(d)->need_sync and
> > > need_sync is set as:
> > > 
> > >    if ( !is_hardware_domain(d) || iommu_hwdom_strict )
> > >        hd->need_sync = !iommu_use_hap_pt(d);
> > > 
> > > iommu_use_hap_pt(d) means that the page-table used by the IOMMU is the
> > > P2M. It is true on ARM. need_sync means that you have a separate IOMMU
> > > page-table and it needs to be updated for every change. need_sync is set
> > > to false on ARM. Hence, gnttab_need_iommu_mapping(d) is false too,
> > > which is wrong.
> > > 
> > > As a consequence, when using PV network from a domU on a system where
> > > IOMMU is on from Dom0, I get:
> > > 
> > > (XEN) smmu: /smmu@fd800000: Unhandled context fault: fsr=0x402, 
> > > iova=0x8424cb148, fsynr=0xb0001, cb=0
> > > [   68.290307] macb ff0e0000.ethernet eth0: DMA bus error: HRESP not OK
> > > 
> > > The fix is to go back to something along the lines of the old
> > > implementation of gnttab_need_iommu_mapping.
> > > 
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> > > Fixes: 91d4eca7add
> > > Backport: 4.12+
> > > 
> > > ---
> > > 
> > > Given the severity of the bug, I would like to request this patch to be
> > > backported to 4.12 too, even if 4.12 is security-fixes only since Oct
> > > 2020.
> > > 
> > > For the 4.12 backport, we can use iommu_enabled() instead of
> > > is_iommu_enabled() in the implementation of gnttab_need_iommu_mapping.
> > > 
> > > Changes in v2:
> > > - improve commit message
> > > - add is_iommu_enabled(d) to the check
> > > ---
> > > xen/include/asm-arm/grant_table.h | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/xen/include/asm-arm/grant_table.h 
> > > b/xen/include/asm-arm/grant_table.h
> > > index 6f585b1538..0ce77f9a1c 100644
> > > --- a/xen/include/asm-arm/grant_table.h
> > > +++ b/xen/include/asm-arm/grant_table.h
> > > @@ -89,7 +89,7 @@ int replace_grant_host_mapping(unsigned long gpaddr, 
> > > mfn_t mfn,
> > >     (((i) >= nr_status_frames(t)) ? INVALID_GFN : (t)->arch.status_gfn[i])
> > > 
> > > #define gnttab_need_iommu_mapping(d)                    \
> > > -    (is_domain_direct_mapped(d) && need_iommu_pt_sync(d))
> > > +    (is_domain_direct_mapped(d) && is_iommu_enabled(d))
> > > 
> > > #endif /* __ASM_GRANT_TABLE_H__ */
> > 
> > I tested the patch and while creating the guest I observed the below 
> > warning from Linux for block device.
> > https://elixir.bootlin.com/linux/v4.3/source/drivers/block/xen-blkback/xenbus.c#L258
> 
> So you are creating a guest with "xl create" in dom0 and you see the
> warnings below printed by the Dom0 kernel? I imagine the domU has a
> virtual "disk" of some sort.
> 
> 
> > I did initial debugging and found out that there are many calls to 
> > iommu_legacy_{,un}map() while creating the guest but when 
> > iommu_legacy_unmap() function unmap the pages something is written  wrong 
> > in page tables because of that when next time same page is mapped via 
> > create_grant_host_mapping() we observed below warning. 
> 
> By "while creating a guest", do you mean before the domU is even
> unpaused? Hence, the calls are a result of dom0 operations? Or after
> domU is unpaused, hence, the calls are a result of domU operations
> (probably the domU simply trying to access its virtual disk)?
> Please note that you can start a guest paused with xl create -p.
> 
> Looking at the logs, it is probably the latter. The following line
> should be printed when the domU PV block frontend connects to the
> backend in dom0:
> 
> [  138.639934] xen-blkback: backend/vbd/0/51712: using 4 queues, protocol 1 
> (arm-abi) persistent grants
> 
> I'll see if I can repro the issue here.

I cannot repro the issue at my end. If there is anything interesting in
your setup that is necessary to reproduce the problem please share the
details.

Looking at the code, xen_blkif_disconnect is called by frontend_changed
on XenbusStateConnected. I don't know for sure why we need to call
xen_blkif_disconnect() before connecting, but I imagine it is to make sure
we are starting from a clean slate.

In the case of domU creation, I'd imagine the xen_blkif_disconnect() is
unnecessary. And, in fact, in my setup blkif->nr_rings is 0 so the loop
at the beginning of xen_blkif_disconnect is not even entered.

I wonder why it is not the same at your end. I don't know how it could
be that blkif->nr_rings is not 0 if you are just starting the VM for the
first time. Also, are you sure that this is related to this patch?

  
> > [  138.639934] xen-blkback: backend/vbd/0/51712: using 4 queues, protocol 1 
> > (arm-abi) persistent grants
> > (XEN) gnttab_mark_dirty not implemented yet
> > [  138.659702] xen-blkback: backend/vbd/0/51712: using 4 queues, protocol 1 
> > (arm-abi) persistent grants
> > [  138.669827] vbd vbd-0-51712: 9 mapping in shared page 8 from domain 0
> > [  138.676636] vbd vbd-0-51712: 9 mapping ring-ref port 5
> > [  138.682089] ------------[ cut here ]------------
> > [  138.686605] WARNING: CPU: 2 PID: 37 at 
> > drivers/block/xen-blkback/xenbus.c:296 xen_blkif_disconnect+0x20c/0x230
> > [  138.696668] Modules linked in: bridge stp llc ipv6 nf_defrag_ipv6
> > [  138.702833] CPU: 2 PID: 37 Comm: xenwatch Not tainted 
> > 5.4.0-yocto-standard #1
> > [  138.710037] Hardware name: Arm Neoverse N1 System Development Platform 
> > (DT)
> > [  138.717067] pstate: 80c00005 (Nzcv daif +PAN +UAO)
> > [  138.721927] pc : xen_blkif_disconnect+0x20c/0x230
> > [  138.726701] lr : xen_blkif_disconnect+0xbc/0x230
> > [  138.731388] sp : ffff800011cb3c80
> > [  138.734773] x29: ffff800011cb3c80 x28: ffff00005b6da940 
> > [  138.740156] x27: 0000000000000000 x26: 0000000000000000 
> > [  138.745536] x25: ffff000029755060 x24: 0000000000000170 
> > [  138.750919] x23: ffff000029755040 x22: ffff000059c72000 
> > [  138.756299] x21: 0000000000000000 x20: ffff000029755000 
> > [  138.761681] x19: 0000000000000001 x18: 0000000000000000 
> > [  138.767063] x17: 0000000000000000 x16: 0000000000000000 
> > [  138.772444] x15: 0000000000000000 x14: 0000000000000000 
> > [  138.777826] x13: 0000000000000000 x12: 0000000000000000 
> > [  138.783207] x11: 0000000000000001 x10: 0000000000000990 
> > [  138.788589] x9 : 0000000000000001 x8 : 0000000000210d00 
> > [  138.793971] x7 : 0000000000000018 x6 : ffff00005ddf72a0 
> > [  138.799352] x5 : ffff800011cb3c28 x4 : 0000000000000000 
> > [  138.804734] x3 : ffff000029755118 x2 : 0000000000000000 
> > [  138.810117] x1 : ffff000029755120 x0 : 0000000000000001 
> > [  138.815497] Call trace:
> > [  138.818015]  xen_blkif_disconnect+0x20c/0x230
> > [  138.822442]  frontend_changed+0x1b0/0x54c
> > [  138.826523]  xenbus_otherend_changed+0x80/0xb0
> > [  138.831035]  frontend_changed+0x10/0x20
> > [  138.834941]  xenwatch_thread+0x80/0x144
> > [  138.838849]  kthread+0x118/0x120
> > [  138.842147]  ret_from_fork+0x10/0x18
> > [  138.845791] ---[ end trace fb9f0a3b3b48a55f ]—
>