[PATCH 2 of 6 V6] amd iommu: call guest_iommu_set_base from hvmloader

[PATCH 2 of 6 V6] amd iommu: call guest_iommu_set_base from hvmloader

[Wei Wang]

# HG changeset patch
# User Wei Wang <wei.wang2@amd.com>
# Date 1331210214 -3600
# Node ID e9d74ec1077472f9127c43903811ce3107fc038d
# Parent  810296995a893c1bec898a366f08520fbae755fb
amd iommu: call guest_iommu_set_base from hvmloader.

IOMMU MMIO base address is dynamically allocated by firmware.
This patch allows hvmloader to notify hypervisor where the
iommu mmio pages are.

Signed-off-by: Wei Wang <wei.wang2@amd.com>

diff -r 810296995a89 -r e9d74ec10774 xen/arch/x86/hvm/hvm.c
--- a/xen/arch/x86/hvm/hvm.c	Thu Mar 08 13:36:51 2012 +0100
+++ b/xen/arch/x86/hvm/hvm.c	Thu Mar 08 13:36:54 2012 +0100
@@ -66,6 +66,7 @@
 #include <asm/mem_event.h>
 #include <asm/mem_access.h>
 #include <public/mem_event.h>
+#include <asm/hvm/svm/amd-iommu-proto.h>
 
 bool_t __read_mostly hvm_enabled;
 
@@ -3786,6 +3787,9 @@ long do_hvm_op(unsigned long op, XEN_GUE
             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 ) 
diff -r 810296995a89 -r e9d74ec10774 xen/include/public/hvm/params.h
--- a/xen/include/public/hvm/params.h	Thu Mar 08 13:36:51 2012 +0100
+++ b/xen/include/public/hvm/params.h	Thu Mar 08 13:36:54 2012 +0100
@@ -141,7 +141,8 @@
 
 /* Boolean: Enable nestedhvm (hvm only) */
 #define HVM_PARAM_NESTEDHVM    24
+#define HVM_PARAM_IOMMU_BASE   27
 
-#define HVM_NR_PARAMS          27
+#define HVM_NR_PARAMS          28
 
 #endif /* __XEN_PUBLIC_HVM_PARAMS_H__ */

[PATCH 2 of 6 V6] amd iommu: call guest_iommu_set_base from hvmloader

[Wei Wang]

[-- Attachment #1: Type: text/plain, Size: 2 bytes --]




[-- Attachment #2: 0002-amd-iommu-call-guest_iommu_set_base-from-hvmloader.patch --]
[-- Type: text/plain, Size: 1724 bytes --]

From 8110e0a223488a1123fa805051905d8788610cf2 Mon Sep 17 00:00:00 2001
From: Wei Wang <wei.wang2@amd.com>
Date: Wed, 26 Sep 2012 11:44:19 +0200
Subject: [PATCH 2/6] amd iommu: call guest_iommu_set_base from hvmloader.

IOMMU MMIO base address is dynamically allocated by firmware.
This patch allows hvmloader to notify hypervisor where the
iommu mmio pages are.

Signed-off-by: Wei Wang <wei.wang2@amd.com>
---
 xen/arch/x86/hvm/hvm.c          |    4 ++++
 xen/include/public/hvm/params.h |    3 ++-
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index d69e419..e5c1e32 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -66,6 +66,7 @@
 #include <asm/mem_event.h>
 #include <asm/mem_access.h>
 #include <public/mem_event.h>
+#include <asm/hvm/svm/amd-iommu-proto.h>
 
 bool_t __read_mostly hvm_enabled;
 
@@ -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 ) 
diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
index 6b05f61..aab6b7a 100644
--- a/xen/include/public/hvm/params.h
+++ b/xen/include/public/hvm/params.h
@@ -140,7 +140,8 @@
 #define HVM_PARAM_PAGING_RING_PFN   27
 #define HVM_PARAM_ACCESS_RING_PFN   28
 #define HVM_PARAM_SHARING_RING_PFN  29
+#define HVM_PARAM_IOMMU_BASE   30
 
-#define HVM_NR_PARAMS          30
+#define HVM_NR_PARAMS          31
 
 #endif /* __XEN_PUBLIC_HVM_PARAMS_H__ */
-- 
1.7.4



[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 2 of 6 V6] amd iommu: call guest_iommu_set_base from hvmloader

[Jan Beulich]

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

Re: [PATCH 2 of 6 V6] amd iommu: call guest_iommu_set_base from hvmloader

[Wei Wang]

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

Re: [PATCH 2 of 6 V6] amd iommu: call guest_iommu_set_base from hvmloader

[Jan Beulich]

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

Re: [PATCH 2 of 6 V6] amd iommu: call guest_iommu_set_base from hvmloader

[Wei Wang]

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

Re: [PATCH 2 of 6 V6] amd iommu: call guest_iommu_set_base from hvmloader

[Jan Beulich]

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

Re: [PATCH 2 of 6 V6] amd iommu: call guest_iommu_set_base from hvmloader

[Wei Wang]

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

Re: [PATCH 2 of 6 V6] amd iommu: call guest_iommu_set_base from hvmloader

[Jan Beulich]

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

Re: [PATCH 2 of 6 V6] amd iommu: call guest_iommu_set_base from hvmloader

[Zhang, Yang Z]

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.