Magenheimer, Dan (HP Labs Fort Collins)
2005-Jun-10 21:08 UTC
[Xen-devel] Review needed for "commonisation" of common/grant_table.c
common/grant_table.c is currently not built for Xen/ia64 and has
never been "common-ized". I''m working on identifying the
x86-specific portions. I have a version compiled and
minimally working on Xen/ia64 now but there''s a lot of x86
code that should probably be moved to macros or to separate
arch-specific files and I would like to get
some review from a grant_table.c expert to ensure I haven''t
missed (or misunderstood) anything.
Attached is a patch for grant_table.c. It is NOT intended
to be applied, but to make the changes easier to review. Apply
it to your local version of grant_table.c (rev 1.48, yesterday),
then look at each __ia64__. Comments/feedback would be
greatly appreciated.
Thanks,
Dan
===== common/grant_table.c 1.48 vs edited ====--- 1.48/xen/common/grant_table.c
Thu Jun 9 09:25:28 2005
+++ edited/common/grant_table.c Fri Jun 10 14:32:00 2005
@@ -30,6 +30,19 @@
#include <xen/sched.h>
#include <xen/shadow.h>
#include <xen/mm.h>
+#ifdef __ia64__
+#define __addr_ok(a) 1 // FIXME-ia64: a variant of access_ok??
+// FIXME-ia64: need to implement real cmpxchg_user on ia64
+#define cmpxchg_user(_p,_o,_n) ((*_p == _o) ? ((*_p = _n), 0) : ((_o *_p), 0))
+// FIXME-ia64: the following are meaningless on ia64? move to some
header file
+#define put_page(x) do { } while (0)
+#define put_page_type(x) do { } while (0)
+// FIXME-ia64: these belong in an asm/grant_table.h... PAGE_SIZE
different
+#undef ORDER_GRANT_FRAMES
+//#undef NUM_GRANT_FRAMES
+#define ORDER_GRANT_FRAMES 0
+//#define NUM_GRANT_FRAMES (1U << ORDER_GRANT_FRAMES)
+#endif
#define PIN_FAIL(_lbl, _rc, _f, _a...) \
do { \
@@ -162,6 +175,9 @@
frame = __gpfn_to_mfn_foreign(granting_d, sha->frame);
+#ifdef __ia64__
+// FIXME-ia64: any error checking need to be done here?
+#else
if ( unlikely(!pfn_valid(frame)) ||
unlikely(!((dev_hst_ro_flags & GNTMAP_readonly) ?
get_page(&frame_table[frame], granting_d) :
@@ -173,6 +189,7 @@
PIN_FAIL(unlock_out, GNTST_general_error,
"Could not pin the granted frame (%lx)!\n",
frame);
}
+#endif
if ( dev_hst_ro_flags & GNTMAP_device_map )
act->pin += (dev_hst_ro_flags & GNTMAP_readonly) ?
@@ -226,6 +243,9 @@
sflags = prev_sflags;
}
+#ifdef __ia64__
+// FIXME-ia64: any error checking need to be done here?
+#else
if ( unlikely(!get_page_type(&frame_table[frame],
PGT_writable_page)) )
{
@@ -233,6 +253,7 @@
PIN_FAIL(unlock_out, GNTST_general_error,
"Attempt to write-pin a unwritable
page.\n");
}
+#endif
}
if ( dev_hst_ro_flags & GNTMAP_device_map )
@@ -253,6 +274,9 @@
spin_unlock(&granting_d->grant_table->lock);
+#ifdef __ia64__
+// FIXME-ia64: any error checking need to be done here?
+#else
if ( (host_virt_addr != 0) && (dev_hst_ro_flags &
GNTMAP_host_map)
)
{
/* Write update into the pagetable. */
@@ -298,6 +322,7 @@
}
}
+#endif
*pframe = frame;
return rc;
@@ -444,10 +469,14 @@
if ( __gnttab_map_grant_ref(&uop[i], &va) == 0 )
flush++;
+#ifdef __ia64__
+// FIXME-ia64: probably need to do something here to avoid stale
mappings?
+#else
if ( flush == 1 )
flush_tlb_one_mask(current->domain->cpumask, va);
else if ( flush != 0 )
flush_tlb_mask(current->domain->cpumask);
+#endif
return 0;
}
@@ -542,6 +571,9 @@
/* Frame is now unmapped for device access. */
}
+#ifdef __ia64__
+// FIXME-ia64: any error checking need to be done here?
+#else
if ( (virt != 0) &&
(flags & GNTMAP_host_map) &&
((act->pin & (GNTPIN_hstw_mask | GNTPIN_hstr_mask)) > 0))
@@ -596,6 +628,7 @@
rc = 0;
*va = virt;
}
+#endif
if ( (map->ref_and_flags & (GNTMAP_device_map|GNTMAP_host_map)) =0)
{
@@ -603,10 +636,15 @@
put_maptrack_handle(ld->grant_table, handle);
}
+#ifdef __ia64__
+// FIXME-ia64: any error checking need to be done here? I think not
and then
+// this can probably be macro-ized into nothingness
+#else
/* If just unmapped a writable mapping, mark as dirtied */
if ( unlikely(shadow_mode_log_dirty(rd)) &&
!( flags & GNTMAP_readonly ) )
mark_dirty(rd, frame);
+#endif
/* If the last writable mapping has been removed, put_page_type */
if ( ( (act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask) ) == 0)
&&
@@ -640,10 +678,14 @@
if ( __gnttab_unmap_grant_ref(&uop[i], &va) == 0 )
flush++;
+#ifdef __ia64__
+// FIXME-ia64: probably need to do something here to avoid stale
mappings?
+#else
if ( flush == 1 )
flush_tlb_one_mask(current->domain->cpumask, va);
else if ( flush != 0 )
flush_tlb_mask(current->domain->cpumask);
+#endif
return 0;
}
@@ -1050,6 +1092,9 @@
spin_lock(&rd->grant_table->lock);
+#ifdef __ia64__
+// FIXME-ia64: any error checking need to be done here?
+#else
pfn = sha->frame;
if ( unlikely(pfn >= max_page ) )
@@ -1064,6 +1109,7 @@
if (shadow_mode_translate(ld))
__phys_to_machine_mapping[pfn] = frame;
}
+#endif
sha->frame = __mfn_to_gpfn(rd, frame);
sha->domid = rd->domain_id;
wmb();
@@ -1109,6 +1155,9 @@
goto no_mem;
memset(t->shared, 0, NR_GRANT_FRAMES * PAGE_SIZE);
+#ifdef __ia64__
+// I don''t think there''s anything to do here on ia64?...
+#else
for ( i = 0; i < NR_GRANT_FRAMES; i++ )
{
SHARE_PFN_WITH_DOMAIN(
@@ -1116,6 +1165,7 @@
machine_to_phys_mapping[(virt_to_phys(t->shared) >>
PAGE_SHIFT)
+ i] INVALID_M2P_ENTRY;
}
+#endif
/* Okay, install the structure. */
wmb(); /* avoid races with lock-free access to d->grant_table */
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Christopher Clark
2005-Jun-10 21:49 UTC
Re: [Xen-devel] Review needed for "commonisation" of common/grant_table.c
Dan Can you send me your patched grant_table.c ? I don''t have the tools or a source tree to hand right now to apply your patch, but I''d like to take a look. What do these comments mean? -: // FIXME-ia64: any error checking need to be done here? Why doesn''t ia64 use put_page and put_page_type? Isn''t there a frame table with reference counts on ia64? Christopher On 6/10/05, Magenheimer, Dan (HP Labs Fort Collins) <dan.magenheimer@hp.com> wrote:> common/grant_table.c is currently not built for Xen/ia64 and has > never been "common-ized". I''m working on identifying the > x86-specific portions. I have a version compiled and > minimally working on Xen/ia64 now but there''s a lot of x86 > code that should probably be moved to macros or to separate > arch-specific files and I would like to get > some review from a grant_table.c expert to ensure I haven''t > missed (or misunderstood) anything. > > Attached is a patch for grant_table.c. It is NOT intended > to be applied, but to make the changes easier to review. Apply > it to your local version of grant_table.c (rev 1.48, yesterday), > then look at each __ia64__. Comments/feedback would be > greatly appreciated. > > Thanks, > Dan > > ===== common/grant_table.c 1.48 vs edited ====> --- 1.48/xen/common/grant_table.c Thu Jun 9 09:25:28 2005 > +++ edited/common/grant_table.c Fri Jun 10 14:32:00 2005 > @@ -30,6 +30,19 @@ > #include <xen/sched.h> > #include <xen/shadow.h> > #include <xen/mm.h> > +#ifdef __ia64__ > +#define __addr_ok(a) 1 // FIXME-ia64: a variant of access_ok?? > +// FIXME-ia64: need to implement real cmpxchg_user on ia64 > +#define cmpxchg_user(_p,_o,_n) ((*_p == _o) ? ((*_p = _n), 0) : ((_o > *_p), 0)) > +// FIXME-ia64: the following are meaningless on ia64? move to some > header file > +#define put_page(x) do { } while (0) > +#define put_page_type(x) do { } while (0) > +// FIXME-ia64: these belong in an asm/grant_table.h... PAGE_SIZE > different > +#undef ORDER_GRANT_FRAMES > +//#undef NUM_GRANT_FRAMES > +#define ORDER_GRANT_FRAMES 0 > +//#define NUM_GRANT_FRAMES (1U << ORDER_GRANT_FRAMES) > +#endif > > #define PIN_FAIL(_lbl, _rc, _f, _a...) \ > do { \ > @@ -162,6 +175,9 @@ > > frame = __gpfn_to_mfn_foreign(granting_d, sha->frame); > > +#ifdef __ia64__ > +// FIXME-ia64: any error checking need to be done here? > +#else > if ( unlikely(!pfn_valid(frame)) || > unlikely(!((dev_hst_ro_flags & GNTMAP_readonly) ? > get_page(&frame_table[frame], granting_d) : > @@ -173,6 +189,7 @@ > PIN_FAIL(unlock_out, GNTST_general_error, > "Could not pin the granted frame (%lx)!\n", > frame); > } > +#endif > > if ( dev_hst_ro_flags & GNTMAP_device_map ) > act->pin += (dev_hst_ro_flags & GNTMAP_readonly) ? > @@ -226,6 +243,9 @@ > sflags = prev_sflags; > } > > +#ifdef __ia64__ > +// FIXME-ia64: any error checking need to be done here? > +#else > if ( unlikely(!get_page_type(&frame_table[frame], > PGT_writable_page)) ) > { > @@ -233,6 +253,7 @@ > PIN_FAIL(unlock_out, GNTST_general_error, > "Attempt to write-pin a unwritable page.\n"); > } > +#endif > } > > if ( dev_hst_ro_flags & GNTMAP_device_map ) > @@ -253,6 +274,9 @@ > > spin_unlock(&granting_d->grant_table->lock); > > +#ifdef __ia64__ > +// FIXME-ia64: any error checking need to be done here? > +#else > if ( (host_virt_addr != 0) && (dev_hst_ro_flags & GNTMAP_host_map) > ) > { > /* Write update into the pagetable. */ > @@ -298,6 +322,7 @@ > } > > } > +#endif > > *pframe = frame; > return rc; > @@ -444,10 +469,14 @@ > if ( __gnttab_map_grant_ref(&uop[i], &va) == 0 ) > flush++; > > +#ifdef __ia64__ > +// FIXME-ia64: probably need to do something here to avoid stale > mappings? > +#else > if ( flush == 1 ) > flush_tlb_one_mask(current->domain->cpumask, va); > else if ( flush != 0 ) > flush_tlb_mask(current->domain->cpumask); > +#endif > > return 0; > } > @@ -542,6 +571,9 @@ > /* Frame is now unmapped for device access. */ > } > > +#ifdef __ia64__ > +// FIXME-ia64: any error checking need to be done here? > +#else > if ( (virt != 0) && > (flags & GNTMAP_host_map) && > ((act->pin & (GNTPIN_hstw_mask | GNTPIN_hstr_mask)) > 0)) > @@ -596,6 +628,7 @@ > rc = 0; > *va = virt; > } > +#endif > > if ( (map->ref_and_flags & (GNTMAP_device_map|GNTMAP_host_map)) => 0) > { > @@ -603,10 +636,15 @@ > put_maptrack_handle(ld->grant_table, handle); > } > > +#ifdef __ia64__ > +// FIXME-ia64: any error checking need to be done here? I think not > and then > +// this can probably be macro-ized into nothingness > +#else > /* If just unmapped a writable mapping, mark as dirtied */ > if ( unlikely(shadow_mode_log_dirty(rd)) && > !( flags & GNTMAP_readonly ) ) > mark_dirty(rd, frame); > +#endif > > /* If the last writable mapping has been removed, put_page_type */ > if ( ( (act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask) ) == 0) && > @@ -640,10 +678,14 @@ > if ( __gnttab_unmap_grant_ref(&uop[i], &va) == 0 ) > flush++; > > +#ifdef __ia64__ > +// FIXME-ia64: probably need to do something here to avoid stale > mappings? > +#else > if ( flush == 1 ) > flush_tlb_one_mask(current->domain->cpumask, va); > else if ( flush != 0 ) > flush_tlb_mask(current->domain->cpumask); > +#endif > > return 0; > } > @@ -1050,6 +1092,9 @@ > > spin_lock(&rd->grant_table->lock); > > +#ifdef __ia64__ > +// FIXME-ia64: any error checking need to be done here? > +#else > pfn = sha->frame; > > if ( unlikely(pfn >= max_page ) ) > @@ -1064,6 +1109,7 @@ > if (shadow_mode_translate(ld)) > __phys_to_machine_mapping[pfn] = frame; > } > +#endif > sha->frame = __mfn_to_gpfn(rd, frame); > sha->domid = rd->domain_id; > wmb(); > @@ -1109,6 +1155,9 @@ > goto no_mem; > memset(t->shared, 0, NR_GRANT_FRAMES * PAGE_SIZE); > > +#ifdef __ia64__ > +// I don''t think there''s anything to do here on ia64?... > +#else > for ( i = 0; i < NR_GRANT_FRAMES; i++ ) > { > SHARE_PFN_WITH_DOMAIN( > @@ -1116,6 +1165,7 @@ > machine_to_phys_mapping[(virt_to_phys(t->shared) >> PAGE_SHIFT) > + i] > INVALID_M2P_ENTRY; > } > +#endif > > /* Okay, install the structure. */ > wmb(); /* avoid races with lock-free access to d->grant_table */ > > _______________________________________________ > 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
Magenheimer, Dan (HP Labs Fort Collins)
2005-Jun-10 21:56 UTC
RE: [Xen-devel] Review needed for "commonisation" of common/grant_table.c
> Can you send me your patched grant_table.c ? I don''t have the tools or > a source tree to hand right now to apply your patch, but I''d like to > take a look.Will send as separate file to avoid cluttering the whole list.> What do these comments mean? -: > // FIXME-ia64: any error checking need to be done here?It means that on x86, there are a number of error conditions due to the way page tables are handled, but none (or perhaps different error conditions) apply on ia64. The checks for the page table issues are x86 specific and would need to be macro-ized somehow for the code to be portable.> Why doesn''t ia64 use put_page and put_page_type? Isn''t there a frame > table with reference counts on ia64?There''s a frame table but no reference counts because there are no "special" (e.g. L1 page table) frames (at least not in the current implementation). No get_page either (but that is only used in code which is commented out for other x86-specific reasons). Dan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel