Re: [PATCH] hw/i386/intel_iommu: Block CFI when necessary

[cmd]

On 27/06/2024 07:08, Yuke Peng wrote:

On Wed, Jun 26, 2024 at 2:15 PM cmd <clement.mathieudrif.etu@gmail.com> wrote:

Hi,

On 25/06/2024 13:28, Yuke Peng wrote:
> According to Intel VT-d specification 5.1.4, CFI must be blocked when
> Extended Interrupt Mode is enabled or Compatibility format interrupts
> are disabled.
>
> Signed-off-by: Yuke Peng <pykfirst@gmail.com>
> ---
>   hw/i386/intel_iommu.c         | 28 ++++++++++++++++++++++++++++
>   hw/i386/trace-events          |  1 +
>   include/hw/i386/intel_iommu.h |  1 +
>   3 files changed, 30 insertions(+)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 5085a6fee3..dfa2f979e7 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2410,6 +2410,22 @@ static void vtd_handle_gcmd_ire(IntelIOMMUState *s, bool en)
>       }
>   }
>   
> +/* Handle Compatibility Format Interrupts Enable/Disable */
> +static void vtd_handle_gcmd_cfi(IntelIOMMUState *s, bool en)
> +{
> +    trace_vtd_cfi_enable(en);
> +
> +    if (en) {
> +        s->cfi_enabled = true;
> +        /* Ok - report back to driver */
> +        vtd_set_clear_mask_long(s, DMAR_GSTS_REG, 0, VTD_GSTS_CFIS);
> +    } else {
> +        s->cfi_enabled = false;
> +        /* Ok - report back to driver */
> +        vtd_set_clear_mask_long(s, DMAR_GSTS_REG, VTD_GSTS_CFIS, 0);
> +    }
You have the same comment in both branches of the if statement, couldn't
we put it between 'trace' and 'if'?

The reason I put the comment within the branches is that the function 

'vtd_handle_gcmd_cfi'  is similar to the function 'vtd_handle_gcmd_ire' 

and  'vtd_handle_gcmd_te'. So for consistency, I also put comments 

within the branches instead of in front of 'if'. Besides, I think the comments

only explain the statement 'vtd_set_clear_mask_long'.

The 'vtd_handle_gcmd_ire' function:

    ok fine

> /* Handle Interrupt Remap Enable/Disable */
> static void vtd_handle_gcmd_ire(IntelIOMMUState *s, bool en)
> {
>     trace_vtd_ir_enable(en);
> 
>     if (en) {
>         s->intr_enabled = true;
>         /* Ok - report back to driver */
>         vtd_set_clear_mask_long(s, DMAR_GSTS_REG, 0, VTD_GSTS_IRES);
>     } else {
>         s->intr_enabled = false;
>         /* Ok - report back to driver */
>         vtd_set_clear_mask_long(s, DMAR_GSTS_REG, VTD_GSTS_IRES, 0);
>     }
> }