[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [v7][RFC][PATCH 08/13] xen/x86/p2m: set p2m_access_n for reserved device memory mapping
On 2014/10/28 18:12, Jan Beulich wrote:
On 28.10.14 at 09:26, <tiejun.chen@xxxxxxxxx> wrote:
On 2014/10/27 18:33, Jan Beulich wrote:
On 27.10.14 at 10:05, <tiejun.chen@xxxxxxxxx> wrote:
On 2014/10/24 23:11, Jan Beulich wrote:
On 24.10.14 at 09:34, <tiejun.chen@xxxxxxxxx> wrote:
+ {
+ /*
+ * Just set p2m_access_n in case of shared-ept
+ * or non-shared ept but 1:1 mapping.
+ */
+ if ( iommu_use_hap_pt(d) ||
+ (!iommu_use_hap_pt(d) && mfn == gfn) )
How would, other than by chance, mfn equal gfn here? Also the
double use of iommu_use_hap_pt(d) is pointless here.
There are two scenarios we should concern:
#1 in case of shared-ept.
We always need to check so iommu_use_hap_pt(d) is good.
#2 in case of non-sharepd-ept
If mfn != gfn I think guest don't access RMRR range, so its allowed.
And what if subsequently a device needing a 1:1 mapping at this GFN
gets assigned? (I simply don't see why shared vs non-shared would
matter here.)
In case of non-shared ept we just create VT-d table, if we assign a
device with 1:1 RMRR mapping. So as long as mfn != gfn, its not
necessary to set p2m_access_n.
I think it was mentioned before (and not just by me) that it's at least
risky to allow the two page tables to get out of sync wrt the
translations they do (as opposed to permissions they set).
Okay I will remove all condition check here.
+ {
+ rc = p2m_set_entry(p2m, gfn, _mfn(mfn), page_order, t,
+ p2m_access_n);
+ if ( rc )
+ gdprintk(XENLOG_WARNING, "set rdm p2m failed:
(%#lx)\n",
+ gfn);
Such messages are (due to acting on a foreign domain) relatively
useless without also logging the domain that is affected. Conversely,
logging the current domain and vCPU (due to using gdprintk()) is
rather pointless. Also please drop either the colon or the
parentheses in the message.
Can P2M_DEBUG work here?
P2M_DEBUG("set rdm p2m failed: %#lx\n", gfn);
I don't think this would magically add the missing information. Plus it
Sorry, is it okay?
gdprintk(XENLOG_WARNING, "Domain %hu set rdm p2m failed: %#lx\n",
d->domain_id, gfn);
Almost. Use printk() instead of gdprintk(), and %d instead of %hu.
would limit output to the !NDEBUG case, putting the practical
usefulness of this under question even more.
But anyway, looking at the existing code again, I think you'd be better
off falling through to the p2m_set_entry() that's already there, just
Are you saying I should do this in p2m_set_entry()?
No. I said that I'd prefer you to use the _existing call to_
p2m_set_entry().
altering the access permission value you pass. Less code, better
But here, guest_physmap_add_entry() just initiate to set p2m_access_n.
We will alter the access permission until if we really assign device to
create a 1:1 mapping.
And I didn't question that. All I said is to use the existing call by simply
replacing the unconditional use of the default access value with one
conditionally set to p2m_access_n.
Okay,
@@ -686,8 +686,19 @@ guest_physmap_add_entry(struct domain *d, unsigned
long gfn,
/* Now, actually do the two-way mapping */
if ( mfn_valid(_mfn(mfn)) )
{
- rc = p2m_set_entry(p2m, gfn, _mfn(mfn), page_order, t,
- p2m->default_access);
+ rc = 0;
+ if ( !is_hardware_domain(d) )
+ {
+ rc =
iommu_get_reserved_device_memory(p2m_check_reserved_device_memory,
+ &gfn);
+ if ( rc < 0 )
+ printk("Domain %d can't can't check reserved device
memory.\n",
+ d->domain_id);
+ }
+
+ /* We need to set reserved device memory as p2m_access_n. */
+ a = ( rc == 1 ) ? p2m_access_n : p2m->default_access;
+ rc = p2m_set_entry(p2m, gfn, _mfn(mfn), page_order, t, a);
if ( rc )
goto out; /* Failed to update p2m, bail without updating
m2p. */
Thanks
Tiejun
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
