Xen devel - Sep 2012 - [PATCH 2 of 6 V6] amd iommu: call guest_iommu_set_base from hvmloader

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

>>> On 26.09.12 at 16:46, Wei Wang <wei.wang2@amd.com> wrote:
>@@ -3834,6 +3835,9 @@ long do_hvm_op(unsigned long op,
XEN_GUEST_HANDLE(void) arg)
>             case HVM_PARAM_BUFIOREQ_EVTCHN:
>                 rc = -EINVAL;
>                 break;
>+            case HVM_PARAM_IOMMU_BASE:
>+                rc = guest_iommu_set_base(d, a.value);
This suggests that you''re allowing for only a single IOMMU per
guest - is that not going to become an issue sooner or later?

Jan
>+                break;
>             }
> 
>             if ( rc == 0 )

On 09/27/2012 10:27 AM, Jan Beulich wrote:
>>>> On 26.09.12 at 16:46, Wei Wang<wei.wang2@amd.com>  wrote:
>> @@ -3834,6 +3835,9 @@ long do_hvm_op(unsigned long op,
XEN_GUEST_HANDLE(void) arg)
>>              case HVM_PARAM_BUFIOREQ_EVTCHN:
>>                  rc = -EINVAL;
>>                  break;
>> +            case HVM_PARAM_IOMMU_BASE:
>> +                rc = guest_iommu_set_base(d, a.value);
>
> This suggests that you''re allowing for only a single IOMMU per
> guest - is that not going to become an issue sooner or later?
>
> Jan
>
>> +                break;
>>              }
>>
>>              if ( rc == 0 )
>
>
>
HI Jan,
I think that one iommu per guest is probably enough. Because guest IVRS 
table is totally virtual, it does not reflect any pci relationship of 
real systems. Even if qemu supports multi pci buses, we can still 
virtually group them together into one virtual IVRS table. It might be 
an issue if qemu uses multi pci segments, but so far even hardware iommu 
only uses segment 0. Additionally, the guest iommu is only used by ats 
capable GPUs. Normal passthrough device should not make use of it. So,, 
What do you think?

Thanks,
Wei

>>> On 15.10.12 at 12:00, Wei Wang <wei.wang2@amd.com> wrote:
> On 09/27/2012 10:27 AM, Jan Beulich wrote:
>>>>> On 26.09.12 at 16:46, Wei Wang<wei.wang2@amd.com> 
wrote:
>>> @@ -3834,6 +3835,9 @@ long do_hvm_op(unsigned long op,
XEN_GUEST_HANDLE(void)
> arg)
>>>              case HVM_PARAM_BUFIOREQ_EVTCHN:
>>>                  rc = -EINVAL;
>>>                  break;
>>> +            case HVM_PARAM_IOMMU_BASE:
>>> +                rc = guest_iommu_set_base(d, a.value);
>>
>> This suggests that you''re allowing for only a single IOMMU per
>> guest - is that not going to become an issue sooner or later?
>
> I think that one iommu per guest is probably enough. Because guest IVRS 
> table is totally virtual, it does not reflect any pci relationship of 
> real systems. Even if qemu supports multi pci buses, we can still 
> virtually group them together into one virtual IVRS table. It might be 
> an issue if qemu uses multi pci segments, but so far even hardware iommu 
> only uses segment 0. Additionally, the guest iommu is only used by ats 
> capable GPUs. Normal passthrough device should not make use of it. So,, 
> What do you think?
Especially the multi-segment aspect makes me think that the
interface should allow for multiple IOMMUs, even if the
implementation supports only one for now.

Jan

On 10/15/2012 12:11 PM, Jan Beulich wrote:
>>>> On 15.10.12 at 12:00, Wei Wang<wei.wang2@amd.com>  wrote:
>> On 09/27/2012 10:27 AM, Jan Beulich wrote:
>>>>>> On 26.09.12 at 16:46, Wei Wang<wei.wang2@amd.com>
wrote:
>>>> @@ -3834,6 +3835,9 @@ long do_hvm_op(unsigned long op,
XEN_GUEST_HANDLE(void)
>> arg)
>>>>               case HVM_PARAM_BUFIOREQ_EVTCHN:
>>>>                   rc = -EINVAL;
>>>>                   break;
>>>> +            case HVM_PARAM_IOMMU_BASE:
>>>> +                rc = guest_iommu_set_base(d, a.value);
>>>
>>> This suggests that you''re allowing for only a single IOMMU
per
>>> guest - is that not going to become an issue sooner or later?
>>
>> I think that one iommu per guest is probably enough. Because guest IVRS
>> table is totally virtual, it does not reflect any pci relationship of
>> real systems. Even if qemu supports multi pci buses, we can still
>> virtually group them together into one virtual IVRS table. It might be
>> an issue if qemu uses multi pci segments, but so far even hardware
iommu
>> only uses segment 0. Additionally, the guest iommu is only used by ats
>> capable GPUs. Normal passthrough device should not make use of it. So,,
>> What do you think?
>
> Especially the multi-segment aspect makes me think that the
> interface should allow for multiple IOMMUs, even if the
> implementation supports only one for now.
Ok, then I will rework the interface to take iommu segment as an 
additional parameter.
Thanks,
Wei
> Jan
>
>

>>> On 15.10.12 at 14:23, Wei Wang <wei.wang2@amd.com> wrote:
> On 10/15/2012 12:11 PM, Jan Beulich wrote:
>>>>> On 15.10.12 at 12:00, Wei Wang<wei.wang2@amd.com> 
wrote:
>>> On 09/27/2012 10:27 AM, Jan Beulich wrote:
>>>>>>> On 26.09.12 at 16:46, Wei
Wang<wei.wang2@amd.com>   wrote:
>>>>> @@ -3834,6 +3835,9 @@ long do_hvm_op(unsigned long op,
XEN_GUEST_HANDLE(void)
>>> arg)
>>>>>               case HVM_PARAM_BUFIOREQ_EVTCHN:
>>>>>                   rc = -EINVAL;
>>>>>                   break;
>>>>> +            case HVM_PARAM_IOMMU_BASE:
>>>>> +                rc = guest_iommu_set_base(d, a.value);
>>>>
>>>> This suggests that you''re allowing for only a single
IOMMU per
>>>> guest - is that not going to become an issue sooner or later?
>>>
>>> I think that one iommu per guest is probably enough. Because guest
IVRS
>>> table is totally virtual, it does not reflect any pci relationship
of
>>> real systems. Even if qemu supports multi pci buses, we can still
>>> virtually group them together into one virtual IVRS table. It might
be
>>> an issue if qemu uses multi pci segments, but so far even hardware
iommu
>>> only uses segment 0. Additionally, the guest iommu is only used by
ats
>>> capable GPUs. Normal passthrough device should not make use of it.
So,,
>>> What do you think?
>>
>> Especially the multi-segment aspect makes me think that the
>> interface should allow for multiple IOMMUs, even if the
>> implementation supports only one for now.
> 
> Ok, then I will rework the interface to take iommu segment as an 
> additional parameter.
That''ll likely make the interface even more ugly than the more
flexible variant allowing for multiple IOMMUs independent of
the segment they''re on/for. But let''s see what you come up
with...

Jan

On 10/15/2012 02:52 PM, Jan Beulich wrote:
>>>> On 15.10.12 at 14:23, Wei Wang<wei.wang2@amd.com>  wrote:
>> On 10/15/2012 12:11 PM, Jan Beulich wrote:
>>>>>> On 15.10.12 at 12:00, Wei Wang<wei.wang2@amd.com>
wrote:
>>>> On 09/27/2012 10:27 AM, Jan Beulich wrote:
>>>>>>>> On 26.09.12 at 16:46, Wei
Wang<wei.wang2@amd.com>    wrote:
>>>>>> @@ -3834,6 +3835,9 @@ long do_hvm_op(unsigned long op,
XEN_GUEST_HANDLE(void)
>>>> arg)
>>>>>>                case HVM_PARAM_BUFIOREQ_EVTCHN:
>>>>>>                    rc = -EINVAL;
>>>>>>                    break;
>>>>>> +            case HVM_PARAM_IOMMU_BASE:
>>>>>> +                rc = guest_iommu_set_base(d, a.value);
>>>>>
>>>>> This suggests that you''re allowing for only a
single IOMMU per
>>>>> guest - is that not going to become an issue sooner or
later?
>>>>
>>>> I think that one iommu per guest is probably enough. Because
guest IVRS
>>>> table is totally virtual, it does not reflect any pci
relationship of
>>>> real systems. Even if qemu supports multi pci buses, we can
still
>>>> virtually group them together into one virtual IVRS table. It
might be
>>>> an issue if qemu uses multi pci segments, but so far even
hardware iommu
>>>> only uses segment 0. Additionally, the guest iommu is only used
by ats
>>>> capable GPUs. Normal passthrough device should not make use of
it. So,,
>>>> What do you think?
>>>
>>> Especially the multi-segment aspect makes me think that the
>>> interface should allow for multiple IOMMUs, even if the
>>> implementation supports only one for now.
>>
>> Ok, then I will rework the interface to take iommu segment as an
>> additional parameter.
>
> That''ll likely make the interface even more ugly than the more
> flexible variant allowing for multiple IOMMUs independent of
> the segment they''re on/for. But let''s see what you come
up
> with...
Hi Jan
My idea is to make the interface looks like that
guest_iommu_set_base(d, iommu_base, iommu_seg)

This will allow hvmloader to choose a non-zero iommu segment.
if iommu_seg > 0, then I will add a new iommu to guest iommu list and 
update iommu_segment field accordingly. But I seem to see no reason to 
add multiple guest iommus to pci segment 0.

Thanks,
Wei
> Jan
>
>

>>> On 15.10.12 at 15:41, Wei Wang <wei.wang2@amd.com> wrote:
> My idea is to make the interface looks like that
> guest_iommu_set_base(d, iommu_base, iommu_seg)
That part is obvious; the more interesting part is how you plan
on getting this fix the HVM parameter model.
> This will allow hvmloader to choose a non-zero iommu segment.
> if iommu_seg > 0, then I will add a new iommu to guest iommu list and 
> update iommu_segment field accordingly. But I seem to see no reason to 
> add multiple guest iommus to pci segment 0.
I don''t see a need for this at present too, and as I said before
there''s no need to implement support for multiple IOMMUs. The
interface definition, however, has to be flexible enough so we
don''t need to introduce another interface in a few years time
just because right now we don''t expect multiple IOMMUs to be
needed for guests (after all, there must be reasons why real
hardware frequently comes with more than one IOMMU despite
supporting only a single PCI segment - just consider the guest
visible NUMA case, where you would quite likely want an IOMMU
per guest visible node).

Jan

Wei Wang wrote on 2012-09-26:
>
@@ -3834,6 +3835,9 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE(void)
arg)
             case HVM_PARAM_BUFIOREQ_EVTCHN:
                 rc = -EINVAL;
                 break;
+            case HVM_PARAM_IOMMU_BASE:
+                rc = guest_iommu_set_base(d, a.value);
+                break;
             }
 
             if ( rc == 0 )

We also have the same function. It''s better to use platform specific
ops to call it in common file and just leave it as null in intel platform.