Re: [Kernel-BR] Re: [PATCH] sound: pci: emu10k1: code refactoring and casting removal
> 2013/10/17 Takashi Iwai : >> >> Geyslan, you don't have to waste too much of your time (and my time >> for review) for this kind of so old driver code unless it really fixes >> the bugs. A clean up is good in general, but it can be sometimes >> worse than nothing since it also breaks the history, thus makes hard >> to follow via "git blame", for example, and of course, there is always >> a potential danger of regression. >> >> So, if it's about clean up, do it only in a systematic way that others >> can follow easily, and don't do it too intensively. >> >> >> thanks, >> >> Takashi His code refactoring proposal is welcome since the old code sucks from several points of view, e.g. maintainability, legibility, etc. Yes, of course, it's important to be careful in order not to introduce regressions. And yes, he could improve something here and there. As long as changes improve the maintainability and legibility of the code, there is no reason to refuse them. -- Raphael S. Carvalho -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Kernel-BR] Re: [PATCH] sound: pci: emu10k1: code refactoring and casting removal
2013/10/17 Takashi Iwai ti...@suse.de: Geyslan, you don't have to waste too much of your time (and my time for review) for this kind of so old driver code unless it really fixes the bugs. A clean up is good in general, but it can be sometimes worse than nothing since it also breaks the history, thus makes hard to follow via git blame, for example, and of course, there is always a potential danger of regression. So, if it's about clean up, do it only in a systematic way that others can follow easily, and don't do it too intensively. thanks, Takashi His code refactoring proposal is welcome since the old code sucks from several points of view, e.g. maintainability, legibility, etc. Yes, of course, it's important to be careful in order not to introduce regressions. And yes, he could improve something here and there. As long as changes improve the maintainability and legibility of the code, there is no reason to refuse them. -- Raphael S. Carvalho -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Fwd: [PATCH 1/1] staging/speakup/kobjects.c: Code improvement.
On Mon, Sep 9, 2013 at 12:56 AM, Chris Brannon wrote:
>
> "Raphael S.Carvalho" writes:
>
> > + /*
> > + * If voice was just changed, we might need to reset our
> > default
> > + * pitch and volume.
> > + */
> > + if (param->var_id == VOICE) {
> > + spk_reset_default_value("pitch", synth->default_pitch,
> > + value);
> > + spk_reset_default_value("vol", synth->default_vol,
> > + value);
>
> There's an "invalid read" bug here. You didn't introduce it; it has
> been there all along. It's possible that value contains a value that is
> out of range, in which case, the spk_reset_default_value calls could
> fetch invalid data. The value of ret should be sufficient for
> determining whether value is in range, so I'd change the condition of
> the if statement to this:
Wouldn't the following code (right before the statement: if
(param->var_id == VOICE))
check if value is out of range?
value = simple_strtol(cp, NULL, 10);
ret = spk_set_num_var(value, param, len);
if (ret == -ERANGE) {
var_data = param->data;
pr_warn("value for %s out of range, expect %d to %d\n",
param->name,
var_data->u.n.low, var_data->u.n.high);
}
>
>
> if (param->var_id == VOICE && ret != -ERANGE) {
>
> Or possibly better:
> if (param->var_id == VOICE && ret == 0) {
>
> I'd say please resend with that fix, or if not, I can send a one-line
> patch to be applied after yours.
>
> -- Chris
Regards,
Raphael S. Carvalho
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Fwd: [PATCH 1/1] staging/speakup/kobjects.c: Code improvement.
On Mon, Sep 9, 2013 at 12:56 AM, Chris Brannon ch...@the-brannons.com wrote:
Raphael S.Carvalho raphael.sc...@gmail.com writes:
+ /*
+ * If voice was just changed, we might need to reset our
default
+ * pitch and volume.
+ */
+ if (param-var_id == VOICE) {
+ spk_reset_default_value(pitch, synth-default_pitch,
+ value);
+ spk_reset_default_value(vol, synth-default_vol,
+ value);
There's an invalid read bug here. You didn't introduce it; it has
been there all along. It's possible that value contains a value that is
out of range, in which case, the spk_reset_default_value calls could
fetch invalid data. The value of ret should be sufficient for
determining whether value is in range, so I'd change the condition of
the if statement to this:
Wouldn't the following code (right before the statement: if
(param-var_id == VOICE))
check if value is out of range?
value = simple_strtol(cp, NULL, 10);
ret = spk_set_num_var(value, param, len);
if (ret == -ERANGE) {
var_data = param-data;
pr_warn(value for %s out of range, expect %d to %d\n,
param-name,
var_data-u.n.low, var_data-u.n.high);
}
if (param-var_id == VOICE ret != -ERANGE) {
Or possibly better:
if (param-var_id == VOICE ret == 0) {
I'd say please resend with that fix, or if not, I can send a one-line
patch to be applied after yours.
-- Chris
Regards,
Raphael S. Carvalho
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] staging/speakup/kobjects.c: Code improvement.
Well, there is no need to use strcmp since we can make a test of similar
semantic by using the var_id field of param.
I moved the test into the VAR_NUM:VAR_TIME case since VAR_STRING will never be
"voice".
spk_xlate isn't used anymore (in line 608), then there is no difference between
using cp and buf in VAR_STRING case.
Besides, buf is a const char and those changes remove one uneeded line.
I created the function spk_reset_default_value because it clarifies the code
and allows code reusing.
Signed-off-by: Raphael S. Carvalho
---
drivers/staging/speakup/kobjects.c | 73 +++
1 files changed, 40 insertions(+), 33 deletions(-)
diff --git a/drivers/staging/speakup/kobjects.c
b/drivers/staging/speakup/kobjects.c
index 943b6c1..4660ce3 100644
--- a/drivers/staging/speakup/kobjects.c
+++ b/drivers/staging/speakup/kobjects.c
@@ -585,6 +585,25 @@ ssize_t spk_var_show(struct kobject *kobj, struct
kobj_attribute *attr,
}
EXPORT_SYMBOL_GPL(spk_var_show);
+/*
+ * Used to reset either default_pitch or default_vol.
+ */
+static inline void spk_reset_default_value(char *header_name,
+ int *synth_default_value, int idx)
+{
+ struct st_var_header *param;
+
+ if (synth && synth_default_value) {
+ param = spk_var_header_by_name(header_name);
+ if (param) {
+ spk_set_num_var(synth_default_value[idx],
+ param, E_NEW_DEFAULT);
+ spk_set_num_var(0, param, E_DEFAULT);
+ pr_info("%s reset to default value\n", param->name);
+ }
+ }
+}
+
/*
* This function is called when a user echos a value to one of the
* variable parameters.
@@ -624,56 +643,44 @@ ssize_t spk_var_store(struct kobject *kobj, struct
kobj_attribute *attr,
if (ret == -ERANGE) {
var_data = param->data;
pr_warn("value for %s out of range, expect %d to %d\n",
- attr->attr.name,
+ param->name,
var_data->u.n.low, var_data->u.n.high);
}
+
+ /*
+ * If voice was just changed, we might need to reset our default
+ * pitch and volume.
+ */
+ if (param->var_id == VOICE) {
+ spk_reset_default_value("pitch", synth->default_pitch,
+ value);
+ spk_reset_default_value("vol", synth->default_vol,
+ value);
+ }
break;
case VAR_STRING:
- len = strlen(buf);
- if ((len >= 1) && (buf[len - 1] == '\n'))
+ len = strlen(cp);
+ if ((len >= 1) && (cp[len - 1] == '\n'))
--len;
- if ((len >= 2) && (buf[0] == '"') && (buf[len - 1] == '"')) {
- ++buf;
+ if ((len >= 2) && (cp[0] == '"') && (cp[len - 1] == '"')) {
+ ++cp;
len -= 2;
}
- cp = (char *) buf;
cp[len] = '\0';
- ret = spk_set_string_var(buf, param, len);
+ ret = spk_set_string_var(cp, param, len);
if (ret == -E2BIG)
pr_warn("value too long for %s\n",
- attr->attr.name);
+ param->name);
break;
default:
pr_warn("%s unknown type %d\n",
param->name, (int)param->var_type);
- break;
- }
- /*
-* If voice was just changed, we might need to reset our default
-* pitch and volume.
-*/
- if (strcmp(attr->attr.name, "voice") == 0) {
- if (synth && synth->default_pitch) {
- param = spk_var_header_by_name("pitch");
- if (param) {
- spk_set_num_var(synth->default_pitch[value],
- param, E_NEW_DEFAULT);
- spk_set_num_var(0, param, E_DEFAULT);
- }
- }
- if (synth && synth->default_vol) {
- param = spk_var_header_by_name("vol");
- if (param) {
- spk_set_num_var(synth->default_vol[value],
- param, E_NEW_DEFAULT);
- spk_set_num_var(0, param, E_DEFA
[PATCH 1/1] staging/speakup/kobjects.c: Code improvement.
Well, there is no need to use strcmp since we can make a test of similar
semantic by using the var_id field of param.
I moved the test into the VAR_NUM:VAR_TIME case since VAR_STRING will never be
voice.
spk_xlate isn't used anymore (in line 608), then there is no difference between
using cp and buf in VAR_STRING case.
Besides, buf is a const char and those changes remove one uneeded line.
I created the function spk_reset_default_value because it clarifies the code
and allows code reusing.
Signed-off-by: Raphael S. Carvalho raphael.sc...@gmail.com
---
drivers/staging/speakup/kobjects.c | 73 +++
1 files changed, 40 insertions(+), 33 deletions(-)
diff --git a/drivers/staging/speakup/kobjects.c
b/drivers/staging/speakup/kobjects.c
index 943b6c1..4660ce3 100644
--- a/drivers/staging/speakup/kobjects.c
+++ b/drivers/staging/speakup/kobjects.c
@@ -585,6 +585,25 @@ ssize_t spk_var_show(struct kobject *kobj, struct
kobj_attribute *attr,
}
EXPORT_SYMBOL_GPL(spk_var_show);
+/*
+ * Used to reset either default_pitch or default_vol.
+ */
+static inline void spk_reset_default_value(char *header_name,
+ int *synth_default_value, int idx)
+{
+ struct st_var_header *param;
+
+ if (synth synth_default_value) {
+ param = spk_var_header_by_name(header_name);
+ if (param) {
+ spk_set_num_var(synth_default_value[idx],
+ param, E_NEW_DEFAULT);
+ spk_set_num_var(0, param, E_DEFAULT);
+ pr_info(%s reset to default value\n, param-name);
+ }
+ }
+}
+
/*
* This function is called when a user echos a value to one of the
* variable parameters.
@@ -624,56 +643,44 @@ ssize_t spk_var_store(struct kobject *kobj, struct
kobj_attribute *attr,
if (ret == -ERANGE) {
var_data = param-data;
pr_warn(value for %s out of range, expect %d to %d\n,
- attr-attr.name,
+ param-name,
var_data-u.n.low, var_data-u.n.high);
}
+
+ /*
+ * If voice was just changed, we might need to reset our default
+ * pitch and volume.
+ */
+ if (param-var_id == VOICE) {
+ spk_reset_default_value(pitch, synth-default_pitch,
+ value);
+ spk_reset_default_value(vol, synth-default_vol,
+ value);
+ }
break;
case VAR_STRING:
- len = strlen(buf);
- if ((len = 1) (buf[len - 1] == '\n'))
+ len = strlen(cp);
+ if ((len = 1) (cp[len - 1] == '\n'))
--len;
- if ((len = 2) (buf[0] == '') (buf[len - 1] == '')) {
- ++buf;
+ if ((len = 2) (cp[0] == '') (cp[len - 1] == '')) {
+ ++cp;
len -= 2;
}
- cp = (char *) buf;
cp[len] = '\0';
- ret = spk_set_string_var(buf, param, len);
+ ret = spk_set_string_var(cp, param, len);
if (ret == -E2BIG)
pr_warn(value too long for %s\n,
- attr-attr.name);
+ param-name);
break;
default:
pr_warn(%s unknown type %d\n,
param-name, (int)param-var_type);
- break;
- }
- /*
-* If voice was just changed, we might need to reset our default
-* pitch and volume.
-*/
- if (strcmp(attr-attr.name, voice) == 0) {
- if (synth synth-default_pitch) {
- param = spk_var_header_by_name(pitch);
- if (param) {
- spk_set_num_var(synth-default_pitch[value],
- param, E_NEW_DEFAULT);
- spk_set_num_var(0, param, E_DEFAULT);
- }
- }
- if (synth synth-default_vol) {
- param = spk_var_header_by_name(vol);
- if (param) {
- spk_set_num_var(synth-default_vol[value],
- param, E_NEW_DEFAULT);
- spk_set_num_var(0, param, E_DEFAULT);
- }
- }
- }
+ break;
+ }
spk_unlock(flags);
if (ret == -ERESTART)
- pr_info(%s reset to default value\n, attr-attr.name);
+ pr_info(%s reset to default value\n, param-name);
return
[PATCH 1/1] kernel/pid.c: Moving statement.
Moving statement to static initilization of init_pid_ns.
Signed-off-by: Raphael S. Carvalho
---
kernel/pid.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/kernel/pid.c b/kernel/pid.c
index 0db3e79..c577d3c 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -75,6 +75,7 @@ struct pid_namespace init_pid_ns = {
[ 0 ... PIDMAP_ENTRIES-1] = { ATOMIC_INIT(BITS_PER_PAGE), NULL }
},
.last_pid = 0,
+ .nr_hashed = PIDNS_HASH_ADDING,
.level = 0,
.child_reaper = _task,
.user_ns = _user_ns,
@@ -594,7 +595,6 @@ void __init pidmap_init(void)
/* Reserve PID 0. We never call free_pidmap(0) */
set_bit(0, init_pid_ns.pidmap[0].page);
atomic_dec(_pid_ns.pidmap[0].nr_free);
- init_pid_ns.nr_hashed = PIDNS_HASH_ADDING;
init_pid_ns.pid_cachep = KMEM_CACHE(pid,
SLAB_HWCACHE_ALIGN | SLAB_PANIC);
--
1.7.2.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] kernel/pid.c: Masking the flag out to get the actual value.
This patch shouldn't be applied if those branches must only be taken when
the pid_allocation(PIDNS_HASH_ADDING) flag was turned off.
Otherwise, we must turn the PIDNS_HASH_ADDING flag (1U << 31) off
before getting into the switch-cases.
Signed-off-by: Raphael S. Carvalho
---
kernel/pid.c |5 -
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/kernel/pid.c b/kernel/pid.c
index 0db3e79..6bda527 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -263,7 +263,10 @@ void free_pid(struct pid *pid)
struct upid *upid = pid->numbers + i;
struct pid_namespace *ns = upid->ns;
hlist_del_rcu(>pid_chain);
- switch(--ns->nr_hashed) {
+
+ /* We must turn the PIDNS_HASH_ADDING flag off to
+ get the actual value of nr_hashed */
+ switch ((--ns->nr_hashed) & ~(PIDNS_HASH_ADDING)) {
case 1:
/* When all that is left in the pid namespace
* is the reaper wake up the reaper. The reaper
--
1.7.2.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] kernel/pid.c: Masking the flag out to get the actual value.
This patch shouldn't be applied if those branches must only be taken when
the pid_allocation(PIDNS_HASH_ADDING) flag was turned off.
Otherwise, we must turn the PIDNS_HASH_ADDING flag (1U 31) off
before getting into the switch-cases.
Signed-off-by: Raphael S. Carvalho raphael.sc...@gmail.com
---
kernel/pid.c |5 -
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/kernel/pid.c b/kernel/pid.c
index 0db3e79..6bda527 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -263,7 +263,10 @@ void free_pid(struct pid *pid)
struct upid *upid = pid-numbers + i;
struct pid_namespace *ns = upid-ns;
hlist_del_rcu(upid-pid_chain);
- switch(--ns-nr_hashed) {
+
+ /* We must turn the PIDNS_HASH_ADDING flag off to
+ get the actual value of nr_hashed */
+ switch ((--ns-nr_hashed) ~(PIDNS_HASH_ADDING)) {
case 1:
/* When all that is left in the pid namespace
* is the reaper wake up the reaper. The reaper
--
1.7.2.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] kernel/pid.c: Moving statement.
Moving statement to static initilization of init_pid_ns.
Signed-off-by: Raphael S. Carvalho raphael.sc...@gmail.com
---
kernel/pid.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/kernel/pid.c b/kernel/pid.c
index 0db3e79..c577d3c 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -75,6 +75,7 @@ struct pid_namespace init_pid_ns = {
[ 0 ... PIDMAP_ENTRIES-1] = { ATOMIC_INIT(BITS_PER_PAGE), NULL }
},
.last_pid = 0,
+ .nr_hashed = PIDNS_HASH_ADDING,
.level = 0,
.child_reaper = init_task,
.user_ns = init_user_ns,
@@ -594,7 +595,6 @@ void __init pidmap_init(void)
/* Reserve PID 0. We never call free_pidmap(0) */
set_bit(0, init_pid_ns.pidmap[0].page);
atomic_dec(init_pid_ns.pidmap[0].nr_free);
- init_pid_ns.nr_hashed = PIDNS_HASH_ADDING;
init_pid_ns.pid_cachep = KMEM_CACHE(pid,
SLAB_HWCACHE_ALIGN | SLAB_PANIC);
--
1.7.2.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] kernel/auditfilter.c: Fixing a warning. GCC warning message: kernel/auditfilter.c:426: warning: this decimal constant is unsigned only in ISO C90
Signed-off-by: Raphael S. Carvalho
---
kernel/auditfilter.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 83a2970..3f3f837 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -423,7 +423,7 @@ static struct audit_entry *audit_data_to_entry(struct
audit_rule_data *data,
f->lsm_rule = NULL;
/* Support legacy tests for a valid loginuid */
- if ((f->type == AUDIT_LOGINUID) && (f->val == 4294967295)) {
+ if ((f->type == AUDIT_LOGINUID) && (f->val == 4294967295U)) {
f->type = AUDIT_LOGINUID_SET;
f->val = 0;
}
--
1.7.2.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] kernel/auditfilter.c: Fixing a warning. GCC warning message: kernel/auditfilter.c:426: warning: this decimal constant is unsigned only in ISO C90
Signed-off-by: Raphael S. Carvalho raphael.sc...@gmail.com
---
kernel/auditfilter.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 83a2970..3f3f837 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -423,7 +423,7 @@ static struct audit_entry *audit_data_to_entry(struct
audit_rule_data *data,
f-lsm_rule = NULL;
/* Support legacy tests for a valid loginuid */
- if ((f-type == AUDIT_LOGINUID) (f-val == 4294967295)) {
+ if ((f-type == AUDIT_LOGINUID) (f-val == 4294967295U)) {
f-type = AUDIT_LOGINUID_SET;
f-val = 0;
}
--
1.7.2.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] kernel/pid.c: Improve flow of a loop inside alloc_pidmap.
On Mon, Mar 11, 2013 at 6:57 PM, Andrew Morton
wrote:
> On Mon, 11 Mar 2013 18:18:56 -0300 "Raphael S. Carvalho"
> wrote:
>
>> From: Raphael S.Carvalho
>>
>> Notes: find_next_offset searches for an available "cleaned bit"
>> in the respective pid bitmap (page), so returns the offset if found,
>> otherwise it returns a value equals to BITS_PER_PAGE.
>>
>> For example, suppose find_next_offset didn't find any available
>> bit, so there's no purpose to call mk_pid (Wasteful Cpu Cycles).
>>
>> Therefore, I found it could be better to call mk_pid after
>> the checking (offset < BITS_PER_PAGE) returned sucessfully!
>> Another point: If (offset < BITS_PER_PAGE) results in a "failure",
>> then mk_pid would be called again afterwards.
>>
>> ...
>>
>> --- a/kernel/pid.c
>> +++ b/kernel/pid.c
>> @@ -190,8 +190,8 @@ static int alloc_pidmap(struct pid_namespace *pid_ns)
>> return pid;
>> }
>> offset = find_next_offset(map, offset);
>> - pid = mk_pid(pid_ns, map, offset);
>> - } while (offset < BITS_PER_PAGE && pid < pid_max);
>> + } while (offset < BITS_PER_PAGE &&
>> + (pid = mk_pid(pid_ns, map, offset)) < pid_max);
>> }
>> if (map < _ns->pidmap[(pid_max-1)/BITS_PER_PAGE]) {
>> ++map;
>
> Looks OK. But I think it's simpler and more straightforward to do it
> this way?
Yes, it looks much better.
>
> for ( ; ; ) {
> if (!test_and_set_bit(offset, map->page)) {
> atomic_dec(>nr_free);
> set_last_pid(pid_ns, last, pid);
> return pid;
> }
> offset = find_next_offset(map, offset);
> if (offset >= BITS_PER_PAGE)
> break;
> pid = mk_pid(pid_ns, map, offset);
> if (pid >= pid_max)
> break;
> }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] kernel/pid.c: Improve flow of a loop inside alloc_pidmap.
From: Raphael S.Carvalho
Notes: find_next_offset searches for an available "cleaned bit"
in the respective pid bitmap (page), so returns the offset if found,
otherwise it returns a value equals to BITS_PER_PAGE.
For example, suppose find_next_offset didn't find any available
bit, so there's no purpose to call mk_pid (Wasteful Cpu Cycles).
Therefore, I found it could be better to call mk_pid after
the checking (offset < BITS_PER_PAGE) returned sucessfully!
Another point: If (offset < BITS_PER_PAGE) results in a "failure",
then mk_pid would be called again afterwards.
Signed-off-by: Raphael S. Carvalho
---
kernel/pid.c |4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/pid.c b/kernel/pid.c
index 047dc62..7ecb09a 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -190,8 +190,8 @@ static int alloc_pidmap(struct pid_namespace *pid_ns)
return pid;
}
offset = find_next_offset(map, offset);
- pid = mk_pid(pid_ns, map, offset);
- } while (offset < BITS_PER_PAGE && pid < pid_max);
+ } while (offset < BITS_PER_PAGE &&
+ (pid = mk_pid(pid_ns, map, offset)) < pid_max);
}
if (map < _ns->pidmap[(pid_max-1)/BITS_PER_PAGE]) {
++map;
--
1.7.2.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] kernel/pid.c: Improve flow of a loop inside alloc_pidmap.
From: Raphael S.Carvalho raphael.sc...@gmail.com
Notes: find_next_offset searches for an available cleaned bit
in the respective pid bitmap (page), so returns the offset if found,
otherwise it returns a value equals to BITS_PER_PAGE.
For example, suppose find_next_offset didn't find any available
bit, so there's no purpose to call mk_pid (Wasteful Cpu Cycles).
Therefore, I found it could be better to call mk_pid after
the checking (offset BITS_PER_PAGE) returned sucessfully!
Another point: If (offset BITS_PER_PAGE) results in a failure,
then mk_pid would be called again afterwards.
Signed-off-by: Raphael S. Carvalho raphael.sc...@gmail.com
---
kernel/pid.c |4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/pid.c b/kernel/pid.c
index 047dc62..7ecb09a 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -190,8 +190,8 @@ static int alloc_pidmap(struct pid_namespace *pid_ns)
return pid;
}
offset = find_next_offset(map, offset);
- pid = mk_pid(pid_ns, map, offset);
- } while (offset BITS_PER_PAGE pid pid_max);
+ } while (offset BITS_PER_PAGE
+ (pid = mk_pid(pid_ns, map, offset)) pid_max);
}
if (map pid_ns-pidmap[(pid_max-1)/BITS_PER_PAGE]) {
++map;
--
1.7.2.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] kernel/pid.c: Improve flow of a loop inside alloc_pidmap.
On Mon, Mar 11, 2013 at 6:57 PM, Andrew Morton
a...@linux-foundation.org wrote:
On Mon, 11 Mar 2013 18:18:56 -0300 Raphael S. Carvalho
raphael.sc...@gmail.com wrote:
From: Raphael S.Carvalho raphael.sc...@gmail.com
Notes: find_next_offset searches for an available cleaned bit
in the respective pid bitmap (page), so returns the offset if found,
otherwise it returns a value equals to BITS_PER_PAGE.
For example, suppose find_next_offset didn't find any available
bit, so there's no purpose to call mk_pid (Wasteful Cpu Cycles).
Therefore, I found it could be better to call mk_pid after
the checking (offset BITS_PER_PAGE) returned sucessfully!
Another point: If (offset BITS_PER_PAGE) results in a failure,
then mk_pid would be called again afterwards.
...
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -190,8 +190,8 @@ static int alloc_pidmap(struct pid_namespace *pid_ns)
return pid;
}
offset = find_next_offset(map, offset);
- pid = mk_pid(pid_ns, map, offset);
- } while (offset BITS_PER_PAGE pid pid_max);
+ } while (offset BITS_PER_PAGE
+ (pid = mk_pid(pid_ns, map, offset)) pid_max);
}
if (map pid_ns-pidmap[(pid_max-1)/BITS_PER_PAGE]) {
++map;
Looks OK. But I think it's simpler and more straightforward to do it
this way?
Yes, it looks much better.
for ( ; ; ) {
if (!test_and_set_bit(offset, map-page)) {
atomic_dec(map-nr_free);
set_last_pid(pid_ns, last, pid);
return pid;
}
offset = find_next_offset(map, offset);
if (offset = BITS_PER_PAGE)
break;
pid = mk_pid(pid_ns, map, offset);
if (pid = pid_max)
break;
}
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] scripts/gen_sendmail.py: Script Added: Generate git send-email arguments automatically!
I guess this script won't be merged upstream, but I think it could be useful
for someone else.
Description borrowed from the header:
---
!# This script generates the git send-email automatically
!# by looking at the output generated by scripts/get_maintainer.pl
!#
!# You can pass as many patch files as needed.
!# Usage: python send-mail.py [option] ...
Example of use:
Passed as arguments one patch file and -v (verbose mode).
---
raphaelsc@debian:~/kernel/linux$ scripts/gen_sendmail.py -v
0001-kernel-pid.c-Improve-flow-of-a-loop-inside-alloc_pid.patch
git send-email --from "Raphael S. Carvalho " --to
"Eric W. Biederman " --to "Andrew Morton
" --to "Serge E. Hallyn " --to
"Serge Hallyn " --to "David S. Miller
" --cc "linux-kernel@vger.kernel.org"
0001-kernel-pid.c-Improve-flow-of-a-loop-inside-alloc_pid.patch
* Statistics: Maintainer(s): 5, List(s): 1
---
Any bug reports or improvements are welcome!
Signed-off-by: Raphael S. Carvalho
---
scripts/gen_sendmail.py | 165 +++
1 files changed, 165 insertions(+), 0 deletions(-)
create mode 100755 scripts/gen_sendmail.py
diff --git a/scripts/gen_sendmail.py b/scripts/gen_sendmail.py
new file mode 100755
index 000..921a8cc
--- /dev/null
+++ b/scripts/gen_sendmail.py
@@ -0,0 +1,165 @@
+#! /usr/bin/env python
+#
+# Generate git send-email arguments (gen_sendmail.py)
+# (c) 2013, Raphael S.Carvalho
+#
+# This script generates the git send-email automatically
+# by looking at the output generated by scripts/get_maintainer.pl
+#
+# You can pass as many patch files as needed.
+# Usage: python send-mail.py [options] ...
+#
+# Licensed under the terms of the GNU GPL License version 2
+
+import commands
+import sys
+import StringIO
+import getopt
+
+# Default definitions
+GIT_SENDMAIL = "git send-email"
+SCRIPT = "scripts/get_maintainer.pl"
+
+
+def get_user_gitconfig(git_config):
+ get_name = "git config user.name"
+ get_email = "git config user.email"
+
+ (stat, name) = commands.getstatusoutput(get_name)
+ if (stat != 0):
+ return (stat, get_name)
+
+ (stat, email) = commands.getstatusoutput(get_email)
+ if (stat != 0):
+ return (stat, get_email)
+
+ # Setup git config structure!
+ git_config['user_name'] = name
+ git_config['user_email'] = email
+
+ return (0, None)
+
+
+# Try to execute: get_maintainer.pl
+def exec_get_maintainers(patch_name):
+ command = SCRIPT + ' ' + patch_name
+ (stat, output) = commands.getstatusoutput(command)
+ return (stat, output)
+
+
+def find_maintainer(maintainers, email):
+ for m in maintainers:
+ if m['email'] == email:
+ return True
+ return False
+
+
+# Get file and/or mail from each line,
+# and build a simple maintainers database.
+def build_list(buf):
+ maintainers = []
+
+ for line in iter(buf.readline, ""):
+ name = ""
+ email = ""
+
+ pos = line.find("<")
+ if pos != -1:
+ name = line[: pos-1].replace('"', "")
+ pos2 = line.find("(")
+ email = line[pos : pos2-1]
+ else:
+ pos = line.find("(")
+ email = line[: pos-1]
+
+ # If not find_maintainer, then add to the list.
+ if not find_maintainer(maintainers, email):
+ maintainer = {'name': name, 'email': email}
+ maintainers.append(maintainer)
+
+ return maintainers
+
+
+# Generates command from the built database.
+def generate_gitmail_cmd(maintainers, git_config, args):
+ mnt_count = list_count = 0
+
+ print '%s --from "%s <%s>"' % \
+ (GIT_SENDMAIL, \
+ git_config['user_name'], git_config['user_email']),
+
+ for m in maintainers:
+ if m['name'] != "":
+ print '--to "%s %s"' % (m['name'], m['email']),
+ mnt_count += 1
+ else:
+ print '--cc "%s"' % m['email'],
+ list_count += 1
+ for arg in args:
+ print arg,
+
+ return (mnt_count, list_count)
+
+
+def usage(program):
+ print 'Usage: %s [options] \n' \
+ '-v --verbose: Print verbose messages.\n' \
+ '-h --help: Print this information.\n\n' \
+ 'This script must be installed in the directory' \
+ ' scripts of linux tree!' % program,
+ sys.exit(2)
+
+
+def main(argc, argv):
+ verbose = False
+
+ try:
+ o
[PATCH 1/1] scripts/gen_sendmail.py: Script Added: Generate git send-email arguments automatically!
I guess this script won't be merged upstream, but I think it could be useful
for someone else.
Description borrowed from the header:
---
!# This script generates the git send-email automatically
!# by looking at the output generated by scripts/get_maintainer.pl
!#
!# You can pass as many patch files as needed.
!# Usage: python send-mail.py [option] patch1 patch2 ...
Example of use:
Passed as arguments one patch file and -v (verbose mode).
---
raphaelsc@debian:~/kernel/linux$ scripts/gen_sendmail.py -v
0001-kernel-pid.c-Improve-flow-of-a-loop-inside-alloc_pid.patch
git send-email --from Raphael S. Carvalho raphael.sc...@gmail.com --to
Eric W. Biederman ebied...@xmission.com --to Andrew Morton
a...@linux-foundation.org --to Serge E. Hallyn se...@hallyn.com --to
Serge Hallyn serge.hal...@canonical.com --to David S. Miller
da...@davemloft.net --cc linux-kernel@vger.kernel.org
0001-kernel-pid.c-Improve-flow-of-a-loop-inside-alloc_pid.patch
* Statistics: Maintainer(s): 5, List(s): 1
---
Any bug reports or improvements are welcome!
Signed-off-by: Raphael S. Carvalho raphael.sc...@gmail.com
---
scripts/gen_sendmail.py | 165 +++
1 files changed, 165 insertions(+), 0 deletions(-)
create mode 100755 scripts/gen_sendmail.py
diff --git a/scripts/gen_sendmail.py b/scripts/gen_sendmail.py
new file mode 100755
index 000..921a8cc
--- /dev/null
+++ b/scripts/gen_sendmail.py
@@ -0,0 +1,165 @@
+#! /usr/bin/env python
+#
+# Generate git send-email arguments (gen_sendmail.py)
+# (c) 2013, Raphael S.Carvalho raphael.sc...@gmail.com
+#
+# This script generates the git send-email automatically
+# by looking at the output generated by scripts/get_maintainer.pl
+#
+# You can pass as many patch files as needed.
+# Usage: python send-mail.py [options] patch1 patch2 ...
+#
+# Licensed under the terms of the GNU GPL License version 2
+
+import commands
+import sys
+import StringIO
+import getopt
+
+# Default definitions
+GIT_SENDMAIL = git send-email
+SCRIPT = scripts/get_maintainer.pl
+
+
+def get_user_gitconfig(git_config):
+ get_name = git config user.name
+ get_email = git config user.email
+
+ (stat, name) = commands.getstatusoutput(get_name)
+ if (stat != 0):
+ return (stat, get_name)
+
+ (stat, email) = commands.getstatusoutput(get_email)
+ if (stat != 0):
+ return (stat, get_email)
+
+ # Setup git config structure!
+ git_config['user_name'] = name
+ git_config['user_email'] = email
+
+ return (0, None)
+
+
+# Try to execute: get_maintainer.pl patch
+def exec_get_maintainers(patch_name):
+ command = SCRIPT + ' ' + patch_name
+ (stat, output) = commands.getstatusoutput(command)
+ return (stat, output)
+
+
+def find_maintainer(maintainers, email):
+ for m in maintainers:
+ if m['email'] == email:
+ return True
+ return False
+
+
+# Get file and/or mail from each line,
+# and build a simple maintainers database.
+def build_list(buf):
+ maintainers = []
+
+ for line in iter(buf.readline, ):
+ name =
+ email =
+
+ pos = line.find()
+ if pos != -1:
+ name = line[: pos-1].replace('', )
+ pos2 = line.find(()
+ email = line[pos : pos2-1]
+ else:
+ pos = line.find(()
+ email = line[: pos-1]
+
+ # If not find_maintainer, then add to the list.
+ if not find_maintainer(maintainers, email):
+ maintainer = {'name': name, 'email': email}
+ maintainers.append(maintainer)
+
+ return maintainers
+
+
+# Generates command from the built database.
+def generate_gitmail_cmd(maintainers, git_config, args):
+ mnt_count = list_count = 0
+
+ print '%s --from %s %s' % \
+ (GIT_SENDMAIL, \
+ git_config['user_name'], git_config['user_email']),
+
+ for m in maintainers:
+ if m['name'] != :
+ print '--to %s %s' % (m['name'], m['email']),
+ mnt_count += 1
+ else:
+ print '--cc %s' % m['email'],
+ list_count += 1
+ for arg in args:
+ print arg,
+
+ return (mnt_count, list_count)
+
+
+def usage(program):
+ print 'Usage: %s [options] patch(es) file\n' \
+ '-v --verbose: Print verbose messages.\n' \
+ '-h --help: Print this information.\n\n' \
+ 'This script must be installed in the directory' \
+ ' scripts of linux tree!' % program,
+ sys.exit(2)
+
+
+def main(argc, argv):
+ verbose = False
+
+ try:
+ opts, args = getopt.getopt(argv[1:], 'hv', ['help', 'verbose
Re: [PATCH 1/1] kernel/pid_namespace.c: Fixing a lack of cleanup (Probable resources leak).
On Tue, Mar 5, 2013 at 12:51 AM, Gao feng wrote: > On 2013/03/05 11:26, Eric W. Biederman wrote: >> From: Raphael S.Carvalho >> >> Starting point: create_pid_namespace() >> >> Suppose create_pid_cachep() was executed sucessfully, thus: >> pcache was allocated by kmalloc(). >> cachep received a cache created by kmem_cache_create(). >> and pcache->list was added to the list pid_caches_lh. >> >> So what would happen if proc_alloc_inum() returns an error? >> The resources allocated by create_pid_namespace() would be deallocated! >> How about those resources allocated by create_pid_cachep()? >> By knowing that, I created this patch in order to fix that! >> >> Signed-off-by: Raphael S.Carvalho >> --- > > Actually I noticed this problem and I think it is not a BUG. > Since the pid_cache is created for all pid namespace which have the same > level. > Even this pid namespace is failed to create, the pid_cache will not be > leaked, Other > pid namespace which has the same level will use the pid_cache and no need to > allocate it again. In other words, the pid_cache for every level pid > namespace will > only be created once. > > I also think this patch add a bug,because there may be other pid namespace's > pid_cachep > points to the same pid_cache which will be free at the by label > out_free_cachep. > Yeah, I found the snippet of code which searches for the pcache with the same level. 46list_for_each_entry(pcache, _caches_lh, list) 47if (pcache->nr_ids == nr_ids) 48goto out; Regards, Raphael S.Carvalho -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] kernel/pid_namespace.c: Fixing a lack of cleanup (Probable resources leak).
On Tue, Mar 5, 2013 at 12:51 AM, Gao feng gaof...@cn.fujitsu.com wrote: On 2013/03/05 11:26, Eric W. Biederman wrote: From: Raphael S.Carvalho raphael.sc...@gmail.com Starting point: create_pid_namespace() Suppose create_pid_cachep() was executed sucessfully, thus: pcache was allocated by kmalloc(). cachep received a cache created by kmem_cache_create(). and pcache-list was added to the list pid_caches_lh. So what would happen if proc_alloc_inum() returns an error? The resources allocated by create_pid_namespace() would be deallocated! How about those resources allocated by create_pid_cachep()? By knowing that, I created this patch in order to fix that! Signed-off-by: Raphael S.Carvalho raphael.sc...@gmail.com --- Actually I noticed this problem and I think it is not a BUG. Since the pid_cache is created for all pid namespace which have the same level. Even this pid namespace is failed to create, the pid_cache will not be leaked, Other pid namespace which has the same level will use the pid_cache and no need to allocate it again. In other words, the pid_cache for every level pid namespace will only be created once. I also think this patch add a bug,because there may be other pid namespace's pid_cachep points to the same pid_cache which will be free at the by label out_free_cachep. Yeah, I found the snippet of code which searches for the pcache with the same level. 46list_for_each_entry(pcache, pid_caches_lh, list) 47if (pcache-nr_ids == nr_ids) 48goto out; Regards, Raphael S.Carvalho -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
About namespaces and unshare() syscall.
Hi Eric W. Biederman and Serge Hallyn,
I'm a newcomer to Linux kernel Development, and I really like the way
Linux manages namespaces.
By the way, I'm studying how copy_process() deals with it. I mean,
sharing namespaces by default and duplicating namespaces on demand.
I would appreciate if you can give me tips about where to get started
(Which areas are needing either help or improvement).
Regarding to unshare syscall, I really care about how the following
fragment of code will work in the future:
(kernel/fork.c) static int check_unshare_flags(unsigned long unshare_flags):
...
1735if (unshare_flags & (CLONE_THREAD | CLONE_SIGHAND | CLONE_VM)) {
1736/* FIXME: get_task_mm() increments ->mm_users */
1737if (atomic_read(>mm->mm_users) > 1)
1738return -EINVAL;
1739}
For example, suppose CLONE_VM was added to Unshare syscall, and so you
created an application which relies on this new feature (Unshare VM).
So I found your application in the Internet, but I'm running a kernel
which still has the above checking.
It won't work properly! why?
Taking a careful look at it, I realized that if one of those flags
were passed to the unshare syscall and the current process is sharing
its VM among other processes, the syscall would always return an
invalid error.
Conclusion: Your application (relying on Unshare CLONE_VM feature)
wouldn't work on previous kernel versions since the old
check_unshare_flags() was programmed so that CLONE_VM (with current
process sharing its VM) is implicitly an invalid operation. Thus,
lacking backward compatibility.
I also was reading why CLONE_VM was removed from Unshare syscall, it
seems a core dump could be processing at the same time, so bad things
might happen. However, it seems this feature will only be implemented
when really needed.
Regards,
Raphael S.Carvalho
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
About namespaces and unshare() syscall.
Hi Eric W. Biederman and Serge Hallyn,
I'm a newcomer to Linux kernel Development, and I really like the way
Linux manages namespaces.
By the way, I'm studying how copy_process() deals with it. I mean,
sharing namespaces by default and duplicating namespaces on demand.
I would appreciate if you can give me tips about where to get started
(Which areas are needing either help or improvement).
Regarding to unshare syscall, I really care about how the following
fragment of code will work in the future:
(kernel/fork.c) static int check_unshare_flags(unsigned long unshare_flags):
...
1735if (unshare_flags (CLONE_THREAD | CLONE_SIGHAND | CLONE_VM)) {
1736/* FIXME: get_task_mm() increments -mm_users */
1737if (atomic_read(current-mm-mm_users) 1)
1738return -EINVAL;
1739}
For example, suppose CLONE_VM was added to Unshare syscall, and so you
created an application which relies on this new feature (Unshare VM).
So I found your application in the Internet, but I'm running a kernel
which still has the above checking.
It won't work properly! why?
Taking a careful look at it, I realized that if one of those flags
were passed to the unshare syscall and the current process is sharing
its VM among other processes, the syscall would always return an
invalid error.
Conclusion: Your application (relying on Unshare CLONE_VM feature)
wouldn't work on previous kernel versions since the old
check_unshare_flags() was programmed so that CLONE_VM (with current
process sharing its VM) is implicitly an invalid operation. Thus,
lacking backward compatibility.
I also was reading why CLONE_VM was removed from Unshare syscall, it
seems a core dump could be processing at the same time, so bad things
might happen. However, it seems this feature will only be implemented
when really needed.
Regards,
Raphael S.Carvalho raphael.sc...@gmail.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] arch/x86/kvm/cpuid.c: cpuid_maxphyaddr "bad" handling.
Well, the below function reports the physical-address width supported
by the processor.
It does its work very well, though I found a detail which it doesn't
handle at all.
PS: The following function is not a patch.
int cpuid_maxphyaddr(struct kvm_vcpu *vcpu)
{
struct kvm_cpuid_entry2 *best;
best = kvm_find_cpuid_entry(vcpu, 0x8000, 0);
if (!best || best->eax < 0x8008)
goto not_found;
best = kvm_find_cpuid_entry(vcpu, 0x8008, 0);
if (best)
return best->eax & 0xff;
not_found:
return 36;
}
As I'm seeing, its(above function) first step is to check whether the
CPU provides the CPUID function 8008H,
if so, it gets the physical-address width from the available CPUID
function, otherwise it implicitly returns 36.
Intel manual says the following:
"For processors that do not support CPUID function 8008H, the
width is generally 36 if CPUID.01H:EDX.PAE [bit 6] = 1
and 32 otherwise."
According to the above-mentioned statement, we would have to return 32
whether PAE is not supported by the CPU.
So I was wondering if such a function would work efficiently on
processors that do not support PAE extension.
I also would like to share that MAXPHYADDR can be at most 52. However,
not sure if such an implementation is even needed.
NOTE:
arch/x86/include/asm/cpufeature.h provides the below macro.
#define cpu_has_pae boot_cpu_has(X86_FEATURE_PAE)
Does the above macro return 1 whether CPU does support the PAE feature?
If so, we would have to make a single change in the code:
Signed-off-by: Raphael S.Carvalho
--- a/arch/x86/kvm/cpuid.c 2012-11-19 23:44:29.0 -0200
+++ b/arch/x86/kvm/cpuid.c 2012-11-30 15:05:51.0 -0200
@@ -617,7 +617,10 @@
if (best)
return best->eax & 0xff;
not_found:
- return 36;
+ /* Check whether CPU supports PAE, if so the MAXPHYADDR
+* is 36, otherwise 32.
+*/
+ return (cpu_has_pae) ? 36 : 32;
}
Follows a different patch otherwise:
arch/x86/include/asm/processor.h does provide a generic cpuid function
called native_cpuid, however, I added another procedure for
simplicity/efficiency purposes.
Signed-off-by: Raphael S.Carvalho
--- a/arch/x86/kvm/cpuid.c 2012-11-19 23:44:29.0 -0200
+++ b/arch/x86/kvm/cpuid.c 2012-11-30 14:28:22.0 -0200
@@ -606,6 +606,30 @@
+#ifndef PAE_BIT
+#define PAE_BIT (1ULL << 6)
+#endif
+static inline unsigned cpuid_cpu_pae_support(void)
+{
+ unsigned int __edx;
+ const unsigned int cpu_id_param = 0x01;
+
+ /* According to Intel Manual we can check
+* whether the processor does provide PAE by
+* using the CPUID instruction.
+* Syntax: CPUID.01H:EDX.PAE [bit 6] = 1
+*/
+ __edx = 0;
+ asm volatile(
+ "cpuid"
+ : "=d"(__edx)
+ : "a"(cpu_id_param)
+ : "ecx","ebx"
+ );
+
+ return (__edx & PAE_BIT);
+}
+
int cpuid_maxphyaddr(struct kvm_vcpu *vcpu)
{
struct kvm_cpuid_entry2 *best;
@@ -617,7 +641,10 @@
if (best)
return best->eax & 0xff;
not_found:
- return 36;
+ /* Check whether CPU supports PAE, if so the MAXPHYADDR
+* is 36, otherwise 32.
+*/
+ return (cpuid_cpu_pae_support()) ? 36 : 32;
}
Regards,
Raphael S.Carvalho
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] arch/x86/kvm/cpuid.c: cpuid_maxphyaddr bad handling.
Well, the below function reports the physical-address width supported
by the processor.
It does its work very well, though I found a detail which it doesn't
handle at all.
PS: The following function is not a patch.
int cpuid_maxphyaddr(struct kvm_vcpu *vcpu)
{
struct kvm_cpuid_entry2 *best;
best = kvm_find_cpuid_entry(vcpu, 0x8000, 0);
if (!best || best-eax 0x8008)
goto not_found;
best = kvm_find_cpuid_entry(vcpu, 0x8008, 0);
if (best)
return best-eax 0xff;
not_found:
return 36;
}
As I'm seeing, its(above function) first step is to check whether the
CPU provides the CPUID function 8008H,
if so, it gets the physical-address width from the available CPUID
function, otherwise it implicitly returns 36.
Intel manual says the following:
For processors that do not support CPUID function 8008H, the
width is generally 36 if CPUID.01H:EDX.PAE [bit 6] = 1
and 32 otherwise.
According to the above-mentioned statement, we would have to return 32
whether PAE is not supported by the CPU.
So I was wondering if such a function would work efficiently on
processors that do not support PAE extension.
I also would like to share that MAXPHYADDR can be at most 52. However,
not sure if such an implementation is even needed.
NOTE:
arch/x86/include/asm/cpufeature.h provides the below macro.
#define cpu_has_pae boot_cpu_has(X86_FEATURE_PAE)
Does the above macro return 1 whether CPU does support the PAE feature?
If so, we would have to make a single change in the code:
Signed-off-by: Raphael S.Carvalho raphael.sc...@gmail.com
--- a/arch/x86/kvm/cpuid.c 2012-11-19 23:44:29.0 -0200
+++ b/arch/x86/kvm/cpuid.c 2012-11-30 15:05:51.0 -0200
@@ -617,7 +617,10 @@
if (best)
return best-eax 0xff;
not_found:
- return 36;
+ /* Check whether CPU supports PAE, if so the MAXPHYADDR
+* is 36, otherwise 32.
+*/
+ return (cpu_has_pae) ? 36 : 32;
}
Follows a different patch otherwise:
arch/x86/include/asm/processor.h does provide a generic cpuid function
called native_cpuid, however, I added another procedure for
simplicity/efficiency purposes.
Signed-off-by: Raphael S.Carvalho raphael.sc...@gmail.com
--- a/arch/x86/kvm/cpuid.c 2012-11-19 23:44:29.0 -0200
+++ b/arch/x86/kvm/cpuid.c 2012-11-30 14:28:22.0 -0200
@@ -606,6 +606,30 @@
+#ifndef PAE_BIT
+#define PAE_BIT (1ULL 6)
+#endif
+static inline unsigned cpuid_cpu_pae_support(void)
+{
+ unsigned int __edx;
+ const unsigned int cpu_id_param = 0x01;
+
+ /* According to Intel Manual we can check
+* whether the processor does provide PAE by
+* using the CPUID instruction.
+* Syntax: CPUID.01H:EDX.PAE [bit 6] = 1
+*/
+ __edx = 0;
+ asm volatile(
+ cpuid
+ : =d(__edx)
+ : a(cpu_id_param)
+ : ecx,ebx
+ );
+
+ return (__edx PAE_BIT);
+}
+
int cpuid_maxphyaddr(struct kvm_vcpu *vcpu)
{
struct kvm_cpuid_entry2 *best;
@@ -617,7 +641,10 @@
if (best)
return best-eax 0xff;
not_found:
- return 36;
+ /* Check whether CPU supports PAE, if so the MAXPHYADDR
+* is 36, otherwise 32.
+*/
+ return (cpuid_cpu_pae_support()) ? 36 : 32;
}
Regards,
Raphael S.Carvalho raphael.sc...@gmail.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] arch/x86/kvm/cpuid.c: cpuid_maxphyaddr "bad" handling.
Well, the below function reports the physical-address width supported
by the processor.
It does its work very well, though I found a detail which it doesn't
handle at all.
PS: The following function is not a patch.
int cpuid_maxphyaddr(struct kvm_vcpu *vcpu)
{
struct kvm_cpuid_entry2 *best;
best = kvm_find_cpuid_entry(vcpu, 0x8000, 0);
if (!best || best->eax < 0x8008)
goto not_found;
best = kvm_find_cpuid_entry(vcpu, 0x8008, 0);
if (best)
return best->eax & 0xff;
not_found:
return 36;
}
As I'm seeing, its(above function) first step is to check whether the
CPU provides the CPUID function 8008H,
if so, it gets the physical-address width from the available CPUID
function, otherwise it implicitly returns 36.
Intel manual says the following:
"For processors that do not support CPUID function 8008H, the
width is generally 36 if CPUID.01H:EDX.PAE [bit 6] = 1
and 32 otherwise."
According to the above-mentioned statement, we would have to return 32
whether PAE is not supported by the CPU.
So I was wondering if such a function would work efficiently on
processors that do not support PAE extension.
arch/x86/include/asm/processor.h does provide a generic cpuid function
called native_cpuid, however, I added another procedure for
simplicity/efficiency purposes. Besides, I didn't find where the Linux
kernel defines such flags (PAE flag (1 << 6)). For avoiding duplicated
definitions, it would be a pleasure to get my code modified.
I also would like to share that MAXPHYADDR can be at most 52. However,
not sure if such a implementation is even needed.
Signed-off-by: Raphael S.Carvalho
--- cpuid2.c2012-11-29 22:44:43.881876294 -0200
+++ cpuid.c 2012-11-30 00:02:08.565710892 -0200
@@ -606,6 +606,30 @@
}
EXPORT_SYMBOL_GPL(kvm_find_cpuid_entry);
+#ifndef PAE_BIT
+#define PAE_BIT (1ULL << 6)
+#endif
+static inline unsigned cpuid_cpu_pae_support(void)
+{
+ unsigned int __edx;
+ const unsigned int cpu_id_param = 0x01;
+
+ /* According to Intel Manual we can check
+* whether the processor does provide PAE by
+* using the CPUID instruction.
+* Syntax: CPUID.01H:EDX.PAE [bit 6] = 1
+*/
+ __edx = 0;
+ asm volatile(
+ "cpuid"
+ : "=d"(__edx)
+ : "a"(cpu_id_param)
+ : "ecx","ebx"
+ );
+
+ return (__edx & PAE_BIT);
+}
+
int cpuid_maxphyaddr(struct kvm_vcpu *vcpu)
{
struct kvm_cpuid_entry2 *best;
@@ -617,7 +641,10 @@
if (best)
return best->eax & 0xff;
not_found:
- return 36;
+ /* Check whether CPU supports PAE, if so the MAXPHYADDR
+* is 36, otherwise 32.
+*/
+ return (cpuid_cpu_pae_support()) ? 36 : 32;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] arch/x86/kvm/cpuid.c: cpuid_maxphyaddr bad handling.
Well, the below function reports the physical-address width supported
by the processor.
It does its work very well, though I found a detail which it doesn't
handle at all.
PS: The following function is not a patch.
int cpuid_maxphyaddr(struct kvm_vcpu *vcpu)
{
struct kvm_cpuid_entry2 *best;
best = kvm_find_cpuid_entry(vcpu, 0x8000, 0);
if (!best || best-eax 0x8008)
goto not_found;
best = kvm_find_cpuid_entry(vcpu, 0x8008, 0);
if (best)
return best-eax 0xff;
not_found:
return 36;
}
As I'm seeing, its(above function) first step is to check whether the
CPU provides the CPUID function 8008H,
if so, it gets the physical-address width from the available CPUID
function, otherwise it implicitly returns 36.
Intel manual says the following:
For processors that do not support CPUID function 8008H, the
width is generally 36 if CPUID.01H:EDX.PAE [bit 6] = 1
and 32 otherwise.
According to the above-mentioned statement, we would have to return 32
whether PAE is not supported by the CPU.
So I was wondering if such a function would work efficiently on
processors that do not support PAE extension.
arch/x86/include/asm/processor.h does provide a generic cpuid function
called native_cpuid, however, I added another procedure for
simplicity/efficiency purposes. Besides, I didn't find where the Linux
kernel defines such flags (PAE flag (1 6)). For avoiding duplicated
definitions, it would be a pleasure to get my code modified.
I also would like to share that MAXPHYADDR can be at most 52. However,
not sure if such a implementation is even needed.
Signed-off-by: Raphael S.Carvalho raphael.sc...@gmail.com
--- cpuid2.c2012-11-29 22:44:43.881876294 -0200
+++ cpuid.c 2012-11-30 00:02:08.565710892 -0200
@@ -606,6 +606,30 @@
}
EXPORT_SYMBOL_GPL(kvm_find_cpuid_entry);
+#ifndef PAE_BIT
+#define PAE_BIT (1ULL 6)
+#endif
+static inline unsigned cpuid_cpu_pae_support(void)
+{
+ unsigned int __edx;
+ const unsigned int cpu_id_param = 0x01;
+
+ /* According to Intel Manual we can check
+* whether the processor does provide PAE by
+* using the CPUID instruction.
+* Syntax: CPUID.01H:EDX.PAE [bit 6] = 1
+*/
+ __edx = 0;
+ asm volatile(
+ cpuid
+ : =d(__edx)
+ : a(cpu_id_param)
+ : ecx,ebx
+ );
+
+ return (__edx PAE_BIT);
+}
+
int cpuid_maxphyaddr(struct kvm_vcpu *vcpu)
{
struct kvm_cpuid_entry2 *best;
@@ -617,7 +641,10 @@
if (best)
return best-eax 0xff;
not_found:
- return 36;
+ /* Check whether CPU supports PAE, if so the MAXPHYADDR
+* is 36, otherwise 32.
+*/
+ return (cpuid_cpu_pae_support()) ? 36 : 32;
}
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
