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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 24/4/26 22:18, Junjie Cao wrote:
> Raise .impl.min_access_size from 4 to 8 in vtd_mem_ops so the memory
> subsystem always widens guest accesses to 8 bytes before calling the
> handler.  This eliminates all 25 assert(size == 4) sites that crashed
> QEMU on an 8-byte access to a 32-bit-only register.
> 
> With size always 8, the if/else branches for 64-bit register pairs
> collapse.  A zero-extended 4-byte write to the low half is safe:
> wmask protects read-only upper bits, and trigger functions re-read
> the register file and guard on their action bits.
> 
> The entry bounds check is relaxed to `addr >= DMAR_REG_SIZE` since
> the widened size no longer reflects the guest access width; the
> framework guarantees addr stays within the MemoryRegion.  Default
> branches fall back to vtd_get/set_long() when addr + 8 would exceed
> the register file.
> 
> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Junjie Cao <junjie.cao@intel.com>
> ---
>   hw/i386/intel_iommu.c | 121 ++++++++----------------------------------
>   1 file changed, 23 insertions(+), 98 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index f395fa248c..4b25907778 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3697,7 +3697,7 @@ static uint64_t vtd_mem_read(void *opaque, hwaddr addr, unsigned size)
>   
>       trace_vtd_reg_read(addr, size);
>   
> -    if (addr + size > DMAR_REG_SIZE) {
> +    if (addr >= DMAR_REG_SIZE) {
>           error_report_once("%s: MMIO over range: addr=0x%" PRIx64
>                             " size=0x%x", __func__, addr, size);
>           return (uint64_t)-1;
> @@ -3707,13 +3707,9 @@ static uint64_t vtd_mem_read(void *opaque, hwaddr addr, unsigned size)
>       /* Root Table Address Register, 64-bit */
>       case DMAR_RTADDR_REG:
>           val = vtd_get_quad_raw(s, DMAR_RTADDR_REG);
> -        if (size == 4) {
> -            val = val & ((1ULL << 32) - 1);
> -        }
>           break;
>   
>       case DMAR_RTADDR_REG_HI:
> -        assert(size == 4);
>           val = vtd_get_quad_raw(s, DMAR_RTADDR_REG) >> 32;
>           break;
>   
> @@ -3722,26 +3718,21 @@ static uint64_t vtd_mem_read(void *opaque, hwaddr addr, unsigned size)
>           val = s->iq |
>                 (vtd_get_quad(s, DMAR_IQA_REG) &
>                 (VTD_IQA_QS | VTD_IQA_DW_MASK));
> -        if (size == 4) {
> -            val = val & ((1ULL << 32) - 1);
> -        }
>           break;
>   
>       case DMAR_IQA_REG_HI:
> -        assert(size == 4);
>           val = s->iq >> 32;
>           break;
>   
>       case DMAR_PEUADDR_REG:
> -        assert(size == 4);

Does this device support unaligned accesses? (I doubt). Otherwise
aren't all these assert(size == 4) now g_assert_not_reached()?


> @@ -4184,7 +4109,7 @@ static const MemoryRegionOps vtd_mem_ops = {
>       .write = vtd_mem_write,
>       .endianness = DEVICE_LITTLE_ENDIAN,
>       .impl = {
> -        .min_access_size = 4,
> +        .min_access_size = 8,
>           .max_access_size = 8,
>       },
>       .valid = {