[PATCH 0/2] x86: Fix & refactoring for IOMMU unit counting

[Jan Kiszka]

Valentine, I bet you have a use case for patch 2...

Jan

Jan Kiszka (2):
x86: vtd: Do not overrun while counting IOMMU units
x86: Factor out iommu_count_units

hypervisor/arch/x86/Makefile | 2 +-
hypervisor/arch/x86/include/asm/iommu.h | 2 ++
hypervisor/arch/x86/iommu.c | 24 ++++++++++++++++++++++++
hypervisor/arch/x86/vtd.c | 6 ++----
4 files changed, 29 insertions(+), 5 deletions(-)
create mode 100644 hypervisor/arch/x86/iommu.c

--
2.1.4

[Jan Kiszka]

Will be required for both IOMMU variants.

Signed-off-by: Jan Kiszka <jan.k...@siemens.com>
---

hypervisor/arch/x86/Makefile | 2 +-
hypervisor/arch/x86/include/asm/iommu.h | 2 ++
hypervisor/arch/x86/iommu.c | 24 ++++++++++++++++++++++++

hypervisor/arch/x86/vtd.c | 7 ++-----
4 files changed, 29 insertions(+), 6 deletions(-)
create mode 100644 hypervisor/arch/x86/iommu.c

diff --git a/hypervisor/arch/x86/Makefile b/hypervisor/arch/x86/Makefile
index 012ec46..0d3670e 100644
--- a/hypervisor/arch/x86/Makefile
+++ b/hypervisor/arch/x86/Makefile
@@ -13,7 +13,7 @@
#

BUILT_IN_OBJECTS := built-in-amd.o built-in-intel.o
-COMMON_OBJECTS := apic.o dbg-write.o entry.o setup.o control.o mmio.o \
+COMMON_OBJECTS := apic.o dbg-write.o entry.o setup.o control.o mmio.o iommu.o \
paging.o ../../pci.o pci.o ioapic.o i8042.o vcpu.o \
../../pci_ivshmem.o

diff --git a/hypervisor/arch/x86/include/asm/iommu.h b/hypervisor/arch/x86/include/asm/iommu.h
index e476bbf..a0be3c3 100644
--- a/hypervisor/arch/x86/include/asm/iommu.h
+++ b/hypervisor/arch/x86/include/asm/iommu.h
@@ -21,6 +21,8 @@
#include <asm/cell.h>
#include <asm/percpu.h>

+unsigned int iommu_count_units(void);
+
int iommu_init(void);

int iommu_cell_init(struct cell *cell);
diff --git a/hypervisor/arch/x86/iommu.c b/hypervisor/arch/x86/iommu.c
new file mode 100644
index 0000000..3da61d6
--- /dev/null
+++ b/hypervisor/arch/x86/iommu.c
@@ -0,0 +1,24 @@
+/*
+ * Jailhouse, a Linux-based partitioning hypervisor
+ *
+ * Copyright (c) Siemens AG, 2014, 2015
+ *
+ * Authors:
+ * Jan Kiszka <jan.k...@siemens.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ */
+
+#include <jailhouse/control.h>
+#include <asm/iommu.h>
+
+unsigned int iommu_count_units(void)
+{
+ unsigned int units = 0;
+
+ while (units < JAILHOUSE_MAX_IOMMU_UNITS &&
+ system_config->platform_info.x86.iommu_base[units])
+ units++;
+ return units;
+}
diff --git a/hypervisor/arch/x86/vtd.c b/hypervisor/arch/x86/vtd.c
index 44474fb..d36c8ce 100644
--- a/hypervisor/arch/x86/vtd.c
+++ b/hypervisor/arch/x86/vtd.c
@@ -463,8 +463,7 @@ static int vtd_init_ir_emulation(unsigned int unit_no, void *reg_base)
int iommu_init(void)
{
unsigned long version, caps, ecaps, ctrls, sllps_caps = ~0UL;
- unsigned int pt_levels, num_did, n;
- unsigned int units = 0;
+ unsigned int units, pt_levels, num_did, n;
void *reg_base;
u64 base_addr;
int err;
@@ -482,9 +481,7 @@ int iommu_init(void)

int_remap_table_size_log2 = n;

- while (units < JAILHOUSE_MAX_IOMMU_UNITS &&
- system_config->platform_info.x86.iommu_base[units])
- units++;
+ units = iommu_count_units();
if (units == 0)
return trace_error(-EINVAL);

--
2.1.4

[Valentine Sinitsyn]

Hi Jan,

On 02.07.2015 12:53, Jan Kiszka wrote:
> Valentine, I bet you have a use case for patch 2...

Yes, although I change the config structure a bit (need to pass
additional flags from ACPI to Jailhouse). Will account for this new
function when rebasing, thanks!

Valentine

[Jan Kiszka]

Perfect.

BTW, I'm not changing IOMMU code significantly right now, just hacking a
bit (at slow pace) on better MMIO dispatching. For that I need to
pre-calculate the number of regions a cell will have, thus also require
the number of VT-d units that emulate IR.

Jan

--
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

[Valentine Sinitsyn]

On 02.07.2015 13:02, Jan Kiszka wrote:
> On 2015-07-02 09:58, Valentine Sinitsyn wrote:
>> Hi Jan,
>>
>> On 02.07.2015 12:53, Jan Kiszka wrote:
>>> Valentine, I bet you have a use case for patch 2...
>> Yes, although I change the config structure a bit (need to pass
>> additional flags from ACPI to Jailhouse). Will account for this new
>> function when rebasing, thanks!
>
> Perfect.
>
> BTW, I'm not changing IOMMU code significantly right now, just hacking a
> bit (at slow pace) on better MMIO dispatching. For that I need to
> pre-calculate the number of regions a cell will have, thus also require
> the number of VT-d units that emulate IR.

Sound like we'll finally need to unionize iommu member in the config?

Regarding better dispatching: I noticed that we iterate over all IOMMUs
enabled sometimes, like in vtd_flush_domain_caches(). Actually, I do the
same in my code (a side note: I should finally rebase it and post on
GitHub so we discuss something we both see). Wouldn't it be better to
iterate only over IOMMUs affected (i.e. servicing the Domain ID in
question)?

Valentine

[Jan Kiszka]

On 2015-07-02 10:08, Valentine Sinitsyn wrote:
> On 02.07.2015 13:02, Jan Kiszka wrote:
>> On 2015-07-02 09:58, Valentine Sinitsyn wrote:
>>> Hi Jan,
>>>
>>> On 02.07.2015 12:53, Jan Kiszka wrote:
>>>> Valentine, I bet you have a use case for patch 2...
>>> Yes, although I change the config structure a bit (need to pass
>>> additional flags from ACPI to Jailhouse). Will account for this new
>>> function when rebasing, thanks!
>>
>> Perfect.
>>
>> BTW, I'm not changing IOMMU code significantly right now, just hacking a
>> bit (at slow pace) on better MMIO dispatching. For that I need to
>> pre-calculate the number of regions a cell will have, thus also require
>> the number of VT-d units that emulate IR.
> Sound like we'll finally need to unionize iommu member in the config?

Cannot follow yet - which members do you have in mind?

>
> Regarding better dispatching: I noticed that we iterate over all IOMMUs
> enabled sometimes, like in vtd_flush_domain_caches(). Actually, I do the
> same in my code (a side note: I should finally rebase it and post on
> GitHub so we discuss something we both see). Wouldn't it be better to
> iterate only over IOMMUs affected (i.e. servicing the Domain ID in
> question)?

Config updates are slow paths, so I went for simplicity. If the
optimization is trivial, I wouldn't oppose, though.

[Valentine Sinitsyn]

On 02.07.2015 13:12, Jan Kiszka wrote:
> On 2015-07-02 10:08, Valentine Sinitsyn wrote:
>> On 02.07.2015 13:02, Jan Kiszka wrote:
>>> On 2015-07-02 09:58, Valentine Sinitsyn wrote:
>>>> Hi Jan,
>>>>
>>>> On 02.07.2015 12:53, Jan Kiszka wrote:
>>>>> Valentine, I bet you have a use case for patch 2...
>>>> Yes, although I change the config structure a bit (need to pass
>>>> additional flags from ACPI to Jailhouse). Will account for this new
>>>> function when rebasing, thanks!
>>>
>>> Perfect.
>>>
>>> BTW, I'm not changing IOMMU code significantly right now, just hacking a
>>> bit (at slow pace) on better MMIO dispatching. For that I need to
>>> pre-calculate the number of regions a cell will have, thus also require
>>> the number of VT-d units that emulate IR.
>> Sound like we'll finally need to unionize iommu member in the config?
>
> Cannot follow yet - which members do you have in mind?

Sorry. I meant we are both changing how to specify IOMMU parameters in
struct jailhouse_cell_config, so I thought decoupling AMD and VTD
implementation the way we do it with SVM/VMX would be convenient.

Valentine

[Jan Kiszka]

Actually, I'm not changing the config for that, just using existing
information.

I guess we best discuss this based on a first version for the AMD IOMMU.
I'd like to see which parameters you need.

[Valentine Sinitsyn]

Yes, makes sense. Let's return to this discussion as I push my initial
version.

Valentine