Re: [PATCH v2 19/24] vdpa: make sure set_features in invoked for legacy
On 2020/8/6 下午6:00, Michael S. Tsirkin wrote: On Thu, Aug 06, 2020 at 03:27:38PM +0800, Jason Wang wrote: On 2020/8/6 下午1:53, Michael S. Tsirkin wrote: On Thu, Aug 06, 2020 at 11:23:05AM +0800, Jason Wang wrote: On 2020/8/5 下午7:40, Michael S. Tsirkin wrote: On Wed, Aug 05, 2020 at 02:14:07PM +0800, Jason Wang wrote: On 2020/8/4 上午5:00, Michael S. Tsirkin wrote: Some legacy guests just assume features are 0 after reset. We detect that config space is accessed before features are set and set features to 0 automatically. Note: some legacy guests might not even access config space, if this is reported in the field we might need to catch a kick to handle these. I wonder whether it's easier to just support modern device? Thanks Well hardware vendors are I think interested in supporting legacy guests. Limiting vdpa to modern only would make it uncompetitive. My understanding is that, IOMMU_PLATFORM is mandatory for hardware vDPA to work. Hmm I don't really see why. Assume host maps guest memory properly, VM does not have an IOMMU, legacy guest can just work. Yes, guest may not set IOMMU_PLATFORM. Care explaining what's wrong with this picture? The problem is virtio_vdpa, without IOMMU_PLATFORM it uses PA which can not work if IOMMU is enabled. Thanks So that's a virtio_vdpa limitation. Probably not, I think this goes back to the long debate of whether to use DMA API unconditionally. If we did that, we can support legacy virtio driver. The vDPA device needs to provide a DMA device and the virtio core and perform DMA API with that device which should work for all of the cases. But a big question is, do upstream care about out of tree virtio drivers? Thanks In the same way, if a device does not have an on-device iommu *and* is not behind an iommu, then vdpa can't bind to it. But this virtio_vdpa specific hack does not belong in a generic vdpa code. So it can only work for modern device ... Thanks ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 19/24] vdpa: make sure set_features in invoked for legacy
On Thu, Aug 06, 2020 at 03:27:38PM +0800, Jason Wang wrote: > > On 2020/8/6 下午1:53, Michael S. Tsirkin wrote: > > On Thu, Aug 06, 2020 at 11:23:05AM +0800, Jason Wang wrote: > > > On 2020/8/5 下午7:40, Michael S. Tsirkin wrote: > > > > On Wed, Aug 05, 2020 at 02:14:07PM +0800, Jason Wang wrote: > > > > > On 2020/8/4 上午5:00, Michael S. Tsirkin wrote: > > > > > > Some legacy guests just assume features are 0 after reset. > > > > > > We detect that config space is accessed before features are > > > > > > set and set features to 0 automatically. > > > > > > Note: some legacy guests might not even access config space, if > > > > > > this is > > > > > > reported in the field we might need to catch a kick to handle these. > > > > > I wonder whether it's easier to just support modern device? > > > > > > > > > > Thanks > > > > Well hardware vendors are I think interested in supporting legacy > > > > guests. Limiting vdpa to modern only would make it uncompetitive. > > > > > > My understanding is that, IOMMU_PLATFORM is mandatory for hardware vDPA to > > > work. > > Hmm I don't really see why. Assume host maps guest memory properly, > > VM does not have an IOMMU, legacy guest can just work. > > > Yes, guest may not set IOMMU_PLATFORM. > > > > > > Care explaining what's wrong with this picture? > > > The problem is virtio_vdpa, without IOMMU_PLATFORM it uses PA which can not > work if IOMMU is enabled. > > Thanks So that's a virtio_vdpa limitation. In the same way, if a device does not have an on-device iommu *and* is not behind an iommu, then vdpa can't bind to it. But this virtio_vdpa specific hack does not belong in a generic vdpa code. > > > > > > > > So it can only work for modern device ... > > > > > > Thanks > > > > > > > > > > > > > > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 19/24] vdpa: make sure set_features in invoked for legacy
On 2020/8/6 下午1:53, Michael S. Tsirkin wrote: On Thu, Aug 06, 2020 at 11:23:05AM +0800, Jason Wang wrote: On 2020/8/5 下午7:40, Michael S. Tsirkin wrote: On Wed, Aug 05, 2020 at 02:14:07PM +0800, Jason Wang wrote: On 2020/8/4 上午5:00, Michael S. Tsirkin wrote: Some legacy guests just assume features are 0 after reset. We detect that config space is accessed before features are set and set features to 0 automatically. Note: some legacy guests might not even access config space, if this is reported in the field we might need to catch a kick to handle these. I wonder whether it's easier to just support modern device? Thanks Well hardware vendors are I think interested in supporting legacy guests. Limiting vdpa to modern only would make it uncompetitive. My understanding is that, IOMMU_PLATFORM is mandatory for hardware vDPA to work. Hmm I don't really see why. Assume host maps guest memory properly, VM does not have an IOMMU, legacy guest can just work. Yes, guest may not set IOMMU_PLATFORM. Care explaining what's wrong with this picture? The problem is virtio_vdpa, without IOMMU_PLATFORM it uses PA which can not work if IOMMU is enabled. Thanks So it can only work for modern device ... Thanks ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 19/24] vdpa: make sure set_features in invoked for legacy
On Thu, Aug 06, 2020 at 11:23:05AM +0800, Jason Wang wrote: > > On 2020/8/5 下午7:40, Michael S. Tsirkin wrote: > > On Wed, Aug 05, 2020 at 02:14:07PM +0800, Jason Wang wrote: > > > On 2020/8/4 上午5:00, Michael S. Tsirkin wrote: > > > > Some legacy guests just assume features are 0 after reset. > > > > We detect that config space is accessed before features are > > > > set and set features to 0 automatically. > > > > Note: some legacy guests might not even access config space, if this is > > > > reported in the field we might need to catch a kick to handle these. > > > I wonder whether it's easier to just support modern device? > > > > > > Thanks > > Well hardware vendors are I think interested in supporting legacy > > guests. Limiting vdpa to modern only would make it uncompetitive. > > > My understanding is that, IOMMU_PLATFORM is mandatory for hardware vDPA to > work. Hmm I don't really see why. Assume host maps guest memory properly, VM does not have an IOMMU, legacy guest can just work. Care explaining what's wrong with this picture? > So it can only work for modern device ... > > Thanks > > > > > > > > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 19/24] vdpa: make sure set_features in invoked for legacy
On 2020/8/5 下午7:40, Michael S. Tsirkin wrote: On Wed, Aug 05, 2020 at 02:14:07PM +0800, Jason Wang wrote: On 2020/8/4 上午5:00, Michael S. Tsirkin wrote: Some legacy guests just assume features are 0 after reset. We detect that config space is accessed before features are set and set features to 0 automatically. Note: some legacy guests might not even access config space, if this is reported in the field we might need to catch a kick to handle these. I wonder whether it's easier to just support modern device? Thanks Well hardware vendors are I think interested in supporting legacy guests. Limiting vdpa to modern only would make it uncompetitive. My understanding is that, IOMMU_PLATFORM is mandatory for hardware vDPA to work. So it can only work for modern device ... Thanks ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 19/24] vdpa: make sure set_features in invoked for legacy
On Wed, Aug 05, 2020 at 02:14:07PM +0800, Jason Wang wrote:
>
> On 2020/8/4 上午5:00, Michael S. Tsirkin wrote:
> > Some legacy guests just assume features are 0 after reset.
> > We detect that config space is accessed before features are
> > set and set features to 0 automatically.
> > Note: some legacy guests might not even access config space, if this is
> > reported in the field we might need to catch a kick to handle these.
>
>
> I wonder whether it's easier to just support modern device?
>
> Thanks
Well hardware vendors are I think interested in supporting legacy
guests. Limiting vdpa to modern only would make it uncompetitive.
>
> >
> > Signed-off-by: Michael S. Tsirkin
> > ---
> > drivers/vdpa/vdpa.c | 1 +
> > include/linux/vdpa.h | 34 ++
> > 2 files changed, 35 insertions(+)
> >
> > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> > index de211ef3738c..7105265e4793 100644
> > --- a/drivers/vdpa/vdpa.c
> > +++ b/drivers/vdpa/vdpa.c
> > @@ -96,6 +96,7 @@ struct vdpa_device *__vdpa_alloc_device(struct device
> > *parent,
> > vdev->dev.release = vdpa_release_dev;
> > vdev->index = err;
> > vdev->config = config;
> > + vdev->features_valid = false;
> > err = dev_set_name(>dev, "vdpa%u", vdev->index);
> > if (err)
> > diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> > index 239db794357c..29b8296f1414 100644
> > --- a/include/linux/vdpa.h
> > +++ b/include/linux/vdpa.h
> > @@ -33,12 +33,14 @@ struct vdpa_notification_area {
> >* @dma_dev: the actual device that is performing DMA
> >* @config: the configuration ops for this device.
> >* @index: device index
> > + * @features_valid: were features initialized? for legacy guests
> >*/
> > struct vdpa_device {
> > struct device dev;
> > struct device *dma_dev;
> > const struct vdpa_config_ops *config;
> > unsigned int index;
> > + bool features_valid;
> > };
> > /**
> > @@ -266,4 +268,36 @@ static inline struct device *vdpa_get_dma_dev(struct
> > vdpa_device *vdev)
> > {
> > return vdev->dma_dev;
> > }
> > +
> > +static inline void vdpa_reset(struct vdpa_device *vdev)
> > +{
> > +const struct vdpa_config_ops *ops = vdev->config;
> > +
> > + vdev->features_valid = false;
> > +ops->set_status(vdev, 0);
> > +}
> > +
> > +static inline int vdpa_set_features(struct vdpa_device *vdev, u64 features)
> > +{
> > +const struct vdpa_config_ops *ops = vdev->config;
> > +
> > + vdev->features_valid = true;
> > +return ops->set_features(vdev, features);
> > +}
> > +
> > +
> > +static inline void vdpa_get_config(struct vdpa_device *vdev, unsigned
> > offset,
> > + void *buf, unsigned int len)
> > +{
> > +const struct vdpa_config_ops *ops = vdev->config;
> > +
> > + /*
> > +* Config accesses aren't supposed to trigger before features are set.
> > +* If it does happen we assume a legacy guest.
> > +*/
> > + if (!vdev->features_valid)
> > + vdpa_set_features(vdev, 0);
> > + ops->get_config(vdev, offset, buf, len);
> > +}
> > +
> > #endif /* _LINUX_VDPA_H */
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 19/24] vdpa: make sure set_features in invoked for legacy
On 2020/8/4 上午5:00, Michael S. Tsirkin wrote:
Some legacy guests just assume features are 0 after reset.
We detect that config space is accessed before features are
set and set features to 0 automatically.
Note: some legacy guests might not even access config space, if this is
reported in the field we might need to catch a kick to handle these.
I wonder whether it's easier to just support modern device?
Thanks
Signed-off-by: Michael S. Tsirkin
---
drivers/vdpa/vdpa.c | 1 +
include/linux/vdpa.h | 34 ++
2 files changed, 35 insertions(+)
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index de211ef3738c..7105265e4793 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -96,6 +96,7 @@ struct vdpa_device *__vdpa_alloc_device(struct device *parent,
vdev->dev.release = vdpa_release_dev;
vdev->index = err;
vdev->config = config;
+ vdev->features_valid = false;
err = dev_set_name(>dev, "vdpa%u", vdev->index);
if (err)
diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index 239db794357c..29b8296f1414 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h
@@ -33,12 +33,14 @@ struct vdpa_notification_area {
* @dma_dev: the actual device that is performing DMA
* @config: the configuration ops for this device.
* @index: device index
+ * @features_valid: were features initialized? for legacy guests
*/
struct vdpa_device {
struct device dev;
struct device *dma_dev;
const struct vdpa_config_ops *config;
unsigned int index;
+ bool features_valid;
};
/**
@@ -266,4 +268,36 @@ static inline struct device *vdpa_get_dma_dev(struct
vdpa_device *vdev)
{
return vdev->dma_dev;
}
+
+static inline void vdpa_reset(struct vdpa_device *vdev)
+{
+const struct vdpa_config_ops *ops = vdev->config;
+
+ vdev->features_valid = false;
+ops->set_status(vdev, 0);
+}
+
+static inline int vdpa_set_features(struct vdpa_device *vdev, u64 features)
+{
+const struct vdpa_config_ops *ops = vdev->config;
+
+ vdev->features_valid = true;
+return ops->set_features(vdev, features);
+}
+
+
+static inline void vdpa_get_config(struct vdpa_device *vdev, unsigned offset,
+ void *buf, unsigned int len)
+{
+const struct vdpa_config_ops *ops = vdev->config;
+
+ /*
+* Config accesses aren't supposed to trigger before features are set.
+* If it does happen we assume a legacy guest.
+*/
+ if (!vdev->features_valid)
+ vdpa_set_features(vdev, 0);
+ ops->get_config(vdev, offset, buf, len);
+}
+
#endif /* _LINUX_VDPA_H */
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
