| Message ID | 1462524880-67205-9-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/arch/x86/mm/p2m-ept.c > +++ b/xen/arch/x86/mm/p2m-ept.c > @@ -832,7 +832,8 @@ out: > need_modify_vtd_table ) > { > if ( iommu_hap_pt_share ) > - iommu_pte_flush(d, gfn, &ept_entry->epte, order, vtd_pte_present); > + ret = iommu_pte_flush(d, gfn, &ept_entry->epte, > + order, vtd_pte_present); > else > { > if ( iommu_flags ) Looking at this in conjunction with patch 3, I can't see where "ret" would get consumed. Jan
On Tue, May 10, 2016 at 10:09 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote: >> --- a/xen/arch/x86/mm/p2m-ept.c >> +++ b/xen/arch/x86/mm/p2m-ept.c >> @@ -832,7 +832,8 @@ out: >> need_modify_vtd_table ) >> { >> if ( iommu_hap_pt_share ) >> - iommu_pte_flush(d, gfn, &ept_entry->epte, order, vtd_pte_present); >> + ret = iommu_pte_flush(d, gfn, &ept_entry->epte, >> + order, vtd_pte_present); >> else >> { >> if ( iommu_flags ) > > Looking at this in conjunction with patch 3, I can't see where "ret" > would get consumed. Hmm, and here I see where "rc == 0" might be a better option than "entry_written". If we know rc is zero, we can just use rc here instead of 'ret', and I think everything falls out. If rc is not zero, then we have to do this "if ( !rc ) rc = ret;" business, which seems a bit silly to do when we know it's zero and don't expect that to change. On the other hand, using rc *without* actually checking that it's zero seems like asking for trouble. So perhaps it would be better if we take your advice for patch 3, and then use 'rc' here? Everything else looks good. -George
>>> On 10.05.16 at 16:58, <George.Dunlap@eu.citrix.com> wrote: > On Tue, May 10, 2016 at 10:09 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote: >>> --- a/xen/arch/x86/mm/p2m-ept.c >>> +++ b/xen/arch/x86/mm/p2m-ept.c >>> @@ -832,7 +832,8 @@ out: >>> need_modify_vtd_table ) >>> { >>> if ( iommu_hap_pt_share ) >>> - iommu_pte_flush(d, gfn, &ept_entry->epte, order, > vtd_pte_present); >>> + ret = iommu_pte_flush(d, gfn, &ept_entry->epte, >>> + order, vtd_pte_present); >>> else >>> { >>> if ( iommu_flags ) >> >> Looking at this in conjunction with patch 3, I can't see where "ret" >> would get consumed. > > Hmm, and here I see where "rc == 0" might be a better option than > "entry_written". > > If we know rc is zero, we can just use rc here instead of 'ret', and I > think everything falls out. > > If rc is not zero, then we have to do this "if ( !rc ) rc = ret;" > business, which seems a bit silly to do when we know it's zero and > don't expect that to change. > > On the other hand, using rc *without* actually checking that it's zero > seems like asking for trouble. > > So perhaps it would be better if we take your advice for patch 3, and > then use 'rc' here? Ah, yes, that's a good 2nd reason. Jan
On Tuesday, May 10, 2016 5:10 PM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote: > > --- a/xen/arch/x86/mm/p2m-ept.c > > +++ b/xen/arch/x86/mm/p2m-ept.c > > @@ -832,7 +832,8 @@ out: > > need_modify_vtd_table ) > > { > > if ( iommu_hap_pt_share ) > > - iommu_pte_flush(d, gfn, &ept_entry->epte, order, vtd_pte_present); > > + ret = iommu_pte_flush(d, gfn, &ept_entry->epte, > > + order, vtd_pte_present); > > else > > { > > if ( iommu_flags ) > > Looking at this in conjunction with patch 3, I can't see where "ret" > would get consumed. > Sorry, this 'ret' should be 'rc' here, as I removed the pending " + if ( !rc ) + rc = ret; + " from v4. Quan
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c index 814cb72..34b96f8 100644 --- a/xen/arch/x86/mm/p2m-ept.c +++ b/xen/arch/x86/mm/p2m-ept.c @@ -832,7 +832,8 @@ out: need_modify_vtd_table ) { if ( iommu_hap_pt_share ) - iommu_pte_flush(d, gfn, &ept_entry->epte, order, vtd_pte_present); + ret = iommu_pte_flush(d, gfn, &ept_entry->epte, + order, vtd_pte_present); else { if ( iommu_flags ) diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c index a749c67..bf77417 100644 --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -1773,8 +1773,8 @@ static int intel_iommu_unmap_page(struct domain *d, unsigned long gfn) return dma_pte_clear_one(d, (paddr_t)gfn << PAGE_SHIFT_4K); } -int iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, - int order, bool_t present) +int __must_check iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, + int order, bool_t present) { struct acpi_drhd_unit *drhd; struct iommu *iommu = NULL; diff --git a/xen/include/asm-x86/iommu.h b/xen/include/asm-x86/iommu.h index 43f1620..3d2c354 100644 --- a/xen/include/asm-x86/iommu.h +++ b/xen/include/asm-x86/iommu.h @@ -27,7 +27,8 @@ int iommu_setup_hpet_msi(struct msi_desc *); /* While VT-d specific, this must get declared in a generic header. */ int adjust_vtd_irq_affinities(void); -int iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, int order, bool_t present); +int __must_check iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, + int order, bool_t present); bool_t iommu_supports_eim(void); int iommu_enable_x2apic_IR(void); void iommu_disable_x2apic_IR(void);