1. Patchew
  2. QEMU
  3. View series

[PATCH v2] xio3130_upstream: Add ACS (Access Control Services) capability

Paul Schlacter posted 1 patch 1 year, 8 months ago
Failed in applying to current master (apply log)
There is a newer version of this series
1 file changed, 11 insertions(+), 1 deletion(-)
[PATCH v2] xio3130_upstream: Add ACS (Access Control Services) capability
Posted by Paul Schlacter 1 year, 8 months ago 
v1 -> v2:
- Allow ACS to be disabled.
- Suggested by Michael S. Tsirkin, use disable-acs to set property.

v1:
- Add ACS (Access Control Services) capability.

If it is a pcie device, check that all devices on the path from

the device to the root complex have ACS enabled, and then the

device will become an iommu_group.

it will have the effect of isolation



Signed-off-by: wangliang <wlfightup@gmail.com>

Signed-off-by: wangliang <wangliang40@baidu.com>



---

 hw/pci-bridge/xio3130_upstream.c | 12 +++++++++++-

 1 file changed, 11 insertions(+), 1 deletion(-)


diff --git a/hw/pci-bridge/xio3130_upstream.c
b/hw/pci-bridge/xio3130_upstream.c

index 2df952222b..5433d06fb3 100644

--- a/hw/pci-bridge/xio3130_upstream.c

+++ b/hw/pci-bridge/xio3130_upstream.c

@@ -24,6 +24,7 @@

 #include "hw/pci/msi.h"

 #include "hw/pci/pcie.h"

 #include "hw/pci/pcie_port.h"

+#include "hw/qdev-properties.h"

 #include "migration/vmstate.h"

 #include "qemu/module.h"



@@ -59,6 +60,7 @@ static void xio3130_upstream_reset(DeviceState *qdev)

 static void xio3130_upstream_realize(PCIDevice *d, Error **errp)

 {

     PCIEPort *p = PCIE_PORT(d);

+    PCIESlot *s = PCIE_SLOT(d);

     int rc;



     pci_bridge_initfn(d, TYPE_PCIE_BUS);

@@ -94,7 +96,9 @@ static void xio3130_upstream_realize(PCIDevice *d, Error
**errp)

         goto err;

     }



-    pcie_acs_init(d, XIO3130_ACS_OFFSET);

+    if (!s->disable_acs) {

+        pcie_acs_init(d, XIO3130_ACS_OFFSET);

+    }

     return;



 err:

@@ -113,6 +117,11 @@ static void xio3130_upstream_exitfn(PCIDevice *d)

     pci_bridge_exitfn(d);

 }



+static Property xio3130_upstream_props[] = {

+    DEFINE_PROP_BOOL("disable-acs", PCIESlot, disable_acs, false),

+    DEFINE_PROP_END_OF_LIST()

+};

+

 static const VMStateDescription vmstate_xio3130_upstream = {

     .name = "xio3130-express-upstream-port",

     .priority = MIG_PRI_PCI_BUS,

@@ -142,6 +151,7 @@ static void xio3130_upstream_class_init(ObjectClass
*klass, void *data)

     dc->desc = "TI X3130 Upstream Port of PCI Express Switch";

     dc->reset = xio3130_upstream_reset;

     dc->vmsd = &vmstate_xio3130_upstream;

+    device_class_set_props(dc, xio3130_upstream_props);

 }



 static const TypeInfo xio3130_upstream_info = {

-- 

2.24.3 (Apple Git-128)
Re: [PATCH v2] xio3130_upstream: Add ACS (Access Control Services) capability
Posted by Michael S. Tsirkin 1 year, 8 months ago 
On Tue, Aug 16, 2022 at 05:16:38PM +0800, Paul Schlacter wrote:
> v1 -> v2:
> - Allow ACS to be disabled.
> - Suggested by Michael S. Tsirkin, use disable-acs to set property.
> 
> v1:
> - Add ACS (Access Control Services) capability.

changelog generally after ---

> 
> If it is a pcie device, check that all devices on the path from


Hmm I don't see any checks on a path. what does this refer to?

> 
> the device to the root complex have ACS enabled, and then the
> 
> device will become an iommu_group.
> 
> it will have the effect of isolation
> 
> 
> 
> Signed-off-by: wangliang <wlfightup@gmail.com>
> 
> Signed-off-by: wangliang <wangliang40@baidu.com>
> 

why two signatures?

> 
> ---

> 
>  hw/pci-bridge/xio3130_upstream.c | 12 +++++++++++-
> 
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 

Patch has corrupted whitespace.

> 
> diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/
> xio3130_upstream.c
> 
> index 2df952222b..5433d06fb3 100644
> 
> --- a/hw/pci-bridge/xio3130_upstream.c
> 
> +++ b/hw/pci-bridge/xio3130_upstream.c
> 
> @@ -24,6 +24,7 @@
> 
>  #include "hw/pci/msi.h"
> 
>  #include "hw/pci/pcie.h"
> 
>  #include "hw/pci/pcie_port.h"
> 
> +#include "hw/qdev-properties.h"
> 
>  #include "migration/vmstate.h"
> 
>  #include "qemu/module.h"
> 
>  
> 
> @@ -59,6 +60,7 @@ static void xio3130_upstream_reset(DeviceState *qdev)
> 
>  static void xio3130_upstream_realize(PCIDevice *d, Error **errp)
> 
>  {
> 
>      PCIEPort *p = PCIE_PORT(d);
> 
> +    PCIESlot *s = PCIE_SLOT(d);
> 
>      int rc;
> 
>  
> 
>      pci_bridge_initfn(d, TYPE_PCIE_BUS);
> 
> @@ -94,7 +96,9 @@ static void xio3130_upstream_realize(PCIDevice *d, Error
> **errp)
> 
>          goto err;
> 
>      }
> 
>  
> 
> -    pcie_acs_init(d, XIO3130_ACS_OFFSET);
> 
> +    if (!s->disable_acs) {
> 
> +        pcie_acs_init(d, XIO3130_ACS_OFFSET);
> 
> +    }
> 
>      return;
> 
>  
> 
>  err:
> 
> @@ -113,6 +117,11 @@ static void xio3130_upstream_exitfn(PCIDevice *d)
> 
>      pci_bridge_exitfn(d);
> 
>  }
> 
>  
> 
> +static Property xio3130_upstream_props[] = {
> 
> +    DEFINE_PROP_BOOL("disable-acs", PCIESlot, disable_acs, false),
> 
> +    DEFINE_PROP_END_OF_LIST()
> 
> +};
> 
> +

I'd say prefix the property with "x-".


> 
>  static const VMStateDescription vmstate_xio3130_upstream = {
> 
>      .name = "xio3130-express-upstream-port",
> 
>      .priority = MIG_PRI_PCI_BUS,
> 
> @@ -142,6 +151,7 @@ static void xio3130_upstream_class_init(ObjectClass *klass,
> void *data)
> 
>      dc->desc = "TI X3130 Upstream Port of PCI Express Switch";
> 
>      dc->reset = xio3130_upstream_reset;
> 
>      dc->vmsd = &vmstate_xio3130_upstream;
> 
> +    device_class_set_props(dc, xio3130_upstream_props);
> 
>  }
> 

Seems to lack compat machinety for existing machine types.


>  
> 
>  static const TypeInfo xio3130_upstream_info = {
> 
> -- 
> 
> 2.24.3 (Apple Git-128)
>
Re: [PATCH v2] xio3130_upstream: Add ACS (Access Control Services) capability
Posted by Paul Schlacter 1 year, 8 months ago 
On Tue, Aug 16, 2022 at 6:11 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Aug 16, 2022 at 05:16:38PM +0800, Paul Schlacter wrote:
> > v1 -> v2:
> > - Allow ACS to be disabled.
> > - Suggested by Michael S. Tsirkin, use disable-acs to set property.
> >
> > v1:
> > - Add ACS (Access Control Services) capability.
>
> changelog generally after ---
>
> >
> > If it is a pcie device, check that all devices on the path from
>
>
> Hmm I don't see any checks on a path. what does this refer to?

pci_acs_path_enabled, this function in the Linux kernel,
it means that if the device is a PCIe device,
check the path from the device to the root complex. If ACS is all enabled,
the device will become an iommu_group.
acs determine whether it is a separate iommu_group.
>
> >
> > the device to the root complex have ACS enabled, and then the
> >
> > device will become an iommu_group.
> >
> > it will have the effect of isolation
> >
> >
> >
> > Signed-off-by: wangliang <wlfightup@gmail.com>
> >
> > Signed-off-by: wangliang <wangliang40@baidu.com>
> >
>
> why two signatures?
>
> >
> > ---
>
> >
> >  hw/pci-bridge/xio3130_upstream.c | 12 +++++++++++-
> >
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
>
> Patch has corrupted whitespace.
>
> >
> > diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/
> > xio3130_upstream.c
> >
> > index 2df952222b..5433d06fb3 100644
> >
> > --- a/hw/pci-bridge/xio3130_upstream.c
> >
> > +++ b/hw/pci-bridge/xio3130_upstream.c
> >
> > @@ -24,6 +24,7 @@
> >
> >  #include "hw/pci/msi.h"
> >
> >  #include "hw/pci/pcie.h"
> >
> >  #include "hw/pci/pcie_port.h"
> >
> > +#include "hw/qdev-properties.h"
> >
> >  #include "migration/vmstate.h"
> >
> >  #include "qemu/module.h"
> >
> >
> >
> > @@ -59,6 +60,7 @@ static void xio3130_upstream_reset(DeviceState *qdev)
> >
> >  static void xio3130_upstream_realize(PCIDevice *d, Error **errp)
> >
> >  {
> >
> >      PCIEPort *p = PCIE_PORT(d);
> >
> > +    PCIESlot *s = PCIE_SLOT(d);
> >
> >      int rc;
> >
> >
> >
> >      pci_bridge_initfn(d, TYPE_PCIE_BUS);
> >
> > @@ -94,7 +96,9 @@ static void xio3130_upstream_realize(PCIDevice *d, Error
> > **errp)
> >
> >          goto err;
> >
> >      }
> >
> >
> >
> > -    pcie_acs_init(d, XIO3130_ACS_OFFSET);
> >
> > +    if (!s->disable_acs) {
> >
> > +        pcie_acs_init(d, XIO3130_ACS_OFFSET);
> >
> > +    }
> >
> >      return;
> >
> >
> >
> >  err:
> >
> > @@ -113,6 +117,11 @@ static void xio3130_upstream_exitfn(PCIDevice *d)
> >
> >      pci_bridge_exitfn(d);
> >
> >  }
> >
> >
> >
> > +static Property xio3130_upstream_props[] = {
> >
> > +    DEFINE_PROP_BOOL("disable-acs", PCIESlot, disable_acs, false),
> >
> > +    DEFINE_PROP_END_OF_LIST()
> >
> > +};
> >
> > +
>
> I'd say prefix the property with "x-".

Do you want me to change it to this?
DEFINE_PROP_BOOL("x-disable-acs", PCIESlot, disable_acs, false),

disable_acs is a field in PCIESlot, which is also used by other code.
It may not be good to modify this field
>
>
> >
> >  static const VMStateDescription vmstate_xio3130_upstream = {
> >
> >      .name = "xio3130-express-upstream-port",
> >
> >      .priority = MIG_PRI_PCI_BUS,
> >
> > @@ -142,6 +151,7 @@ static void xio3130_upstream_class_init(ObjectClass *klass,
> > void *data)
> >
> >      dc->desc = "TI X3130 Upstream Port of PCI Express Switch";
> >
> >      dc->reset = xio3130_upstream_reset;
> >
> >      dc->vmsd = &vmstate_xio3130_upstream;
> >
> > +    device_class_set_props(dc, xio3130_upstream_props);
> >
> >  }
> >
>
> Seems to lack compat machinety for existing machine types.

DEFINE_PROP_BOOL("x-disable-acs", PCIESlot, disable_acs, true),
If I change the default value to true, can I not add compat machine
>
>
> >
> >
> >  static const TypeInfo xio3130_upstream_info = {
> >
> > --
> >
> > 2.24.3 (Apple Git-128)
> >
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.