| Message ID | 20230424112147.17083-2-viktor@daynix.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,1/4] pci: add handling of Enable bit in ATS Control Register | expand |
在 2023/4/24 19:21, Viktor Prutyanov 写道: > According to PCIe Address Translation Services specification 5.1.3., > ATS Control Register has Enable bit to enable/disable ATS. > Add a new field for a trigger function which is called at the Enable > bit change, so that PCIe devices can handle ATS enable/disable. > > Signed-off-by: Viktor Prutyanov <viktor@daynix.com> > --- > hw/pci/pci.c | 1 + > hw/pci/pcie.c | 21 +++++++++++++++++++++ > include/hw/pci/pci_device.h | 3 +++ > include/hw/pci/pcie.h | 4 ++++ > 4 files changed, 29 insertions(+) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index 208c16f450..79a47d2589 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -1550,6 +1550,7 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val_in, int > msi_write_config(d, addr, val_in, l); > msix_write_config(d, addr, val_in, l); > pcie_sriov_config_write(d, addr, val_in, l); > + pcie_ats_config_write(d, addr, val_in, l); > } > > /***********************************************************/ > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > index 924fdabd15..e0217161e5 100644 > --- a/hw/pci/pcie.c > +++ b/hw/pci/pcie.c > @@ -1057,6 +1057,27 @@ void pcie_ats_init(PCIDevice *dev, uint16_t offset, bool aligned) > pci_set_word(dev->wmask + dev->exp.ats_cap + PCI_ATS_CTRL, 0x800f); > } > > +void pcie_ats_config_write(PCIDevice *dev, uint32_t address, uint32_t val, > + int len) > +{ > + uint32_t off; > + uint16_t ats_cap = dev->exp.ats_cap; > + > + if (!ats_cap || address < ats_cap) { > + return; > + } > + off = address - ats_cap; > + if (off >= PCI_EXT_CAP_ATS_SIZEOF) { > + return; > + } > + > + if (range_covers_byte(off, len, PCI_ATS_CTRL + 1)) { Do we really need +1 here? The rest looks good. Thanks > + if (dev->ats_ctrl_trigger) { > + dev->ats_ctrl_trigger(dev, !!(val & PCI_ATS_CTRL_ENABLE)); > + } > + } > +} > + > /* ACS (Access Control Services) */ > void pcie_acs_init(PCIDevice *dev, uint16_t offset) > { > diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h > index d3dd0f64b2..2bb1d68f3b 100644 > --- a/include/hw/pci/pci_device.h > +++ b/include/hw/pci/pci_device.h > @@ -160,6 +160,9 @@ struct PCIDevice { > /* ID of standby device in net_failover pair */ > char *failover_pair_id; > uint32_t acpi_index; > + > + /* PCI ATS enable/disable trigger */ > + void (*ats_ctrl_trigger)(PCIDevice *dev, bool enable); > }; > > static inline int pci_intx(PCIDevice *pci_dev) > diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h > index 798a262a0a..5f2dbd87cf 100644 > --- a/include/hw/pci/pcie.h > +++ b/include/hw/pci/pcie.h > @@ -154,4 +154,8 @@ void pcie_cap_slot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > Error **errp); > void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev, > DeviceState *dev, Error **errp); > + > +void pcie_ats_config_write(PCIDevice *dev, uint32_t address, uint32_t val, > + int len); > + > #endif /* QEMU_PCIE_H */
在 2023/4/26 13:31, Jason Wang 写道: > > 在 2023/4/24 19:21, Viktor Prutyanov 写道: >> According to PCIe Address Translation Services specification 5.1.3., >> ATS Control Register has Enable bit to enable/disable ATS. >> Add a new field for a trigger function which is called at the Enable >> bit change, so that PCIe devices can handle ATS enable/disable. >> >> Signed-off-by: Viktor Prutyanov <viktor@daynix.com> >> --- >> hw/pci/pci.c | 1 + >> hw/pci/pcie.c | 21 +++++++++++++++++++++ >> include/hw/pci/pci_device.h | 3 +++ >> include/hw/pci/pcie.h | 4 ++++ >> 4 files changed, 29 insertions(+) >> >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >> index 208c16f450..79a47d2589 100644 >> --- a/hw/pci/pci.c >> +++ b/hw/pci/pci.c >> @@ -1550,6 +1550,7 @@ void pci_default_write_config(PCIDevice *d, >> uint32_t addr, uint32_t val_in, int >> msi_write_config(d, addr, val_in, l); >> msix_write_config(d, addr, val_in, l); >> pcie_sriov_config_write(d, addr, val_in, l); >> + pcie_ats_config_write(d, addr, val_in, l); >> } >> /***********************************************************/ >> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c >> index 924fdabd15..e0217161e5 100644 >> --- a/hw/pci/pcie.c >> +++ b/hw/pci/pcie.c >> @@ -1057,6 +1057,27 @@ void pcie_ats_init(PCIDevice *dev, uint16_t >> offset, bool aligned) >> pci_set_word(dev->wmask + dev->exp.ats_cap + PCI_ATS_CTRL, >> 0x800f); >> } >> +void pcie_ats_config_write(PCIDevice *dev, uint32_t address, >> uint32_t val, >> + int len) >> +{ >> + uint32_t off; >> + uint16_t ats_cap = dev->exp.ats_cap; >> + >> + if (!ats_cap || address < ats_cap) { >> + return; >> + } >> + off = address - ats_cap; >> + if (off >= PCI_EXT_CAP_ATS_SIZEOF) { >> + return; >> + } >> + >> + if (range_covers_byte(off, len, PCI_ATS_CTRL + 1)) { > > > Do we really need +1 here? > > The rest looks good. > > Thanks > > >> + if (dev->ats_ctrl_trigger) { >> + dev->ats_ctrl_trigger(dev, !!(val & PCI_ATS_CTRL_ENABLE)); >> + } >> + } Speak too fast. We have virtio_write_config(), can we hook there? (We've already dealt with FLR and bus master there) Thanks >> +} >> + >> /* ACS (Access Control Services) */ >> void pcie_acs_init(PCIDevice *dev, uint16_t offset) >> { >> diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h >> index d3dd0f64b2..2bb1d68f3b 100644 >> --- a/include/hw/pci/pci_device.h >> +++ b/include/hw/pci/pci_device.h >> @@ -160,6 +160,9 @@ struct PCIDevice { >> /* ID of standby device in net_failover pair */ >> char *failover_pair_id; >> uint32_t acpi_index; >> + >> + /* PCI ATS enable/disable trigger */ >> + void (*ats_ctrl_trigger)(PCIDevice *dev, bool enable); >> }; >> static inline int pci_intx(PCIDevice *pci_dev) >> diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h >> index 798a262a0a..5f2dbd87cf 100644 >> --- a/include/hw/pci/pcie.h >> +++ b/include/hw/pci/pcie.h >> @@ -154,4 +154,8 @@ void pcie_cap_slot_unplug_cb(HotplugHandler >> *hotplug_dev, DeviceState *dev, >> Error **errp); >> void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev, >> DeviceState *dev, Error **errp); >> + >> +void pcie_ats_config_write(PCIDevice *dev, uint32_t address, >> uint32_t val, >> + int len); >> + >> #endif /* QEMU_PCIE_H */
On Wed, Apr 26, 2023 at 8:32 AM Jason Wang <jasowang@redhat.com> wrote: > > > 在 2023/4/24 19:21, Viktor Prutyanov 写道: > > According to PCIe Address Translation Services specification 5.1.3., > > ATS Control Register has Enable bit to enable/disable ATS. > > Add a new field for a trigger function which is called at the Enable > > bit change, so that PCIe devices can handle ATS enable/disable. > > > > Signed-off-by: Viktor Prutyanov <viktor@daynix.com> > > --- > > hw/pci/pci.c | 1 + > > hw/pci/pcie.c | 21 +++++++++++++++++++++ > > include/hw/pci/pci_device.h | 3 +++ > > include/hw/pci/pcie.h | 4 ++++ > > 4 files changed, 29 insertions(+) > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > index 208c16f450..79a47d2589 100644 > > --- a/hw/pci/pci.c > > +++ b/hw/pci/pci.c > > @@ -1550,6 +1550,7 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val_in, int > > msi_write_config(d, addr, val_in, l); > > msix_write_config(d, addr, val_in, l); > > pcie_sriov_config_write(d, addr, val_in, l); > > + pcie_ats_config_write(d, addr, val_in, l); > > } > > > > /***********************************************************/ > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > > index 924fdabd15..e0217161e5 100644 > > --- a/hw/pci/pcie.c > > +++ b/hw/pci/pcie.c > > @@ -1057,6 +1057,27 @@ void pcie_ats_init(PCIDevice *dev, uint16_t offset, bool aligned) > > pci_set_word(dev->wmask + dev->exp.ats_cap + PCI_ATS_CTRL, 0x800f); > > } > > > > +void pcie_ats_config_write(PCIDevice *dev, uint32_t address, uint32_t val, > > + int len) > > +{ > > + uint32_t off; > > + uint16_t ats_cap = dev->exp.ats_cap; > > + > > + if (!ats_cap || address < ats_cap) { > > + return; > > + } > > + off = address - ats_cap; > > + if (off >= PCI_EXT_CAP_ATS_SIZEOF) { > > + return; > > + } > > + > > + if (range_covers_byte(off, len, PCI_ATS_CTRL + 1)) { > > > Do we really need +1 here? The Enable bit is the 15th in the ATS Control Register, so it is in a byte next to PCI_ATS_CTRL. Although I'm not sure that this is the best way to test this bit. All other comments I tried to take into account in the v2. Thanks, Viktor Prutyanov > > The rest looks good. > > Thanks > > > > + if (dev->ats_ctrl_trigger) { > > + dev->ats_ctrl_trigger(dev, !!(val & PCI_ATS_CTRL_ENABLE)); > > + } > > + } > > +} > > + > > /* ACS (Access Control Services) */ > > void pcie_acs_init(PCIDevice *dev, uint16_t offset) > > { > > diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h > > index d3dd0f64b2..2bb1d68f3b 100644 > > --- a/include/hw/pci/pci_device.h > > +++ b/include/hw/pci/pci_device.h > > @@ -160,6 +160,9 @@ struct PCIDevice { > > /* ID of standby device in net_failover pair */ > > char *failover_pair_id; > > uint32_t acpi_index; > > + > > + /* PCI ATS enable/disable trigger */ > > + void (*ats_ctrl_trigger)(PCIDevice *dev, bool enable); > > }; > > > > static inline int pci_intx(PCIDevice *pci_dev) > > diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h > > index 798a262a0a..5f2dbd87cf 100644 > > --- a/include/hw/pci/pcie.h > > +++ b/include/hw/pci/pcie.h > > @@ -154,4 +154,8 @@ void pcie_cap_slot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > > Error **errp); > > void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev, > > DeviceState *dev, Error **errp); > > + > > +void pcie_ats_config_write(PCIDevice *dev, uint32_t address, uint32_t val, > > + int len); > > + > > #endif /* QEMU_PCIE_H */ >
diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 208c16f450..79a47d2589 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -1550,6 +1550,7 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val_in, int msi_write_config(d, addr, val_in, l); msix_write_config(d, addr, val_in, l); pcie_sriov_config_write(d, addr, val_in, l); + pcie_ats_config_write(d, addr, val_in, l); } /***********************************************************/ diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c index 924fdabd15..e0217161e5 100644 --- a/hw/pci/pcie.c +++ b/hw/pci/pcie.c @@ -1057,6 +1057,27 @@ void pcie_ats_init(PCIDevice *dev, uint16_t offset, bool aligned) pci_set_word(dev->wmask + dev->exp.ats_cap + PCI_ATS_CTRL, 0x800f); } +void pcie_ats_config_write(PCIDevice *dev, uint32_t address, uint32_t val, + int len) +{ + uint32_t off; + uint16_t ats_cap = dev->exp.ats_cap; + + if (!ats_cap || address < ats_cap) { + return; + } + off = address - ats_cap; + if (off >= PCI_EXT_CAP_ATS_SIZEOF) { + return; + } + + if (range_covers_byte(off, len, PCI_ATS_CTRL + 1)) { + if (dev->ats_ctrl_trigger) { + dev->ats_ctrl_trigger(dev, !!(val & PCI_ATS_CTRL_ENABLE)); + } + } +} + /* ACS (Access Control Services) */ void pcie_acs_init(PCIDevice *dev, uint16_t offset) { diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h index d3dd0f64b2..2bb1d68f3b 100644 --- a/include/hw/pci/pci_device.h +++ b/include/hw/pci/pci_device.h @@ -160,6 +160,9 @@ struct PCIDevice { /* ID of standby device in net_failover pair */ char *failover_pair_id; uint32_t acpi_index; + + /* PCI ATS enable/disable trigger */ + void (*ats_ctrl_trigger)(PCIDevice *dev, bool enable); }; static inline int pci_intx(PCIDevice *pci_dev) diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h index 798a262a0a..5f2dbd87cf 100644 --- a/include/hw/pci/pcie.h +++ b/include/hw/pci/pcie.h @@ -154,4 +154,8 @@ void pcie_cap_slot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp); void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp); + +void pcie_ats_config_write(PCIDevice *dev, uint32_t address, uint32_t val, + int len); + #endif /* QEMU_PCIE_H */
According to PCIe Address Translation Services specification 5.1.3., ATS Control Register has Enable bit to enable/disable ATS. Add a new field for a trigger function which is called at the Enable bit change, so that PCIe devices can handle ATS enable/disable. Signed-off-by: Viktor Prutyanov <viktor@daynix.com> --- hw/pci/pci.c | 1 + hw/pci/pcie.c | 21 +++++++++++++++++++++ include/hw/pci/pci_device.h | 3 +++ include/hw/pci/pcie.h | 4 ++++ 4 files changed, 29 insertions(+)