On Wed, Apr 21, 2021 at 03:41:36PM +0800, Jason Wang wrote:> > ? 2021/4/12 ??5:23, Jason Wang ??: > > > > ? 2021/4/12 ??5:09, Michael S. Tsirkin ??: > > > On Mon, Apr 12, 2021 at 02:35:07PM +0800, Jason Wang wrote: > > > > ? 2021/4/10 ??12:04, Michael S. Tsirkin ??: > > > > > On Fri, Apr 09, 2021 at 12:47:55PM +0800, Jason Wang wrote: > > > > > > ? 2021/4/8 ??11:59, Michael S. Tsirkin ??: > > > > > > > On Thu, Apr 08, 2021 at 04:26:48PM +0800, Jason Wang wrote: > > > > > > > > This patch mandates 1.0 for vDPA devices. The goal is to have the > > > > > > > > semantic of normative statement in the virtio > > > > > > > > spec and eliminate the > > > > > > > > burden of transitional device for both vDPA bus and vDPA parent. > > > > > > > > > > > > > > > > uAPI seems fine since all the vDPA parent mandates > > > > > > > > VIRTIO_F_ACCESS_PLATFORM which implies 1.0 devices. > > > > > > > > > > > > > > > > For legacy guests, it can still work since Qemu will mediate when > > > > > > > > necessary (e.g doing the endian conversion). > > > > > > > > > > > > > > > > Signed-off-by: Jason Wang <jasowang at redhat.com> > > > > > > > Hmm. If we do this, don't we still have a problem with > > > > > > > legacy drivers which don't ack 1.0? > > > > > > Yes, but it's not something that is introduced in this > > > > > > commit. The legacy > > > > > > driver never work ... > > > > > My point is this neither fixes or prevents this. > > > > > > > > > > So my suggestion is to finally add ioctls along the lines > > > > > of PROTOCOL_FEATURES of vhost-user. > > > > > > > > > > Then that one can have bits for legacy le, legacy be and modern. > > > > > > > > > > BTW I looked at vhost-user and it does not look like that > > > > > has a solution for this problem either, right? > > > > > > > > Right. > > > > > > > > > > > > > > > > > > > > Note 1.0 affects ring endianness which is not mediated in QEMU > > > > > > > so QEMU can't pretend to device guest is 1.0. > > > > > > Right, I plan to send patches to do mediation in the > > > > > > Qemu to unbreak legacy > > > > > > drivers. > > > > > > > > > > > > Thanks > > > > > I frankly think we'll need PROTOCOL_FEATURES anyway, it's > > > > > too useful ... > > > > > so why not teach drivers about it and be done with it? You > > > > > can't emulate > > > > > legacy on modern in a cross endian situation because of vring > > > > > endian-ness ... > > > > > > > > So the problem still. This can only work when the hardware can support > > > > legacy vring endian-ness. > > > > > > > > Consider: > > > > > > > > 1) the leagcy driver support is non-normative in the spec > > > > 2) support a transitional device in the kenrel may requires the > > > > hardware > > > > support and a burden of kernel codes > > > > > > > > I'd rather simply drop the legacy driver support > > > > > > My point is this patch does not drop legacy support. It merely mandates > > > modern support. > > > > > > I am not sure I get here. This patch fails the set_feature if VERSION_1 > > is not negotiated. This means: > > > > 1) vDPA presents a modern device instead of transitonal device > > 2) legacy driver can't be probed > > > > What I'm missing? > > > Hi Michael: > > Do you agree to find the way to present modern device? We need a conclusion > to make the netlink API work to move forward. > > ThanksI think we need a way to support legacy with no data path overhead. qemu setting VERSION_1 for a legacy guest affects the ring format so it does not really work. This seems to rule out emulating config space entirely in userspace. So I think we should add an ioctl along the lines of protocol features. Then I think we can reserve feature bits for config space format: legacy LE, legacy BE, modern. Querying the feature bits will provide us with info about what does the device support. Acking them will tell device what does guest need.> > > > > > > > > > > > to have a simple and easy > > > > abstarction in the kenrel. For legacy driver in the guest, > > > > hypervisor is in > > > > charge of the mediation: > > > > > > > > 1) config space access endian conversion > > > > 2) using shadow virtqueue to change the endian in the vring > > > > > > > > Thanks > > > I'd like to avoid shadow virtqueue hacks if at all possible. > > > Last I checked performance wasn't much better than just emulating > > > virtio in software. > > > > > > I think the legacy driver support is just a nice to have. Or do you see > > any value to that? I guess for mellanox and intel, only modern device is > > supported in the hardware. > > > > Thanks > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > --- > > > > > > > > ??? include/linux/vdpa.h | 6 ++++++ > > > > > > > > ??? 1 file changed, 6 insertions(+) > > > > > > > > > > > > > > > > diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h > > > > > > > > index 0fefeb976877..cfde4ec999b4 100644 > > > > > > > > --- a/include/linux/vdpa.h > > > > > > > > +++ b/include/linux/vdpa.h > > > > > > > > @@ -6,6 +6,7 @@ > > > > > > > > ??? #include <linux/device.h> > > > > > > > > ??? #include <linux/interrupt.h> > > > > > > > > ??? #include <linux/vhost_iotlb.h> > > > > > > > > +#include <uapi/linux/virtio_config.h> > > > > > > > > ??? /** > > > > > > > > ???? * vDPA callback definition. > > > > > > > > @@ -317,6 +318,11 @@ static inline int > > > > > > > > vdpa_set_features(struct vdpa_device *vdev, u64 > > > > > > > > features) > > > > > > > > ??? { > > > > > > > > ??????????? const struct vdpa_config_ops *ops = vdev->config; > > > > > > > > +??????? /* Mandating 1.0 to have semantics of > > > > > > > > normative statements in > > > > > > > > +???????? * the spec. */ > > > > > > > > +??????? if (!(features & BIT_ULL(VIRTIO_F_VERSION_1))) > > > > > > > > +??????? return -EINVAL; > > > > > > > > + > > > > > > > > ??????? vdev->features_valid = true; > > > > > > > > ??????????? return ops->set_features(vdev, features); > > > > > > > > ??? } > > > > > > > > -- > > > > > > > > 2.25.1 > >
? 2021/4/21 ??4:03, Michael S. Tsirkin ??:> On Wed, Apr 21, 2021 at 03:41:36PM +0800, Jason Wang wrote: >> ? 2021/4/12 ??5:23, Jason Wang ??: >>> ? 2021/4/12 ??5:09, Michael S. Tsirkin ??: >>>> On Mon, Apr 12, 2021 at 02:35:07PM +0800, Jason Wang wrote: >>>>> ? 2021/4/10 ??12:04, Michael S. Tsirkin ??: >>>>>> On Fri, Apr 09, 2021 at 12:47:55PM +0800, Jason Wang wrote: >>>>>>> ? 2021/4/8 ??11:59, Michael S. Tsirkin ??: >>>>>>>> On Thu, Apr 08, 2021 at 04:26:48PM +0800, Jason Wang wrote: >>>>>>>>> This patch mandates 1.0 for vDPA devices. The goal is to have the >>>>>>>>> semantic of normative statement in the virtio >>>>>>>>> spec and eliminate the >>>>>>>>> burden of transitional device for both vDPA bus and vDPA parent. >>>>>>>>> >>>>>>>>> uAPI seems fine since all the vDPA parent mandates >>>>>>>>> VIRTIO_F_ACCESS_PLATFORM which implies 1.0 devices. >>>>>>>>> >>>>>>>>> For legacy guests, it can still work since Qemu will mediate when >>>>>>>>> necessary (e.g doing the endian conversion). >>>>>>>>> >>>>>>>>> Signed-off-by: Jason Wang <jasowang at redhat.com> >>>>>>>> Hmm. If we do this, don't we still have a problem with >>>>>>>> legacy drivers which don't ack 1.0? >>>>>>> Yes, but it's not something that is introduced in this >>>>>>> commit. The legacy >>>>>>> driver never work ... >>>>>> My point is this neither fixes or prevents this. >>>>>> >>>>>> So my suggestion is to finally add ioctls along the lines >>>>>> of PROTOCOL_FEATURES of vhost-user. >>>>>> >>>>>> Then that one can have bits for legacy le, legacy be and modern. >>>>>> >>>>>> BTW I looked at vhost-user and it does not look like that >>>>>> has a solution for this problem either, right? >>>>> Right. >>>>> >>>>> >>>>>>>> Note 1.0 affects ring endianness which is not mediated in QEMU >>>>>>>> so QEMU can't pretend to device guest is 1.0. >>>>>>> Right, I plan to send patches to do mediation in the >>>>>>> Qemu to unbreak legacy >>>>>>> drivers. >>>>>>> >>>>>>> Thanks >>>>>> I frankly think we'll need PROTOCOL_FEATURES anyway, it's >>>>>> too useful ... >>>>>> so why not teach drivers about it and be done with it? You >>>>>> can't emulate >>>>>> legacy on modern in a cross endian situation because of vring >>>>>> endian-ness ... >>>>> So the problem still. This can only work when the hardware can support >>>>> legacy vring endian-ness. >>>>> >>>>> Consider: >>>>> >>>>> 1) the leagcy driver support is non-normative in the spec >>>>> 2) support a transitional device in the kenrel may requires the >>>>> hardware >>>>> support and a burden of kernel codes >>>>> >>>>> I'd rather simply drop the legacy driver support >>>> My point is this patch does not drop legacy support. It merely mandates >>>> modern support. >>> >>> I am not sure I get here. This patch fails the set_feature if VERSION_1 >>> is not negotiated. This means: >>> >>> 1) vDPA presents a modern device instead of transitonal device >>> 2) legacy driver can't be probed >>> >>> What I'm missing? >> >> Hi Michael: >> >> Do you agree to find the way to present modern device? We need a conclusion >> to make the netlink API work to move forward. >> >> Thanks > I think we need a way to support legacy with no data path overhead. qemu > setting VERSION_1 for a legacy guest affects the ring format so it does > not really work. This seems to rule out emulating config space entirely > in userspace.So I'd rather drop the legacy support in this case. It never work for vDPA in the past and virtio-vDPA doesn't even need that. Note that ACCESS_PLATFORM is mandated for all the vDPA parents right now which implies modern device and LE. I wonder what's the value for supporting legacy in this case or do we really encourage vendors to ship card with legacy support (e.g endian support in the hardware)?> > So I think we should add an ioctl along the lines of > protocol features. Then I think we can reserve feature bits > for config space format: legacy LE, legacy BE, modern.We had VHOST_SET_VRING_ENDIAN but this will complicates both the vDPA parent and management. What's more important, legacy behaviour is not restrictied by the spec.> > Querying the feature bits will provide us with info about > what does the device support. Acking them will tell device > what does guest need.I think this can work, but I wonder how much we can gain from such complexitiy. Thanks> > > > > >>> >>>>> to have a simple and easy >>>>> abstarction in the kenrel. For legacy driver in the guest, >>>>> hypervisor is in >>>>> charge of the mediation: >>>>> >>>>> 1) config space access endian conversion >>>>> 2) using shadow virtqueue to change the endian in the vring >>>>> >>>>> Thanks >>>> I'd like to avoid shadow virtqueue hacks if at all possible. >>>> Last I checked performance wasn't much better than just emulating >>>> virtio in software. >>> >>> I think the legacy driver support is just a nice to have. Or do you see >>> any value to that? I guess for mellanox and intel, only modern device is >>> supported in the hardware. >>> >>> Thanks >>> >>> >>>>>>>> >>>>>>>> >>>>>>>>> --- >>>>>>>>> ??? include/linux/vdpa.h | 6 ++++++ >>>>>>>>> ??? 1 file changed, 6 insertions(+) >>>>>>>>> >>>>>>>>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h >>>>>>>>> index 0fefeb976877..cfde4ec999b4 100644 >>>>>>>>> --- a/include/linux/vdpa.h >>>>>>>>> +++ b/include/linux/vdpa.h >>>>>>>>> @@ -6,6 +6,7 @@ >>>>>>>>> ??? #include <linux/device.h> >>>>>>>>> ??? #include <linux/interrupt.h> >>>>>>>>> ??? #include <linux/vhost_iotlb.h> >>>>>>>>> +#include <uapi/linux/virtio_config.h> >>>>>>>>> ??? /** >>>>>>>>> ???? * vDPA callback definition. >>>>>>>>> @@ -317,6 +318,11 @@ static inline int >>>>>>>>> vdpa_set_features(struct vdpa_device *vdev, u64 >>>>>>>>> features) >>>>>>>>> ??? { >>>>>>>>> ??????????? const struct vdpa_config_ops *ops = vdev->config; >>>>>>>>> +??????? /* Mandating 1.0 to have semantics of >>>>>>>>> normative statements in >>>>>>>>> +???????? * the spec. */ >>>>>>>>> +??????? if (!(features & BIT_ULL(VIRTIO_F_VERSION_1))) >>>>>>>>> +??????? return -EINVAL; >>>>>>>>> + >>>>>>>>> ??????? vdev->features_valid = true; >>>>>>>>> ??????????? return ops->set_features(vdev, features); >>>>>>>>> ??? } >>>>>>>>> -- >>>>>>>>> 2.25.1
----- ???? -----> > ? 2021/4/21 ??4:03, Michael S. Tsirkin ??: > > On Wed, Apr 21, 2021 at 03:41:36PM +0800, Jason Wang wrote: > >> ? 2021/4/12 ??5:23, Jason Wang ??: > >>> ? 2021/4/12 ??5:09, Michael S. Tsirkin ??: > >>>> On Mon, Apr 12, 2021 at 02:35:07PM +0800, Jason Wang wrote: > >>>>> ? 2021/4/10 ??12:04, Michael S. Tsirkin ??: > >>>>>> On Fri, Apr 09, 2021 at 12:47:55PM +0800, Jason Wang wrote: > >>>>>>> ? 2021/4/8 ??11:59, Michael S. Tsirkin ??: > >>>>>>>> On Thu, Apr 08, 2021 at 04:26:48PM +0800, Jason Wang wrote: > >>>>>>>>> This patch mandates 1.0 for vDPA devices. The goal is to have the > >>>>>>>>> semantic of normative statement in the virtio > >>>>>>>>> spec and eliminate the > >>>>>>>>> burden of transitional device for both vDPA bus and vDPA parent. > >>>>>>>>> > >>>>>>>>> uAPI seems fine since all the vDPA parent mandates > >>>>>>>>> VIRTIO_F_ACCESS_PLATFORM which implies 1.0 devices. > >>>>>>>>> > >>>>>>>>> For legacy guests, it can still work since Qemu will mediate when > >>>>>>>>> necessary (e.g doing the endian conversion). > >>>>>>>>> > >>>>>>>>> Signed-off-by: Jason Wang <jasowang at redhat.com> > >>>>>>>> Hmm. If we do this, don't we still have a problem with > >>>>>>>> legacy drivers which don't ack 1.0? > >>>>>>> Yes, but it's not something that is introduced in this > >>>>>>> commit. The legacy > >>>>>>> driver never work ... > >>>>>> My point is this neither fixes or prevents this. > >>>>>> > >>>>>> So my suggestion is to finally add ioctls along the lines > >>>>>> of PROTOCOL_FEATURES of vhost-user. > >>>>>> > >>>>>> Then that one can have bits for legacy le, legacy be and modern. > >>>>>> > >>>>>> BTW I looked at vhost-user and it does not look like that > >>>>>> has a solution for this problem either, right? > >>>>> Right. > >>>>> > >>>>> > >>>>>>>> Note 1.0 affects ring endianness which is not mediated in QEMU > >>>>>>>> so QEMU can't pretend to device guest is 1.0. > >>>>>>> Right, I plan to send patches to do mediation in the > >>>>>>> Qemu to unbreak legacy > >>>>>>> drivers. > >>>>>>> > >>>>>>> Thanks > >>>>>> I frankly think we'll need PROTOCOL_FEATURES anyway, it's > >>>>>> too useful ... > >>>>>> so why not teach drivers about it and be done with it? You > >>>>>> can't emulate > >>>>>> legacy on modern in a cross endian situation because of vring > >>>>>> endian-ness ... > >>>>> So the problem still. This can only work when the hardware can support > >>>>> legacy vring endian-ness. > >>>>> > >>>>> Consider: > >>>>> > >>>>> 1) the leagcy driver support is non-normative in the spec > >>>>> 2) support a transitional device in the kenrel may requires the > >>>>> hardware > >>>>> support and a burden of kernel codes > >>>>> > >>>>> I'd rather simply drop the legacy driver support > >>>> My point is this patch does not drop legacy support. It merely mandates > >>>> modern support. > >>> > >>> I am not sure I get here. This patch fails the set_feature if VERSION_1 > >>> is not negotiated. This means: > >>> > >>> 1) vDPA presents a modern device instead of transitonal device > >>> 2) legacy driver can't be probed > >>> > >>> What I'm missing? > >> > >> Hi Michael: > >> > >> Do you agree to find the way to present modern device? We need a > >> conclusion > >> to make the netlink API work to move forward. > >> > >> Thanks > > I think we need a way to support legacy with no data path overhead. qemu > > setting VERSION_1 for a legacy guest affects the ring format so it does > > not really work. This seems to rule out emulating config space entirely > > in userspace. > > > So I'd rather drop the legacy support in this case. It never work for > vDPA in the past and virtio-vDPA doesn't even need that. Note that > ACCESS_PLATFORM is mandated for all the vDPA parents right now which > implies modern device and LE. I wonder what's the value for supporting > legacy in this case or do we really encourage vendors to ship card with > legacy support (e.g endian support in the hardware)?Hi Michael: Any thoughts on this approach? My understanding is that dropping legacy support will simplify a lot of stuffs. Thanks> > > > > > So I think we should add an ioctl along the lines of > > protocol features. Then I think we can reserve feature bits > > for config space format: legacy LE, legacy BE, modern. > > > We had VHOST_SET_VRING_ENDIAN but this will complicates both the vDPA > parent and management. What's more important, legacy behaviour is not > restrictied by the spec. > > > > > > Querying the feature bits will provide us with info about > > what does the device support. Acking them will tell device > > what does guest need. > > > I think this can work, but I wonder how much we can gain from such > complexitiy. > > Thanks > > > > > > > > > > > > > >>> > >>>>> to have a simple and easy > >>>>> abstarction in the kenrel. For legacy driver in the guest, > >>>>> hypervisor is in > >>>>> charge of the mediation: > >>>>> > >>>>> 1) config space access endian conversion > >>>>> 2) using shadow virtqueue to change the endian in the vring > >>>>> > >>>>> Thanks > >>>> I'd like to avoid shadow virtqueue hacks if at all possible. > >>>> Last I checked performance wasn't much better than just emulating > >>>> virtio in software. > >>> > >>> I think the legacy driver support is just a nice to have. Or do you see > >>> any value to that? I guess for mellanox and intel, only modern device is > >>> supported in the hardware. > >>> > >>> Thanks > >>> > >>> > >>>>>>>> > >>>>>>>> > >>>>>>>>> --- > >>>>>>>>> ??? include/linux/vdpa.h | 6 ++++++ > >>>>>>>>> ??? 1 file changed, 6 insertions(+) > >>>>>>>>> > >>>>>>>>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h > >>>>>>>>> index 0fefeb976877..cfde4ec999b4 100644 > >>>>>>>>> --- a/include/linux/vdpa.h > >>>>>>>>> +++ b/include/linux/vdpa.h > >>>>>>>>> @@ -6,6 +6,7 @@ > >>>>>>>>> ??? #include <linux/device.h> > >>>>>>>>> ??? #include <linux/interrupt.h> > >>>>>>>>> ??? #include <linux/vhost_iotlb.h> > >>>>>>>>> +#include <uapi/linux/virtio_config.h> > >>>>>>>>> ??? /** > >>>>>>>>> ???? * vDPA callback definition. > >>>>>>>>> @@ -317,6 +318,11 @@ static inline int > >>>>>>>>> vdpa_set_features(struct vdpa_device *vdev, u64 > >>>>>>>>> features) > >>>>>>>>> ??? { > >>>>>>>>> ??????????? const struct vdpa_config_ops *ops = vdev->config; > >>>>>>>>> +??????? /* Mandating 1.0 to have semantics of > >>>>>>>>> normative statements in > >>>>>>>>> +???????? * the spec. */ > >>>>>>>>> +??????? if (!(features & BIT_ULL(VIRTIO_F_VERSION_1))) > >>>>>>>>> +??????? return -EINVAL; > >>>>>>>>> + > >>>>>>>>> ??????? vdev->features_valid = true; > >>>>>>>>> ??????????? return ops->set_features(vdev, features); > >>>>>>>>> ??? } > >>>>>>>>> -- > >>>>>>>>> 2.25.1 > >