On Wed, Apr 26, 2023 at 8:32 AM Jason Wang <jasow...@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 <vik...@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 */
>
