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

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

To: Isaku Yamahata <yamahata@xxxxxxxxxxxxx>
Subject: [PATCH]RE: [Xen-ia64-devel][PATCH][VTD]change ioports_permit_access interface
From: "Xu, Anthony" <anthony.xu@xxxxxxxxx>
Date: Fri, 17 Oct 2008 16:17:08 +0800
Accept-language: en-US
Acceptlanguage: en-US
Cc: xen-ia64-devel <xen-ia64-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Fri, 17 Oct 2008 01:17:27 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <20081017075600.GJ17594%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>
Sender: xen-ia64-devel-bounces@xxxxxxxxxxxxxxxxxxx
Thread-index: AckwLdG00JWWuXJbRC+Wmo0tCXuyTAAAtPcQ
Thread-topic: [PATCH]RE: [Xen-ia64-devel][PATCH][VTD]change ioports_permit_access interface
use VTD to assing device, guest port may not be equal to host port.
Change ioports_permit_access interface

Signed-off-by; Anthony Xu < anthony.xu@xxxxxxxxx >



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

Attachment: change_ioports_permit_access.patch
Description: change_ioports_permit_access.patch

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