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

[Aviv B.D.]

see comments below.

Thanks for your inputs,
Aviv

On Mon, Apr 11, 2016 at 6:40 AM, Peter Xu <address@hidden> wrote:
> 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.

You are correct, next time I'll use git-send-email.

>
> [...]
>
>> @@ -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).
>
you are correct, I'll fix those locations to use the correct enum.
The translate function returns entry.perm == IOMMU_NONE if no mapping
is existing, and this is a valid case. Actually, the kernel may signal
that the hardware should invalidate a chunk of 4 consecutive pages
with one page in the middle that is in invalid state. I want to skip
those pages (in the mapping stage of the function).

>
> - 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?

You are correct, I want the code firstly to work correctly (even if a
bit slower than optimal). I'll try to include an optimizations for
this in the next version (but currently I can't promise that I'll have
the time).

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

yes. Domains can be shared by more than one device, but if QEMU assign
those devices to the same domain on host the map and unmap will happen
only once. As far as I understand the VFIO code in QEMU, it always try
to assign all of those devices to the same domain on host. Therefore
this problem doesn't existing.

> [...]
>
>>  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?

Will do.
IOMMU_ANY is flag to suppress errors reports to guest kernel in case
of not existing mapping, So in this case I'm not sure that this enum
represent correctly this intent.

>
> Thanks.
>
> -- peterx