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