Re: [PATCH V3 28/29] x86/vvtd: Add queued invalidation (QI) support

[Chao Gao <chao.gao@intel.com>]

On Fri, Oct 20, 2017 at 12:20:06PM +0100, Roger Pau Monné wrote:
>On Thu, Sep 21, 2017 at 11:02:09PM -0400, Lan Tianyu wrote:
>> From: Chao Gao <chao.gao@intel.com>
>> 
>> Queued Invalidation Interface is an expanded invalidation interface with
>> extended capabilities. Hardware implementations report support for queued
>> invalidation interface through the Extended Capability Register. The queued
>> invalidation interface uses an Invalidation Queue (IQ), which is a circular
>> buffer in system memory. Software submits commands by writing Invalidation
>> Descriptors to the IQ.
>> 
>> In this patch, a new function viommu_process_iq() is used for emulating how
>> hardware handles invalidation requests through QI.
>> 
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>> ---
>> +static int process_iqe(struct vvtd *vvtd, int i)
>
>unsigned int.
>
>> +{
>> +    uint64_t iqa;
>> +    struct qinval_entry *qinval_page;
>> +    int ret = 0;
>> +
>> +    iqa = vvtd_get_reg_quad(vvtd, DMAR_IQA_REG);
>> +    qinval_page = map_guest_page(vvtd->domain, DMA_IQA_ADDR(iqa)>>PAGE_SHIFT);
>
>PFN_DOWN instead of open coding the shift. Both can be initialized
>at declaration. Also AFAICT iqa is only used once, so the local
>variable is not needed.
>
>> +    if ( IS_ERR(qinval_page) )
>> +    {
>> +        gdprintk(XENLOG_ERR, "Can't map guest IRT (rc %ld)",
>> +                 PTR_ERR(qinval_page));
>> +        return PTR_ERR(qinval_page);
>> +    }
>> +
>> +    switch ( qinval_page[i].q.inv_wait_dsc.lo.type )
>> +    {
>> +    case TYPE_INVAL_WAIT:
>> +        if ( qinval_page[i].q.inv_wait_dsc.lo.sw )
>> +        {
>> +            uint32_t data = qinval_page[i].q.inv_wait_dsc.lo.sdata;
>> +            uint64_t addr = (qinval_page[i].q.inv_wait_dsc.hi.saddr << 2);
>
>Unneeded parentheses.
>
>> +
>> +            ret = hvm_copy_to_guest_phys(addr, &data, sizeof(data), current);
>> +            if ( ret )
>> +                vvtd_info("Failed to write status address");
>
>Don't you need to return or do something here? (like raise some kind
>of error?)

The 'addr' is programmed by guest. Here vvtd cannot finish this write
for some reason (i.e. the 'addr' may be not in the guest physical memory space).
According to VT-d spec 6.5.2.8 Invalidation Wait Descriptor, "Hardware
behavior is undefined if the Status Address specified is not an address
route-able to memory (such as peer address, interrupt address range of
0xFEEX_XXXX etc.) I think that Xen can just ignore it. I should use
vvtd_debug() for it is guest triggerable.

>> +                if ( !vvtd_test_bit(vvtd, DMAR_IECTL_REG, DMA_IECTL_IM_SHIFT) )
>> +                {
>> +                    ie_data = vvtd_get_reg(vvtd, DMAR_IEDATA_REG);
>> +                    ie_addr = vvtd_get_reg(vvtd, DMAR_IEADDR_REG);
>> +                    vvtd_generate_interrupt(vvtd, ie_addr, ie_data);
>
>...you don't seem two need the two local variables. They are used only
>once.
>
>> +                    vvtd_clear_bit(vvtd, DMAR_IECTL_REG, DMA_IECTL_IP_SHIFT);
>> +                }
>> +            }
>> +        }
>> +        break;
>> +
>> +    case TYPE_INVAL_IEC:
>> +        /*
>> +         * Currently, no cache is preserved in hypervisor. Only need to update
>> +         * pIRTEs which are modified in binding process.
>> +         */
>> +        break;
>> +
>> +    default:
>> +        goto error;
>
>There's no reason to use a label that's only used for the default
>case. Simply place the code in the error label here.
>
>> +    }
>> +
>> +    unmap_guest_page((void*)qinval_page);
>> +    return ret;
>> +
>> + error:
>> +    unmap_guest_page((void*)qinval_page);
>> +    gdprintk(XENLOG_ERR, "Internal error in Queue Invalidation.\n");
>> +    domain_crash(vvtd->domain);
>
>Do you really need to crash the domain in such case?

We reach here when guest requests some operations vvtd doesn't claim
supported or emulated. I am afraid it also can be triggered by guest.
How about ignoring the invalidation request?

I will change the error message for it isn't internal error.

Thanks
Chao

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