Re: [Xen-devel] [PATCH v2] xen/x86: Remove unnecessary cast on void pointer

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Sun, Mar 29, 2020 at 02:36:51PM +0100, Wei Liu wrote:
> On Sun, Mar 29, 2020 at 10:25:12AM +0530, Simran Singhal wrote:
> > Assignment to a typed pointer is sufficient in C.
> > No cast is needed.
> > 
> > Also, changed some u64/u32 to uint64_t/uint32_t.
> > 
> > Signed-off-by: Simran Singhal <singhalsimran0@xxxxxxxxx>
> > ---
> > Changes in v2:
> >     - Took the chance to change some uintX to uintX_t.
> > 
> >  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          | 12 ++++++------
> >  5 files changed, 10 insertions(+), 10 deletions(-)
> > 
> > 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);
> 
> I believe you shouldn't drop the cast here either because dest is of
> type void*.
> 
> Although the calculation in the end is the same (void* considered of
> size 1), I would still keep the cast such that the semantics is correct.

IMO dropping the case here is fine, as dest is of type void * the
calculation is correct and the cast just obfuscates it.

I usually cast things to void * instead of char * or uint8_t * in
order to do pointer additions with size 1.

Thanks, Roger.