Re: [XenPPC] Re: [Xen-devel] [PATCH] [GNTTAB] expandable grant table

2007-02-26 Thread Keir Fraser
On 26/2/07 23:31, Hollis Blanchard [EMAIL PROTECTED] wrote:

 
 Hi Yamahata-san, thanks very much for your patch!
 
 I'm confused about one thing though: why do we need to take a lock to
 read d-grant_table-nr_grant_frames? It's a simple integer, so no lock
 is required or useful.

I haven't got the ppc code immediately to hand but the x86 code will
dynamically grow the grant table based on requests made via the
add_to_physmap hypercall, hence it needs to take the lock. I see ia64 takes
it also even though it only reads from nr_grant_frames (it won;t dynamically
expand via the add_to_physmap hypercall). That's possibly overkill although
there is some concern over reading nr_grant_frames versus looking up a
grant-table page that appears within the range [0, nr_grant_frames-1] which
taking the lock trivially sidesteps. If ia64 didn't take the lock it might
be possible to see an increased value of nr_grant_frames that the expand
function hadn't yet fully installed the new grant-table pages for yet.

 -- Keir



___
Xen-ppc-devel mailing list
Xen-ppc-devel@lists.xensource.com
http://lists.xensource.com/xen-ppc-devel


Re: [XenPPC] Re: [Xen-devel] [PATCH] [GNTTAB] expandable grant table

2007-02-26 Thread Hollis Blanchard
On Mon, 2007-02-26 at 23:39 +, Keir Fraser wrote:
 On 26/2/07 23:31, Hollis Blanchard [EMAIL PROTECTED] wrote:
 
  
  Hi Yamahata-san, thanks very much for your patch!
  
  I'm confused about one thing though: why do we need to take a lock to
  read d-grant_table-nr_grant_frames? It's a simple integer, so no lock
  is required or useful.
 
 I haven't got the ppc code immediately to hand but the x86 code will
 dynamically grow the grant table based on requests made via the
 add_to_physmap hypercall, hence it needs to take the lock.

OK, I see x86's XENMAPSPACE_grant_table handler; the locking makes sense
there.

 I see ia64 takes
 it also even though it only reads from nr_grant_frames (it won;t dynamically
 expand via the add_to_physmap hypercall). That's possibly overkill although
 there is some concern over reading nr_grant_frames versus looking up a
 grant-table page that appears within the range [0, nr_grant_frames-1] which
 taking the lock trivially sidesteps. If ia64 didn't take the lock it might
 be possible to see an increased value of nr_grant_frames that the expand
 function hadn't yet fully installed the new grant-table pages for yet.

I don't believe that's a concern, since updating
grant_table-nr_grant_frames is the very last step in
gnttab_grow_table(), and it will only grow.

The comments about locking around nr_grant_frames() and
nr_grant_entries() are confusing at best, since you don't need a lock to
read an integer...

-- 
Hollis Blanchard
IBM Linux Technology Center


___
Xen-ppc-devel mailing list
Xen-ppc-devel@lists.xensource.com
http://lists.xensource.com/xen-ppc-devel


Re: [XenPPC] Re: [Xen-devel] [PATCH] [GNTTAB] expandable grant table

2007-02-26 Thread Keir Fraser



On 27/2/07 00:05, Hollis Blanchard [EMAIL PROTECTED] wrote:

 I don't believe that's a concern, since updating
 grant_table-nr_grant_frames is the very last step in
 gnttab_grow_table(), and it will only grow.

If there's a memory barrier before the update of nr_grant_frames, explicitly
or implied, then removing the locking from add_to_physmap is fine. Otherwise
not: reading an integer is atomic, but using that as a flag to indicate
presence of other state updated under the same lock is just a little bit
suspect (but might be okay).

 -- Keir



___
Xen-ppc-devel mailing list
Xen-ppc-devel@lists.xensource.com
http://lists.xensource.com/xen-ppc-devel


[XenPPC] Re: [Xen-devel] [PATCH] [GNTTAB] expandable grant table

2007-02-19 Thread Isaku Yamahata

Attached the updated patches. Please find them

- 13985_4fa9b86d41b8_grow_granttable_acm.patch
acm part. I did only compile test.

- 13986_cca098fd122c_grow_granttable_ia64.patch
ia64 part.

- 13987_f5f232aef7bd_grow_granttable_powerpc.patch
powerpc part. This patch prohibits growing grant table.
I didn't even compile test.
It needs review by xen/powerpc developers and they may
have their own opinions.

Thanks,

On Thu, Feb 15, 2007 at 02:39:37PM +, Keir Fraser wrote:
 
 Hi Isaku,
 
 The dynamic grant table patch is now in xen-unstable (c/s
 13952:4bd0ea9c499). Can you update and resend these patches?
 
  Thanks,
  Keir
 
 
 On 6/1/07 06:48, Isaku Yamahata [EMAIL PROTECTED] wrote:
 
  On Fri, Jan 05, 2007 at 01:43:10PM +, Keir Fraser wrote:
  This one is waiting for final review before being checked in. Some people
  want the functionality this enables (3 VIFs per guest) so I'm posting the
  patch in advance of applying it to xen-unstable.
  
  I've been working on the same issue so that I have some patches.
  I attached a part of them. Although they should be adjusted to
  your patch which is to be commited, I wanted to send these out before
  checking in.
  If necessary, I'm willing to update them.
  
  - acm code uses NR_GRANT_ENTRIES.
The attached patch is only compile-tested.
  
  - IA64 code needs a slight domain initialization adjustment.
  
  - PPC code depends on the fact that shared grant table is machine 
  continguous.
I'm not familiar with PPC code so that I'm not sure how to solve it.
If arch specific code can override maximal grant table size,
Xen/PPC can prohibit growing grant table.
After solving the issue, they can allow to grow grant table.
 
 
 ___
 Xen-devel mailing list
 [EMAIL PROTECTED]
 http://lists.xensource.com/xen-devel
 

-- 
yamahata
# HG changeset patch
# User [EMAIL PROTECTED]
# Date 1171867951 -32400
# Node ID 4fa9b86d41b8faeeb4338dabe62956ecb359b475
# Parent  b5fc88aad1b0eb35d12e503982c70fdc27f0544a
acm catch up for expandable grant table.
only compile-test is done.
PATCHNAME: grow_granttable_acm

Signed-off-by: Isaku Yamahata [EMAIL PROTECTED]

diff -r b5fc88aad1b0 -r 4fa9b86d41b8 xen/acm/acm_simple_type_enforcement_hooks.c
--- a/xen/acm/acm_simple_type_enforcement_hooks.c	Sun Feb 18 15:29:40 2007 +
+++ b/xen/acm/acm_simple_type_enforcement_hooks.c	Mon Feb 19 15:52:31 2007 +0900
@@ -234,18 +234,16 @@ ste_init_state(struct acm_ste_policy_buf
 }
 } 
 /* b) check for grant table conflicts on shared pages */
-if ((*pd)-grant_table-shared == NULL) {
-printkd(%s: Grant ... sharing for domain %x not setup!\n, __func__, (*pd)-domain_id);
-continue;
-}
-for ( i = 0; i  NR_GRANT_ENTRIES; i++ ) {
-sha_copy =  (*pd)-grant_table-shared[i];
+spin_lock((*pd)-grant_table-lock);
+for ( i = 0; i  nr_grant_entries((*pd)-grant_table); i++ ) {
+sha_copy = shared_entry((*pd)-grant_table, i);
 if ( sha_copy.flags ) {
 printkd(%s: grant dom (%hu) SHARED (%d) flags:(%hx) dom:(%hu) frame:(%lx)\n,
 __func__, (*pd)-domain_id, i, sha_copy.flags, sha_copy.domid, 
 (unsigned long)sha_copy.frame);
 rdomid = sha_copy.domid;
 if ((rdom = get_domain_by_id(rdomid)) == NULL) {
+spin_unlock((*pd)-grant_table-lock);
 printkd(%s: domain not found ERROR!\n, __func__);
 goto out;
 };
@@ -253,14 +251,17 @@ ste_init_state(struct acm_ste_policy_buf
 ste_rssid = GET_SSIDP(ACM_SIMPLE_TYPE_ENFORCEMENT_POLICY, 
   (struct acm_ssid_domain *)(rdom-ssid));
 ste_rssidref = ste_rssid-ste_ssidref;
-put_domain(rdom);
 if (!have_common_type(ste_ssidref, ste_rssidref)) {
+spin_unlock((*pd)-grant_table-lock);
 printkd(%s: Policy violation in grant table sharing domain %x - domain %x.\n,
 __func__, (*pd)-domain_id, rdomid);
+put_domain(rdom);
 goto out;
 }
+put_domain(rdom);
 }
 }
+spin_unlock((*pd)-grant_table-lock);
 }
 violation = 0;
  out:
diff -r b5fc88aad1b0 -r 4fa9b86d41b8 xen/common/grant_table.c
--- a/xen/common/grant_table.c	Sun Feb 18 15:29:40 2007 +
+++ b/xen/common/grant_table.c	Mon Feb 19 15:52:31 2007 +0900
@@ -100,9 +100,6 @@ nr_active_grant_frames(struct grant_tabl
 return num_act_frames_from_sha_frames(nr_grant_frames(gt));
 }
 
-#define SHGNT_PER_PAGE (PAGE_SIZE / sizeof(grant_entry_t))
-#define shared_entry(t, e) \
-((t)-shared[(e)/SHGNT_PER_PAGE][(e)%SHGNT_PER_PAGE])
 #define ACGNT_PER_PAGE (PAGE_SIZE /