Re: [Xen-devel] [PATCH v4 07/17] x86/hvm: add length to mmio check op

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: xen-devel-bounces@xxxxxxxxxxxxx [mailto:xen-devel-
> bounces@xxxxxxxxxxxxx] On Behalf Of Jan Beulich
> Sent: 24 June 2015 14:08
> To: Paul Durrant
> Cc: Andrew Cooper; Keir (Xen.org); xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH v4 07/17] x86/hvm: add length to mmio
> check op
> 
> >>> On 24.06.15 at 13:24, <paul.durrant@xxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/hvm/hpet.c
> > +++ b/xen/arch/x86/hvm/hpet.c
> > @@ -498,10 +498,11 @@ static int hpet_write(
> >      return X86EMUL_OKAY;
> >  }
> >
> > -static int hpet_range(struct vcpu *v, unsigned long addr)
> > +static int hpet_range(struct vcpu *v, unsigned long addr,
> > +                      unsigned long length)
> >  {
> > -    return ( (addr >= HPET_BASE_ADDRESS) &&
> > -             (addr < (HPET_BASE_ADDRESS + HPET_MMAP_SIZE)) );
> > +    return (addr >= HPET_BASE_ADDRESS) &&
> > +           ((addr + length) < (HPET_BASE_ADDRESS + HPET_MMAP_SIZE));
> 
> <=

Yep.

> 
> > --- a/xen/arch/x86/hvm/vlapic.c
> > +++ b/xen/arch/x86/hvm/vlapic.c
> > @@ -986,14 +986,16 @@ int hvm_x2apic_msr_write(struct vcpu *v,
> unsigned int msr, uint64_t msr_content)
> >      return vlapic_reg_write(v, offset, (uint32_t)msr_content);
> >  }
> >
> > -static int vlapic_range(struct vcpu *v, unsigned long addr)
> > +static int vlapic_range(struct vcpu *v, unsigned long address,
> > +                        unsigned long len)
> >  {
> >      struct vlapic *vlapic = vcpu_vlapic(v);
> > -    unsigned long offset  = addr - vlapic_base_address(vlapic);
> > +    unsigned long offset  = address - vlapic_base_address(vlapic);
> >
> >      return !vlapic_hw_disabled(vlapic) &&
> >             !vlapic_x2apic_mode(vlapic) &&
> > -           (offset < PAGE_SIZE);
> > +           (address >= vlapic_base_address(vlapic)) &&
> > +           ((offset + len) <= PAGE_SIZE);
> 
> I'd prefer to stay with checking just offset here, unless you see
> anything wrong with that.
> 

No, I'm happy with that if you are.

> > @@ -333,12 +333,15 @@ out:
> >      return r;
> >  }
> >
> > -static int msixtbl_range(struct vcpu *v, unsigned long addr)
> > +static int msixtbl_range(struct vcpu *v, unsigned long address,
> > +                         unsigned long len)
> >  {
> > +    struct msixtbl_entry *entry;
> >      const struct msi_desc *desc;
> >
> >      rcu_read_lock(&msixtbl_rcu_lock);
> > -    desc = msixtbl_addr_to_desc(msixtbl_find_entry(v, addr), addr);
> > +    entry = msixtbl_find_entry(v, address, len);
> > +    desc = msixtbl_addr_to_desc(entry, address);
> 
> Again I don't see the need to do more adjustments here than
> necessary for your purpose.
> 

Sure.

> > --- a/xen/include/asm-x86/hvm/io.h
> > +++ b/xen/include/asm-x86/hvm/io.h
> > @@ -35,7 +35,9 @@ typedef int (*hvm_mmio_write_t)(struct vcpu *v,
> >                                  unsigned long addr,
> >                                  unsigned long length,
> >                                  unsigned long val);
> > -typedef int (*hvm_mmio_check_t)(struct vcpu *v, unsigned long addr);
> > +typedef int (*hvm_mmio_check_t)(struct vcpu *v,
> > +                                unsigned long addr,
> > +                                unsigned long length);
> 
> I don't think this really needs to be "long"?
> 

For consistency with the mmio read and write function types I went with 'long'. 
Is there any harm in that?

  Paul

> Jan
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel