| Message ID | 20200328053834.GA12753@simran-Inspiron-5558 (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/x86: Remove unnecessary cast on void pointer | expand |
Thanks! On Sat, Mar 28, 2020 at 11:08:35AM +0530, Simran Singhal wrote: > diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c > index f049920196..a53d3ca2a4 100644 > --- a/xen/arch/x86/hvm/vmx/vvmx.c > +++ b/xen/arch/x86/hvm/vmx/vvmx.c > @@ -256,7 +256,7 @@ static int vvmcs_offset(u32 width, u32 type, u32 index) > u64 get_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding) > { > union vmcs_encoding enc; > - u64 *content = (u64 *) vvmcs; > + u64 *content = vvmcs; > int offset; > u64 res; > > @@ -310,7 +310,7 @@ enum vmx_insn_errno get_vvmcs_real_safe(const struct vcpu *v, u32 encoding, > void set_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding, u64 val) > { > union vmcs_encoding enc; > - u64 *content = (u64 *) vvmcs; > + u64 *content = vvmcs; While there, would you mind changing u64 to uint64_t? (here and above) > int offset; > u64 res; > > diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c > index eb66077496..058b9b8adf 100644 > --- a/xen/arch/x86/mm/p2m-pt.c > +++ b/xen/arch/x86/mm/p2m-pt.c > @@ -127,7 +127,7 @@ p2m_find_entry(void *table, unsigned long *gfn_remainder, > return NULL; > } > *gfn_remainder &= (1 << shift) - 1; > - return (l1_pgentry_t *)table + index; > + return table + index; I don't think removing this cast is correct, as you would be doing a plain addition to a pointer instead of fetching the next entry in the array of l1_pgentry_t entries. If you want to get rid of the cast here you need to change the type of the table parameter to l1_pgentry_t * instead of void *. Thanks, Roger.
On Sat, Mar 28, 2020 at 11:18:40AM +0100, Roger Pau Monné wrote: > Thanks! > > On Sat, Mar 28, 2020 at 11:08:35AM +0530, Simran Singhal wrote: > > diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c > > index f049920196..a53d3ca2a4 100644 > > --- a/xen/arch/x86/hvm/vmx/vvmx.c > > +++ b/xen/arch/x86/hvm/vmx/vvmx.c > > @@ -256,7 +256,7 @@ static int vvmcs_offset(u32 width, u32 type, u32 index) > > u64 get_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding) > > { > > union vmcs_encoding enc; > > - u64 *content = (u64 *) vvmcs; > > + u64 *content = vvmcs; > > int offset; > > u64 res; > > > > @@ -310,7 +310,7 @@ enum vmx_insn_errno get_vvmcs_real_safe(const struct vcpu *v, u32 encoding, > > void set_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding, u64 val) > > { > > union vmcs_encoding enc; > > - u64 *content = (u64 *) vvmcs; > > + u64 *content = vvmcs; > > While there, would you mind changing u64 to uint64_t? (here and > above) > To add some context to this comment: new code has been using uintX variants. We want to migrate existing code gradually. Since you're touching these lines anyway, it is a good chance to do the migration. When you do this in your next version, please add a line in the commit message saying something along the line that "Take the chance to change some u64 to uint64_t". Wei.
Hi Roger, Thanks for your suggestions! On Sat, Mar 28, 2020 at 3:48 PM Roger Pau Monné <roger.pau@citrix.com> wrote: > Thanks! > > On Sat, Mar 28, 2020 at 11:08:35AM +0530, Simran Singhal wrote: > > diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c > > index f049920196..a53d3ca2a4 100644 > > --- a/xen/arch/x86/hvm/vmx/vvmx.c > > +++ b/xen/arch/x86/hvm/vmx/vvmx.c > > @@ -256,7 +256,7 @@ static int vvmcs_offset(u32 width, u32 type, u32 > index) > > u64 get_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding) > > { > > union vmcs_encoding enc; > > - u64 *content = (u64 *) vvmcs; > > + u64 *content = vvmcs; > > int offset; > > u64 res; > > > > @@ -310,7 +310,7 @@ enum vmx_insn_errno get_vvmcs_real_safe(const struct > vcpu *v, u32 encoding, > > void set_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding, u64 val) > > { > > union vmcs_encoding enc; > > - u64 *content = (u64 *) vvmcs; > > + u64 *content = vvmcs; > > While there, would you mind changing u64 to uint64_t? (here and > above) > I'll do that in the next version. > > > int offset; > > u64 res; > > > > diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c > > index eb66077496..058b9b8adf 100644 > > --- a/xen/arch/x86/mm/p2m-pt.c > > +++ b/xen/arch/x86/mm/p2m-pt.c > > @@ -127,7 +127,7 @@ p2m_find_entry(void *table, unsigned long > *gfn_remainder, > > return NULL; > > } > > *gfn_remainder &= (1 << shift) - 1; > > - return (l1_pgentry_t *)table + index; > > + return table + index; > > I don't think removing this cast is correct, as you would be doing a > plain addition to a pointer instead of fetching the next entry in the > array of l1_pgentry_t entries. > > If you want to get rid of the cast here you need to change the type of > the table parameter to l1_pgentry_t * instead of void *. > Yes, you are correct. Since void* is a pointer to an unknown type we can't do pointer arithmetic on it, as the compiler wouldn't know how big the thing pointed to is. Thus, it is necessary to keep the cast on the "table". Ah! I am sorry for this mistake. But, I am afraid why I didn't get warning during compilation. I'll remove these changes in the next version. Thanks Simran > > Thanks, Roger. >
On Sat, Mar 28, 2020 at 4:28 PM Wei Liu <wl@xen.org> wrote: > On Sat, Mar 28, 2020 at 11:18:40AM +0100, Roger Pau Monné wrote: > > Thanks! > > > > On Sat, Mar 28, 2020 at 11:08:35AM +0530, Simran Singhal wrote: > > > diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c > > > index f049920196..a53d3ca2a4 100644 > > > --- a/xen/arch/x86/hvm/vmx/vvmx.c > > > +++ b/xen/arch/x86/hvm/vmx/vvmx.c > > > @@ -256,7 +256,7 @@ static int vvmcs_offset(u32 width, u32 type, u32 > index) > > > u64 get_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding) > > > { > > > union vmcs_encoding enc; > > > - u64 *content = (u64 *) vvmcs; > > > + u64 *content = vvmcs; > > > int offset; > > > u64 res; > > > > > > @@ -310,7 +310,7 @@ enum vmx_insn_errno get_vvmcs_real_safe(const > struct vcpu *v, u32 encoding, > > > void set_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding, u64 val) > > > { > > > union vmcs_encoding enc; > > > - u64 *content = (u64 *) vvmcs; > > > + u64 *content = vvmcs; > > > > While there, would you mind changing u64 to uint64_t? (here and > > above) > > > > To add some context to this comment: new code has been using uintX > variants. We want to migrate existing code gradually. Since you're > touching these lines anyway, it is a good chance to do the migration. > > When you do this in your next version, please add a line in the commit > message saying something along the line that "Take the chance to change > some u64 to uint64_t". > Thanks for the suggestion. I'll do the changes in the next version. Thanks Simran > > Wei. >
On Sat, Mar 28, 2020 at 05:47:18PM +0530, Simran Singhal wrote: [...] > > > > > int offset; > > > u64 res; > > > > > > diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c > > > index eb66077496..058b9b8adf 100644 > > > --- a/xen/arch/x86/mm/p2m-pt.c > > > +++ b/xen/arch/x86/mm/p2m-pt.c > > > @@ -127,7 +127,7 @@ p2m_find_entry(void *table, unsigned long > > *gfn_remainder, > > > return NULL; > > > } > > > *gfn_remainder &= (1 << shift) - 1; > > > - return (l1_pgentry_t *)table + index; > > > + return table + index; > > > > I don't think removing this cast is correct, as you would be doing a > > plain addition to a pointer instead of fetching the next entry in the > > array of l1_pgentry_t entries. > > > > If you want to get rid of the cast here you need to change the type of > > the table parameter to l1_pgentry_t * instead of void *. > > > > Yes, you are correct. Since void* is a pointer to an unknown type we can't > do pointer arithmetic on it, as the compiler wouldn't know how big the > thing pointed to is. Thus, it is necessary to keep the cast on the "table". > > Ah! I am sorry for this mistake. But, I am afraid why I didn't get warning > during compilation. Pointer arithmetic on void* is allowed. It is treated as if the size of the object is 1. That's probably why you didn't get a warning. Wei.
diff --git a/xen/arch/x86/acpi/cpufreq/powernow.c b/xen/arch/x86/acpi/cpufreq/powernow.c index 3cf9c6cd05..f620bebc7e 100644 --- a/xen/arch/x86/acpi/cpufreq/powernow.c +++ b/xen/arch/x86/acpi/cpufreq/powernow.c @@ -58,7 +58,7 @@ static void transition_pstate(void *pstate) static void update_cpb(void *data) { - struct cpufreq_policy *policy = (struct cpufreq_policy *)data; + struct cpufreq_policy *policy = data; if (policy->turbo != CPUFREQ_TURBO_UNSUPPORTED) { uint64_t msr_content; diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c index e50d478d23..1ed39ef03f 100644 --- a/xen/arch/x86/cpu/vpmu.c +++ b/xen/arch/x86/cpu/vpmu.c @@ -337,7 +337,7 @@ void vpmu_do_interrupt(struct cpu_user_regs *regs) static void vpmu_save_force(void *arg) { - struct vcpu *v = (struct vcpu *)arg; + struct vcpu *v = arg; struct vpmu_struct *vpmu = vcpu_vpmu(v); if ( !vpmu_is_set(vpmu, VPMU_CONTEXT_LOADED) ) diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c index 86929b9ba1..c46e7cf4ee 100644 --- a/xen/arch/x86/hpet.c +++ b/xen/arch/x86/hpet.c @@ -215,7 +215,7 @@ again: static void hpet_interrupt_handler(int irq, void *data, struct cpu_user_regs *regs) { - struct hpet_event_channel *ch = (struct hpet_event_channel *)data; + struct hpet_event_channel *ch = data; this_cpu(irq_count)--; diff --git a/xen/arch/x86/hvm/save.c b/xen/arch/x86/hvm/save.c index 0fc59d3487..a2c56fbc1e 100644 --- a/xen/arch/x86/hvm/save.c +++ b/xen/arch/x86/hvm/save.c @@ -417,7 +417,7 @@ void _hvm_read_entry(struct hvm_domain_context *h, void *dest, memcpy(dest, &h->data[h->cur], d->length); if ( d->length < dest_len ) - memset((char *)dest + d->length, 0, dest_len - d->length); + memset(dest + d->length, 0, dest_len - d->length); h->cur += d->length; } diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c index f049920196..a53d3ca2a4 100644 --- a/xen/arch/x86/hvm/vmx/vvmx.c +++ b/xen/arch/x86/hvm/vmx/vvmx.c @@ -256,7 +256,7 @@ static int vvmcs_offset(u32 width, u32 type, u32 index) u64 get_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding) { union vmcs_encoding enc; - u64 *content = (u64 *) vvmcs; + u64 *content = vvmcs; int offset; u64 res; @@ -310,7 +310,7 @@ enum vmx_insn_errno get_vvmcs_real_safe(const struct vcpu *v, u32 encoding, void set_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding, u64 val) { union vmcs_encoding enc; - u64 *content = (u64 *) vvmcs; + u64 *content = vvmcs; int offset; u64 res; diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c index eb66077496..058b9b8adf 100644 --- a/xen/arch/x86/mm/p2m-pt.c +++ b/xen/arch/x86/mm/p2m-pt.c @@ -127,7 +127,7 @@ p2m_find_entry(void *table, unsigned long *gfn_remainder, return NULL; } *gfn_remainder &= (1 << shift) - 1; - return (l1_pgentry_t *)table + index; + return table + index; } /* Free intermediate tables from a p2m sub-tree */
Assignment to a typed pointer is sufficient in C. No cast is needed. Signed-off-by: Simran Singhal <singhalsimran0@gmail.com> --- xen/arch/x86/acpi/cpufreq/powernow.c | 2 +- xen/arch/x86/cpu/vpmu.c | 2 +- xen/arch/x86/hpet.c | 2 +- xen/arch/x86/hvm/save.c | 2 +- xen/arch/x86/hvm/vmx/vvmx.c | 4 ++-- xen/arch/x86/mm/p2m-pt.c | 2 +- 6 files changed, 7 insertions(+), 7 deletions(-)