Messages in this thread | | | Subject | Re: [PATCH] vdpa/mlx5: set_features should allow reset to zero | From | Jason Wang <> | Date | Wed, 24 Feb 2021 14:53:08 +0800 |
| |
On 2021/2/24 2:46 下午, Michael S. Tsirkin wrote: > On Wed, Feb 24, 2021 at 02:04:36PM +0800, Jason Wang wrote: >> On 2021/2/24 1:04 下午, Michael S. Tsirkin wrote: >>> On Tue, Feb 23, 2021 at 11:35:57AM -0800, Si-Wei Liu wrote: >>>> On 2/23/2021 5:26 AM, Michael S. Tsirkin wrote: >>>>> On Tue, Feb 23, 2021 at 10:03:57AM +0800, Jason Wang wrote: >>>>>> On 2021/2/23 9:12 上午, Si-Wei Liu wrote: >>>>>>> On 2/21/2021 11:34 PM, Michael S. Tsirkin wrote: >>>>>>>> On Mon, Feb 22, 2021 at 12:14:17PM +0800, Jason Wang wrote: >>>>>>>>> On 2021/2/19 7:54 下午, Si-Wei Liu wrote: >>>>>>>>>> Commit 452639a64ad8 ("vdpa: make sure set_features is invoked >>>>>>>>>> for legacy") made an exception for legacy guests to reset >>>>>>>>>> features to 0, when config space is accessed before features >>>>>>>>>> are set. We should relieve the verify_min_features() check >>>>>>>>>> and allow features reset to 0 for this case. >>>>>>>>>> >>>>>>>>>> It's worth noting that not just legacy guests could access >>>>>>>>>> config space before features are set. For instance, when >>>>>>>>>> feature VIRTIO_NET_F_MTU is advertised some modern driver >>>>>>>>>> will try to access and validate the MTU present in the config >>>>>>>>>> space before virtio features are set. >>>>>>>>> This looks like a spec violation: >>>>>>>>> >>>>>>>>> " >>>>>>>>> >>>>>>>>> The following driver-read-only field, mtu only exists if >>>>>>>>> VIRTIO_NET_F_MTU is >>>>>>>>> set. >>>>>>>>> This field specifies the maximum MTU for the driver to use. >>>>>>>>> " >>>>>>>>> >>>>>>>>> Do we really want to workaround this? >>>>>>>>> >>>>>>>>> Thanks >>>>>>>> And also: >>>>>>>> >>>>>>>> The driver MUST follow this sequence to initialize a device: >>>>>>>> 1. Reset the device. >>>>>>>> 2. Set the ACKNOWLEDGE status bit: the guest OS has noticed the device. >>>>>>>> 3. Set the DRIVER status bit: the guest OS knows how to drive the >>>>>>>> device. >>>>>>>> 4. Read device feature bits, and write the subset of feature bits >>>>>>>> understood by the OS and driver to the >>>>>>>> device. During this step the driver MAY read (but MUST NOT write) >>>>>>>> the device-specific configuration >>>>>>>> fields to check that it can support the device before accepting it. >>>>>>>> 5. Set the FEATURES_OK status bit. The driver MUST NOT accept new >>>>>>>> feature bits after this step. >>>>>>>> 6. Re-read device status to ensure the FEATURES_OK bit is still set: >>>>>>>> otherwise, the device does not >>>>>>>> support our subset of features and the device is unusable. >>>>>>>> 7. Perform device-specific setup, including discovery of virtqueues >>>>>>>> for the device, optional per-bus setup, >>>>>>>> reading and possibly writing the device’s virtio configuration >>>>>>>> space, and population of virtqueues. >>>>>>>> 8. Set the DRIVER_OK status bit. At this point the device is “live”. >>>>>>>> >>>>>>>> >>>>>>>> so accessing config space before FEATURES_OK is a spec violation, right? >>>>>>> It is, but it's not relevant to what this commit tries to address. I >>>>>>> thought the legacy guest still needs to be supported. >>>>>>> >>>>>>> Having said, a separate patch has to be posted to fix the guest driver >>>>>>> issue where this discrepancy is introduced to virtnet_validate() (since >>>>>>> commit fe36cbe067). But it's not technically related to this patch. >>>>>>> >>>>>>> -Siwei >>>>>> I think it's a bug to read config space in validate, we should move it to >>>>>> virtnet_probe(). >>>>>> >>>>>> Thanks >>>>> I take it back, reading but not writing seems to be explicitly allowed by spec. >>>>> So our way to detect a legacy guest is bogus, need to think what is >>>>> the best way to handle this. >>>> Then maybe revert commit fe36cbe067 and friends, and have QEMU detect legacy >>>> guest? Supposedly only config space write access needs to be guarded before >>>> setting FEATURES_OK. >>>> >>>> -Siwie >>> Detecting it isn't enough though, we will need a new ioctl to notify >>> the kernel that it's a legacy guest. Ugh :( >> >> I'm not sure I get this, how can we know if there's a legacy driver before >> set_features()? > qemu knows for sure. It does not communicate this information to the > kernel right now unfortunately.
I may miss something, but I still don't get how the new ioctl is supposed to work.
Thanks
> >> And I wonder what will hapeen if we just revert the set_features(0)? >> >> Thanks >> >> >>> >>>>>>>>>> Rejecting reset to 0 >>>>>>>>>> prematurely causes correct MTU and link status unable to load >>>>>>>>>> for the very first config space access, rendering issues like >>>>>>>>>> guest showing inaccurate MTU value, or failure to reject >>>>>>>>>> out-of-range MTU. >>>>>>>>>> >>>>>>>>>> Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for >>>>>>>>>> supported mlx5 devices") >>>>>>>>>> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com> >>>>>>>>>> --- >>>>>>>>>> drivers/vdpa/mlx5/net/mlx5_vnet.c | 15 +-------------- >>>>>>>>>> 1 file changed, 1 insertion(+), 14 deletions(-) >>>>>>>>>> >>>>>>>>>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c >>>>>>>>>> b/drivers/vdpa/mlx5/net/mlx5_vnet.c >>>>>>>>>> index 7c1f789..540dd67 100644 >>>>>>>>>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c >>>>>>>>>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c >>>>>>>>>> @@ -1490,14 +1490,6 @@ static u64 >>>>>>>>>> mlx5_vdpa_get_features(struct vdpa_device *vdev) >>>>>>>>>> return mvdev->mlx_features; >>>>>>>>>> } >>>>>>>>>> -static int verify_min_features(struct mlx5_vdpa_dev *mvdev, >>>>>>>>>> u64 features) >>>>>>>>>> -{ >>>>>>>>>> - if (!(features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM))) >>>>>>>>>> - return -EOPNOTSUPP; >>>>>>>>>> - >>>>>>>>>> - return 0; >>>>>>>>>> -} >>>>>>>>>> - >>>>>>>>>> static int setup_virtqueues(struct mlx5_vdpa_net *ndev) >>>>>>>>>> { >>>>>>>>>> int err; >>>>>>>>>> @@ -1558,18 +1550,13 @@ static int >>>>>>>>>> mlx5_vdpa_set_features(struct vdpa_device *vdev, u64 >>>>>>>>>> features) >>>>>>>>>> { >>>>>>>>>> struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev); >>>>>>>>>> struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev); >>>>>>>>>> - int err; >>>>>>>>>> print_features(mvdev, features, true); >>>>>>>>>> - err = verify_min_features(mvdev, features); >>>>>>>>>> - if (err) >>>>>>>>>> - return err; >>>>>>>>>> - >>>>>>>>>> ndev->mvdev.actual_features = features & >>>>>>>>>> ndev->mvdev.mlx_features; >>>>>>>>>> ndev->config.mtu = cpu_to_mlx5vdpa16(mvdev, ndev->mtu); >>>>>>>>>> ndev->config.status |= cpu_to_mlx5vdpa16(mvdev, >>>>>>>>>> VIRTIO_NET_S_LINK_UP); >>>>>>>>>> - return err; >>>>>>>>>> + return 0; >>>>>>>>>> } >>>>>>>>>> static void mlx5_vdpa_set_config_cb(struct vdpa_device >>>>>>>>>> *vdev, struct vdpa_callback *cb)
| |