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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

> >
> >> +    }
> >> +    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?

> >> +        vvtd_handle_fault(vvtd, irq, NULL, VTD_FR_IR_REQ_RSVD, 
> >> record_fault);
> >> +        return -EINVAL;
> >> +    }
> >> +
> >> +    if ( entry > vvtd->status.irt_max_entry )
> >> +    {
> >> +        vvtd_handle_fault(vvtd, irq, NULL, VTD_FR_IR_INDEX_OVER, 
> >> record_fault);
> >> +        return -EACCES;
> >> +    }
> >> +
> >> +    irt_page = map_guest_page(vvtd->domain,
> >> +                              vvtd->status.irt + (entry >> 
> >> IREMAP_ENTRY_ORDER));
> >
> >Since AFAICT you have to read this page(s) every time an interrupt
> >needs to be delivered, wouldn't it make sense for performance reasons
> >to have the page permanently mapped?
> 
> Yes. It is. Actually, we have a draft patch to do this. But to justify
> the necessity, I should run some benchmark at first. Mapping a guest
> page is slow on x86, right?

The issue is the tblflush, not the actual modifications of the page
tables.

> >
> >What's the maximum number of pages that can be used here?
> 
> VT-d current support 2^16 entries at most. The size of each entry is 128
> byte. Thus, we need 2^11 pages at most.

Those are guest pages at the end, so it shouldn't be a problem.

Roger.

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