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

[Aviv B.D.]

Hi,
You are right, and I will revert those functions you mentioned.

Thanks,
Aviv.



On Wed, Oct 19, 2016 at 11:35 AM, Peter Xu <address@hidden> wrote:

> 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
>