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
