Re: privcmd.c not calling set_phys_to_machine

[Juergen Gross <jgross@xxxxxxxx>]

On 14.10.22 23:04, Stefano Stabellini wrote:
> Hi Juergen and all,
>
> I am writing again to ask a question about privcmd.c in PV dom0 x86.
> This is related to the previous pin_user_pages_fast issue:
>
> https://marc.info/?l=xen-devel&m=166268914727630
> https://marc.info/?l=xen-devel&m=166322385912052
>
>
> In summary this is the situation:
>
> 1. domU (HVM) kernel space:
>      a. pages allocation with get_free_pages()
>      b. get dma_handle by calling dma_map_page() on the pages allocated in (1.a)
>      c. send dma_handle to dom0 (PV) using virtio queue
>
> 2. dom0 userspace (QEMU):
>          a. read dma_handle from virtio queue
>          b. map dma_handle using QEMU dma_memory_map(), which calls
>             xenforeignmemory_map2, which is IOCTL_PRIVCMD_MMAPBATCH_V2,
>             which ends up calling drivers/xen/privcmd.c:privcmd_ioctl_mmap_batch
>             [this is verified to work correctly, the mapping works]
>          c. open /dev/tee node and make an ioctl call to register the
>             virtual address (from step 2.b) with TEE.
>
> 3. dom0 kernel space:
>          a. AMD TEE driver get the virtual address passed by userspace
>          b. AMD TEE driver get the list of pages corresponding to the
>             virtual address (3.a) and calls dma_map_page() on them
I'm rather sure "AMD TEE driver get the list of pages corresponding to the
virtual address" is the problem. The PTEs should have the "special" flag
set, meaning that there is no struct page associated with this virtual area.

> The last step (3.b) misbehaves as dev_addr at the beginning of
> xen_swiotlb_map_page (which implements dma_map_page() in dom)) is 0.
>
>    dma_addr_t dev_addr = xen_phys_to_dma(dev, phys);
>    /* dev_addr here is zero */
>
>
> Could it be that the original mapping of the foreign pages in Dom0, done
> by step 2.b, is not complete? Looking into
> privcmd_ioctl_mmap_batch, for PV guests, it is calling mmap_batch_fn:
>
>         BUG_ON(traverse_pages_block(m.num, sizeof(xen_pfn_t),
>                                     &pagelist, mmap_batch_fn, &state));
>
> mmap_batch_fn calls xen_remap_domain_gfn_array, which calls
> xen_remap_pfn.
>
> xen_remap_pfn only changes the VA->PA mapping and does nothing else.
> Specifically, nobody seems to call set_phys_to_machine in this code
> path. Isn't set_phys_to_machine required?
Not for special memory pages.

> Don't we need a call to set_phys_to_machine so that the next time a
> driver tries to call:
>
>    /* address is the virtual address passed by QEMU userspace */
>    dma_map_page(virt_to_page(address))
>
> it will behave correctly? Or am I missing something?
>
>
> How is xen_phys_to_dma expected to work correctly for:
>
>    /* address is the virtual address passed by QEMU userspace and mapped
>     * in 2.b */
>    phys_addr = virt_to_phys(address);
>    xen_phys_to_dma(dev, phys_addr);
>
>
> My guess would be that we need to add:
>
>    set_phys_to_machine(pfn, FOREIGN_FRAME(mfn));
>
> in mmap_batch_fn or xen_remap_pfn?
I think this might be a little bit more complicated.

This could work, if there is really a struct page available for the PFN.
OTOH this might be not the case quite often, as we are using zone device
memory for foreign mappings per default for some time now.

Solving this might require something like dma_map_pfn() instead of
dma_map_page(), which sounds a little bit like dma_direct_mmap().


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature