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

To: Isaku Yamahata <yamahata@xxxxxxxxxxxxx>
Subject: RE: [Xen-ia64-devel][PATCH][VTD]change ioports_permit_access interface
From: "Xu, Anthony" <anthony.xu@xxxxxxxxx>
Date: Fri, 17 Oct 2008 17:07:47 +0800
Accept-language: en-US
Acceptlanguage: en-US
Cc: xen-ia64-devel <xen-ia64-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Fri, 17 Oct 2008 02:08:02 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <20081017085845.GK17594%yamahata@xxxxxxxxxxxxx>
List-help: <mailto:xen-ia64-devel-request@lists.xensource.com?subject=help>
List-id: Discussion of the ia64 port of Xen <xen-ia64-devel.lists.xensource.com>
List-post: <mailto:xen-ia64-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-ia64-devel>, <mailto:xen-ia64-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-ia64-devel>, <mailto:xen-ia64-devel-request@lists.xensource.com?subject=unsubscribe>
References: <F7C8A4D3A9905B45A80E4C194793FA6501AE5334A2@xxxxxxxxxxxxxxxxxxxxxxxxxxxx> <20081017065507.GE17594%yamahata@xxxxxxxxxxxxx> <F7C8A4D3A9905B45A80E4C194793FA6501AE533A48@xxxxxxxxxxxxxxxxxxxxxxxxxxxx> <20081017075600.GJ17594%yamahata@xxxxxxxxxxxxx> <F7C8A4D3A9905B45A80E4C194793FA6501AE533B01@xxxxxxxxxxxxxxxxxxxxxxxxxxxx> <20081017085845.GK17594%yamahata@xxxxxxxxxxxxx>
Sender: xen-ia64-devel-bounces@xxxxxxxxxxxxxxxxxxx
Thread-index: AckwNppP21gPK0gNR22U/HLKpur0bQAASiaw
Thread-topic: [Xen-ia64-devel][PATCH][VTD]change ioports_permit_access interface
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?
Yes it returns NULL

> or
> Did you hit BUG_ON(page_get_owner(page) == NULL)?
>
>
>
>>
>> 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

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