Parav Pandit
2021-Mar-01 07:29 UTC
[PATCH linux-next 2/9] vdpa: Introduce query of device config layout
> From: Jason Wang <jasowang at redhat.com> > Sent: Monday, March 1, 2021 9:29 AM > > On 2021/2/26 4:50 ??, Parav Pandit wrote: > > > >> From: Jason Wang <jasowang at redhat.com> > >> Sent: Friday, February 26, 2021 1:56 PM > >> > >> > >> On 2021/2/26 1:32 ??, Parav Pandit wrote: > >>>> From: Jason Wang <jasowang at redhat.com> > >>>> Sent: Wednesday, February 24, 2021 2:18 PM > >>>> > >>>> On 2021/2/24 2:18 ??, Parav Pandit wrote: > >>>>> + > >>>>> + if (nla_put_u16(msg, > VDPA_ATTR_DEV_NET_CFG_MAX_VQP, > >>>>> + config->max_virtqueue_pairs)) > >>>>> + return -EMSGSIZE; > >>>> We probably need to check VIRTIO_NET_F_RSS here. > >>> Yes. Will use it in v2. > >>> > >>>>> + if (nla_put_u8(msg, > VDPA_ATTR_DEV_NET_CFG_RSS_MAX_KEY_LEN, > >>>>> + config->rss_max_key_size)) > >>>>> + return -EMSGSIZE; > >>>>> + > >>>>> + rss_field = le16_to_cpu(config->rss_max_key_size); > >>>>> + if (nla_put_u16(msg, > VDPA_ATTR_DEV_NET_CFG_RSS_MAX_IT_LEN, > >>>> rss_field)) > >>>>> + return -EMSGSIZE; > >>>>> + > >>>>> + hash_types = le32_to_cpu(config->supported_hash_types); > >>>>> + if (nla_put_u32(msg, > VDPA_ATTR_DEV_NET_CFG_RSS_HASH_TYPES, > >>>>> + config->supported_hash_types)) > >>>>> + return -EMSGSIZE; > >>>>> + return 0; > >>>>> +} > >>>>> + > >>>>> +static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, > >>>>> +struct sk_buff *msg) { > >>>>> + struct virtio_net_config config = {}; > >>>>> + > >>>>> + vdev->config->get_config(vdev, 0, &config, sizeof(config)); > >>>> Do we need to sync with other possible get_config calls here? > >>> To readers of get_config() is ok. No need to sync. > >>> However, I think set_config() and get_config() should sync otherwise > >> get_config can get partial value. > >>> Will probably have to add vdpa_device->config_access_lock(). > >> > >> Probably. And the set_config() should be synchronized with the > >> requrest that comes from vDPA bus. > > Yes, a rw semaphore is good enough. > > Device config fields can be change from the device side anyway, such as > link status anyway. > > Sync using lock shouldn?t be problem. > > > >> This makes me think whether we should go back to two phases method, > >> create and enable. > >> > >> The vDPA device is only registred after enabling, then there's no > >> need to sync with vDPA bus, and mgmt is not allowed to change config > after enalbing? > >> > > In that case we should be able to disable it as well. Disable should take the > device of the bus. > > I find it weird to have this model to linger around the device without > anchoring on the bus. > > For example device is not yet enabled, so it is not anchored on the bus, but > its name is still have to remain unique. > > > Can we do some synchornization between vdpa device allocation and vdpa > device registier to solve the naming issue?mgmt tool querying the device config space after vdpa device is in use is real. So I don't see solving it any differently. Now upper layers of vdpa to not access the device on the placed on the vdpa bus, can be taken care by existing driver code using echo 0 > /sys/bus/vdpa/drivers_autoprobe By default vdpa device placed on the bus wont bind to any driver (net/vhost etc). 1. Decision to bind to which driver is done after config of the vdpa device is done. 2. orchestration sw does create and config before it binds to the right driver 3. which driver to bind to decided based on the use case of that individual vdpa device For example, (a) vdpa0 binds to net driver to have Netdev for container (b) vdpa1 binds to vhost driver to map vdpa device to VM> > > > Do we have to anchor vdpa device on the bus? Can vdpa be a class? > > > I think not, it's a bus that is expected to be bound to different driver > instead of a subsystem.Yeah, I realized it lately after writing the previous email. Lets see if above scheme works or not to use existing drivers_autoprobe file.
Jason Wang
2021-Mar-01 07:50 UTC
[PATCH linux-next 2/9] vdpa: Introduce query of device config layout
On 2021/3/1 3:29 ??, Parav Pandit wrote:> >> From: Jason Wang <jasowang at redhat.com> >> Sent: Monday, March 1, 2021 9:29 AM >> >> On 2021/2/26 4:50 ??, Parav Pandit wrote: >>>> From: Jason Wang <jasowang at redhat.com> >>>> Sent: Friday, February 26, 2021 1:56 PM >>>> >>>> >>>> On 2021/2/26 1:32 ??, Parav Pandit wrote: >>>>>> From: Jason Wang <jasowang at redhat.com> >>>>>> Sent: Wednesday, February 24, 2021 2:18 PM >>>>>> >>>>>> On 2021/2/24 2:18 ??, Parav Pandit wrote: >>>>>>> + >>>>>>> + if (nla_put_u16(msg, >> VDPA_ATTR_DEV_NET_CFG_MAX_VQP, >>>>>>> + config->max_virtqueue_pairs)) >>>>>>> + return -EMSGSIZE; >>>>>> We probably need to check VIRTIO_NET_F_RSS here. >>>>> Yes. Will use it in v2. >>>>> >>>>>>> + if (nla_put_u8(msg, >> VDPA_ATTR_DEV_NET_CFG_RSS_MAX_KEY_LEN, >>>>>>> + config->rss_max_key_size)) >>>>>>> + return -EMSGSIZE; >>>>>>> + >>>>>>> + rss_field = le16_to_cpu(config->rss_max_key_size); >>>>>>> + if (nla_put_u16(msg, >> VDPA_ATTR_DEV_NET_CFG_RSS_MAX_IT_LEN, >>>>>> rss_field)) >>>>>>> + return -EMSGSIZE; >>>>>>> + >>>>>>> + hash_types = le32_to_cpu(config->supported_hash_types); >>>>>>> + if (nla_put_u32(msg, >> VDPA_ATTR_DEV_NET_CFG_RSS_HASH_TYPES, >>>>>>> + config->supported_hash_types)) >>>>>>> + return -EMSGSIZE; >>>>>>> + return 0; >>>>>>> +} >>>>>>> + >>>>>>> +static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, >>>>>>> +struct sk_buff *msg) { >>>>>>> + struct virtio_net_config config = {}; >>>>>>> + >>>>>>> + vdev->config->get_config(vdev, 0, &config, sizeof(config)); >>>>>> Do we need to sync with other possible get_config calls here? >>>>> To readers of get_config() is ok. No need to sync. >>>>> However, I think set_config() and get_config() should sync otherwise >>>> get_config can get partial value. >>>>> Will probably have to add vdpa_device->config_access_lock(). >>>> Probably. And the set_config() should be synchronized with the >>>> requrest that comes from vDPA bus. >>> Yes, a rw semaphore is good enough. >>> Device config fields can be change from the device side anyway, such as >> link status anyway. >>> Sync using lock shouldn?t be problem. >>> >>>> This makes me think whether we should go back to two phases method, >>>> create and enable. >>>> >>>> The vDPA device is only registred after enabling, then there's no >>>> need to sync with vDPA bus, and mgmt is not allowed to change config >> after enalbing? >>> In that case we should be able to disable it as well. Disable should take the >> device of the bus. >>> I find it weird to have this model to linger around the device without >> anchoring on the bus. >>> For example device is not yet enabled, so it is not anchored on the bus, but >> its name is still have to remain unique. >> >> >> Can we do some synchornization between vdpa device allocation and vdpa >> device registier to solve the naming issue? > mgmt tool querying the device config space after vdpa device is in use is real. > So I don't see solving it any differently. > > Now upper layers of vdpa to not access the device on the placed on the vdpa bus, can be taken care by existing driver code using > echo 0 > /sys/bus/vdpa/drivers_autoprobe > > By default vdpa device placed on the bus wont bind to any driver (net/vhost etc). > 1. Decision to bind to which driver is done after config of the vdpa device is done. > 2. orchestration sw does create and config before it binds to the right driver > 3. which driver to bind to decided based on the use case of that individual vdpa device > For example, > (a) vdpa0 binds to net driver to have Netdev for container > (b) vdpa1 binds to vhost driver to map vdpa device to VMI think it should work. Adding Adrian, Maxime and Sean for more comments.> >>> Do we have to anchor vdpa device on the bus? Can vdpa be a class? >> >> I think not, it's a bus that is expected to be bound to different driver >> instead of a subsystem. > Yeah, I realized it lately after writing the previous email. > Lets see if above scheme works or not to use existing drivers_autoprobe file.Yes. Thanks