Re: [Qemu-devel] [PATCH v4 RESEND 2/3] IOMMU: change iommu_op->translate's is_write to flags, add support to NO_FAIL flag mode

[Peter Xu <peterx@redhat.com>]

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