* Re: [v2][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT
2014-07-24 9:11 ` Andrew Cooper
@ 2014-07-24 9:31 ` Jan Beulich
2014-07-24 16:56 ` Tian, Kevin
2014-07-24 9:39 ` Chen, Tiejun
2014-07-24 9:47 ` Tim Deegan
2 siblings, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2014-07-24 9:31 UTC (permalink / raw)
To: Andrew Cooper, Tiejun Chen; +Cc: yang.z.zhang, kevin.tian, xen-devel
>>> On 24.07.14 at 11:11, <andrew.cooper3@citrix.com> wrote:
> On 24/07/14 10:03, Chen, Tiejun wrote:
>> On 2014/7/24 16:57, Andrew Cooper wrote:
>>> On 24/07/14 09:50, Tiejun Chen wrote:
>>>> --- a/xen/drivers/passthrough/vtd/iommu.c
>>>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>>>> @@ -41,6 +41,7 @@
>>>> #include "extern.h"
>>>> #include "vtd.h"
>>>> #include "../ats.h"
>>>> +#include "../../../arch/x86/mm/mm-locks.h"
>>>
>>> <asm/mm/mm-locks.h>
>>
>> iommu.c:38:29: fatal error: asm/mm/mm-locks.h: No such file or directory
>> #include <asm/mm/mm-locks.h>
>> ^
>> compilation terminated.
>>
>> Tiejun
>
> Hmm - my mistake. The lack of easy access to this header file goes to
> emphasise that it is private to arch/x86/mm, and there are indeed no
> current users outside of that part of the tree.
>
> Would it not be better have some public p2m_set_identity() function in
> p2m.c ?
Actually I think this should be using set_mmio_p2m_entry(), which in
turn points out another problem: What is happening with the GFN
that gets replaced here? How will the guest know this is not RAM? I
pointed out on an earlier occasion that passing through devices with
associated RMRR is problematic/dangerous - this is another proof.
Perhaps this should be disallowed, at least when sharing page tables.
Jan
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v2][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT
2014-07-24 9:31 ` Jan Beulich
@ 2014-07-24 16:56 ` Tian, Kevin
2014-07-25 6:57 ` Jan Beulich
0 siblings, 1 reply; 11+ messages in thread
From: Tian, Kevin @ 2014-07-24 16:56 UTC (permalink / raw)
To: Jan Beulich, Andrew Cooper, Chen, Tiejun
Cc: Zhang, Yang Z, xen-devel@lists.xen.org
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Thursday, July 24, 2014 2:32 AM
>
> >>> On 24.07.14 at 11:11, <andrew.cooper3@citrix.com> wrote:
> > On 24/07/14 10:03, Chen, Tiejun wrote:
> >> On 2014/7/24 16:57, Andrew Cooper wrote:
> >>> On 24/07/14 09:50, Tiejun Chen wrote:
> >>>> --- a/xen/drivers/passthrough/vtd/iommu.c
> >>>> +++ b/xen/drivers/passthrough/vtd/iommu.c
> >>>> @@ -41,6 +41,7 @@
> >>>> #include "extern.h"
> >>>> #include "vtd.h"
> >>>> #include "../ats.h"
> >>>> +#include "../../../arch/x86/mm/mm-locks.h"
> >>>
> >>> <asm/mm/mm-locks.h>
> >>
> >> iommu.c:38:29: fatal error: asm/mm/mm-locks.h: No such file or directory
> >> #include <asm/mm/mm-locks.h>
> >> ^
> >> compilation terminated.
> >>
> >> Tiejun
> >
> > Hmm - my mistake. The lack of easy access to this header file goes to
> > emphasise that it is private to arch/x86/mm, and there are indeed no
> > current users outside of that part of the tree.
> >
> > Would it not be better have some public p2m_set_identity() function in
> > p2m.c ?
>
> Actually I think this should be using set_mmio_p2m_entry(), which in
> turn points out another problem: What is happening with the GFN
> that gets replaced here? How will the guest know this is not RAM? I
> pointed out on an earlier occasion that passing through devices with
> associated RMRR is problematic/dangerous - this is another proof.
> Perhaps this should be disallowed, at least when sharing page tables.
>
yes, that's a problem. we need some way (possibly a new hypercall) to
tell user space reserving a list of GFN holes in guest e820. I'm not sure
the sequence of current e820 construction and device pass-through. A
simpler policy may be always reserve those holes even when no device
is assigned.
Thanks
Kevin
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v2][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT
2014-07-24 16:56 ` Tian, Kevin
@ 2014-07-25 6:57 ` Jan Beulich
2014-07-25 12:47 ` Tian, Kevin
0 siblings, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2014-07-25 6:57 UTC (permalink / raw)
To: Andrew Cooper, Kevin Tian, Tiejun Chen
Cc: Yang Z Zhang, xen-devel@lists.xen.org
>>> On 24.07.14 at 18:56, <kevin.tian@intel.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Thursday, July 24, 2014 2:32 AM
>>
>> >>> On 24.07.14 at 11:11, <andrew.cooper3@citrix.com> wrote:
>> > On 24/07/14 10:03, Chen, Tiejun wrote:
>> >> On 2014/7/24 16:57, Andrew Cooper wrote:
>> >>> On 24/07/14 09:50, Tiejun Chen wrote:
>> >>>> --- a/xen/drivers/passthrough/vtd/iommu.c
>> >>>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>> >>>> @@ -41,6 +41,7 @@
>> >>>> #include "extern.h"
>> >>>> #include "vtd.h"
>> >>>> #include "../ats.h"
>> >>>> +#include "../../../arch/x86/mm/mm-locks.h"
>> >>>
>> >>> <asm/mm/mm-locks.h>
>> >>
>> >> iommu.c:38:29: fatal error: asm/mm/mm-locks.h: No such file or directory
>> >> #include <asm/mm/mm-locks.h>
>> >> ^
>> >> compilation terminated.
>> >>
>> >> Tiejun
>> >
>> > Hmm - my mistake. The lack of easy access to this header file goes to
>> > emphasise that it is private to arch/x86/mm, and there are indeed no
>> > current users outside of that part of the tree.
>> >
>> > Would it not be better have some public p2m_set_identity() function in
>> > p2m.c ?
>>
>> Actually I think this should be using set_mmio_p2m_entry(), which in
>> turn points out another problem: What is happening with the GFN
>> that gets replaced here? How will the guest know this is not RAM? I
>> pointed out on an earlier occasion that passing through devices with
>> associated RMRR is problematic/dangerous - this is another proof.
>> Perhaps this should be disallowed, at least when sharing page tables.
>
> yes, that's a problem. we need some way (possibly a new hypercall) to
> tell user space reserving a list of GFN holes in guest e820. I'm not sure
> the sequence of current e820 construction and device pass-through. A
> simpler policy may be always reserve those holes even when no device
> is assigned.
Yes, that would be desirable, but not necessarily possible: On the
one system entertaining RMRRs that I have here, these regions are
in the range E9000...EEFFF, i.e. are - with a BIOS image of more
than 64k size - overlapping the base BIOS, at least until the resident
size of the BIOS has been determined. I'm not sure about the
consequences of this overlap.
Jan
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v2][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT
2014-07-25 6:57 ` Jan Beulich
@ 2014-07-25 12:47 ` Tian, Kevin
0 siblings, 0 replies; 11+ messages in thread
From: Tian, Kevin @ 2014-07-25 12:47 UTC (permalink / raw)
To: Jan Beulich, Andrew Cooper, Chen, Tiejun
Cc: Zhang, Yang Z, xen-devel@lists.xen.org
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Thursday, July 24, 2014 11:58 PM
>
> >>> On 24.07.14 at 18:56, <kevin.tian@intel.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Thursday, July 24, 2014 2:32 AM
> >>
> >> >>> On 24.07.14 at 11:11, <andrew.cooper3@citrix.com> wrote:
> >> > On 24/07/14 10:03, Chen, Tiejun wrote:
> >> >> On 2014/7/24 16:57, Andrew Cooper wrote:
> >> >>> On 24/07/14 09:50, Tiejun Chen wrote:
> >> >>>> --- a/xen/drivers/passthrough/vtd/iommu.c
> >> >>>> +++ b/xen/drivers/passthrough/vtd/iommu.c
> >> >>>> @@ -41,6 +41,7 @@
> >> >>>> #include "extern.h"
> >> >>>> #include "vtd.h"
> >> >>>> #include "../ats.h"
> >> >>>> +#include "../../../arch/x86/mm/mm-locks.h"
> >> >>>
> >> >>> <asm/mm/mm-locks.h>
> >> >>
> >> >> iommu.c:38:29: fatal error: asm/mm/mm-locks.h: No such file or
> directory
> >> >> #include <asm/mm/mm-locks.h>
> >> >> ^
> >> >> compilation terminated.
> >> >>
> >> >> Tiejun
> >> >
> >> > Hmm - my mistake. The lack of easy access to this header file goes to
> >> > emphasise that it is private to arch/x86/mm, and there are indeed no
> >> > current users outside of that part of the tree.
> >> >
> >> > Would it not be better have some public p2m_set_identity() function in
> >> > p2m.c ?
> >>
> >> Actually I think this should be using set_mmio_p2m_entry(), which in
> >> turn points out another problem: What is happening with the GFN
> >> that gets replaced here? How will the guest know this is not RAM? I
> >> pointed out on an earlier occasion that passing through devices with
> >> associated RMRR is problematic/dangerous - this is another proof.
> >> Perhaps this should be disallowed, at least when sharing page tables.
> >
> > yes, that's a problem. we need some way (possibly a new hypercall) to
> > tell user space reserving a list of GFN holes in guest e820. I'm not sure
> > the sequence of current e820 construction and device pass-through. A
> > simpler policy may be always reserve those holes even when no device
> > is assigned.
>
> Yes, that would be desirable, but not necessarily possible: On the
> one system entertaining RMRRs that I have here, these regions are
> in the range E9000...EEFFF, i.e. are - with a BIOS image of more
> than 64k size - overlapping the base BIOS, at least until the resident
> size of the BIOS has been determined. I'm not sure about the
> consequences of this overlap.
>
hypervisor provided list can be a reference, not a must. User space can
detect the confliction e.g. with large BIOS size and then reject assignment
of devices associated with conflicting region. but that also means exposing
not just the hole, but also the exact RMRR knowledge to user space...
Thanks
Kevin
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v2][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT
2014-07-24 9:11 ` Andrew Cooper
2014-07-24 9:31 ` Jan Beulich
@ 2014-07-24 9:39 ` Chen, Tiejun
2014-07-24 9:56 ` Tim Deegan
2014-07-24 9:47 ` Tim Deegan
2 siblings, 1 reply; 11+ messages in thread
From: Chen, Tiejun @ 2014-07-24 9:39 UTC (permalink / raw)
To: Andrew Cooper, JBeulich, yang.z.zhang, kevin.tian; +Cc: xen-devel
On 2014/7/24 17:11, Andrew Cooper wrote:
> On 24/07/14 10:03, Chen, Tiejun wrote:
>> On 2014/7/24 16:57, Andrew Cooper wrote:
>>> On 24/07/14 09:50, Tiejun Chen wrote:
>>>> intel_iommu_map_page() does nothing if VT-d shares EPT page table.
>>>> So rmrr_identity_mapping() never create RMRR mapping but in some
>>>> cases like some GFX drivers it still need to access RMRR.
>>>>
>>>> Here we will create those RMRR mappings even in shared EPT case.
>>>>
>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>>> ---
>>>> xen/drivers/passthrough/vtd/iommu.c | 18 ++++++++++++++++--
>>>> 1 file changed, 16 insertions(+), 2 deletions(-)
>>>>
>>>> v2:
>>>>
>>>> * Fix coding style.
>>>> * Still need to abide intel_iommu_map_page(), so we should do nothing
>>>> if dom0 and iommu supports pass thru.
>>>>
>>>> diff --git a/xen/drivers/passthrough/vtd/iommu.c
>>>> b/xen/drivers/passthrough/vtd/iommu.c
>>>> index 042b882..aca79db 100644
>>>> --- a/xen/drivers/passthrough/vtd/iommu.c
>>>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>>>> @@ -41,6 +41,7 @@
>>>> #include "extern.h"
>>>> #include "vtd.h"
>>>> #include "../ats.h"
>>>> +#include "../../../arch/x86/mm/mm-locks.h"
>>>
>>> <asm/mm/mm-locks.h>
>>
>> iommu.c:38:29: fatal error: asm/mm/mm-locks.h: No such file or directory
>> #include <asm/mm/mm-locks.h>
>> ^
>> compilation terminated.
>>
>> Tiejun
>
> Hmm - my mistake. The lack of easy access to this header file goes to
> emphasise that it is private to arch/x86/mm, and there are indeed no
> current users outside of that part of the tree.
>
> Would it not be better have some public p2m_set_identity() function in
> p2m.c ?
>
Sounds good so what about this?
Subject: [PATCH 1/2] xen:p2m: introduce a public p2m_set_identity
Often p2m_set_entry() always needs to hold p2m_lock but
p2m_lock() is not easy included for some common occasions.
So here introduce an new public p2m_set_identity holding this lock.
Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
xen/arch/x86/mm/p2m.c | 13 +++++++++++++
xen/include/asm-x86/p2m.h | 3 +++
2 files changed, 16 insertions(+)
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 642ec28..d0cc586 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -385,6 +385,19 @@ int p2m_set_entry(struct p2m_domain *p2m, unsigned
long gfn, mfn_t mfn,
return rc;
}
+/* Just hold p2m_lock then public. */
+int p2m_set_identity(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
+ unsigned int page_order, p2m_type_t p2mt,
p2m_access_t p2ma)
+{
+ int rc = 0;
+
+ p2m_lock(p2m);
+ rc = p2m_set_entry(p2m, gfn, mfn, page_order, p2mt, p2ma);
+ p2m_unlock(p2m);
+
+ return rc;
+}
+
struct page_info *p2m_alloc_ptp(struct p2m_domain *p2m, unsigned long
type)
{
struct page_info *pg;
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 0ddbadb..169bcd7 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -625,6 +625,9 @@ void p2m_free_ptp(struct p2m_domain *p2m, struct
page_info *pg);
int p2m_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
unsigned int page_order, p2m_type_t p2mt,
p2m_access_t p2ma);
+int p2m_set_identity(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
+ unsigned int page_order, p2m_type_t p2mt,
p2m_access_t p2ma);
+
/* Set up function pointers for PT implementation: only for use by p2m
code */
extern void p2m_pt_init(struct p2m_domain *p2m);
--
1.9.1
Thanks
Tiejun
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [v2][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT
2014-07-24 9:39 ` Chen, Tiejun
@ 2014-07-24 9:56 ` Tim Deegan
0 siblings, 0 replies; 11+ messages in thread
From: Tim Deegan @ 2014-07-24 9:56 UTC (permalink / raw)
To: Chen, Tiejun; +Cc: yang.z.zhang, Andrew Cooper, kevin.tian, JBeulich, xen-devel
Hi,
At 17:39 +0800 on 24 Jul (1406219964), Chen, Tiejun wrote:
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 642ec28..d0cc586 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -385,6 +385,19 @@ int p2m_set_entry(struct p2m_domain *p2m, unsigned
> long gfn, mfn_t mfn,
> return rc;
> }
>
> +/* Just hold p2m_lock then public. */
I can't understand this comment at all. If you need to add a
new function, it should have a comment describing _what it does_.
> +int p2m_set_identity(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
> + unsigned int page_order, p2m_type_t p2mt,
> p2m_access_t p2ma)
Also it shouldn't just allow the caller to set anything in a p2m
entry; that will cause endless trouble down the line as code outside
x86/mm breaks invariants that the p2m code is maintaining.
AFAICT a function to set an identity map should only need two
arguments, the domain and the mfn; it should then internally DTRT with
the other arguments.
But as mentioned in a few other emails, you might be able to use
set_mmio_p2m_entry() instead of adding new code.
Cheers,
Tim.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v2][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT
2014-07-24 9:11 ` Andrew Cooper
2014-07-24 9:31 ` Jan Beulich
2014-07-24 9:39 ` Chen, Tiejun
@ 2014-07-24 9:47 ` Tim Deegan
2 siblings, 0 replies; 11+ messages in thread
From: Tim Deegan @ 2014-07-24 9:47 UTC (permalink / raw)
To: Andrew Cooper; +Cc: yang.z.zhang, Chen, Tiejun, kevin.tian, JBeulich, xen-devel
At 10:11 +0100 on 24 Jul (1406193065), Andrew Cooper wrote:
> On 24/07/14 10:03, Chen, Tiejun wrote:
> > On 2014/7/24 16:57, Andrew Cooper wrote:
> >> On 24/07/14 09:50, Tiejun Chen wrote:
> >>> intel_iommu_map_page() does nothing if VT-d shares EPT page table.
> >>> So rmrr_identity_mapping() never create RMRR mapping but in some
> >>> cases like some GFX drivers it still need to access RMRR.
> >>>
> >>> Here we will create those RMRR mappings even in shared EPT case.
> >>>
> >>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> >>> ---
> >>> xen/drivers/passthrough/vtd/iommu.c | 18 ++++++++++++++++--
> >>> 1 file changed, 16 insertions(+), 2 deletions(-)
> >>>
> >>> v2:
> >>>
> >>> * Fix coding style.
> >>> * Still need to abide intel_iommu_map_page(), so we should do nothing
> >>> if dom0 and iommu supports pass thru.
> >>>
> >>> diff --git a/xen/drivers/passthrough/vtd/iommu.c
> >>> b/xen/drivers/passthrough/vtd/iommu.c
> >>> index 042b882..aca79db 100644
> >>> --- a/xen/drivers/passthrough/vtd/iommu.c
> >>> +++ b/xen/drivers/passthrough/vtd/iommu.c
> >>> @@ -41,6 +41,7 @@
> >>> #include "extern.h"
> >>> #include "vtd.h"
> >>> #include "../ats.h"
> >>> +#include "../../../arch/x86/mm/mm-locks.h"
> >>
> >> <asm/mm/mm-locks.h>
> >
> > iommu.c:38:29: fatal error: asm/mm/mm-locks.h: No such file or directory
> > #include <asm/mm/mm-locks.h>
> > ^
> > compilation terminated.
> >
> > Tiejun
>
> Hmm - my mistake. The lack of easy access to this header file goes to
> emphasise that it is private to arch/x86/mm, and there are indeed no
> current users outside of that part of the tree.
Indeed -- outside code should not be touching the mm locks, and
furthermore the declaration of p2m_set_entry() has a comment right
beside it that says "only for use by p2m code".
I suppose you can't use the usual add-to-physmap operations
because they would try to call back into iommu code to keep the vtd
tables in sync? In that case you will need some sort of new API to
the p2m code to handle this case, as Andrew suggested. But OTOH
from the commit message it looks like this path only ever needed
when vtd is using the p2m table directly? In which case, maybe
set_mmio_p2m_entry() already does what you need.
I'm a bit surprised that there's no check for an existing mapping
here, btw -- what if the guest already has memory mapped at that
address? That's going to go badly when it tries to use that memory
(either from the CPU or DMA).
Cheers,
Tim.
> ~Andrew
>
> >
> >>
> >> ~Andrew
> >>
> >>>
> >>> struct mapped_rmrr {
> >>> struct list_head list;
> >>> @@ -1842,6 +1843,7 @@ static int rmrr_identity_mapping(struct domain
> >>> *d,
> >>> unsigned long base_pfn, end_pfn;
> >>> struct mapped_rmrr *mrmrr;
> >>> struct hvm_iommu *hd = domain_hvm_iommu(d);
> >>> + struct p2m_domain *p2m = p2m_get_hostp2m(d);
> >>>
> >>> ASSERT(spin_is_locked(&pcidevs_lock));
> >>> ASSERT(rmrr->base_address < rmrr->end_address);
> >>> @@ -1867,8 +1869,20 @@ static int rmrr_identity_mapping(struct
> >>> domain *d,
> >>>
> >>> while ( base_pfn < end_pfn )
> >>> {
> >>> - if ( intel_iommu_map_page(d, base_pfn, base_pfn,
> >>> - IOMMUF_readable|IOMMUF_writable) )
> >>> + if ( iommu_use_hap_pt(d) && (!iommu_passthrough ||
> >>> + !is_hardware_domain(d)) )
> >>> + {
> >>> + p2m_lock(p2m);
> >>> + if ( p2m_set_entry(p2m, base_pfn, _mfn(base_pfn),
> >>> PAGE_ORDER_4K,
> >>> + p2m_mmio_direct, p2m_access_rw) )
> >>> + {
> >>> + p2m_unlock(p2m);
> >>> + return -1;
> >>> + }
> >>> + p2m_unlock(p2m);
> >>> + }
> >>> + else if ( intel_iommu_map_page(d, base_pfn, base_pfn,
> >>> +
> >>> IOMMUF_readable|IOMMUF_writable) )
> >>> return -1;
> >>> base_pfn++;
> >>> }
> >>
> >>
> >>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 11+ messages in thread