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

["Xu, Anthony" <anthony.xu@xxxxxxxxx>]

Hi isaku,

If I remembered correctly, last time I encountered issue at line page = 
mfn_to_page(mfn);
Page returns 0.

I suspect if page_info for IOPORTs is initialized.
Any idea?

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 < 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

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