Re: [Xen-devel] [PATCH v4 10/28] x86/vvtd: Enable Interrupt Remapping through GCMD

[Chao Gao <chao.gao@xxxxxxxxx>]

On Mon, Feb 12, 2018 at 11:30:18AM +0000, Roger Pau Monné wrote:
>On Sun, Feb 11, 2018 at 01:05:01PM +0800, Chao Gao wrote:
>> On Fri, Feb 09, 2018 at 05:15:17PM +0000, Roger Pau Monné wrote:
>> >On Fri, Nov 17, 2017 at 02:22:17PM +0800, Chao Gao wrote:
>> >> +static void write_gcmd_ire(struct vvtd *vvtd, uint32_t val)
>> >> +{
>> >> +    bool set = val & DMA_GCMD_IRE;
>> >> +
>> >> +    vvtd_info("%sable Interrupt Remapping\n", set ? "En" : "Dis");
>> >> +
>> >> +    vvtd->hw.intremap_enabled = set;
>> >> +    (set ? vvtd_set_bit : vvtd_clear_bit)
>> >> +        (vvtd, DMAR_GSTS_REG, DMA_GSTS_IRES_SHIFT);
>> >> +}
>> >> +
>> >>  static void write_gcmd_sirtp(struct vvtd *vvtd, uint32_t val)
>> >>  {
>> >>      uint64_t irta = vvtd_get_reg_quad(vvtd, DMAR_IRTA_REG);
>> >> @@ -131,16 +205,29 @@ static void write_gcmd_sirtp(struct vvtd *vvtd, 
>> >> uint32_t val)
>> >>       * the 'Set Interrupt Remap Table Pointer' operation.
>> >>       */
>> >>      vvtd_clear_bit(vvtd, DMAR_GSTS_REG, DMA_GSTS_SIRTPS_SHIFT);
>> >> +    if ( vvtd->hw.intremap_enabled )
>> >> +        vvtd_info("Update Interrupt Remapping Table when active\n");
>> >>  
>> >>      if ( gfn_x(vvtd->hw.irt) != PFN_DOWN(DMA_IRTA_ADDR(irta)) ||
>> >>           vvtd->hw.irt_max_entry != DMA_IRTA_SIZE(irta) )
>> >>      {
>> >> +        if ( vvtd->irt_base )
>> >> +        {
>> >> +            unmap_guest_pages(vvtd->irt_base,
>> >> +                              PFN_UP(vvtd->hw.irt_max_entry *
>> >> +                                     sizeof(struct iremap_entry)));
>> >> +            vvtd->irt_base = NULL;
>> >> +        }
>> >
>> >Shouldn't this be done when sirtp is switched off, instead of when
>> >it's updated?
>> >
>> >What happens in the following scenario:
>> >
>> >- Guest writes gfn to irta.
>> >- Guest enables sirtps.
>> >- Guest disables sirtps.
>> 
>> Disabling SIRTP isn't clear to me. Maybe you mean writing to GCMD with
>> SIRTP cleared. Hardware ignores write 0 to SIRTP I think becasue SIRTP
>> is a one-shot bit. Please refer to the example in VT-d spec 10.4.4.
>> Each time IRTP is updated, the old mapping should be destroyed and the
>> new mapping should be created.
>
>After reading the spec I agree, there's no such thing as clearing
>SIRTP.
>
>You should however unmap the IRTA address when IRE is cleared
>(interrupt remapping disabled), which AFAICT you don't to do now.

Yes. I agree.

Thanks
Chao

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel