Re: [Xen-devel] [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT
On 17/06/2019 11:48, George Dunlap wrote: > On 6/13/19 11:56 AM, Alexandru Stefan ISAILA wrote: >> >> On 26.09.2018 19:47, George Dunlap wrote: >>> From: Isaila Alexandru >>> >>> This patch adds access control for NPT mode. >>> >>> There aren’t enough extra bits to store the access rights in the NPT p2m >>> table, so we add a radix tree to store extra information. >>> >>> For efficiency: >>> - Only allocate this radix tree when we first store "non-default" >>> extra information >>> >>> - Remove entires which match the default extra information rather >>> than continuing to store them >>> >>> - For superpages, only store an entry for the first gfn in the >>> superpage. Use the order of the p2m entry being read to determine >>> the proper place to look in the radix table. >>> >>> Modify p2m_type_to_flags() to accept and interpret an access value, >>> parallel to the ept code. >>> >>> Add a set_default_access() method to the p2m-pt and p2m-ept versions >>> of the p2m rather than setting it directly, to deal with different >>> default permitted access values. >>> >>> Signed-off-by: Alexandru Isaila >>> Signed-off-by: George Dunlap >>> --- >>> NB, this is compile-tested only. >>> >>> cc'ing Paul because this is functionality he may want at some point in >>> the future. >>> >>> I'm not sure why we only allow 'int' to be stored in the radix tree, >>> but that throws away 30-some bits we could otherwise use. We might >>> consider revising this if we run out of bits here. >>> >>> CC: Andrew Cooper >>> CC: Jan Beulich >>> CC: Tim Deegan >>> CC: Tamas K Lengyel >>> CC: Paul Durrant >>> CC: Razvan Cojocaru >> Hi all, >> >> I know it's been some time from the start of this patch but can this >> move forward? Any thoughts or acks are appreciated. > Right, well where we left this, the situation was that on AMD hardware > with IOMMU / p2m sharing, there aren't enough bits. > > The two general fixes we have so far are: > 1. Add a parallel tree for extra bits (this patch) > 2. Rip out IOMMU / p2m sharing for AMD. > > #2 has the advantage that we don't need an entirely separate tree, as > well as getting rid of code that has (apparently) been completely dead > for 5 years. #1 has the advantage that we're set up for having a much > larger number of IOREQ servers in the future. > > Nobody objected to #2. Without looking deeply into it, it seems like it > might be a good idea, but I can't be sure without seeing what it would > actually look like. > > The easiest way to press the point then would be to post a patch series > ripping it out. IOMMU / p2m sharing on AMD doesn't work, and isn't available any more (despite the code looking suspiciously like it is usable). There was a bugfix to allow DMA mapping of granted pages, which is a prerequisite for PVH support, which requires using a nonzero p2m type, and is therefore incompatible with IOMMU/p2m sharing. I don't see any feasible way to bring p2m-sharing back into a working state on AMD. As a result, we'd be much better ripping out the dead code and more formally acknowledging that it is a dead feature, after which the existing p2m type/access infrastructure should work compatibly with the Intel implementation. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT
On 6/13/19 11:56 AM, Alexandru Stefan ISAILA wrote: > > > On 26.09.2018 19:47, George Dunlap wrote: >> From: Isaila Alexandru >> >> This patch adds access control for NPT mode. >> >> There aren’t enough extra bits to store the access rights in the NPT p2m >> table, so we add a radix tree to store extra information. >> >> For efficiency: >> - Only allocate this radix tree when we first store "non-default" >> extra information >> >> - Remove entires which match the default extra information rather >> than continuing to store them >> >> - For superpages, only store an entry for the first gfn in the >> superpage. Use the order of the p2m entry being read to determine >> the proper place to look in the radix table. >> >> Modify p2m_type_to_flags() to accept and interpret an access value, >> parallel to the ept code. >> >> Add a set_default_access() method to the p2m-pt and p2m-ept versions >> of the p2m rather than setting it directly, to deal with different >> default permitted access values. >> >> Signed-off-by: Alexandru Isaila >> Signed-off-by: George Dunlap >> --- >> NB, this is compile-tested only. >> >> cc'ing Paul because this is functionality he may want at some point in >> the future. >> >> I'm not sure why we only allow 'int' to be stored in the radix tree, >> but that throws away 30-some bits we could otherwise use. We might >> consider revising this if we run out of bits here. >> >> CC: Andrew Cooper >> CC: Jan Beulich >> CC: Tim Deegan >> CC: Tamas K Lengyel >> CC: Paul Durrant >> CC: Razvan Cojocaru > > Hi all, > > I know it's been some time from the start of this patch but can this > move forward? Any thoughts or acks are appreciated. Right, well where we left this, the situation was that on AMD hardware with IOMMU / p2m sharing, there aren't enough bits. The two general fixes we have so far are: 1. Add a parallel tree for extra bits (this patch) 2. Rip out IOMMU / p2m sharing for AMD. #2 has the advantage that we don't need an entirely separate tree, as well as getting rid of code that has (apparently) been completely dead for 5 years. #1 has the advantage that we're set up for having a much larger number of IOREQ servers in the future. Nobody objected to #2. Without looking deeply into it, it seems like it might be a good idea, but I can't be sure without seeing what it would actually look like. The easiest way to press the point then would be to post a patch series ripping it out. -George ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT
On 6/13/19 6:19 PM, Tamas Lengyel wrote: On Wed, Sep 26, 2018 at 10:49 AM George Dunlap wrote: From: Isaila Alexandru This patch adds access control for NPT mode. There aren’t enough extra bits to store the access rights in the NPT p2m table, so we add a radix tree to store extra information. For efficiency: - Only allocate this radix tree when we first store "non-default" extra information - Remove entires which match the default extra information rather than continuing to store them - For superpages, only store an entry for the first gfn in the superpage. Use the order of the p2m entry being read to determine the proper place to look in the radix table. Modify p2m_type_to_flags() to accept and interpret an access value, parallel to the ept code. Add a set_default_access() method to the p2m-pt and p2m-ept versions of the p2m rather than setting it directly, to deal with different default permitted access values. Signed-off-by: Alexandru Isaila Signed-off-by: George Dunlap The mem_access/monitor bits are fairly trivial: Acked-by: Tamas K Lengyel --- NB, this is compile-tested only. Are you planning to do some actual testing? I would highly recommend that we see real test results before this is merged to verify functionality. We did do some testing with xen-access at the time, but limited testing with the actual full-blown introspection agent (because not all the needed pieces align yet). Things did appear to work as intended. Thanks, Razvan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT
On Wed, Sep 26, 2018 at 10:49 AM George Dunlap wrote: > > From: Isaila Alexandru > > This patch adds access control for NPT mode. > > There aren’t enough extra bits to store the access rights in the NPT p2m > table, so we add a radix tree to store extra information. > > For efficiency: > - Only allocate this radix tree when we first store "non-default" >extra information > > - Remove entires which match the default extra information rather >than continuing to store them > > - For superpages, only store an entry for the first gfn in the >superpage. Use the order of the p2m entry being read to determine >the proper place to look in the radix table. > > Modify p2m_type_to_flags() to accept and interpret an access value, > parallel to the ept code. > > Add a set_default_access() method to the p2m-pt and p2m-ept versions > of the p2m rather than setting it directly, to deal with different > default permitted access values. > > Signed-off-by: Alexandru Isaila > Signed-off-by: George Dunlap The mem_access/monitor bits are fairly trivial: Acked-by: Tamas K Lengyel > --- > NB, this is compile-tested only. Are you planning to do some actual testing? I would highly recommend that we see real test results before this is merged to verify functionality. Thanks, Tamas ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT
On 26.09.2018 19:47, George Dunlap wrote: > From: Isaila Alexandru > > This patch adds access control for NPT mode. > > There aren’t enough extra bits to store the access rights in the NPT p2m > table, so we add a radix tree to store extra information. > > For efficiency: > - Only allocate this radix tree when we first store "non-default" > extra information > > - Remove entires which match the default extra information rather > than continuing to store them > > - For superpages, only store an entry for the first gfn in the > superpage. Use the order of the p2m entry being read to determine > the proper place to look in the radix table. > > Modify p2m_type_to_flags() to accept and interpret an access value, > parallel to the ept code. > > Add a set_default_access() method to the p2m-pt and p2m-ept versions > of the p2m rather than setting it directly, to deal with different > default permitted access values. > > Signed-off-by: Alexandru Isaila > Signed-off-by: George Dunlap > --- > NB, this is compile-tested only. > > cc'ing Paul because this is functionality he may want at some point in > the future. > > I'm not sure why we only allow 'int' to be stored in the radix tree, > but that throws away 30-some bits we could otherwise use. We might > consider revising this if we run out of bits here. > > CC: Andrew Cooper > CC: Jan Beulich > CC: Tim Deegan > CC: Tamas K Lengyel > CC: Paul Durrant > CC: Razvan Cojocaru Hi all, I know it's been some time from the start of this patch but can this move forward? Any thoughts or acks are appreciated. Thanks, Alex ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT
Ping Suravee / Brian / Boris any ideas on this topic are appreciated. Regards, Alex On 27.09.2018 13:37, George Dunlap wrote: > On 09/26/2018 06:22 PM, Andrew Cooper wrote: >> On 26/09/18 17:47, George Dunlap wrote: >>> From: Isaila Alexandru >>> >>> This patch adds access control for NPT mode. >>> >>> There aren’t enough extra bits to store the access rights in the NPT p2m >>> table, so we add a radix tree to store extra information. >> >> I'm sorry to re-open this argument, but why? >> >> ISTR there being some argument based on pagetable sharing with the >> IOMMU, but that doesn't work at the moment and can't reasonably be made >> to work. For one, attempting to use pt sharing will break as soon as >> you try and DMA to a mapped grant. >> >> I'm disinclined to let a broken vestigial feature get in the way of real >> improvements. >> >> Beyond that, an NPT PTE has basically the same number of software >> available bits as an EPT PTE. >> >> Am I missing anything? > > Wow -- looks like IOMMU/p2m sharing has been disabled unconditionally > since 2014. If nobody has complained since then, that seems like a good > enough reason to me to rip it out. > > Suravee / Brian / Boris -- any opinions? > > The main reason to go with the 'extra bits' solution rather than the > 'rip out iommu/p2m sharing' solution is because people have been > prognosticating for years that we would be running out of bits and need > more at some point in the future. I thought Paul, for instance, might > have a use for the extra bits. But I'm happy to wait until such time as > we need it and then fish this patch out of the mail archives. > > -George > ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT
> -Original Message- > From: George Dunlap [mailto:george.dun...@citrix.com] > Sent: 27 September 2018 11:38 > To: Andrew Cooper ; xen- > de...@lists.xenproject.org > Cc: Isaila Alexandru ; Jan Beulich > ; Tim (Xen.org) ; Tamas K Lengyel > ; Paul Durrant ; > Razvan Cojocaru ; Suravee Suthikulpanit > ; Brian Woods ; Boris > Ostrovsky > Subject: Re: [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT > > On 09/26/2018 06:22 PM, Andrew Cooper wrote: > > On 26/09/18 17:47, George Dunlap wrote: > >> From: Isaila Alexandru > >> > >> This patch adds access control for NPT mode. > >> > >> There aren’t enough extra bits to store the access rights in the NPT > p2m > >> table, so we add a radix tree to store extra information. > > > > I'm sorry to re-open this argument, but why? > > > > ISTR there being some argument based on pagetable sharing with the > > IOMMU, but that doesn't work at the moment and can't reasonably be made > > to work. For one, attempting to use pt sharing will break as soon as > > you try and DMA to a mapped grant. > > > > I'm disinclined to let a broken vestigial feature get in the way of real > > improvements. > > > > Beyond that, an NPT PTE has basically the same number of software > > available bits as an EPT PTE. > > > > Am I missing anything? > > Wow -- looks like IOMMU/p2m sharing has been disabled unconditionally > since 2014. If nobody has complained since then, that seems like a good > enough reason to me to rip it out. > > Suravee / Brian / Boris -- any opinions? > > The main reason to go with the 'extra bits' solution rather than the > 'rip out iommu/p2m sharing' solution is because people have been > prognosticating for years that we would be running out of bits and need > more at some point in the future. I thought Paul, for instance, might > have a use for the extra bits. But I'm happy to wait until such time as > we need it and then fish this patch out of the mail archives. > The main angle I had was to have a more generic page-to-type mapping such that it would be suitable to allow steering of accesses to certain pages to distinct IOREQ servers. Paul > -George ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT
On 09/26/2018 06:22 PM, Andrew Cooper wrote: > On 26/09/18 17:47, George Dunlap wrote: >> From: Isaila Alexandru >> >> This patch adds access control for NPT mode. >> >> There aren’t enough extra bits to store the access rights in the NPT p2m >> table, so we add a radix tree to store extra information. > > I'm sorry to re-open this argument, but why? > > ISTR there being some argument based on pagetable sharing with the > IOMMU, but that doesn't work at the moment and can't reasonably be made > to work. For one, attempting to use pt sharing will break as soon as > you try and DMA to a mapped grant. > > I'm disinclined to let a broken vestigial feature get in the way of real > improvements. > > Beyond that, an NPT PTE has basically the same number of software > available bits as an EPT PTE. > > Am I missing anything? Wow -- looks like IOMMU/p2m sharing has been disabled unconditionally since 2014. If nobody has complained since then, that seems like a good enough reason to me to rip it out. Suravee / Brian / Boris -- any opinions? The main reason to go with the 'extra bits' solution rather than the 'rip out iommu/p2m sharing' solution is because people have been prognosticating for years that we would be running out of bits and need more at some point in the future. I thought Paul, for instance, might have a use for the extra bits. But I'm happy to wait until such time as we need it and then fish this patch out of the mail archives. -George ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT
On Wed, 2018-09-26 at 17:47 +0100, George Dunlap wrote: > From: Isaila Alexandru > > This patch adds access control for NPT mode. > > There aren’t enough extra bits to store the access rights in the NPT > p2m > table, so we add a radix tree to store extra information. > > For efficiency: > - Only allocate this radix tree when we first store "non-default" >extra information > > - Remove entires which match the default extra information rather >than continuing to store them > > - For superpages, only store an entry for the first gfn in the >superpage. Use the order of the p2m entry being read to determine >the proper place to look in the radix table. > > Modify p2m_type_to_flags() to accept and interpret an access value, > parallel to the ept code. > > Add a set_default_access() method to the p2m-pt and p2m-ept versions > of the p2m rather than setting it directly, to deal with different > default permitted access values. > > Signed-off-by: Alexandru Isaila > Signed-off-by: George Dunlap > --- > NB, this is compile-tested only. I've tested this with xen-access and it works as expected > > diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c > index 3c42e21906..2e6b1e75e4 100644 > --- a/xen/arch/x86/monitor.c > +++ b/xen/arch/x86/monitor.c > @@ -20,6 +20,7 @@ > */ > > #include > +#include Is this intended? Regards, Alex ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT
On 26/09/18 17:47, George Dunlap wrote: > From: Isaila Alexandru > > This patch adds access control for NPT mode. > > There aren’t enough extra bits to store the access rights in the NPT p2m > table, so we add a radix tree to store extra information. I'm sorry to re-open this argument, but why? ISTR there being some argument based on pagetable sharing with the IOMMU, but that doesn't work at the moment and can't reasonably be made to work. For one, attempting to use pt sharing will break as soon as you try and DMA to a mapped grant. I'm disinclined to let a broken vestigial feature get in the way of real improvements. Beyond that, an NPT PTE has basically the same number of software available bits as an EPT PTE. Am I missing anything? > > For efficiency: > - Only allocate this radix tree when we first store "non-default" >extra information > > - Remove entires which match the default extra information rather >than continuing to store them > > - For superpages, only store an entry for the first gfn in the >superpage. Use the order of the p2m entry being read to determine >the proper place to look in the radix table. > > Modify p2m_type_to_flags() to accept and interpret an access value, > parallel to the ept code. > > Add a set_default_access() method to the p2m-pt and p2m-ept versions > of the p2m rather than setting it directly, to deal with different > default permitted access values. > > Signed-off-by: Alexandru Isaila > Signed-off-by: George Dunlap > --- > NB, this is compile-tested only. > > cc'ing Paul because this is functionality he may want at some point in > the future. > > I'm not sure why we only allow 'int' to be stored in the radix tree, > but that throws away 30-some bits we could otherwise use. We might > consider revising this if we run out of bits here. Probably because we have a old fork of the Linux radix tree functionality, rather than any specific reason to waste 30-some bits. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT
From: Isaila Alexandru
This patch adds access control for NPT mode.
There aren’t enough extra bits to store the access rights in the NPT p2m
table, so we add a radix tree to store extra information.
For efficiency:
- Only allocate this radix tree when we first store "non-default"
extra information
- Remove entires which match the default extra information rather
than continuing to store them
- For superpages, only store an entry for the first gfn in the
superpage. Use the order of the p2m entry being read to determine
the proper place to look in the radix table.
Modify p2m_type_to_flags() to accept and interpret an access value,
parallel to the ept code.
Add a set_default_access() method to the p2m-pt and p2m-ept versions
of the p2m rather than setting it directly, to deal with different
default permitted access values.
Signed-off-by: Alexandru Isaila
Signed-off-by: George Dunlap
---
NB, this is compile-tested only.
cc'ing Paul because this is functionality he may want at some point in
the future.
I'm not sure why we only allow 'int' to be stored in the radix tree,
but that throws away 30-some bits we could otherwise use. We might
consider revising this if we run out of bits here.
CC: Andrew Cooper
CC: Jan Beulich
CC: Tim Deegan
CC: Tamas K Lengyel
CC: Paul Durrant
CC: Razvan Cojocaru
---
xen/arch/x86/mm/mem_access.c | 5 +-
xen/arch/x86/mm/p2m-ept.c| 9 ++
xen/arch/x86/mm/p2m-pt.c | 173 ---
xen/arch/x86/mm/p2m.c| 6 ++
xen/arch/x86/monitor.c | 1 +
xen/include/asm-x86/mem_access.h | 2 +-
xen/include/asm-x86/p2m.h| 18
7 files changed, 192 insertions(+), 22 deletions(-)
diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index 699a315076..0f96c43f88 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -389,10 +389,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn,
uint32_t nr,
/* If request to set default access. */
if ( gfn_eq(gfn, INVALID_GFN) )
-{
-p2m->default_access = a;
-return 0;
-}
+return p2m->set_default_access(p2m, a);
p2m_lock(p2m);
if ( ap2m )
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 1ff4f14ae4..37bdbcf186 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -667,6 +667,14 @@ bool_t ept_handle_misconfig(uint64_t gpa)
return spurious ? (rc >= 0) : (rc > 0);
}
+int ept_set_default_access(struct p2m_domain *p2m, p2m_access_t p2ma)
+{
+/* All access is permitted */
+p2m->default_access = p2ma;
+
+return 0;
+}
+
/*
* ept_set_entry() computes 'need_modify_vtd_table' for itself,
* by observing whether any gfn->mfn translations are modified.
@@ -1255,6 +1263,7 @@ int ept_p2m_init(struct p2m_domain *p2m)
p2m->change_entry_type_global = ept_change_entry_type_global;
p2m->change_entry_type_range = ept_change_entry_type_range;
p2m->memory_type_changed = ept_memory_type_changed;
+p2m->set_default_access = ept_set_default_access;
p2m->audit_p2m = NULL;
p2m->tlb_flush = ept_tlb_flush;
diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index 40bfc76a6f..cd8b8c9187 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -68,7 +68,8 @@
static unsigned long p2m_type_to_flags(const struct p2m_domain *p2m,
p2m_type_t t,
mfn_t mfn,
- unsigned int level)
+ unsigned int level,
+ p2m_access_t access)
{
unsigned long flags;
/*
@@ -87,23 +88,28 @@ static unsigned long p2m_type_to_flags(const struct
p2m_domain *p2m,
case p2m_ram_paged:
case p2m_ram_paging_in:
default:
-return flags | _PAGE_NX_BIT;
+flags |= _PAGE_NX_BIT;
+break;
case p2m_grant_map_ro:
-return flags | P2M_BASE_FLAGS | _PAGE_NX_BIT;
+flags |= P2M_BASE_FLAGS | _PAGE_NX_BIT;
+break;
case p2m_ioreq_server:
flags |= P2M_BASE_FLAGS | _PAGE_RW | _PAGE_NX_BIT;
if ( p2m->ioreq.flags & XEN_DMOP_IOREQ_MEM_ACCESS_WRITE )
-return flags & ~_PAGE_RW;
-return flags;
+flags &= ~_PAGE_RW;
+break;
case p2m_ram_ro:
case p2m_ram_logdirty:
case p2m_ram_shared:
-return flags | P2M_BASE_FLAGS;
+flags |= P2M_BASE_FLAGS;
+break;
case p2m_ram_rw:
-return flags | P2M_BASE_FLAGS | _PAGE_RW;
+flags |= P2M_BASE_FLAGS | _PAGE_RW;
+break;
case p2m_grant_map_rw:
case p2m_map_foreign:
-return flags | P2M_BASE_FLAGS | _PAGE_RW | _PAGE_NX_BIT;
+flags |= P2M_BASE_FLAGS | _PAGE_RW | _PAGE_NX_BIT;
+break;
case p2m_mmio_direct:
if (
