Thanks for your review, Ian.
See following,
- } while ((nflags = sync_cmpxchg(&shared[ref].flags, flags, 0)) != flags);
+ } while ((nflags = sync_cmpxchg(&gnttab_shared.v1[ref].flags, flags, 0))
+ != flags);
I think this is one of those cases where strictly adhering to an
80-column rule hurts the readability of the code.
If you had left the static global as "shared" rather than
"gnttab_shared" you wouldn't have this issue. If you want a more
descriptive name why not just call it "gnttab"?
Actually, whether the name is "gnttab_shared" or "shared" or "gnttab",
the code line still breaks the 80-column rule.
return 1;
}
+
+int gnttab_end_foreign_access_ref(grant_ref_t ref, int readonly)
+{
+ return gnttab_interface.end_foreign_access_ref(ref, readonly);
+}
EXPORT_SYMBOL_GPL(gnttab_end_foreign_access_ref);
void gnttab_end_foreign_access(grant_ref_t ref, int readonly,
@@ -246,37 +309,45 @@ EXPORT_SYMBOL_GPL(gnttab_grant_foreign_transfer);
void gnttab_grant_foreign_transfer_ref(grant_ref_t ref, domid_t domid,
unsigned long pfn)
{
- update_grant_entry(ref, domid, pfn, GTF_accept_transfer);
+ gnttab_interface.update_entry(ref, domid, pfn, GTF_accept_transfer);
}
EXPORT_SYMBOL_GPL(gnttab_grant_foreign_transfer_ref);
-unsigned long gnttab_end_foreign_transfer_ref(grant_ref_t ref)
+static unsigned long gnttab_end_foreign_transfer_ref_v1(grant_ref_t ref)
{
unsigned long frame;
u16 flags;
+ u16 *pflags;
+
+ pflags = &gnttab_shared.v1[ref].flags;
It would be nice if these refactoring bits could be separated out from
the more mechanical renaming and abstracting to fn pointer aspects of
the patch.
I am not so sure about your meaning, do you mean change gnttab_shared
back to shared?
/*
* If a transfer is not even yet started, try to reclaim the grant
* reference and return failure (== 0).
*/
- while (!((flags = shared[ref].flags) & GTF_transfer_committed)) {
- if (sync_cmpxchg(&shared[ref].flags, flags, 0) == flags)
+ while (!((flags = *pflags) & GTF_transfer_committed)) {
+ if (sync_cmpxchg(pflags, flags, 0) == flags)
return 0;
cpu_relax();
}
/* If a transfer is in progress then wait until it is completed. */
while (!(flags & GTF_transfer_completed)) {
- flags = shared[ref].flags;
+ flags = *pflags;
cpu_relax();
}
rmb(); /* Read the frame number /after/ reading completion status. */
- frame = shared[ref].frame;
+ frame = gnttab_shared.v1[ref].frame;
BUG_ON(frame == 0);
return frame;
}
+
+unsigned long gnttab_end_foreign_transfer_ref(grant_ref_t ref)
+{
+ return gnttab_interface.end_foreign_transfer_ref(ref);
+}
EXPORT_SYMBOL_GPL(gnttab_end_foreign_transfer_ref);
unsigned long gnttab_end_foreign_transfer(grant_ref_t ref)
@@ -520,6 +591,23 @@ int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
}
EXPORT_SYMBOL_GPL(gnttab_unmap_refs);
+static int gnttab_map_frames_v1(unsigned long *frames, unsigned int nr_gframes)
+{
+ int rc;
+
+ rc = arch_gnttab_map_shared(frames, nr_gframes,
+ gnttab_max_grant_frames(),
+ &gnttab_shared.addr);
+ BUG_ON(rc);
+
+ return 0;
+}
+
+static void gnttab_unmap_frames_v1(void)
+{
+ arch_gnttab_unmap_shared(gnttab_shared.addr, nr_grant_frames);
+}
+
static int gnttab_map(unsigned int start_idx, unsigned int end_idx)
{
struct gnttab_setup_table setup;
@@ -567,19 +655,33 @@ static int gnttab_map(unsigned int start_idx, unsigned int end_idx)
BUG_ON(rc || setup.status);
- rc = arch_gnttab_map_shared(frames, nr_gframes, gnttab_max_grant_frames(),
- &shared);
- BUG_ON(rc);
+ rc = gnttab_interface.map_frames(frames, nr_gframes);
Nothing checks rc here now?
In fact the gnttab_map_frames_v1 function has its own BUG_ON(rc) and
always returns 0 if it returns at all so perhaps that hook should be
returning void?
Yes, it should be that if there is only v1 function existing.
However, I added returns 0 here in order to keep consistence with v2
function of next patch. The function pointer type is: int
(*map_frames)(....), and v2 function returning value is meaningful. The
returning value directly decides returning value of gnttab_map. See
following code in function gnttab_map of v2 patch:
ÂÂÂÂÂÂÂ if (rc < 0)
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return rc;
ÂÂÂÂÂÂÂ return 0;
If gnttab_map_frames_v1
returns void here, it is necessary to change it back(including "void
(*map_frames)"Â --> "int (*map_frames)") in next v2 implementation
patch. So I only added return 0 here.
kfree(frames);
return 0;
}
+static void gnttab_request_version(void)
+{
+ grant_table_version = 1;
+ gnttab_interface.map_frames = gnttab_map_frames_v1;
+ gnttab_interface.unmap_frames = gnttab_unmap_frames_v1;
+ gnttab_interface.update_entry = update_grant_entry_v1;
+ gnttab_interface.end_foreign_access_ref =
+ gnttab_end_foreign_access_ref_v1;
+ gnttab_interface.end_foreign_transfer_ref =
+ gnttab_end_foreign_transfer_ref_v1;
+ gnttab_interface.query_foreign_access = gnttab_query_foreign_access_v1;
The more normal way to do this would be to make gnttab_interface a
pointer, define gnttab_v1_ops and do:
gnttab_interface = &gnttab_v1_ops;
or if the pointer overhead is significant remove that and just do a
struct assignment:
gnttab_interface = gnttab_v1_ops;
If using this way, we need two more public structures(gnttab_v1_ops and
gnttab_v2_ops), and two more functions to initialize those two
structures and then initialize the pointer gnttab_interface. It is more
complicated, am i missing something?
.....
+/*
* Bitfield values for update_pin_status.flags.
*/
/* Map the grant entry for access by I/O devices. */
diff --git a/include/xen/interface/xen.h b/include/xen/interface/xen.h
index 6a6e914..710afe0 100644
--- a/include/xen/interface/xen.h
+++ b/include/xen/interface/xen.h
@@ -523,6 +523,8 @@ struct tmem_op {
} u;
};
+DEFINE_GUEST_HANDLE(uint64_t);
The kernel uses uN style types rather than the uintN_t style ones,
although include/xen/interface/grant_table.h seems not to adhere to that
at the moment. It might be worth cleaning that up as you go passed.
Thanks, I'd like to change it.
Thanks
Annie
+
#else /* __ASSEMBLY__ */
/* In assembly code we cannot use C numeric constant suffixes. */
--
1.7.6.4
|
