Re: [Xen-devel] [PATCH v10 2/3] vt-d: synchronize for Device-TLB flush one by one
On May 18, 2016 5:29 PM, Jan Beulichwrote: > >>> On 18.05.16 at 10:53, wrote: > > On May 17, 2016 8:37 PM, Jan Beulich wrote: > >> >>> On 22.04.16 at 12:54, wrote: > >> > -static void queue_invalidate_iotlb(struct iommu *iommu, > >> > -u8 granu, u8 dr, u8 dw, u16 did, u8 am, u8 ih, u64 addr) > >> > +static int __must_check queue_invalidate_iotlb_sync(struct iommu > >> *iommu, > >> > +u8 granu, u8 dr, u8 > >> > dw, > >> > +u16 did, u8 am, u8 > >> > ih, > >> > +u64 addr) > >> > { > >> > unsigned long flags; > >> > unsigned int index; > >> > @@ -133,10 +141,12 @@ static void queue_invalidate_iotlb(struct > >> > iommu > >> *iommu, > >> > unmap_vtd_domain_page(qinval_entries); > >> > qinval_update_qtail(iommu, index); > >> > spin_unlock_irqrestore(>register_lock, flags); > >> > + > >> > +return invalidate_sync(iommu); > >> > } > >> > >> With this, ... > >> > >> > @@ -346,9 +353,13 @@ static int flush_iotlb_qi( > >> > if (cap_read_drain(iommu->cap)) > >> > dr = 1; > >> > /* Need to conside the ih bit later */ > >> > -queue_invalidate_iotlb(iommu, > >> > - type >> DMA_TLB_FLUSH_GRANU_OFFSET, dr, > >> > - dw, did, size_order, 0, addr); > >> > +ret = queue_invalidate_iotlb_sync(iommu, > >> > + type >> > >> > DMA_TLB_FLUSH_GRANU_OFFSET, > >> > + dr, dw, did, size_order, > >> > + 0, addr); > >> > + > >> > +if ( ret ) > >> > +return ret; > >> > + > >> > if ( flush_dev_iotlb ) > >> > ret = dev_invalidate_iotlb(iommu, did, addr, size_order, > >> > type); > >> > rc = invalidate_sync(iommu); > >> > >> ... why does this invalidate_sync() not go away? > >> > > > > Oh, it is your suggestion -- leaving the existing logic as is would be > > better - best effort invalidation even when an error has occurred. > > > > http://lists.xenproject.org/archives/html/xen-devel/2016-04/msg00523.h > > tml > > Look like this was a bad comment of mine (resulting from > dev_invalidate_iotlb(), other than the other respective functions, not > getting a > _sync tag added), and I would have appreciated if you had simply pointed out > the redundancy. I just issued an open for this point in v9 discussion. I felt a strange, but really didn't have obvious reasons at that time. -- I'll drop this invalidate_sync() in v11. > Please remember that the review process is bi-directional, > and hence doesn't mean you need to blindly do everything a reviewer asks for: > Things you agree with should be changed in code. For things you don't agree > with you should reply verbally, explaining why a requested change shouldn't > be done. > Thanks. I will try to follow it. Quan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v10 2/3] vt-d: synchronize for Device-TLB flush one by one
>>> On 18.05.16 at 10:53,wrote: > On May 17, 2016 8:37 PM, Jan Beulich wrote: >> >>> On 22.04.16 at 12:54, wrote: >> > -static void queue_invalidate_iotlb(struct iommu *iommu, >> > -u8 granu, u8 dr, u8 dw, u16 did, u8 am, u8 ih, u64 addr) >> > +static int __must_check queue_invalidate_iotlb_sync(struct iommu >> *iommu, >> > +u8 granu, u8 dr, u8 >> > dw, >> > +u16 did, u8 am, u8 ih, >> > +u64 addr) >> > { >> > unsigned long flags; >> > unsigned int index; >> > @@ -133,10 +141,12 @@ static void queue_invalidate_iotlb(struct iommu >> *iommu, >> > unmap_vtd_domain_page(qinval_entries); >> > qinval_update_qtail(iommu, index); >> > spin_unlock_irqrestore(>register_lock, flags); >> > + >> > +return invalidate_sync(iommu); >> > } >> >> With this, ... >> >> > @@ -346,9 +353,13 @@ static int flush_iotlb_qi( >> > if (cap_read_drain(iommu->cap)) >> > dr = 1; >> > /* Need to conside the ih bit later */ >> > -queue_invalidate_iotlb(iommu, >> > - type >> DMA_TLB_FLUSH_GRANU_OFFSET, dr, >> > - dw, did, size_order, 0, addr); >> > +ret = queue_invalidate_iotlb_sync(iommu, >> > + type >> >> > DMA_TLB_FLUSH_GRANU_OFFSET, >> > + dr, dw, did, size_order, 0, >> > addr); >> > + >> > +if ( ret ) >> > +return ret; >> > + >> > if ( flush_dev_iotlb ) >> > ret = dev_invalidate_iotlb(iommu, did, addr, size_order, >> > type); >> > rc = invalidate_sync(iommu); >> >> ... why does this invalidate_sync() not go away? >> > > Oh, it is your suggestion -- leaving the existing logic as is would be better > - > best effort invalidation even when an error has occurred. > > http://lists.xenproject.org/archives/html/xen-devel/2016-04/msg00523.html Look like this was a bad comment of mine (resulting from dev_invalidate_iotlb(), other than the other respective functions, not getting a _sync tag added), and I would have appreciated if you had simply pointed out the redundancy. Please remember that the review process is bi-directional, and hence doesn't mean you need to blindly do everything a reviewer asks for: Things you agree with should be changed in code. For things you don't agree with you should reply verbally, explaining why a requested change shouldn't be done. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v10 2/3] vt-d: synchronize for Device-TLB flush one by one
On May 17, 2016 8:37 PM, Jan Beulichwrote: > >>> On 22.04.16 at 12:54, wrote: > > --- a/xen/drivers/passthrough/vtd/qinval.c > > +++ b/xen/drivers/passthrough/vtd/qinval.c > > @@ -33,6 +33,8 @@ integer_param("vtd_qi_timeout", vtd_qi_timeout); > > > > #define IOMMU_QI_TIMEOUT (vtd_qi_timeout * MILLISECS(1)) > > > > +static int invalidate_sync(struct iommu *iommu); > > __must_check? > Yes, I will add it. > > -static void queue_invalidate_iotlb(struct iommu *iommu, > > -u8 granu, u8 dr, u8 dw, u16 did, u8 am, u8 ih, u64 addr) > > +static int __must_check queue_invalidate_iotlb_sync(struct iommu > *iommu, > > +u8 granu, u8 dr, u8 dw, > > +u16 did, u8 am, u8 ih, > > +u64 addr) > > { > > unsigned long flags; > > unsigned int index; > > @@ -133,10 +141,12 @@ static void queue_invalidate_iotlb(struct iommu > *iommu, > > unmap_vtd_domain_page(qinval_entries); > > qinval_update_qtail(iommu, index); > > spin_unlock_irqrestore(>register_lock, flags); > > + > > +return invalidate_sync(iommu); > > } > > With this, ... > > > @@ -346,9 +353,13 @@ static int flush_iotlb_qi( > > if (cap_read_drain(iommu->cap)) > > dr = 1; > > /* Need to conside the ih bit later */ > > -queue_invalidate_iotlb(iommu, > > - type >> DMA_TLB_FLUSH_GRANU_OFFSET, dr, > > - dw, did, size_order, 0, addr); > > +ret = queue_invalidate_iotlb_sync(iommu, > > + type >> > > DMA_TLB_FLUSH_GRANU_OFFSET, > > + dr, dw, did, size_order, 0, > > addr); > > + > > +if ( ret ) > > +return ret; > > + > > if ( flush_dev_iotlb ) > > ret = dev_invalidate_iotlb(iommu, did, addr, size_order, type); > > rc = invalidate_sync(iommu); > > ... why does this invalidate_sync() not go away? > Oh, it is your suggestion -- leaving the existing logic as is would be better - best effort invalidation even when an error has occurred. http://lists.xenproject.org/archives/html/xen-devel/2016-04/msg00523.html Quan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v10 2/3] vt-d: synchronize for Device-TLB flush one by one
>>> On 22.04.16 at 12:54,wrote: > --- a/xen/drivers/passthrough/vtd/qinval.c > +++ b/xen/drivers/passthrough/vtd/qinval.c > @@ -33,6 +33,8 @@ integer_param("vtd_qi_timeout", vtd_qi_timeout); > > #define IOMMU_QI_TIMEOUT (vtd_qi_timeout * MILLISECS(1)) > > +static int invalidate_sync(struct iommu *iommu); __must_check? > -static void queue_invalidate_iotlb(struct iommu *iommu, > -u8 granu, u8 dr, u8 dw, u16 did, u8 am, u8 ih, u64 addr) > +static int __must_check queue_invalidate_iotlb_sync(struct iommu *iommu, > +u8 granu, u8 dr, u8 dw, > +u16 did, u8 am, u8 ih, > +u64 addr) > { > unsigned long flags; > unsigned int index; > @@ -133,10 +141,12 @@ static void queue_invalidate_iotlb(struct iommu *iommu, > unmap_vtd_domain_page(qinval_entries); > qinval_update_qtail(iommu, index); > spin_unlock_irqrestore(>register_lock, flags); > + > +return invalidate_sync(iommu); > } With this, ... > @@ -346,9 +353,13 @@ static int flush_iotlb_qi( > if (cap_read_drain(iommu->cap)) > dr = 1; > /* Need to conside the ih bit later */ > -queue_invalidate_iotlb(iommu, > - type >> DMA_TLB_FLUSH_GRANU_OFFSET, dr, > - dw, did, size_order, 0, addr); > +ret = queue_invalidate_iotlb_sync(iommu, > + type >> DMA_TLB_FLUSH_GRANU_OFFSET, > + dr, dw, did, size_order, 0, addr); > + > +if ( ret ) > +return ret; > + > if ( flush_dev_iotlb ) > ret = dev_invalidate_iotlb(iommu, did, addr, size_order, type); > rc = invalidate_sync(iommu); ... why does this invalidate_sync() not go away? Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v10 2/3] vt-d: synchronize for Device-TLB flush one by one
Today we do Device-TLB flush synchronization after issuing flush requests for all ATS devices belonging to a VM. Doing so however imposes a limitation, i.e. that we can not figure out which flush request is blocked in the flush queue list, based on VT-d spec. To prepare correct Device-TLB flush timeout handling in next patch, we change the behavior to synchronize for every Device-TLB flush request. So the Device-TLB flush interface is changed a little bit, by checking timeout within the function instead of outside of function. Accordingly we also do a similar change for flush interfaces of IOTLB/IEC/Context, i.e. moving synchronization into the function. Since there is no user of a non-synced interface, we just rename existing ones with _sync suffix. Signed-off-by: Quan Xu--- xen/drivers/passthrough/vtd/extern.h | 5 +-- xen/drivers/passthrough/vtd/qinval.c | 61 +-- xen/drivers/passthrough/vtd/x86/ats.c | 8 ++--- 3 files changed, 43 insertions(+), 31 deletions(-) diff --git a/xen/drivers/passthrough/vtd/extern.h b/xen/drivers/passthrough/vtd/extern.h index d4d37c3..ab7ecad 100644 --- a/xen/drivers/passthrough/vtd/extern.h +++ b/xen/drivers/passthrough/vtd/extern.h @@ -59,8 +59,9 @@ int ats_device(const struct pci_dev *, const struct acpi_drhd_unit *); int dev_invalidate_iotlb(struct iommu *iommu, u16 did, u64 addr, unsigned int size_order, u64 type); -int qinval_device_iotlb(struct iommu *iommu, -u32 max_invs_pend, u16 sid, u16 size, u64 addr); +int __must_check qinval_device_iotlb_sync(struct iommu *iommu, + u32 max_invs_pend, + u16 sid, u16 size, u64 addr); unsigned int get_cache_line_size(void); void cacheline_flush(char *); diff --git a/xen/drivers/passthrough/vtd/qinval.c b/xen/drivers/passthrough/vtd/qinval.c index 52ba2c2..69cc6bf 100644 --- a/xen/drivers/passthrough/vtd/qinval.c +++ b/xen/drivers/passthrough/vtd/qinval.c @@ -33,6 +33,8 @@ integer_param("vtd_qi_timeout", vtd_qi_timeout); #define IOMMU_QI_TIMEOUT (vtd_qi_timeout * MILLISECS(1)) +static int invalidate_sync(struct iommu *iommu); + static void print_qi_regs(struct iommu *iommu) { u64 val; @@ -72,8 +74,10 @@ static void qinval_update_qtail(struct iommu *iommu, unsigned int index) dmar_writeq(iommu->reg, DMAR_IQT_REG, (val << QINVAL_INDEX_SHIFT)); } -static void queue_invalidate_context(struct iommu *iommu, -u16 did, u16 source_id, u8 function_mask, u8 granu) +static int __must_check queue_invalidate_context_sync(struct iommu *iommu, + u16 did, u16 source_id, + u8 function_mask, + u8 granu) { unsigned long flags; unsigned int index; @@ -100,10 +104,14 @@ static void queue_invalidate_context(struct iommu *iommu, spin_unlock_irqrestore(>register_lock, flags); unmap_vtd_domain_page(qinval_entries); + +return invalidate_sync(iommu); } -static void queue_invalidate_iotlb(struct iommu *iommu, -u8 granu, u8 dr, u8 dw, u16 did, u8 am, u8 ih, u64 addr) +static int __must_check queue_invalidate_iotlb_sync(struct iommu *iommu, +u8 granu, u8 dr, u8 dw, +u16 did, u8 am, u8 ih, +u64 addr) { unsigned long flags; unsigned int index; @@ -133,10 +141,12 @@ static void queue_invalidate_iotlb(struct iommu *iommu, unmap_vtd_domain_page(qinval_entries); qinval_update_qtail(iommu, index); spin_unlock_irqrestore(>register_lock, flags); + +return invalidate_sync(iommu); } static int __must_check queue_invalidate_wait(struct iommu *iommu, -u8 iflag, u8 sw, u8 fn) + u8 iflag, u8 sw, u8 fn) { s_time_t timeout; volatile u32 poll_slot = QINVAL_STAT_INIT; @@ -196,8 +206,10 @@ static int invalidate_sync(struct iommu *iommu) return 0; } -int qinval_device_iotlb(struct iommu *iommu, -u32 max_invs_pend, u16 sid, u16 size, u64 addr) +int __must_check qinval_device_iotlb_sync(struct iommu *iommu, + u32 max_invs_pend, + u16 sid, u16 size, + u64 addr) { unsigned long flags; unsigned int index; @@ -226,15 +238,17 @@ int qinval_device_iotlb(struct iommu *iommu, qinval_update_qtail(iommu, index); spin_unlock_irqrestore(>register_lock, flags); -return 0; +return invalidate_sync(iommu); } -static void queue_invalidate_iec(struct iommu *iommu, u8 granu, u8 im, u16 iidx) +static int __must_check queue_invalidate_iec_sync(struct iommu *iommu, +