Re: [PATCH v2 22/24] vdpa_sim: fix endian-ness of config space
On 2020/8/5 下午8:06, Michael S. Tsirkin wrote:
On Wed, Aug 05, 2020 at 02:21:07PM +0800, Jason Wang wrote:
On 2020/8/4 上午5:00, Michael S. Tsirkin wrote:
VDPA sim accesses config space as native endian - this is
wrong since it's a modern device and actually uses LE.
It only supports modern guests so we could punt and
just force LE, but let's use the full virtio APIs since people
tend to copy/paste code, and this is not data path anyway.
Signed-off-by: Michael S. Tsirkin
---
drivers/vdpa/vdpa_sim/vdpa_sim.c | 31 ++-
1 file changed, 26 insertions(+), 5 deletions(-)
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
index a9bc5e0fb353..fa05e065ff69 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -72,6 +73,23 @@ struct vdpasim {
u64 features;
};
+/* TODO: cross-endian support */
+static inline bool vdpasim_is_little_endian(struct vdpasim *vdpasim)
+{
+ return virtio_legacy_is_little_endian() ||
+ (vdpasim->features & (1ULL << VIRTIO_F_VERSION_1));
+}
+
+static inline u16 vdpasim16_to_cpu(struct vdpasim *vdpasim, __virtio16 val)
+{
+ return __virtio16_to_cpu(vdpasim_is_little_endian(vdpasim), val);
+}
+
+static inline __virtio16 cpu_to_vdpasim16(struct vdpasim *vdpasim, u16 val)
+{
+ return __cpu_to_virtio16(vdpasim_is_little_endian(vdpasim), val);
+}
+
static struct vdpasim *vdpasim_dev;
static struct vdpasim *vdpa_to_sim(struct vdpa_device *vdpa)
@@ -306,7 +324,6 @@ static const struct vdpa_config_ops vdpasim_net_config_ops;
static struct vdpasim *vdpasim_create(void)
{
- struct virtio_net_config *config;
struct vdpasim *vdpasim;
struct device *dev;
int ret = -ENOMEM;
@@ -331,10 +348,7 @@ static struct vdpasim *vdpasim_create(void)
if (!vdpasim->buffer)
goto err_iommu;
- config = &vdpasim->config;
- config->mtu = 1500;
- config->status = VIRTIO_NET_S_LINK_UP;
- eth_random_addr(config->mac);
+ eth_random_addr(vdpasim->config.mac);
vringh_set_iotlb(&vdpasim->vqs[0].vring, vdpasim->iommu);
vringh_set_iotlb(&vdpasim->vqs[1].vring, vdpasim->iommu);
@@ -448,6 +462,7 @@ static u64 vdpasim_get_features(struct vdpa_device *vdpa)
static int vdpasim_set_features(struct vdpa_device *vdpa, u64 features)
{
struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
+ struct virtio_net_config *config = &vdpasim->config;
/* DMA mapping must be done by driver */
if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
@@ -455,6 +470,12 @@ static int vdpasim_set_features(struct vdpa_device *vdpa,
u64 features)
vdpasim->features = features & vdpasim_features;
+ /* We only know whether guest is using the legacy interface here, so
+* that's the earliest we can set config fields.
+*/
We check whether or not ACCESS_PLATFORM is set before which is probably a
hint that only modern device is supported. So I wonder just force LE and
fail if VERSION_1 is not set is better?
Thanks
So how about I add a comment along the lines of
/*
* vdpasim ATM requires VIRTIO_F_ACCESS_PLATFORM, so we don't need to
* support legacy guests. Keep transitional device code around for
* the benefit of people who might copy-and-paste this into transitional
* device code.
*/
That's fine.
Thanks
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 22/24] vdpa_sim: fix endian-ness of config space
On Wed, Aug 05, 2020 at 02:21:07PM +0800, Jason Wang wrote:
>
> On 2020/8/4 上午5:00, Michael S. Tsirkin wrote:
> > VDPA sim accesses config space as native endian - this is
> > wrong since it's a modern device and actually uses LE.
> >
> > It only supports modern guests so we could punt and
> > just force LE, but let's use the full virtio APIs since people
> > tend to copy/paste code, and this is not data path anyway.
> >
> > Signed-off-by: Michael S. Tsirkin
> > ---
> > drivers/vdpa/vdpa_sim/vdpa_sim.c | 31 ++-
> > 1 file changed, 26 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > index a9bc5e0fb353..fa05e065ff69 100644
> > --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > @@ -24,6 +24,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -72,6 +73,23 @@ struct vdpasim {
> > u64 features;
> > };
> > +/* TODO: cross-endian support */
> > +static inline bool vdpasim_is_little_endian(struct vdpasim *vdpasim)
> > +{
> > + return virtio_legacy_is_little_endian() ||
> > + (vdpasim->features & (1ULL << VIRTIO_F_VERSION_1));
> > +}
> > +
> > +static inline u16 vdpasim16_to_cpu(struct vdpasim *vdpasim, __virtio16 val)
> > +{
> > + return __virtio16_to_cpu(vdpasim_is_little_endian(vdpasim), val);
> > +}
> > +
> > +static inline __virtio16 cpu_to_vdpasim16(struct vdpasim *vdpasim, u16 val)
> > +{
> > + return __cpu_to_virtio16(vdpasim_is_little_endian(vdpasim), val);
> > +}
> > +
> > static struct vdpasim *vdpasim_dev;
> > static struct vdpasim *vdpa_to_sim(struct vdpa_device *vdpa)
> > @@ -306,7 +324,6 @@ static const struct vdpa_config_ops
> > vdpasim_net_config_ops;
> > static struct vdpasim *vdpasim_create(void)
> > {
> > - struct virtio_net_config *config;
> > struct vdpasim *vdpasim;
> > struct device *dev;
> > int ret = -ENOMEM;
> > @@ -331,10 +348,7 @@ static struct vdpasim *vdpasim_create(void)
> > if (!vdpasim->buffer)
> > goto err_iommu;
> > - config = &vdpasim->config;
> > - config->mtu = 1500;
> > - config->status = VIRTIO_NET_S_LINK_UP;
> > - eth_random_addr(config->mac);
> > + eth_random_addr(vdpasim->config.mac);
> > vringh_set_iotlb(&vdpasim->vqs[0].vring, vdpasim->iommu);
> > vringh_set_iotlb(&vdpasim->vqs[1].vring, vdpasim->iommu);
> > @@ -448,6 +462,7 @@ static u64 vdpasim_get_features(struct vdpa_device
> > *vdpa)
> > static int vdpasim_set_features(struct vdpa_device *vdpa, u64 features)
> > {
> > struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
> > + struct virtio_net_config *config = &vdpasim->config;
> > /* DMA mapping must be done by driver */
> > if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
> > @@ -455,6 +470,12 @@ static int vdpasim_set_features(struct vdpa_device
> > *vdpa, u64 features)
> > vdpasim->features = features & vdpasim_features;
> > + /* We only know whether guest is using the legacy interface here, so
> > +* that's the earliest we can set config fields.
> > +*/
>
>
> We check whether or not ACCESS_PLATFORM is set before which is probably a
> hint that only modern device is supported. So I wonder just force LE and
> fail if VERSION_1 is not set is better?
>
> Thanks
So how about I add a comment along the lines of
/*
* vdpasim ATM requires VIRTIO_F_ACCESS_PLATFORM, so we don't need to
* support legacy guests. Keep transitional device code around for
* the benefit of people who might copy-and-paste this into transitional
* device code.
*/
>
> > +
> > + config->mtu = cpu_to_vdpasim16(vdpasim, 1500);
> > + config->status = cpu_to_vdpasim16(vdpasim, VIRTIO_NET_S_LINK_UP);
> > return 0;
> > }
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 22/24] vdpa_sim: fix endian-ness of config space
On Wed, Aug 05, 2020 at 02:21:07PM +0800, Jason Wang wrote:
>
> On 2020/8/4 上午5:00, Michael S. Tsirkin wrote:
> > VDPA sim accesses config space as native endian - this is
> > wrong since it's a modern device and actually uses LE.
> >
> > It only supports modern guests so we could punt and
> > just force LE, but let's use the full virtio APIs since people
> > tend to copy/paste code, and this is not data path anyway.
> >
> > Signed-off-by: Michael S. Tsirkin
> > ---
> > drivers/vdpa/vdpa_sim/vdpa_sim.c | 31 ++-
> > 1 file changed, 26 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > index a9bc5e0fb353..fa05e065ff69 100644
> > --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > @@ -24,6 +24,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -72,6 +73,23 @@ struct vdpasim {
> > u64 features;
> > };
> > +/* TODO: cross-endian support */
> > +static inline bool vdpasim_is_little_endian(struct vdpasim *vdpasim)
> > +{
> > + return virtio_legacy_is_little_endian() ||
> > + (vdpasim->features & (1ULL << VIRTIO_F_VERSION_1));
> > +}
> > +
> > +static inline u16 vdpasim16_to_cpu(struct vdpasim *vdpasim, __virtio16 val)
> > +{
> > + return __virtio16_to_cpu(vdpasim_is_little_endian(vdpasim), val);
> > +}
> > +
> > +static inline __virtio16 cpu_to_vdpasim16(struct vdpasim *vdpasim, u16 val)
> > +{
> > + return __cpu_to_virtio16(vdpasim_is_little_endian(vdpasim), val);
> > +}
> > +
> > static struct vdpasim *vdpasim_dev;
> > static struct vdpasim *vdpa_to_sim(struct vdpa_device *vdpa)
> > @@ -306,7 +324,6 @@ static const struct vdpa_config_ops
> > vdpasim_net_config_ops;
> > static struct vdpasim *vdpasim_create(void)
> > {
> > - struct virtio_net_config *config;
> > struct vdpasim *vdpasim;
> > struct device *dev;
> > int ret = -ENOMEM;
> > @@ -331,10 +348,7 @@ static struct vdpasim *vdpasim_create(void)
> > if (!vdpasim->buffer)
> > goto err_iommu;
> > - config = &vdpasim->config;
> > - config->mtu = 1500;
> > - config->status = VIRTIO_NET_S_LINK_UP;
> > - eth_random_addr(config->mac);
> > + eth_random_addr(vdpasim->config.mac);
> > vringh_set_iotlb(&vdpasim->vqs[0].vring, vdpasim->iommu);
> > vringh_set_iotlb(&vdpasim->vqs[1].vring, vdpasim->iommu);
> > @@ -448,6 +462,7 @@ static u64 vdpasim_get_features(struct vdpa_device
> > *vdpa)
> > static int vdpasim_set_features(struct vdpa_device *vdpa, u64 features)
> > {
> > struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
> > + struct virtio_net_config *config = &vdpasim->config;
> > /* DMA mapping must be done by driver */
> > if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
> > @@ -455,6 +470,12 @@ static int vdpasim_set_features(struct vdpa_device
> > *vdpa, u64 features)
> > vdpasim->features = features & vdpasim_features;
> > + /* We only know whether guest is using the legacy interface here, so
> > +* that's the earliest we can set config fields.
> > +*/
>
>
> We check whether or not ACCESS_PLATFORM is set before which is probably a
> hint that only modern device is supported. So I wonder just force LE and
> fail if VERSION_1 is not set is better?
>
> Thanks
>
Hmm good point. As usual legacy does not have a clean way to fail though,
are we sure we don't ever want to support legacy guests?
> > +
> > + config->mtu = cpu_to_vdpasim16(vdpasim, 1500);
> > + config->status = cpu_to_vdpasim16(vdpasim, VIRTIO_NET_S_LINK_UP);
> > return 0;
> > }
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 22/24] vdpa_sim: fix endian-ness of config space
On 2020/8/4 上午5:00, Michael S. Tsirkin wrote:
VDPA sim accesses config space as native endian - this is
wrong since it's a modern device and actually uses LE.
It only supports modern guests so we could punt and
just force LE, but let's use the full virtio APIs since people
tend to copy/paste code, and this is not data path anyway.
Signed-off-by: Michael S. Tsirkin
---
drivers/vdpa/vdpa_sim/vdpa_sim.c | 31 ++-
1 file changed, 26 insertions(+), 5 deletions(-)
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
index a9bc5e0fb353..fa05e065ff69 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -72,6 +73,23 @@ struct vdpasim {
u64 features;
};
+/* TODO: cross-endian support */
+static inline bool vdpasim_is_little_endian(struct vdpasim *vdpasim)
+{
+ return virtio_legacy_is_little_endian() ||
+ (vdpasim->features & (1ULL << VIRTIO_F_VERSION_1));
+}
+
+static inline u16 vdpasim16_to_cpu(struct vdpasim *vdpasim, __virtio16 val)
+{
+ return __virtio16_to_cpu(vdpasim_is_little_endian(vdpasim), val);
+}
+
+static inline __virtio16 cpu_to_vdpasim16(struct vdpasim *vdpasim, u16 val)
+{
+ return __cpu_to_virtio16(vdpasim_is_little_endian(vdpasim), val);
+}
+
static struct vdpasim *vdpasim_dev;
static struct vdpasim *vdpa_to_sim(struct vdpa_device *vdpa)
@@ -306,7 +324,6 @@ static const struct vdpa_config_ops vdpasim_net_config_ops;
static struct vdpasim *vdpasim_create(void)
{
- struct virtio_net_config *config;
struct vdpasim *vdpasim;
struct device *dev;
int ret = -ENOMEM;
@@ -331,10 +348,7 @@ static struct vdpasim *vdpasim_create(void)
if (!vdpasim->buffer)
goto err_iommu;
- config = &vdpasim->config;
- config->mtu = 1500;
- config->status = VIRTIO_NET_S_LINK_UP;
- eth_random_addr(config->mac);
+ eth_random_addr(vdpasim->config.mac);
vringh_set_iotlb(&vdpasim->vqs[0].vring, vdpasim->iommu);
vringh_set_iotlb(&vdpasim->vqs[1].vring, vdpasim->iommu);
@@ -448,6 +462,7 @@ static u64 vdpasim_get_features(struct vdpa_device *vdpa)
static int vdpasim_set_features(struct vdpa_device *vdpa, u64 features)
{
struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
+ struct virtio_net_config *config = &vdpasim->config;
/* DMA mapping must be done by driver */
if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
@@ -455,6 +470,12 @@ static int vdpasim_set_features(struct vdpa_device *vdpa,
u64 features)
vdpasim->features = features & vdpasim_features;
+ /* We only know whether guest is using the legacy interface here, so
+* that's the earliest we can set config fields.
+*/
We check whether or not ACCESS_PLATFORM is set before which is probably
a hint that only modern device is supported. So I wonder just force LE
and fail if VERSION_1 is not set is better?
Thanks
+
+ config->mtu = cpu_to_vdpasim16(vdpasim, 1500);
+ config->status = cpu_to_vdpasim16(vdpasim, VIRTIO_NET_S_LINK_UP);
return 0;
}
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 22/24] vdpa_sim: fix endian-ness of config space
VDPA sim accesses config space as native endian - this is
wrong since it's a modern device and actually uses LE.
It only supports modern guests so we could punt and
just force LE, but let's use the full virtio APIs since people
tend to copy/paste code, and this is not data path anyway.
Signed-off-by: Michael S. Tsirkin
---
drivers/vdpa/vdpa_sim/vdpa_sim.c | 31 ++-
1 file changed, 26 insertions(+), 5 deletions(-)
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
index a9bc5e0fb353..fa05e065ff69 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -72,6 +73,23 @@ struct vdpasim {
u64 features;
};
+/* TODO: cross-endian support */
+static inline bool vdpasim_is_little_endian(struct vdpasim *vdpasim)
+{
+ return virtio_legacy_is_little_endian() ||
+ (vdpasim->features & (1ULL << VIRTIO_F_VERSION_1));
+}
+
+static inline u16 vdpasim16_to_cpu(struct vdpasim *vdpasim, __virtio16 val)
+{
+ return __virtio16_to_cpu(vdpasim_is_little_endian(vdpasim), val);
+}
+
+static inline __virtio16 cpu_to_vdpasim16(struct vdpasim *vdpasim, u16 val)
+{
+ return __cpu_to_virtio16(vdpasim_is_little_endian(vdpasim), val);
+}
+
static struct vdpasim *vdpasim_dev;
static struct vdpasim *vdpa_to_sim(struct vdpa_device *vdpa)
@@ -306,7 +324,6 @@ static const struct vdpa_config_ops vdpasim_net_config_ops;
static struct vdpasim *vdpasim_create(void)
{
- struct virtio_net_config *config;
struct vdpasim *vdpasim;
struct device *dev;
int ret = -ENOMEM;
@@ -331,10 +348,7 @@ static struct vdpasim *vdpasim_create(void)
if (!vdpasim->buffer)
goto err_iommu;
- config = &vdpasim->config;
- config->mtu = 1500;
- config->status = VIRTIO_NET_S_LINK_UP;
- eth_random_addr(config->mac);
+ eth_random_addr(vdpasim->config.mac);
vringh_set_iotlb(&vdpasim->vqs[0].vring, vdpasim->iommu);
vringh_set_iotlb(&vdpasim->vqs[1].vring, vdpasim->iommu);
@@ -448,6 +462,7 @@ static u64 vdpasim_get_features(struct vdpa_device *vdpa)
static int vdpasim_set_features(struct vdpa_device *vdpa, u64 features)
{
struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
+ struct virtio_net_config *config = &vdpasim->config;
/* DMA mapping must be done by driver */
if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
@@ -455,6 +470,12 @@ static int vdpasim_set_features(struct vdpa_device *vdpa,
u64 features)
vdpasim->features = features & vdpasim_features;
+ /* We only know whether guest is using the legacy interface here, so
+* that's the earliest we can set config fields.
+*/
+
+ config->mtu = cpu_to_vdpasim16(vdpasim, 1500);
+ config->status = cpu_to_vdpasim16(vdpasim, VIRTIO_NET_S_LINK_UP);
return 0;
}
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
