On Mon, Oct 17, 2016 at 06:44:23PM +0300, Aviv B.D wrote:
[...]
> @@ -364,7 +364,7 @@ static void vtd_set_frcd_and_update_ppf(IntelIOMMUState
> *s, uint16_t index)
> /* Must not update F field now, should be done later */
> 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)
I think we don't need to change this is_write into flags. We can just
make sure we translate the flags into is_write when calling
vtd_record_frcd. After all, NO_FAIL makes no sense in this function.
> {
> uint64_t hi = 0, lo;
> hwaddr frcd_reg_addr = DMAR_FRCD_REG_OFFSET + (((uint64_t)index) << 4);
> @@ -373,7 +373,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)) {
> hi |= VTD_FRCD_T;
> }
> vtd_set_quad_raw(s, frcd_reg_addr, lo);
> @@ -404,7 +404,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)
Here as well. IMHO we can just keep the old is_write, and tune the
callers.
> {
> uint32_t fsts_reg = vtd_get_long_raw(s, DMAR_FSTS_REG);
>
> @@ -415,7 +415,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");
> @@ -433,7 +433,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);
... so if you agree on my previous comments, here we can use:
vtd_record_frcd(s, s->next_frcd_reg, source_id, addr, fault,
flags & IOMMU_WO);
and keep the vtd_record_frcd() untouched.
[...]
> @@ -789,11 +804,12 @@ 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
Need to fix comment to suite the new flag.
> * @entry: IOMMUTLBEntry that contain the addr to be translated and result
> */
> static void vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,
> - uint8_t devfn, hwaddr addr, bool is_write,
> + uint8_t devfn, hwaddr addr,
> + IOMMUAccessFlags flags,
> IOMMUTLBEntry *entry)
> {
> IntelIOMMUState *s = vtd_as->iommu_state;
> @@ -811,7 +827,7 @@ static void vtd_do_iommu_translate(VTDAddressSpace
> *vtd_as, PCIBus *bus,
>
> /* Check if the request is in interrupt address range */
> if (vtd_is_interrupt_addr(addr)) {
> - if (is_write) {
> + if (flags == IOMMU_WO || flags == IOMMU_RW) {
I suggest we directly use (flags & IOMMU_WO) in all similar places.
> /* FIXME: since we don't know the length of the access here, we
> * treat Non-DWORD length write requests without PASID as
> * interrupt requests, too. Withoud interrupt remapping support,
> @@ -827,7 +843,7 @@ static void vtd_do_iommu_translate(VTDAddressSpace
> *vtd_as, PCIBus *bus,
> } else {
> VTD_DPRINTF(GENERAL, "error: read request from interrupt address
> "
> "gpa 0x%"PRIx64, addr);
> - vtd_report_dmar_fault(s, source_id, addr, VTD_FR_READ, is_write);
> + vtd_report_dmar_fault(s, source_id, addr, VTD_FR_READ, flags);
Again, we can avoid touching vtd_report_dmar_fault() interface, and
use the old is_write. Looks like NO_FAIL makes no sense for it as well.
Thanks,
-- peterx
