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 <pet...@redhat.com> 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
>
