| Message ID | 1462524880-67205-8-git-send-email-quan.xu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote: > --- a/xen/drivers/passthrough/vtd/iommu.c > +++ b/xen/drivers/passthrough/vtd/iommu.c > @@ -604,15 +604,15 @@ static int iommu_flush_iotlb(struct domain *d, unsigned long gfn, > return rc; > } > > -static void iommu_flush_iotlb_page(struct domain *d, unsigned long gfn, > - unsigned int page_count) > +static int iommu_flush_iotlb_page(struct domain *d, unsigned long gfn, > + unsigned int page_count) > { > - iommu_flush_iotlb(d, gfn, 1, page_count); > + return iommu_flush_iotlb(d, gfn, 1, page_count); > } > > -static void iommu_flush_iotlb_all(struct domain *d) > +static int iommu_flush_iotlb_all(struct domain *d) > { > - iommu_flush_iotlb(d, INVALID_GFN, 0, 0); > + return iommu_flush_iotlb(d, INVALID_GFN, 0, 0); > } As already indicated in a reply to an earlier patch, despite what was said on the earlier version I think we should have __must_check here and ... > --- a/xen/include/xen/iommu.h > +++ b/xen/include/xen/iommu.h > @@ -179,8 +179,8 @@ struct iommu_ops { > void (*resume)(void); > void (*share_p2m)(struct domain *d); > void (*crash_shutdown)(void); > - void (*iotlb_flush)(struct domain *d, unsigned long gfn, unsigned int page_count); > - void (*iotlb_flush_all)(struct domain *d); > + int (*iotlb_flush)(struct domain *d, unsigned long gfn, unsigned int page_count); > + int (*iotlb_flush_all)(struct domain *d); ... here. Jan
On May 10, 2016 5:07 PM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote: > > --- a/xen/drivers/passthrough/vtd/iommu.c > > +++ b/xen/drivers/passthrough/vtd/iommu.c > > @@ -604,15 +604,15 @@ static int iommu_flush_iotlb(struct domain *d, > unsigned long gfn, > > return rc; > > } > > > > -static void iommu_flush_iotlb_page(struct domain *d, unsigned long gfn, > > - unsigned int page_count) > > +static int iommu_flush_iotlb_page(struct domain *d, unsigned long gfn, > > + unsigned int page_count) > > { > > - iommu_flush_iotlb(d, gfn, 1, page_count); > > + return iommu_flush_iotlb(d, gfn, 1, page_count); > > } > > > > -static void iommu_flush_iotlb_all(struct domain *d) > > +static int iommu_flush_iotlb_all(struct domain *d) > > { > > - iommu_flush_iotlb(d, INVALID_GFN, 0, 0); > > + return iommu_flush_iotlb(d, INVALID_GFN, 0, 0); > > } > > As already indicated in a reply to an earlier patch, despite what was said on the > earlier version I think we should have __must_check here If the static one is initialized for .callback, is it really necessary to add __must_check here? I check it with compiler, and it is ok when I didn't add __must_check here. If yes, I'll add __must_check for ARM one as well. > and ... > > > --- a/xen/include/xen/iommu.h > > +++ b/xen/include/xen/iommu.h > > @@ -179,8 +179,8 @@ struct iommu_ops { > > void (*resume)(void); > > void (*share_p2m)(struct domain *d); > > void (*crash_shutdown)(void); > > - void (*iotlb_flush)(struct domain *d, unsigned long gfn, unsigned int > page_count); > > - void (*iotlb_flush_all)(struct domain *d); > > + int (*iotlb_flush)(struct domain *d, unsigned long gfn, unsigned int > page_count); > > + int (*iotlb_flush_all)(struct domain *d); > > ... here. Yes, it is necessary here. Quan
>>> On 11.05.16 at 08:47, <quan.xu@intel.com> wrote: > On May 10, 2016 5:07 PM, Jan Beulich <JBeulich@suse.com> wrote: >> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote: >> > --- a/xen/drivers/passthrough/vtd/iommu.c >> > +++ b/xen/drivers/passthrough/vtd/iommu.c >> > @@ -604,15 +604,15 @@ static int iommu_flush_iotlb(struct domain *d, >> unsigned long gfn, >> > return rc; >> > } >> > >> > -static void iommu_flush_iotlb_page(struct domain *d, unsigned long gfn, >> > - unsigned int page_count) >> > +static int iommu_flush_iotlb_page(struct domain *d, unsigned long gfn, >> > + unsigned int page_count) >> > { >> > - iommu_flush_iotlb(d, gfn, 1, page_count); >> > + return iommu_flush_iotlb(d, gfn, 1, page_count); >> > } >> > >> > -static void iommu_flush_iotlb_all(struct domain *d) >> > +static int iommu_flush_iotlb_all(struct domain *d) >> > { >> > - iommu_flush_iotlb(d, INVALID_GFN, 0, 0); >> > + return iommu_flush_iotlb(d, INVALID_GFN, 0, 0); >> > } >> >> As already indicated in a reply to an earlier patch, despite what was said on the >> earlier version I think we should have __must_check here > > If the static one is initialized for .callback, is it really necessary to > add __must_check here? > I check it with compiler, and it is ok when I didn't add __must_check here. Without you telling us what exactly you checked, I can't respond to this. Extending from the reply just sent to patch 3(?) and for the avoidance of doubt, you now obviously also need to __must_check-annotate the function pointer (to match the desire of wanting to never lose such an annotation on the way back up the call tree). > If yes, I'll add __must_check for ARM one as well. Of course. Jan
On May 11, 2016 3:06 PM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 11.05.16 at 08:47, <quan.xu@intel.com> wrote: > > On May 10, 2016 5:07 PM, Jan Beulich <JBeulich@suse.com> wrote: > >> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote: > >> > --- a/xen/drivers/passthrough/vtd/iommu.c > >> > +++ b/xen/drivers/passthrough/vtd/iommu.c > >> > @@ -604,15 +604,15 @@ static int iommu_flush_iotlb(struct domain > >> > *d, > >> unsigned long gfn, > >> > return rc; > >> > } > >> > > >> > -static void iommu_flush_iotlb_page(struct domain *d, unsigned long > gfn, > >> > - unsigned int page_count) > >> > +static int iommu_flush_iotlb_page(struct domain *d, unsigned long gfn, > >> > + unsigned int page_count) > >> > { > >> > - iommu_flush_iotlb(d, gfn, 1, page_count); > >> > + return iommu_flush_iotlb(d, gfn, 1, page_count); > >> > } > >> > > >> > -static void iommu_flush_iotlb_all(struct domain *d) > >> > +static int iommu_flush_iotlb_all(struct domain *d) > >> > { > >> > - iommu_flush_iotlb(d, INVALID_GFN, 0, 0); > >> > + return iommu_flush_iotlb(d, INVALID_GFN, 0, 0); > >> > } > >> > >> As already indicated in a reply to an earlier patch, despite what was > >> said on the earlier version I think we should have __must_check here > > > > If the static one is initialized for .callback, is it really necessary > > to add __must_check here? > > I check it with compiler, and it is ok when I didn't add __must_check here. > > Without you telling us what exactly you checked, I can't respond to this. > Extending from the reply just sent to patch 3(?) and for the avoidance of > doubt, you now obviously also need to __must_check-annotate the function > pointer (to match the desire of wanting to never lose such an annotation on > the way back up the call tree). > I checked -- without __must_check for iommu_flush_iotlb_page() / iommu_flush_iotlb_all(). > > If yes, I'll add __must_check for ARM one as well. > > Of course. > Got it. Quan
>>> On 11.05.16 at 09:12, <quan.xu@intel.com> wrote: > On May 11, 2016 3:06 PM, Jan Beulich <JBeulich@suse.com> wrote: >> >>> On 11.05.16 at 08:47, <quan.xu@intel.com> wrote: >> > On May 10, 2016 5:07 PM, Jan Beulich <JBeulich@suse.com> wrote: >> >> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote: >> >> > --- a/xen/drivers/passthrough/vtd/iommu.c >> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c >> >> > @@ -604,15 +604,15 @@ static int iommu_flush_iotlb(struct domain >> >> > *d, >> >> unsigned long gfn, >> >> > return rc; >> >> > } >> >> > >> >> > -static void iommu_flush_iotlb_page(struct domain *d, unsigned long >> gfn, >> >> > - unsigned int page_count) >> >> > +static int iommu_flush_iotlb_page(struct domain *d, unsigned long gfn, >> >> > + unsigned int page_count) >> >> > { >> >> > - iommu_flush_iotlb(d, gfn, 1, page_count); >> >> > + return iommu_flush_iotlb(d, gfn, 1, page_count); >> >> > } >> >> > >> >> > -static void iommu_flush_iotlb_all(struct domain *d) >> >> > +static int iommu_flush_iotlb_all(struct domain *d) >> >> > { >> >> > - iommu_flush_iotlb(d, INVALID_GFN, 0, 0); >> >> > + return iommu_flush_iotlb(d, INVALID_GFN, 0, 0); >> >> > } >> >> >> >> As already indicated in a reply to an earlier patch, despite what was >> >> said on the earlier version I think we should have __must_check here >> > >> > If the static one is initialized for .callback, is it really necessary >> > to add __must_check here? >> > I check it with compiler, and it is ok when I didn't add __must_check here. >> >> Without you telling us what exactly you checked, I can't respond to this. >> Extending from the reply just sent to patch 3(?) and for the avoidance of >> doubt, you now obviously also need to __must_check-annotate the function >> pointer (to match the desire of wanting to never lose such an annotation on >> the way back up the call tree). >> > > I checked -- without __must_check for iommu_flush_iotlb_page() / > iommu_flush_iotlb_all(). But _what_ did you check? I.e. the question isn't which functions you did your check with, but what behavioral checking you did. Jan
On May 11, 2016 3:17 PM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 11.05.16 at 09:12, <quan.xu@intel.com> wrote: > > On May 11, 2016 3:06 PM, Jan Beulich <JBeulich@suse.com> wrote: > >> >>> On 11.05.16 at 08:47, <quan.xu@intel.com> wrote: > >> > On May 10, 2016 5:07 PM, Jan Beulich <JBeulich@suse.com> wrote: > >> >> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote: > >> >> > --- a/xen/drivers/passthrough/vtd/iommu.c > >> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c > >> >> > @@ -604,15 +604,15 @@ static int iommu_flush_iotlb(struct domain > >> >> > *d, > >> >> unsigned long gfn, > >> >> > return rc; > >> >> > } > >> >> > > >> >> > -static void iommu_flush_iotlb_page(struct domain *d, unsigned > >> >> > long > >> gfn, > >> >> > - unsigned int page_count) > >> >> > +static int iommu_flush_iotlb_page(struct domain *d, unsigned long > gfn, > >> >> > + unsigned int page_count) > >> >> > { > >> >> > - iommu_flush_iotlb(d, gfn, 1, page_count); > >> >> > + return iommu_flush_iotlb(d, gfn, 1, page_count); > >> >> > } > >> >> > > >> >> > -static void iommu_flush_iotlb_all(struct domain *d) > >> >> > +static int iommu_flush_iotlb_all(struct domain *d) > >> >> > { > >> >> > - iommu_flush_iotlb(d, INVALID_GFN, 0, 0); > >> >> > + return iommu_flush_iotlb(d, INVALID_GFN, 0, 0); > >> >> > } > >> >> > >> >> As already indicated in a reply to an earlier patch, despite what > >> >> was said on the earlier version I think we should have > >> >> __must_check here > >> > > >> > If the static one is initialized for .callback, is it really > >> > necessary to add __must_check here? > >> > I check it with compiler, and it is ok when I didn't add __must_check here. > >> > >> Without you telling us what exactly you checked, I can't respond to this. > >> Extending from the reply just sent to patch 3(?) and for the > >> avoidance of doubt, you now obviously also need to > >> __must_check-annotate the function pointer (to match the desire of > >> wanting to never lose such an annotation on the way back up the call tree). > >> > > > > I checked -- without __must_check for iommu_flush_iotlb_page() / > > iommu_flush_iotlb_all(). > > But _what_ did you check? I.e. the question isn't which functions you did your > check with, but what behavioral checking you did. > without __must_check for iommu_flush_iotlb_page() /iommu_flush_iotlb_all(), I can run 'make xen' successfully. Sorry. Quan
>>> On 11.05.16 at 09:20, <quan.xu@intel.com> wrote: > On May 11, 2016 3:17 PM, Jan Beulich <JBeulich@suse.com> wrote: >> >>> On 11.05.16 at 09:12, <quan.xu@intel.com> wrote: >> > On May 11, 2016 3:06 PM, Jan Beulich <JBeulich@suse.com> wrote: >> >> >>> On 11.05.16 at 08:47, <quan.xu@intel.com> wrote: >> >> > On May 10, 2016 5:07 PM, Jan Beulich <JBeulich@suse.com> wrote: >> >> >> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote: >> >> >> > --- a/xen/drivers/passthrough/vtd/iommu.c >> >> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c >> >> >> > @@ -604,15 +604,15 @@ static int iommu_flush_iotlb(struct domain >> >> >> > *d, >> >> >> unsigned long gfn, >> >> >> > return rc; >> >> >> > } >> >> >> > >> >> >> > -static void iommu_flush_iotlb_page(struct domain *d, unsigned >> >> >> > long >> >> gfn, >> >> >> > - unsigned int page_count) >> >> >> > +static int iommu_flush_iotlb_page(struct domain *d, unsigned long >> gfn, >> >> >> > + unsigned int page_count) >> >> >> > { >> >> >> > - iommu_flush_iotlb(d, gfn, 1, page_count); >> >> >> > + return iommu_flush_iotlb(d, gfn, 1, page_count); >> >> >> > } >> >> >> > >> >> >> > -static void iommu_flush_iotlb_all(struct domain *d) >> >> >> > +static int iommu_flush_iotlb_all(struct domain *d) >> >> >> > { >> >> >> > - iommu_flush_iotlb(d, INVALID_GFN, 0, 0); >> >> >> > + return iommu_flush_iotlb(d, INVALID_GFN, 0, 0); >> >> >> > } >> >> >> >> >> >> As already indicated in a reply to an earlier patch, despite what >> >> >> was said on the earlier version I think we should have >> >> >> __must_check here >> >> > >> >> > If the static one is initialized for .callback, is it really >> >> > necessary to add __must_check here? >> >> > I check it with compiler, and it is ok when I didn't add __must_check > here. >> >> >> >> Without you telling us what exactly you checked, I can't respond to this. >> >> Extending from the reply just sent to patch 3(?) and for the >> >> avoidance of doubt, you now obviously also need to >> >> __must_check-annotate the function pointer (to match the desire of >> >> wanting to never lose such an annotation on the way back up the call tree). >> >> >> > >> > I checked -- without __must_check for iommu_flush_iotlb_page() / >> > iommu_flush_iotlb_all(). >> >> But _what_ did you check? I.e. the question isn't which functions you did > your >> check with, but what behavioral checking you did. >> > without __must_check for iommu_flush_iotlb_page() /iommu_flush_iotlb_all(), > I can run 'make xen' successfully. Sorry. Oh, sure you can. Leaving the annotation out will always result in no more (and likely less) diagnostics. I.e. that's not a valid criteria. Jan
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c index 54a03a6..1864b6d 100644 --- a/xen/drivers/passthrough/arm/smmu.c +++ b/xen/drivers/passthrough/arm/smmu.c @@ -2540,7 +2540,7 @@ static int force_stage = 2; */ static u32 platform_features = ARM_SMMU_FEAT_COHERENT_WALK; -static void arm_smmu_iotlb_flush_all(struct domain *d) +static int arm_smmu_iotlb_flush_all(struct domain *d) { struct arm_smmu_xen_domain *smmu_domain = dom_iommu(d)->arch.priv; struct iommu_domain *cfg; @@ -2557,13 +2557,15 @@ static void arm_smmu_iotlb_flush_all(struct domain *d) arm_smmu_tlb_inv_context(cfg->priv); } spin_unlock(&smmu_domain->lock); + + return 0; } -static void arm_smmu_iotlb_flush(struct domain *d, unsigned long gfn, - unsigned int page_count) +static int arm_smmu_iotlb_flush(struct domain *d, unsigned long gfn, + unsigned int page_count) { - /* ARM SMMU v1 doesn't have flush by VMA and VMID */ - arm_smmu_iotlb_flush_all(d); + /* ARM SMMU v1 doesn't have flush by VMA and VMID */ + return arm_smmu_iotlb_flush_all(d); } static struct iommu_domain *arm_smmu_get_domain(struct domain *d, diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c index 7b5fc59..c013d91 100644 --- a/xen/drivers/passthrough/iommu.c +++ b/xen/drivers/passthrough/iommu.c @@ -320,9 +320,7 @@ int __must_check iommu_iotlb_flush(struct domain *d, unsigned long gfn, if ( !iommu_enabled || !hd->platform_ops || !hd->platform_ops->iotlb_flush ) return 0; - hd->platform_ops->iotlb_flush(d, gfn, page_count); - - return 0; + return hd->platform_ops->iotlb_flush(d, gfn, page_count); } int __must_check iommu_iotlb_flush_all(struct domain *d) @@ -332,9 +330,7 @@ int __must_check iommu_iotlb_flush_all(struct domain *d) if ( !iommu_enabled || !hd->platform_ops || !hd->platform_ops->iotlb_flush_all ) return 0; - hd->platform_ops->iotlb_flush_all(d); - - return 0; + return hd->platform_ops->iotlb_flush_all(d); } int __init iommu_setup(void) diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c index 584982a..a749c67 100644 --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -604,15 +604,15 @@ static int iommu_flush_iotlb(struct domain *d, unsigned long gfn, return rc; } -static void iommu_flush_iotlb_page(struct domain *d, unsigned long gfn, - unsigned int page_count) +static int iommu_flush_iotlb_page(struct domain *d, unsigned long gfn, + unsigned int page_count) { - iommu_flush_iotlb(d, gfn, 1, page_count); + return iommu_flush_iotlb(d, gfn, 1, page_count); } -static void iommu_flush_iotlb_all(struct domain *d) +static int iommu_flush_iotlb_all(struct domain *d) { - iommu_flush_iotlb(d, INVALID_GFN, 0, 0); + return iommu_flush_iotlb(d, INVALID_GFN, 0, 0); } /* clear one page's page table */ diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h index 75c17a7..6a64014 100644 --- a/xen/include/xen/iommu.h +++ b/xen/include/xen/iommu.h @@ -179,8 +179,8 @@ struct iommu_ops { void (*resume)(void); void (*share_p2m)(struct domain *d); void (*crash_shutdown)(void); - void (*iotlb_flush)(struct domain *d, unsigned long gfn, unsigned int page_count); - void (*iotlb_flush_all)(struct domain *d); + int (*iotlb_flush)(struct domain *d, unsigned long gfn, unsigned int page_count); + int (*iotlb_flush_all)(struct domain *d); int (*get_reserved_device_memory)(iommu_grdm_t *, void *); void (*dump_p2m_table)(struct domain *d); };
Propagate the IOMMU Device-TLB flush error up to the iommu_iotlb_flush{,_all}. This patch fixes the leaf ones. Signed-off-by: Quan Xu <quan.xu@intel.com> Acked-by: Kevin Tian <kevin.tian@intel.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien.grall@arm.com> CC: Jan Beulich <jbeulich@suse.com> CC: Kevin Tian <kevin.tian@intel.com> CC: Feng Wu <feng.wu@intel.com> --- xen/drivers/passthrough/arm/smmu.c | 12 +++++++----- xen/drivers/passthrough/iommu.c | 8 ++------ xen/drivers/passthrough/vtd/iommu.c | 10 +++++----- xen/include/xen/iommu.h | 4 ++-- 4 files changed, 16 insertions(+), 18 deletions(-)