Re: [Xen-ia64-devel][PATCH][VTD]change ioports_permit_access interface

[Isaku Yamahata <yamahata@xxxxxxxxxxxxx>]

On Thu, Oct 16, 2008 at 07:37:51PM +0800, Xu, Anthony wrote:
> use VTD to assing device, guest port may not be equal to host port.
> Change ioports_permit_access interface
> add deassign_domain_mmio_page interface
> 
> Signed-off-by; Anthony Xu < anthony.xu@xxxxxxxxx >

Some comments.

- ioports_permit_access()
  x86 didn't change its prototype.
  Why does only ia64 need to change it?
  Or will x86 also change it?

  I suppose, it would be nice to map the port io area
  to arbitrary guest physical area.
  But I'm not sure how x86 will go with XEN_DOMCTL_ioport_mapping.

- deassign_domain_mmio_page()
  calling __assgin_domain_page() may result in page reference count
  leak because the corresponding p2m entry may be changed to another
  value.
  So you want to modify zap_domain_page_one() so that it accepts
  iomem page and call it from deassign_domain_mmio_page().

- probably it would better to split this patch into two ones.

thanks,

> use VTD to assing device, guest port may not be equal to host port.
> Change ioports_permit_access interface
> add deassign_domain_mmio_page interface
> 
> Signed-off-by; Anthony Xu < anthony.xu@xxxxxxxxx >
> 
> 
> 
> 
> 
> diff -r 42c7733c1a2a xen/arch/ia64/xen/dom0_ops.c
> --- a/xen/arch/ia64/xen/dom0_ops.c    Thu Oct 16 18:18:39 2008 +0800
> +++ b/xen/arch/ia64/xen/dom0_ops.c    Thu Oct 16 19:13:01 2008 +0800
> @@ -203,7 +203,7 @@
>              ret = 0;
>          else {
>              if (op->u.ioport_permission.allow_access)
> -                ret = ioports_permit_access(d, fp, lp);
> +                ret = ioports_permit_access(d, fp, fp, lp);
>              else
>                  ret = ioports_deny_access(d, fp, lp);
>          }
> @@ -522,7 +522,7 @@
>      fp = space_number << IO_SPACE_BITS;
>      lp = fp | 0xffff;
>  
> -    return ioports_permit_access(d, fp, lp);
> +    return ioports_permit_access(d, fp, fp, lp);
>  }
>  
>  unsigned long
> diff -r 42c7733c1a2a xen/arch/ia64/xen/domain.c
> --- a/xen/arch/ia64/xen/domain.c      Thu Oct 16 18:18:39 2008 +0800
> +++ b/xen/arch/ia64/xen/domain.c      Thu Oct 16 19:13:01 2008 +0800
> @@ -1995,7 +1995,7 @@
>               BUG();
>       if (irqs_permit_access(d, 0, NR_IRQS-1))
>               BUG();
> -     if (ioports_permit_access(d, 0, 0xffff))
> +     if (ioports_permit_access(d, 0, 0, 0xffff))
>               BUG();
>  }
>  
> diff -r 42c7733c1a2a xen/arch/ia64/xen/mm.c
> --- a/xen/arch/ia64/xen/mm.c  Thu Oct 16 18:18:39 2008 +0800
> +++ b/xen/arch/ia64/xen/mm.c  Thu Oct 16 19:13:01 2008 +0800
> @@ -984,15 +984,22 @@
>                                 ASSIGN_writable | ASSIGN_pgc_allocated);
>  }
>  
> +/* 
> + * Inpurt
> + * fgp: first guest port
> + * fmp: first machine port
> + * lmp: last machine port
> + */
>  int
> -ioports_permit_access(struct domain *d, unsigned int fp, unsigned int lp)
> +ioports_permit_access(struct domain *d, unsigned int fgp,
> +        unsigned int fmp, unsigned int lmp)
>  {
>      struct io_space *space;
> -    unsigned long mmio_start, mmio_end, mach_start;
> +    unsigned long mmio_start, mach_start, mach_end;
>      int ret;
>  
> -    if (IO_SPACE_NR(fp) >= num_io_spaces) {
> -        dprintk(XENLOG_WARNING, "Unknown I/O Port range 0x%x - 0x%x\n", fp, 
> lp);
> +    if (IO_SPACE_NR(fmp) >= num_io_spaces) {
> +        dprintk(XENLOG_WARNING, "Unknown I/O Port range 0x%x - 0x%x\n", fmp, 
> lmp);
>          return -EFAULT;
>      }
>  
> @@ -1006,42 +1013,44 @@
>       * I/O port spaces and thus will number port spaces differently.
>       * This is ok, they don't make use of this interface.
>       */
> -    ret = rangeset_add_range(d->arch.ioport_caps, fp, lp);
> +    ret = rangeset_add_range(d->arch.ioport_caps, fmp, lmp);
>      if (ret != 0)
>          return ret;
>  
> -    space = &io_space[IO_SPACE_NR(fp)];
> +    space = &io_space[IO_SPACE_NR(fmp)];
>  
>      /* Legacy I/O on dom0 is already setup */
>      if (d == dom0 && space == &io_space[0])
>          return 0;
>  
> -    fp = IO_SPACE_PORT(fp);
> -    lp = IO_SPACE_PORT(lp);
> +    fmp = IO_SPACE_PORT(fmp);
> +    lmp = IO_SPACE_PORT(lmp);
>  
>      if (space->sparse) {
> -        mmio_start = IO_SPACE_SPARSE_ENCODING(fp) & PAGE_MASK;
> -        mmio_end = PAGE_ALIGN(IO_SPACE_SPARSE_ENCODING(lp));
> +        mach_start = IO_SPACE_SPARSE_ENCODING(fmp) & PAGE_MASK;
> +        mach_end = PAGE_ALIGN(IO_SPACE_SPARSE_ENCODING(lmp));
>      } else {
> -        mmio_start = fp & PAGE_MASK;
> -        mmio_end = PAGE_ALIGN(lp);
> +        mach_start = fmp & PAGE_MASK;
> +        mach_end = PAGE_ALIGN(lmp);
>      }
>  
>      /*
>       * The "machine first port" is not necessarily identity mapped
>       * to the guest first port.  At least for the legacy range.
>       */
> -    mach_start = mmio_start | __pa(space->mmio_base);
> +    mach_start = mach_start | __pa(space->mmio_base);
> +    mach_end = mach_end | __pa(space->mmio_base);
>  
> -    if (space == &io_space[0]) {
> +    mmio_start = IO_SPACE_SPARSE_ENCODING(fgp) & PAGE_MASK;
> +
> +    if ( VMX_DOMAIN(d->vcpu[0]))
> +        mmio_start |= LEGACY_IO_START;
> +    else if (space == &io_space[0])
>          mmio_start |= IO_PORTS_PADDR;
> -        mmio_end |= IO_PORTS_PADDR;
> -    } else {
> +    else
>          mmio_start |= __pa(space->mmio_base);
> -        mmio_end |= __pa(space->mmio_base);
> -    }
>  
> -    while (mmio_start <= mmio_end) {
> +    while (mach_start < mach_end) {
>          __assign_domain_page(d, mmio_start,
>                  mach_start, ASSIGN_nocache | ASSIGN_direct_io); 
>          mmio_start += PAGE_SIZE;
> @@ -1091,7 +1100,9 @@
>          mmio_end = PAGE_ALIGN(lp_base);
>      }
>  
> -    if (space == &io_space[0] && d != dom0)
> +    if (VMX_DOMAIN(d->vcpu[0]))
> +        mmio_base = LEGACY_IO_START;
> +    else if (space == &io_space[0] && d != dom0)
>          mmio_base = IO_PORTS_PADDR;
>      else
>          mmio_base = __pa(space->mmio_base);
> @@ -1217,6 +1228,33 @@
>  
>      return mpaddr;
>  }
> +
> +int
> +deassign_domain_mmio_page(struct domain *d, unsigned long mpaddr,
> +        unsigned long phys_addr, unsigned long size )
> +{
> +    unsigned long addr = mpaddr & PAGE_MASK;
> +    unsigned long end = PAGE_ALIGN(mpaddr + size);
> +
> +    if (size == 0) {
> +        gdprintk(XENLOG_INFO, "%s: domain %p mpaddr 0x%lx size = 0x%lx\n",
> +                __func__, d, mpaddr, size);
> +    }
> +    if (!efi_mmio(phys_addr, size)) {
> +#ifndef NDEBUG
> +        gdprintk(XENLOG_INFO, "%s: domain %p mpaddr 0x%lx size = 0x%lx\n",
> +                __func__, d, mpaddr, size);
> +#endif
> +        return -EINVAL;
> +    }
> +
> +    for (; addr < end; addr += PAGE_SIZE ) {
> +        __assign_domain_page(d, addr, 0,
> +                ASSIGN_nocache | ASSIGN_direct_io);
> +    }
> +    return 0;
> +}
> +
>  
>  unsigned long
>  assign_domain_mach_page(struct domain *d,
> diff -r 42c7733c1a2a xen/include/asm-ia64/iocap.h
> --- a/xen/include/asm-ia64/iocap.h    Thu Oct 16 18:18:39 2008 +0800
> +++ b/xen/include/asm-ia64/iocap.h    Thu Oct 16 19:13:01 2008 +0800
> @@ -7,7 +7,7 @@
>  #ifndef __IA64_IOCAP_H__
>  #define __IA64_IOCAP_H__
>  
> -extern int ioports_permit_access(struct domain *d,
> +extern int ioports_permit_access(struct domain *d, unsigned int gs,
>                                unsigned int s, unsigned int e);
>  extern int ioports_deny_access(struct domain *d,
>                              unsigned int s, unsigned int e);
> 
> 

-- 
yamahata

_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ia64-devel