|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] RE: [Xen-ia64-devel][PATCH][VTD] small patches for VTD
Isaku Yamahata wrote:
> Doesn't this break the intention of the c/s 15134:466f71b1e831?a
> To be honest, I'm not sure. Kyouya or Akio, do you have any comments?
>
>
>> maddr = ((maddr & _PAGE_PPN_MASK) & PAGE_MASK) | (paddr &
>> ~PAGE_MASK);
>> diff -r e1ed3e5cd001 xen/arch/ia64/xen/domain.c
>> --- a/xen/arch/ia64/xen/domain.c Tue Oct 21 18:55:22 2008 +0800
>> +++ b/xen/arch/ia64/xen/domain.c Tue Oct 21 19:13:45 2008 +0800
>> @@ -569,6 +569,7 @@ if (is_idle_domain(d))
>> return 0;
>>
>> + INIT_LIST_HEAD(&d->arch.pdev_list);
>> foreign_p2m_init(d);
>> #ifdef CONFIG_XEN_IA64_PERVCPU_VHPT
>> d->arch.has_pervcpu_vhpt = opt_pervcpu_vhpt;
>
> This hunk shoundn't be included in this patch.
Agree
>
>
>> @@ -601,6 +602,9 @@
>> if ((d->arch.mm.pgd = pgd_alloc(&d->arch.mm)) == NULL)
>> goto fail_nomem;
>>
>> + if(iommu_domain_init(d) != 0)
>> + goto fail_iommu;
>> +
>> /*
>> * grant_table_create() can't fully initialize grant table for
>> domain
>> * because it is called before arch_domain_create(). @@ -617,6
>> +621,8 @@ dprintk(XENLOG_DEBUG, "arch_domain_create:
>> domain=%p\n", d); return 0;
>>
>> +fail_iommu:
>> + iommu_domain_destroy(d);
>> fail_nomem:
>> tlb_track_destroy(d);
>> fail_nomem1:
>> @@ -635,6 +641,10 @@
>> if (d->shared_info != NULL)
>> free_xenheap_pages(d->shared_info,
>> get_order_from_shift(XSI_SHIFT)); +
>> + pci_release_devices(d);
>> + if( !is_idle_domain(d))
>> + iommu_domain_destroy(d);
>>
>> tlb_track_destroy(d);
>>
>> diff -r e1ed3e5cd001 xen/arch/ia64/xen/irq.c
>> --- a/xen/arch/ia64/xen/irq.c Tue Oct 21 18:55:22 2008 +0800
>> +++ b/xen/arch/ia64/xen/irq.c Tue Oct 21 19:13:45 2008 +0800 @@
>> -312,12 +312,25 @@ struct domain *guest[IRQ_MAX_GUESTS];
>> } irq_guest_action_t;
>>
>> +static struct timer irq_guest_eoi_timer[NR_IRQS];
>> +static void irq_guest_eoi_timer_fn(void *data)
>> +{
>> + irq_desc_t *desc = data;
>> + unsigned vector = desc - irq_desc;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&desc->lock, flags);
>> + desc->status &= ~IRQ_INPROGRESS;
>> + desc->handler->enable(vector);
>> + spin_unlock_irqrestore(&desc->lock, flags);
>> +}
>> +
>> void __do_IRQ_guest(int irq)
>> {
>> irq_desc_t *desc = &irq_desc[irq];
>> irq_guest_action_t *action = (irq_guest_action_t
>> *)desc->action; struct domain *d; - int
>> i; + int i, already_pending = 0;
>>
>> for ( i = 0; i < action->nr_guests; i++ )
>> {
>> @@ -325,8 +338,29 @@
>> if ( (action->ack_type != ACKTYPE_NONE) &&
>> !test_and_set_bit(irq, &d->pirq_mask) )
>> action->in_flight++;
>> - send_guest_pirq(d, irq);
>> - }
>> + if ( hvm_do_IRQ_dpci(d, irq) )
>> + {
>> + if ( action->ack_type == ACKTYPE_NONE ) +
>> { + already_pending += !!(desc->status &
>> IRQ_INPROGRESS); + desc->status |=
>> IRQ_INPROGRESS; /* cleared during hvm eoi */ + }
>> + } + else if ( send_guest_pirq(d, irq) &&
>> + (action->ack_type == ACKTYPE_NONE) ) +
>> { + already_pending++;
>> + }
>> + }
>> +
>> + if ( already_pending == action->nr_guests )
>> + {
>> + desc->handler->disable(irq);
>> + stop_timer(&irq_guest_eoi_timer[irq]);
>> + init_timer(&irq_guest_eoi_timer[irq],
>> + irq_guest_eoi_timer_fn, desc,
>> smp_processor_id()); +
>> set_timer(&irq_guest_eoi_timer[irq], NOW() + MILLISECS(1)); + }
>> }
>>
>> int pirq_acktype(int irq)
>
> Is those hunk for interrupt storm avoidance as 17963:1db0b09b290e?
> If so, please split it out and send it as another patch.
It is different,
17963 is used to prevent interrupt storm of host MSI, xen/ia64 don't support
MSI so far.
This code segment is used to handle interrupt sharing between multiple domain.
>
>
>> diff -r e1ed3e5cd001 xen/arch/ia64/xen/mm.c
>> --- a/xen/arch/ia64/xen/mm.c Tue Oct 21 18:55:22 2008 +0800
>> +++ b/xen/arch/ia64/xen/mm.c Tue Oct 21 19:13:45 2008 +0800 @@
>> -189,6 +189,10 @@
>>
>> static void __xencomm_mark_dirty(struct domain *d,
>> unsigned long addr, unsigned int
>> len); + +static void
>> +zap_domain_page_one(struct domain *d, unsigned long mpaddr,
>> + int clear_PGC_allocate, unsigned long mfn);
>>
>> extern unsigned long ia64_iobase;
>>
>> @@ -908,20 +912,21 @@
>> unsigned long flags)
>> {
>> volatile pte_t *pte;
>> - pte_t old_pte;
>> pte_t new_pte;
>> pte_t ret_pte;
>> unsigned long prot = flags_to_prot(flags);
>>
>> pte = lookup_alloc_domain_pte(d, mpaddr);
>> + new_pte = pfn_pte(physaddr >> PAGE_SHIFT, __pgprot(prot));
>> + if(pte_val(new_pte) == pte_val(*pte))
>> + return 0;
>>
>> - old_pte = __pte(0);
>> - new_pte = pfn_pte(physaddr >> PAGE_SHIFT, __pgprot(prot));
>> - ret_pte = ptep_cmpxchg_rel(&d->arch.mm, mpaddr, pte, old_pte,
>> new_pte);
>> - if (pte_val(ret_pte) == pte_val(old_pte)) {
>> - smp_mb();
>> - return 0;
>> - }
>> + /* for assigned MMIO, the old pte may be set to _PAGE_IO
>> attribute, + * so zap it first, then set up it.
>> + */
>> +
>> + zap_domain_page_one(d, mpaddr, 1, INVALID_MFN);
>> + ret_pte = ptep_xchg(&d->arch.mm, mpaddr, pte, new_pte);
>>
>> // dom0 tries to map real machine's I/O region, but failed.
>> // It is very likely that dom0 doesn't boot correctly because
>
> Hmmm, are you really sure that the above is SMP-safe?
> We are touching p2m table locklessly so we must be extremely
> careful. The above hunk split the atomic operation into two phase
> and makes the following logic not make sense.
>
>
Yes, it is not SMP-safe there is lock for p2m.
Modifying p2m is not a frequent operation, why not add a lock for it?
>> if (mfn == INVALID_MFN) {
>> // clear pte
>> old_pte = ptep_get_and_clear(mm, mpaddr, pte);
>>+ if(!pte_mem(old_pte))
>>+ return;
>> mfn = pte_pfn(old_pte);
>> } else {
>> unsigned long old_arflags;
>> @@ -1461,6 +1468,7 @@
>> if(!mfn_valid(mfn))
>> return;
>>
>> + iommu_unmap_page(d, mpaddr>>PAGE_SHIFT);
>Isn't PAGE_SHIFT_4K loop necessary?
Good catch!
Thanks
Anthony
_______________________________________________
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 |