From: "Zhu, Lingshan" <lingshan.zhu@intel.com>
To: Jason Wang <jasowang@redhat.com>,
Zhu Lingshan <lingshan.zhu@linux.intel.com>,
mst@redhat.com, lulu@redhat.com, leonro@nvidia.com
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
kvm@vger.kernel.org, virtualization@lists.linux-foundation.org
Subject: Re: [PATCH V3 6/6] vDPA/ifcvf: verify mandatory feature bits for vDPA
Date: Fri, 12 Mar 2021 14:40:30 +0800 [thread overview]
Message-ID: <2a6e31d3-ea31-9b64-0749-1f149b656623@intel.com> (raw)
In-Reply-To: <67be60b6-bf30-de85-ed42-d9fad974f42b@redhat.com>
On 3/12/2021 1:52 PM, Jason Wang wrote:
>
> On 2021/3/11 3:19 下午, Zhu, Lingshan wrote:
>>
>>
>> On 3/11/2021 2:20 PM, Jason Wang wrote:
>>>
>>> On 2021/3/11 12:16 下午, Zhu Lingshan wrote:
>>>>
>>>>
>>>> On 3/11/2021 11:20 AM, Jason Wang wrote:
>>>>>
>>>>> On 2021/3/10 5:00 下午, Zhu Lingshan wrote:
>>>>>> vDPA requres VIRTIO_F_ACCESS_PLATFORM as a must, this commit
>>>>>> examines this when set features.
>>>>>>
>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>>> ---
>>>>>> drivers/vdpa/ifcvf/ifcvf_base.c | 8 ++++++++
>>>>>> drivers/vdpa/ifcvf/ifcvf_base.h | 1 +
>>>>>> drivers/vdpa/ifcvf/ifcvf_main.c | 5 +++++
>>>>>> 3 files changed, 14 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c
>>>>>> b/drivers/vdpa/ifcvf/ifcvf_base.c
>>>>>> index ea6a78791c9b..58f47fdce385 100644
>>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
>>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
>>>>>> @@ -224,6 +224,14 @@ u64 ifcvf_get_features(struct ifcvf_hw *hw)
>>>>>> return hw->hw_features;
>>>>>> }
>>>>>> +int ifcvf_verify_min_features(struct ifcvf_hw *hw)
>>>>>> +{
>>>>>> + if (!(hw->hw_features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM)))
>>>>>> + return -EINVAL;
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> void ifcvf_read_net_config(struct ifcvf_hw *hw, u64 offset,
>>>>>> void *dst, int length)
>>>>>> {
>>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>>> index dbb8c10aa3b1..91c5735d4dc9 100644
>>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>>> @@ -123,6 +123,7 @@ void io_write64_twopart(u64 val, u32 *lo, u32
>>>>>> *hi);
>>>>>> void ifcvf_reset(struct ifcvf_hw *hw);
>>>>>> u64 ifcvf_get_features(struct ifcvf_hw *hw);
>>>>>> u64 ifcvf_get_hw_features(struct ifcvf_hw *hw);
>>>>>> +int ifcvf_verify_min_features(struct ifcvf_hw *hw);
>>>>>> u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid);
>>>>>> int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num);
>>>>>> struct ifcvf_adapter *vf_to_adapter(struct ifcvf_hw *hw);
>>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>>> index 25fb9dfe23f0..f624f202447d 100644
>>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>>> @@ -179,6 +179,11 @@ static u64 ifcvf_vdpa_get_features(struct
>>>>>> vdpa_device *vdpa_dev)
>>>>>> static int ifcvf_vdpa_set_features(struct vdpa_device
>>>>>> *vdpa_dev, u64 features)
>>>>>> {
>>>>>> struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>>>>> + int ret;
>>>>>> +
>>>>>> + ret = ifcvf_verify_min_features(vf);
>>>>>
>>>>>
>>>>> So this validate device features instead of driver which is the
>>>>> one we really want to check?
>>>>>
>>>>> Thanks
>>>>
>>>> Hi Jason,
>>>>
>>>> Here we check device feature bits to make sure the device support
>>>> ACCESS_PLATFORM.
>>>
>>>
>>> If you want to check device features, you need to do that during
>>> probe() and fail the probing if without the feature. But I think you
>>> won't ship cards without ACCESS_PLATFORM.
>> Yes, there are no reasons ship a card without ACCESS_PLATFORM
>>>
>>>
>>>> In get_features(),
>>>> it will return a intersection of device features bit and driver
>>>> supported features bits(which includes ACCESS_PLATFORM).
>>>> Other components like QEMU should not set features bits more than
>>>> this intersection of bits. so we can make sure if this
>>>> ifcvf_verify_min_features() passed, both device and driver support
>>>> ACCESS_PLATFORM.
>>>>
>>>> Are you suggesting check driver feature bits in
>>>> ifcvf_verify_min_features() in the meantime as well?
>>>
>>>
>>> So it really depends on your hardware. If you hardware can always
>>> offer ACCESS_PLATFORM, you just need to check driver features. This
>>> is how vdpa_sim and mlx5_vdpa work.
>> Yes, we always support ACCESS_PLATFORM, so it is hard coded in the
>> macro IFCVF_SUPPORTED_FEATURES.
>
>
> That's not what I read from the code:
>
> features = ifcvf_get_features(vf) & IFCVF_SUPPORTED_FEATURES;
ifcvf_get_features() reads device feature bits(which should always has
ACCSSS_PLATFORM) and IFCVF_SUPPORTED_FEATURES is the driver supported
feature bits which hard coded ACCESS_PLATFORM, so the intersection
should include ACCESS_PLATFORM.
the intersection "features" is returned in get_features(), qemu should
set features according to it.
>
>
>> Now we check whether device support this feature bit as a double
>> conformation, are you suggesting we should check whether
>> ACCESS_PLATFORM & IFCVF_SUPPORTED_FEATURES
>> in set_features() as well?
>
>
> If we know device will always offer ACCESS_PLATFORM, there's no need
> to check it again. What we should check if whether driver set that,
> and if it doesn't we need to fail set_features(). I think there's
> little chance that IFCVF can work when IOMMU_PLATFORM is not negotiated.
Agree, will check the features bit to set instead of device feature
bits. Thanks!
>
>
>
>> I prefer check both device and IFCVF_SUPPORTED_FEATURES both, more
>> reliable.
>
>
> So again, if you want to check device features, set_features() is not
> the proper place. We need to fail the probe in this case.
>
> Thanks
>
>
>>
>> Thanks!
>>>
>>> Thanks
>>>
>>>
>>>>
>>>> Thanks!
>>>>>
>>>>>
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> vf->req_features = features;
>>>>>
>>>>> _______________________________________________
>>>>> Virtualization mailing list
>>>>> Virtualization@lists.linux-foundation.org
>>>>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
>>>>
>>>
>>
>
next prev parent reply other threads:[~2021-03-12 6:41 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-10 9:00 [PATCH V3 0/6] vDPA/ifcvf: enables Intel C5000X-PL virtio-net Zhu Lingshan
2021-03-10 9:00 ` [PATCH V3 1/6] vDPA/ifcvf: get_vendor_id returns a device specific vendor id Zhu Lingshan
2021-03-11 3:23 ` Jason Wang
2021-03-11 4:21 ` Zhu Lingshan
2021-03-11 6:13 ` Jason Wang
2021-03-11 7:34 ` Zhu, Lingshan
2021-03-10 9:00 ` [PATCH V3 2/6] vDPA/ifcvf: enable Intel C5000X-PL virtio-net for vDPA Zhu Lingshan
2021-03-10 9:00 ` [PATCH V3 3/6] vDPA/ifcvf: rename original IFCVF dev ids to N3000 ids Zhu Lingshan
2021-03-11 3:25 ` Jason Wang
2021-03-11 4:23 ` Zhu Lingshan
2021-03-11 6:14 ` Jason Wang
2021-03-10 9:00 ` [PATCH V3 4/6] vDPA/ifcvf: remove the version number string Zhu Lingshan
2021-03-10 9:16 ` Leon Romanovsky
2021-03-10 9:00 ` [PATCH V3 5/6] vDPA/ifcvf: fetch device feature bits when probe Zhu Lingshan
2021-03-10 9:00 ` [PATCH V3 6/6] vDPA/ifcvf: verify mandatory feature bits for vDPA Zhu Lingshan
2021-03-11 3:20 ` Jason Wang
2021-03-11 4:16 ` Zhu Lingshan
2021-03-11 6:20 ` Jason Wang
2021-03-11 7:19 ` Zhu, Lingshan
2021-03-12 5:52 ` Jason Wang
2021-03-12 6:40 ` Zhu, Lingshan [this message]
2021-03-12 7:00 ` Jason Wang
2021-03-12 7:08 ` Zhu, Lingshan
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=2a6e31d3-ea31-9b64-0749-1f149b656623@intel.com \
--to=lingshan.zhu@intel.com \
--cc=jasowang@redhat.com \
--cc=kvm@vger.kernel.org \
--cc=leonro@nvidia.com \
--cc=lingshan.zhu@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=lulu@redhat.com \
--cc=mst@redhat.com \
--cc=netdev@vger.kernel.org \
--cc=virtualization@lists.linux-foundation.org \
/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).