Re: [Xen-devel] [PATCH V3 15/29] x86/vvtd: Process interrupt remapping request

[Chao Gao <chao.gao@xxxxxxxxx>]

On Fri, Oct 20, 2017 at 11:01:03AM +0100, Roger Pau Monné wrote:
>On Fri, Oct 20, 2017 at 01:16:37PM +0800, Chao Gao wrote:
>> On Thu, Oct 19, 2017 at 03:26:30PM +0100, Roger Pau Monné wrote:
>> >On Thu, Sep 21, 2017 at 11:01:56PM -0400, Lan Tianyu wrote:
>> >> +static void unmap_guest_page(void *virt)
>> >> +{
>> >> +    struct page_info *page;
>> >> +
>> >> +    ASSERT((unsigned long)virt & PAGE_MASK);
>> >
>> >I'm not sure I get the point of the check above.
>> 
>> I intended to check the address is 4K-page aligned. It should be
>> 
>> ASSERT(!((unsigned long)virt & (PAGE_SIZE - 1)))
>
>Please use the IS_ALIGNED macro.

Ok.

>
>> >
>> >> +    }
>> >> +    return;
>> >> +}
>> >> +
>> >> +static bool vvtd_irq_request_sanity_check(const struct vvtd *vvtd,
>> >> +                                          struct 
>> >> arch_irq_remapping_request *irq)
>> >> +{
>> >> +    if ( irq->type == VIOMMU_REQUEST_IRQ_APIC )
>> >> +    {
>> >> +        struct IO_APIC_route_remap_entry rte = { .val = irq->msg.rte };
>> >> +
>> >> +        ASSERT(rte.format);
>> >
>> >Is it fine to ASSERT here? Can't the guest set rte.format to whatever
>> >it wants?
>> 
>> Guest can use legacy format interrupt (i.e. rte.format = 0). However,
>> we only reach here when callback 'check_irq_remapping' return true and
>> for vvtd, 'check_irq_remapping' just returns the format bit of irq request.
>> If here ret.format isn't true, there must be a bug in our code.
>
>Are you sure the correct locks are hold here to prevent the guest
>from changing rte while all this processing is happening?

The rte here isn't the registers in IOAPIC. It is only (or part of) the
interrupt request (abstract of ioapic rte and msi message). Every time
an interrupt is to be delivered, the interrupt request is composed
according the IOAPIC RTE or MSI message on stack. Then we recognize
the format of the interrupt, means remapping format or not remapping
format. Only for remapping format, the function is called. For
non-remapping format, the interrupt is delivered by ioapic directly and
needn't come here and be translated by vIOMMU.

Thanks
Chao

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel