annie li
2011-Nov-16 12:55 UTC
[Xen-devel] Improved patches of upstreaming grant table version 2
annie li
2011-Nov-16 13:10 UTC
[Xen-devel] Improved patches of upstreaming grant table version 2
annie li
2011-Nov-16 13:23 UTC
Re: [Xen-devel] Improved patches of upstreaming grant table version 2
Diff:
arch/x86/xen/grant-table.c | 42 ++++-
drivers/xen/grant-table.c | 355
++++++++++++++++++++++++++++++----
include/xen/grant_table.h | 10 +-
include/xen/interface/grant_table.h | 167 ++++++++++++++++-
include/xen/interface/xen.h | 2 +
5 files changed, 521 insertions(+), 55 deletions(-)
Short log:
Annie Li (3):
xen/granttable: Introducing grant table V2 stucture
xen/granttable: Grant tables V2 implementation
xen/granttable: Keep code format clean
Thanks
Annie
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
annie.li@oracle.com
2011-Nov-16 13:48 UTC
[Xen-devel] [PATCH 1/3] xen/granttable: Introducing grant table V2 stucture
From: Annie Li <annie.li@oracle.com>
This patch introduces new structures of grant table V2, grant table V2 is an
extension from V1. Grant table is shared between guest and Xen, and Xen is
responsible to do corresponding work for grant operations, such as: figure
out guest''s grant table version, perform different actions based on
different grant table version, etc. Although full-page structure of V2
is different from V1, it play the same role as V1.
Signed-off-by: Annie Li <annie.li@oracle.com>
---
arch/x86/xen/grant-table.c | 7 +-
drivers/xen/grant-table.c | 181 +++++++++++++++++++++++++++--------
include/xen/grant_table.h | 4 +-
include/xen/interface/grant_table.h | 167 +++++++++++++++++++++++++++++++-
include/xen/interface/xen.h | 2 +
5 files changed, 311 insertions(+), 50 deletions(-)
diff --git a/arch/x86/xen/grant-table.c b/arch/x86/xen/grant-table.c
index 6bbfd7a..c6ab2e7 100644
--- a/arch/x86/xen/grant-table.c
+++ b/arch/x86/xen/grant-table.c
@@ -64,10 +64,10 @@ static int unmap_pte_fn(pte_t *pte, struct page *pmd_page,
int arch_gnttab_map_shared(unsigned long *frames, unsigned long nr_gframes,
unsigned long max_nr_gframes,
- struct grant_entry **__shared)
+ void **__shared)
{
int rc;
- struct grant_entry *shared = *__shared;
+ void *shared = *__shared;
if (shared == NULL) {
struct vm_struct *area @@ -83,8 +83,7 @@ int arch_gnttab_map_shared(unsigned
long *frames, unsigned long nr_gframes,
return rc;
}
-void arch_gnttab_unmap_shared(struct grant_entry *shared,
- unsigned long nr_gframes)
+void arch_gnttab_unmap_shared(void *shared, unsigned long nr_gframes)
{
apply_to_page_range(&init_mm, (unsigned long)shared,
PAGE_SIZE * nr_gframes, unmap_pte_fn, NULL);
diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index bf1c094..1bd9d5f 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -53,7 +53,7 @@
/* External tools reserve first few grant table entries. */
#define NR_RESERVED_ENTRIES 8
#define GNTTAB_LIST_END 0xffffffff
-#define GREFS_PER_GRANT_FRAME (PAGE_SIZE / sizeof(struct grant_entry))
+#define GREFS_PER_GRANT_FRAME (PAGE_SIZE / sizeof(struct grant_entry_v1))
static grant_ref_t **gnttab_list;
static unsigned int nr_grant_frames;
@@ -64,7 +64,61 @@ static DEFINE_SPINLOCK(gnttab_list_lock);
unsigned long xen_hvm_resume_frames;
EXPORT_SYMBOL_GPL(xen_hvm_resume_frames);
-static struct grant_entry *shared;
+static union {
+ struct grant_entry_v1 *v1;
+ void *addr;
+} gnttab_shared;
+
+/*This is a structure of function pointers for grant table*/
+static struct {
+ /*
+ * Mapping a list of frames for storing grant entries. First input
+ * parameter is used to storing grant table address when grant table
+ * being setup, second parameter is the number of frames to map grant
+ * table. Returning GNTST_okay means success and negative value means
+ * failure.
+ */
+ int (*map_frames)(unsigned long *, unsigned int);
+ /*
+ * Release a list of frames which are mapped in map_frames for grant
+ * entry status.
+ */
+ void (*unmap_frames)(void);
+ /*
+ * Introducing a valid entry into the grant table, granting the frame
+ * of this grant entry to domain for accessing, or transfering, or
+ * transitively accessing. First input parameter is reference of this
+ * introduced grant entry, second one is domid of granted domain, third
+ * one is the frame to be granted, and the last one is status of the
+ * grant entry to be updated.
+ */
+ void (*update_entry)(grant_ref_t, domid_t, unsigned long, unsigned);
+ /*
+ * Stop granting a grant entry to domain for accessing. First input
+ * parameter is reference of a grant entry whose grant access will be
+ * stopped, second one is not in use now. If the grant entry is
+ * currently mapped for reading or writing, just return failure(==0)
+ * directly and don''t tear down the grant access. Otherwise, stop
grant
+ * access for this entry and return success(==1).
+ */
+ int (*end_foreign_access_ref)(grant_ref_t, int);
+ /*
+ * Stop granting a grant entry to domain for transfer. If tranfer has
+ * not started, just reclaim the grant entry and return failure(==0).
+ * Otherwise, wait for the transfer to complete and then return the
+ * frame.
+ */
+ unsigned long (*end_foreign_transfer_ref)(grant_ref_t);
+ /*
+ * Query the status of a grant entry. Input parameter is reference of
+ * queried grant entry, return value is the status of queried entry.
+ * Detailed status(writing/reading) can be gotten from the return value
+ * by bit operations.
+ */
+ int (*query_foreign_access)(grant_ref_t);
+} gnttab_interface;
+
+static int grant_table_version;
static struct gnttab_free_callback *gnttab_free_callback_list;
@@ -142,23 +196,23 @@ static void put_free_entry(grant_ref_t ref)
spin_unlock_irqrestore(&gnttab_list_lock, flags);
}
-static void update_grant_entry(grant_ref_t ref, domid_t domid,
- unsigned long frame, unsigned flags)
+/*
+ * Introducing a valid entry into the grant table:
+ * 1. Write ent->domid.
+ * 2. Write ent->frame:
+ * GTF_permit_access: Frame to which access is permitted.
+ * GTF_accept_transfer: Pseudo-phys frame slot being filled by new
+ * frame, or zero if none.
+ * 3. Write memory barrier (WMB).
+ * 4. Write ent->flags, inc. valid type.
+ */
+static void update_grant_entry_v1(grant_ref_t ref, domid_t domid,
+ unsigned long frame, unsigned flags)
{
- /*
- * Introducing a valid entry into the grant table:
- * 1. Write ent->domid.
- * 2. Write ent->frame:
- * GTF_permit_access: Frame to which access is permitted.
- * GTF_accept_transfer: Pseudo-phys frame slot being filled by new
- * frame, or zero if none.
- * 3. Write memory barrier (WMB).
- * 4. Write ent->flags, inc. valid type.
- */
- shared[ref].frame = frame;
- shared[ref].domid = domid;
+ gnttab_shared.v1[ref].domid = domid;
+ gnttab_shared.v1[ref].frame = frame;
wmb();
- shared[ref].flags = flags;
+ gnttab_shared.v1[ref].flags = flags;
}
/*
@@ -167,7 +221,7 @@ static void update_grant_entry(grant_ref_t ref, domid_t
domid,
void gnttab_grant_foreign_access_ref(grant_ref_t ref, domid_t domid,
unsigned long frame, int readonly)
{
- update_grant_entry(ref, domid, frame,
+ gnttab_interface.update_entry(ref, domid, frame,
GTF_permit_access | (readonly ? GTF_readonly : 0));
}
EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access_ref);
@@ -187,31 +241,40 @@ int gnttab_grant_foreign_access(domid_t domid, unsigned
long frame,
}
EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access);
-int gnttab_query_foreign_access(grant_ref_t ref)
+static int gnttab_query_foreign_access_v1(grant_ref_t ref)
{
- u16 nflags;
-
- nflags = shared[ref].flags;
+ return gnttab_shared.v1[ref].flags & (GTF_reading|GTF_writing);
+}
- return nflags & (GTF_reading|GTF_writing);
+int gnttab_query_foreign_access(grant_ref_t ref)
+{
+ return gnttab_interface.query_foreign_access(ref);
}
EXPORT_SYMBOL_GPL(gnttab_query_foreign_access);
-int gnttab_end_foreign_access_ref(grant_ref_t ref, int readonly)
+static int gnttab_end_foreign_access_ref_v1(grant_ref_t ref, int readonly)
{
u16 flags, nflags;
+ u16 *pflags;
- nflags = shared[ref].flags;
+ pflags = &gnttab_shared.v1[ref].flags;
+ nflags = *pflags;
do {
flags = nflags;
if (flags & (GTF_reading|GTF_writing)) {
printk(KERN_ALERT "WARNING: g.e. still in use!\n");
return 0;
}
- } while ((nflags = sync_cmpxchg(&shared[ref].flags, flags, 0)) != flags);
+ } while ((nflags = sync_cmpxchg(&gnttab_shared.v1[ref].flags, flags, 0))
+ != flags);
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;
/*
* 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);
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;
+ printk(KERN_INFO "Grant tables using version %d layout.\n",
+ grant_table_version);
+}
+
int gnttab_resume(void)
{
unsigned int max_nr_gframes;
+ gnttab_request_version();
max_nr_gframes = gnttab_max_grant_frames();
if (max_nr_gframes < nr_grant_frames)
return -ENOSYS;
@@ -587,9 +689,10 @@ int gnttab_resume(void)
if (xen_pv_domain())
return gnttab_map(0, nr_grant_frames - 1);
- if (!shared) {
- shared = ioremap(xen_hvm_resume_frames, PAGE_SIZE * max_nr_gframes);
- if (shared == NULL) {
+ if (gnttab_shared.addr == NULL) {
+ gnttab_shared.addr = ioremap(xen_hvm_resume_frames,
+ PAGE_SIZE * max_nr_gframes);
+ if (gnttab_shared.addr == NULL) {
printk(KERN_WARNING
"Failed to ioremap gnttab share frames!");
return -ENOMEM;
@@ -603,7 +706,7 @@ int gnttab_resume(void)
int gnttab_suspend(void)
{
- arch_gnttab_unmap_shared(shared, nr_grant_frames);
+ gnttab_interface.unmap_frames();
return 0;
}
diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
index 11e2dfc..c7a40f8 100644
--- a/include/xen/grant_table.h
+++ b/include/xen/grant_table.h
@@ -145,8 +145,8 @@ gnttab_set_unmap_op(struct gnttab_unmap_grant_ref *unmap,
phys_addr_t addr,
int arch_gnttab_map_shared(unsigned long *frames, unsigned long nr_gframes,
unsigned long max_nr_gframes,
- struct grant_entry **__shared);
-void arch_gnttab_unmap_shared(struct grant_entry *shared,
+ void **__shared);
+void arch_gnttab_unmap_shared(void *shared,
unsigned long nr_gframes);
extern unsigned long xen_hvm_resume_frames;
diff --git a/include/xen/interface/grant_table.h
b/include/xen/interface/grant_table.h
index 39e5717..a17d844 100644
--- a/include/xen/interface/grant_table.h
+++ b/include/xen/interface/grant_table.h
@@ -85,12 +85,22 @@
*/
/*
+ * Reference to a grant entry in a specified domain''s grant table.
+ */
+typedef uint32_t grant_ref_t;
+
+/*
* A grant table comprises a packed array of grant entries in one or more
* page frames shared between Xen and a guest.
* [XEN]: This field is written by Xen and read by the sharing guest.
* [GST]: This field is written by the guest and read by Xen.
*/
-struct grant_entry {
+
+/*
+ * Version 1 of the grant table entry structure is maintained purely
+ * for backwards compatibility. New guests should use version 2.
+ */
+struct grant_entry_v1 {
/* GTF_xxx: various type and flag information. [XEN,GST] */
uint16_t flags;
/* The domain being granted foreign privileges. [GST] */
@@ -108,10 +118,13 @@ struct grant_entry {
* GTF_permit_access: Allow @domid to map/access @frame.
* GTF_accept_transfer: Allow @domid to transfer ownership of one page frame
* to this guest. Xen writes the page number to @frame.
+ * GTF_transitive: Allow @domid to transitively access a subrange of
+ * @trans_grant in @trans_domid. No mappings are allowed.
*/
#define GTF_invalid (0U<<0)
#define GTF_permit_access (1U<<0)
#define GTF_accept_transfer (2U<<0)
+#define GTF_transitive (3U<<0)
#define GTF_type_mask (3U<<0)
/*
@@ -119,6 +132,9 @@ struct grant_entry {
* GTF_readonly: Restrict @domid to read-only mappings and accesses. [GST]
* GTF_reading: Grant entry is currently mapped for reading by @domid. [XEN]
* GTF_writing: Grant entry is currently mapped for writing by @domid. [XEN]
+ * GTF_sub_page: Grant access to only a subrange of the page. @domid
+ * will only be allowed to copy from the grant, and not
+ * map it. [GST]
*/
#define _GTF_readonly (2)
#define GTF_readonly (1U<<_GTF_readonly)
@@ -126,6 +142,8 @@ struct grant_entry {
#define GTF_reading (1U<<_GTF_reading)
#define _GTF_writing (4)
#define GTF_writing (1U<<_GTF_writing)
+#define _GTF_sub_page (8)
+#define GTF_sub_page (1U<<_GTF_sub_page)
/*
* Subflags for GTF_accept_transfer:
@@ -142,15 +160,81 @@ struct grant_entry {
#define _GTF_transfer_completed (3)
#define GTF_transfer_completed (1U<<_GTF_transfer_completed)
+/*
+ * Version 2 grant table entries. These fulfil the same role as
+ * version 1 entries, but can represent more complicated operations.
+ * Any given domain will have either a version 1 or a version 2 table,
+ * and every entry in the table will be the same version.
+ *
+ * The interface by which domains use grant references does not depend
+ * on the grant table version in use by the other domain.
+ */
-/***********************************
- * GRANT TABLE QUERIES AND USES
+/*
+ * Version 1 and version 2 grant entries share a common prefix. The
+ * fields of the prefix are documented as part of struct
+ * grant_entry_v1.
*/
+struct grant_entry_header {
+ uint16_t flags;
+ domid_t domid;
+};
/*
- * Reference to a grant entry in a specified domain''s grant table.
+ * Version 2 of the grant entry structure, here is an union because three
+ * different types are suppotted: full_page, sub_page and transitive.
+ */
+union grant_entry_v2 {
+ struct grant_entry_header hdr;
+
+ /*
+ * This member is used for V1-style full page grants, where either:
+ *
+ * -- hdr.type is GTF_accept_transfer, or
+ * -- hdr.type is GTF_permit_access and GTF_sub_page is not set.
+ *
+ * In that case, the frame field has the same semantics as the
+ * field of the same name in the V1 entry structure.
+ */
+ struct {
+ struct grant_entry_header hdr;
+ uint32_t pad0;
+ uint64_t frame;
+ } full_page;
+
+ /*
+ * If the grant type is GTF_grant_access and GTF_sub_page is set,
+ * @domid is allowed to access bytes [@page_off,@page_off+@length)
+ * in frame @frame.
+ */
+ struct {
+ struct grant_entry_header hdr;
+ uint16_t page_off;
+ uint16_t length;
+ uint64_t frame;
+ } sub_page;
+
+ /*
+ * If the grant is GTF_transitive, @domid is allowed to use the
+ * grant @gref in domain @trans_domid, as if it was the local
+ * domain. Obviously, the transitive access must be compatible
+ * with the original grant.
+ */
+ struct {
+ struct grant_entry_header hdr;
+ domid_t trans_domid;
+ uint16_t pad0;
+ grant_ref_t gref;
+ } transitive;
+
+ uint32_t __spacer[4]; /* Pad to a power of two */
+};
+
+typedef uint16_t grant_status_t;
+
+/***********************************
+ * GRANT TABLE QUERIES AND USES
*/
-typedef uint32_t grant_ref_t;
/*
* Handle to track a mapping created via a grant reference.
@@ -322,6 +406,79 @@ struct gnttab_query_size {
DEFINE_GUEST_HANDLE_STRUCT(gnttab_query_size);
/*
+ * GNTTABOP_unmap_and_replace: Destroy one or more grant-reference mappings
+ * tracked by <handle> but atomically replace the page table entry with
one
+ * pointing to the machine address under <new_addr>. <new_addr>
will be
+ * redirected to the null entry.
+ * NOTES:
+ * 1. The call may fail in an undefined manner if either mapping is not
+ * tracked by <handle>.
+ * 2. After executing a batch of unmaps, it is guaranteed that no stale
+ * mappings will remain in the device or host TLBs.
+ */
+#define GNTTABOP_unmap_and_replace 7
+struct gnttab_unmap_and_replace {
+ /* IN parameters. */
+ uint64_t host_addr;
+ uint64_t new_addr;
+ grant_handle_t handle;
+ /* OUT parameters. */
+ int16_t status; /* GNTST_* */
+};
+DEFINE_GUEST_HANDLE_STRUCT(gnttab_unmap_and_replace);
+
+/*
+ * GNTTABOP_set_version: Request a particular version of the grant
+ * table shared table structure. This operation can only be performed
+ * once in any given domain. It must be performed before any grants
+ * are activated; otherwise, the domain will be stuck with version 1.
+ * The only defined versions are 1 and 2.
+ */
+#define GNTTABOP_set_version 8
+struct gnttab_set_version {
+ /* IN parameters */
+ uint32_t version;
+};
+DEFINE_GUEST_HANDLE_STRUCT(gnttab_set_version);
+
+/*
+ * GNTTABOP_get_status_frames: Get the list of frames used to store grant
+ * status for <dom>. In grant format version 2, the status is separated
+ * from the other shared grant fields to allow more efficient synchronization
+ * using barriers instead of atomic cmpexch operations.
+ * <nr_frames> specify the size of vector <frame_list>.
+ * The frame addresses are returned in the <frame_list>.
+ * Only <nr_frames> addresses are returned, even if the table is larger.
+ * NOTES:
+ * 1. <dom> may be specified as DOMID_SELF.
+ * 2. Only a sufficiently-privileged domain may specify <dom> !=
DOMID_SELF.
+ */
+#define GNTTABOP_get_status_frames 9
+struct gnttab_get_status_frames {
+ /* IN parameters. */
+ uint32_t nr_frames;
+ domid_t dom;
+ /* OUT parameters. */
+ int16_t status; /* GNTST_* */
+ GUEST_HANDLE(uint64_t) frame_list;
+};
+DEFINE_GUEST_HANDLE_STRUCT(gnttab_get_status_frames);
+
+/*
+ * GNTTABOP_get_version: Get the grant table version which is in
+ * effect for domain <dom>.
+ */
+#define GNTTABOP_get_version 10
+struct gnttab_get_version {
+ /* IN parameters */
+ domid_t dom;
+ uint16_t pad;
+ /* OUT parameters */
+ uint32_t version;
+};
+DEFINE_GUEST_HANDLE_STRUCT(gnttab_get_version);
+
+/*
* 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);
+
#else /* __ASSEMBLY__ */
/* In assembly code we cannot use C numeric constant suffixes. */
--
1.7.6.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
annie.li@oracle.com
2011-Nov-16 13:49 UTC
[Xen-devel] [PATCH 2/3] xen/granttable: Grant tables V2 implementation
From: Annie Li <annie.li@oracle.com>
Receiver-side copying of packets is based on this implementation, it gives
better performance and better CPU accounting. It totally supports three types:
full-page, sub-page and transitive grants.
However this patch does not cover sub-page and transitive grants, it mainly
focus on Full-page part and implements grant table V2 interfaces corresponding
to what already exists in grant table V1, such as: grant table V2
initialization, mapping, releasing and exported interfaces.
Each guest can only supports one type of grant table type, every entry in grant
table should be the same version. It is necessary to set V1 or V2 version before
initializing the grant table.
Grant table exported interfaces of V2 are same with those of V1, Xen is
responsible to judge what grant table version guests are using in every grant
operation.
V2 fulfills the same role of V1, and it is totally backwards compitable with V1.
If dom0 support grant table V2, the guests runing on it can run with either V1
or V2.
Signed-off-by: Annie Li <annie.li@oracle.com>
---
arch/x86/xen/grant-table.c | 37 +++++++++-
drivers/xen/grant-table.c | 187 +++++++++++++++++++++++++++++++++++++++++--
include/xen/grant_table.h | 6 +-
3 files changed, 218 insertions(+), 12 deletions(-)
diff --git a/arch/x86/xen/grant-table.c b/arch/x86/xen/grant-table.c
index c6ab2e7..510fca0 100644
--- a/arch/x86/xen/grant-table.c
+++ b/arch/x86/xen/grant-table.c
@@ -54,6 +54,20 @@ static int map_pte_fn(pte_t *pte, struct page *pmd_page,
return 0;
}
+/*
+ * This function is used to map shared frames to store grant status. It is
+ * different from map_pte_fn above, the frames type here is uint64_t.
+ */
+static int map_pte_fn_status(pte_t *pte, struct page *pmd_page,
+ unsigned long addr, void *data)
+{
+ uint64_t **frames = (uint64_t **)data;
+
+ set_pte_at(&init_mm, addr, pte, mfn_pte((*frames)[0], PAGE_KERNEL));
+ (*frames)++;
+ return 0;
+}
+
static int unmap_pte_fn(pte_t *pte, struct page *pmd_page,
unsigned long addr, void *data)
{
@@ -83,7 +97,28 @@ int arch_gnttab_map_shared(unsigned long *frames, unsigned
long nr_gframes,
return rc;
}
-void arch_gnttab_unmap_shared(void *shared, unsigned long nr_gframes)
+int arch_gnttab_map_status(uint64_t *frames, unsigned long nr_gframes,
+ unsigned long max_nr_gframes,
+ grant_status_t **__shared)
+{
+ int rc;
+ grant_status_t *shared = *__shared;
+
+ if (shared == NULL) {
+ struct vm_struct *area + alloc_vm_area(PAGE_SIZE * max_nr_gframes);
+ BUG_ON(area == NULL);
+ shared = area->addr;
+ *__shared = shared;
+ }
+
+ rc = apply_to_page_range(&init_mm, (unsigned long)shared,
+ PAGE_SIZE * nr_gframes,
+ map_pte_fn_status, &frames);
+ return rc;
+}
+
+void arch_gnttab_unmap(void *shared, unsigned long nr_gframes)
{
apply_to_page_range(&init_mm, (unsigned long)shared,
PAGE_SIZE * nr_gframes, unmap_pte_fn, NULL);
diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index 1bd9d5f..0b412ce 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -44,6 +44,7 @@
#include <xen/page.h>
#include <xen/grant_table.h>
#include <xen/interface/memory.h>
+#include <xen/hvc-console.h>
#include <asm/xen/hypercall.h>
#include <asm/pgtable.h>
@@ -53,7 +54,10 @@
/* External tools reserve first few grant table entries. */
#define NR_RESERVED_ENTRIES 8
#define GNTTAB_LIST_END 0xffffffff
-#define GREFS_PER_GRANT_FRAME (PAGE_SIZE / sizeof(struct grant_entry_v1))
+#define GREFS_PER_GRANT_FRAME \
+(grant_table_version == 1 ? \
+ (PAGE_SIZE / sizeof(struct grant_entry_v1)) : \
+ (PAGE_SIZE / sizeof(union grant_entry_v2)))
static grant_ref_t **gnttab_list;
static unsigned int nr_grant_frames;
@@ -66,6 +70,7 @@ EXPORT_SYMBOL_GPL(xen_hvm_resume_frames);
static union {
struct grant_entry_v1 *v1;
+ union grant_entry_v2 *v2;
void *addr;
} gnttab_shared;
@@ -118,6 +123,9 @@ static struct {
int (*query_foreign_access)(grant_ref_t);
} gnttab_interface;
+/*This reflects status of grant entries, so act as a global value*/
+static grant_status_t *grstatus;
+
static int grant_table_version;
static struct gnttab_free_callback *gnttab_free_callback_list;
@@ -125,6 +133,7 @@ static struct gnttab_free_callback
*gnttab_free_callback_list;
static int gnttab_expand(unsigned int req_entries);
#define RPP (PAGE_SIZE / sizeof(grant_ref_t))
+#define SPP (PAGE_SIZE / sizeof(grant_status_t))
static inline grant_ref_t *__gnttab_entry(grant_ref_t entry)
{
@@ -197,6 +206,7 @@ static void put_free_entry(grant_ref_t ref)
}
/*
+ * Following comments apply to update_grant_entry_v1 and update_grant_entry_v2.
* Introducing a valid entry into the grant table:
* 1. Write ent->domid.
* 2. Write ent->frame:
@@ -215,6 +225,15 @@ static void update_grant_entry_v1(grant_ref_t ref, domid_t
domid,
gnttab_shared.v1[ref].flags = flags;
}
+static void update_grant_entry_v2(grant_ref_t ref, domid_t domid,
+ unsigned long frame, unsigned flags)
+{
+ gnttab_shared.v2[ref].hdr.domid = domid;
+ gnttab_shared.v2[ref].full_page.frame = frame;
+ wmb();
+ gnttab_shared.v2[ref].hdr.flags = GTF_permit_access | flags;
+}
+
/*
* Public grant-issuing interface functions
*/
@@ -246,6 +265,11 @@ static int gnttab_query_foreign_access_v1(grant_ref_t ref)
return gnttab_shared.v1[ref].flags & (GTF_reading|GTF_writing);
}
+static int gnttab_query_foreign_access_v2(grant_ref_t ref)
+{
+ return grstatus[ref] & (GTF_reading|GTF_writing);
+}
+
int gnttab_query_foreign_access(grant_ref_t ref)
{
return gnttab_interface.query_foreign_access(ref);
@@ -271,6 +295,29 @@ static int gnttab_end_foreign_access_ref_v1(grant_ref_t
ref, int readonly)
return 1;
}
+static int gnttab_end_foreign_access_ref_v2(grant_ref_t ref, int readonly)
+{
+ gnttab_shared.v2[ref].hdr.flags = 0;
+ mb();
+ if (grstatus[ref] & (GTF_reading|GTF_writing)) {
+ return 0;
+ } else {
+ /* The read of grstatus needs to have acquire
+ semantics. On x86, reads already have
+ that, and we just need to protect against
+ compiler reorderings. On other
+ architectures we may need a full
+ barrier. */
+#ifdef CONFIG_X86
+ barrier();
+#else
+ mb();
+#endif
+ }
+
+ return 1;
+}
+
int gnttab_end_foreign_access_ref(grant_ref_t ref, int readonly)
{
return gnttab_interface.end_foreign_access_ref(ref, readonly);
@@ -344,6 +391,37 @@ static unsigned long
gnttab_end_foreign_transfer_ref_v1(grant_ref_t ref)
return frame;
}
+static unsigned long gnttab_end_foreign_transfer_ref_v2(grant_ref_t ref)
+{
+ unsigned long frame;
+ u16 flags;
+ u16 *pflags;
+
+ pflags = &gnttab_shared.v2[ref].hdr.flags;
+
+ /*
+ * If a transfer is not even yet started, try to reclaim the grant
+ * reference and return failure (== 0).
+ */
+ 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 = *pflags;
+ cpu_relax();
+ }
+
+ rmb(); /* Read the frame number /after/ reading completion status. */
+ frame = gnttab_shared.v2[ref].full_page.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);
@@ -591,6 +669,11 @@ int gnttab_unmap_refs(struct gnttab_unmap_grant_ref
*unmap_ops,
}
EXPORT_SYMBOL_GPL(gnttab_unmap_refs);
+static unsigned nr_status_frames(unsigned nr_grant_frames)
+{
+ return (nr_grant_frames * GREFS_PER_GRANT_FRAME + SPP - 1) / SPP;
+}
+
static int gnttab_map_frames_v1(unsigned long *frames, unsigned int nr_gframes)
{
int rc;
@@ -605,7 +688,56 @@ static int gnttab_map_frames_v1(unsigned long *frames,
unsigned int nr_gframes)
static void gnttab_unmap_frames_v1(void)
{
- arch_gnttab_unmap_shared(gnttab_shared.addr, nr_grant_frames);
+ arch_gnttab_unmap(gnttab_shared.addr, nr_grant_frames);
+}
+
+static int gnttab_map_frames_v2(unsigned long *frames, unsigned int nr_gframes)
+{
+ uint64_t *sframes;
+ unsigned int nr_sframes;
+ struct gnttab_get_status_frames getframes;
+ int rc;
+
+ nr_sframes = nr_status_frames(nr_gframes);
+
+ /* No need for kzalloc as it is initialized in following hypercall
+ * GNTTABOP_get_status_frames.
+ */
+ sframes = kmalloc(nr_sframes * sizeof(uint64_t), GFP_ATOMIC);
+ if (!sframes)
+ return -ENOMEM;
+
+ getframes.dom = DOMID_SELF;
+ getframes.nr_frames = nr_sframes;
+ set_xen_guest_handle(getframes.frame_list, sframes);
+
+ rc = HYPERVISOR_grant_table_op(GNTTABOP_get_status_frames,
+ &getframes, 1);
+ if (rc == -ENOSYS) {
+ kfree(sframes);
+ return -ENOSYS;
+ }
+
+ BUG_ON(rc || getframes.status);
+
+ rc = arch_gnttab_map_status(sframes, nr_sframes,
+ nr_status_frames(gnttab_max_grant_frames()),
+ &grstatus);
+ BUG_ON(rc);
+ kfree(sframes);
+
+ 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_v2(void)
+{
+ arch_gnttab_unmap(gnttab_shared.addr, nr_grant_frames);
+ arch_gnttab_unmap(grstatus, nr_status_frames(nr_grant_frames));
}
static int gnttab_map(unsigned int start_idx, unsigned int end_idx)
@@ -639,6 +771,9 @@ static int gnttab_map(unsigned int start_idx, unsigned int
end_idx)
return rc;
}
+ /* No need for kzalloc as it is initialized in following hypercall
+ * GNTTABOP_setup_table.
+ */
frames = kmalloc(nr_gframes * sizeof(unsigned long), GFP_ATOMIC);
if (!frames)
return -ENOMEM;
@@ -659,20 +794,54 @@ static int gnttab_map(unsigned int start_idx, unsigned int
end_idx)
kfree(frames);
+ if (rc < 0)
+ return rc;
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 + int rc;
+ struct gnttab_set_version gsv;
+ const char *str = "we need grant tables version 2, but only version 1 is
available\n";
+
+ gsv.version = 2;
+ rc = HYPERVISOR_grant_table_op(GNTTABOP_set_version, &gsv, 1);
+ if (rc == 0) {
+ grant_table_version = 2;
+ gnttab_interface.map_frames = gnttab_map_frames_v2;
+ gnttab_interface.unmap_frames = gnttab_unmap_frames_v2;
+ gnttab_interface.update_entry = update_grant_entry_v2;
+ gnttab_interface.end_foreign_access_ref +
gnttab_end_foreign_access_ref_v2;
+ gnttab_interface.end_foreign_transfer_ref +
gnttab_end_foreign_transfer_ref_v2;
+ gnttab_interface.query_foreign_access + gnttab_query_foreign_access_v2;
+ } else {
+ if (grant_table_version == 2) {
+ /*
+ * If we''ve already used version 2 features,
+ * but then suddenly discover that they''re not
+ * available (e.g. migrating to an older
+ * version of Xen), almost unbounded badness
+ * can happen.
+ */
+ xen_raw_printk(str);
+ panic(str);
+ }
+ 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_interface.end_foreign_transfer_ref
gnttab_end_foreign_transfer_ref_v1;
- gnttab_interface.query_foreign_access = gnttab_query_foreign_access_v1;
+ gnttab_interface.query_foreign_access + gnttab_query_foreign_access_v1;
+
+ }
printk(KERN_INFO "Grant tables using version %d layout.\n",
grant_table_version);
}
diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
index c7a40f8..5494c40 100644
--- a/include/xen/grant_table.h
+++ b/include/xen/grant_table.h
@@ -146,8 +146,10 @@ gnttab_set_unmap_op(struct gnttab_unmap_grant_ref *unmap,
phys_addr_t addr,
int arch_gnttab_map_shared(unsigned long *frames, unsigned long nr_gframes,
unsigned long max_nr_gframes,
void **__shared);
-void arch_gnttab_unmap_shared(void *shared,
- unsigned long nr_gframes);
+int arch_gnttab_map_status(uint64_t *frames, unsigned long nr_gframes,
+ unsigned long max_nr_gframes,
+ grant_status_t **__shared);
+void arch_gnttab_unmap(void *shared, unsigned long nr_gframes);
extern unsigned long xen_hvm_resume_frames;
unsigned int gnttab_max_grant_frames(void);
--
1.7.6.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
annie.li@oracle.com
2011-Nov-16 13:50 UTC
[Xen-devel] [PATCH 3/3] xen/granttable: Keep code format clean
From: Annie Li <annie.li@oracle.com>
Signed-off-by: Annie Li <annie.li@oracle.com>
---
drivers/xen/grant-table.c | 7 +++----
include/xen/grant_table.h | 2 +-
2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index 0b412ce..a7eabb0 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -50,7 +50,6 @@
#include <asm/pgtable.h>
#include <asm/sync_bitops.h>
-
/* External tools reserve first few grant table entries. */
#define NR_RESERVED_ENTRIES 8
#define GNTTAB_LIST_END 0xffffffff
@@ -597,8 +596,8 @@ unsigned int gnttab_max_grant_frames(void)
EXPORT_SYMBOL_GPL(gnttab_max_grant_frames);
int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
- struct gnttab_map_grant_ref *kmap_ops,
- struct page **pages, unsigned int count)
+ struct gnttab_map_grant_ref *kmap_ops,
+ struct page **pages, unsigned int count)
{
int i, ret;
pte_t *pte;
@@ -648,7 +647,7 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
EXPORT_SYMBOL_GPL(gnttab_map_refs);
int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
- struct page **pages, unsigned int count)
+ struct page **pages, unsigned int count)
{
int i, ret;
diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
index 5494c40..fea4954 100644
--- a/include/xen/grant_table.h
+++ b/include/xen/grant_table.h
@@ -157,7 +157,7 @@ unsigned int gnttab_max_grant_frames(void);
#define gnttab_map_vaddr(map) ((void *)(map.host_virt_addr))
int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
- struct gnttab_map_grant_ref *kmap_ops,
+ struct gnttab_map_grant_ref *kmap_ops,
struct page **pages, unsigned int count);
int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
struct page **pages, unsigned int count);
--
1.7.6.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Campbell
2011-Nov-17 10:35 UTC
[Xen-devel] Re: [PATCH 1/3] xen/granttable: Introducing grant table V2 stucture
On Wed, 2011-11-16 at 13:48 +0000, annie.li@oracle.com wrote:> From: Annie Li <annie.li@oracle.com> > > This patch introduces new structures of grant table V2, grant table V2 is an > extension from V1. Grant table is shared between guest and Xen, and Xen is > responsible to do corresponding work for grant operations, such as: figure > out guest''s grant table version, perform different actions based on > different grant table version, etc. Although full-page structure of V2 > is different from V1, it play the same role as V1. > > Signed-off-by: Annie Li <annie.li@oracle.com> > --- > arch/x86/xen/grant-table.c | 7 +- > drivers/xen/grant-table.c | 181 +++++++++++++++++++++++++++-------- > include/xen/grant_table.h | 4 +- > include/xen/interface/grant_table.h | 167 +++++++++++++++++++++++++++++++- > include/xen/interface/xen.h | 2 + > 5 files changed, 311 insertions(+), 50 deletions(-) > > diff --git a/arch/x86/xen/grant-table.c b/arch/x86/xen/grant-table.c > index 6bbfd7a..c6ab2e7 100644 > --- a/arch/x86/xen/grant-table.c > +++ b/arch/x86/xen/grant-table.c > @@ -64,10 +64,10 @@ static int unmap_pte_fn(pte_t *pte, struct page *pmd_page, > > int arch_gnttab_map_shared(unsigned long *frames, unsigned long nr_gframes, > unsigned long max_nr_gframes, > - struct grant_entry **__shared) > + void **__shared) > { > int rc; > - struct grant_entry *shared = *__shared; > + void *shared = *__shared; > > if (shared == NULL) { > struct vm_struct *area > @@ -83,8 +83,7 @@ int arch_gnttab_map_shared(unsigned long *frames, unsigned long nr_gframes, > return rc; > } > > -void arch_gnttab_unmap_shared(struct grant_entry *shared, > - unsigned long nr_gframes) > +void arch_gnttab_unmap_shared(void *shared, unsigned long nr_gframes) > { > apply_to_page_range(&init_mm, (unsigned long)shared, > PAGE_SIZE * nr_gframes, unmap_pte_fn, NULL); > diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c > index bf1c094..1bd9d5f 100644 > --- a/drivers/xen/grant-table.c > +++ b/drivers/xen/grant-table.c > @@ -53,7 +53,7 @@ > /* External tools reserve first few grant table entries. */ > #define NR_RESERVED_ENTRIES 8 > #define GNTTAB_LIST_END 0xffffffff > -#define GREFS_PER_GRANT_FRAME (PAGE_SIZE / sizeof(struct grant_entry)) > +#define GREFS_PER_GRANT_FRAME (PAGE_SIZE / sizeof(struct grant_entry_v1)) > > static grant_ref_t **gnttab_list; > static unsigned int nr_grant_frames; > @@ -64,7 +64,61 @@ static DEFINE_SPINLOCK(gnttab_list_lock); > unsigned long xen_hvm_resume_frames; > EXPORT_SYMBOL_GPL(xen_hvm_resume_frames); > > -static struct grant_entry *shared; > +static union { > + struct grant_entry_v1 *v1; > + void *addr; > +} gnttab_shared; > + > +/*This is a structure of function pointers for grant table*/ > +static struct { > + /* > + * Mapping a list of frames for storing grant entries. First input > + * parameter is used to storing grant table address when grant table > + * being setup, second parameter is the number of frames to map grant > + * table. Returning GNTST_okay means success and negative value means > + * failure. > + */ > + int (*map_frames)(unsigned long *, unsigned int); > + /* > + * Release a list of frames which are mapped in map_frames for grant > + * entry status. > + */ > + void (*unmap_frames)(void); > + /* > + * Introducing a valid entry into the grant table, granting the frame > + * of this grant entry to domain for accessing, or transfering, or > + * transitively accessing. First input parameter is reference of this > + * introduced grant entry, second one is domid of granted domain, third > + * one is the frame to be granted, and the last one is status of the > + * grant entry to be updated. > + */ > + void (*update_entry)(grant_ref_t, domid_t, unsigned long, unsigned); > + /* > + * Stop granting a grant entry to domain for accessing. First input > + * parameter is reference of a grant entry whose grant access will be > + * stopped, second one is not in use now. If the grant entry is > + * currently mapped for reading or writing, just return failure(==0) > + * directly and don''t tear down the grant access. Otherwise, stop grant > + * access for this entry and return success(==1). > + */ > + int (*end_foreign_access_ref)(grant_ref_t, int); > + /* > + * Stop granting a grant entry to domain for transfer. If tranfer has > + * not started, just reclaim the grant entry and return failure(==0). > + * Otherwise, wait for the transfer to complete and then return the > + * frame. > + */ > + unsigned long (*end_foreign_transfer_ref)(grant_ref_t); > + /* > + * Query the status of a grant entry. Input parameter is reference of > + * queried grant entry, return value is the status of queried entry. > + * Detailed status(writing/reading) can be gotten from the return value > + * by bit operations. > + */ > + int (*query_foreign_access)(grant_ref_t); > +} gnttab_interface; > + > +static int grant_table_version; > > static struct gnttab_free_callback *gnttab_free_callback_list; > > @@ -142,23 +196,23 @@ static void put_free_entry(grant_ref_t ref) > spin_unlock_irqrestore(&gnttab_list_lock, flags); > } > > -static void update_grant_entry(grant_ref_t ref, domid_t domid, > - unsigned long frame, unsigned flags) > +/* > + * Introducing a valid entry into the grant table: > + * 1. Write ent->domid. > + * 2. Write ent->frame: > + * GTF_permit_access: Frame to which access is permitted. > + * GTF_accept_transfer: Pseudo-phys frame slot being filled by new > + * frame, or zero if none. > + * 3. Write memory barrier (WMB). > + * 4. Write ent->flags, inc. valid type. > + */ > +static void update_grant_entry_v1(grant_ref_t ref, domid_t domid, > + unsigned long frame, unsigned flags) > { > - /* > - * Introducing a valid entry into the grant table: > - * 1. Write ent->domid. > - * 2. Write ent->frame: > - * GTF_permit_access: Frame to which access is permitted. > - * GTF_accept_transfer: Pseudo-phys frame slot being filled by new > - * frame, or zero if none. > - * 3. Write memory barrier (WMB). > - * 4. Write ent->flags, inc. valid type. > - */ > - shared[ref].frame = frame; > - shared[ref].domid = domid; > + gnttab_shared.v1[ref].domid = domid; > + gnttab_shared.v1[ref].frame = frame; > wmb(); > - shared[ref].flags = flags; > + gnttab_shared.v1[ref].flags = flags; > } > > /* > @@ -167,7 +221,7 @@ static void update_grant_entry(grant_ref_t ref, domid_t domid, > void gnttab_grant_foreign_access_ref(grant_ref_t ref, domid_t domid, > unsigned long frame, int readonly) > { > - update_grant_entry(ref, domid, frame, > + gnttab_interface.update_entry(ref, domid, frame, > GTF_permit_access | (readonly ? GTF_readonly : 0)); > } > EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access_ref); > @@ -187,31 +241,40 @@ int gnttab_grant_foreign_access(domid_t domid, unsigned long frame, > } > EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access); > > -int gnttab_query_foreign_access(grant_ref_t ref) > +static int gnttab_query_foreign_access_v1(grant_ref_t ref) > { > - u16 nflags; > - > - nflags = shared[ref].flags; > + return gnttab_shared.v1[ref].flags & (GTF_reading|GTF_writing); > +} > - return nflags & (GTF_reading|GTF_writing); > +int gnttab_query_foreign_access(grant_ref_t ref) > +{ > + return gnttab_interface.query_foreign_access(ref); > } > EXPORT_SYMBOL_GPL(gnttab_query_foreign_access); > > -int gnttab_end_foreign_access_ref(grant_ref_t ref, int readonly) > +static int gnttab_end_foreign_access_ref_v1(grant_ref_t ref, int readonly) > { > u16 flags, nflags; > + u16 *pflags; > > - nflags = shared[ref].flags; > + pflags = &gnttab_shared.v1[ref].flags; > + nflags = *pflags; > do { > flags = nflags; > if (flags & (GTF_reading|GTF_writing)) { > printk(KERN_ALERT "WARNING: g.e. still in use!\n"); > return 0; > } > - } 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"?> > 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.> > /* > * 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?> > 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;> + printk(KERN_INFO "Grant tables using version %d layout.\n", > + grant_table_version); > +} > + > int gnttab_resume(void) > { > unsigned int max_nr_gframes; > > + gnttab_request_version(); > max_nr_gframes = gnttab_max_grant_frames(); > if (max_nr_gframes < nr_grant_frames) > return -ENOSYS; > @@ -587,9 +689,10 @@ int gnttab_resume(void) > if (xen_pv_domain()) > return gnttab_map(0, nr_grant_frames - 1); > > - if (!shared) { > - shared = ioremap(xen_hvm_resume_frames, PAGE_SIZE * max_nr_gframes); > - if (shared == NULL) { > + if (gnttab_shared.addr == NULL) { > + gnttab_shared.addr = ioremap(xen_hvm_resume_frames, > + PAGE_SIZE * max_nr_gframes); > + if (gnttab_shared.addr == NULL) { > printk(KERN_WARNING > "Failed to ioremap gnttab share frames!"); > return -ENOMEM; > @@ -603,7 +706,7 @@ int gnttab_resume(void) > > int gnttab_suspend(void) > { > - arch_gnttab_unmap_shared(shared, nr_grant_frames); > + gnttab_interface.unmap_frames(); > return 0; > } > > diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h > index 11e2dfc..c7a40f8 100644 > --- a/include/xen/grant_table.h > +++ b/include/xen/grant_table.h > @@ -145,8 +145,8 @@ gnttab_set_unmap_op(struct gnttab_unmap_grant_ref *unmap, phys_addr_t addr, > > int arch_gnttab_map_shared(unsigned long *frames, unsigned long nr_gframes, > unsigned long max_nr_gframes, > - struct grant_entry **__shared); > -void arch_gnttab_unmap_shared(struct grant_entry *shared, > + void **__shared); > +void arch_gnttab_unmap_shared(void *shared, > unsigned long nr_gframes); > > extern unsigned long xen_hvm_resume_frames; > diff --git a/include/xen/interface/grant_table.h b/include/xen/interface/grant_table.h > index 39e5717..a17d844 100644 > --- a/include/xen/interface/grant_table.h > +++ b/include/xen/interface/grant_table.h > @@ -85,12 +85,22 @@ > */ > > /* > + * Reference to a grant entry in a specified domain''s grant table. > + */ > +typedef uint32_t grant_ref_t; > + > +/* > * A grant table comprises a packed array of grant entries in one or more > * page frames shared between Xen and a guest. > * [XEN]: This field is written by Xen and read by the sharing guest. > * [GST]: This field is written by the guest and read by Xen. > */ > -struct grant_entry { > + > +/* > + * Version 1 of the grant table entry structure is maintained purely > + * for backwards compatibility. New guests should use version 2. > + */ > +struct grant_entry_v1 { > /* GTF_xxx: various type and flag information. [XEN,GST] */ > uint16_t flags; > /* The domain being granted foreign privileges. [GST] */ > @@ -108,10 +118,13 @@ struct grant_entry { > * GTF_permit_access: Allow @domid to map/access @frame. > * GTF_accept_transfer: Allow @domid to transfer ownership of one page frame > * to this guest. Xen writes the page number to @frame. > + * GTF_transitive: Allow @domid to transitively access a subrange of > + * @trans_grant in @trans_domid. No mappings are allowed. > */ > #define GTF_invalid (0U<<0) > #define GTF_permit_access (1U<<0) > #define GTF_accept_transfer (2U<<0) > +#define GTF_transitive (3U<<0) > #define GTF_type_mask (3U<<0) > > /* > @@ -119,6 +132,9 @@ struct grant_entry { > * GTF_readonly: Restrict @domid to read-only mappings and accesses. [GST] > * GTF_reading: Grant entry is currently mapped for reading by @domid. [XEN] > * GTF_writing: Grant entry is currently mapped for writing by @domid. [XEN] > + * GTF_sub_page: Grant access to only a subrange of the page. @domid > + * will only be allowed to copy from the grant, and not > + * map it. [GST] > */ > #define _GTF_readonly (2) > #define GTF_readonly (1U<<_GTF_readonly) > @@ -126,6 +142,8 @@ struct grant_entry { > #define GTF_reading (1U<<_GTF_reading) > #define _GTF_writing (4) > #define GTF_writing (1U<<_GTF_writing) > +#define _GTF_sub_page (8) > +#define GTF_sub_page (1U<<_GTF_sub_page) > > /* > * Subflags for GTF_accept_transfer: > @@ -142,15 +160,81 @@ struct grant_entry { > #define _GTF_transfer_completed (3) > #define GTF_transfer_completed (1U<<_GTF_transfer_completed) > > +/* > + * Version 2 grant table entries. These fulfil the same role as > + * version 1 entries, but can represent more complicated operations. > + * Any given domain will have either a version 1 or a version 2 table, > + * and every entry in the table will be the same version. > + * > + * The interface by which domains use grant references does not depend > + * on the grant table version in use by the other domain. > + */ > > -/*********************************** > - * GRANT TABLE QUERIES AND USES > +/* > + * Version 1 and version 2 grant entries share a common prefix. The > + * fields of the prefix are documented as part of struct > + * grant_entry_v1. > */ > +struct grant_entry_header { > + uint16_t flags; > + domid_t domid; > +}; > > /* > - * Reference to a grant entry in a specified domain''s grant table. > + * Version 2 of the grant entry structure, here is an union because three > + * different types are suppotted: full_page, sub_page and transitive. > + */ > +union grant_entry_v2 { > + struct grant_entry_header hdr; > + > + /* > + * This member is used for V1-style full page grants, where either: > + * > + * -- hdr.type is GTF_accept_transfer, or > + * -- hdr.type is GTF_permit_access and GTF_sub_page is not set. > + * > + * In that case, the frame field has the same semantics as the > + * field of the same name in the V1 entry structure. > + */ > + struct { > + struct grant_entry_header hdr; > + uint32_t pad0; > + uint64_t frame; > + } full_page; > + > + /* > + * If the grant type is GTF_grant_access and GTF_sub_page is set, > + * @domid is allowed to access bytes [@page_off,@page_off+@length) > + * in frame @frame. > + */ > + struct { > + struct grant_entry_header hdr; > + uint16_t page_off; > + uint16_t length; > + uint64_t frame; > + } sub_page; > + > + /* > + * If the grant is GTF_transitive, @domid is allowed to use the > + * grant @gref in domain @trans_domid, as if it was the local > + * domain. Obviously, the transitive access must be compatible > + * with the original grant. > + */ > + struct { > + struct grant_entry_header hdr; > + domid_t trans_domid; > + uint16_t pad0; > + grant_ref_t gref; > + } transitive; > + > + uint32_t __spacer[4]; /* Pad to a power of two */ > +}; > + > +typedef uint16_t grant_status_t; > + > +/*********************************** > + * GRANT TABLE QUERIES AND USES > */ > -typedef uint32_t grant_ref_t; > > /* > * Handle to track a mapping created via a grant reference. > @@ -322,6 +406,79 @@ struct gnttab_query_size { > DEFINE_GUEST_HANDLE_STRUCT(gnttab_query_size); > > /* > + * GNTTABOP_unmap_and_replace: Destroy one or more grant-reference mappings > + * tracked by <handle> but atomically replace the page table entry with one > + * pointing to the machine address under <new_addr>. <new_addr> will be > + * redirected to the null entry. > + * NOTES: > + * 1. The call may fail in an undefined manner if either mapping is not > + * tracked by <handle>. > + * 2. After executing a batch of unmaps, it is guaranteed that no stale > + * mappings will remain in the device or host TLBs. > + */ > +#define GNTTABOP_unmap_and_replace 7 > +struct gnttab_unmap_and_replace { > + /* IN parameters. */ > + uint64_t host_addr; > + uint64_t new_addr; > + grant_handle_t handle; > + /* OUT parameters. */ > + int16_t status; /* GNTST_* */ > +}; > +DEFINE_GUEST_HANDLE_STRUCT(gnttab_unmap_and_replace); > + > +/* > + * GNTTABOP_set_version: Request a particular version of the grant > + * table shared table structure. This operation can only be performed > + * once in any given domain. It must be performed before any grants > + * are activated; otherwise, the domain will be stuck with version 1. > + * The only defined versions are 1 and 2. > + */ > +#define GNTTABOP_set_version 8 > +struct gnttab_set_version { > + /* IN parameters */ > + uint32_t version; > +}; > +DEFINE_GUEST_HANDLE_STRUCT(gnttab_set_version); > + > +/* > + * GNTTABOP_get_status_frames: Get the list of frames used to store grant > + * status for <dom>. In grant format version 2, the status is separated > + * from the other shared grant fields to allow more efficient synchronization > + * using barriers instead of atomic cmpexch operations. > + * <nr_frames> specify the size of vector <frame_list>. > + * The frame addresses are returned in the <frame_list>. > + * Only <nr_frames> addresses are returned, even if the table is larger. > + * NOTES: > + * 1. <dom> may be specified as DOMID_SELF. > + * 2. Only a sufficiently-privileged domain may specify <dom> != DOMID_SELF. > + */ > +#define GNTTABOP_get_status_frames 9 > +struct gnttab_get_status_frames { > + /* IN parameters. */ > + uint32_t nr_frames; > + domid_t dom; > + /* OUT parameters. */ > + int16_t status; /* GNTST_* */ > + GUEST_HANDLE(uint64_t) frame_list; > +}; > +DEFINE_GUEST_HANDLE_STRUCT(gnttab_get_status_frames); > + > +/* > + * GNTTABOP_get_version: Get the grant table version which is in > + * effect for domain <dom>. > + */ > +#define GNTTABOP_get_version 10 > +struct gnttab_get_version { > + /* IN parameters */ > + domid_t dom; > + uint16_t pad; > + /* OUT parameters */ > + uint32_t version; > +}; > +DEFINE_GUEST_HANDLE_STRUCT(gnttab_get_version); > + > +/* > * 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 >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
annie li
2011-Nov-17 16:06 UTC
[Xen-devel] Re: [PATCH 1/3] xen/granttable: Introducing grant table V2 stucture
Konrad Rzeszutek Wilk
2011-Nov-17 16:29 UTC
[Xen-devel] Re: [PATCH 1/3] xen/granttable: Introducing grant table V2 stucture
> >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?Why two functions? I agree on the structures - but they need not to be public (they can be static). For a good example look at how apic_physflat is done. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Nov-17 16:55 UTC
[Xen-devel] Re: [PATCH 1/3] xen/granttable: Introducing grant table V2 stucture
On Thu, 2011-11-17 at 16:06 +0000, annie li wrote:> 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.My point about strictly adhering to the 80-column limit still stands.> > > 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?I mean that this patch combines a bunch of renaming, an abstraction to function pointers and other refactoring such as this which actually change behaviour (or might). It''s easier to review if the purely mechanical stuff (like renaming or adding a layer of function pointers) is split out from changes like this one.> > > /* > > > * 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;Can rc ever be +ve? Doesn''t look like it, in which case this is the same as "return rc".> 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.OK.> > ..... > > > +/* > > > * 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 > > > > > > > > > > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
ANNIE LI
2011-Nov-18 03:08 UTC
Re: [Xen-devel] Re: [PATCH 1/3] xen/granttable: Introducing grant table V2 stucture
On 2011-11-18 0:29, Konrad Rzeszutek Wilk wrote:>>> 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? > Why two functions? I agree on the structures - but they need not to be > public (they can be static). > > For a good example look at how apic_physflat is done.Thanks, static structure is simpler and clean. I am very glad to change that.:-) Thanks Annie> _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
ANNIE LI
2011-Nov-18 10:07 UTC
Re: [Xen-devel] [PATCH 1/3] xen/granttable: Introducing grant table V2 stucture
Hi I split this patch into two, 0001-** is about renaming and abstracting to function pointers, 0002-** is for refactoring code. Thanks Annie _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
ANNIE LI
2011-Nov-18 10:13 UTC
[Xen-devel] Re: [PATCH 2/3] xen/granttable: Grant tables V2 implementation
Defining a function pointer structure gnttab_v2_ops, the changes correspond to those in [PATCH 1/3] xen/granttable: Introducing grant table V2 stucture. Thanks Annie _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
ANNIE LI
2011-Nov-18 10:16 UTC
[Xen-devel] Re: [PATCH 3/3] xen/granttable: Keep code format clean
This patch does not change from the initial one: [PATCH 3/3] xen/granttable: Keep code format clean. Thanks Annie _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Nov-18 10:54 UTC
Re: [Xen-devel] [PATCH 1/3] xen/granttable: Introducing grant table V2 stucture
Thanks for splitting these up. On Fri, 2011-11-18 at 10:07 +0000, ANNIE LI wrote:> [...]> return value > + * by bit operations. > + */ > + int (*query_foreign_access)(grant_ref_t); > +}; > + > +static struct gnttab_ops gnttab_v1_ops;You don''t actually need this forward declaration since the struct definition and usage are ordered correctly.> +static struct gnttab_ops gnttab_v1_ops = { > + .map_frames = gnttab_map_frames_v1, > + .unmap_frames = gnttab_unmap_frames_v1, > + .update_entry = update_grant_entry_v1,Any reason this one is not gnttab_foo? Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Nov-18 11:02 UTC
Re: [PATCH 2/3] xen/granttable: Grant tables V2 implementation
On Fri, 2011-11-18 at 10:13 +0000, ANNIE LI wrote:> static void gnttab_request_version(void) > { > - grant_table_version = 1; > - gnttab_interface = &gnttab_v1_ops; > + int rc; > + struct gnttab_set_version gsv; > + const char *str = "we need grant tables version 2, but only version 1 is available\n"; > + > + gsv.version = 2; > + rc = HYPERVISOR_grant_table_op(GNTTABOP_set_version, &gsv, 1); > + if (rc == 0) { > + grant_table_version = 2; > + gnttab_interface = &gnttab_v2_ops; > + } else { > + if (grant_table_version == 2) {} else if (...) { panic } else { setup v1 }> + /* > + * If we''ve already used version 2 features, > + * but then suddenly discover that they''re not > + * available (e.g. migrating to an older > + * version of Xen), almost unbounded badness > + * can happen. > + */ > + xen_raw_printk(str); > + panic(str);I expect you''ve just copied this style from elsewhere but I really dislike this duplication of prints. If panic is not useful here we really ought to address that at the root instead of going around patching things to print every panic message twice. I thought earlyprintk was supposed to solve this problem. Perhaps a generic early_panic_print could be added to the panic code? If we stick with this (which is fair enough since it is outside the scope of this series) you should move the const char *str into this code block so it is near to the use.> + } > + grant_table_version = 1; > + gnttab_interface = &gnttab_v1_ops; > + } > printk(KERN_INFO "Grant tables using version %d layout.\n", > grant_table_version); > }
Konrad Rzeszutek Wilk
2011-Nov-18 13:52 UTC
Re: [Xen-devel] Re: [PATCH 2/3] xen/granttable: Grant tables V2 implementation
> > + xen_raw_printk(str); > > + panic(str); > > I expect you''ve just copied this style from elsewhere but I really > dislike this duplication of prints. If panic is not useful here we > really ought to address that at the root instead of going around > patching things to print every panic message twice. I thought > earlyprintk was supposed to solve this problem. Perhaps a generic > early_panic_print could be added to the panic code?We are using this combo in swiotlb-xen and as well in the xen pci. We could declere a ''xen_raw_panic'' that would do the job? The problem is that panic() uses the "late" printk mechanism (so it goes through the buffer that ends up not beign flushed) and the panic never sees the light. The ''xen_raw_printk'' is synchronous.. But I wonder if the panic surfaces if ''earlyprintk=xen'' is used? At which point it might be that the those extra xen_raw_printk become pointless?
Ian Campbell
2011-Nov-18 14:00 UTC
Re: [Xen-devel] Re: [PATCH 2/3] xen/granttable: Grant tables V2 implementation
On Fri, 2011-11-18 at 13:52 +0000, Konrad Rzeszutek Wilk wrote:> > > + xen_raw_printk(str); > > > + panic(str); > > > > I expect you''ve just copied this style from elsewhere but I really > > dislike this duplication of prints. If panic is not useful here we > > really ought to address that at the root instead of going around > > patching things to print every panic message twice. I thought > > earlyprintk was supposed to solve this problem. Perhaps a generic > > early_panic_print could be added to the panic code? > > We are using this combo in swiotlb-xen and as well in the xen pci. > We could declere a ''xen_raw_panic'' that would do the job? > > The problem is that panic() uses the "late" printk mechanism (so > it goes through the buffer that ends up not beign flushed) and the > panic never sees the light.So lets fix that instead of working around it...> The ''xen_raw_printk'' is synchronous.. > > But I wonder if the panic surfaces if ''earlyprintk=xen'' is used? > At which point it might be that the those extra xen_raw_printk > become pointless?I think panic''s do come out with earlyprintk (unless they are truly super early). Ian.
annie li
2011-Nov-18 16:02 UTC
Re: [PATCH 1/3] xen/granttable: Introducing grant table V2 stucture
On 2011-11-18 18:54, Ian Campbell wrote:> Thanks for splitting these up. > > On Fri, 2011-11-18 at 10:07 +0000, ANNIE LI wrote: > >> [...] >> > > >> return value >> + * by bit operations. >> + */ >> + int (*query_foreign_access)(grant_ref_t); >> +}; >> + >> +static struct gnttab_ops gnttab_v1_ops; >> > > You don''t actually need this forward declaration since the struct > definition and usage are ordered correctly. > >Yes, you are right, this line should be removed.>> +static struct gnttab_ops gnttab_v1_ops = { >> + .map_frames = gnttab_map_frames_v1, >> + .unmap_frames = gnttab_unmap_frames_v1, >> + .update_entry = update_grant_entry_v1, >> > > Any reason this one is not gnttab_foo? >Actually no, just keep the original name of this function. I''d like to change it, maybe gnttab_update_entry_v1 is better? Thanks Annie> Ian. > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Nov-18 16:04 UTC
Re: [Xen-devel] [PATCH 1/3] xen/granttable: Introducing grant table V2 stucture
> > > +static struct gnttab_ops gnttab_v1_ops = { > > > + .map_frames = gnttab_map_frames_v1, > > > + .unmap_frames = gnttab_unmap_frames_v1, > > > + .update_entry = update_grant_entry_v1, > > > > > > > Any reason this one is not gnttab_foo? > > > Actually no, just keep the original name of this function. I''d like to > change it, maybe gnttab_update_entry_v1 is better?I think so. Similarly for the v2 variant. Ian.
annie li
2011-Nov-18 16:10 UTC
Re: [Xen-devel] Re: [PATCH 2/3] xen/granttable: Grant tables V2 implementation
On 2011-11-18 22:00, Ian Campbell wrote:> On Fri, 2011-11-18 at 13:52 +0000, Konrad Rzeszutek Wilk wrote: > >>>> + xen_raw_printk(str); >>>> + panic(str); >>>> >>> I expect you''ve just copied this style from elsewhere but I really >>> dislike this duplication of prints. If panic is not useful here we >>> really ought to address that at the root instead of going around >>> patching things to print every panic message twice. I thought >>> earlyprintk was supposed to solve this problem. Perhaps a generic >>> early_panic_print could be added to the panic code? >>> >> We are using this combo in swiotlb-xen and as well in the xen pci. >> We could declere a ''xen_raw_panic'' that would do the job? >> >> The problem is that panic() uses the "late" printk mechanism (so >> it goes through the buffer that ends up not beign flushed) and the >> panic never sees the light. >> > > So lets fix that instead of working around it... > > >> The ''xen_raw_printk'' is synchronous.. >> >> But I wonder if the panic surfaces if ''earlyprintk=xen'' is used? >> At which point it might be that the those extra xen_raw_printk >> become pointless? >> > > I think panic''s do come out with earlyprintk (unless they are truly > super early). >So we have two candidates: xen_raw_printk + panic panic + earlyprintk=xen If panic does work with earlyprintk, then the latter one is better. Otherwise, there will be duplicated string printed out with ''earlyprintk=xen''. Thanks Annie> Ian. > > > >
Konrad Rzeszutek Wilk
2011-Nov-18 18:05 UTC
Re: [Xen-devel] Re: [PATCH 2/3] xen/granttable: Grant tables V2 implementation
> So we have two candidates: > xen_raw_printk + panic > panic + earlyprintk=xen > > If panic does work with earlyprintk, then the latter one is better. > Otherwise, there will be duplicated string printed out with > ''earlyprintk=xen''.The idea is just to have one function. Whichever prints the string and panics the machine. If ''panic'' does this properly (and properly meaning it actually prints data when using the earlyprintk=xen as well as console=hvc0) printout system the we cuold just use ''panic'' and not worry about it. But if it does not, then we (and by we I mean you) should provide a variant of panic() that prints the data properly using the earlprintk mechanism. Preferrabily to make it generic.
annie li
2011-Nov-19 03:36 UTC
Re: [Xen-devel] Re: [PATCH 2/3] xen/granttable: Grant tables V2 implementation
On 2011-11-19 2:05, Konrad Rzeszutek Wilk wrote:>> So we have two candidates: >> xen_raw_printk + panic >> panic + earlyprintk=xen >> >> If panic does work with earlyprintk, then the latter one is better. >> Otherwise, there will be duplicated string printed out with >> ''earlyprintk=xen''. >> > > The idea is just to have one function. Whichever prints the > string and panics the machine. If ''panic'' does this properly > (and properly meaning it actually prints data when using > the earlyprintk=xen as well as console=hvc0) printout system > the we cuold just use ''panic'' and not worry about it. > >Ok.> But if it does not, then we (and by we I mean you) should > provide a variant of panic() that prints the data properly using the > earlprintk mechanism. Preferrabily to make it generic. >So this work depends on whether panic works well with earlyprink=xen, I will do some test on panic first. Thanks Annie
ANNIE LI
2011-Nov-21 09:51 UTC
Re: [Xen-devel] [PATCH 1/3] xen/granttable: Introducing grant table V2 stucture
This patch changes two places from the last one, * removing gnttab_v1_ops * change update_grant_entry_v1 into gnttab_update_entry_v1 Thanks Annie On 2011-11-19 0:04, Ian Campbell wrote:>>>> +static struct gnttab_ops gnttab_v1_ops = { >>>> + .map_frames = gnttab_map_frames_v1, >>>> + .unmap_frames = gnttab_unmap_frames_v1, >>>> + .update_entry = update_grant_entry_v1, >>>> >>> Any reason this one is not gnttab_foo? >>> >> Actually no, just keep the original name of this function. I''d like to >> change it, maybe gnttab_update_entry_v1 is better? > I think so. Similarly for the v2 variant. > > Ian. > >
ANNIE LI
2011-Nov-21 09:51 UTC
Re: [Xen-devel] Re: [PATCH 2/3] xen/granttable: Grant tables V2 implementation
> The idea is just to have one function. Whichever prints the > string and panics the machine. If ''panic'' does this properly > (and properly meaning it actually prints data when using > the earlyprintk=xen as well as console=hvc0) printout system > the we cuold just use ''panic'' and not worry about it. > > > But if it does not, then we (and by we I mean you) should > provide a variant of panic() that prints the data properly using the > earlprintk mechanism. Preferrabily to make it generic.I did a simple test, a serial cable was connected, and console=hvc0 was added in grub.conf. Whether earlyprintk=xen is set or not, both panic() and xen_raw_printk all can print out strings in gnttab_request_version of grant-table.c. So I changed the xen_raw_printk(); panic(); back to panic(); Other change is the re-arrange "else if" format in gnttab_request_version function. Please refer to the patch attached. Thanks Annie
Ian Campbell
2011-Nov-21 10:35 UTC
Re: [Xen-devel] Re: [PATCH 2/3] xen/granttable: Grant tables V2 implementation
On Mon, 2011-11-21 at 09:51 +0000, ANNIE LI wrote:> > The idea is just to have one function. Whichever prints the > > string and panics the machine. If ''panic'' does this properly > > (and properly meaning it actually prints data when using > > the earlyprintk=xen as well as console=hvc0) printout system > > the we cuold just use ''panic'' and not worry about it. > > > > > > But if it does not, then we (and by we I mean you) should > > provide a variant of panic() that prints the data properly using the > > earlprintk mechanism. Preferrabily to make it generic. > I did a simple test, a serial cable was connected, and console=hvc0 was > added in grub.conf. > Whether earlyprintk=xen is set or not, both panic() and xen_raw_printk > all can print out strings in gnttab_request_version of grant-table.c. > > So I changed the > > xen_raw_printk(); > panic(); > > back to > > panic();> Other change is the re-arrange "else if" format in > gnttab_request_version function.Neither of these changes are in the attached patch, did you resend an old one by mistake? I think a fresh reposting of the series is in order, at least I''ve rather lost track of which patches are the most recent ones in this thread. Ian.> > Please refer to the patch attached. > > Thanks > Annie
Ian Campbell
2011-Nov-21 10:36 UTC
Re: [Xen-devel] [PATCH 1/3] xen/granttable: Introducing grant table V2 stucture
On Mon, 2011-11-21 at 09:51 +0000, ANNIE LI wrote:> This patch changes two places from the last one, > * removing gnttab_v1_ops > * change update_grant_entry_v1 into gnttab_update_entry_v1The attached patch didn''t have those changes. Ian.> > Thanks > Annie > > On 2011-11-19 0:04, Ian Campbell wrote: > >>>> +static struct gnttab_ops gnttab_v1_ops = { > >>>> + .map_frames = gnttab_map_frames_v1, > >>>> + .unmap_frames = gnttab_unmap_frames_v1, > >>>> + .update_entry = update_grant_entry_v1, > >>>> > >>> Any reason this one is not gnttab_foo? > >>> > >> Actually no, just keep the original name of this function. I''d like to > >> change it, maybe gnttab_update_entry_v1 is better? > > I think so. Similarly for the v2 variant. > > > > Ian. > > > >
annie li
2011-Nov-21 11:42 UTC
Re: [Xen-devel] Re: [PATCH 2/3] xen/granttable: Grant tables V2 implementation
> Neither of these changes are in the attached patch, did you resend an > old one by mistake? >Sorry, I sent the old ones by mistake. :(> I think a fresh reposting of the series is in order, at least I''ve > rather lost track of which patches are the most recent ones in this > thread. >OK, I will repost the patches in a new thread. Thanks Annie> Ian. >