| Message ID | 1536684589-11718-7-git-send-email-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86_iommu/amd: add interrupt remap support | expand |
On Tue, Sep 11, 2018 at 11:49:49AM -0500, Brijesh Singh wrote: > Now that amd-iommu support interrupt remapping, enable the GASup in IVRS > table and GASup in extended feature register to indicate that IOMMU > support guest virtual APIC mode. > > Note that the GAMSup is set to zero to indicate that Guest Virtual > APIC does not support advanced interrupt features (i.e virtualized > interrupts using the guest virtual APIC). > > See Table 21 from IOMMU spec for interrupt virtualization controls > > IOMMU spec: https://support.amd.com/TechDocs/48882_IOMMU.pdf > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Richard Henderson <rth@twiddle.net> > Cc: Eduardo Habkost <ehabkost@redhat.com> > Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> > Cc: Tom Lendacky <Thomas.Lendacky@amd.com> > Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > hw/i386/acpi-build.c | 3 ++- > hw/i386/amd_iommu.h | 2 +- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index 5c2c638..1cbc8ba 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -2565,7 +2565,8 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker) > build_append_int_noprefix(table_data, > (48UL << 30) | /* HATS */ > (48UL << 28) | /* GATS */ > - (1UL << 2), /* GTSup */ > + (1UL << 2) | /* GTSup */ > + (1UL << 6), /* GASup */ Sorry if I misunderstood - is this for nested? I'm a bit confused here... IIUC in your previous patches you didn't really implement guest_mode==1 case in IRTEs. So if you have this set then the guest should be able to setup IRTEs with guest_mode==1? How did it work? Thanks,
On Tue, 11 Sep 2018 11:49:49 -0500 Brijesh Singh <brijesh.singh@amd.com> wrote: > Now that amd-iommu support interrupt remapping, enable the GASup in IVRS > table and GASup in extended feature register to indicate that IOMMU > support guest virtual APIC mode. > > Note that the GAMSup is set to zero to indicate that Guest Virtual > APIC does not support advanced interrupt features (i.e virtualized > interrupts using the guest virtual APIC). > > See Table 21 from IOMMU spec for interrupt virtualization controls > > IOMMU spec: https://support.amd.com/TechDocs/48882_IOMMU.pdf Table numbers and URLs tend to change over long time span, pls exact spec title and revision to above info, so one could easily google it. > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Richard Henderson <rth@twiddle.net> > Cc: Eduardo Habkost <ehabkost@redhat.com> > Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> > Cc: Tom Lendacky <Thomas.Lendacky@amd.com> > Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > hw/i386/acpi-build.c | 3 ++- > hw/i386/amd_iommu.h | 2 +- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index 5c2c638..1cbc8ba 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -2565,7 +2565,8 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker) > build_append_int_noprefix(table_data, > (48UL << 30) | /* HATS */ > (48UL << 28) | /* GATS */ > - (1UL << 2), /* GTSup */ > + (1UL << 2) | /* GTSup */ > + (1UL << 6), /* GASup */ > 4); > /* > * Type 1 device entry reporting all devices > diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h > index 1dab974..5defaac 100644 > --- a/hw/i386/amd_iommu.h > +++ b/hw/i386/amd_iommu.h > @@ -177,7 +177,7 @@ > /* extended feature support */ > #define AMDVI_EXT_FEATURES (AMDVI_FEATURE_PREFETCH | AMDVI_FEATURE_PPR | \ > AMDVI_FEATURE_IA | AMDVI_FEATURE_GT | AMDVI_FEATURE_HE | \ > - AMDVI_GATS_MODE | AMDVI_HATS_MODE) > + AMDVI_GATS_MODE | AMDVI_HATS_MODE | AMDVI_FEATURE_GA) > > /* capabilities header */ > #define AMDVI_CAPAB_FEATURES (AMDVI_CAPAB_FLAT_EXT | \
On 09/11/2018 11:52 PM, Peter Xu wrote: ... >> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c >> index 5c2c638..1cbc8ba 100644 >> --- a/hw/i386/acpi-build.c >> +++ b/hw/i386/acpi-build.c >> @@ -2565,7 +2565,8 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker) >> build_append_int_noprefix(table_data, >> (48UL << 30) | /* HATS */ >> (48UL << 28) | /* GATS */ >> - (1UL << 2), /* GTSup */ >> + (1UL << 2) | /* GTSup */ >> + (1UL << 6), /* GASup */ > > Sorry if I misunderstood - is this for nested? > > I'm a bit confused here... IIUC in your previous patches you didn't > really implement guest_mode==1 case in IRTEs. So if you have this set > then the guest should be able to setup IRTEs with guest_mode==1? How > did it work? > Yes, sometime spec can be confusing ;) let me see if I can explain it. Suravee, please correct me if I misunderstood something - A legacy interrupt remap support is available on all IOMMU versions. Guest OS makes the decision whether to use the feature. Guest OS sets the IV bit in DTE to indicate when there is a valid interrupt map (See Table 7 DTE definition). In this mode, IRTE is 32-bit. The field details are available in Table 20 - Section 2.2.5.2. The third patch in this series implements the support for this case. (NOTE: As I explained [1], the Linux guest enables intr remap only when it sees a special IVHD IOAPIC type device is present in IVRS) [1] https://marc.info/?l=qemu-devel&m=153677956506196&w=2 - When AVIC is used in the guest, the intr remap logic is different. Based on the guest_mode, IOMMU and AVIC may need to work together to remap the interrupts (IOMMU spec refers this as Guest Virtual APIC Enabled -- Section 2.2.5.3). The GASup bit in extended feature register and IVHD IOMMU feature reporting field in IVRS are used to tell whether the IOMMU supports intr remap when AVIC is enabled in the guest. In this mode, the IRTE is 128-bit. To make things interesting, there is a GAMSup bit in extended feature register. It is used by IOMMU to tell the AVIC supported modes: a) 0 = intr_remap only (i.e IOMMU can remap the interrupt on its own) b) 1 = Both guest AVIC and IOMMU will work together to remap the intr The patch 5 in the series implements the intr_remap (#a) I have not implemented the mode=1. I am not sure if its worth implementing this mode for the emulated IOMMU case. See Table 21 for control knobs from IOMMU. This patch series implements the first three rows. I hope my explanation does not add more confusions ;) -Brijesh
Peter, On 9/12/18 11:52 AM, Peter Xu wrote: > On Tue, Sep 11, 2018 at 11:49:49AM -0500, Brijesh Singh wrote: >> Now that amd-iommu support interrupt remapping, enable the GASup in IVRS >> table and GASup in extended feature register to indicate that IOMMU >> support guest virtual APIC mode. >> >> Note that the GAMSup is set to zero to indicate that Guest Virtual >> APIC does not support advanced interrupt features (i.e virtualized >> interrupts using the guest virtual APIC). >> >> See Table 21 from IOMMU spec for interrupt virtualization controls >> >> IOMMU spec: https://support.amd.com/TechDocs/48882_IOMMU.pdf >> >> Cc: "Michael S. Tsirkin" <mst@redhat.com> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Cc: Richard Henderson <rth@twiddle.net> >> Cc: Eduardo Habkost <ehabkost@redhat.com> >> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> >> Cc: Tom Lendacky <Thomas.Lendacky@amd.com> >> Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> >> --- >> hw/i386/acpi-build.c | 3 ++- >> hw/i386/amd_iommu.h | 2 +- >> 2 files changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c >> index 5c2c638..1cbc8ba 100644 >> --- a/hw/i386/acpi-build.c >> +++ b/hw/i386/acpi-build.c >> @@ -2565,7 +2565,8 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker) >> build_append_int_noprefix(table_data, >> (48UL << 30) | /* HATS */ >> (48UL << 28) | /* GATS */ >> - (1UL << 2), /* GTSup */ >> + (1UL << 2) | /* GTSup */ >> + (1UL << 6), /* GASup */ > > Sorry if I misunderstood - is this for nested? > > I'm a bit confused here... IIUC in your previous patches you didn't > really implement guest_mode==1 case in IRTEs. So if you have this set > then the guest should be able to setup IRTEs with guest_mode==1? How > did it work? > > Thanks, > The naming of these bits are confusing. Please allow me to help explain. There are two capability bits: * GASup : This is to allow 128-bit IRTE when GAEn is set. * GAMSup : This is for Guest Virtual APIC mode support, which is not currently supported in vIOMMU. The commit message in patch 5 is incorrect. Here, we set GASup in order to allow 128-bit IRTE support, which is needed for some of future AMD IOMMU features. Thanks, Suravee
Brijesh/Peter, On 9/13/18 4:14 AM, Brijesh Singh wrote: > > > On 09/11/2018 11:52 PM, Peter Xu wrote: > ... > >>> >>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c >>> index 5c2c638..1cbc8ba 100644 >>> --- a/hw/i386/acpi-build.c >>> +++ b/hw/i386/acpi-build.c >>> @@ -2565,7 +2565,8 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker) >>> build_append_int_noprefix(table_data, >>> (48UL << 30) | /* HATS */ >>> (48UL << 28) | /* GATS */ >>> - (1UL << 2), /* GTSup */ >>> + (1UL << 2) | /* GTSup */ >>> + (1UL << 6), /* GASup */ >> >> Sorry if I misunderstood - is this for nested? >> >> I'm a bit confused here... IIUC in your previous patches you didn't >> really implement guest_mode==1 case in IRTEs. So if you have this set >> then the guest should be able to setup IRTEs with guest_mode==1? How >> did it work? >> > > Yes, sometime spec can be confusing ;) let me see if I can explain it. > > Suravee, please correct me if I misunderstood something > > - A legacy interrupt remap support is available on all IOMMU versions. > > Guest OS makes the decision whether to use the feature. Guest OS > sets the IV bit in DTE to indicate when there is a valid interrupt > map (See Table 7 DTE definition). > > In this mode, IRTE is 32-bit. The field details are available > in Table 20 - Section 2.2.5.2. The third patch in this series > implements the support for this case. This is correct. > > - When AVIC is used in the guest, the intr remap logic is different. > > Based on the guest_mode, IOMMU and AVIC may need to work together > to remap the interrupts (IOMMU spec refers this as Guest Virtual APIC > Enabled -- Section 2.2.5.3). > > The GASup bit in extended feature register and IVHD IOMMU feature > reporting field in IVRS are used to tell whether the IOMMU supports > intr remap when AVIC is enabled in the guest. > > In this mode, the IRTE is 128-bit. GASup is a prereq for GAMSup and several other features in the future. > > To make things interesting, there is a GAMSup bit in extended > feature register. It is used by IOMMU to tell the AVIC supported > modes: > > a) 0 = intr_remap only (i.e IOMMU can remap the interrupt on its own) > b) 1 = Both guest AVIC and IOMMU will work together to remap the intr > > The patch 5 in the series implements the intr_remap (#a) > > I have not implemented the mode=1. I am not sure if its worth > implementing this mode for the emulated IOMMU case. > > See Table 21 for control knobs from IOMMU. This patch series > implements the first three rows. GAMSup is guest virtual APIC mode (aka AVIC), and for vIOMMU case, would be used for nested VM. This is not currently supported for vIOMMU, Regards, Suravee > > -Brijesh
On Thu, Sep 13, 2018 at 03:36:28PM +0700, Suravee Suthikulpanit wrote: > Brijesh/Peter, > > On 9/13/18 4:14 AM, Brijesh Singh wrote: > > > > > > On 09/11/2018 11:52 PM, Peter Xu wrote: > > ... > > > > > > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > > > index 5c2c638..1cbc8ba 100644 > > > > --- a/hw/i386/acpi-build.c > > > > +++ b/hw/i386/acpi-build.c > > > > @@ -2565,7 +2565,8 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker) > > > > build_append_int_noprefix(table_data, > > > > (48UL << 30) | /* HATS */ > > > > (48UL << 28) | /* GATS */ > > > > - (1UL << 2), /* GTSup */ > > > > + (1UL << 2) | /* GTSup */ > > > > + (1UL << 6), /* GASup */ > > > > > > Sorry if I misunderstood - is this for nested? > > > > > > I'm a bit confused here... IIUC in your previous patches you didn't > > > really implement guest_mode==1 case in IRTEs. So if you have this set > > > then the guest should be able to setup IRTEs with guest_mode==1? How > > > did it work? > > > > > > > Yes, sometime spec can be confusing ;) let me see if I can explain it. > > > > Suravee, please correct me if I misunderstood something > > > > - A legacy interrupt remap support is available on all IOMMU versions. > > > > Guest OS makes the decision whether to use the feature. Guest OS > > sets the IV bit in DTE to indicate when there is a valid interrupt > > map (See Table 7 DTE definition). > > > > In this mode, IRTE is 32-bit. The field details are available > > in Table 20 - Section 2.2.5.2. The third patch in this series > > implements the support for this case. > > This is correct. > > > > > - When AVIC is used in the guest, the intr remap logic is different. > > > > Based on the guest_mode, IOMMU and AVIC may need to work together > > to remap the interrupts (IOMMU spec refers this as Guest Virtual APIC > > Enabled -- Section 2.2.5.3). > > > > The GASup bit in extended feature register and IVHD IOMMU feature > > reporting field in IVRS are used to tell whether the IOMMU supports > > intr remap when AVIC is enabled in the guest. > > > > In this mode, the IRTE is 128-bit. > > GASup is a prereq for GAMSup and several other features in the future. > > > > > To make things interesting, there is a GAMSup bit in extended > > feature register. It is used by IOMMU to tell the AVIC supported > > modes: > > > > a) 0 = intr_remap only (i.e IOMMU can remap the interrupt on its own) > > b) 1 = Both guest AVIC and IOMMU will work together to remap the intr > > > > The patch 5 in the series implements the intr_remap (#a) > > > > I have not implemented the mode=1. I am not sure if its worth > > implementing this mode for the emulated IOMMU case. > > > > See Table 21 for control knobs from IOMMU. This patch series > > implements the first three rows. > > GAMSup is guest virtual APIC mode (aka AVIC), and for vIOMMU case, > would be used for nested VM. This is not currently supported for vIOMMU, Hi, Brijesh, Suravee, Thank you for the explanations. Now with that I understand the states and, yes, I agree that the patchset has implemented 128bits int remap so you should have GASup set on both places (and keep GAMSup==0). Then please take mine: Reviewed-by: Peter Xu <peterx@redhat.com> Actually I don't quite understand why these information needs to be declared in two places (ACPI and MMIO region). More interestingly, I noticed that there is a note in the ACPI chapter 5: Software Implementation Note: Information conveyed in the IVRS overrides the corresponding information available through the IOMMU hardware registers. System software is required to honor the ACPI settings. Does it mean that there could be chance where both data do not match? I would be curious why not only declare the capabilities at one place? AFAIU that's what Intel is doing, e.g., Intel ACPI DRHD structures do not have the feature bit declarations but they are all in the capability registers. Thanks,
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 5c2c638..1cbc8ba 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -2565,7 +2565,8 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker) build_append_int_noprefix(table_data, (48UL << 30) | /* HATS */ (48UL << 28) | /* GATS */ - (1UL << 2), /* GTSup */ + (1UL << 2) | /* GTSup */ + (1UL << 6), /* GASup */ 4); /* * Type 1 device entry reporting all devices diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h index 1dab974..5defaac 100644 --- a/hw/i386/amd_iommu.h +++ b/hw/i386/amd_iommu.h @@ -177,7 +177,7 @@ /* extended feature support */ #define AMDVI_EXT_FEATURES (AMDVI_FEATURE_PREFETCH | AMDVI_FEATURE_PPR | \ AMDVI_FEATURE_IA | AMDVI_FEATURE_GT | AMDVI_FEATURE_HE | \ - AMDVI_GATS_MODE | AMDVI_HATS_MODE) + AMDVI_GATS_MODE | AMDVI_HATS_MODE | AMDVI_FEATURE_GA) /* capabilities header */ #define AMDVI_CAPAB_FEATURES (AMDVI_CAPAB_FLAT_EXT | \
Now that amd-iommu support interrupt remapping, enable the GASup in IVRS table and GASup in extended feature register to indicate that IOMMU support guest virtual APIC mode. Note that the GAMSup is set to zero to indicate that Guest Virtual APIC does not support advanced interrupt features (i.e virtualized interrupts using the guest virtual APIC). See Table 21 from IOMMU spec for interrupt virtualization controls IOMMU spec: https://support.amd.com/TechDocs/48882_IOMMU.pdf Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Richard Henderson <rth@twiddle.net> Cc: Eduardo Habkost <ehabkost@redhat.com> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> Cc: Tom Lendacky <Thomas.Lendacky@amd.com> Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- hw/i386/acpi-build.c | 3 ++- hw/i386/amd_iommu.h | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-)