WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-ia64-devel

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

On Fri, Oct 17, 2008 at 03:22:18PM +0800, Xu, Anthony wrote:
> Isaku Yamahata wrote:
> > 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?
> In x86 side, only it is identity mapping, guest can access the port directly.
> If it is not identity mapping, guest can not access the port directly, at 
> this situation,
>  xen will intercept io port access first, and xen access the corresponding 
> physical port.
> 
> In ia64 side, guest can acess all assigned port whenever it is identity 
> mapping or not.
> So we need to change the interface.

Understood. x86 have io instructions.
Okay, please split out ioports_permit_access(), then I'll take 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().
> 
> There is no page_info for mmio page, I didn't see reference count issue.
> 
> If VTD is enabled, in the life of guest, p2m can not be changed,
> Because it VTD operation hit a page table miss, the DMA operation can not be 
> resumed.

Hmm, it is possible for qemu-dm (or any tools stack in dom0) can
issue racy hypercall, isn't it.
Anyway __assgin_domain_page() isn't assumed to use for deassigning
page. (Sorry, I have to admit those functions are somewhat confusing...)
How about the following patch?  I did only compile test, though.
With the following modification zap_domain_page_one() could be used
by deassign_domain_mmio_page().
Probably you may want to twist mfn_valid() with is_iomem_page() or
something.

thanks,

diff -r 7db30bf36b0e xen/arch/ia64/xen/mm.c
--- a/xen/arch/ia64/xen/mm.c    Fri Oct 17 15:33:03 2008 +0900
+++ b/xen/arch/ia64/xen/mm.c    Fri Oct 17 16:58:34 2008 +0900
@@ -1422,8 +1422,9 @@
     again:
         // memory_exchange() calls guest_physmap_remove_page() with
         // a stealed page. i.e. page owner = NULL.
-        BUG_ON(page_get_owner(mfn_to_page(mfn)) != d &&
-               page_get_owner(mfn_to_page(mfn)) != NULL);
+        BUG_ON(mfn_valid(mfn) &&
+               (page_get_owner(mfn_to_page(mfn)) != d &&
+                page_get_owner(mfn_to_page(mfn)) != NULL));
         old_arflags = pte_val(*pte) & ~_PAGE_PPN_MASK;
         old_pte = pfn_pte(mfn, __pgprot(old_arflags));
         new_pte = __pte(0);
@@ -1445,12 +1446,15 @@
         BUG_ON(mfn != pte_pfn(ret_pte));
     }
 
+    perfc_incr(zap_domain_page_one);
+    if (!mfn_valid(mfn))
+        return;
+
     page = mfn_to_page(mfn);
     BUG_ON((page->count_info & PGC_count_mask) == 0);
 
     BUG_ON(clear_PGC_allocate && (page_get_owner(page) == NULL));
     domain_put_page(d, mpaddr, pte, old_pte, clear_PGC_allocate);
-    perfc_incr(zap_domain_page_one);
 }
 
 unsigned long


> 
> 
> Thanks,
> Anthony
> 
> >
> > - 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);
> 
> _______________________________________________
> Xen-ia64-devel mailing list
> Xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-ia64-devel
> 

-- 
yamahata

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