Introduce a set of macros that abstract version specifics of the grant table
structures. This lays the ground work for being able to turn off v2 logic and
code.
Authored-by: Christopher Clark <christopher.clark6@baesystems.com>
Signed-off-by: Christopher Clark <christopher.w.clark@gmail.com>
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
xen/common/grant_table.c | 114 +++++++++++++++++----------------------
1 file changed, 48 insertions(+), 66 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 6c77867f8c..f70a5333d8 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -181,6 +181,7 @@ static int cf_check parse_gnttab_max_maptrack_frames(const char *arg)
#ifndef GNTTAB_MAX_VERSION
#define GNTTAB_MAX_VERSION 2
#endif
+#define get_gt_version(gt) ((gt)->gt_version)
unsigned int __read_mostly opt_gnttab_max_version = GNTTAB_MAX_VERSION;
static bool __read_mostly opt_transitive_grants = true;
@@ -310,16 +311,30 @@ nr_maptrack_frames(struct grant_table *t)
#define SHGNT_PER_PAGE_V1 (PAGE_SIZE / sizeof(grant_entry_v1_t))
#define shared_entry_v1(t, e) \
((t)->shared_v1[(e)/SHGNT_PER_PAGE_V1][(e)%SHGNT_PER_PAGE_V1])
+
+/* Operations providing a single interface agnostic to grant table version */
#define SHGNT_PER_PAGE_V2 (PAGE_SIZE / sizeof(grant_entry_v2_t))
#define shared_entry_v2(t, e) \
((t)->shared_v2[(e)/SHGNT_PER_PAGE_V2][(e)%SHGNT_PER_PAGE_V2])
+
+#define shared_entry_full_frame(gt, ref) \
+ ( get_gt_version(gt) == 1 ? shared_entry_v1((gt), (ref)).frame : \
+ shared_entry_v2((gt), (ref)).full_page.frame )
+#define set_shared_entry(gt, ref, val) \
+ ( get_gt_version(gt) == 1 ? (shared_entry_v1((gt), (ref)).frame = (val)) : \
+ (shared_entry_v2((gt), (ref)).full_page.frame = (val)) )
+#define status_addr(gt, ref, flags_addr) \
+ ( evaluate_nospec(get_gt_version(gt) == 1) ? (flags_addr) : &status_entry((gt), (ref)) )
+
#define STGNT_PER_PAGE (PAGE_SIZE / sizeof(grant_status_t))
#define status_entry(t, e) \
((t)->status[(e)/STGNT_PER_PAGE][(e)%STGNT_PER_PAGE])
+
+
static grant_entry_header_t *
shared_entry_header(struct grant_table *t, grant_ref_t ref)
{
- switch ( t->gt_version )
+ switch ( get_gt_version(t) )
{
case 1:
/* Returned values should be independent of speculative execution */
@@ -709,7 +724,7 @@ get_maptrack_handle(
/* Number of grant table entries. Caller must hold d's grant table lock. */
static unsigned int nr_grant_entries(struct grant_table *gt)
{
- switch ( gt->gt_version )
+ switch ( get_gt_version(gt) )
{
#define f2e(nr, ver) (((nr) << PAGE_SHIFT) / sizeof(grant_entry_v##ver##_t))
case 1:
@@ -1090,23 +1105,20 @@ map_grant_ref(
}
/* Make sure we do not access memory speculatively */
- status = evaluate_nospec(rgt->gt_version == 1) ? &shah->flags
- : &status_entry(rgt, ref);
+ status = status_addr(rgt, ref, &shah->flags);
if ( !act->pin ||
(!(op->flags & GNTMAP_readonly) &&
!(act->pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask))) )
{
- if ( (rc = _set_status(shah, status, rd, rgt->gt_version, act,
+ if ( (rc = _set_status(shah, status, rd, get_gt_version(rgt), act,
op->flags & GNTMAP_readonly, 1,
ld->domain_id)) != GNTST_okay )
goto act_release_out;
if ( !act->pin )
{
- unsigned long gfn = evaluate_nospec(rgt->gt_version == 1) ?
- shared_entry_v1(rgt, ref).frame :
- shared_entry_v2(rgt, ref).full_page.frame;
+ unsigned long gfn = shared_entry_full_frame(rgt, ref);
rc = get_paged_frame(gfn, &mfn, &pg,
op->flags & GNTMAP_readonly, rd);
@@ -1603,11 +1615,7 @@ unmap_common_complete(struct gnttab_unmap_common *op)
act = active_entry_acquire(rgt, op->ref);
sha = shared_entry_header(rgt, op->ref);
-
- if ( evaluate_nospec(rgt->gt_version == 1) )
- status = &sha->flags;
- else
- status = &status_entry(rgt, op->ref);
+ status = status_addr(rgt, op->ref, &sha->flags);
pg = !is_iomem_page(act->mfn) ? mfn_to_page(op->mfn) : NULL;
@@ -1930,7 +1938,7 @@ gnttab_grow_table(struct domain *d, unsigned int req_nr_frames)
}
/* Status pages - version 2 */
- if ( evaluate_nospec(gt->gt_version > 1) )
+ if ( evaluate_nospec(get_gt_version(gt) > 1) )
{
if ( gnttab_populate_status_frames(d, gt, req_nr_frames) )
goto shared_alloc_failed;
@@ -2105,7 +2113,7 @@ gnttab_setup_table(
}
if ( (op.nr_frames > nr_grant_frames(gt) ||
- ((gt->gt_version > 1) &&
+ ((get_gt_version(gt) > 1) &&
(grant_to_status_frames(op.nr_frames) > nr_status_frames(gt)))) &&
gnttab_grow_table(d, op.nr_frames) )
{
@@ -2267,6 +2275,7 @@ gnttab_transfer(
mfn_t mfn;
unsigned int max_bitsize;
struct active_grant_entry *act;
+ unsigned long frame;
if ( !opt_grant_transfer )
return -EOPNOTSUPP;
@@ -2354,7 +2363,7 @@ gnttab_transfer(
}
max_bitsize = domain_clamp_alloc_bitsize(
- e, e->grant_table->gt_version > 1 || paging_mode_translate(e)
+ e, get_gt_version(e->grant_table) > 1 || paging_mode_translate(e)
? BITS_PER_LONG + PAGE_SHIFT : 32 + PAGE_SHIFT);
if ( max_bitsize < BITS_PER_LONG + PAGE_SHIFT &&
(mfn_x(mfn) >> (max_bitsize - PAGE_SHIFT)) )
@@ -2452,22 +2461,12 @@ gnttab_transfer(
grant_read_lock(e->grant_table);
act = active_entry_acquire(e->grant_table, gop.ref);
- if ( evaluate_nospec(e->grant_table->gt_version == 1) )
- {
- grant_entry_v1_t *sha = &shared_entry_v1(e->grant_table, gop.ref);
+ frame = shared_entry_full_frame(e->grant_table, gop.ref);
+ rc = guest_physmap_add_page(e, _gfn(frame), mfn, 0);
- rc = guest_physmap_add_page(e, _gfn(sha->frame), mfn, 0);
- if ( !paging_mode_translate(e) )
- sha->frame = mfn_x(mfn);
- }
- else
- {
- grant_entry_v2_t *sha = &shared_entry_v2(e->grant_table, gop.ref);
+ if ( !paging_mode_translate(e) )
+ set_shared_entry(e->grant_table, gop.ref, mfn_x(mfn));
- rc = guest_physmap_add_page(e, _gfn(sha->full_page.frame), mfn, 0);
- if ( !paging_mode_translate(e) )
- sha->full_page.frame = mfn_x(mfn);
- }
smp_wmb();
shared_entry_header(e->grant_table, gop.ref)->flags |=
GTF_transfer_completed;
@@ -2512,16 +2511,15 @@ release_grant_for_copy(
act = active_entry_acquire(rgt, gref);
sha = shared_entry_header(rgt, gref);
mfn = act->mfn;
+ status = status_addr(rgt, gref, &sha->flags);
- if ( evaluate_nospec(rgt->gt_version == 1) )
+ if ( evaluate_nospec(get_gt_version(rgt) == 1) )
{
- status = &sha->flags;
td = rd;
trans_gref = gref;
}
else
{
- status = &status_entry(rgt, gref);
td = (act->src_domid == rd->domain_id)
? rd : knownalive_domain_from_domid(act->src_domid);
trans_gref = act->trans_gref;
@@ -2573,7 +2571,6 @@ acquire_grant_for_copy(
struct active_grant_entry *act;
grant_status_t *status;
uint32_t old_pin;
- domid_t trans_domid;
grant_ref_t trans_gref;
struct domain *td;
mfn_t grant_mfn;
@@ -2597,6 +2594,7 @@ acquire_grant_for_copy(
/* This call also ensures the above check cannot be passed speculatively */
shah = shared_entry_header(rgt, gref);
act = active_entry_acquire(rgt, gref);
+ old_pin = act->pin;
/* If already pinned, check the active domid and avoid refcnt overflow. */
if ( act->pin &&
@@ -2610,7 +2608,7 @@ acquire_grant_for_copy(
goto unlock_out;
}
- if ( evaluate_nospec(rgt->gt_version == 1) )
+ if ( evaluate_nospec(get_gt_version(rgt) == 1) )
{
sha2 = NULL;
status = &shah->flags;
@@ -2621,9 +2619,10 @@ acquire_grant_for_copy(
status = &status_entry(rgt, gref);
}
- old_pin = act->pin;
if ( sha2 && (shah->flags & GTF_type_mask) == GTF_transitive )
{
+ domid_t trans_domid;
+
if ( (!old_pin || (!readonly &&
!(old_pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)))) &&
(rc = _set_status_v2(shah, status, rd, act, readonly, 0,
@@ -2752,7 +2751,7 @@ acquire_grant_for_copy(
else if ( !old_pin ||
(!readonly && !(old_pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask))) )
{
- if ( (rc = _set_status(shah, status, rd, rgt->gt_version, act,
+ if ( (rc = _set_status(shah, status, rd, get_gt_version(rgt), act,
readonly, 0, ldom)) != GNTST_okay )
goto unlock_out;
@@ -3406,7 +3405,7 @@ gnttab_get_version(XEN_GUEST_HANDLE_PARAM(gnttab_get_version_t) uop)
return rc;
}
- op.version = d->grant_table->gt_version;
+ op.version = get_gt_version(d->grant_table);
rcu_unlock_domain(d);
@@ -3464,7 +3463,7 @@ swap_grant_ref(grant_ref_t ref_a, grant_ref_t ref_b)
goto out;
}
- if ( evaluate_nospec(gt->gt_version == 1) )
+ if ( evaluate_nospec(get_gt_version(gt) == 1) )
{
grant_entry_v1_t shared;
@@ -3878,10 +3877,7 @@ int gnttab_release_mappings(struct domain *d)
act = active_entry_acquire(rgt, ref);
sha = shared_entry_header(rgt, ref);
- if ( rgt->gt_version == 1 )
- status = &sha->flags;
- else
- status = &status_entry(rgt, ref);
+ status = status_addr(rgt, ref, &sha->flags);
pg = !is_iomem_page(act->mfn) ? mfn_to_page(act->mfn) : NULL;
@@ -4047,11 +4043,11 @@ int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref,
grant_read_lock(gt);
- if ( gt->gt_version < 1 )
+ if ( get_gt_version(gt) < 1 )
rc = -EINVAL;
else if ( ref >= nr_grant_entries(gt) )
rc = -ENOENT;
- else if ( evaluate_nospec(gt->gt_version == 1) )
+ else if ( evaluate_nospec(get_gt_version(gt) == 1) )
{
const grant_entry_v1_t *sha1 = &shared_entry_v1(gt, ref);
@@ -4072,12 +4068,7 @@ int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref,
if ( !rc && (flags & GTF_type_mask) != GTF_permit_access )
rc = -ENXIO;
else if ( !rc && status )
- {
- if ( evaluate_nospec(gt->gt_version == 1) )
- *status = flags;
- else
- *status = status_entry(gt, ref);
- }
+ *status = *status_addr(gt, ref, &flags);
grant_read_unlock(gt);
@@ -4131,7 +4122,7 @@ static int gnttab_get_shared_frame_mfn(struct domain *d,
{
const struct grant_table *gt = d->grant_table;
- ASSERT(gt->gt_version != 0);
+ ASSERT(get_gt_version(gt) != 0);
if ( idx >= nr_grant_frames(gt) )
{
@@ -4204,7 +4195,7 @@ int gnttab_acquire_resource(
break;
case XENMEM_resource_grant_table_id_status:
- if ( gt->gt_version != 2 )
+ if ( get_gt_version(gt) != 2 )
break;
/* Check that void ** is a suitable representation for gt->status. */
@@ -4256,7 +4247,7 @@ int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn, mfn_t *mfn)
grant_write_lock(gt);
- if ( evaluate_nospec(gt->gt_version == 2) && (idx & XENMAPIDX_grant_table_status) )
+ if ( evaluate_nospec(get_gt_version(gt) == 2) && (idx & XENMAPIDX_grant_table_status) )
{
idx &= ~XENMAPIDX_grant_table_status;
status = true;
@@ -4299,7 +4290,7 @@ static void gnttab_usage_print(struct domain *rd)
printk("grant-table for remote d%d (v%u)\n"
" %u frames (%u max), %u maptrack frames (%u max)\n",
- rd->domain_id, gt->gt_version,
+ rd->domain_id, get_gt_version(gt),
nr_grant_frames(gt), gt->max_grant_frames,
nr_maptrack_frames(gt), gt->max_maptrack_frames);
@@ -4319,17 +4310,8 @@ static void gnttab_usage_print(struct domain *rd)
}
sha = shared_entry_header(gt, ref);
-
- if ( gt->gt_version == 1 )
- {
- status = sha->flags;
- frame = shared_entry_v1(gt, ref).frame;
- }
- else
- {
- frame = shared_entry_v2(gt, ref).full_page.frame;
- status = status_entry(gt, ref);
- }
+ frame = shared_entry_full_frame(gt, ref);
+ status = *status_addr(gt, ref, &sha->flags);
first = 0;
--
2.34.1On 10.01.2025 14:37, Daniel P. Smith wrote:
> @@ -310,16 +311,30 @@ nr_maptrack_frames(struct grant_table *t)
> #define SHGNT_PER_PAGE_V1 (PAGE_SIZE / sizeof(grant_entry_v1_t))
> #define shared_entry_v1(t, e) \
> ((t)->shared_v1[(e)/SHGNT_PER_PAGE_V1][(e)%SHGNT_PER_PAGE_V1])
> +
> +/* Operations providing a single interface agnostic to grant table version */
> #define SHGNT_PER_PAGE_V2 (PAGE_SIZE / sizeof(grant_entry_v2_t))
> #define shared_entry_v2(t, e) \
> ((t)->shared_v2[(e)/SHGNT_PER_PAGE_V2][(e)%SHGNT_PER_PAGE_V2])
The comment looks like it was misplaced, and rather wanted to go ahead of ...
> +
> +#define shared_entry_full_frame(gt, ref) \
> + ( get_gt_version(gt) == 1 ? shared_entry_v1((gt), (ref)).frame : \
> + shared_entry_v2((gt), (ref)).full_page.frame )
> +#define set_shared_entry(gt, ref, val) \
> + ( get_gt_version(gt) == 1 ? (shared_entry_v1((gt), (ref)).frame = (val)) : \
> + (shared_entry_v2((gt), (ref)).full_page.frame = (val)) )
> +#define status_addr(gt, ref, flags_addr) \
> + ( evaluate_nospec(get_gt_version(gt) == 1) ? (flags_addr) : &status_entry((gt), (ref)) )
... this set of new #define-s?
Also a nit: There shouldn't be blanks immediately inside the opening parentheses
here. For readability you further want to omit parentheses around parameters
which are passed on unaltered (i.e. not becoming part of an expression). Plus
some of the lines are pretty obviously too long.
As to the single use of evaluate_nospec(): That looks odd here; on the surface
one would expect all three constructs to use it, or none of them. Deviations
from that likely require a comment, or at least some explanation in the
description.
> +
> #define STGNT_PER_PAGE (PAGE_SIZE / sizeof(grant_status_t))
> #define status_entry(t, e) \
> ((t)->status[(e)/STGNT_PER_PAGE][(e)%STGNT_PER_PAGE])
> +
> +
No double blank lines please.
> @@ -1090,23 +1105,20 @@ map_grant_ref(
> }
>
> /* Make sure we do not access memory speculatively */
> - status = evaluate_nospec(rgt->gt_version == 1) ? &shah->flags
> - : &status_entry(rgt, ref);
> + status = status_addr(rgt, ref, &shah->flags);
I was already wondering at the macro definition: How can it be that a
version-abstracting macro is passed something that's clearly version
1 only? That is, consider the opposite of the goal of this series,
someone wanting to allow building a gnttab-v2-only hypervisor. In that
case grant_entry_header_t would ideally not be defined at all (as a
struct; there may be a placeholder type). It would then be impossible
to de-reference the pointer. Hence I think that de-referencing ought to
live inside the macro.
> @@ -1930,7 +1938,7 @@ gnttab_grow_table(struct domain *d, unsigned int req_nr_frames)
> }
>
> /* Status pages - version 2 */
> - if ( evaluate_nospec(gt->gt_version > 1) )
> + if ( evaluate_nospec(get_gt_version(gt) > 1) )
At this example: Assuming in the next patch get_gt_version() can end
up expanding to constant 1, the evaluate_nospec() would still be there,
for no good reason. I expect that wants further abstracting.
> @@ -2267,6 +2275,7 @@ gnttab_transfer(
> mfn_t mfn;
> unsigned int max_bitsize;
> struct active_grant_entry *act;
> + unsigned long frame;
Just going from indentation, ...
> @@ -2354,7 +2363,7 @@ gnttab_transfer(
> }
>
> max_bitsize = domain_clamp_alloc_bitsize(
> - e, e->grant_table->gt_version > 1 || paging_mode_translate(e)
> + e, get_gt_version(e->grant_table) > 1 || paging_mode_translate(e)
> ? BITS_PER_LONG + PAGE_SHIFT : 32 + PAGE_SHIFT);
> if ( max_bitsize < BITS_PER_LONG + PAGE_SHIFT &&
> (mfn_x(mfn) >> (max_bitsize - PAGE_SHIFT)) )
> @@ -2452,22 +2461,12 @@ gnttab_transfer(
> grant_read_lock(e->grant_table);
> act = active_entry_acquire(e->grant_table, gop.ref);
>
> - if ( evaluate_nospec(e->grant_table->gt_version == 1) )
> - {
> - grant_entry_v1_t *sha = &shared_entry_v1(e->grant_table, gop.ref);
> + frame = shared_entry_full_frame(e->grant_table, gop.ref);
> + rc = guest_physmap_add_page(e, _gfn(frame), mfn, 0);
>
> - rc = guest_physmap_add_page(e, _gfn(sha->frame), mfn, 0);
> - if ( !paging_mode_translate(e) )
> - sha->frame = mfn_x(mfn);
> - }
> - else
> - {
> - grant_entry_v2_t *sha = &shared_entry_v2(e->grant_table, gop.ref);
> + if ( !paging_mode_translate(e) )
> + set_shared_entry(e->grant_table, gop.ref, mfn_x(mfn));
>
> - rc = guest_physmap_add_page(e, _gfn(sha->full_page.frame), mfn, 0);
> - if ( !paging_mode_translate(e) )
> - sha->full_page.frame = mfn_x(mfn);
> - }
> smp_wmb();
> shared_entry_header(e->grant_table, gop.ref)->flags |=
> GTF_transfer_completed;
... the variable looks to want declaring in a more narrow scope.
Also, related to an earlier comment: Where did the evaluate_nospec() go?
> @@ -2512,16 +2511,15 @@ release_grant_for_copy(
> act = active_entry_acquire(rgt, gref);
> sha = shared_entry_header(rgt, gref);
> mfn = act->mfn;
> + status = status_addr(rgt, gref, &sha->flags);
>
> - if ( evaluate_nospec(rgt->gt_version == 1) )
> + if ( evaluate_nospec(get_gt_version(rgt) == 1) )
> {
> - status = &sha->flags;
> td = rd;
> trans_gref = gref;
> }
> else
> {
> - status = &status_entry(rgt, gref);
> td = (act->src_domid == rd->domain_id)
> ? rd : knownalive_domain_from_domid(act->src_domid);
> trans_gref = act->trans_gref;
Same here - you're moving code across that speculation barrier.
Anything like this - assuming it is deliberate in the first place -
needs justifying.
> @@ -2573,7 +2571,6 @@ acquire_grant_for_copy(
> struct active_grant_entry *act;
> grant_status_t *status;
> uint32_t old_pin;
> - domid_t trans_domid;
> grant_ref_t trans_gref;
> struct domain *td;
> mfn_t grant_mfn;
I fear I understand neither the movement of this variable, ...
> @@ -2597,6 +2594,7 @@ acquire_grant_for_copy(
> /* This call also ensures the above check cannot be passed speculatively */
> shah = shared_entry_header(rgt, gref);
> act = active_entry_acquire(rgt, gref);
> + old_pin = act->pin;
... nor that of this assignment ...
> @@ -2610,7 +2608,7 @@ acquire_grant_for_copy(
> goto unlock_out;
> }
>
> - if ( evaluate_nospec(rgt->gt_version == 1) )
> + if ( evaluate_nospec(get_gt_version(rgt) == 1) )
> {
> sha2 = NULL;
> status = &shah->flags;
> @@ -2621,9 +2619,10 @@ acquire_grant_for_copy(
> status = &status_entry(rgt, gref);
> }
>
> - old_pin = act->pin;
... (from here).
> if ( sha2 && (shah->flags & GTF_type_mask) == GTF_transitive )
> {
> + domid_t trans_domid;
This makes me guess you want to narrow the scope of the variable, yet
then: How's that related to the subject of this patch?
> @@ -3878,10 +3877,7 @@ int gnttab_release_mappings(struct domain *d)
>
> act = active_entry_acquire(rgt, ref);
> sha = shared_entry_header(rgt, ref);
> - if ( rgt->gt_version == 1 )
> - status = &sha->flags;
> - else
> - status = &status_entry(rgt, ref);
> + status = status_addr(rgt, ref, &sha->flags);
Again relating to earlier comments, yet this time the other way around:
Suddenly we're gaining evaluate_nospec() here, for no apparent reason.
> @@ -4256,7 +4247,7 @@ int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn, mfn_t *mfn)
>
> grant_write_lock(gt);
>
> - if ( evaluate_nospec(gt->gt_version == 2) && (idx & XENMAPIDX_grant_table_status) )
> + if ( evaluate_nospec(get_gt_version(gt) == 2) && (idx & XENMAPIDX_grant_table_status) )
I realize the line was already too long, but since you touch it you also
want to wrap it.
Jan
© 2016 - 2025 Red Hat, Inc.