Re: [RFC 2/6] linux-stubdomain: Compile Linux

[Anthony PERARD <anthony.perard@citrix.com>]

On 19/04/13 11:33, Stefano Stabellini wrote:
> On Wed, 17 Apr 2013, Anthony PERARD wrote:
>> diff --git a/stubdom-linux/0001-xen-Don-t-check-for-xen_initial_domain-in-privcmd_io.patch b/stubdom-linux/0001-xen-Don-t-check-for-xen_initial_domain-in-privcmd_io.patch
>> new file mode 100644
>> index 0000000..627b337
>> --- /dev/null
>> +++ b/stubdom-linux/0001-xen-Don-t-check-for-xen_initial_domain-in-privcmd_io.patch
>> @@ -0,0 +1,39 @@
>> +From 94d3502e70882a78ec3abb22379a79afc1292fb0 Mon Sep 17 00:00:00 2001
>> +From: Anthony PERARD <anthony.perard@citrix.com>
>> +Date: Fri, 1 Jun 2012 15:46:39 +0100
>> +Subject: [PATCH 1/2] xen: Don't check for xen_initial_domain in
>> + privcmd_ioctl_mmap*.
>> +
>> +This prevent a stubdom from working.
>> +
>> +---
>> + drivers/xen/privcmd.c | 6 ------
>> + 1 file changed, 6 deletions(-)
>> +
>> +diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
>> +index ccee0f1..a8d71a3 100644
>> +--- a/drivers/xen/privcmd.c
>> ++++ b/drivers/xen/privcmd.c
>> +@@ -196,9 +196,6 @@ static long privcmd_ioctl_mmap(void __user *udata)
>> +       LIST_HEAD(pagelist);
>> +       struct mmap_mfn_state state;
>> +
>> +-      if (!xen_initial_domain())
>> +-              return -EPERM;
>> +-
>> +       if (copy_from_user(&mmapcmd, udata, sizeof(mmapcmd)))
>> +               return -EFAULT;
>> +
>> +@@ -286,9 +283,6 @@ static long privcmd_ioctl_mmap_batch(void __user *udata)
>> +       LIST_HEAD(pagelist);
>> +       struct mmap_batch_state state;
>> +
>> +-      if (!xen_initial_domain())
>> +-              return -EPERM;
>> +-
>> +       if (copy_from_user(&m, udata, sizeof(m)))
>> +               return -EFAULT;
>> +
> 
> 
> I think you should submit both patches separately for inclusion in the
> Linux kernel.

Definitely.
Should we remove the check for initial_domain all together and leave
this permission to be handle by Xen? Or should we try to find out if the
function is called in a stubdom/dom0 ?

>> +Anthony PERARD
>> +
>> diff --git a/stubdom-linux/0002-fix-remap_area_mfn_pte_fn.patch b/stubdom-linux/0002-fix-remap_area_mfn_pte_fn.patch
>> new file mode 100644
>> index 0000000..0d5c262
>> --- /dev/null
>> +++ b/stubdom-linux/0002-fix-remap_area_mfn_pte_fn.patch
>> @@ -0,0 +1,36 @@
>> +From 61cd574f29f41046f1c709cfa9da118156babf83 Mon Sep 17 00:00:00 2001
>> +From: Anthony PERARD <anthony.perard@citrix.com>
>> +Date: Fri, 1 Jun 2012 15:47:01 +0100
>> +Subject: [PATCH 2/2] fix/remap_area_mfn_pte_fn
> 
> I think we need a better commit message

Yes, this is definitely not a patch title/comment.

>> +---
>> + arch/x86/xen/mmu.c | 13 ++++++++++++-
>> + 1 file changed, 12 insertions(+), 1 deletion(-)
>> +
>> +diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
>> +index 69f5857..999fc82 100644
>> +--- a/arch/x86/xen/mmu.c
>> ++++ b/arch/x86/xen/mmu.c
>> +@@ -2315,7 +2315,18 @@ static int remap_area_mfn_pte_fn(pte_t *ptep, pgtable_t token,
>> +                                unsigned long addr, void *data)
>> + {
>> +       struct remap_data *rmd = data;
>> +-      pte_t pte = pte_mkspecial(pfn_pte(rmd->mfn++, rmd->prot));
>> ++
>> ++        /* Use the native_make_pte function because we are sure we don't
>> ++         * have to do any pfn->mfn translations but at the same time we
>> ++         * could in a stubdom so xen_initial_domain() would return false. */
>> ++        pte_t pte = pte_mkspecial(native_make_pte(((phys_addr_t)(rmd->mfn++) << PAGE_SHIFT)
>> ++                                                  | massage_pgprot(rmd->prot)));
> 
> This change is OK. The stubdom part of the comment is a bit confusing
> and would benefit from a clearer explanation.

OK, I try to clean the explanation.

> Also the indentation is wrong.
> 
> 
>> ++        pteval_t val = pte_val_ma(pte);
>> ++
>> ++        if (pat_enabled && !WARN_ON(val & _PAGE_PAT)) {
>> ++            if ((val & (_PAGE_PCD | _PAGE_PWT)) == _PAGE_PWT)
>> ++                val = (val & ~(_PAGE_PCD | _PAGE_PWT)) | _PAGE_PAT;
>> ++        }
> 
> Konrad disabled PAT in upstream kernels, see:
> 
> commit 8eaffa67b43e99ae581622c5133e20b0f48bcef1
> Author: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date:   Fri Feb 10 09:16:27 2012 -0500
> 
>     xen/pat: Disable PAT support for now.
> 


-- 
Anthony PERARD