|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] 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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |