From: Yishai Hadas <yishaih@nvidia.com>
To: Alex Williamson <alex.williamson@redhat.com>
Cc: Jason Gunthorpe <jgg@nvidia.com>, <bhelgaas@google.com>,
<saeedm@nvidia.com>, <linux-pci@vger.kernel.org>,
<kvm@vger.kernel.org>, <netdev@vger.kernel.org>,
<kuba@kernel.org>, <leonro@nvidia.com>, <kwankhede@nvidia.com>,
<mgurtovoy@nvidia.com>, <maorg@nvidia.com>
Subject: Re: [PATCH V1 mlx5-next 11/13] vfio/mlx5: Implement vfio_pci driver for mlx5 devices
Date: Mon, 18 Oct 2021 16:46:35 +0300 [thread overview]
Message-ID: <17cd04da-7fc5-0c62-20bc-bb62f4a5d621@nvidia.com> (raw)
In-Reply-To: <20211018074232.1008f54c.alex.williamson@redhat.com>
On 10/18/2021 4:42 PM, Alex Williamson wrote:
> On Mon, 18 Oct 2021 16:26:16 +0300
> Yishai Hadas <yishaih@nvidia.com> wrote:
>
>> On 10/18/2021 2:51 PM, Jason Gunthorpe wrote:
>>> On Sun, Oct 17, 2021 at 05:03:28PM +0300, Yishai Hadas wrote:
>>>> On 10/15/2021 11:59 PM, Alex Williamson wrote:
>>>>> On Fri, 15 Oct 2021 17:16:54 -0300
>>>>> Jason Gunthorpe <jgg@nvidia.com> wrote:
>>>>>
>>>>>> On Fri, Oct 15, 2021 at 02:12:01PM -0600, Alex Williamson wrote:
>>>>>>> On Fri, 15 Oct 2021 16:59:37 -0300
>>>>>>> Jason Gunthorpe <jgg@nvidia.com> wrote:
>>>>>>>> On Fri, Oct 15, 2021 at 01:48:20PM -0600, Alex Williamson wrote:
>>>>>>>>>> +static int mlx5vf_pci_set_device_state(struct mlx5vf_pci_core_device *mvdev,
>>>>>>>>>> + u32 state)
>>>>>>>>>> +{
>>>>>>>>>> + struct mlx5vf_pci_migration_info *vmig = &mvdev->vmig;
>>>>>>>>>> + u32 old_state = vmig->vfio_dev_state;
>>>>>>>>>> + int ret = 0;
>>>>>>>>>> +
>>>>>>>>>> + if (vfio_is_state_invalid(state) || vfio_is_state_invalid(old_state))
>>>>>>>>>> + return -EINVAL;
>>>>>>>>> if (!VFIO_DEVICE_STATE_VALID(old_state) || !VFIO_DEVICE_STATE_VALID(state))
>>>>>>>> AFAICT this macro doesn't do what is needed, eg
>>>>>>>>
>>>>>>>> VFIO_DEVICE_STATE_VALID(0xF000) == true
>>>>>>>>
>>>>>>>> What Yishai implemented is at least functionally correct - states this
>>>>>>>> driver does not support are rejected.
>>>>>>> if (!VFIO_DEVICE_STATE_VALID(old_state) || !VFIO_DEVICE_STATE_VALID(state)) || (state & ~VFIO_DEVICE_STATE_MASK))
>>>>>>>
>>>>>>> old_state is controlled by the driver and can never have random bits
>>>>>>> set, user state should be sanitized to prevent setting undefined bits.
>>>>>> In that instance let's just write
>>>>>>
>>>>>> old_state != VFIO_DEVICE_STATE_ERROR
>>>>>>
>>>>>> ?
>>>>> Not quite, the user can't set either of the other invalid states
>>>>> either.
>>>> OK so let's go with below as you suggested.
>>>> if (!VFIO_DEVICE_STATE_VALID(old_state) ||
>>>> !VFIO_DEVICE_STATE_VALID(state) ||
>>>> (state & ~VFIO_DEVICE_STATE_MASK))
>>>>
>>> This is my preference:
>>>
>>> if (vmig->vfio_dev_state != VFIO_DEVICE_STATE_ERROR ||
>>> !vfio_device_state_valid(state) ||
>>> (state & !MLX5VF_SUPPORTED_DEVICE_STATES))
>>>
>> OK, let's go with this approach which enforces what the driver supports
>> as well.
>>
>> We may have the below post making it accurate and complete.
>>
>> enum {
>> MLX5VF_SUPPORTED_DEVICE_STATES = VFIO_DEVICE_STATE_RUNNING |
>> VFIO_DEVICE_STATE_SAVING |
>> VFIO_DEVICE_STATE_RESUMING,
>> };
>>
>> if (old_state == VFIO_DEVICE_STATE_ERROR ||
>> !vfio_device_state_valid(state) ||
>> (state & ~MLX5VF_SUPPORTED_DEVICE_STATES))
>> return -EINVAL;
>>
>>>> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
>>>> index b53a9557884a..37376dadca5a 100644
>>>> +++ b/include/linux/vfio.h
>>>> @@ -15,6 +15,8 @@
>>>> #include <linux/poll.h>
>>>> #include <uapi/linux/vfio.h>
>>>>
>>>> +static const int VFIO_DEVICE_STATE_ERROR = VFIO_DEVICE_STATE_SAVING |
>>>> + VFIO_DEVICE_STATE_RESUMING;
>>> Do not put static variables in header files
>>>
>>> Jason
>> OK, we can come with an enum instead.
>>
>> enum {
>>
>> VFIO_DEVICE_STATE_ERROR = VFIO_DEVICE_STATE_SAVING | VFIO_DEVICE_STATE_RESUMING,
>>
>> };
>>
>> Alex,
>>
>> Do you prefer to put it under include/uapi/vfio.h or that it can go
>> under inlcude/linux/vfio.h for internal drivers usage ?
> I don't understand why this wouldn't just be a continuation of the
> #defines in the uapi header. Thanks,
>
> Alex
>
Sure, let's go with this.
Thanks,
Yishai
next prev parent reply other threads:[~2021-10-18 13:48 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-13 9:46 [PATCH V1 mlx5-next 00/13] Add mlx5 live migration driver Yishai Hadas
2021-10-13 9:46 ` [PATCH V1 mlx5-next 01/13] PCI/IOV: Provide internal VF index Yishai Hadas
2021-10-13 18:14 ` Bjorn Helgaas
2021-10-14 9:08 ` Yishai Hadas
2021-10-13 9:46 ` [PATCH V1 mlx5-next 02/13] net/mlx5: Reuse exported virtfn index function call Yishai Hadas
2021-10-13 9:46 ` [PATCH V1 mlx5-next 03/13] net/mlx5: Disable SRIOV before PF removal Yishai Hadas
2021-10-13 9:46 ` [PATCH V1 mlx5-next 04/13] PCI/IOV: Allow SRIOV VF drivers to reach the drvdata of a PF Yishai Hadas
2021-10-13 18:27 ` Bjorn Helgaas
2021-10-14 22:11 ` Alex Williamson
2021-10-17 13:43 ` Yishai Hadas
2021-10-13 9:46 ` [PATCH V1 mlx5-next 05/13] net/mlx5: Expose APIs to get/put the mlx5 core device Yishai Hadas
2021-10-13 9:47 ` [PATCH V1 mlx5-next 06/13] vdpa/mlx5: Use mlx5_vf_get_core_dev() to get PF device Yishai Hadas
2021-10-13 9:47 ` [PATCH V1 mlx5-next 07/13] vfio: Add 'invalid' state definitions Yishai Hadas
2021-10-15 16:38 ` Alex Williamson
2021-10-17 14:07 ` Yishai Hadas
2021-10-13 9:47 ` [PATCH V1 mlx5-next 08/13] vfio/pci_core: Make the region->release() function optional Yishai Hadas
2021-10-13 9:47 ` [PATCH V1 mlx5-next 09/13] net/mlx5: Introduce migration bits and structures Yishai Hadas
2021-10-13 9:47 ` [PATCH V1 mlx5-next 10/13] vfio/mlx5: Expose migration commands over mlx5 device Yishai Hadas
2021-10-13 9:47 ` [PATCH V1 mlx5-next 11/13] vfio/mlx5: Implement vfio_pci driver for mlx5 devices Yishai Hadas
2021-10-15 19:48 ` Alex Williamson
2021-10-15 19:59 ` Jason Gunthorpe
2021-10-15 20:12 ` Alex Williamson
2021-10-15 20:16 ` Jason Gunthorpe
2021-10-15 20:59 ` Alex Williamson
2021-10-17 14:03 ` Yishai Hadas
2021-10-18 11:51 ` Jason Gunthorpe
2021-10-18 13:26 ` Yishai Hadas
2021-10-18 13:42 ` Alex Williamson
2021-10-18 13:46 ` Yishai Hadas [this message]
2021-10-19 9:59 ` Shameerali Kolothum Thodi
2021-10-19 10:30 ` Yishai Hadas
2021-10-19 11:26 ` Shameerali Kolothum Thodi
2021-10-19 11:24 ` Jason Gunthorpe
2021-10-13 9:47 ` [PATCH V1 mlx5-next 12/13] vfio/pci: Add infrastructure to let vfio_pci_core drivers trap device RESET Yishai Hadas
2021-10-15 19:52 ` Alex Williamson
2021-10-15 20:03 ` Jason Gunthorpe
2021-10-15 21:12 ` Alex Williamson
2021-10-17 14:29 ` Yishai Hadas
2021-10-18 12:02 ` Jason Gunthorpe
2021-10-18 13:41 ` Yishai Hadas
2021-10-13 9:47 ` [PATCH V1 mlx5-next 13/13] vfio/mlx5: Trap device RESET and update state accordingly Yishai Hadas
2021-10-13 18:06 ` Jason Gunthorpe
2021-10-14 9:18 ` Yishai Hadas
2021-10-15 19:54 ` Alex Williamson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=17cd04da-7fc5-0c62-20bc-bb62f4a5d621@nvidia.com \
--to=yishaih@nvidia.com \
--cc=alex.williamson@redhat.com \
--cc=bhelgaas@google.com \
--cc=jgg@nvidia.com \
--cc=kuba@kernel.org \
--cc=kvm@vger.kernel.org \
--cc=kwankhede@nvidia.com \
--cc=leonro@nvidia.com \
--cc=linux-pci@vger.kernel.org \
--cc=maorg@nvidia.com \
--cc=mgurtovoy@nvidia.com \
--cc=netdev@vger.kernel.org \
--cc=saeedm@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).