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

[Chao Gao <chao.gao@xxxxxxxxx>]

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:
>> From: Chao Gao <chao.gao@xxxxxxxxx>
>> 
>> When a remapping interrupt request arrives, remapping hardware computes the
>> interrupt_index per the algorithm described in VTD spec
>> "Interrupt Remapping Table", interprets the IRTE and generates a remapped
>> interrupt request.
>> 
>> This patch introduces viommu_handle_irq_request() to emulate the process how
>> remapping hardware handles a remapping interrupt request.
>> 
>> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
>> Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
>> 
>> ---
>>  
>> +enum VTD_FAULT_TYPE
>> +{
>> +    /* Interrupt remapping transition faults */
>> +    VTD_FR_IR_REQ_RSVD      = 0x20, /* One or more IR request reserved
>> +                                     * fields set */
>> +    VTD_FR_IR_INDEX_OVER    = 0x21, /* Index value greater than max */
>> +    VTD_FR_IR_ENTRY_P       = 0x22, /* Present (P) not set in IRTE */
>> +    VTD_FR_IR_ROOT_INVAL    = 0x23, /* IR Root table invalid */
>> +    VTD_FR_IR_IRTE_RSVD     = 0x24, /* IRTE Rsvd field non-zero with
>> +                                     * Present flag set */
>> +    VTD_FR_IR_REQ_COMPAT    = 0x25, /* Encountered compatible IR
>> +                                     * request while disabled */
>> +    VTD_FR_IR_SID_ERR       = 0x26, /* Invalid Source-ID */
>> +};
>
>Why does this need to be an enum? Plus enum type names should not be
>all in uppercase.
>
>In any case, I would just use defines, like it's done for all other
>values in the file.

Sure. Will follow your suggestion.

>> +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)))

>> +}
>> +
>> +static inline uint32_t irte_dest(struct vvtd *vvtd, uint32_t dest)
>> +{
>> +    /* In xAPIC mode, only 8-bits([15:8]) are valid */
>> +    return vvtd->status.eim_enabled ? dest
>                                       : MASK_EXTR(dest, IRTE_xAPIC_DEST_MASK);
>
>It's easier to read style wise.

sure.

>
>> +}
>> +
>>  static void vvtd_handle_gcmd_ire(struct vvtd *vvtd, uint32_t val)
>>  {
>>      vvtd_info("%sable Interrupt Remapping",
>> @@ -255,6 +387,135 @@ static const struct hvm_mmio_ops vvtd_mmio_ops = {
>>      .write = vvtd_write
>>  };
>>  
>> +static void vvtd_handle_fault(struct vvtd *vvtd,
>> +                              struct arch_irq_remapping_request *irq,
>> +                              struct iremap_entry *irte,
>> +                              unsigned int fault,
>> +                              bool record_fault)
>> +{
>> +   if ( !record_fault )
>> +        return;
>> +
>> +    switch ( fault )
>> +    {
>> +    case VTD_FR_IR_SID_ERR:
>> +    case VTD_FR_IR_IRTE_RSVD:
>> +    case VTD_FR_IR_ENTRY_P:
>> +        if ( qinval_fault_disable(*irte) )
>> +            break;
>> +    /* fall through */
>> +    case VTD_FR_IR_INDEX_OVER:
>> +    case VTD_FR_IR_ROOT_INVAL:
>> +        /* TODO: handle fault (e.g. record and report this fault to VM */
>> +        break;
>> +
>> +    default:
>> +        gdprintk(XENLOG_INFO, "Can't handle VT-d fault %x\n", fault);
>
>You already defined some vvtd specific debug helpers, why are those
>not used here? gdprintk (as the 'd' denotes) is only for debug
>purposes.

The default case means we encounter a bug in our code. I want to output
this kind of message even for non-debug version. I should use gprintk.

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

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

>
>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.

>
>> +    if ( IS_ERR(irt_page) )
>> +    {
>> +        vvtd_handle_fault(vvtd, irq, NULL, VTD_FR_IR_ROOT_INVAL, 
>> record_fault);
>> +        return PTR_ERR(irt_page);
>> +    }
>> +
>> +    irte = irt_page + (entry % (1 << IREMAP_ENTRY_ORDER));
>> +    dest->val = irte->val;
>
>Not that it matters much, but for coherency reasons I would only set
>dest->val after all the checks have been performed.

agree.

Thanks
chao

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