Re: [Xen-devel] Review needed for "commonisation" of common/grant_table.c

[Christopher Clark <christopher.w.clark@xxxxxxxxx>]

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@xxxxxx> 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@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel
>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel