Re: [RFC PATCH 1/4] pci: add handling of Enable bit in ATS Control Register

[Jason Wang <jasowang@redhat.com>]

在 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 */