Actually, whether the name is "gnttab_shared" or "shared" or "gnttab", the code line still breaks the 80-column rule.- } 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"?
I am not so sure about your meaning, do you mean change gnttab_shared back to shared?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.
Yes, it should be that if there is only v1 function existing./* * 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?
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?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;
Thanks, I'd like to change it.+/* * 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.
+ #else /* __ASSEMBLY__ */ /* In assembly code we cannot use C numeric constant suffixes. */ -- 1.7.6.4