| Message ID | 20220822090811.427029-3-ani@anisinha.ca (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | enable ATS per pcie root port, not globally | expand |
On Mon, 22 Aug 2022 14:38:09 +0530 Ani Sinha <ani@anisinha.ca> wrote: > Currently the bit 0 of the flags field of Root Port ATS capability reporting > structure sub-table under the DMAR table is set to 1. This indicates ALL_PORTS, > thus enabling ATS capability for all pcie roots without the ability to turn off > ATS for some ports and leaving ATS on for others. > > This change clears the bit 0 of the flags field of the above structure and > explicitly adds scopes for every pcie root port in the structure so that ATS > is enabled for all of them. In future, we might add new attribite to the root > ports so that we can selectively enable ATS for some and leave ATS off for > others. Thanks, it was worth a try, unfortunately since we are shooting in dark this time it was a miss. > Signed-off-by: Ani Sinha <ani@anisinha.ca> > Suggested-by: Michael Tsirkin <mst@redhat.com> > --- > hw/i386/acpi-build.c | 74 ++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 72 insertions(+), 2 deletions(-) > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index 0355bd3dda..9c5a555536 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -60,6 +60,7 @@ > #include "hw/i386/fw_cfg.h" > #include "hw/i386/ich9.h" > #include "hw/pci/pci_bus.h" > +#include "hw/pci/pcie_port.h" > #include "hw/pci-host/q35.h" > #include "hw/i386/x86-iommu.h" > > @@ -2118,6 +2119,60 @@ dmar_host_bridges(Object *obj, void *opaque) > return 0; > } > > +/* > + * Insert DMAR scope for PCIE root ports > + */ > +static void > +insert_pcie_root_port_scope(PCIBus *bus, PCIDevice *dev, void *opaque) > +{ > + const size_t device_scope_size = 6 + 2; > + /* device scope structure + 1 path entry */ > + GArray *scope_blob = opaque; > + > + /* > + * We are only interested in PCIE root ports. We can extend > + * this to check for specific properties of PCIE root ports and based > + * on that remove some ports from having ATS capability. > + */ > + if (!object_dynamic_cast(OBJECT(dev), TYPE_PCIE_ROOT_PORT)) { > + return; > + } > + > + /* Dmar Scope Type: 0x02 for all PCIE root ports */ > + build_append_int_noprefix(scope_blob, 0x02, 1); > + > + /* length */ > + build_append_int_noprefix(scope_blob, device_scope_size, 1); > + /* reserved */ > + build_append_int_noprefix(scope_blob, 0, 2); > + /* enumeration_id */ > + build_append_int_noprefix(scope_blob, 0, 1); > + /* bus */ > + build_append_int_noprefix(scope_blob, pci_bus_num(bus), 1); > + /* device */ > + build_append_int_noprefix(scope_blob, PCI_SLOT(dev->devfn), 1); > + /* function */ > + build_append_int_noprefix(scope_blob, PCI_FUNC(dev->devfn), 1); > +} > + > +/* For a given PCI host bridge, walk and insert DMAR scope */ > +static int > +dmar_pcie_root_ports(Object *obj, void *opaque) > +{ > + GArray *scope_blob = opaque; > + > + if (object_dynamic_cast(obj, TYPE_PCI_HOST_BRIDGE)) { > + PCIBus *bus = PCI_HOST_BRIDGE(obj)->bus; > + > + if (bus && !pci_bus_bypass_iommu(bus)) { > + pci_for_each_device_under_bus(bus, insert_pcie_root_port_scope, > + scope_blob); > + } > + } > + > + return 0; > +} > + > /* > * Intel ® Virtualization Technology for Directed I/O > * Architecture Specification. Revision 3.3 > @@ -2190,11 +2245,26 @@ build_dmar_q35(GArray *table_data, BIOSLinker *linker, const char *oem_id, > > if (iommu->dt_supported) { > /* 8.5 Root Port ATS Capability Reporting Structure */ > + /* > + * A PCI bus walk, for each PCIE root port. > + * Since we did not enable ALL_PORTS bit in the flags above, we > + * need to add the scope for each pcie root port explicitly > + * that are attached to bus0 with iommu enabled. > + */ > + scope_blob = g_array_new(false, true, 1); > + object_child_foreach_recursive(object_get_root(), > + dmar_pcie_root_ports, scope_blob); > + > build_append_int_noprefix(table_data, 2, 2); /* Type */ > - build_append_int_noprefix(table_data, 8, 2); /* Length */ > - build_append_int_noprefix(table_data, 1 /* ALL_PORTS */, 1); /* Flags */ > + build_append_int_noprefix(table_data, > + 8 + scope_blob->len, 2); /* Length */ > + build_append_int_noprefix(table_data, 0, 1); /* Flags */ > build_append_int_noprefix(table_data, 0, 1); /* Reserved */ > build_append_int_noprefix(table_data, 0, 2); /* Segment Number */ > + > + /* now add the scope to the sub-table */ > + g_array_append_vals(table_data, scope_blob->data, scope_blob->len); > + g_array_free(scope_blob, true); > } > > acpi_table_end(linker, &table);
On Wed, Aug 24, 2022 at 8:54 PM Igor Mammedov <imammedo@redhat.com> wrote: > On Mon, 22 Aug 2022 14:38:09 +0530 > Ani Sinha <ani@anisinha.ca> wrote: > > > Currently the bit 0 of the flags field of Root Port ATS capability > reporting > > structure sub-table under the DMAR table is set to 1. This indicates > ALL_PORTS, > > thus enabling ATS capability for all pcie roots without the ability to > turn off > > ATS for some ports and leaving ATS on for others. > > > > This change clears the bit 0 of the flags field of the above structure > and > > explicitly adds scopes for every pcie root port in the structure so that > ATS > > is enabled for all of them. In future, we might add new attribite to the > root > > ports so that we can selectively enable ATS for some and leave ATS off > for > > others. > > Thanks, it was worth a try, > unfortunately since we are shooting in dark this time it was a miss. So I take it that even with this patch Windows still exhibited the issue? Is it worth pushing the patch anyway? > > > > Signed-off-by: Ani Sinha <ani@anisinha.ca> > > Suggested-by: Michael Tsirkin <mst@redhat.com> > > --- > > hw/i386/acpi-build.c | 74 ++++++++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 72 insertions(+), 2 deletions(-) > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > index 0355bd3dda..9c5a555536 100644 > > --- a/hw/i386/acpi-build.c > > +++ b/hw/i386/acpi-build.c > > @@ -60,6 +60,7 @@ > > #include "hw/i386/fw_cfg.h" > > #include "hw/i386/ich9.h" > > #include "hw/pci/pci_bus.h" > > +#include "hw/pci/pcie_port.h" > > #include "hw/pci-host/q35.h" > > #include "hw/i386/x86-iommu.h" > > > > @@ -2118,6 +2119,60 @@ dmar_host_bridges(Object *obj, void *opaque) > > return 0; > > } > > > > +/* > > + * Insert DMAR scope for PCIE root ports > > + */ > > +static void > > +insert_pcie_root_port_scope(PCIBus *bus, PCIDevice *dev, void *opaque) > > +{ > > + const size_t device_scope_size = 6 + 2; > > + /* device scope structure + 1 path > entry */ > > + GArray *scope_blob = opaque; > > + > > + /* > > + * We are only interested in PCIE root ports. We can extend > > + * this to check for specific properties of PCIE root ports and > based > > + * on that remove some ports from having ATS capability. > > + */ > > + if (!object_dynamic_cast(OBJECT(dev), TYPE_PCIE_ROOT_PORT)) { > > + return; > > + } > > + > > + /* Dmar Scope Type: 0x02 for all PCIE root ports */ > > + build_append_int_noprefix(scope_blob, 0x02, 1); > > + > > + /* length */ > > + build_append_int_noprefix(scope_blob, device_scope_size, 1); > > + /* reserved */ > > + build_append_int_noprefix(scope_blob, 0, 2); > > + /* enumeration_id */ > > + build_append_int_noprefix(scope_blob, 0, 1); > > + /* bus */ > > + build_append_int_noprefix(scope_blob, pci_bus_num(bus), 1); > > + /* device */ > > + build_append_int_noprefix(scope_blob, PCI_SLOT(dev->devfn), 1); > > + /* function */ > > + build_append_int_noprefix(scope_blob, PCI_FUNC(dev->devfn), 1); > > +} > > + > > +/* For a given PCI host bridge, walk and insert DMAR scope */ > > +static int > > +dmar_pcie_root_ports(Object *obj, void *opaque) > > +{ > > + GArray *scope_blob = opaque; > > + > > + if (object_dynamic_cast(obj, TYPE_PCI_HOST_BRIDGE)) { > > + PCIBus *bus = PCI_HOST_BRIDGE(obj)->bus; > > + > > + if (bus && !pci_bus_bypass_iommu(bus)) { > > + pci_for_each_device_under_bus(bus, > insert_pcie_root_port_scope, > > + scope_blob); > > + } > > + } > > + > > + return 0; > > +} > > + > > /* > > * Intel ® Virtualization Technology for Directed I/O > > * Architecture Specification. Revision 3.3 > > @@ -2190,11 +2245,26 @@ build_dmar_q35(GArray *table_data, BIOSLinker > *linker, const char *oem_id, > > > > if (iommu->dt_supported) { > > /* 8.5 Root Port ATS Capability Reporting Structure */ > > + /* > > + * A PCI bus walk, for each PCIE root port. > > + * Since we did not enable ALL_PORTS bit in the flags above, we > > + * need to add the scope for each pcie root port explicitly > > + * that are attached to bus0 with iommu enabled. > > + */ > > + scope_blob = g_array_new(false, true, 1); > > + object_child_foreach_recursive(object_get_root(), > > + dmar_pcie_root_ports, > scope_blob); > > + > > build_append_int_noprefix(table_data, 2, 2); /* Type */ > > - build_append_int_noprefix(table_data, 8, 2); /* Length */ > > - build_append_int_noprefix(table_data, 1 /* ALL_PORTS */, 1); /* > Flags */ > > + build_append_int_noprefix(table_data, > > + 8 + scope_blob->len, 2); /* Length */ > > + build_append_int_noprefix(table_data, 0, 1); /* Flags */ > > build_append_int_noprefix(table_data, 0, 1); /* Reserved */ > > build_append_int_noprefix(table_data, 0, 2); /* Segment Number > */ > > + > > + /* now add the scope to the sub-table */ > > + g_array_append_vals(table_data, scope_blob->data, > scope_blob->len); > > + g_array_free(scope_blob, true); > > } > > > > acpi_table_end(linker, &table); > >
On Wed, 24 Aug 2022 21:46:58 +0530 Ani Sinha <ani@anisinha.ca> wrote: > On Wed, Aug 24, 2022 at 8:54 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > On Mon, 22 Aug 2022 14:38:09 +0530 > > Ani Sinha <ani@anisinha.ca> wrote: > > > > > Currently the bit 0 of the flags field of Root Port ATS capability > > reporting > > > structure sub-table under the DMAR table is set to 1. This indicates > > ALL_PORTS, > > > thus enabling ATS capability for all pcie roots without the ability to > > turn off > > > ATS for some ports and leaving ATS on for others. > > > > > > This change clears the bit 0 of the flags field of the above structure > > and > > > explicitly adds scopes for every pcie root port in the structure so that > > ATS > > > is enabled for all of them. In future, we might add new attribite to the > > root > > > ports so that we can selectively enable ATS for some and leave ATS off > > for > > > others. > > > > Thanks, it was worth a try, > > unfortunately since we are shooting in dark this time it was a miss. > > > So I take it that even with this patch Windows still exhibited the issue? unfortunately, yep > Is it worth pushing the patch anyway? it will extra cpu time and guest RAM, so unless we have to have to I'd rather not. > > > > > > > > Signed-off-by: Ani Sinha <ani@anisinha.ca> > > > Suggested-by: Michael Tsirkin <mst@redhat.com> > > > --- > > > hw/i386/acpi-build.c | 74 ++++++++++++++++++++++++++++++++++++++++++-- > > > 1 file changed, 72 insertions(+), 2 deletions(-) > > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > > index 0355bd3dda..9c5a555536 100644 > > > --- a/hw/i386/acpi-build.c > > > +++ b/hw/i386/acpi-build.c > > > @@ -60,6 +60,7 @@ > > > #include "hw/i386/fw_cfg.h" > > > #include "hw/i386/ich9.h" > > > #include "hw/pci/pci_bus.h" > > > +#include "hw/pci/pcie_port.h" > > > #include "hw/pci-host/q35.h" > > > #include "hw/i386/x86-iommu.h" > > > > > > @@ -2118,6 +2119,60 @@ dmar_host_bridges(Object *obj, void *opaque) > > > return 0; > > > } > > > > > > +/* > > > + * Insert DMAR scope for PCIE root ports > > > + */ > > > +static void > > > +insert_pcie_root_port_scope(PCIBus *bus, PCIDevice *dev, void *opaque) > > > +{ > > > + const size_t device_scope_size = 6 + 2; > > > + /* device scope structure + 1 path > > entry */ > > > + GArray *scope_blob = opaque; > > > + > > > + /* > > > + * We are only interested in PCIE root ports. We can extend > > > + * this to check for specific properties of PCIE root ports and > > based > > > + * on that remove some ports from having ATS capability. > > > + */ > > > + if (!object_dynamic_cast(OBJECT(dev), TYPE_PCIE_ROOT_PORT)) { > > > + return; > > > + } > > > + > > > + /* Dmar Scope Type: 0x02 for all PCIE root ports */ > > > + build_append_int_noprefix(scope_blob, 0x02, 1); > > > + > > > + /* length */ > > > + build_append_int_noprefix(scope_blob, device_scope_size, 1); > > > + /* reserved */ > > > + build_append_int_noprefix(scope_blob, 0, 2); > > > + /* enumeration_id */ > > > + build_append_int_noprefix(scope_blob, 0, 1); > > > + /* bus */ > > > + build_append_int_noprefix(scope_blob, pci_bus_num(bus), 1); > > > + /* device */ > > > + build_append_int_noprefix(scope_blob, PCI_SLOT(dev->devfn), 1); > > > + /* function */ > > > + build_append_int_noprefix(scope_blob, PCI_FUNC(dev->devfn), 1); > > > +} > > > + > > > +/* For a given PCI host bridge, walk and insert DMAR scope */ > > > +static int > > > +dmar_pcie_root_ports(Object *obj, void *opaque) > > > +{ > > > + GArray *scope_blob = opaque; > > > + > > > + if (object_dynamic_cast(obj, TYPE_PCI_HOST_BRIDGE)) { > > > + PCIBus *bus = PCI_HOST_BRIDGE(obj)->bus; > > > + > > > + if (bus && !pci_bus_bypass_iommu(bus)) { > > > + pci_for_each_device_under_bus(bus, > > insert_pcie_root_port_scope, > > > + scope_blob); > > > + } > > > + } > > > + > > > + return 0; > > > +} > > > + > > > /* > > > * Intel ® Virtualization Technology for Directed I/O > > > * Architecture Specification. Revision 3.3 > > > @@ -2190,11 +2245,26 @@ build_dmar_q35(GArray *table_data, BIOSLinker > > *linker, const char *oem_id, > > > > > > if (iommu->dt_supported) { > > > /* 8.5 Root Port ATS Capability Reporting Structure */ > > > + /* > > > + * A PCI bus walk, for each PCIE root port. > > > + * Since we did not enable ALL_PORTS bit in the flags above, we > > > + * need to add the scope for each pcie root port explicitly > > > + * that are attached to bus0 with iommu enabled. > > > + */ > > > + scope_blob = g_array_new(false, true, 1); > > > + object_child_foreach_recursive(object_get_root(), > > > + dmar_pcie_root_ports, > > scope_blob); > > > + > > > build_append_int_noprefix(table_data, 2, 2); /* Type */ > > > - build_append_int_noprefix(table_data, 8, 2); /* Length */ > > > - build_append_int_noprefix(table_data, 1 /* ALL_PORTS */, 1); /* > > Flags */ > > > + build_append_int_noprefix(table_data, > > > + 8 + scope_blob->len, 2); /* Length */ > > > + build_append_int_noprefix(table_data, 0, 1); /* Flags */ > > > build_append_int_noprefix(table_data, 0, 1); /* Reserved */ > > > build_append_int_noprefix(table_data, 0, 2); /* Segment Number > > */ > > > + > > > + /* now add the scope to the sub-table */ > > > + g_array_append_vals(table_data, scope_blob->data, > > scope_blob->len); > > > + g_array_free(scope_blob, true); > > > } > > > > > > acpi_table_end(linker, &table); > > > >
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 0355bd3dda..9c5a555536 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -60,6 +60,7 @@ #include "hw/i386/fw_cfg.h" #include "hw/i386/ich9.h" #include "hw/pci/pci_bus.h" +#include "hw/pci/pcie_port.h" #include "hw/pci-host/q35.h" #include "hw/i386/x86-iommu.h" @@ -2118,6 +2119,60 @@ dmar_host_bridges(Object *obj, void *opaque) return 0; } +/* + * Insert DMAR scope for PCIE root ports + */ +static void +insert_pcie_root_port_scope(PCIBus *bus, PCIDevice *dev, void *opaque) +{ + const size_t device_scope_size = 6 + 2; + /* device scope structure + 1 path entry */ + GArray *scope_blob = opaque; + + /* + * We are only interested in PCIE root ports. We can extend + * this to check for specific properties of PCIE root ports and based + * on that remove some ports from having ATS capability. + */ + if (!object_dynamic_cast(OBJECT(dev), TYPE_PCIE_ROOT_PORT)) { + return; + } + + /* Dmar Scope Type: 0x02 for all PCIE root ports */ + build_append_int_noprefix(scope_blob, 0x02, 1); + + /* length */ + build_append_int_noprefix(scope_blob, device_scope_size, 1); + /* reserved */ + build_append_int_noprefix(scope_blob, 0, 2); + /* enumeration_id */ + build_append_int_noprefix(scope_blob, 0, 1); + /* bus */ + build_append_int_noprefix(scope_blob, pci_bus_num(bus), 1); + /* device */ + build_append_int_noprefix(scope_blob, PCI_SLOT(dev->devfn), 1); + /* function */ + build_append_int_noprefix(scope_blob, PCI_FUNC(dev->devfn), 1); +} + +/* For a given PCI host bridge, walk and insert DMAR scope */ +static int +dmar_pcie_root_ports(Object *obj, void *opaque) +{ + GArray *scope_blob = opaque; + + if (object_dynamic_cast(obj, TYPE_PCI_HOST_BRIDGE)) { + PCIBus *bus = PCI_HOST_BRIDGE(obj)->bus; + + if (bus && !pci_bus_bypass_iommu(bus)) { + pci_for_each_device_under_bus(bus, insert_pcie_root_port_scope, + scope_blob); + } + } + + return 0; +} + /* * Intel ® Virtualization Technology for Directed I/O * Architecture Specification. Revision 3.3 @@ -2190,11 +2245,26 @@ build_dmar_q35(GArray *table_data, BIOSLinker *linker, const char *oem_id, if (iommu->dt_supported) { /* 8.5 Root Port ATS Capability Reporting Structure */ + /* + * A PCI bus walk, for each PCIE root port. + * Since we did not enable ALL_PORTS bit in the flags above, we + * need to add the scope for each pcie root port explicitly + * that are attached to bus0 with iommu enabled. + */ + scope_blob = g_array_new(false, true, 1); + object_child_foreach_recursive(object_get_root(), + dmar_pcie_root_ports, scope_blob); + build_append_int_noprefix(table_data, 2, 2); /* Type */ - build_append_int_noprefix(table_data, 8, 2); /* Length */ - build_append_int_noprefix(table_data, 1 /* ALL_PORTS */, 1); /* Flags */ + build_append_int_noprefix(table_data, + 8 + scope_blob->len, 2); /* Length */ + build_append_int_noprefix(table_data, 0, 1); /* Flags */ build_append_int_noprefix(table_data, 0, 1); /* Reserved */ build_append_int_noprefix(table_data, 0, 2); /* Segment Number */ + + /* now add the scope to the sub-table */ + g_array_append_vals(table_data, scope_blob->data, scope_blob->len); + g_array_free(scope_blob, true); } acpi_table_end(linker, &table);
Currently the bit 0 of the flags field of Root Port ATS capability reporting structure sub-table under the DMAR table is set to 1. This indicates ALL_PORTS, thus enabling ATS capability for all pcie roots without the ability to turn off ATS for some ports and leaving ATS on for others. This change clears the bit 0 of the flags field of the above structure and explicitly adds scopes for every pcie root port in the structure so that ATS is enabled for all of them. In future, we might add new attribite to the root ports so that we can selectively enable ATS for some and leave ATS off for others. Signed-off-by: Ani Sinha <ani@anisinha.ca> Suggested-by: Michael Tsirkin <mst@redhat.com> --- hw/i386/acpi-build.c | 74 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 72 insertions(+), 2 deletions(-)