[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <eab568ea-f39e-5399-6af6-0518832dfc91@nvidia.com>
Date: Tue, 26 Jul 2022 11:37:50 +0300
From: Yishai Hadas <yishaih@...dia.com>
To: "Tian, Kevin" <kevin.tian@...el.com>,
"alex.williamson@...hat.com" <alex.williamson@...hat.com>,
"jgg@...dia.com" <jgg@...dia.com>
CC: "saeedm@...dia.com" <saeedm@...dia.com>,
"kvm@...r.kernel.org" <kvm@...r.kernel.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"kuba@...nel.org" <kuba@...nel.org>,
"Martins, Joao" <joao.m.martins@...cle.com>,
"leonro@...dia.com" <leonro@...dia.com>,
"maorg@...dia.com" <maorg@...dia.com>,
"cohuck@...hat.com" <cohuck@...hat.com>,
Yishai Hadas <yishaih@...dia.com>
Subject: Re: [PATCH V2 vfio 03/11] vfio: Introduce DMA logging uAPIs
On 25/07/2022 10:30, Tian, Kevin wrote:
> <please use plain-text next time>
>
>> From: Yishai Hadas <yishaih@...dia.com>
>> Sent: Thursday, July 21, 2022 7:06 PM
>>>> +/*
>>>> + * Upon VFIO_DEVICE_FEATURE_SET start device DMA logging.
>>> both 'start'/'stop' are via VFIO_DEVICE_FEATURE_SET
>> Right, we have a note for that near VFIO_DEVICE_FEATURE_DMA_LOGGING_STOP.
>> Here it refers to the start option.
> let's make it accurate here.
OK
>
>>>> + * page_size is an input that hints what tracking granularity the device
>>>> + * should try to achieve. If the device cannot do the hinted page size then it
>>>> + * should pick the next closest page size it supports. On output the device
>>> next closest 'smaller' page size?
>> Not only, it depends on the device capabilities/support and should be a driver choice.
> 'should pick next closest" is a guideline to the driver. If user requests
> 8KB while the device supports 4KB and 16KB, which one is closest?
>
> It's probably safer to just say that it's a driver choice when the hinted page
> size cannot be set?
Yes, may rephrase in V3 accordingly.
>
>>>> +struct vfio_device_feature_dma_logging_control {
>>>> + __aligned_u64 page_size;
>>>> + __u32 num_ranges;
>>>> + __u32 __reserved;
>>>> + __aligned_u64 ranges;
>>>> +};
>>> should we move the definition of LOG_MAX_RANGES to be here
>>> so the user can know the max limits of tracked ranges?
>> This was raised as an option as part of this mail thread.
>> However, for now it seems redundant as we may not expect user space to hit this limit and it mainly comes to protect kernel from memory exploding by a malicious user.
> No matter how realistic an user might hit an limitation, it doesn't
> sound good to not expose it if existing.
As Jason replied at some point here, we need to see a clear use case for
more than a few 10's of ranges before we complicate things.
For now we don't see one. If one does crop up someday it is easy to add
a new query, or some other behavior.
Alex,
Can you please comment here so that we can converge and be ready for V3 ?
>>>> +
>>>> +struct vfio_device_feature_dma_logging_range {
>>>> + __aligned_u64 iova;
>>>> + __aligned_u64 length;
>>>> +};
>>>> +
>>>> +#define VFIO_DEVICE_FEATURE_DMA_LOGGING_START 3
>>> Can the user update the range list by doing another START?
>> No, single start to ask the device what to track and a matching single stop should follow at the end.
> let's document it then.
OK
>
> Thanks
> Kevin
>
Thanks,
Yishai
Powered by blists - more mailing lists