On Thu, Mar 24, 2016 at 01:57:58PM +0800, Quan Xu wrote:
Hey!
Thanks for the patch!
I see that you have __must_check..
But if you check the callchain:
iommu_flush_iec_[index|global] ->
__iommu_flush_iec->invalidate_sync-> queue_invalidate_wait
you will see that the callers of iommu_flush_iec_[index|global]
ignore the return value.
So ... perhaps you could explain in this commit description on how to address
that? Is there an followup patch (if so just put in the name in here) to
address that?
Or should the top callers: enable_intremap, ioapic_rte_to_remap_entry,
free_remap_entry, msi_msg_to_remap_entry, and pi_update_irte do something?
I guess the 'free_remap_entry' reall can't. As you are suppose to always
be able to free something.
msi_msg_to_remap_entry, _msg_to_remap_entry, and ioapic_rte_to_remap_entry
could return an value... Or considering this is v8 - was there some epic
conversation that went over this quite a lot? In which case I would recommend
you say why it was not attempted this way so that folks six months from
now when reading this patch won't ask again.
> Signed-off-by: Quan Xu <quan...@intel.com>
> ---
> docs/misc/xen-command-line.markdown | 7 +++++++
> xen/drivers/passthrough/vtd/qinval.c | 17 ++++++++++++-----
> 2 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/docs/misc/xen-command-line.markdown
> b/docs/misc/xen-command-line.markdown
> index ca77e3b..384dde7 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1532,6 +1532,13 @@ Note that if **watchdog** option is also specified
> vpmu will be turned off.
> As the virtualisation is not 100% safe, don't use the vpmu flag on
> production systems (see http://xenbits.xen.org/xsa/advisory-163.html)!
>
> +### vtd\_qi\_timeout (VT-d)
> +> `= <integer>`
> +
> +> Default: `1`
> +
> +Specify the timeout of the VT-d Queued Invalidation in milliseconds.
> +
> ### watchdog
> > `= force | <boolean>`
>
> diff --git a/xen/drivers/passthrough/vtd/qinval.c
> b/xen/drivers/passthrough/vtd/qinval.c
> index b81b0bd..52ba2c2 100644
> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -28,6 +28,11 @@
> #include "vtd.h"
> #include "extern.h"
>
> +static unsigned int __read_mostly vtd_qi_timeout = 1;
> +integer_param("vtd_qi_timeout", vtd_qi_timeout);
> +
> +#define IOMMU_QI_TIMEOUT (vtd_qi_timeout * MILLISECS(1))
> +
> static void print_qi_regs(struct iommu *iommu)
> {
> u64 val;
> @@ -130,10 +135,10 @@ static void queue_invalidate_iotlb(struct iommu *iommu,
> spin_unlock_irqrestore(&iommu->register_lock, flags);
> }
>
> -static int queue_invalidate_wait(struct iommu *iommu,
> +static int __must_check queue_invalidate_wait(struct iommu *iommu,
> u8 iflag, u8 sw, u8 fn)
> {
> - s_time_t start_time;
> + s_time_t timeout;
> volatile u32 poll_slot = QINVAL_STAT_INIT;
> unsigned int index;
> unsigned long flags;
> @@ -164,13 +169,15 @@ static int queue_invalidate_wait(struct iommu *iommu,
> if ( sw )
> {
> /* In case all wait descriptor writes to same addr with same data */
> - start_time = NOW();
> + timeout = NOW() + IOMMU_QI_TIMEOUT;
> while ( poll_slot != QINVAL_STAT_DONE )
> {
> - if ( NOW() > (start_time + DMAR_OPERATION_TIMEOUT) )
> + if ( NOW() > timeout )
> {
> print_qi_regs(iommu);
> - panic("queue invalidate wait descriptor was not executed");
> + printk(XENLOG_WARNING VTDPREFIX
> + "Queue invalidate wait descriptor timed out.\n");
> + return -ETIMEDOUT;
> }
> cpu_relax();
> }
> --
> 1.9.1
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
