[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH 08/15] xen/x86: p2m: Use typesafe gfn for the P2M callbacks get_entry and set_entry
Hi,
On 13/09/2017 20:08, Razvan Cojocaru wrote:
@@ -142,7 +142,7 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
vm_event_request_t **req_ptr)
{
struct vcpu *v = current;
- unsigned long gfn = gpa >> PAGE_SHIFT;
+ gfn_t gfn = gaddr_to_gfn(gpa);
struct domain *d = v->domain;
struct p2m_domain *p2m = NULL;
mfn_t mfn;
@@ -215,7 +215,7 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
*req_ptr = req;
req->reason = VM_EVENT_REASON_MEM_ACCESS;
- req->u.mem_access.gfn = gfn;
+ req->u.mem_access.gfn = gfn_x(gfn);
req->u.mem_access.offset = gpa & ((1 << PAGE_SHIFT) - 1);
if ( npfec.gla_valid )
{
@@ -247,7 +247,7 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct
p2m_domain *hp2m,
unsigned long gfn_l = gfn_x(gfn);
I'd drop gfn_l completely here and just use gfn_x(gfn) in the two places
below where it's used. It would get rid of a line of code in this
function. But I wouldn't hold the patch over this, so if nobody else has
comments it's fine as is.
Technically more clean-up could be done in every functions. If you
noticed it, I kept the changes minimal because this is already a boring
but necessary job and I really don't want to explore all the possible
clean-up in each single function... Feel free to do more clean-up.
int rc;
- mfn = ap2m->get_entry(ap2m, gfn_l, &t, &old_a, 0, NULL, NULL);
+ mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
/* Check host p2m if no valid entry in alternate */
if ( !mfn_valid(mfn) )
@@ -264,16 +264,16 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct
p2m_domain *hp2m,
if ( page_order != PAGE_ORDER_4K )
{
unsigned long mask = ~((1UL << page_order) - 1);
- unsigned long gfn2_l = gfn_l & mask;
+ gfn_t gfn2 = _gfn(gfn_l & mask);
mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
- rc = ap2m->set_entry(ap2m, gfn2_l, mfn2, page_order, t, old_a, 1);
+ rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
if ( rc )
return rc;
}
}
- return ap2m->set_entry(ap2m, gfn_l, mfn, PAGE_ORDER_4K, t, a,
+ return ap2m->set_entry(ap2m, gfn, mfn, PAGE_ORDER_4K, t, a,
(current->domain != d));
If you need to send V2, could you please also indent the last line so
that '(' is under the 'a' in 'ap2m'? You don't have to, but it'd be a
coding style fix coming in on top of this patch.
Will do.
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 23c0518733..dff214cf7b 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -674,11 +674,12 @@ bool_t ept_handle_misconfig(uint64_t gpa)
* Returns: 0 for success, -errno for failure
*/
static int
-ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
+ept_set_entry(struct p2m_domain *p2m, gfn_t gfn_t, mfn_t mfn,
unsigned int order, p2m_type_t p2mt, p2m_access_t p2ma,
int sve)
{
ept_entry_t *table, *ept_entry = NULL;
+ unsigned long gfn = gfn_x(gfn_t);
Should we call this gfn_l, as done above?
unsigned long gfn_remainder = gfn;
unsigned int i, target = order / EPT_TABLE_ORDER;
unsigned long fn_mask = !mfn_eq(mfn, INVALID_MFN) ? (gfn | mfn_x(mfn)) :
gfn;
@@ -910,11 +911,12 @@ out:
/* Read ept p2m entries */
static mfn_t ept_get_entry(struct p2m_domain *p2m,
- unsigned long gfn, p2m_type_t *t, p2m_access_t* a,
+ gfn_t gfn_t, p2m_type_t *t, p2m_access_t* a,
p2m_query_t q, unsigned int *page_order,
bool_t *sve)
{
ept_entry_t *table =
map_domain_page(_mfn(pagetable_get_pfn(p2m_get_pagetable(p2m))));
+ unsigned long gfn = gfn_x(gfn_t);
gfn_l here too?
[...]
diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index 0e63d6ed11..57878b1886 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -479,12 +479,13 @@ int p2m_pt_handle_deferred_changes(uint64_t gpa)
/* Returns: 0 for success, -errno for failure */
static int
-p2m_pt_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
+p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_t, mfn_t mfn,
unsigned int page_order, p2m_type_t p2mt, p2m_access_t p2ma,
int sve)
{
/* XXX -- this might be able to be faster iff current->domain == d */
void *table;
+ unsigned long gfn = gfn_x(gfn_t);
gfn_l?
unsigned long i, gfn_remainder = gfn;
l1_pgentry_t *p2m_entry, entry_content;
/* Intermediate table to free if we're replacing it with a superpage. */
@@ -731,11 +732,12 @@ p2m_pt_set_entry(struct p2m_domain *p2m, unsigned long
gfn, mfn_t mfn,
}
static mfn_t
-p2m_pt_get_entry(struct p2m_domain *p2m, unsigned long gfn,
+p2m_pt_get_entry(struct p2m_domain *p2m, gfn_t gfn_t,
p2m_type_t *t, p2m_access_t *a, p2m_query_t q,
unsigned int *page_order, bool_t *sve)
{
mfn_t mfn;
+ unsigned long gfn = gfn_x(gfn_t);
gfn_l?
If you do that you would have to rename all gfn to gfn_l in the function
increase a bit more the size of this boring patch. The right solution
would be to make the rest of the code typesafe, but that's not my plan.
I have done enough typesafe clean-up myself, so I will leave it with
unsigned long gfn = gfn_x(gfn_);
Until someone step up and decide to do further clean-up.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
