Re: [Qemu-devel] [PATCH RFC v2] IOMMU: Add Support to VFIO devices with vIOMMU present

[Peter Xu <peterx@redhat.com>]

Hi, Aviv,

On Sat, Apr 09, 2016 at 09:03:38PM +0300, Aviv B.D. wrote:

[...]

> +static int vtd_get_did_dev(IntelIOMMUState *s, uint8_t bus_num,
> uint8_t devfn, uint16_t * domain_id)

It seems that there are two lines above, however what I feel is that
this is a long line splitted by the email client or something
else... are you sending the patch using git-send-email? Not sure
whether this would be a problem.

I see the same thing in previous patches too.

[...]

> @@ -785,7 +816,7 @@ static inline bool vtd_is_interrupt_addr(hwaddr addr)
>   * @entry: IOMMUTLBEntry that contain the addr to be translated and result
>   */
>  static void vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,
> -                                   uint8_t devfn, hwaddr addr, bool is_write,
> +                                   uint8_t devfn, hwaddr addr,
> IOMMUAccessPermissions is_write,

First of all, if we are to change this "is_write" into a permission,
I would prefer change it's name too from "is_write" to "perm" or
else, so that people would know this is not a boolean any more.

Secondly, I assume you have not handled all the is_write uses below,
right? So the code seems not consistent. Many places are still using
it as boolean (I suppose).

[...]

> uint16_t domain_id,
> +                                      hwaddr addr, uint8_t am)
> +{
> +    VFIOGuestIOMMU * giommu;
> +
> +    QLIST_FOREACH(giommu, &(s->giommu_list), iommu_next){
> +        VTDAddressSpace *vtd_as = container_of(giommu->iommu,
> VTDAddressSpace, iommu);
> +        uint16_t vfio_domain_id;
> +        int ret = vtd_get_did_dev(s, pci_bus_num(vtd_as->bus),
> vtd_as->devfn, &vfio_domain_id);
> +        int i=0;
> +        if (!ret && domain_id == vfio_domain_id){
> +            IOMMUTLBEntry entry;
> +
> +            /* do vfio unmap */
> +            VTD_DPRINTF(GENERAL, "Remove addr 0x%"PRIx64 " mask %d", addr, am);
> +            entry.target_as = NULL;
> +            entry.iova = addr & VTD_PAGE_MASK_4K;
> +            entry.translated_addr = 0;
> +            entry.addr_mask = ~VTD_PAGE_MASK(VTD_PAGE_SHIFT_4K + am);
> +            entry.perm = IOMMU_NONE;
> +            memory_region_notify_iommu(giommu->iommu, entry);
> +
> +            /* do vfio map */
> +            VTD_DPRINTF(GENERAL, "add addr 0x%"PRIx64 " mask %d", addr, am);
> +            /* call to vtd_iommu_translate */
> +            for (i = 0; i < (1 << am); i++, addr+=(1 << VTD_PAGE_SHIFT_4K)){
> +                IOMMUTLBEntry entry =
> s->iommu_ops.translate(giommu->iommu, addr, IOMMU_ANY);
> +                if (entry.perm != IOMMU_NONE){
> +                    memory_region_notify_iommu(giommu->iommu, entry);
> +                }

Questions (that I am curious about only, not to mean that there is
something worng):

- What should we do if entry.perm == IOMMU_NONE? Is it possible? If
  not, I'd prefer assert to if.

- Here, we do the remap for every 4K, guess even with huge
  pages. Better way to do? A stupid one from me: take special care
  for am == 9, 18 cases?

- Is it possible that multiple devices use same domain ID? Not
  sure. If so, we will always map/unmap the same address twice?

[...]

>  static void vfio_listener_region_add(MemoryListener *listener,
>                                       MemoryRegionSection *section)
>  {
> @@ -344,6 +347,7 @@ static void
> vfio_listener_region_add(MemoryListener *listener,
>      iova = TARGET_PAGE_ALIGN(section->offset_within_address_space);
>      llend = int128_make64(section->offset_within_address_space);
>      llend = int128_add(llend, section->size);
> +    llend = int128_add(llend, int128_exts64(-1));

It seems that Bandan has fixed this. Please try pull latest master
branch and check commit 55efcc537d330. If so, maybe we need to
rebase?

>      llend = int128_and(llend, int128_exts64(TARGET_PAGE_MASK));
> 
>      if (int128_ge(int128_make64(iova), llend)) {
> @@ -381,11 +385,13 @@ static void
> vfio_listener_region_add(MemoryListener *listener,
>          giommu->n.notify = vfio_iommu_map_notify;
>          QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next);
> 
> +        vtd_register_giommu(giommu);
>          memory_region_register_iommu_notifier(giommu->iommu, &giommu->n);
> +#if 0
>          memory_region_iommu_replay(giommu->iommu, &giommu->n,
>                                     vfio_container_granularity(container),
>                                     false);
> -
> +#endif
>          return;
>      }
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 2de7898..0e814ab 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -146,10 +146,14 @@ struct MemoryRegionOps {
>  };
> 
>  typedef struct MemoryRegionIOMMUOps MemoryRegionIOMMUOps;
> -
> +typedef enum IOMMUAccessPermissions{
> +    IOMMU_READ = 0,
> +    IOMMU_WRITE = 1,
> +    IOMMU_ANY = 2
> +} IOMMUAccessPermissions;

I see this:

typedef enum {
    IOMMU_NONE = 0,
    IOMMU_RO   = 1,
    IOMMU_WO   = 2,
    IOMMU_RW   = 3,
} IOMMUAccessFlags;

in include/exec/memory.h. Shall we leverage it rather than define
another one?

Thanks.

-- peterx