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

[cmd <clement.mathieudrif.etu@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2745 bytes --]


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);
> >     }
> > }

[-- Attachment #2: Type: text/html, Size: 5160 bytes --]