From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:32858) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b9mih-0006p9-UG for qemu-devel@nongnu.org; Mon, 06 Jun 2016 01:05:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b9mia-0006Q5-NP for qemu-devel@nongnu.org; Mon, 06 Jun 2016 01:04:58 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58125) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b9mia-0006Pv-FV for qemu-devel@nongnu.org; Mon, 06 Jun 2016 01:04:52 -0400 Date: Mon, 6 Jun 2016 13:04:45 +0800 From: Peter Xu Message-ID: <20160606050445.GC21254@pxdev.xzpeter.org> References: <1463847590-22782-1-git-send-email-bd.aviv@gmail.com> <1463847590-22782-3-git-send-email-bd.aviv@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1463847590-22782-3-git-send-email-bd.aviv@gmail.com> Subject: Re: [Qemu-devel] [PATCH v3 2/3] IOMMU: change iommu_op->translate's is_write to flags, add support to NO_FAIL flag mode List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Aviv B.D" Cc: qemu-devel@nongnu.org, "Michael S. Tsirkin" , Alex Williamson , Jan Kiszka On Sat, May 21, 2016 at 07:19:49PM +0300, Aviv B.D wrote: [...] > static void vtd_record_frcd(IntelIOMMUState *s, uint16_t index, > uint16_t source_id, hwaddr addr, > - VTDFaultReason fault, bool is_write) > + VTDFaultReason fault, IOMMUAccessFlags flags) > { > uint64_t hi = 0, lo; > hwaddr frcd_reg_addr = DMAR_FRCD_REG_OFFSET + (((uint64_t)index) << 4); > @@ -365,7 +365,7 @@ static void vtd_record_frcd(IntelIOMMUState *s, uint16_t index, > > lo = VTD_FRCD_FI(addr); > hi = VTD_FRCD_SID(source_id) | VTD_FRCD_FR(fault); > - if (!is_write) { > + if (!(flags == IOMMU_WO || flags == IOMMU_RW)) { Is it possible that we have IOMMU_RW here? I see this flag is only used when DMAR is disabled and we assume all address are rw. No other place is using it. If so, I'd suggest we can make it simpler like: if (!(flags == IOMMU_WO)) { > hi |= VTD_FRCD_T; > } > vtd_set_quad_raw(s, frcd_reg_addr, lo); > @@ -396,7 +396,7 @@ static bool vtd_try_collapse_fault(IntelIOMMUState *s, uint16_t source_id) > /* Log and report an DMAR (address translation) fault to software */ > static void vtd_report_dmar_fault(IntelIOMMUState *s, uint16_t source_id, > hwaddr addr, VTDFaultReason fault, > - bool is_write) > + IOMMUAccessFlags flags) > { > uint32_t fsts_reg = vtd_get_long_raw(s, DMAR_FSTS_REG); > > @@ -407,7 +407,7 @@ static void vtd_report_dmar_fault(IntelIOMMUState *s, uint16_t source_id, > return; > } > VTD_DPRINTF(FLOG, "sid 0x%"PRIx16 ", fault %d, addr 0x%"PRIx64 > - ", is_write %d", source_id, fault, addr, is_write); > + ", flags %d", source_id, fault, addr, flags); > if (fsts_reg & VTD_FSTS_PFO) { > VTD_DPRINTF(FLOG, "new fault is not recorded due to " > "Primary Fault Overflow"); > @@ -425,7 +425,7 @@ static void vtd_report_dmar_fault(IntelIOMMUState *s, uint16_t source_id, > return; > } > > - vtd_record_frcd(s, s->next_frcd_reg, source_id, addr, fault, is_write); > + vtd_record_frcd(s, s->next_frcd_reg, source_id, addr, fault, flags); > > if (fsts_reg & VTD_FSTS_PPF) { > VTD_DPRINTF(FLOG, "there are pending faults already, " > @@ -621,7 +621,7 @@ static bool vtd_slpte_nonzero_rsvd(uint64_t slpte, uint32_t level) > /* Given the @gpa, get relevant @slptep. @slpte_level will be the last level > * of the translation, can be used for deciding the size of large page. > */ > -static int vtd_gpa_to_slpte(VTDContextEntry *ce, uint64_t gpa, bool is_write, > +static int vtd_gpa_to_slpte(VTDContextEntry *ce, uint64_t gpa, IOMMUAccessFlags flags, > uint64_t *slptep, uint32_t *slpte_level, > bool *reads, bool *writes) > { > @@ -641,7 +641,20 @@ static int vtd_gpa_to_slpte(VTDContextEntry *ce, uint64_t gpa, bool is_write, > } > > /* FIXME: what is the Atomics request here? */ > - access_right_check = is_write ? VTD_SL_W : VTD_SL_R; > + switch(flags){ > + case IOMMU_WO: > + access_right_check = VTD_SL_W; > + break; > + case IOMMU_RO: > + access_right_check = VTD_SL_R; > + break; > + case IOMMU_RW: /* passthrow */ Same as above. Remove this line? (btw, it's "passthrough" :) > + case IOMMU_NO_FAIL: > + access_right_check = VTD_SL_R | VTD_SL_W; > + break; > + default: > + assert(0); > + } > > while (true) { > offset = vtd_gpa_level_offset(gpa, level); > @@ -663,8 +676,8 @@ static int vtd_gpa_to_slpte(VTDContextEntry *ce, uint64_t gpa, bool is_write, > if (!(slpte & access_right_check)) { > VTD_DPRINTF(GENERAL, "error: lack of %s permission for " > "gpa 0x%"PRIx64 " slpte 0x%"PRIx64, > - (is_write ? "write" : "read"), gpa, slpte); > - return is_write ? -VTD_FR_WRITE : -VTD_FR_READ; > + (flags == IOMMU_WO ? "write" : "read"), gpa, slpte); > + return (flags == IOMMU_RW || flags == IOMMU_WO) ? -VTD_FR_WRITE : -VTD_FR_READ; Same. (please looking for all IOMMU_RW places, will stop here) > } > if (vtd_slpte_nonzero_rsvd(slpte, level)) { > VTD_DPRINTF(GENERAL, "error: non-zero reserved field in second " > @@ -781,11 +794,11 @@ static inline bool vtd_is_interrupt_addr(hwaddr addr) > * > * @bus_num: The bus number > * @devfn: The devfn, which is the combined of device and function number > - * @is_write: The access is a write operation > + * @flags: The access is a write operation Please change comments after "flags:" correspondingly. -- peterx