Re: [PATCH v2 2/6] hwtracing: Add trace function support for HiSilicon PCIe Tune and Trace device

[Yicong Yang]

On 2021/11/16 18:56, Robin Murphy wrote:
> On 2021-11-16 09:06, Yicong Yang via iommu wrote:
> [...]
>> +/*
>> + * Get RMR address if provided by the firmware.
>> + * Return 0 if the IOMMU doesn't present or the policy of the
>> + * IOMMU domain is passthrough or we get a usable RMR region.
>> + * Otherwise a negative value is returned.
>> + */
>> +static int hisi_ptt_get_rmr(struct hisi_ptt *hisi_ptt)
>> +{
>> +    struct pci_dev *pdev = hisi_ptt->pdev;
>> +    struct iommu_domain *iommu_domain;
>> +    struct iommu_resv_region *region;
>> +    LIST_HEAD(list);
>> +
>> +    /*
>> +     * Use direct DMA if IOMMU does not present or the policy of the
>> +     * IOMMU domain is passthrough.
>> +     */
>> +    iommu_domain = iommu_get_domain_for_dev(&pdev->dev);
>> +    if (!iommu_domain || iommu_domain->type == IOMMU_DOMAIN_IDENTITY)
>> +        return 0;
>> +
>> +    iommu_get_resv_regions(&pdev->dev, &list);
>> +    list_for_each_entry(region, &list, list)
>> +        if (region->type == IOMMU_RESV_DIRECT &&
>> +            region->length >= HISI_PTT_TRACE_BUFFER_SIZE) {
>> +            hisi_ptt->trace_ctrl.has_rmr = true;
>> +            hisi_ptt->trace_ctrl.rmr_addr = region->start;
>> +            hisi_ptt->trace_ctrl.rmr_length = region->length;
>> +            break;
>> +        }
>> +
>> +    iommu_put_resv_regions(&pdev->dev, &list);
>> +    return hisi_ptt->trace_ctrl.has_rmr ? 0 : -ENOMEM;
>> +}
> 
> No.
> 
> The whole point of RMRs is for devices that are already configured to access the given address range in a manner beyond the kernel's control. If you can do this, it proves that you should not have an RMR in the first place.
> 
> The notion of a kernel driver explicitly configuring its device to DMA into any random RMR that looks big enough is so egregiously wrong that I'm almost lost for words...
> 

our bios will reserve such a region and reported it through iort. the device will write to the region and in the driver we need to access the region
to get the traced data. the region is reserved exclusively and will not be accessed by kernel or other devices.

is it ok to let bios configure the address to the device and from CPU side we just read it?

Thanks,
Yicong