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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hello Julien,

> On 10 Feb 2021, at 5:34 pm, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi,
> 
> On 10/02/2021 15:06, Rahul Singh wrote:
>>> On 9 Feb 2021, at 8:36 pm, Stefano Stabellini <sstabellini@xxxxxxxxxx> 
>>> 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.
>> Yes you are right I am trying to create the guest with "xl create” and 
>> before that, I created the logical volume and trying to attach the logical 
>> volume
>> block to the domain with “xl block-attach”. I observed this error with the 
>> "xl block-attach” command.
>> This issue occurs after applying this patch as what I observed this patch 
>> introduce the calls to iommu_legacy_{, un}map() to map the grant pages for
>> IOMMU that touches the page-tables. I am not sure but what I observed is 
>> that something is written wrong when iomm_unmap calls unmap the pages
>> because of that issue is observed.
> 
> Can you clarify what you mean by "written wrong"? What sort of error do you 
> see in the iommu_unmap()?


I might be wrong as per my understanding for ARM we are sharing the P2M between 
CPU and IOMMU always and the map_grant_ref() function is written in such a way 
that we have to call iommu_legacy_{, un}map() only if P2M is not shared. 

As we are sharing the P2M when we call the iommu_map() function it will 
overwrite the existing GFN -> MFN ( For DOM0 GFN is same as MFN) entry and when 
we call iommu_unmap() it will unmap the  (GFN -> MFN ) entry from the 
page-table. Next time when map_grant_ref() tries to map the same frame it will 
try to get the corresponding GFN but there will no entry in P2M as 
iommu_unmap() already unmapped the GFN because of that this error might be 
observed.

Sequence of events that results the issue :

gnttab_map_grant_ref (GFN=a99fb MFN=a99fb)
arm_iommu_map_page() DOMID:0 dfn = a99fb mfn = a99fb 

gnttab_map_grant_ref ( GFN=d9913 MFN=d9913)
arm_iommu_map_page() DOMID:0 dfn = d9913 mfn = d9913

gnttab_map_grant_ref ( GFN=d9846 MFN=d9846)
arm_iommu_map_page() DOMID:0 dfn = d9846 mfn = d9846 

gnttab_map_grant_ref (GFN=a8474 MFN=a8474)
arm_iommu_map_page() DOMID:0 dfn = a8474 mfn = a8474

arm_iommu_unmap_page() DOMID:0 dfn = a99fb
arm_iommu_unmap_page() DOMID:0 dfn = d9913
arm_iommu_unmap_page() DOMID:0 dfn = d9846
arm_iommu_unmap_page() DOMID:0 dfn = a8474

Try to map the same frame that is unmapped earlier by iommu_unmap call()
gnttab_map_grant_ref (GFN=a99fb MFN=0xffffffff).-> Not able to find the GFN in 
p2m error is observed after that. 

> 
>> You can reproduce the issue by just creating the dummy image file and try to 
>> attach it with “xl block-attach”
>> dd if=/dev/zero of=test.img bs=1024 count=0 seek=1024
>> xl block-attach 0 phy:test.img xvda w
>> Sequence of command that I follow is as follows to reproduce the issue:
>> lvs vg-xen/myguest
>> lvcreate -y -L 4G -n myguest vg-xen
>> parted -s /dev/vg-xen/myguest mklabel msdos
>> parted -s /dev/vg-xen/myguest unit MB mkpart primary 1 4096
>> sync
>> xl block-attach 0 phy:/dev/vg-xen/myguest xvda w
>> libxl: error: libxl_xshelp.c:201:libxl__xs_read_mandatory: xenstore read 
>> failed: `/libxl/0/type': No such file or directory
>> libxl: warning: libxl_dom.c:51:libxl__domain_type: unable to get domain type 
>> for domid=0, assuming HVM
>> [  162.632232] xen-blkback: backend/vbd/0/51712: using 4 queues, protocol 1 
>> (arm-abi) persistent grants
>> [  162.764847] vbd vbd-0-51712: 9 mapping in shared page 8 from domain 0
>> [  162.771740] vbd vbd-0-51712: 9 mapping ring-ref port 5
>> [  162.777650] ------------[ cut here ]------------
>> [  162.782167] WARNING: CPU: 2 PID: 37 at 
>> drivers/block/xen-blkback/xenbus.c:296 xen_blkif_disconnect+0x20c/0x230
> 
> Just to confirm, this splat comes from:
> 
> WARN_ON(i != (XEN_BLKIF_REQS_PER_PAGE * blkif->nr_ring_pages));
> 
> If so, what are the values for i and blkif->nr_ring_pages?

Let me find out and will share with you.

Regards,
Rahul
> 
> Cheers,
> 
> -- 
> Julien Grall