Re: [PATCH v2 1/2] intel_iommu: widen impl.min_access_size to 8 to fix MMIO abort

[Junjie Cao <junjie.cao@intel.com>]

Hi Zhenzhong,

Thanks for the careful review.

You are right that the code path has changed.  With min_access_size=8
the handler always receives size=8, so a guest 4-byte write to a base
offset calls vtd_set_quad() with a zero-extended value instead of
vtd_set_long().  Two effects:


1. Triggers called unconditionally

CCMD_REG, IOTLB_REG, and FRCD_REG_0_2 originally guarded the
trigger on size==8.  In v2 all three are still no-ops:
vtd_handle_ccmd_write() and vtd_handle_iotlb_write() gate on
ICC/IVT (bit 63), which the zero-extended write just cleared
through the wmask.  For FRCD_REG_0_2, wmask is zero and the w1c
bit 63 does not fire (val bit 63 = 0), so the register is
unchanged.


2. Upper writable bits cleared by zero-extended vtd_set_quad()

Where the upper 32-bit wmask is non-zero, zero-extended val clears
those bits:

  Register       Offset  Upper wmask   Bits cleared
  CCMD_REG       0x28    0xe0000003    ICC, CIRG, 33:32
  IOTLB_REG      0xf8    0xb003ffff    IVT, IIRG, DID
  RTADDR_REG     0x20    0xffffffff    address 63:32
  IQA_REG        0x90    0xffffffff    address 63:32
  IVA_REG        0xf0    0xffffffff    address 63:32
  IRTA_REG       0xb8    0xffffffff    address 63:32

  (IQT_REG, FRCD_0_0, and FRCD_0_2 have upper wmask = 0 -- safe.)

This is safe because a guest must program the full register before
the value is consumed: CCMD/IOTLB fields are set before the action
bit (ICC/IVT) in the high-half write; RTADDR, IQA, and IRTA are
latched only when a GCMD enable bit (SRTP/QIE/SIRTP) fires; IVA is
read when IOTLB invalidation triggers via IVT.

However, a read-back between the low-half and high-half writes
would observe zeroed upper bits -- a real behavioral difference.


Two possible paths

(a) Keep v2 as-is, add comments documenting the safety argument
    above.

(b) Stay with min_access_size=4.  Simply remove all 25 asserts.
    21 sit at non-8-aligned offsets -- unreachable by any 8-byte
    guest access (alignment check rejects them).  The remaining 4
    at 8-aligned offsets (FECTL 0x38, IECTL 0xa0, IEADDR 0xa8,
    PECTL 0xe0) fall through to vtd_set_long() which implicitly
    truncates the value -- safe and no semantic change.  All
    existing size branches in 64-bit register pairs are preserved.

As I see it, both approaches are correct; (a) is simpler but
changes observable semantics in an edge case, (b) is conservative
with zero semantic change.  That said, I am happy to hear any other
suggestions.

Thanks,
Junjie