Isaku Yamahata wrote:
> On Fri, Oct 17, 2008 at 05:58:45PM +0900, Isaku Yamahata wrote:
>> On Fri, Oct 17, 2008 at 04:45:09PM +0800, Xu, Anthony wrote:
>>> Hi isaku,
>>>
>>> If I remembered correctly, last time I encountered issue at line
>>> page = mfn_to_page(mfn); Page returns 0.
>>
>> Which line do you have trouble?
>> Do you mean that mfn_to_page(mfn) returned NULL?
>> or
>> Did you hit BUG_ON(page_get_owner(page) == NULL)?
>>
>>
>>
>>>
>>> I suspect if page_info for IOPORTs is initialized.
>>> Any idea?
>
> Ah, I think they are initialized, but they are free.
> i.e. page refcount = 0, page owner = NULL.
> So you may need to make them owned by DOMID_IO somewhere of the
> initialization.
> i.e. page refcount = 1, page owner = DOMID_IO

I suspect it,
Because xen doesn't know all MMIO information, xen just initiates page_info 
through efi memory description/
Efi memory description doesn't provide MMIO information execpt PORT IO 
information.


Anthony


>
>>>
>>> Thanks,
>>> Anthony
>>>
>>>
>>> mm_teardown_pte(struct domain* d, volatile pte_t* pte, unsigned
>>>     long offset) { pte_t old_pte;
>>>     unsigned long mfn;
>>>     struct page_info* page;
>>>
>>>     old_pte = ptep_get_and_clear(&d->arch.mm, offset, pte);//
>>> acquire semantics
>>>
>>>     // vmx domain use bit[58:56] to distinguish io region from
>>>     memory. // see vmx_build_physmap_table() in vmx_init.c
>>>     if (!pte_mem(old_pte))
>>>         return;
>>>
>>>     // domain might map IO space or acpi table pages. check it.
>>>     mfn = pte_pfn(old_pte); if (!mfn_valid(mfn))
>>>         return;
>>>     page = mfn_to_page(mfn);
>>>     BUG_ON(page_get_owner(page) == NULL);
>>>
>>>
>>>
>>>
>>> Isaku Yamahata wrote:
>>>> 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 < [EMAIL PROTECTED] >
>>>>>>
>>>>>> 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 < [EMAIL PROTECTED] >
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 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@lists.xensource.com
>>>>> http://lists.xensource.com/xen-ia64-devel
>>>
>>> _______________________________________________
>>> Xen-ia64-devel mailing list
>>> Xen-ia64-devel@lists.xensource.com
>>> http://lists.xensource.com/xen-ia64-devel

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

Reply via email to