[PATCH] x86: Embed page for EPT/NPT root_table into cell structure

[Jan Kiszka]

Both Intel and AMD need this page and currently allocate it
programmatically. We can safe some logic, specifically error handling,
by reserving the page in the cell structure.

Signed-off-by: Jan Kiszka <jan.k...@siemens.com>
---
hypervisor/arch/x86/include/asm/cell.h | 3 +++
hypervisor/arch/x86/svm.c | 9 ++-------
hypervisor/arch/x86/vmx.c | 9 ++-------
3 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/hypervisor/arch/x86/include/asm/cell.h b/hypervisor/arch/x86/include/asm/cell.h
index 81050c5..668620d 100644
--- a/hypervisor/arch/x86/include/asm/cell.h
+++ b/hypervisor/arch/x86/include/asm/cell.h
@@ -92,6 +92,9 @@ struct cell {
u8 padding[PAGE_SIZE];
} __attribute__((aligned(PAGE_SIZE))) comm_page;
/**< Page containing the communication region (shared with cell). */
+
+ /** Buffer for the EPT/NPT root-level page table. */
+ u8 __attribute__((aligned(PAGE_SIZE))) root_table_page[PAGE_SIZE];
};

extern struct cell root_cell;
diff --git a/hypervisor/arch/x86/svm.c b/hypervisor/arch/x86/svm.c
index b824472..bc72327 100644
--- a/hypervisor/arch/x86/svm.c
+++ b/hypervisor/arch/x86/svm.c
@@ -298,9 +298,7 @@ int vcpu_vendor_cell_init(struct cell *cell)

/* build root NPT of cell */
cell->svm.npt_structs.root_paging = npt_paging;
- cell->svm.npt_structs.root_table = page_alloc(&mem_pool, 1);
- if (!cell->svm.npt_structs.root_table)
- goto err_free_iopm;
+ cell->svm.npt_structs.root_table = (page_table_t)cell->root_table_page;

if (!has_avic) {
/*
@@ -320,12 +318,10 @@ int vcpu_vendor_cell_init(struct cell *cell)
PAGING_NON_COHERENT);
}
if (err)
- goto err_free_root_table;
+ goto err_free_iopm;

return 0;

-err_free_root_table:
- page_free(&mem_pool, cell->svm.npt_structs.root_table, 1);
err_free_iopm:
page_free(&mem_pool, cell->svm.iopm, 3);

@@ -362,7 +358,6 @@ void vcpu_vendor_cell_exit(struct cell *cell)
{
paging_destroy(&cell->svm.npt_structs, XAPIC_BASE, PAGE_SIZE,
PAGING_NON_COHERENT);
- page_free(&mem_pool, cell->svm.npt_structs.root_table, 1);
page_free(&mem_pool, cell->svm.iopm, 3);
}

diff --git a/hypervisor/arch/x86/vmx.c b/hypervisor/arch/x86/vmx.c
index 575cc70..d922f0f 100644
--- a/hypervisor/arch/x86/vmx.c
+++ b/hypervisor/arch/x86/vmx.c
@@ -335,9 +335,7 @@ int vcpu_vendor_cell_init(struct cell *cell)

/* build root EPT of cell */
cell->vmx.ept_structs.root_paging = ept_paging;
- cell->vmx.ept_structs.root_table = page_alloc(&mem_pool, 1);
- if (!cell->vmx.ept_structs.root_table)
- goto err_free_io_bitmap;
+ cell->vmx.ept_structs.root_table = (page_table_t)cell->root_table_page;

err = paging_create(&cell->vmx.ept_structs,
paging_hvirt2phys(apic_access_page),
@@ -345,12 +343,10 @@ int vcpu_vendor_cell_init(struct cell *cell)
EPT_FLAG_READ | EPT_FLAG_WRITE | EPT_FLAG_WB_TYPE,
PAGING_NON_COHERENT);
if (err)
- goto err_free_root_table;
+ goto err_free_io_bitmap;

return 0;

-err_free_root_table:
- page_free(&mem_pool, cell->vmx.ept_structs.root_table, 1);
err_free_io_bitmap:
page_free(&mem_pool, cell->vmx.io_bitmap, 2);

@@ -387,7 +383,6 @@ void vcpu_vendor_cell_exit(struct cell *cell)
{
paging_destroy(&cell->vmx.ept_structs, XAPIC_BASE, PAGE_SIZE,
PAGING_NON_COHERENT);
- page_free(&mem_pool, cell->vmx.ept_structs.root_table, 1);
page_free(&mem_pool, cell->vmx.io_bitmap, 2);
}

--
2.1.4

[Valentine Sinitsyn]

Hi Jan,

On 01.07.2015 10:19, Jan Kiszka wrote:
> Both Intel and AMD need this page and currently allocate it
> programmatically. We can safe some logic, specifically error handling,
> by reserving the page in the cell structure.

We need another page for IOMMU page root as well. Should we also embed it?

Just a note: AMD IOMMU can actually share page tables with NPT, but
separate root are needed in this case as well.

Valentine

[Jan Kiszka]

On 2015-07-01 08:08, Valentine Sinitsyn wrote:
> Hi Jan,
>
> On 01.07.2015 10:19, Jan Kiszka wrote:
>> Both Intel and AMD need this page and currently allocate it
>> programmatically. We can safe some logic, specifically error handling,
>> by reserving the page in the cell structure.
> We need another page for IOMMU page root as well. Should we also embed it?

Yes, would be consistent.

>
> Just a note: AMD IOMMU can actually share page tables with NPT, but
> separate root are needed in this case as well.

So the root table structure is different, but the rest is the same
again, or how does this come? Do you see a chance to exploit?

Jan

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

[Jan Kiszka]

On 2015-07-01 08:32, Jan Kiszka wrote:
> On 2015-07-01 08:08, Valentine Sinitsyn wrote:
>>
>> Just a note: AMD IOMMU can actually share page tables with NPT, but
>> separate root are needed in this case as well.
>
> So the root table structure is different, but the rest is the same
> again, or how does this come? Do you see a chance to exploit?

In fact, there is currently a difference in how we build both tables
semantically: device only see regions that are marked as DMA-capable
while VCPUs see them all. We would have to give up on this filtering
then. Not sure, though, if it is useful except for reducing the IOMMU
table size a bit.

[Valentine Sinitsyn]

I haven't tried to exploit in, mainly due to the fact you already
mentioned: we map memory for I/O and for the cells differently. So it's
on "TODO/Maybe somewhere in future" list now.

Allocating IOMMU root pt statically seems to be a good idea anyways,
these are barely related things.

On 01.07.2015 11:53, Jan Kiszka wrote:
> On 2015-07-01 08:32, Jan Kiszka wrote:
>> On 2015-07-01 08:08, Valentine Sinitsyn wrote:
>>>
>>> Just a note: AMD IOMMU can actually share page tables with NPT, but
>>> separate root are needed in this case as well.
>>
>> So the root table structure is different, but the rest is the same
>> again, or how does this come? Do you see a chance to exploit?
>
> In fact, there is currently a difference in how we build both tables
> semantically: device only see regions that are marked as DMA-capable
> while VCPUs see them all. We would have to give up on this filtering
> then. Not sure, though, if it is useful except for reducing the IOMMU
> table size a bit.
>
> Jan
>

Valentine

[Jan Kiszka]

On 2015-07-01 09:05, Valentine Sinitsyn wrote:
> I haven't tried to exploit in, mainly due to the fact you already
> mentioned: we map memory for I/O and for the cells differently. So it's
> on "TODO/Maybe somewhere in future" list now.

Yes, one after the other. We would basically have to redefine
JAILHOUSE_MEM_DMA as "must be DMA", while other region then become "can
be DMA as well".