Re: [PATCH net-next v4 09/10] virtio-net: xsk zero copy xmit implement wakeup and xmit
在 2021/4/15 下午6:27, Xuan Zhuo 写道:
On Wed, 14 Apr 2021 13:46:45 +0800, Jason Wang wrote:
在 2021/4/13 上午11:15, Xuan Zhuo 写道:
This patch implements the core part of xsk zerocopy xmit.
When the user calls sendto to consume the data in the xsk tx queue,
virtnet_xsk_wakeup() will be called.
In wakeup, it will try to send a part of the data directly. There are
two purposes for this realization:
1. Send part of the data quickly to reduce the transmission delay of the
first packet.
2. Trigger tx interrupt, start napi to consume xsk tx data.
All sent xsk packets share the virtio-net header of xsk_hdr. If xsk
needs to support csum and other functions later, consider assigning xsk
hdr separately for each sent packet.
There are now three situations in free_old_xmit(): skb, xdp frame, xsk
desc. Based on the last two bit of ptr returned by virtqueue_get_buf():
00 is skb by default.
01 represents the packet sent by xdp
10 is the packet sent by xsk
If the xmit work of xsk has not been completed, but the ring is full,
napi must first exit and wait for the ring to be available, so
need_wakeup() is set. If free_old_xmit() is called first by start_xmit(),
we can quickly wake up napi to execute xsk xmit task.
When recycling, we need to count the number of bytes sent, so put xsk
desc->len into the ptr pointer. Because ptr does not point to meaningful
objects in xsk.
Signed-off-by: Xuan Zhuo
Reviewed-by: Dust Li
---
drivers/net/virtio_net.c | 296 ++-
1 file changed, 292 insertions(+), 4 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 8242a9e9f17d..c441d6bf1510 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -46,6 +46,11 @@ module_param(napi_tx, bool, 0644);
#define VIRTIO_XDP_REDIR BIT(1)
#define VIRTIO_XDP_FLAG BIT(0)
+#define VIRTIO_XSK_FLAGBIT(1)
+
+#define VIRTIO_XSK_PTR_SHIFT 4
+
+static struct virtio_net_hdr_mrg_rxbuf xsk_hdr;
/* RX packet size EWMA. The average packet size is used to determine the
packet
* buffer size when refilling RX rings. As the entire RX ring may be refilled
@@ -138,6 +143,12 @@ struct send_queue {
struct {
/* xsk pool */
struct xsk_buff_pool __rcu *pool;
+
+ /* save the desc for next xmit, when xmit fail. */
+ struct xdp_desc last_desc;
As replied in the pervious version this looks tricky. I think we need to
make sure to reserve some slots as skb path did.
This looks exactly like what stmmac did which alos shares XDP and skb
for the same ring.
+
+ /* xsk wait for tx inter or softirq */
+ bool need_wakeup;
} xsk;
};
@@ -255,6 +266,15 @@ struct padded_vnet_hdr {
char padding[4];
};
+static int virtnet_xsk_run(struct send_queue *sq, struct xsk_buff_pool *pool,
+ int budget, bool in_napi);
+static void virtnet_xsk_complete(struct send_queue *sq, u32 num);
+
+static bool is_skb_ptr(void *ptr)
+{
+ return !((unsigned long)ptr & (VIRTIO_XDP_FLAG | VIRTIO_XSK_FLAG));
+}
+
static bool is_xdp_frame(void *ptr)
{
return (unsigned long)ptr & VIRTIO_XDP_FLAG;
@@ -265,6 +285,19 @@ static void *xdp_to_ptr(struct xdp_frame *ptr)
return (void *)((unsigned long)ptr | VIRTIO_XDP_FLAG);
}
+static void *xsk_to_ptr(struct xdp_desc *desc)
+{
+ /* save the desc len to ptr */
+ u64 p = desc->len << VIRTIO_XSK_PTR_SHIFT;
+
+ return (void *)((unsigned long)p | VIRTIO_XSK_FLAG);
+}
+
+static void ptr_to_xsk(void *ptr, struct xdp_desc *desc)
+{
+ desc->len = ((unsigned long)ptr) >> VIRTIO_XSK_PTR_SHIFT;
+}
+
static struct xdp_frame *ptr_to_xdp(void *ptr)
{
return (struct xdp_frame *)((unsigned long)ptr & ~VIRTIO_XDP_FLAG);
@@ -273,25 +306,35 @@ static struct xdp_frame *ptr_to_xdp(void *ptr)
static void __free_old_xmit(struct send_queue *sq, bool in_napi,
struct virtnet_sq_stats *stats)
{
+ unsigned int xsknum = 0;
unsigned int len;
void *ptr;
while ((ptr = virtqueue_get_buf(sq->vq, )) != NULL) {
- if (likely(!is_xdp_frame(ptr))) {
+ if (is_skb_ptr(ptr)) {
struct sk_buff *skb = ptr;
pr_debug("Sent skb %p\n", skb);
stats->bytes += skb->len;
napi_consume_skb(skb, in_napi);
- } else {
+ } else if (is_xdp_frame(ptr)) {
struct xdp_frame *frame = ptr_to_xdp(ptr);
stats->bytes += frame->len;
xdp_return_frame(frame);
+ } else {
+ struct xdp_desc desc;
+
+ ptr_to_xsk(ptr, );
+ stats->bytes += desc.len;
+ ++xsknum;
}
Re: [PATCH v6 09/10] vduse: Introduce VDUSE - vDPA Device in Userspace
在 2021/3/31 下午4:05, Xie Yongji 写道: + } + case VDUSE_INJECT_VQ_IRQ: + ret = -EINVAL; + if (arg >= dev->vq_num) + break; + + ret = 0; + queue_work(vduse_irq_wq, >vqs[arg].inject); + break; One additional note: Please use array_index_nospec() for all vqs[idx] access where idx is under the control of userspace to avoid potential spectre exploitation. Thanks ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v6 10/10] Documentation: Add documentation for VDUSE
在 2021/4/16 上午10:58, Yongji Xie 写道:
On Fri, Apr 16, 2021 at 10:20 AM Jason Wang wrote:
在 2021/4/15 下午7:17, Yongji Xie 写道:
On Thu, Apr 15, 2021 at 5:05 PM Jason Wang wrote:
在 2021/4/15 下午4:36, Jason Wang 写道:
Please state this explicitly at the start of the document. Existing
interfaces like FUSE are designed to avoid trusting userspace.
There're some subtle difference here. VDUSE present a device to kernel
which means IOMMU is probably the only thing to prevent a malicous
device.
Therefore
people might think the same is the case here. It's critical that people
are aware of this before deploying VDUSE with virtio-vdpa.
We should probably pause here and think about whether it's possible to
avoid trusting userspace. Even if it takes some effort and costs some
performance it would probably be worthwhile.
Since the bounce buffer is used the only attack surface is the
coherent area, if we want to enforce stronger isolation we need to use
shadow virtqueue (which is proposed in earlier version by me) in this
case. But I'm not sure it's worth to do that.
So this reminds me the discussion in the end of last year. We need to
make sure we don't suffer from the same issues for VDUSE at least
https://yhbt.net/lore/all/c3629a27-3590-1d9f-211b-c0b7be152...@redhat.com/T/#mc6b6e2343cbeffca68ca7a97e0f473aaa871c95b
Or we can solve it at virtio level, e.g remember the dma address instead
of depending on the addr in the descriptor ring
I might miss something. But VDUSE has recorded the dma address during
dma mapping, so we would not do bouncing if the addr/length is invalid
during dma unmapping. Is it enough?
E.g malicous device write a buggy dma address in the descriptor ring, so
we had:
vring_unmap_one_split(desc->addr, desc->len)
dma_unmap_single()
vduse_dev_unmap_page()
vduse_domain_bounce()
And in vduse_domain_bounce() we had:
while (size) {
map = >bounce_maps[iova >> PAGE_SHIFT];
offset = offset_in_page(iova);
sz = min_t(size_t, PAGE_SIZE - offset, size);
This means we trust the iova which is dangerous and exacly the issue
mentioned in the above link.
From VDUSE level need to make sure iova is legal.
I think we already do that in vduse_domain_bounce():
while (size) {
map = >bounce_maps[iova >> PAGE_SHIFT];
if (WARN_ON(!map->bounce_page ||
map->orig_phys == INVALID_PHYS_ADDR))
return;
So you don't check whether iova is legal before using it, so it's at
least a possible out of bound access of the bounce_maps[] isn't it? (e.g
what happens if iova is ULLONG_MAX).
From virtio level, we should not truse desc->addr.
We would not touch desc->addr after vring_unmap_one_split(). So I'm
not sure what we need to do at the virtio level.
I think the point is to record the dma addres/len somewhere instead of
reading them from descriptor ring.
Thanks
Thanks,
Yongji
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 4/4] vhost poll: fix coding style
在 2021/4/16 上午1:27, Mike Christie 写道:
We use like 3 coding styles in this struct. Switch to just tabs.
Signed-off-by: Mike Christie
Reviewed-by: Chaitanya Kulkarni
Reviewed-by: Stefan Hajnoczi
---
Acked-by: Jason Wang
drivers/vhost/vhost.h | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 1ba8e814989d..575c8180caad 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -28,12 +28,12 @@ struct vhost_work {
/* Poll a file (eventfd or socket) */
/* Note: there's nothing vhost specific about this structure. */
struct vhost_poll {
- poll_tabletable;
- wait_queue_head_t*wqh;
- wait_queue_entry_t wait;
- struct vhost_work work;
- __poll_t mask;
- struct vhost_dev *dev;
+ poll_table table;
+ wait_queue_head_t *wqh;
+ wait_queue_entry_t wait;
+ struct vhost_work work;
+ __poll_tmask;
+ struct vhost_dev*dev;
};
void vhost_work_init(struct vhost_work *work, vhost_work_fn_t fn);
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 2/3] vDPA/ifcvf: enable Intel C5000X-PL virtio-block for vDPA
On 4/15/2021 9:41 PM, Stefano Garzarella wrote:
On Thu, Apr 15, 2021 at 05:53:35PM +0800, Zhu Lingshan wrote:
This commit enabled Intel FPGA SmartNIC C5000X-PL virtio-block
for vDPA.
Signed-off-by: Zhu Lingshan
---
drivers/vdpa/ifcvf/ifcvf_base.h | 8 +++-
drivers/vdpa/ifcvf/ifcvf_main.c | 10 +-
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h
b/drivers/vdpa/ifcvf/ifcvf_base.h
index 1c04cd256fa7..0111bfdeb342 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.h
+++ b/drivers/vdpa/ifcvf/ifcvf_base.h
@@ -15,6 +15,7 @@
#include
#include
#include
+#include
#include
#include
@@ -28,7 +29,12 @@
#define C5000X_PL_SUBSYS_VENDOR_ID 0x8086
#define C5000X_PL_SUBSYS_DEVICE_ID 0x0001
-#define IFCVF_SUPPORTED_FEATURES \
+#define C5000X_PL_BLK_VENDOR_ID 0x1AF4
+#define C5000X_PL_BLK_DEVICE_ID 0x1001
+#define C5000X_PL_BLK_SUBSYS_VENDOR_ID 0x8086
+#define C5000X_PL_BLK_SUBSYS_DEVICE_ID 0x0002
+
+#define IFCVF_NET_SUPPORTED_FEATURES \
((1ULL << VIRTIO_NET_F_MAC) | \
(1ULL << VIRTIO_F_ANY_LAYOUT) | \
(1ULL << VIRTIO_F_VERSION_1) | \
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
b/drivers/vdpa/ifcvf/ifcvf_main.c
index 469a9b5737b7..cea1313b1a3f 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -171,7 +171,11 @@ static u64 ifcvf_vdpa_get_features(struct
vdpa_device *vdpa_dev)
struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
u64 features;
- features = ifcvf_get_features(vf) & IFCVF_SUPPORTED_FEATURES;
+ if (vf->dev_type == VIRTIO_ID_NET)
+ features = ifcvf_get_features(vf) &
IFCVF_NET_SUPPORTED_FEATURES;
+
+ if (vf->dev_type == VIRTIO_ID_BLOCK)
+ features = ifcvf_get_features(vf);
Should we put a warning here too otherwise feature could be seen
unassigned?
Thanks, it will be a switch code block too.
Thanks,
Stefano
return features;
}
@@ -517,6 +521,10 @@ static struct pci_device_id ifcvf_pci_ids[] = {
C5000X_PL_DEVICE_ID,
C5000X_PL_SUBSYS_VENDOR_ID,
C5000X_PL_SUBSYS_DEVICE_ID) },
+ { PCI_DEVICE_SUB(C5000X_PL_BLK_VENDOR_ID,
+ C5000X_PL_BLK_DEVICE_ID,
+ C5000X_PL_BLK_SUBSYS_VENDOR_ID,
+ C5000X_PL_BLK_SUBSYS_DEVICE_ID) },
{ 0 },
};
--
2.27.0
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 3/3] vDPA/ifcvf: get_config_size should return dev specific config size
On 4/15/2021 9:48 PM, Stefano Garzarella wrote:
On Thu, Apr 15, 2021 at 05:53:36PM +0800, Zhu Lingshan wrote:
get_config_size() should return the size based on the decected
device type.
Signed-off-by: Zhu Lingshan
---
drivers/vdpa/ifcvf/ifcvf_main.c | 18 +-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
b/drivers/vdpa/ifcvf/ifcvf_main.c
index cea1313b1a3f..6844c49fe1de 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -347,7 +347,23 @@ static u32 ifcvf_vdpa_get_vq_align(struct
vdpa_device *vdpa_dev)
static size_t ifcvf_vdpa_get_config_size(struct vdpa_device *vdpa_dev)
{
- return sizeof(struct virtio_net_config);
+ struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev);
+ struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
+ struct pci_dev *pdev = adapter->pdev;
+ size_t size;
+
+ if (vf->dev_type == VIRTIO_ID_NET)
+ size = sizeof(struct virtio_net_config);
+
+ else if (vf->dev_type == VIRTIO_ID_BLOCK)
+ size = sizeof(struct virtio_blk_config);
+
+ else {
+ size = 0;
+ IFCVF_ERR(pdev, "VIRTIO ID %u not supported\n", vf->dev_type);
+ }
I slightly prefer the switch, but I don't have a strong opinion.
However, if we want to use if/else, we should follow
`Documentation/process/coding-style.rst` line 166:
Note that the closing brace is empty on a line of its own,
**except** in
the cases where it is followed by a continuation of the same
statement,
ie a ``while`` in a do-statement or an ``else`` in an if-statement,
like
also `scripts/checkpatch.pl --strict` complains:
CHECK: braces {} should be used on all arms of this statement
#209: FILE: drivers/vdpa/ifcvf/ifcvf_main.c:355:
+ if (vf->dev_type == VIRTIO_ID_NET)
[...]
+ else if (vf->dev_type == VIRTIO_ID_BLOCK)
[...]
+ else {
[...]
CHECK: Unbalanced braces around else statement
#215: FILE: drivers/vdpa/ifcvf/ifcvf_main.c:361:
+ else {
Thanks Stefano, the reason is we only have one line code after if, so
looks like {} is unnecessary, I agree switch can clear up
code style confusions. I will add this in v3.
Thanks!
Thanks,
Stefano
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/4] vhost-scsi: remove extra flushes
在 2021/4/16 上午1:27, Mike Christie 写道:
The vhost work flush function was flushing the entire work queue, so
there is no need for the double vhost_work_dev_flush calls in
vhost_scsi_flush.
And we do not need to call vhost_poll_flush for each poller because
that call also ends up flushing the same work queue thread the
vhost_work_dev_flush call flushed.
Signed-off-by: Mike Christie
Reviewed-by: Stefan Hajnoczi
---
Acked-by: Jason Wang
drivers/vhost/scsi.c | 8
1 file changed, 8 deletions(-)
diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 0fd596da1834..b3e6fe9b1767 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -1430,11 +1430,6 @@ static void vhost_scsi_handle_kick(struct vhost_work
*work)
vhost_scsi_handle_vq(vs, vq);
}
-static void vhost_scsi_flush_vq(struct vhost_scsi *vs, int index)
-{
- vhost_poll_flush(>vqs[index].vq.poll);
-}
-
/* Callers must hold dev mutex */
static void vhost_scsi_flush(struct vhost_scsi *vs)
{
@@ -1453,9 +1448,6 @@ static void vhost_scsi_flush(struct vhost_scsi *vs)
kref_put(_inflight[i]->kref, vhost_scsi_done_inflight);
/* Flush both the vhost poll and vhost work */
- for (i = 0; i < VHOST_SCSI_MAX_VQ; i++)
- vhost_scsi_flush_vq(vs, i);
- vhost_work_dev_flush(>dev);
vhost_work_dev_flush(>dev);
/* Wait for all reqs issued before the flush to be finished */
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v6 10/10] Documentation: Add documentation for VDUSE
在 2021/4/15 下午10:38, Stefan Hajnoczi 写道:
On Thu, Apr 15, 2021 at 04:36:35PM +0800, Jason Wang wrote:
在 2021/4/15 下午3:19, Stefan Hajnoczi 写道:
On Thu, Apr 15, 2021 at 01:38:37PM +0800, Yongji Xie wrote:
On Wed, Apr 14, 2021 at 10:15 PM Stefan Hajnoczi wrote:
On Wed, Mar 31, 2021 at 04:05:19PM +0800, Xie Yongji wrote:
VDUSE (vDPA Device in Userspace) is a framework to support
implementing software-emulated vDPA devices in userspace. This
document is intended to clarify the VDUSE design and usage.
Signed-off-by: Xie Yongji
---
Documentation/userspace-api/index.rst | 1 +
Documentation/userspace-api/vduse.rst | 212
++
2 files changed, 213 insertions(+)
create mode 100644 Documentation/userspace-api/vduse.rst
Just looking over the documentation briefly (I haven't studied the code
yet)...
Thank you!
+How VDUSE works
+
+Each userspace vDPA device is created by the VDUSE_CREATE_DEV ioctl on
+the character device (/dev/vduse/control). Then a device file with the
+specified name (/dev/vduse/$NAME) will appear, which can be used to
+implement the userspace vDPA device's control path and data path.
These steps are taken after sending the VDPA_CMD_DEV_NEW netlink
message? (Please consider reordering the documentation to make it clear
what the sequence of steps are.)
No, VDUSE devices should be created before sending the
VDPA_CMD_DEV_NEW netlink messages which might produce I/Os to VDUSE.
I see. Please include an overview of the steps before going into detail.
Something like:
VDUSE devices are started as follows:
1. Create a new VDUSE instance with ioctl(VDUSE_CREATE_DEV) on
/dev/vduse/control.
2. Begin processing VDUSE messages from /dev/vduse/$NAME. The first
messages will arrive while attaching the VDUSE instance to vDPA.
3. Send the VDPA_CMD_DEV_NEW netlink message to attach the VDUSE
instance to vDPA.
VDUSE devices are stopped as follows:
...
+ static int netlink_add_vduse(const char *name, int device_id)
+ {
+ struct nl_sock *nlsock;
+ struct nl_msg *msg;
+ int famid;
+
+ nlsock = nl_socket_alloc();
+ if (!nlsock)
+ return -ENOMEM;
+
+ if (genl_connect(nlsock))
+ goto free_sock;
+
+ famid = genl_ctrl_resolve(nlsock, VDPA_GENL_NAME);
+ if (famid < 0)
+ goto close_sock;
+
+ msg = nlmsg_alloc();
+ if (!msg)
+ goto close_sock;
+
+ if (!genlmsg_put(msg, NL_AUTO_PORT, NL_AUTO_SEQ, famid, 0, 0,
+ VDPA_CMD_DEV_NEW, 0))
+ goto nla_put_failure;
+
+ NLA_PUT_STRING(msg, VDPA_ATTR_DEV_NAME, name);
+ NLA_PUT_STRING(msg, VDPA_ATTR_MGMTDEV_DEV_NAME, "vduse");
+ NLA_PUT_U32(msg, VDPA_ATTR_DEV_ID, device_id);
What are the permission/capability requirements for VDUSE?
Now I think we need privileged permission (root user). Because
userspace daemon is able to access avail vring, used vring, descriptor
table in kernel driver directly.
Please state this explicitly at the start of the document. Existing
interfaces like FUSE are designed to avoid trusting userspace.
There're some subtle difference here. VDUSE present a device to kernel which
means IOMMU is probably the only thing to prevent a malicous device.
Therefore
people might think the same is the case here. It's critical that people
are aware of this before deploying VDUSE with virtio-vdpa.
We should probably pause here and think about whether it's possible to
avoid trusting userspace. Even if it takes some effort and costs some
performance it would probably be worthwhile.
Since the bounce buffer is used the only attack surface is the coherent
area, if we want to enforce stronger isolation we need to use shadow
virtqueue (which is proposed in earlier version by me) in this case. But I'm
not sure it's worth to do that.
The security situation needs to be clear before merging this feature.
+1
I think the IOMMU and vring can be made secure. What is more concerning
is the kernel code that runs on top: VIRTIO device drivers, network
stack, file systems, etc. They trust devices to an extent.
Since virtio-vdpa is a big reason for doing VDUSE in the first place I
don't think it makes sense to disable virtio-vdpa with VDUSE. A solution
is needed.
Yes, so the case of VDUSE is something similar to the case of e.g SEV.
Both cases won't trust device and use some kind of software IOTLB.
That means we need to protect at both IOTLB and virtio drivers.
Let me post patches for virtio first.
I'm going to be offline for a week and don't want to be a bottleneck.
I'll catch up when I'm back.
Thanks a lot for comments and I think we had sufficent time to make
VDUSE safe before merging.
Stefan
___
Re: [PATCH v6 10/10] Documentation: Add documentation for VDUSE
在 2021/4/15 下午7:17, Yongji Xie 写道:
On Thu, Apr 15, 2021 at 5:05 PM Jason Wang wrote:
在 2021/4/15 下午4:36, Jason Wang 写道:
Please state this explicitly at the start of the document. Existing
interfaces like FUSE are designed to avoid trusting userspace.
There're some subtle difference here. VDUSE present a device to kernel
which means IOMMU is probably the only thing to prevent a malicous
device.
Therefore
people might think the same is the case here. It's critical that people
are aware of this before deploying VDUSE with virtio-vdpa.
We should probably pause here and think about whether it's possible to
avoid trusting userspace. Even if it takes some effort and costs some
performance it would probably be worthwhile.
Since the bounce buffer is used the only attack surface is the
coherent area, if we want to enforce stronger isolation we need to use
shadow virtqueue (which is proposed in earlier version by me) in this
case. But I'm not sure it's worth to do that.
So this reminds me the discussion in the end of last year. We need to
make sure we don't suffer from the same issues for VDUSE at least
https://yhbt.net/lore/all/c3629a27-3590-1d9f-211b-c0b7be152...@redhat.com/T/#mc6b6e2343cbeffca68ca7a97e0f473aaa871c95b
Or we can solve it at virtio level, e.g remember the dma address instead
of depending on the addr in the descriptor ring
I might miss something. But VDUSE has recorded the dma address during
dma mapping, so we would not do bouncing if the addr/length is invalid
during dma unmapping. Is it enough?
E.g malicous device write a buggy dma address in the descriptor ring, so
we had:
vring_unmap_one_split(desc->addr, desc->len)
dma_unmap_single()
vduse_dev_unmap_page()
vduse_domain_bounce()
And in vduse_domain_bounce() we had:
while (size) {
map = >bounce_maps[iova >> PAGE_SHIFT];
offset = offset_in_page(iova);
sz = min_t(size_t, PAGE_SIZE - offset, size);
This means we trust the iova which is dangerous and exacly the issue
mentioned in the above link.
From VDUSE level need to make sure iova is legal.
From virtio level, we should not truse desc->addr.
Thanks
Thanks,
Yongji
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 04/10] drm/aperture: Add infrastructure for aperture ownership
Hi
Am 15.04.21 um 14:57 schrieb Daniel Vetter:
On Thu, Apr 15, 2021 at 08:56:20AM +0200, Thomas Zimmermann wrote:
Hi
Am 09.04.21 um 11:22 schrieb Daniel Vetter:
Is it that easy? simepldrm's detach function has code to synchronize
with
concurrent hotplug removals. If we can use drm_dev_unplug() for everything,
I'm all for it.
Uh, I should have looked at the code instead of just asking silly
questions :-)
Now I'm even more scared, and also more convinced that we're recreating
a
bad version of some of the core driver model concepts.
I think the ideal option here would be if drm_aperture could unload
(unbind really) the platform driver for us, through the driver model.
Then
there's only one place that keeps track whether the driver is unbound
or
not. I'm not sure whether this can be done fully generic on a struct
device, or whether we need special code for each type. Since atm we only
have simpledrm we can just specialize on platform_device and it's good
enough.
I meanwhile found that calling platform_device_unregister() is the right
thing to do. It is like a hot-unplug event. It's simple to implement and
removes the generic device as well. Any memory ranges for the generic device
are gone as well. Only the native driver's native device will remain. That's
better than the existing simplefb driver.
That sounds great.
Which unregister function to call still driver-specific, so I kept the
callback.
Could we have the callback in core code, and you do something like
drm_aperture_acquire_platform (and later on drm_aperture_acquire_pci or
whatever, although tbh I'm not sure we ever get anything else than
platform). That function can do a runtime check that drm_device->dev is
actually a platform dev.
Somehow I knew you wouldn't like the current abstraction. :)
Another idea: Do the runtime casting in the core without anything? Atm we
have platform that needs support, maybe pci device, so we could easily
extend this and just let it do the right thing. Then no callback is
needed. I.e.
if (is_platform_dev(drm_device->dev))
platform_device_unregister(drm_device->dev);
else
WARN(1, "not yet implemented\n");
or something like that.
I don't like that. I spend time to remove the usb and pci device
pointers from code and structs. I don't want to introduce a new
hard-coded special case here.
I just find the callback to essentially unregister a device a bit
redundant.
I'd like to go with your first idea. The callback would be internal and
the public acquire function is specifically for firmware-based platform
devices. That covers simple-framebuffer, VESA, EFI, and probably any
other generic interface that fbdev supported in the last 20+ yrs. I
don't think we'll ever need anything else.
Still, I'd like to have some abstraction between the internals of the
aperture helpers and our actual use case. I'll update the patchset
accordingly.
Best regards
Thomas
-Daniel
Best regards
Thomas
I think best here would be to Cc: gregkh on this patch and the simpledrm
->detach implementatation, and ask for his feedback as driver model
maintainer. Maybe if you could hack together the platform_device unbind
path as proof of concept would be even better.
Either way, this is really tricky.
-Daniel
Best regards
Thomas
Or maybe we should tie this more into the struct device mode and force an
unload that way? That way devm cleanup would work as one expects, and
avoid the need for anything specific (hopefully) in this detach callback.
Just feels a bit like we're reinventing half of the driver model here,
badly.
+ * };
+ *
+ * static int acquire_framebuffers(struct drm_device *dev, struct
pci_dev *pdev)
+ * {
+ * resource_size_t start, len;
+ * struct drm_aperture *ap;
+ *
+ * base = pci_resource_start(pdev, 0);
+ * size = pci_resource_len(pdev, 0);
+ *
+ * ap = devm_acquire_aperture(dev, base, size, _funcs);
+ * if (IS_ERR(ap))
+ * return PTR_ERR(ap);
+ *
+ * return 0;
+ * }
+ *
+ * static int probe(struct pci_dev *pdev)
+ * {
+ * struct drm_device *dev;
+ * int ret;
+ *
+ * // ... Initialize the device...
+ * dev = devm_drm_dev_alloc();
+ * ...
+ *
+ * // ... and acquire ownership of the framebuffer.
+ * ret = acquire_framebuffers(dev, pdev);
+ * if (ret)
+ * return ret;
+ *
+ * drm_dev_register();
+ *
+ * return 0;
+ * }
+ *
+ * The generic driver is now subject to forced removal by other drivers. This
+ * is when the detach function in struct _aperture_funcs comes into play.
+ * When a driver calls drm_fb_helper_remove_conflicting_framebuffers() et al
+ * for the registered framebuffer range, the DRM core calls struct
+ * _aperture_funcs.detach and the generic driver
Re: [PATCH] virtio_blk: Add support for lifetime feature
On Wed, Apr 14, 2021 at 09:44:35AM +0100, Stefan Hajnoczi wrote: > On Mon, Apr 12, 2021 at 10:42:17AM +0100, Christoph Hellwig wrote: > > A note to the virtio committee: eMMC is the worst of all the currently > > active storage standards by a large margin. It defines very strange > > ad-hoc interfaces that expose very specific internals and often provides > > very poor abstractions. It would be great it you could reach out to the > > wider storage community before taking bad ideas from the eMMC standard > > and putting it into virtio. > > As Michael mentioned, there is still time to change the virtio-blk spec > since this feature hasn't been released yet. > > Why exactly is exposing eMMC-style lifetime information problematic? > > Can you and Enrico discuss the use case to figure out an alternative > interface? Mostly because it exposed a very awkward encoding that is not actually documented in any publically available spec. If you want to incorporate a more open definition doing something like the NVMe 'Endurance Estimate' and 'Percentage Used' fields. But the most important thing is to fully document the semantics in the virtio document instead of refercing a closed standard. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_blk: Add support for lifetime feature
On Mon, Apr 12, 2021 at 08:00:24AM -0400, Michael S. Tsirkin wrote: > On Mon, Apr 12, 2021 at 10:42:17AM +0100, Christoph Hellwig wrote: > > A note to the virtio committee: eMMC is the worst of all the currently > > active storage standards by a large margin. It defines very strange > > ad-hoc interfaces that expose very specific internals and often provides > > very poor abstractions. > > Are we talking about the lifetime feature here? UFS has it too right? Ok, the wide margin above ignores UFS, which has a lot of the same issues as EMMC, just a little less cruft. > It's not too late to > change things if necessary... it would be great if you could provide > more of the feedback on this on the TC mailing list. I think the big main issue here is that it just forwards an awkwardly specific concept through virtio. If we want a virtio feature it really needs to stand a lone and be properly documented without referring to external specifications that are not openly available. > > It would be great it you could reach out to the > > wider storage community before taking bad ideas from the eMMC standard > > and putting it into virtio. > > Noted. It would be great if we had more representation from the storage > community ... meanwhile what would a good forum for this be? > linux-bl...@vger.kernel.org ? At least for linux, yes. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 3/3] iommu/virtio: Enable x86 support
On Thu, Mar 18, 2021 at 02:28:02PM -0400, Michael S. Tsirkin wrote: > On Tue, Mar 16, 2021 at 08:16:54PM +0100, Jean-Philippe Brucker wrote: > > With the VIOT support in place, x86 platforms can now use the > > virtio-iommu. > > > > The arm64 Kconfig selects IOMMU_DMA, while x86 IOMMU drivers select it > > themselves. > > > > Signed-off-by: Jean-Philippe Brucker > > Acked-by: Michael S. Tsirkin > > > --- > > drivers/iommu/Kconfig | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig > > index 2819b5c8ec30..ccca83ef2f06 100644 > > --- a/drivers/iommu/Kconfig > > +++ b/drivers/iommu/Kconfig > > @@ -400,8 +400,9 @@ config HYPERV_IOMMU > > config VIRTIO_IOMMU > > tristate "Virtio IOMMU driver" > > depends on VIRTIO > > - depends on ARM64 > > + depends on (ARM64 || X86) > > select IOMMU_API > > + select IOMMU_DMA if X86 > > Would it hurt to just select unconditionally? Seems a bit cleaner > ... Yes I think I'll do this for the moment Thanks, Jean > > > select INTERVAL_TREE > > select ACPI_VIOT if ACPI > > help > > -- > > 2.30.2 > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 3/3] iommu/virtio: Enable x86 support
On Thu, Mar 18, 2021 at 11:43:38AM +, Robin Murphy wrote: > On 2021-03-16 19:16, Jean-Philippe Brucker wrote: > > With the VIOT support in place, x86 platforms can now use the > > virtio-iommu. > > > > The arm64 Kconfig selects IOMMU_DMA, while x86 IOMMU drivers select it > > themselves. > > Actually, now that both AMD and Intel are converted over, maybe it's finally > time to punt that to x86 arch code to match arm64? x86 also has CONFIG_HYPERV_IOMMU that doesn't use IOMMU_DMA, and might not want to pull in dma-iommu.o + iova.o (don't know if they care about guest size). There also is the old gart driver, but that doesn't select IOMMU_SUPPORT. Thanks, Jean ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/3] ACPI: Add driver for the VIOT table
On Fri, Mar 19, 2021 at 11:44:26AM +0100, Auger Eric wrote: > add some kernel-doc comments matching the explanation in the commit message? Yes, I'll add some comments. I got rid of the other issues you pointed out while reworking the driver, should be more straightforward now Thanks, Jean ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v6 10/10] Documentation: Add documentation for VDUSE
On Thu, Apr 15, 2021 at 04:36:35PM +0800, Jason Wang wrote:
>
> 在 2021/4/15 下午3:19, Stefan Hajnoczi 写道:
> > On Thu, Apr 15, 2021 at 01:38:37PM +0800, Yongji Xie wrote:
> > > On Wed, Apr 14, 2021 at 10:15 PM Stefan Hajnoczi
> > > wrote:
> > > > On Wed, Mar 31, 2021 at 04:05:19PM +0800, Xie Yongji wrote:
> > > > > VDUSE (vDPA Device in Userspace) is a framework to support
> > > > > implementing software-emulated vDPA devices in userspace. This
> > > > > document is intended to clarify the VDUSE design and usage.
> > > > >
> > > > > Signed-off-by: Xie Yongji
> > > > > ---
> > > > > Documentation/userspace-api/index.rst | 1 +
> > > > > Documentation/userspace-api/vduse.rst | 212
> > > > > ++
> > > > > 2 files changed, 213 insertions(+)
> > > > > create mode 100644 Documentation/userspace-api/vduse.rst
> > > > Just looking over the documentation briefly (I haven't studied the code
> > > > yet)...
> > > >
> > > Thank you!
> > >
> > > > > +How VDUSE works
> > > > > +
> > > > > +Each userspace vDPA device is created by the VDUSE_CREATE_DEV ioctl
> > > > > on
> > > > > +the character device (/dev/vduse/control). Then a device file with
> > > > > the
> > > > > +specified name (/dev/vduse/$NAME) will appear, which can be used to
> > > > > +implement the userspace vDPA device's control path and data path.
> > > > These steps are taken after sending the VDPA_CMD_DEV_NEW netlink
> > > > message? (Please consider reordering the documentation to make it clear
> > > > what the sequence of steps are.)
> > > >
> > > No, VDUSE devices should be created before sending the
> > > VDPA_CMD_DEV_NEW netlink messages which might produce I/Os to VDUSE.
> > I see. Please include an overview of the steps before going into detail.
> > Something like:
> >
> >VDUSE devices are started as follows:
> >
> >1. Create a new VDUSE instance with ioctl(VDUSE_CREATE_DEV) on
> > /dev/vduse/control.
> >
> >2. Begin processing VDUSE messages from /dev/vduse/$NAME. The first
> > messages will arrive while attaching the VDUSE instance to vDPA.
> >
> >3. Send the VDPA_CMD_DEV_NEW netlink message to attach the VDUSE
> > instance to vDPA.
> >
> >VDUSE devices are stopped as follows:
> >
> >...
> >
> > > > > + static int netlink_add_vduse(const char *name, int device_id)
> > > > > + {
> > > > > + struct nl_sock *nlsock;
> > > > > + struct nl_msg *msg;
> > > > > + int famid;
> > > > > +
> > > > > + nlsock = nl_socket_alloc();
> > > > > + if (!nlsock)
> > > > > + return -ENOMEM;
> > > > > +
> > > > > + if (genl_connect(nlsock))
> > > > > + goto free_sock;
> > > > > +
> > > > > + famid = genl_ctrl_resolve(nlsock, VDPA_GENL_NAME);
> > > > > + if (famid < 0)
> > > > > + goto close_sock;
> > > > > +
> > > > > + msg = nlmsg_alloc();
> > > > > + if (!msg)
> > > > > + goto close_sock;
> > > > > +
> > > > > + if (!genlmsg_put(msg, NL_AUTO_PORT, NL_AUTO_SEQ, famid,
> > > > > 0, 0,
> > > > > + VDPA_CMD_DEV_NEW, 0))
> > > > > + goto nla_put_failure;
> > > > > +
> > > > > + NLA_PUT_STRING(msg, VDPA_ATTR_DEV_NAME, name);
> > > > > + NLA_PUT_STRING(msg, VDPA_ATTR_MGMTDEV_DEV_NAME,
> > > > > "vduse");
> > > > > + NLA_PUT_U32(msg, VDPA_ATTR_DEV_ID, device_id);
> > > > What are the permission/capability requirements for VDUSE?
> > > >
> > > Now I think we need privileged permission (root user). Because
> > > userspace daemon is able to access avail vring, used vring, descriptor
> > > table in kernel driver directly.
> > Please state this explicitly at the start of the document. Existing
> > interfaces like FUSE are designed to avoid trusting userspace.
>
>
> There're some subtle difference here. VDUSE present a device to kernel which
> means IOMMU is probably the only thing to prevent a malicous device.
>
>
> > Therefore
> > people might think the same is the case here. It's critical that people
> > are aware of this before deploying VDUSE with virtio-vdpa.
> >
> > We should probably pause here and think about whether it's possible to
> > avoid trusting userspace. Even if it takes some effort and costs some
> > performance it would probably be worthwhile.
>
>
> Since the bounce buffer is used the only attack surface is the coherent
> area, if we want to enforce stronger isolation we need to use shadow
> virtqueue (which is proposed in earlier version by me) in this case. But I'm
> not sure it's worth to do that.
The security situation needs to be clear before merging this feature.
I think the IOMMU and vring can be made secure. What is more concerning
is the kernel code that runs on top: VIRTIO device drivers, network
stack, file systems, etc. They trust devices
Re: [PATCH 1/3] ACPICA: iASL: Add definitions for the VIOT table
On Thu, Mar 18, 2021 at 06:52:44PM +0100, Auger Eric wrote: > Besides > Reviewed-by: Eric Auger Thanks, though this patch comes from ACPICA and has now been merged with the other ACPICA updates: https://lore.kernel.org/linux-acpi/20210406213028.718796-1-erik.kan...@intel.com/ Thanks, Jean ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/3] ACPI: Add driver for the VIOT table
On Thu, Mar 18, 2021 at 07:36:50PM +, Robin Murphy wrote:
> On 2021-03-16 19:16, Jean-Philippe Brucker wrote:
> > The ACPI Virtual I/O Translation Table describes topology of
> > para-virtual platforms. For now it describes the relation between
> > virtio-iommu and the endpoints it manages. Supporting that requires
> > three steps:
> >
> > (1) acpi_viot_init(): parse the VIOT table, build a list of endpoints
> > and vIOMMUs.
> >
> > (2) acpi_viot_set_iommu_ops(): when the vIOMMU driver is loaded and the
> > device probed, register it to the VIOT driver. This step is required
> > because unlike similar drivers, VIOT doesn't create the vIOMMU
> > device.
>
> Note that you're basically the same as the DT case in this regard, so I'd
> expect things to be closer to that pattern than to that of IORT.
>
> [...]
> > @@ -1506,12 +1507,17 @@ int acpi_dma_configure_id(struct device *dev, enum
> > dev_dma_attr attr,
> > {
> > const struct iommu_ops *iommu;
> > u64 dma_addr = 0, size = 0;
> > + int ret;
> > if (attr == DEV_DMA_NOT_SUPPORTED) {
> > set_dma_ops(dev, _dummy_ops);
> > return 0;
> > }
> > + ret = acpi_viot_dma_setup(dev, attr);
> > + if (ret)
> > + return ret > 0 ? 0 : ret;
>
> I think things could do with a fair bit of refactoring here. Ideally we want
> to process a possible _DMA method (acpi_dma_get_range()) regardless of which
> flavour of IOMMU table might be present, and the amount of duplication we
> fork into at this point is unfortunate.
>
> > +
> > iort_dma_setup(dev, _addr, );
>
> For starters I think most of that should be dragged out to this level here -
> it's really only the {rc,nc}_dma_get_range() bit that deserves to be the
> IORT-specific call.
Makes sense, though I'll move it to drivers/acpi/arm64/dma.c instead of
here, because it has only ever run on CONFIG_ARM64. I don't want to
accidentally break some x86 platform with an invalid _DMA method (same
reason 7ad426398082 and 18b709beb503 kept this code in IORT)
>
> > iommu = iort_iommu_configure_id(dev, input_id);
>
> Similarly, it feels like it's only the table scan part in the middle of that
> that needs dispatching between IORT/VIOT, and its head and tail pulled out
> into a common path.
Agreed
>
> [...]
> > +static const struct iommu_ops *viot_iommu_setup(struct device *dev)
> > +{
> > + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> > + struct viot_iommu *viommu = NULL;
> > + struct viot_endpoint *ep;
> > + u32 epid;
> > + int ret;
> > +
> > + /* Already translated? */
> > + if (fwspec && fwspec->ops)
> > + return NULL;
> > +
> > + mutex_lock(_lock);
> > + list_for_each_entry(ep, _endpoints, list) {
> > + if (viot_device_match(dev, >dev_id, )) {
> > + epid += ep->endpoint_id;
> > + viommu = ep->viommu;
> > + break;
> > + }
> > + }
> > + mutex_unlock(_lock);
> > + if (!viommu)
> > + return NULL;
> > +
> > + /* We're not translating ourself */
> > + if (viot_device_match(dev, >dev_id, ))
> > + return NULL;
> > +
> > + /*
> > +* If we found a PCI range managed by the viommu, we're the one that has
> > +* to request ACS.
> > +*/
> > + if (dev_is_pci(dev))
> > + pci_request_acs();
> > +
> > + if (!viommu->ops || WARN_ON(!viommu->dev))
> > + return ERR_PTR(-EPROBE_DEFER);
>
> Can you create (or look up) a viommu->fwnode when initially parsing the VIOT
> to represent the IOMMU devices to wait for, such that the
> viot_device_match() lookup can resolve to that and let you fall into the
> standard iommu_ops_from_fwnode() path? That's what I mean about following
> the DT pattern - I guess it might need a bit of trickery to rewrite things
> if iommu_device_register() eventually turns up with a new fwnode, so I doubt
> we can get away without *some* kind of private interface between
> virtio-iommu and VIOT, but it would be nice for the common(ish) DMA paths to
> stay as unaware of the specifics as possible.
Yes I can reuse iommu_ops_from_fwnode(). Turns out it's really easy: if we
move the VIOT initialization after acpi_scan_init(), we can use
pci_get_domain_bus_and_slot() directly and create missing fwnodes. That
gets rid of any need for a private interface between virtio-iommu and
VIOT.
>
> > +
> > + ret = iommu_fwspec_init(dev, viommu->dev->fwnode, viommu->ops);
> > + if (ret)
> > + return ERR_PTR(ret);
> > +
> > + iommu_fwspec_add_ids(dev, , 1);
> > +
> > + /*
> > +* If we have reason to believe the IOMMU driver missed the initial
> > +* add_device callback for dev, replay it to get things in order.
> > +*/
> > + if (dev->bus && !device_iommu_mapped(dev))
> > + iommu_probe_device(dev);
> > +
> > + return viommu->ops;
> > +}
> > +
> > +/**
> > + * acpi_viot_dma_setup - Configure DMA for an endpoint described in
Re: Re: Re: [PATCH v6 10/10] Documentation: Add documentation for VDUSE
On Thu, Apr 15, 2021 at 04:33:27PM +0800, Yongji Xie wrote: > On Thu, Apr 15, 2021 at 3:19 PM Stefan Hajnoczi wrote: > > On Thu, Apr 15, 2021 at 01:38:37PM +0800, Yongji Xie wrote: > > > On Wed, Apr 14, 2021 at 10:15 PM Stefan Hajnoczi > > > wrote: > > > > On Wed, Mar 31, 2021 at 04:05:19PM +0800, Xie Yongji wrote: > > It's not obvious to me that there is a fundamental difference between > > the two approaches in terms of performance. > > > > > On the other > > > hand, we can handle the virtqueue in a unified way for both vhost-vdpa > > > case and virtio-vdpa case. Otherwise, userspace daemon needs to know > > > which iova ranges need to be accessed with pread(2)/pwrite(2). And in > > > the future, we might be able to avoid bouncing in some cases. > > > > Ah, I see. So bounce buffers are not used for vhost-vdpa? > > > > Yes. Okay, in that case I understand why mmap is used and it's nice to keep virtio-vpda and vhost-vdpa unified. Thanks! Stefan signature.asc Description: PGP signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 3/3] vDPA/ifcvf: get_config_size should return dev specific config size
On Thu, Apr 15, 2021 at 05:53:36PM +0800, Zhu Lingshan wrote:
get_config_size() should return the size based on the decected
device type.
Signed-off-by: Zhu Lingshan
---
drivers/vdpa/ifcvf/ifcvf_main.c | 18 +-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index cea1313b1a3f..6844c49fe1de 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -347,7 +347,23 @@ static u32 ifcvf_vdpa_get_vq_align(struct vdpa_device
*vdpa_dev)
static size_t ifcvf_vdpa_get_config_size(struct vdpa_device *vdpa_dev)
{
- return sizeof(struct virtio_net_config);
+ struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev);
+ struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
+ struct pci_dev *pdev = adapter->pdev;
+ size_t size;
+
+ if (vf->dev_type == VIRTIO_ID_NET)
+ size = sizeof(struct virtio_net_config);
+
+ else if (vf->dev_type == VIRTIO_ID_BLOCK)
+ size = sizeof(struct virtio_blk_config);
+
+ else {
+ size = 0;
+ IFCVF_ERR(pdev, "VIRTIO ID %u not supported\n", vf->dev_type);
+ }
I slightly prefer the switch, but I don't have a strong opinion.
However, if we want to use if/else, we should follow
`Documentation/process/coding-style.rst` line 166:
Note that the closing brace is empty on a line of its own, **except** in
the cases where it is followed by a continuation of the same statement,
ie a ``while`` in a do-statement or an ``else`` in an if-statement, like
also `scripts/checkpatch.pl --strict` complains:
CHECK: braces {} should be used on all arms of this statement
#209: FILE: drivers/vdpa/ifcvf/ifcvf_main.c:355:
+ if (vf->dev_type == VIRTIO_ID_NET)
[...]
+ else if (vf->dev_type == VIRTIO_ID_BLOCK)
[...]
+ else {
[...]
CHECK: Unbalanced braces around else statement
#215: FILE: drivers/vdpa/ifcvf/ifcvf_main.c:361:
+ else {
Thanks,
Stefano
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 2/3] vDPA/ifcvf: enable Intel C5000X-PL virtio-block for vDPA
On Thu, Apr 15, 2021 at 05:53:35PM +0800, Zhu Lingshan wrote:
This commit enabled Intel FPGA SmartNIC C5000X-PL virtio-block
for vDPA.
Signed-off-by: Zhu Lingshan
---
drivers/vdpa/ifcvf/ifcvf_base.h | 8 +++-
drivers/vdpa/ifcvf/ifcvf_main.c | 10 +-
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
index 1c04cd256fa7..0111bfdeb342 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.h
+++ b/drivers/vdpa/ifcvf/ifcvf_base.h
@@ -15,6 +15,7 @@
#include
#include
#include
+#include
#include
#include
@@ -28,7 +29,12 @@
#define C5000X_PL_SUBSYS_VENDOR_ID 0x8086
#define C5000X_PL_SUBSYS_DEVICE_ID 0x0001
-#define IFCVF_SUPPORTED_FEATURES \
+#define C5000X_PL_BLK_VENDOR_ID0x1AF4
+#define C5000X_PL_BLK_DEVICE_ID0x1001
+#define C5000X_PL_BLK_SUBSYS_VENDOR_ID 0x8086
+#define C5000X_PL_BLK_SUBSYS_DEVICE_ID 0x0002
+
+#define IFCVF_NET_SUPPORTED_FEATURES \
((1ULL << VIRTIO_NET_F_MAC) | \
(1ULL << VIRTIO_F_ANY_LAYOUT)| \
(1ULL << VIRTIO_F_VERSION_1) | \
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index 469a9b5737b7..cea1313b1a3f 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -171,7 +171,11 @@ static u64 ifcvf_vdpa_get_features(struct vdpa_device
*vdpa_dev)
struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
u64 features;
- features = ifcvf_get_features(vf) & IFCVF_SUPPORTED_FEATURES;
+ if (vf->dev_type == VIRTIO_ID_NET)
+ features = ifcvf_get_features(vf) &
IFCVF_NET_SUPPORTED_FEATURES;
+
+ if (vf->dev_type == VIRTIO_ID_BLOCK)
+ features = ifcvf_get_features(vf);
Should we put a warning here too otherwise feature could be seen
unassigned?
Thanks,
Stefano
return features;
}
@@ -517,6 +521,10 @@ static struct pci_device_id ifcvf_pci_ids[] = {
C5000X_PL_DEVICE_ID,
C5000X_PL_SUBSYS_VENDOR_ID,
C5000X_PL_SUBSYS_DEVICE_ID) },
+ { PCI_DEVICE_SUB(C5000X_PL_BLK_VENDOR_ID,
+C5000X_PL_BLK_DEVICE_ID,
+C5000X_PL_BLK_SUBSYS_VENDOR_ID,
+C5000X_PL_BLK_SUBSYS_DEVICE_ID) },
{ 0 },
};
--
2.27.0
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 04/10] drm/aperture: Add infrastructure for aperture ownership
On Thu, Apr 15, 2021 at 08:56:20AM +0200, Thomas Zimmermann wrote:
> Hi
>
> Am 09.04.21 um 11:22 schrieb Daniel Vetter:
> > > Is it that easy? simepldrm's detach function has code to synchronize with
> > > concurrent hotplug removals. If we can use drm_dev_unplug() for
> > > everything,
> > > I'm all for it.
> >
> > Uh, I should have looked at the code instead of just asking silly
> > questions :-)
> >
> > Now I'm even more scared, and also more convinced that we're recreating
> a
> > bad version of some of the core driver model concepts.
> >
> > I think the ideal option here would be if drm_aperture could unload
> > (unbind really) the platform driver for us, through the driver model. Then
> > there's only one place that keeps track whether the driver is unbound or
> > not. I'm not sure whether this can be done fully generic on a struct
> > device, or whether we need special code for each type. Since atm we only
> > have simpledrm we can just specialize on platform_device and it's good
> > enough.
>
> I meanwhile found that calling platform_device_unregister() is the right
> thing to do. It is like a hot-unplug event. It's simple to implement and
> removes the generic device as well. Any memory ranges for the generic device
> are gone as well. Only the native driver's native device will remain. That's
> better than the existing simplefb driver.
That sounds great.
> Which unregister function to call still driver-specific, so I kept the
> callback.
Could we have the callback in core code, and you do something like
drm_aperture_acquire_platform (and later on drm_aperture_acquire_pci or
whatever, although tbh I'm not sure we ever get anything else than
platform). That function can do a runtime check that drm_device->dev is
actually a platform dev.
Another idea: Do the runtime casting in the core without anything? Atm we
have platform that needs support, maybe pci device, so we could easily
extend this and just let it do the right thing. Then no callback is
needed. I.e.
if (is_platform_dev(drm_device->dev))
platform_device_unregister(drm_device->dev);
else
WARN(1, "not yet implemented\n");
or something like that.
I just find the callback to essentially unregister a device a bit
redundant.
-Daniel
>
> Best regards
> Thomas
>
> >
> > I think best here would be to Cc: gregkh on this patch and the simpledrm
> > ->detach implementatation, and ask for his feedback as driver model
> > maintainer. Maybe if you could hack together the platform_device unbind
> > path as proof of concept would be even better.
> >
> > Either way, this is really tricky.
> > -Daniel
> >
> > >
> > > Best regards
> > > Thomas
> > >
> > > >
> > > > Or maybe we should tie this more into the struct device mode and force
> > > > an
> > > > unload that way? That way devm cleanup would work as one expects, and
> > > > avoid the need for anything specific (hopefully) in this detach
> > > > callback.
> > > >
> > > > Just feels a bit like we're reinventing half of the driver model here,
> > > > badly.
> > > >
> > > > > + * };
> > > > > + *
> > > > > + * static int acquire_framebuffers(struct drm_device *dev, struct
> > > > > pci_dev *pdev)
> > > > > + * {
> > > > > + * resource_size_t start, len;
> > > > > + * struct drm_aperture *ap;
> > > > > + *
> > > > > + * base = pci_resource_start(pdev, 0);
> > > > > + * size = pci_resource_len(pdev, 0);
> > > > > + *
> > > > > + * ap = devm_acquire_aperture(dev, base, size, _funcs);
> > > > > + * if (IS_ERR(ap))
> > > > > + * return PTR_ERR(ap);
> > > > > + *
> > > > > + * return 0;
> > > > > + * }
> > > > > + *
> > > > > + * static int probe(struct pci_dev *pdev)
> > > > > + * {
> > > > > + * struct drm_device *dev;
> > > > > + * int ret;
> > > > > + *
> > > > > + * // ... Initialize the device...
> > > > > + * dev = devm_drm_dev_alloc();
> > > > > + * ...
> > > > > + *
> > > > > + * // ... and acquire ownership of the framebuffer.
> > > > > + * ret = acquire_framebuffers(dev, pdev);
> > > > > + * if (ret)
> > > > > + * return ret;
> > > > > + *
> > > > > + * drm_dev_register();
> > > > > + *
> > > > > + * return 0;
> > > > > + * }
> > > > > + *
> > > > > + * The generic driver is now subject to forced removal by other
> > > > > drivers. This
> > > > > + * is when the detach function in struct _aperture_funcs comes
> > > > > into play.
> > > > > + * When a driver calls
> > > > > drm_fb_helper_remove_conflicting_framebuffers() et al
> > > > > + * for the registered framebuffer range, the DRM core calls struct
> > > > > + * _aperture_funcs.detach and the generic driver has to onload
> > > > > itself. It
> > > > > + * may not access the device's registers, framebuffer memory, ROM,
> > > > > etc after
> > > > > + *
Re: [PATCH] sound: virtio: correct the function name in kernel-doc comment
On 15.04.2021 07:26, Randy Dunlap wrote: Fix kernel-doc warning that the wrong function name is used in a kernel-doc comment: ../sound/virtio/virtio_ctl_msg.c:70: warning: expecting prototype for virtsnd_ctl_msg_request(). Prototype was for virtsnd_ctl_msg_response() instead Signed-off-by: Randy Dunlap Cc: Anton Yakovlev Cc: "Michael S. Tsirkin" Cc: virtualization@lists.linux-foundation.org Cc: alsa-de...@alsa-project.org Thanks for fixing the copy/paste mistake. :) Reviewed-by: Anton Yakovlev --- sound/virtio/virtio_ctl_msg.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- linux-next-20210414.orig/sound/virtio/virtio_ctl_msg.c +++ linux-next-20210414/sound/virtio/virtio_ctl_msg.c @@ -61,7 +61,7 @@ void *virtsnd_ctl_msg_request(struct vir } /** - * virtsnd_ctl_msg_request() - Get a pointer to the response header. + * virtsnd_ctl_msg_response() - Get a pointer to the response header. * @msg: Control message. * * Context: Any context. -- Anton Yakovlev Senior Software Engineer OpenSynergy GmbH Rotherstr. 20, 10245 Berlin ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 08/10] drm/simpledrm: Acquire clocks from DT device node
Hi Am 15.04.21 um 11:21 schrieb Maxime Ripard: Hi, On Thu, Apr 15, 2021 at 09:31:01AM +0200, Thomas Zimmermann wrote: Am 08.04.21 um 10:13 schrieb Maxime Ripard: Hi, On Thu, Mar 18, 2021 at 11:29:19AM +0100, Thomas Zimmermann wrote: Make sure required hardware clocks are enabled while the firmware framebuffer is in use. The basic code has been taken from the simplefb driver and adapted to DRM. Clocks are released automatically via devres helpers. Signed-off-by: Thomas Zimmermann Tested-by: nerdopolis Even though it's definitely simpler to review, merging the driver first and then the clocks and regulators will break bisection on the platforms that rely on them I'd like to keep the patches separate for now, but can squash patches 6 to 8 them into one before pushing them. OK? Yep, that works for me :) Another thing worth considering is also that both drivers will probe if they are enabled (which is pretty likely), which is not great :) I guess we should make them mutually exclusive through Kconfig We already have several drivers in fbdev and DRM that handle the same hardware. We don't do this for any other pair, why bother now? Yeah, but simplefb/simpledrm are going to be enabled pretty much everywhere, as opposed to the other drivers that are more specialized. Well, OK. But I'd like to give simpledrm preference over simplefb. There should be an incentive to switch to DRM. Best regards Thomas Maxime -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer OpenPGP_signature Description: OpenPGP digital signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v6 10/10] Documentation: Add documentation for VDUSE
在 2021/4/15 下午4:36, Jason Wang 写道: Please state this explicitly at the start of the document. Existing interfaces like FUSE are designed to avoid trusting userspace. There're some subtle difference here. VDUSE present a device to kernel which means IOMMU is probably the only thing to prevent a malicous device. Therefore people might think the same is the case here. It's critical that people are aware of this before deploying VDUSE with virtio-vdpa. We should probably pause here and think about whether it's possible to avoid trusting userspace. Even if it takes some effort and costs some performance it would probably be worthwhile. Since the bounce buffer is used the only attack surface is the coherent area, if we want to enforce stronger isolation we need to use shadow virtqueue (which is proposed in earlier version by me) in this case. But I'm not sure it's worth to do that. So this reminds me the discussion in the end of last year. We need to make sure we don't suffer from the same issues for VDUSE at least https://yhbt.net/lore/all/c3629a27-3590-1d9f-211b-c0b7be152...@redhat.com/T/#mc6b6e2343cbeffca68ca7a97e0f473aaa871c95b Or we can solve it at virtio level, e.g remember the dma address instead of depending on the addr in the descriptor ring Thanks Is the security situation different with vhost-vdpa? In that case it seems more likely that the host kernel doesn't need to trust the userspace VDUSE device. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v3 2/9] drm/format-helper: Add blitter functions
The blitter functions copy a framebuffer to I/O memory using one of
the existing conversion functions.
Signed-off-by: Thomas Zimmermann
Reviewed-by: Daniel Vetter
Tested-by: nerdopolis
---
drivers/gpu/drm/drm_format_helper.c | 87 +
include/drm/drm_format_helper.h | 8 +++
2 files changed, 95 insertions(+)
diff --git a/drivers/gpu/drm/drm_format_helper.c
b/drivers/gpu/drm/drm_format_helper.c
index 8d5a683afea7..0e885cd34107 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -344,3 +344,90 @@ void drm_fb_xrgb_to_gray8(u8 *dst, void *vaddr, struct
drm_framebuffer *fb,
}
EXPORT_SYMBOL(drm_fb_xrgb_to_gray8);
+/**
+ * drm_fb_blit_rect_dstclip - Copy parts of a framebuffer to display memory
+ * @dst: The display memory to copy to
+ * @dst_pitch: Number of bytes between two consecutive scanlines within dst
+ * @dst_format:FOURCC code of the display's color format
+ * @vmap: The framebuffer memory to copy from
+ * @fb:The framebuffer to copy from
+ * @clip: Clip rectangle area to copy
+ *
+ * This function copies parts of a framebuffer to display memory. If the
+ * formats of the display and the framebuffer mismatch, the blit function
+ * will attempt to convert between them.
+ *
+ * Use drm_fb_blit_dstclip() to copy the full framebuffer.
+ *
+ * Returns:
+ * 0 on success, or
+ * -EINVAL if the color-format conversion failed, or
+ * a negative error code otherwise.
+ */
+int drm_fb_blit_rect_dstclip(void __iomem *dst, unsigned int dst_pitch,
+uint32_t dst_format, void *vmap,
+struct drm_framebuffer *fb,
+struct drm_rect *clip)
+{
+ uint32_t fb_format = fb->format->format;
+
+ /* treat alpha channel like filler bits */
+ if (fb_format == DRM_FORMAT_ARGB)
+ fb_format = DRM_FORMAT_XRGB;
+ if (dst_format == DRM_FORMAT_ARGB)
+ dst_format = DRM_FORMAT_XRGB;
+
+ if (dst_format == fb_format) {
+ drm_fb_memcpy_dstclip(dst, dst_pitch, vmap, fb, clip);
+ return 0;
+
+ } else if (dst_format == DRM_FORMAT_RGB565) {
+ if (fb_format == DRM_FORMAT_XRGB) {
+ drm_fb_xrgb_to_rgb565_dstclip(dst, dst_pitch,
+ vmap, fb, clip,
+ false);
+ return 0;
+ }
+ } else if (dst_format == DRM_FORMAT_RGB888) {
+ if (fb_format == DRM_FORMAT_XRGB) {
+ drm_fb_xrgb_to_rgb888_dstclip(dst, dst_pitch,
+ vmap, fb, clip);
+ return 0;
+ }
+ }
+
+ return -EINVAL;
+}
+EXPORT_SYMBOL(drm_fb_blit_rect_dstclip);
+
+/**
+ * drm_fb_blit_dstclip - Copy framebuffer to display memory
+ * @dst: The display memory to copy to
+ * @dst_pitch: Number of bytes between two consecutive scanlines within dst
+ * @dst_format:FOURCC code of the display's color format
+ * @vmap: The framebuffer memory to copy from
+ * @fb:The framebuffer to copy from
+ *
+ * This function copies a full framebuffer to display memory. If the formats
+ * of the display and the framebuffer mismatch, the copy function will
+ * attempt to convert between them.
+ *
+ * See drm_fb_blit_rect_dstclip() for more inforamtion.
+ *
+ * Returns:
+ * 0 on success, or a negative error code otherwise.
+ */
+int drm_fb_blit_dstclip(void __iomem *dst, unsigned int dst_pitch,
+ uint32_t dst_format, void *vmap,
+ struct drm_framebuffer *fb)
+{
+ struct drm_rect fullscreen = {
+ .x1 = 0,
+ .x2 = fb->width,
+ .y1 = 0,
+ .y2 = fb->height,
+ };
+ return drm_fb_blit_rect_dstclip(dst, dst_pitch, dst_format, vmap, fb,
+ );
+}
+EXPORT_SYMBOL(drm_fb_blit_dstclip);
diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
index 2b5036a5fbe7..4e0258a61311 100644
--- a/include/drm/drm_format_helper.h
+++ b/include/drm/drm_format_helper.h
@@ -28,4 +28,12 @@ void drm_fb_xrgb_to_rgb888_dstclip(void __iomem *dst,
unsigned int dst_pitch
void drm_fb_xrgb_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
struct drm_rect *clip);
+int drm_fb_blit_rect_dstclip(void __iomem *dst, unsigned int dst_pitch,
+uint32_t dst_format, void *vmap,
+struct drm_framebuffer *fb,
+struct drm_rect *rect);
+int drm_fb_blit_dstclip(void __iomem *dst, unsigned int dst_pitch,
+ uint32_t dst_format, void *vmap,
+
[PATCH v3 9/9] drm/simpledrm: Acquire memory aperture for framebuffer
We register the simplekms device with the DRM platform helpers. A
native driver for the graphics hardware will kick-out the simpledrm
driver before taking over the device.
The original generic platform device from the simple-framebuffer boot
code will be unregistered. The native driver will use whatever native
hardware device it received.
v3:
* use platform_device_unregister() and handle detachment
like hot-unplug event (Daniel)
v2:
* adapt to aperture changes
* use drm_dev_unplug() and drm_dev_enter/exit()
* don't split error string
Signed-off-by: Thomas Zimmermann
Tested-by: nerdopolis
---
drivers/gpu/drm/tiny/simpledrm.c | 49 +++-
1 file changed, 48 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 9d522473cd7c..14473e1bccab 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -6,6 +6,7 @@
#include
#include
+#include
#include
#include
#include
@@ -515,16 +516,49 @@ static int simpledrm_device_init_fb(struct
simpledrm_device *sdev)
* Memory management
*/
+static void simpledrm_aperture_detach(struct drm_device *dev, resource_size_t
base,
+ resource_size_t size)
+{
+ struct simpledrm_device *sdev = simpledrm_device_of_dev(dev);
+ struct platform_device *pdev = sdev->pdev;
+
+ /*
+* Remove the device from the bus itself. This is the right thing to
+* do for the generic DRM drivers, such as EFI, VESA or VGA. When the
+* new driver takes over the hardware, the generic device's state will
+* be lost.
+*
+* If the aperture helpers need to handle native drivers, this call
+* would only have to unplug the DRM device, so that the hardware device
+* stays around after detachment.
+*/
+ platform_device_unregister(pdev);
+}
+
+static const struct drm_aperture_funcs simpledrm_aperture_funcs = {
+ .detach = simpledrm_aperture_detach,
+};
+
static int simpledrm_device_init_mm(struct simpledrm_device *sdev)
{
+ struct drm_device *dev = >dev;
struct platform_device *pdev = sdev->pdev;
struct resource *mem;
void __iomem *screen_base;
+ int ret;
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!mem)
return -EINVAL;
+ ret = devm_aperture_acquire(dev, mem->start, resource_size(mem),
+ _aperture_funcs);
+ if (ret) {
+ drm_err(dev, "could not acquire memory range [0x%llx:0x%llx]:
error %d\n",
+ mem->start, mem->end, ret);
+ return ret;
+ }
+
screen_base = devm_ioremap_wc(>dev, mem->start,
resource_size(mem));
if (!screen_base)
@@ -625,12 +659,18 @@ simpledrm_simple_display_pipe_enable(struct
drm_simple_display_pipe *pipe,
struct drm_shadow_plane_state *shadow_plane_state =
to_drm_shadow_plane_state(plane_state);
struct drm_framebuffer *fb = plane_state->fb;
void *vmap = shadow_plane_state->map[0].vaddr; /* TODO: Use mapping
abstraction properly */
+ struct drm_device *dev = >dev;
+ int idx;
if (!fb)
return;
+ if (!drm_dev_enter(dev, ))
+ return;
+
drm_fb_blit_dstclip(sdev->screen_base, sdev->pitch,
sdev->format->format, vmap, fb);
+ drm_dev_exit(idx);
}
static void
@@ -658,7 +698,9 @@ simpledrm_simple_display_pipe_update(struct
drm_simple_display_pipe *pipe,
struct drm_shadow_plane_state *shadow_plane_state =
to_drm_shadow_plane_state(plane_state);
void *vmap = shadow_plane_state->map[0].vaddr; /* TODO: Use mapping
abstraction properly */
struct drm_framebuffer *fb = plane_state->fb;
+ struct drm_device *dev = >dev;
struct drm_rect clip;
+ int idx;
if (!fb)
return;
@@ -666,8 +708,13 @@ simpledrm_simple_display_pipe_update(struct
drm_simple_display_pipe *pipe,
if (!drm_atomic_helper_damage_merged(old_plane_state, plane_state,
))
return;
+ if (!drm_dev_enter(dev, ))
+ return;
+
drm_fb_blit_rect_dstclip(sdev->screen_base, sdev->pitch,
sdev->format->format, vmap, fb, );
+
+ drm_dev_exit(idx);
}
static const struct drm_simple_display_pipe_funcs
@@ -847,7 +894,7 @@ static int simpledrm_remove(struct platform_device *pdev)
struct simpledrm_device *sdev = platform_get_drvdata(pdev);
struct drm_device *dev = >dev;
- drm_dev_unregister(dev);
+ drm_dev_unplug(dev);
return 0;
}
--
2.31.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
[PATCH v3 0/9] drm: Support simple-framebuffer devices and firmware fbs
This patchset adds support for simple-framebuffer platform devices and a handover mechanism for native drivers to take-over control of the hardware. The new driver, called simpledrm, binds to a simple-frambuffer platform device. The kernel's boot code creates such devices for firmware-provided framebuffers, such as EFI-GOP or VESA. Typically the BIOS, UEFI or boot loader sets up the framebuffers. Description via device tree is also an option. Simpledrm is small enough to be linked into the kernel. The driver's main purpose is to provide graphical output during the early phases of the boot process, before the native DRM drivers are available. Native drivers are typically loaded from an initrd ram disk. Occationally simpledrm can also serve as interim solution on graphics hardware without native DRM driver. So far distributions rely on fbdev drivers, such as efifb, vesafb or simplefb, for early-boot graphical output. However fbdev is deprecated and the drivers do not provide DRM interfaces for modern userspace. Patches 1 and 2 prepare the DRM format helpers for simpledrm. Patches 4 to 8 add the simpledrm driver. It's build on simple DRM helpers and SHMEM. It supports 16-bit, 24-bit and 32-bit RGB framebuffers. During pageflips, SHMEM buffers are copied into the framebuffer memory, similar to cirrus or mgag200. The code in patches 7 and 8 handles clocks and regulators. It's based on the simplefb drivers, but has been modified for DRM. Patches 3 and 9 add a hand-over mechanism. Simpledrm acquires it's framebuffer's I/O-memory range and provides a callback function to be removed by a native driver. The native driver will remove simpledrm before taking over the hardware. The removal is integrated into existing helpers, so drivers use it automatically. I've also been working on fastboot support (i.e., flicker-free booting). This requires state-readout from simpledrm via generic interfaces, as outlined in [1]. I do have some prototype code, but it will take a while to get this ready. Simpledrm will then support it. I've tested simpledrm with x86 EFI and VESA framebuffers, which both work reliably. The fbdev console and Weston work automatically. Xorg requires manual configuration of the device. Xorgs current modesetting driver does not work with both, platform and PCI device, for the same physical hardware. Once configured, X11 works. I looked into X11, but couldn't see an easy way of fixing the problem. With the push towards Wayland+Xwayland I expect the problem to become a non-issue soon. Additional testing has been reported at [2]. One cosmetical issue is that simpledrm's device file is card0 and the native driver's device file is card1. After simpledrm has been kicked out, only card1 is left. This does not seem to be a practical problem however. TODO/IDEAS: * provide deferred takeover * provide bootsplash DRM client * make simplekms usable with ARM-EFI fbs v3: * clear screen to black when disabled (Daniel) * rebase onto existing aperture helpers * detach via hot-unplug via platform_device_unregister() v2: * rename to simpledrm, aperture helpers * reorganized patches * use hotplug helpers for removal (Daniel) * added DT match tables (Rob) * use shadow-plane helpers * lots of minor cleanups [1] https://lore.kernel.org/dri-devel/CAKMK7uHtqHy_oz4W7F+hmp9iqp7W5Ra8CxPvJ=9bwmvfu-o...@mail.gmail.com/ [2] https://lore.kernel.org/dri-devel/1761762.3HQLrFs1K7@nerdopolis/ Thomas Zimmermann (9): drm/format-helper: Pass destination pitch to drm_fb_memcpy_dstclip() drm/format-helper: Add blitter functions drm/aperture: Add infrastructure for aperture ownership drm: Add simpledrm driver drm/simpledrm: Add fbdev emulation drm/simpledrm: Initialize framebuffer data from device-tree node drm/simpledrm: Acquire clocks from DT device node drm/simpledrm: Acquire regulators from DT device node drm/simpledrm: Acquire memory aperture for framebuffer MAINTAINERS| 7 + drivers/gpu/drm/drm_aperture.c | 216 +- drivers/gpu/drm/drm_format_helper.c| 96 ++- drivers/gpu/drm/mgag200/mgag200_mode.c | 2 +- drivers/gpu/drm/tiny/Kconfig | 16 + drivers/gpu/drm/tiny/Makefile | 1 + drivers/gpu/drm/tiny/cirrus.c | 2 +- drivers/gpu/drm/tiny/simpledrm.c | 920 + include/drm/drm_aperture.h | 36 +- include/drm/drm_format_helper.h| 10 +- 10 files changed, 1279 insertions(+), 27 deletions(-) create mode 100644 drivers/gpu/drm/tiny/simpledrm.c -- 2.31.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v3 6/9] drm/simpledrm: Initialize framebuffer data from device-tree node
A firmware framebuffer might also be specified via device-tree files. If
no device platform data is given, try the DT device node.
v2:
* add Device Tree match table
* clean-up parser wrappers
Signed-off-by: Thomas Zimmermann
Tested-by: nerdopolis
---
drivers/gpu/drm/tiny/simpledrm.c | 89
1 file changed, 89 insertions(+)
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 64e8a8581d9a..53d6bec7d0b2 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -114,6 +114,74 @@ simplefb_get_format_pd(struct drm_device *dev,
return simplefb_get_validated_format(dev, pd->format);
}
+static int
+simplefb_read_u32_of(struct drm_device *dev, struct device_node *of_node,
+const char *name, u32 *value)
+{
+ int ret = of_property_read_u32(of_node, name, value);
+
+ if (ret)
+ drm_err(dev, "simplefb: cannot parse framebuffer %s: error
%d\n",
+ name, ret);
+ return ret;
+}
+
+static int
+simplefb_read_string_of(struct drm_device *dev, struct device_node *of_node,
+ const char *name, const char **value)
+{
+ int ret = of_property_read_string(of_node, name, value);
+
+ if (ret)
+ drm_err(dev, "simplefb: cannot parse framebuffer %s: error
%d\n",
+ name, ret);
+ return ret;
+}
+
+static int
+simplefb_get_width_of(struct drm_device *dev, struct device_node *of_node)
+{
+ u32 width;
+ int ret = simplefb_read_u32_of(dev, of_node, "width", );
+
+ if (ret)
+ return ret;
+ return simplefb_get_validated_int0(dev, "width", width);
+}
+
+static int
+simplefb_get_height_of(struct drm_device *dev, struct device_node *of_node)
+{
+ u32 height;
+ int ret = simplefb_read_u32_of(dev, of_node, "height", );
+
+ if (ret)
+ return ret;
+ return simplefb_get_validated_int0(dev, "height", height);
+}
+
+static int
+simplefb_get_stride_of(struct drm_device *dev, struct device_node *of_node)
+{
+ u32 stride;
+ int ret = simplefb_read_u32_of(dev, of_node, "stride", );
+
+ if (ret)
+ return ret;
+ return simplefb_get_validated_int(dev, "stride", stride);
+}
+
+static const struct drm_format_info *
+simplefb_get_format_of(struct drm_device *dev, struct device_node *of_node)
+{
+ const char *format;
+ int ret = simplefb_read_string_of(dev, of_node, "format", );
+
+ if (ret)
+ return ERR_PTR(ret);
+ return simplefb_get_validated_format(dev, format);
+}
+
/*
* Simple Framebuffer device
*/
@@ -166,6 +234,7 @@ static int simpledrm_device_init_fb(struct simpledrm_device
*sdev)
struct drm_device *dev = >dev;
struct platform_device *pdev = sdev->pdev;
const struct simplefb_platform_data *pd = dev_get_platdata(>dev);
+ struct device_node *of_node = pdev->dev.of_node;
if (pd) {
width = simplefb_get_width_pd(dev, pd);
@@ -180,6 +249,19 @@ static int simpledrm_device_init_fb(struct
simpledrm_device *sdev)
format = simplefb_get_format_pd(dev, pd);
if (IS_ERR(format))
return PTR_ERR(format);
+ } else if (of_node) {
+ width = simplefb_get_width_of(dev, of_node);
+ if (width < 0)
+ return width;
+ height = simplefb_get_height_of(dev, of_node);
+ if (height < 0)
+ return height;
+ stride = simplefb_get_stride_of(dev, of_node);
+ if (stride < 0)
+ return stride;
+ format = simplefb_get_format_of(dev, of_node);
+ if (IS_ERR(format))
+ return PTR_ERR(format);
} else {
drm_err(dev, "no simplefb configuration found\n");
return -ENODEV;
@@ -534,9 +616,16 @@ static int simpledrm_remove(struct platform_device *pdev)
return 0;
}
+static const struct of_device_id simpledrm_of_match_table[] = {
+ { .compatible = "simple-framebuffer", },
+ { },
+};
+MODULE_DEVICE_TABLE(of, simpledrm_of_match_table);
+
static struct platform_driver simpledrm_platform_driver = {
.driver = {
.name = "simple-framebuffer", /* connect to sysfb */
+ .of_match_table = simpledrm_of_match_table,
},
.probe = simpledrm_probe,
.remove = simpledrm_remove,
--
2.31.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v3 8/9] drm/simpledrm: Acquire regulators from DT device node
Make sure required hardware regulators are enabled while the firmware
framebuffer is in use.
The basic code has been taken from the simplefb driver and adapted
to DRM. Regulators are released automatically via devres helpers.
v2:
* use strscpy()
Signed-off-by: Thomas Zimmermann
Tested-by: nerdopolis
---
drivers/gpu/drm/tiny/simpledrm.c | 128 +++
1 file changed, 128 insertions(+)
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 996318500abf..9d522473cd7c 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -4,6 +4,7 @@
#include
#include
#include
+#include
#include
#include
@@ -197,6 +198,11 @@ struct simpledrm_device {
unsigned int clk_count;
struct clk **clks;
#endif
+ /* regulators */
+#if defined CONFIG_OF && defined CONFIG_REGULATOR
+ unsigned int regulator_count;
+ struct regulator **regulators;
+#endif
/* simplefb settings */
struct drm_display_mode mode;
@@ -316,6 +322,125 @@ static int simpledrm_device_init_clocks(struct
simpledrm_device *sdev)
}
#endif
+#if defined CONFIG_OF && defined CONFIG_REGULATOR
+
+#define SUPPLY_SUFFIX "-supply"
+
+/*
+ * Regulator handling code.
+ *
+ * Here we handle the num-supplies and vin*-supply properties of our
+ * "simple-framebuffer" dt node. This is necessary so that we can make sure
+ * that any regulators needed by the display hardware that the bootloader
+ * set up for us (and for which it provided a simplefb dt node), stay up,
+ * for the life of the simplefb driver.
+ *
+ * When the driver unloads, we cleanly disable, and then release the
+ * regulators.
+ *
+ * We only complain about errors here, no action is taken as the most likely
+ * error can only happen due to a mismatch between the bootloader which set
+ * up simplefb, and the regulator definitions in the device tree. Chances are
+ * that there are no adverse effects, and if there are, a clean teardown of
+ * the fb probe will not help us much either. So just complain and carry on,
+ * and hope that the user actually gets a working fb at the end of things.
+ */
+
+static void simpledrm_device_release_regulators(void *res)
+{
+ struct simpledrm_device *sdev = simpledrm_device_of_dev(res);
+ unsigned int i;
+
+ for (i = 0; i < sdev->regulator_count; ++i) {
+ if (sdev->regulators[i]) {
+ regulator_disable(sdev->regulators[i]);
+ regulator_put(sdev->regulators[i]);
+ }
+ }
+}
+
+static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
+{
+ struct drm_device *dev = >dev;
+ struct platform_device *pdev = sdev->pdev;
+ struct device_node *of_node = pdev->dev.of_node;
+ struct property *prop;
+ struct regulator *regulator;
+ const char *p;
+ unsigned int count = 0, i = 0;
+ int ret;
+
+ if (dev_get_platdata(>dev) || !of_node)
+ return 0;
+
+ /* Count the number of regulator supplies */
+ for_each_property_of_node(of_node, prop) {
+ p = strstr(prop->name, SUPPLY_SUFFIX);
+ if (p && p != prop->name)
+ ++count;
+ }
+
+ if (!count)
+ return 0;
+
+ sdev->regulators = drmm_kzalloc(dev,
+ count * sizeof(sdev->regulators[0]),
+ GFP_KERNEL);
+ if (!sdev->regulators)
+ return -ENOMEM;
+
+ for_each_property_of_node(of_node, prop) {
+ char name[32]; /* 32 is max size of property name */
+ size_t len;
+
+ p = strstr(prop->name, SUPPLY_SUFFIX);
+ if (!p || p == prop->name)
+ continue;
+ len = strlen(prop->name) - strlen(SUPPLY_SUFFIX) + 1;
+ strscpy(name, prop->name, min(sizeof(name), len));
+
+ regulator = regulator_get_optional(>dev, name);
+ if (IS_ERR(regulator)) {
+ ret = PTR_ERR(regulator);
+ if (ret == -EPROBE_DEFER)
+ goto err;
+ drm_err(dev, "regulator %s not found: %d\n",
+ name, ret);
+ continue;
+ }
+
+ ret = regulator_enable(regulator);
+ if (ret) {
+ drm_err(dev, "failed to enable regulator %u: %d\n",
+ i, ret);
+ regulator_put(regulator);
+ }
+
+ sdev->regulators[i++] = regulator;
+ }
+ sdev->regulator_count = i;
+
+ return devm_add_action_or_reset(>dev,
+ simpledrm_device_release_regulators,
+ sdev);
+
+err:
+ while (i) {
+
[PATCH v3 5/9] drm/simpledrm: Add fbdev emulation
This displays a console on simpledrm's framebuffer. The default framebuffer format is being used. Signed-off-by: Thomas Zimmermann Reviewed-by: Daniel Vetter Tested-by: nerdopolis --- drivers/gpu/drm/tiny/simpledrm.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c index 0473a90a4024..64e8a8581d9a 100644 --- a/drivers/gpu/drm/tiny/simpledrm.c +++ b/drivers/gpu/drm/tiny/simpledrm.c @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -518,6 +519,8 @@ static int simpledrm_probe(struct platform_device *pdev) if (ret) return ret; + drm_fbdev_generic_setup(dev, 0); + return 0; } -- 2.31.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v3 1/9] drm/format-helper: Pass destination pitch to drm_fb_memcpy_dstclip()
The memcpy's destination buffer might have a different pitch than the
source. Support different pitches as function argument.
Signed-off-by: Thomas Zimmermann
Reviewed-by: Daniel Vetter
Tested-by: nerdopolis
---
drivers/gpu/drm/drm_format_helper.c| 9 +
drivers/gpu/drm/mgag200/mgag200_mode.c | 2 +-
drivers/gpu/drm/tiny/cirrus.c | 2 +-
include/drm/drm_format_helper.h| 2 +-
4 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_format_helper.c
b/drivers/gpu/drm/drm_format_helper.c
index c043ca364c86..8d5a683afea7 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -52,6 +52,7 @@ EXPORT_SYMBOL(drm_fb_memcpy);
/**
* drm_fb_memcpy_dstclip - Copy clip buffer
* @dst: Destination buffer (iomem)
+ * @dst_pitch: Number of bytes between two consecutive scanlines within dst
* @vaddr: Source buffer
* @fb: DRM framebuffer
* @clip: Clip rectangle area to copy
@@ -59,12 +60,12 @@ EXPORT_SYMBOL(drm_fb_memcpy);
* This function applies clipping on dst, i.e. the destination is a
* full (iomem) framebuffer but only the clip rect content is copied over.
*/
-void drm_fb_memcpy_dstclip(void __iomem *dst, void *vaddr,
- struct drm_framebuffer *fb,
+void drm_fb_memcpy_dstclip(void __iomem *dst, unsigned int dst_pitch,
+ void *vaddr, struct drm_framebuffer *fb,
struct drm_rect *clip)
{
unsigned int cpp = fb->format->cpp[0];
- unsigned int offset = clip_offset(clip, fb->pitches[0], cpp);
+ unsigned int offset = clip_offset(clip, dst_pitch, cpp);
size_t len = (clip->x2 - clip->x1) * cpp;
unsigned int y, lines = clip->y2 - clip->y1;
@@ -73,7 +74,7 @@ void drm_fb_memcpy_dstclip(void __iomem *dst, void *vaddr,
for (y = 0; y < lines; y++) {
memcpy_toio(dst, vaddr, len);
vaddr += fb->pitches[0];
- dst += fb->pitches[0];
+ dst += dst_pitch;
}
}
EXPORT_SYMBOL(drm_fb_memcpy_dstclip);
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c
b/drivers/gpu/drm/mgag200/mgag200_mode.c
index cece3e57fb27..9d576240faed 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -1554,7 +1554,7 @@ mgag200_handle_damage(struct mga_device *mdev, struct
drm_framebuffer *fb,
{
void *vmap = map->vaddr; /* TODO: Use mapping abstraction properly */
- drm_fb_memcpy_dstclip(mdev->vram, vmap, fb, clip);
+ drm_fb_memcpy_dstclip(mdev->vram, fb->pitches[0], vmap, fb, clip);
/* Always scanout image at VRAM offset 0 */
mgag200_set_startadd(mdev, (u32)0);
diff --git a/drivers/gpu/drm/tiny/cirrus.c b/drivers/gpu/drm/tiny/cirrus.c
index e3afb45d9a5c..42611dacde88 100644
--- a/drivers/gpu/drm/tiny/cirrus.c
+++ b/drivers/gpu/drm/tiny/cirrus.c
@@ -324,7 +324,7 @@ static int cirrus_fb_blit_rect(struct drm_framebuffer *fb,
const struct dma_buf_
return -ENODEV;
if (cirrus->cpp == fb->format->cpp[0])
- drm_fb_memcpy_dstclip(cirrus->vram,
+ drm_fb_memcpy_dstclip(cirrus->vram, fb->pitches[0],
vmap, fb, rect);
else if (fb->format->cpp[0] == 4 && cirrus->cpp == 2)
diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
index 5f9e37032468..2b5036a5fbe7 100644
--- a/include/drm/drm_format_helper.h
+++ b/include/drm/drm_format_helper.h
@@ -11,7 +11,7 @@ struct drm_rect;
void drm_fb_memcpy(void *dst, void *vaddr, struct drm_framebuffer *fb,
struct drm_rect *clip);
-void drm_fb_memcpy_dstclip(void __iomem *dst, void *vaddr,
+void drm_fb_memcpy_dstclip(void __iomem *dst, unsigned int dst_pitch, void
*vaddr,
struct drm_framebuffer *fb,
struct drm_rect *clip);
void drm_fb_swab(void *dst, void *src, struct drm_framebuffer *fb,
--
2.31.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v3 4/9] drm: Add simpledrm driver
The simpledrm driver is a DRM driver for simplefb framebuffers as
provided by the kernel's boot code. This driver enables basic
graphical output on many different graphics devices that are provided
by the platform (e.g., EFI, VESA, embedded framebuffers).
With the kernel's simplefb infrastructure, the kernel receives a
pre-configured framebuffer from the system (i.e., firmware, boot
loader). It creates a platform device to which simpledrm attaches.
The system's framebuffer consists of a memory range, size and format.
Based on these values, simpledrm creates a DRM devices. No actual
modesetting is possible.
v3:
* add disable function that clears screen to black (Daniel)
* set shadow buffering only for fbdev emulation
* set platform-driver data during device creation
v2:
* rename driver to simpledrm
* add dri-devel to MAINTAINERS entry
* put native format first in primary-plane format list (Daniel)
* inline simplekms_device_cleanup() (Daniel)
* use helpers for shadow-buffered planes
* fix whitespace errors
Signed-off-by: Thomas Zimmermann
Tested-by: nerdopolis
---
MAINTAINERS | 7 +
drivers/gpu/drm/tiny/Kconfig | 16 +
drivers/gpu/drm/tiny/Makefile| 1 +
drivers/gpu/drm/tiny/simpledrm.c | 545 +++
4 files changed, 569 insertions(+)
create mode 100644 drivers/gpu/drm/tiny/simpledrm.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 7c45120759e6..4935776250e4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5765,6 +5765,13 @@ S: Orphan / Obsolete
F: drivers/gpu/drm/savage/
F: include/uapi/drm/savage_drm.h
+DRM DRIVER FOR SIMPLE FRAMEBUFFERS
+M: Thomas Zimmermann
+L: dri-de...@lists.freedesktop.org
+S: Maintained
+T: git git://anongit.freedesktop.org/drm/drm-misc
+F: drivers/gpu/drm/tiny/simplekms.c
+
DRM DRIVER FOR SIS VIDEO CARDS
S: Orphan / Obsolete
F: drivers/gpu/drm/sis/
diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
index 9bbaa1a69050..d46f95d9196d 100644
--- a/drivers/gpu/drm/tiny/Kconfig
+++ b/drivers/gpu/drm/tiny/Kconfig
@@ -38,6 +38,22 @@ config DRM_GM12U320
This is a KMS driver for projectors which use the GM12U320 chipset
for video transfer over USB2/3, such as the Acer C120 mini projector.
+config DRM_SIMPLEDRM
+ tristate "Simple framebuffer driver"
+ depends on DRM
+ select DRM_GEM_SHMEM_HELPER
+ select DRM_KMS_HELPER
+ help
+ DRM driver for simple platform-provided framebuffers.
+
+ This driver assumes that the display hardware has been initialized
+ by the firmware or bootloader before the kernel boots. Scanout
+ buffer, size, and display format must be provided via device tree,
+ UEFI, VESA, etc.
+
+ On x86 and compatible, you should also select CONFIG_X86_SYSFB to
+ use UEFI and VESA framebuffers.
+
config TINYDRM_HX8357D
tristate "DRM support for HX8357D display panels"
depends on DRM && SPI
diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile
index bef6780bdd6f..9cc847e756da 100644
--- a/drivers/gpu/drm/tiny/Makefile
+++ b/drivers/gpu/drm/tiny/Makefile
@@ -3,6 +3,7 @@
obj-$(CONFIG_DRM_ARCPGU) += arcpgu.o
obj-$(CONFIG_DRM_CIRRUS_QEMU) += cirrus.o
obj-$(CONFIG_DRM_GM12U320) += gm12u320.o
+obj-$(CONFIG_DRM_SIMPLEDRM)+= simpledrm.o
obj-$(CONFIG_TINYDRM_HX8357D) += hx8357d.o
obj-$(CONFIG_TINYDRM_ILI9225) += ili9225.o
obj-$(CONFIG_TINYDRM_ILI9341) += ili9341.o
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
new file mode 100644
index ..0473a90a4024
--- /dev/null
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -0,0 +1,545 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define DRIVER_NAME"simpledrm"
+#define DRIVER_DESC"DRM driver for simple-framebuffer platform devices"
+#define DRIVER_DATE"20200625"
+#define DRIVER_MAJOR 1
+#define DRIVER_MINOR 0
+
+/*
+ * Assume a monitor resolution of 96 dpi to
+ * get a somewhat reasonable screen size.
+ */
+#define RES_MM(d) \
+ (((d) * 254ul) / (96ul * 10ul))
+
+#define SIMPLEDRM_MODE(hd, vd) \
+ DRM_SIMPLE_MODE(hd, vd, RES_MM(hd), RES_MM(vd))
+
+/*
+ * Helpers for simplefb
+ */
+
+static int
+simplefb_get_validated_int(struct drm_device *dev, const char *name,
+ uint32_t value)
+{
+ if (value > INT_MAX) {
+ drm_err(dev, "simplefb: invalid framebuffer %s of %u\n",
+ name, value);
+ return -EINVAL;
+ }
+ return (int)value;
+}
+
+static int
+simplefb_get_validated_int0(struct drm_device *dev, const char
[PATCH v3 7/9] drm/simpledrm: Acquire clocks from DT device node
Make sure required hardware clocks are enabled while the firmware
framebuffer is in use.
The basic code has been taken from the simplefb driver and adapted
to DRM. Clocks are released automatically via devres helpers.
Signed-off-by: Thomas Zimmermann
Tested-by: nerdopolis
---
drivers/gpu/drm/tiny/simpledrm.c | 108 +++
1 file changed, 108 insertions(+)
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 53d6bec7d0b2..996318500abf 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -1,5 +1,7 @@
// SPDX-License-Identifier: GPL-2.0-only
+#include
+#include
#include
#include
@@ -190,6 +192,12 @@ struct simpledrm_device {
struct drm_device dev;
struct platform_device *pdev;
+ /* clocks */
+#if defined CONFIG_OF && defined CONFIG_COMMON_CLK
+ unsigned int clk_count;
+ struct clk **clks;
+#endif
+
/* simplefb settings */
struct drm_display_mode mode;
const struct drm_format_info *format;
@@ -211,6 +219,103 @@ static struct simpledrm_device
*simpledrm_device_of_dev(struct drm_device *dev)
return container_of(dev, struct simpledrm_device, dev);
}
+/*
+ * Hardware
+ */
+
+#if defined CONFIG_OF && defined CONFIG_COMMON_CLK
+/*
+ * Clock handling code.
+ *
+ * Here we handle the clocks property of our "simple-framebuffer" dt node.
+ * This is necessary so that we can make sure that any clocks needed by
+ * the display engine that the bootloader set up for us (and for which it
+ * provided a simplefb dt node), stay up, for the life of the simplefb
+ * driver.
+ *
+ * When the driver unloads, we cleanly disable, and then release the clocks.
+ *
+ * We only complain about errors here, no action is taken as the most likely
+ * error can only happen due to a mismatch between the bootloader which set
+ * up simplefb, and the clock definitions in the device tree. Chances are
+ * that there are no adverse effects, and if there are, a clean teardown of
+ * the fb probe will not help us much either. So just complain and carry on,
+ * and hope that the user actually gets a working fb at the end of things.
+ */
+
+static void simpledrm_device_release_clocks(void *res)
+{
+ struct simpledrm_device *sdev = simpledrm_device_of_dev(res);
+ unsigned int i;
+
+ for (i = 0; i < sdev->clk_count; ++i) {
+ if (sdev->clks[i]) {
+ clk_disable_unprepare(sdev->clks[i]);
+ clk_put(sdev->clks[i]);
+ }
+ }
+}
+
+static int simpledrm_device_init_clocks(struct simpledrm_device *sdev)
+{
+ struct drm_device *dev = >dev;
+ struct platform_device *pdev = sdev->pdev;
+ struct device_node *of_node = pdev->dev.of_node;
+ struct clk *clock;
+ unsigned int i;
+ int ret;
+
+ if (dev_get_platdata(>dev) || !of_node)
+ return 0;
+
+ sdev->clk_count = of_clk_get_parent_count(of_node);
+ if (!sdev->clk_count)
+ return 0;
+
+ sdev->clks = drmm_kzalloc(dev, sdev->clk_count * sizeof(sdev->clks[0]),
+ GFP_KERNEL);
+ if (!sdev->clks)
+ return -ENOMEM;
+
+ for (i = 0; i < sdev->clk_count; ++i) {
+ clock = of_clk_get(of_node, i);
+ if (IS_ERR(clock)) {
+ ret = PTR_ERR(clock);
+ if (ret == -EPROBE_DEFER)
+ goto err;
+ drm_err(dev, "clock %u not found: %d\n", i, ret);
+ continue;
+ }
+ ret = clk_prepare_enable(clock);
+ if (ret) {
+ drm_err(dev, "failed to enable clock %u: %d\n",
+ i, ret);
+ clk_put(clock);
+ }
+ sdev->clks[i] = clock;
+ }
+
+ return devm_add_action_or_reset(>dev,
+ simpledrm_device_release_clocks,
+ sdev);
+
+err:
+ while (i) {
+ --i;
+ if (sdev->clks[i]) {
+ clk_disable_unprepare(sdev->clks[i]);
+ clk_put(sdev->clks[i]);
+ }
+ }
+ return ret;
+}
+#else
+static int simpledrm_device_init_clocks(struct simpledrm_device *sdev)
+{
+ return 0;
+}
+#endif
+
/*
* Simplefb settings
*/
@@ -552,6 +657,9 @@ simpledrm_device_create(struct drm_driver *drv, struct
platform_device *pdev)
sdev->pdev = pdev;
platform_set_drvdata(pdev, sdev);
+ ret = simpledrm_device_init_clocks(sdev);
+ if (ret)
+ return ERR_PTR(ret);
ret = simpledrm_device_init_fb(sdev);
if (ret)
return ERR_PTR(ret);
--
2.31.1
___
Virtualization mailing list
[PATCH v3 3/9] drm/aperture: Add infrastructure for aperture ownership
Platform devices might operate on firmware framebuffers, such as VESA or
EFI. Before a native driver for the graphics hardware can take over the
device, it has to remove any platform driver that operates on the firmware
framebuffer. Aperture helpers provide the infrastructure for platform
drivers to acquire firmware framebuffers, and for native drivers to remove
them later on.
It works similar to the related fbdev mechanism. During initialization, the
platform driver acquires the firmware framebuffer's I/O memory and provides
a callback to be removed. The native driver later uses this information to
remove any platform driver for it's framebuffer I/O memory.
The aperture removal code is integrated into the existing code for removing
conflicting framebuffers, so native drivers use it automatically.
v3:
* rebase onto existing aperture infrastructure
* release aperture from list during detach; fix dangling apertures
* don't export struct drm_aperture
* document struct drm_aperture_funcs
v2:
* rename plaform helpers to aperture helpers
* tie to device lifetime with devm_ functions
* removed unsued remove() callback
* rename kickout to detach
* make struct drm_aperture private
* rebase onto existing drm_aperture.h header file
* use MIT license only for simplicity
* documentation
Signed-off-by: Thomas Zimmermann
Acked-by: Daniel Vetter
Tested-by: nerdopolis
---
drivers/gpu/drm/drm_aperture.c | 216 -
include/drm/drm_aperture.h | 36 +++---
2 files changed, 232 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/drm_aperture.c b/drivers/gpu/drm/drm_aperture.c
index e034dd7f9b09..aeddf125d2b4 100644
--- a/drivers/gpu/drm/drm_aperture.c
+++ b/drivers/gpu/drm/drm_aperture.c
@@ -1,9 +1,17 @@
// SPDX-License-Identifier: MIT
+#include
#include
+#include
+#include
+#include
+#include
+#include
#include
#include
+#include
+#include
/**
* DOC: overview
@@ -62,8 +70,196 @@
* framebuffer apertures automatically. Device drivers without knowledge of
* the framebuffer's location shall call drm_aperture_remove_framebuffers(),
* which removes all drivers for known framebuffer.
+ *
+ * Drivers that are susceptible to being removed by other drivers, such as
+ * generic EFI or VESA drivers, have to register themselves as owners of their
+ * given framebuffer memory. Ownership of the framebuffer memory is achived
+ * by calling devm_aperture_acquire(). On success, the driver is the owner
+ * of the framebuffer range. The function fails if the framebuffer is already
+ * by another driver. See below for an example.
+ *
+ * .. code-block:: c
+ *
+ * static struct drm_aperture_funcs ap_funcs = {
+ * .detach = ...
+ * };
+ *
+ * static int acquire_framebuffers(struct drm_device *dev, struct pci_dev
*pdev)
+ * {
+ * resource_size_t start, len;
+ * struct drm_aperture *ap;
+ *
+ * base = pci_resource_start(pdev, 0);
+ * size = pci_resource_len(pdev, 0);
+ *
+ * ap = devm_acquire_aperture(dev, base, size, _funcs);
+ * if (IS_ERR(ap))
+ * return PTR_ERR(ap);
+ *
+ * return 0;
+ * }
+ *
+ * static int probe(struct pci_dev *pdev)
+ * {
+ * struct drm_device *dev;
+ * int ret;
+ *
+ * // ... Initialize the device...
+ * dev = devm_drm_dev_alloc();
+ * ...
+ *
+ * // ... and acquire ownership of the framebuffer.
+ * ret = acquire_framebuffers(dev, pdev);
+ * if (ret)
+ * return ret;
+ *
+ * drm_dev_register();
+ *
+ * return 0;
+ * }
+ *
+ * The generic driver is now subject to forced removal by other drivers. This
+ * is when the detach function in struct _aperture_funcs comes into play.
+ * When a driver calls drm_fb_helper_remove_conflicting_framebuffers() et al
+ * for the registered framebuffer range, the DRM core calls struct
+ * _aperture_funcs.detach and the generic driver has to onload itself. It
+ * may not access the device's registers, framebuffer memory, ROM, etc after
+ * detach returned. If the driver supports hotplugging, detach can be treated
+ * like an unplug event.
+ *
+ * .. code-block:: c
+ *
+ * static void detach_from_device(struct drm_device *dev,
+ *resource_size_t base,
+ *resource_size_t size)
+ * {
+ * // Signal unplug
+ * drm_dev_unplug(dev);
+ *
+ * // Maybe do other clean-up operations
+ * ...
+ * }
+ *
+ * static struct drm_aperture_funcs ap_funcs = {
+ * .detach = detach_from_device,
+ * };
*/
+/*
+ * struct drm_aperture - Represents a DRM framebuffer aperture
+ */
+struct drm_aperture {
+
Re: [PATCH v6 10/10] Documentation: Add documentation for VDUSE
在 2021/4/15 下午3:19, Stefan Hajnoczi 写道:
On Thu, Apr 15, 2021 at 01:38:37PM +0800, Yongji Xie wrote:
On Wed, Apr 14, 2021 at 10:15 PM Stefan Hajnoczi wrote:
On Wed, Mar 31, 2021 at 04:05:19PM +0800, Xie Yongji wrote:
VDUSE (vDPA Device in Userspace) is a framework to support
implementing software-emulated vDPA devices in userspace. This
document is intended to clarify the VDUSE design and usage.
Signed-off-by: Xie Yongji
---
Documentation/userspace-api/index.rst | 1 +
Documentation/userspace-api/vduse.rst | 212 ++
2 files changed, 213 insertions(+)
create mode 100644 Documentation/userspace-api/vduse.rst
Just looking over the documentation briefly (I haven't studied the code
yet)...
Thank you!
+How VDUSE works
+
+Each userspace vDPA device is created by the VDUSE_CREATE_DEV ioctl on
+the character device (/dev/vduse/control). Then a device file with the
+specified name (/dev/vduse/$NAME) will appear, which can be used to
+implement the userspace vDPA device's control path and data path.
These steps are taken after sending the VDPA_CMD_DEV_NEW netlink
message? (Please consider reordering the documentation to make it clear
what the sequence of steps are.)
No, VDUSE devices should be created before sending the
VDPA_CMD_DEV_NEW netlink messages which might produce I/Os to VDUSE.
I see. Please include an overview of the steps before going into detail.
Something like:
VDUSE devices are started as follows:
1. Create a new VDUSE instance with ioctl(VDUSE_CREATE_DEV) on
/dev/vduse/control.
2. Begin processing VDUSE messages from /dev/vduse/$NAME. The first
messages will arrive while attaching the VDUSE instance to vDPA.
3. Send the VDPA_CMD_DEV_NEW netlink message to attach the VDUSE
instance to vDPA.
VDUSE devices are stopped as follows:
...
+ static int netlink_add_vduse(const char *name, int device_id)
+ {
+ struct nl_sock *nlsock;
+ struct nl_msg *msg;
+ int famid;
+
+ nlsock = nl_socket_alloc();
+ if (!nlsock)
+ return -ENOMEM;
+
+ if (genl_connect(nlsock))
+ goto free_sock;
+
+ famid = genl_ctrl_resolve(nlsock, VDPA_GENL_NAME);
+ if (famid < 0)
+ goto close_sock;
+
+ msg = nlmsg_alloc();
+ if (!msg)
+ goto close_sock;
+
+ if (!genlmsg_put(msg, NL_AUTO_PORT, NL_AUTO_SEQ, famid, 0, 0,
+ VDPA_CMD_DEV_NEW, 0))
+ goto nla_put_failure;
+
+ NLA_PUT_STRING(msg, VDPA_ATTR_DEV_NAME, name);
+ NLA_PUT_STRING(msg, VDPA_ATTR_MGMTDEV_DEV_NAME, "vduse");
+ NLA_PUT_U32(msg, VDPA_ATTR_DEV_ID, device_id);
What are the permission/capability requirements for VDUSE?
Now I think we need privileged permission (root user). Because
userspace daemon is able to access avail vring, used vring, descriptor
table in kernel driver directly.
Please state this explicitly at the start of the document. Existing
interfaces like FUSE are designed to avoid trusting userspace.
There're some subtle difference here. VDUSE present a device to kernel
which means IOMMU is probably the only thing to prevent a malicous device.
Therefore
people might think the same is the case here. It's critical that people
are aware of this before deploying VDUSE with virtio-vdpa.
We should probably pause here and think about whether it's possible to
avoid trusting userspace. Even if it takes some effort and costs some
performance it would probably be worthwhile.
Since the bounce buffer is used the only attack surface is the coherent
area, if we want to enforce stronger isolation we need to use shadow
virtqueue (which is proposed in earlier version by me) in this case. But
I'm not sure it's worth to do that.
Is the security situation different with vhost-vdpa? In that case it
seems more likely that the host kernel doesn't need to trust the
userspace VDUSE device.
Regarding privileges in general: userspace VDUSE processes shouldn't
need to run as root. The VDUSE device lifecycle will require privileges
to attach vhost-vdpa and virtio-vdpa devices, but the actual userspace
process that emulates the device should be able to run unprivileged.
Emulated devices are an attack surface and even if you are comfortable
with running them as root in your specific use case, it will be an issue
as soon as other people want to use VDUSE and could give VDUSE a
reputation for poor security.
In this case, I think it works as other char device:
- privilleged process to create and destroy the VDUSE
- fd is passed via SCM_RIGHTS to unprivilleged process that implements
the device
How does VDUSE interact with namespaces?
Not sure I get your point here. Do you mean how the emulated vDPA
device interact with namespaces? This should work like hardware vDPA
Re: [PATCH 3/3] vDPA/ifcvf: get_config_size should return dev specific config size
On Thu, Apr 15, 2021 at 04:23:15PM +0800, Zhu Lingshan wrote:
On 4/15/2021 4:12 PM, Stefano Garzarella wrote:
On Wed, Apr 14, 2021 at 05:18:32PM +0800, Zhu Lingshan wrote:
get_config_size() should return the size based on the decected
device type.
Signed-off-by: Zhu Lingshan
---
drivers/vdpa/ifcvf/ifcvf_main.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
b/drivers/vdpa/ifcvf/ifcvf_main.c
index 9b6a38b798fa..b48b9789b69e 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -347,7 +347,16 @@ static u32 ifcvf_vdpa_get_vq_align(struct
vdpa_device *vdpa_dev)
static size_t ifcvf_vdpa_get_config_size(struct vdpa_device *vdpa_dev)
{
- return sizeof(struct virtio_net_config);
+ struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
+ size_t size;
+
+ if (vf->dev_type == VIRTIO_ID_NET)
+ size = sizeof(struct virtio_net_config);
+
+ if (vf->dev_type == VIRTIO_ID_BLOCK)
+ size = sizeof(struct virtio_blk_config);
+
+ return size;
I'm not familiar with the ifcvf details, but can it happen that the
device is not block or net?
Should we set `size` to 0 by default to handle this case or are we
sure it's one of the two?
Maybe we should add a comment or a warning message in this case, to
prevent some analysis tool or compiler from worrying that `size`
might be uninitialized.
I was thinking something like this:
switch(vf->dev_type) {
case VIRTIO_ID_NET:
size = sizeof(struct virtio_net_config);
break;
case VIRTIO_ID_BLOCK:
size = sizeof(struct virtio_blk_config);
break;
default:
/* or WARN(1, "") if dev_warn() not apply */
dev_warn(... , "virtio ID [0x%x] not supported\n")
size = 0;
}
Thanks,
Stefano
agree, will add this in V2
Great, maybe something similar also in patch 2/3 for `features` in
ifcvf_vdpa_get_features().
Thanks,
Stefano
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 3/3] vDPA/ifcvf: get_config_size should return dev specific config size
On 4/15/2021 4:12 PM, Stefano Garzarella wrote:
On Wed, Apr 14, 2021 at 05:18:32PM +0800, Zhu Lingshan wrote:
get_config_size() should return the size based on the decected
device type.
Signed-off-by: Zhu Lingshan
---
drivers/vdpa/ifcvf/ifcvf_main.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
b/drivers/vdpa/ifcvf/ifcvf_main.c
index 9b6a38b798fa..b48b9789b69e 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -347,7 +347,16 @@ static u32 ifcvf_vdpa_get_vq_align(struct
vdpa_device *vdpa_dev)
static size_t ifcvf_vdpa_get_config_size(struct vdpa_device *vdpa_dev)
{
- return sizeof(struct virtio_net_config);
+ struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
+ size_t size;
+
+ if (vf->dev_type == VIRTIO_ID_NET)
+ size = sizeof(struct virtio_net_config);
+
+ if (vf->dev_type == VIRTIO_ID_BLOCK)
+ size = sizeof(struct virtio_blk_config);
+
+ return size;
I'm not familiar with the ifcvf details, but can it happen that the
device is not block or net?
Should we set `size` to 0 by default to handle this case or are we
sure it's one of the two?
Maybe we should add a comment or a warning message in this case, to
prevent some analysis tool or compiler from worrying that `size` might
be uninitialized.
I was thinking something like this:
switch(vf->dev_type) {
case VIRTIO_ID_NET:
size = sizeof(struct virtio_net_config);
break;
case VIRTIO_ID_BLOCK:
size = sizeof(struct virtio_blk_config);
break;
default:
/* or WARN(1, "") if dev_warn() not apply */
dev_warn(... , "virtio ID [0x%x] not supported\n")
size = 0;
}
Thanks,
Stefano
agree, will add this in V2
Thanks
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
On 2021/4/15 16:18, Wolfram Sang wrote: On Thu, Apr 15, 2021 at 04:15:07PM +0800, Jie Deng wrote: On 2021/4/15 15:28, Wolfram Sang wrote: Now that we were able to catch you, I will use the opportunity to clarify the doubts I had. - struct mutex lock in struct virtio_i2c, I don't think this is required since the core takes care of locking in absence of this. This is likely correct. OK. Then I will remove the lock. Let me have a look first, please. Sure. Thank you. - Use of I2C_CLASS_DEPRECATED flag, I don't think it is required for new drivers. This is definately correct :) Do you mean a new driver doesn't need to set the following ? vi->adap.class = I2C_CLASS_DEPRECATED; Just leave the class to be 0 ? Yes. DEPRECATED is only for drivers which used to have a class and then chose to remove it. Got it. Thanks for your clarification. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 3/3] vDPA/ifcvf: get_config_size should return dev specific config size
在 2021/4/15 下午4:12, Stefano Garzarella 写道:
On Wed, Apr 14, 2021 at 05:18:32PM +0800, Zhu Lingshan wrote:
get_config_size() should return the size based on the decected
device type.
Signed-off-by: Zhu Lingshan
---
drivers/vdpa/ifcvf/ifcvf_main.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
b/drivers/vdpa/ifcvf/ifcvf_main.c
index 9b6a38b798fa..b48b9789b69e 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -347,7 +347,16 @@ static u32 ifcvf_vdpa_get_vq_align(struct
vdpa_device *vdpa_dev)
static size_t ifcvf_vdpa_get_config_size(struct vdpa_device *vdpa_dev)
{
- return sizeof(struct virtio_net_config);
+ struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
+ size_t size;
+
+ if (vf->dev_type == VIRTIO_ID_NET)
+ size = sizeof(struct virtio_net_config);
+
+ if (vf->dev_type == VIRTIO_ID_BLOCK)
+ size = sizeof(struct virtio_blk_config);
+
+ return size;
I'm not familiar with the ifcvf details, but can it happen that the
device is not block or net?
Should we set `size` to 0 by default to handle this case or are we
sure it's one of the two?
Maybe we should add a comment or a warning message in this case, to
prevent some analysis tool or compiler from worrying that `size` might
be uninitialized.
I was thinking something like this:
switch(vf->dev_type) {
case VIRTIO_ID_NET:
size = sizeof(struct virtio_net_config);
break;
case VIRTIO_ID_BLOCK:
size = sizeof(struct virtio_blk_config);
break;
default:
/* or WARN(1, "") if dev_warn() not apply */
dev_warn(... , "virtio ID [0x%x] not supported\n")
size = 0;
}
Yes, I agree.
Thanks
Thanks,
Stefano
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
On 2021/4/15 15:28, Wolfram Sang wrote: Now that we were able to catch you, I will use the opportunity to clarify the doubts I had. - struct mutex lock in struct virtio_i2c, I don't think this is required since the core takes care of locking in absence of this. This is likely correct. OK. Then I will remove the lock. - Use of I2C_CLASS_DEPRECATED flag, I don't think it is required for new drivers. This is definately correct :) Do you mean a new driver doesn't need to set the following ? vi->adap.class = I2C_CLASS_DEPRECATED; Just leave the class to be 0 ? Let's see if I will have more questions... OK. Thank you. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 3/3] vDPA/ifcvf: get_config_size should return dev specific config size
On Wed, Apr 14, 2021 at 05:18:32PM +0800, Zhu Lingshan wrote:
get_config_size() should return the size based on the decected
device type.
Signed-off-by: Zhu Lingshan
---
drivers/vdpa/ifcvf/ifcvf_main.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index 9b6a38b798fa..b48b9789b69e 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -347,7 +347,16 @@ static u32 ifcvf_vdpa_get_vq_align(struct vdpa_device
*vdpa_dev)
static size_t ifcvf_vdpa_get_config_size(struct vdpa_device *vdpa_dev)
{
- return sizeof(struct virtio_net_config);
+ struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
+ size_t size;
+
+ if (vf->dev_type == VIRTIO_ID_NET)
+ size = sizeof(struct virtio_net_config);
+
+ if (vf->dev_type == VIRTIO_ID_BLOCK)
+ size = sizeof(struct virtio_blk_config);
+
+ return size;
I'm not familiar with the ifcvf details, but can it happen that the
device is not block or net?
Should we set `size` to 0 by default to handle this case or are we sure
it's one of the two?
Maybe we should add a comment or a warning message in this case, to
prevent some analysis tool or compiler from worrying that `size` might
be uninitialized.
I was thinking something like this:
switch(vf->dev_type) {
case VIRTIO_ID_NET:
size = sizeof(struct virtio_net_config);
break;
case VIRTIO_ID_BLOCK:
size = sizeof(struct virtio_blk_config);
break;
default:
/* or WARN(1, "") if dev_warn() not apply */
dev_warn(... , "virtio ID [0x%x] not supported\n")
size = 0;
}
Thanks,
Stefano
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 7/7] vp_vdpa: report doorbell address
This patch reports the per vq doorbell location and size to vDPA
bus. Userspace can then map the doorbell via mmap() via vhost-vDPA bus
driver.
Signed-off-by: Jason Wang
---
drivers/vdpa/virtio_pci/vp_vdpa.c | 18 +-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c
b/drivers/vdpa/virtio_pci/vp_vdpa.c
index 98205e54d089..002b928d0ca1 100644
--- a/drivers/vdpa/virtio_pci/vp_vdpa.c
+++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
@@ -26,6 +26,7 @@ struct vp_vring {
void __iomem *notify;
char msix_name[VP_VDPA_NAME_SIZE];
struct vdpa_callback cb;
+ resource_size_t notify_pa;
int irq;
};
@@ -336,6 +337,19 @@ static void vp_vdpa_set_config_cb(struct vdpa_device *vdpa,
vp_vdpa->config_cb = *cb;
}
+static struct vdpa_notification_area
+vp_vdpa_get_vq_notification(struct vdpa_device *vdpa, u16 qid)
+{
+ struct vp_vdpa *vp_vdpa = vdpa_to_vp(vdpa);
+ struct virtio_pci_modern_device *mdev = _vdpa->mdev;
+ struct vdpa_notification_area notify;
+
+ notify.addr = vp_vdpa->vring[qid].notify_pa;
+ notify.size = mdev->notify_offset_multiplier;
+
+ return notify;
+}
+
static const struct vdpa_config_ops vp_vdpa_ops = {
.get_features = vp_vdpa_get_features,
.set_features = vp_vdpa_set_features,
@@ -343,6 +357,7 @@ static const struct vdpa_config_ops vp_vdpa_ops = {
.set_status = vp_vdpa_set_status,
.get_vq_num_max = vp_vdpa_get_vq_num_max,
.get_vq_state = vp_vdpa_get_vq_state,
+ .get_vq_notification = vp_vdpa_get_vq_notification,
.set_vq_state = vp_vdpa_set_vq_state,
.set_vq_cb = vp_vdpa_set_vq_cb,
.set_vq_ready = vp_vdpa_set_vq_ready,
@@ -416,7 +431,8 @@ static int vp_vdpa_probe(struct pci_dev *pdev, const struct
pci_device_id *id)
for (i = 0; i < vp_vdpa->queues; i++) {
vp_vdpa->vring[i].irq = VIRTIO_MSI_NO_VECTOR;
vp_vdpa->vring[i].notify =
- vp_modern_map_vq_notify(mdev, i, NULL);
+ vp_modern_map_vq_notify(mdev, i,
+ _vdpa->vring[i].notify_pa);
if (!vp_vdpa->vring[i].notify) {
dev_warn(>dev, "Fail to map vq notify %d\n", i);
goto err;
--
2.18.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 6/7] virtio-pci library: report resource address
Sometimes it might be useful to report the capability physical
address. One example is to report the physical address of the doorbell
in order to be mapped by userspace.
Signed-off-by: Jason Wang
---
drivers/vdpa/virtio_pci/vp_vdpa.c | 3 ++-
drivers/virtio/virtio_pci_modern.c | 2 +-
drivers/virtio/virtio_pci_modern_dev.c | 24 +---
include/linux/virtio_pci_modern.h | 4 +++-
4 files changed, 23 insertions(+), 10 deletions(-)
diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c
b/drivers/vdpa/virtio_pci/vp_vdpa.c
index 2afc90645660..98205e54d089 100644
--- a/drivers/vdpa/virtio_pci/vp_vdpa.c
+++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
@@ -415,7 +415,8 @@ static int vp_vdpa_probe(struct pci_dev *pdev, const struct
pci_device_id *id)
for (i = 0; i < vp_vdpa->queues; i++) {
vp_vdpa->vring[i].irq = VIRTIO_MSI_NO_VECTOR;
- vp_vdpa->vring[i].notify = vp_modern_map_vq_notify(mdev, i);
+ vp_vdpa->vring[i].notify =
+ vp_modern_map_vq_notify(mdev, i, NULL);
if (!vp_vdpa->vring[i].notify) {
dev_warn(>dev, "Fail to map vq notify %d\n", i);
goto err;
diff --git a/drivers/virtio/virtio_pci_modern.c
b/drivers/virtio/virtio_pci_modern.c
index 29607d9bd95c..722ea44e7579 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -224,7 +224,7 @@ static struct virtqueue *setup_vq(struct virtio_pci_device
*vp_dev,
virtqueue_get_avail_addr(vq),
virtqueue_get_used_addr(vq));
- vq->priv = vp_modern_map_vq_notify(mdev, index);
+ vq->priv = vp_modern_map_vq_notify(mdev, index, NULL);
if (!vq->priv) {
err = -ENOMEM;
goto err_map_notify;
diff --git a/drivers/virtio/virtio_pci_modern_dev.c
b/drivers/virtio/virtio_pci_modern_dev.c
index 9c241c9bd920..ae87b3fa8858 100644
--- a/drivers/virtio/virtio_pci_modern_dev.c
+++ b/drivers/virtio/virtio_pci_modern_dev.c
@@ -13,13 +13,14 @@
* @start: start from the capability
* @size: map size
* @len: the length that is actually mapped
+ * @pa: physical address of the capability
*
* Returns the io address of for the part of the capability
*/
static void __iomem *
vp_modern_map_capability(struct virtio_pci_modern_device *mdev, int off,
size_t minlen, u32 align, u32 start, u32 size,
-size_t *len)
+size_t *len, resource_size_t *pa)
{
struct pci_dev *dev = mdev->pci_dev;
u8 bar;
@@ -87,6 +88,9 @@ vp_modern_map_capability(struct virtio_pci_modern_device
*mdev, int off,
dev_err(>dev,
"virtio_pci: unable to map virtio %u@%u on bar %i\n",
length, offset, bar);
+ else if (pa)
+ *pa = pci_resource_start(dev, bar) + offset;
+
return p;
}
@@ -273,12 +277,12 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev)
mdev->common = vp_modern_map_capability(mdev, common,
sizeof(struct virtio_pci_common_cfg), 4,
0, sizeof(struct virtio_pci_common_cfg),
- NULL);
+ NULL, NULL);
if (!mdev->common)
goto err_map_common;
mdev->isr = vp_modern_map_capability(mdev, isr, sizeof(u8), 1,
0, 1,
-NULL);
+NULL, NULL);
if (!mdev->isr)
goto err_map_isr;
@@ -306,7 +310,8 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev)
mdev->notify_base = vp_modern_map_capability(mdev, notify,
2, 2,
0, notify_length,
->notify_len);
+>notify_len,
+>notify_pa);
if (!mdev->notify_base)
goto err_map_notify;
} else {
@@ -319,7 +324,8 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev)
if (device) {
mdev->device = vp_modern_map_capability(mdev, device, 0, 4,
0, PAGE_SIZE,
- >device_len);
+ >device_len,
+ NULL);
if (!mdev->device)
goto err_map_device;
}
@@ -595,11 +601,12 @@ static u16 vp_modern_get_queue_notify_off(struct
[PATCH 5/7] virito_pci libray: hide vp_modern_map_capability()
No user now and the capability should not be setup
externally. Instead, every access to the capability should be done via
virtio_pci_modern_device.
Signed-off-by: Jason Wang
---
drivers/virtio/virtio_pci_modern_dev.c | 10 --
include/linux/virtio_pci_modern.h | 5 -
2 files changed, 4 insertions(+), 11 deletions(-)
diff --git a/drivers/virtio/virtio_pci_modern_dev.c
b/drivers/virtio/virtio_pci_modern_dev.c
index 5a657e56b46d..9c241c9bd920 100644
--- a/drivers/virtio/virtio_pci_modern_dev.c
+++ b/drivers/virtio/virtio_pci_modern_dev.c
@@ -16,11 +16,10 @@
*
* Returns the io address of for the part of the capability
*/
-void __iomem *vp_modern_map_capability(struct virtio_pci_modern_device *mdev,
int off,
- size_t minlen,
- u32 align,
- u32 start, u32 size,
- size_t *len)
+static void __iomem *
+vp_modern_map_capability(struct virtio_pci_modern_device *mdev, int off,
+size_t minlen, u32 align, u32 start, u32 size,
+size_t *len)
{
struct pci_dev *dev = mdev->pci_dev;
u8 bar;
@@ -90,7 +89,6 @@ void __iomem *vp_modern_map_capability(struct
virtio_pci_modern_device *mdev, in
length, offset, bar);
return p;
}
-EXPORT_SYMBOL_GPL(vp_modern_map_capability);
/**
* virtio_pci_find_capability - walk capabilities to find device info.
diff --git a/include/linux/virtio_pci_modern.h
b/include/linux/virtio_pci_modern.h
index 179a2fb4bf37..e6e7072413c1 100644
--- a/include/linux/virtio_pci_modern.h
+++ b/include/linux/virtio_pci_modern.h
@@ -99,11 +99,6 @@ void vp_modern_set_queue_size(struct
virtio_pci_modern_device *mdev,
u16 vp_modern_get_queue_size(struct virtio_pci_modern_device *mdev,
u16 idx);
u16 vp_modern_get_num_queues(struct virtio_pci_modern_device *mdev);
-void __iomem *vp_modern_map_capability(struct virtio_pci_modern_device *mdev,
int off,
- size_t minlen,
- u32 align,
- u32 start, u32 size,
- size_t *len);
void *vp_modern_map_vq_notify(struct virtio_pci_modern_device *mdev,
u16 index);
int vp_modern_probe(struct virtio_pci_modern_device *mdev);
--
2.18.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 4/7] virtio_pci_modern: hide vp_modern_get_queue_notify_off()
All users (both virtio-pci library and vp_vdpa driver) has been
switched to use vp_modern_map_vq_notify(). So there's no need to
export the low level helper of vp_modern_get_queue_notify_off().
Signed-off-by: Jason Wang
---
drivers/virtio/virtio_pci_modern_dev.c | 5 ++---
include/linux/virtio_pci_modern.h | 2 --
2 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/virtio/virtio_pci_modern_dev.c
b/drivers/virtio/virtio_pci_modern_dev.c
index 28cb5847fafa..5a657e56b46d 100644
--- a/drivers/virtio/virtio_pci_modern_dev.c
+++ b/drivers/virtio/virtio_pci_modern_dev.c
@@ -584,14 +584,13 @@ EXPORT_SYMBOL_GPL(vp_modern_get_num_queues);
*
* Returns the notification offset for a virtqueue
*/
-u16 vp_modern_get_queue_notify_off(struct virtio_pci_modern_device *mdev,
- u16 index)
+static u16 vp_modern_get_queue_notify_off(struct virtio_pci_modern_device
*mdev,
+ u16 index)
{
vp_iowrite16(index, >common->queue_select);
return vp_ioread16(>common->queue_notify_off);
}
-EXPORT_SYMBOL_GPL(vp_modern_get_queue_notify_off);
/*
* vp_modern_map_vq_notify - map notification area for a
diff --git a/include/linux/virtio_pci_modern.h
b/include/linux/virtio_pci_modern.h
index 1b95d39b00fc..179a2fb4bf37 100644
--- a/include/linux/virtio_pci_modern.h
+++ b/include/linux/virtio_pci_modern.h
@@ -99,8 +99,6 @@ void vp_modern_set_queue_size(struct virtio_pci_modern_device
*mdev,
u16 vp_modern_get_queue_size(struct virtio_pci_modern_device *mdev,
u16 idx);
u16 vp_modern_get_num_queues(struct virtio_pci_modern_device *mdev);
-u16 vp_modern_get_queue_notify_off(struct virtio_pci_modern_device *mdev,
- u16 idx);
void __iomem *vp_modern_map_capability(struct virtio_pci_modern_device *mdev,
int off,
size_t minlen,
u32 align,
--
2.18.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 3/7] vp_vdpa: switch to use vp_modern_map_vq_notify()
This patch switches to use vp_vdpa to use vp_modern_map_notify().
Signed-off-by: Jason Wang
---
drivers/vdpa/virtio_pci/vp_vdpa.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c
b/drivers/vdpa/virtio_pci/vp_vdpa.c
index 1321a2fcd088..2afc90645660 100644
--- a/drivers/vdpa/virtio_pci/vp_vdpa.c
+++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
@@ -369,7 +369,6 @@ static int vp_vdpa_probe(struct pci_dev *pdev, const struct
pci_device_id *id)
struct virtio_pci_modern_device *mdev;
struct device *dev = >dev;
struct vp_vdpa *vp_vdpa;
- u16 notify_off;
int ret, i;
ret = pcim_enable_device(pdev);
@@ -415,10 +414,12 @@ static int vp_vdpa_probe(struct pci_dev *pdev, const
struct pci_device_id *id)
}
for (i = 0; i < vp_vdpa->queues; i++) {
- notify_off = vp_modern_get_queue_notify_off(mdev, i);
vp_vdpa->vring[i].irq = VIRTIO_MSI_NO_VECTOR;
- vp_vdpa->vring[i].notify = mdev->notify_base +
- notify_off * mdev->notify_offset_multiplier;
+ vp_vdpa->vring[i].notify = vp_modern_map_vq_notify(mdev, i);
+ if (!vp_vdpa->vring[i].notify) {
+ dev_warn(>dev, "Fail to map vq notify %d\n", i);
+ goto err;
+ }
}
vp_vdpa->config_irq = VIRTIO_MSI_NO_VECTOR;
--
2.18.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 2/7] virtio-pci library: switch to use vp_modern_map_vq_notify()
This patch switch to use vp_modern_map_notify() for virtio-pci
library.
Signed-off-by: Jason Wang
---
drivers/virtio/virtio_pci_modern.c | 27 ++-
1 file changed, 2 insertions(+), 25 deletions(-)
diff --git a/drivers/virtio/virtio_pci_modern.c
b/drivers/virtio/virtio_pci_modern.c
index fbd4ebc00eb6..29607d9bd95c 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -192,7 +192,7 @@ static struct virtqueue *setup_vq(struct virtio_pci_device
*vp_dev,
struct virtio_pci_modern_device *mdev = _dev->mdev;
struct virtqueue *vq;
- u16 num, off;
+ u16 num;
int err;
if (index >= vp_modern_get_num_queues(mdev))
@@ -208,9 +208,6 @@ static struct virtqueue *setup_vq(struct virtio_pci_device
*vp_dev,
return ERR_PTR(-EINVAL);
}
- /* get offset of notification word for this vq */
- off = vp_modern_get_queue_notify_off(mdev, index);
-
info->msix_vector = msix_vec;
/* create the vring */
@@ -227,27 +224,7 @@ static struct virtqueue *setup_vq(struct virtio_pci_device
*vp_dev,
virtqueue_get_avail_addr(vq),
virtqueue_get_used_addr(vq));
- if (mdev->notify_base) {
- /* offset should not wrap */
- if ((u64)off * mdev->notify_offset_multiplier + 2
- > mdev->notify_len) {
- dev_warn(>pci_dev->dev,
-"bad notification offset %u (x %u) "
-"for queue %u > %zd",
-off, mdev->notify_offset_multiplier,
-index, mdev->notify_len);
- err = -EINVAL;
- goto err_map_notify;
- }
- vq->priv = (void __force *)mdev->notify_base +
- off * mdev->notify_offset_multiplier;
- } else {
- vq->priv = (void __force *)vp_modern_map_capability(mdev,
- mdev->notify_map_cap,
2, 2,
- off *
mdev->notify_offset_multiplier, 2,
- NULL);
- }
-
+ vq->priv = vp_modern_map_vq_notify(mdev, index);
if (!vq->priv) {
err = -ENOMEM;
goto err_map_notify;
--
2.18.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 1/7] virtio_pci_modern: introduce helper to map vq notify area
This patch factors out the logic of vq notify area mapping. Following
patches will switch to use this common helpers for both virtio_pci
library and virtio-pci vDPA driver.
Signed-off-by: Jason Wang
---
drivers/virtio/virtio_pci_modern_dev.c | 35 ++
include/linux/virtio_pci_modern.h | 2 ++
2 files changed, 37 insertions(+)
diff --git a/drivers/virtio/virtio_pci_modern_dev.c
b/drivers/virtio/virtio_pci_modern_dev.c
index cbd667496bb1..28cb5847fafa 100644
--- a/drivers/virtio/virtio_pci_modern_dev.c
+++ b/drivers/virtio/virtio_pci_modern_dev.c
@@ -593,6 +593,41 @@ u16 vp_modern_get_queue_notify_off(struct
virtio_pci_modern_device *mdev,
}
EXPORT_SYMBOL_GPL(vp_modern_get_queue_notify_off);
+/*
+ * vp_modern_map_vq_notify - map notification area for a
+ * specific virtqueue
+ * @mdev: the modern virtio-pci device
+ * @index: the queue index
+ *
+ * Returns the address of the notification area
+ */
+void *vp_modern_map_vq_notify(struct virtio_pci_modern_device *mdev,
+ u16 index)
+{
+ u16 off = vp_modern_get_queue_notify_off(mdev, index);
+
+ if (mdev->notify_base) {
+ /* offset should not wrap */
+ if ((u64)off * mdev->notify_offset_multiplier + 2
+ > mdev->notify_len) {
+ dev_warn(>pci_dev->dev,
+"bad notification offset %u (x %u) "
+"for queue %u > %zd",
+off, mdev->notify_offset_multiplier,
+index, mdev->notify_len);
+ return NULL;
+ }
+ return (void __force *)mdev->notify_base +
+ off * mdev->notify_offset_multiplier;
+ } else {
+ return (void __force *)vp_modern_map_capability(mdev,
+ mdev->notify_map_cap, 2, 2,
+ off * mdev->notify_offset_multiplier, 2,
+ NULL);
+ }
+}
+EXPORT_SYMBOL_GPL(vp_modern_map_vq_notify);
+
MODULE_VERSION("0.1");
MODULE_DESCRIPTION("Modern Virtio PCI Device");
MODULE_AUTHOR("Jason Wang ");
diff --git a/include/linux/virtio_pci_modern.h
b/include/linux/virtio_pci_modern.h
index f26acbeec965..1b95d39b00fc 100644
--- a/include/linux/virtio_pci_modern.h
+++ b/include/linux/virtio_pci_modern.h
@@ -106,6 +106,8 @@ void __iomem *vp_modern_map_capability(struct
virtio_pci_modern_device *mdev, in
u32 align,
u32 start, u32 size,
size_t *len);
+void *vp_modern_map_vq_notify(struct virtio_pci_modern_device *mdev,
+ u16 index);
int vp_modern_probe(struct virtio_pci_modern_device *mdev);
void vp_modern_remove(struct virtio_pci_modern_device *mdev);
#endif
--
2.18.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 0/7] Doorbell mapping support for virito-pci vDPA
Hi All: This series implements the doorbell mapping support for virtio-pci vDPA driver. Tested with page-per-vq=on in a nested guest. Please review Thanks Jason Wang (7): virtio_pci_modern: introduce helper to map vq notify area virtio-pci library: switch to use vp_modern_map_vq_notify() vp_vdpa: switch to use vp_modern_map_vq_notify() virtio_pci_modern: hide vp_modern_get_queue_notify_off() virito_pci libray: hide vp_modern_map_capability() virtio-pci library: report resource address vp_vdpa: report doorbell address drivers/vdpa/virtio_pci/vp_vdpa.c | 26 -- drivers/virtio/virtio_pci_modern.c | 27 +- drivers/virtio/virtio_pci_modern_dev.c | 68 +- include/linux/virtio_pci_modern.h | 11 ++--- 4 files changed, 83 insertions(+), 49 deletions(-) -- 2.18.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 08/10] drm/simpledrm: Acquire clocks from DT device node
Hi Am 08.04.21 um 10:13 schrieb Maxime Ripard: Hi, On Thu, Mar 18, 2021 at 11:29:19AM +0100, Thomas Zimmermann wrote: Make sure required hardware clocks are enabled while the firmware framebuffer is in use. The basic code has been taken from the simplefb driver and adapted to DRM. Clocks are released automatically via devres helpers. Signed-off-by: Thomas Zimmermann Tested-by: nerdopolis Even though it's definitely simpler to review, merging the driver first and then the clocks and regulators will break bisection on the platforms that rely on them I'd like to keep the patches separate for now, but can squash patches 6 to 8 them into one before pushing them. OK? Another thing worth considering is also that both drivers will probe if they are enabled (which is pretty likely), which is not great :) I guess we should make them mutually exclusive through Kconfig We already have several drivers in fbdev and DRM that handle the same hardware. We don't do this for any other pair, why bother now? Best regards Thomas Maxime -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer OpenPGP_signature Description: OpenPGP digital signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/3] vDPA/ifcvf: enable Intel C5000X-PL virtio-block for vDPA
On 4/15/2021 3:17 PM, Jason Wang wrote: 在 2021/4/15 下午2:41, Zhu Lingshan 写道: I think we've discussed this sometime in the past but what's the reason for such whitelist consider there's already a get_features() implemention? E.g Any reason to block VIRTIO_BLK_F_WRITE_ZEROS or VIRTIO_F_RING_PACKED? Thanks The reason is some feature bits are supported in the device but not supported by the driver, e.g, for virtio-net, mq & cq implementation is not ready in the driver. I understand the case of virtio-net but I wonder why we need this for block where we don't vq cvq. Thanks This is still a subset of the feature bits read from hardware, I leave it here to code consistently, and indicate what we support clearly. Are you suggesting remove this feature bits list and just use what we read from hardware? Thansk Yes, please do that. The whiltelist doesn't help in this case I think. OK, will remove this in V2 Thanks Thanks ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/3] vDPA/ifcvf: deduce VIRTIO device ID when probe
On 4/15/2021 3:16 PM, Jason Wang wrote:
在 2021/4/15 下午2:36, Zhu Lingshan 写道:
On 4/15/2021 2:30 PM, Jason Wang wrote:
在 2021/4/15 下午1:52, Zhu Lingshan 写道:
On 4/15/2021 11:30 AM, Jason Wang wrote:
在 2021/4/14 下午5:18, Zhu Lingshan 写道:
This commit deduces VIRTIO device ID as device type when probe,
then ifcvf_vdpa_get_device_id() can simply return the ID.
ifcvf_vdpa_get_features() and ifcvf_vdpa_get_config_size()
can work properly based on the device ID.
Signed-off-by: Zhu Lingshan
---
drivers/vdpa/ifcvf/ifcvf_base.h | 1 +
drivers/vdpa/ifcvf/ifcvf_main.c | 22 ++
2 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h
b/drivers/vdpa/ifcvf/ifcvf_base.h
index b2eeb16b9c2c..1c04cd256fa7 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.h
+++ b/drivers/vdpa/ifcvf/ifcvf_base.h
@@ -84,6 +84,7 @@ struct ifcvf_hw {
u32 notify_off_multiplier;
u64 req_features;
u64 hw_features;
+ u32 dev_type;
struct virtio_pci_common_cfg __iomem *common_cfg;
void __iomem *net_cfg;
struct vring_info vring[IFCVF_MAX_QUEUE_PAIRS * 2];
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
b/drivers/vdpa/ifcvf/ifcvf_main.c
index 44d7586019da..99b0a6b4c227 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -323,19 +323,9 @@ static u32 ifcvf_vdpa_get_generation(struct
vdpa_device *vdpa_dev)
static u32 ifcvf_vdpa_get_device_id(struct vdpa_device
*vdpa_dev)
{
- struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev);
- struct pci_dev *pdev = adapter->pdev;
- u32 ret = -ENODEV;
-
- if (pdev->device < 0x1000 || pdev->device > 0x107f)
- return ret;
-
- if (pdev->device < 0x1040)
- ret = pdev->subsystem_device;
- else
- ret = pdev->device-0x1040;
+ struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
- return ret;
+ return vf->dev_type;
}
static u32 ifcvf_vdpa_get_vendor_id(struct vdpa_device
*vdpa_dev)
@@ -466,6 +456,14 @@ static int ifcvf_probe(struct pci_dev *pdev,
const struct pci_device_id *id)
pci_set_drvdata(pdev, adapter);
vf = >vf;
+ if (pdev->device < 0x1000 || pdev->device > 0x107f)
+ return -EOPNOTSUPP;
+
+ if (pdev->device < 0x1040)
+ vf->dev_type = pdev->subsystem_device;
+ else
+ vf->dev_type = pdev->device - 0x1040;
So a question here, is the device a transtional device or modern one?
If it's a transitonal one, can it swtich endianess automatically
or not?
Thanks
Hi Jason,
This driver should drive both modern and transitional devices as we
discussed before.
If it's a transitional one, it will act as a modern device by
default, legacy mode is a fail-over path.
Note that legacy driver use native endian, support legacy driver
requires the device to know native endian which I'm not sure your
device can do that.
Thanks
Yes, legacy requires guest native endianess, I think we don't need to
worry about this because our transitional device should work in
modern mode by
default(legacy mode is the failover path we will never reach,
get_features will fail if no ACCESS_PLATFORM), we don't support
legacy device in vDPA.
Thanks
Ok, so I think it's better to add a comment here.
sure, will add a comment in V2
Thanks
Thanks
For vDPA, it has to support VIRTIO_1 and ACCESS_PLATFORM, so it
must in modern mode.
I think we don't need to worry about endianess for legacy mode.
Thanks
Zhu Lingshan
+
vf->base = pcim_iomap_table(pdev);
adapter->pdev = pdev;
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: Re: [PATCH v6 10/10] Documentation: Add documentation for VDUSE
On Thu, Apr 15, 2021 at 01:38:37PM +0800, Yongji Xie wrote:
> On Wed, Apr 14, 2021 at 10:15 PM Stefan Hajnoczi wrote:
> >
> > On Wed, Mar 31, 2021 at 04:05:19PM +0800, Xie Yongji wrote:
> > > VDUSE (vDPA Device in Userspace) is a framework to support
> > > implementing software-emulated vDPA devices in userspace. This
> > > document is intended to clarify the VDUSE design and usage.
> > >
> > > Signed-off-by: Xie Yongji
> > > ---
> > > Documentation/userspace-api/index.rst | 1 +
> > > Documentation/userspace-api/vduse.rst | 212
> > > ++
> > > 2 files changed, 213 insertions(+)
> > > create mode 100644 Documentation/userspace-api/vduse.rst
> >
> > Just looking over the documentation briefly (I haven't studied the code
> > yet)...
> >
>
> Thank you!
>
> > > +How VDUSE works
> > > +
> > > +Each userspace vDPA device is created by the VDUSE_CREATE_DEV ioctl on
> > > +the character device (/dev/vduse/control). Then a device file with the
> > > +specified name (/dev/vduse/$NAME) will appear, which can be used to
> > > +implement the userspace vDPA device's control path and data path.
> >
> > These steps are taken after sending the VDPA_CMD_DEV_NEW netlink
> > message? (Please consider reordering the documentation to make it clear
> > what the sequence of steps are.)
> >
>
> No, VDUSE devices should be created before sending the
> VDPA_CMD_DEV_NEW netlink messages which might produce I/Os to VDUSE.
I see. Please include an overview of the steps before going into detail.
Something like:
VDUSE devices are started as follows:
1. Create a new VDUSE instance with ioctl(VDUSE_CREATE_DEV) on
/dev/vduse/control.
2. Begin processing VDUSE messages from /dev/vduse/$NAME. The first
messages will arrive while attaching the VDUSE instance to vDPA.
3. Send the VDPA_CMD_DEV_NEW netlink message to attach the VDUSE
instance to vDPA.
VDUSE devices are stopped as follows:
...
> > > + static int netlink_add_vduse(const char *name, int device_id)
> > > + {
> > > + struct nl_sock *nlsock;
> > > + struct nl_msg *msg;
> > > + int famid;
> > > +
> > > + nlsock = nl_socket_alloc();
> > > + if (!nlsock)
> > > + return -ENOMEM;
> > > +
> > > + if (genl_connect(nlsock))
> > > + goto free_sock;
> > > +
> > > + famid = genl_ctrl_resolve(nlsock, VDPA_GENL_NAME);
> > > + if (famid < 0)
> > > + goto close_sock;
> > > +
> > > + msg = nlmsg_alloc();
> > > + if (!msg)
> > > + goto close_sock;
> > > +
> > > + if (!genlmsg_put(msg, NL_AUTO_PORT, NL_AUTO_SEQ, famid, 0,
> > > 0,
> > > + VDPA_CMD_DEV_NEW, 0))
> > > + goto nla_put_failure;
> > > +
> > > + NLA_PUT_STRING(msg, VDPA_ATTR_DEV_NAME, name);
> > > + NLA_PUT_STRING(msg, VDPA_ATTR_MGMTDEV_DEV_NAME, "vduse");
> > > + NLA_PUT_U32(msg, VDPA_ATTR_DEV_ID, device_id);
> >
> > What are the permission/capability requirements for VDUSE?
> >
>
> Now I think we need privileged permission (root user). Because
> userspace daemon is able to access avail vring, used vring, descriptor
> table in kernel driver directly.
Please state this explicitly at the start of the document. Existing
interfaces like FUSE are designed to avoid trusting userspace. Therefore
people might think the same is the case here. It's critical that people
are aware of this before deploying VDUSE with virtio-vdpa.
We should probably pause here and think about whether it's possible to
avoid trusting userspace. Even if it takes some effort and costs some
performance it would probably be worthwhile.
Is the security situation different with vhost-vdpa? In that case it
seems more likely that the host kernel doesn't need to trust the
userspace VDUSE device.
Regarding privileges in general: userspace VDUSE processes shouldn't
need to run as root. The VDUSE device lifecycle will require privileges
to attach vhost-vdpa and virtio-vdpa devices, but the actual userspace
process that emulates the device should be able to run unprivileged.
Emulated devices are an attack surface and even if you are comfortable
with running them as root in your specific use case, it will be an issue
as soon as other people want to use VDUSE and could give VDUSE a
reputation for poor security.
> > How does VDUSE interact with namespaces?
> >
>
> Not sure I get your point here. Do you mean how the emulated vDPA
> device interact with namespaces? This should work like hardware vDPA
> devices do. VDUSE daemon can reside outside the namespace of a
> container which uses the vDPA device.
Can VDUSE devices run inside containers? Are /dev/vduse/$NAME and vDPA
device names global?
> > What is the meaning of VDPA_ATTR_DEV_ID? I don't see it in Linux
> > v5.12-rc6
Re: [PATCH 2/3] vDPA/ifcvf: enable Intel C5000X-PL virtio-block for vDPA
在 2021/4/15 下午2:41, Zhu Lingshan 写道: I think we've discussed this sometime in the past but what's the reason for such whitelist consider there's already a get_features() implemention? E.g Any reason to block VIRTIO_BLK_F_WRITE_ZEROS or VIRTIO_F_RING_PACKED? Thanks The reason is some feature bits are supported in the device but not supported by the driver, e.g, for virtio-net, mq & cq implementation is not ready in the driver. I understand the case of virtio-net but I wonder why we need this for block where we don't vq cvq. Thanks This is still a subset of the feature bits read from hardware, I leave it here to code consistently, and indicate what we support clearly. Are you suggesting remove this feature bits list and just use what we read from hardware? Thansk Yes, please do that. The whiltelist doesn't help in this case I think. Thanks ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/3] vDPA/ifcvf: deduce VIRTIO device ID when probe
在 2021/4/15 下午2:36, Zhu Lingshan 写道:
On 4/15/2021 2:30 PM, Jason Wang wrote:
在 2021/4/15 下午1:52, Zhu Lingshan 写道:
On 4/15/2021 11:30 AM, Jason Wang wrote:
在 2021/4/14 下午5:18, Zhu Lingshan 写道:
This commit deduces VIRTIO device ID as device type when probe,
then ifcvf_vdpa_get_device_id() can simply return the ID.
ifcvf_vdpa_get_features() and ifcvf_vdpa_get_config_size()
can work properly based on the device ID.
Signed-off-by: Zhu Lingshan
---
drivers/vdpa/ifcvf/ifcvf_base.h | 1 +
drivers/vdpa/ifcvf/ifcvf_main.c | 22 ++
2 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h
b/drivers/vdpa/ifcvf/ifcvf_base.h
index b2eeb16b9c2c..1c04cd256fa7 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.h
+++ b/drivers/vdpa/ifcvf/ifcvf_base.h
@@ -84,6 +84,7 @@ struct ifcvf_hw {
u32 notify_off_multiplier;
u64 req_features;
u64 hw_features;
+ u32 dev_type;
struct virtio_pci_common_cfg __iomem *common_cfg;
void __iomem *net_cfg;
struct vring_info vring[IFCVF_MAX_QUEUE_PAIRS * 2];
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
b/drivers/vdpa/ifcvf/ifcvf_main.c
index 44d7586019da..99b0a6b4c227 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -323,19 +323,9 @@ static u32 ifcvf_vdpa_get_generation(struct
vdpa_device *vdpa_dev)
static u32 ifcvf_vdpa_get_device_id(struct vdpa_device *vdpa_dev)
{
- struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev);
- struct pci_dev *pdev = adapter->pdev;
- u32 ret = -ENODEV;
-
- if (pdev->device < 0x1000 || pdev->device > 0x107f)
- return ret;
-
- if (pdev->device < 0x1040)
- ret = pdev->subsystem_device;
- else
- ret = pdev->device-0x1040;
+ struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
- return ret;
+ return vf->dev_type;
}
static u32 ifcvf_vdpa_get_vendor_id(struct vdpa_device *vdpa_dev)
@@ -466,6 +456,14 @@ static int ifcvf_probe(struct pci_dev *pdev,
const struct pci_device_id *id)
pci_set_drvdata(pdev, adapter);
vf = >vf;
+ if (pdev->device < 0x1000 || pdev->device > 0x107f)
+ return -EOPNOTSUPP;
+
+ if (pdev->device < 0x1040)
+ vf->dev_type = pdev->subsystem_device;
+ else
+ vf->dev_type = pdev->device - 0x1040;
So a question here, is the device a transtional device or modern one?
If it's a transitonal one, can it swtich endianess automatically or
not?
Thanks
Hi Jason,
This driver should drive both modern and transitional devices as we
discussed before.
If it's a transitional one, it will act as a modern device by
default, legacy mode is a fail-over path.
Note that legacy driver use native endian, support legacy driver
requires the device to know native endian which I'm not sure your
device can do that.
Thanks
Yes, legacy requires guest native endianess, I think we don't need to
worry about this because our transitional device should work in modern
mode by
default(legacy mode is the failover path we will never reach,
get_features will fail if no ACCESS_PLATFORM), we don't support legacy
device in vDPA.
Thanks
Ok, so I think it's better to add a comment here.
Thanks
For vDPA, it has to support VIRTIO_1 and ACCESS_PLATFORM, so it must
in modern mode.
I think we don't need to worry about endianess for legacy mode.
Thanks
Zhu Lingshan
+
vf->base = pcim_iomap_table(pdev);
adapter->pdev = pdev;
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 04/10] drm/aperture: Add infrastructure for aperture ownership
Hi
Am 09.04.21 um 11:22 schrieb Daniel Vetter:
Is it that easy? simepldrm's detach function has code to synchronize with
concurrent hotplug removals. If we can use drm_dev_unplug() for everything,
I'm all for it.
Uh, I should have looked at the code instead of just asking silly
questions :-)
Now I'm even more scared, and also more convinced that we're recreating
a
bad version of some of the core driver model concepts.
I think the ideal option here would be if drm_aperture could unload
(unbind really) the platform driver for us, through the driver model. Then
there's only one place that keeps track whether the driver is unbound or
not. I'm not sure whether this can be done fully generic on a struct
device, or whether we need special code for each type. Since atm we only
have simpledrm we can just specialize on platform_device and it's good
enough.
I meanwhile found that calling platform_device_unregister() is the right
thing to do. It is like a hot-unplug event. It's simple to implement and
removes the generic device as well. Any memory ranges for the generic
device are gone as well. Only the native driver's native device will
remain. That's better than the existing simplefb driver.
Which unregister function to call still driver-specific, so I kept the
callback.
Best regards
Thomas
I think best here would be to Cc: gregkh on this patch and the simpledrm
->detach implementatation, and ask for his feedback as driver model
maintainer. Maybe if you could hack together the platform_device unbind
path as proof of concept would be even better.
Either way, this is really tricky.
-Daniel
Best regards
Thomas
Or maybe we should tie this more into the struct device mode and force an
unload that way? That way devm cleanup would work as one expects, and
avoid the need for anything specific (hopefully) in this detach callback.
Just feels a bit like we're reinventing half of the driver model here,
badly.
+ * };
+ *
+ * static int acquire_framebuffers(struct drm_device *dev, struct pci_dev
*pdev)
+ * {
+ * resource_size_t start, len;
+ * struct drm_aperture *ap;
+ *
+ * base = pci_resource_start(pdev, 0);
+ * size = pci_resource_len(pdev, 0);
+ *
+ * ap = devm_acquire_aperture(dev, base, size, _funcs);
+ * if (IS_ERR(ap))
+ * return PTR_ERR(ap);
+ *
+ * return 0;
+ * }
+ *
+ * static int probe(struct pci_dev *pdev)
+ * {
+ * struct drm_device *dev;
+ * int ret;
+ *
+ * // ... Initialize the device...
+ * dev = devm_drm_dev_alloc();
+ * ...
+ *
+ * // ... and acquire ownership of the framebuffer.
+ * ret = acquire_framebuffers(dev, pdev);
+ * if (ret)
+ * return ret;
+ *
+ * drm_dev_register();
+ *
+ * return 0;
+ * }
+ *
+ * The generic driver is now subject to forced removal by other drivers. This
+ * is when the detach function in struct _aperture_funcs comes into play.
+ * When a driver calls drm_fb_helper_remove_conflicting_framebuffers() et al
+ * for the registered framebuffer range, the DRM core calls struct
+ * _aperture_funcs.detach and the generic driver has to onload itself. It
+ * may not access the device's registers, framebuffer memory, ROM, etc after
+ * detach returned. If the driver supports hotplugging, detach can be treated
+ * like an unplug event.
+ *
+ * .. code-block:: c
+ *
+ * static void detach_from_device(struct drm_device *dev,
+ *resource_size_t base,
+ *resource_size_t size)
+ * {
+ * // Signal unplug
+ * drm_dev_unplug(dev);
+ *
+ * // Maybe do other clean-up operations
+ * ...
+ * }
+ *
+ * static struct drm_aperture_funcs ap_funcs = {
+ * .detach = detach_from_device,
+ * };
+ */
+
+/**
+ * struct drm_aperture - Represents a DRM framebuffer aperture
+ *
+ * This structure has no public fields.
+ */
+struct drm_aperture {
+ struct drm_device *dev;
+ resource_size_t base;
+ resource_size_t size;
+
+ const struct drm_aperture_funcs *funcs;
+
+ struct list_head lh;
+};
+
+static LIST_HEAD(drm_apertures);
+
+static DEFINE_MUTEX(drm_apertures_lock);
+
+static bool overlap(resource_size_t base1, resource_size_t end1,
+ resource_size_t base2, resource_size_t end2)
+{
+ return (base1 < end2) && (end1 > base2);
+}
+
+static void devm_aperture_acquire_release(void *data)
+{
+ struct drm_aperture *ap = data;
+ bool detached = !ap->dev;
+
+ if (!detached)
Uh this needs a comment that if ap->dev is NULL then we're called from
drm_aperture_detach_drivers() and hence the lock is already held.
+ mutex_lock(_apertures_lock);
and an
else
Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
On 2021/4/15 14:45, Viresh Kumar wrote: On 23-03-21, 10:27, Arnd Bergmann wrote: I usually recommend the use of __maybe_unused for the suspend/resume callbacks for drivers that use SIMPLE_DEV_PM_OPS() or similar helpers that hide the exact conditions under which the functions get called. In this driver, there is an explicit #ifdef in the reference to the functions, so it would make sense to use the same #ifdef around the definition. Jie, I was talking about this comment when I said I was expecting a new version. I think you still need to make this change. I didn't forget this. It is a very small change. I'm not sure if the maintainer Wolfram has any comments so that I can address them together in one version. Thanks. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/3] vDPA/ifcvf: enable Intel C5000X-PL virtio-block for vDPA
On 4/15/2021 2:31 PM, Jason Wang wrote:
在 2021/4/15 下午1:55, Zhu Lingshan 写道:
On 4/15/2021 11:34 AM, Jason Wang wrote:
在 2021/4/14 下午5:18, Zhu Lingshan 写道:
This commit enabled Intel FPGA SmartNIC C5000X-PL virtio-block
for vDPA.
Signed-off-by: Zhu Lingshan
---
drivers/vdpa/ifcvf/ifcvf_base.h | 17 -
drivers/vdpa/ifcvf/ifcvf_main.c | 10 +-
2 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h
b/drivers/vdpa/ifcvf/ifcvf_base.h
index 1c04cd256fa7..8b403522bf06 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.h
+++ b/drivers/vdpa/ifcvf/ifcvf_base.h
@@ -15,6 +15,7 @@
#include
#include
#include
+#include
#include
#include
@@ -28,7 +29,12 @@
#define C5000X_PL_SUBSYS_VENDOR_ID 0x8086
#define C5000X_PL_SUBSYS_DEVICE_ID 0x0001
-#define IFCVF_SUPPORTED_FEATURES \
+#define C5000X_PL_BLK_VENDOR_ID 0x1AF4
+#define C5000X_PL_BLK_DEVICE_ID 0x1001
+#define C5000X_PL_BLK_SUBSYS_VENDOR_ID 0x8086
+#define C5000X_PL_BLK_SUBSYS_DEVICE_ID 0x0002
+
+#define IFCVF_NET_SUPPORTED_FEATURES \
((1ULL << VIRTIO_NET_F_MAC) | \
(1ULL << VIRTIO_F_ANY_LAYOUT) | \
(1ULL << VIRTIO_F_VERSION_1) | \
@@ -37,6 +43,15 @@
(1ULL << VIRTIO_F_ACCESS_PLATFORM) | \
(1ULL << VIRTIO_NET_F_MRG_RXBUF))
+#define IFCVF_BLK_SUPPORTED_FEATURES \
+ ((1ULL << VIRTIO_BLK_F_SIZE_MAX) | \
+ (1ULL << VIRTIO_BLK_F_SEG_MAX) | \
+ (1ULL << VIRTIO_BLK_F_BLK_SIZE) | \
+ (1ULL << VIRTIO_BLK_F_TOPOLOGY) | \
+ (1ULL << VIRTIO_BLK_F_MQ) | \
+ (1ULL << VIRTIO_F_VERSION_1) | \
+ (1ULL << VIRTIO_F_ACCESS_PLATFORM))
I think we've discussed this sometime in the past but what's the
reason for such whitelist consider there's already a get_features()
implemention?
E.g Any reason to block VIRTIO_BLK_F_WRITE_ZEROS or
VIRTIO_F_RING_PACKED?
Thanks
The reason is some feature bits are supported in the device but not
supported by the driver, e.g, for virtio-net, mq & cq implementation
is not ready in the driver.
I understand the case of virtio-net but I wonder why we need this for
block where we don't vq cvq.
Thanks
This is still a subset of the feature bits read from hardware, I leave
it here to code consistently, and indicate what we support clearly.
Are you suggesting remove this feature bits list and just use what we
read from hardware?
Thansk
Thanks!
+
/* Only one queue pair for now. */
#define IFCVF_MAX_QUEUE_PAIRS 1
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
b/drivers/vdpa/ifcvf/ifcvf_main.c
index 99b0a6b4c227..9b6a38b798fa 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -171,7 +171,11 @@ static u64 ifcvf_vdpa_get_features(struct
vdpa_device *vdpa_dev)
struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
u64 features;
- features = ifcvf_get_features(vf) & IFCVF_SUPPORTED_FEATURES;
+ if (vf->dev_type == VIRTIO_ID_NET)
+ features = ifcvf_get_features(vf) &
IFCVF_NET_SUPPORTED_FEATURES;
+
+ if (vf->dev_type == VIRTIO_ID_BLOCK)
+ features = ifcvf_get_features(vf) &
IFCVF_BLK_SUPPORTED_FEATURES;
return features;
}
@@ -509,6 +513,10 @@ static struct pci_device_id ifcvf_pci_ids[] = {
C5000X_PL_DEVICE_ID,
C5000X_PL_SUBSYS_VENDOR_ID,
C5000X_PL_SUBSYS_DEVICE_ID) },
+ { PCI_DEVICE_SUB(C5000X_PL_BLK_VENDOR_ID,
+ C5000X_PL_BLK_DEVICE_ID,
+ C5000X_PL_BLK_SUBSYS_VENDOR_ID,
+ C5000X_PL_BLK_SUBSYS_DEVICE_ID) },
{ 0 },
};
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/3] vDPA/ifcvf: deduce VIRTIO device ID when probe
On 4/15/2021 2:30 PM, Jason Wang wrote:
在 2021/4/15 下午1:52, Zhu Lingshan 写道:
On 4/15/2021 11:30 AM, Jason Wang wrote:
在 2021/4/14 下午5:18, Zhu Lingshan 写道:
This commit deduces VIRTIO device ID as device type when probe,
then ifcvf_vdpa_get_device_id() can simply return the ID.
ifcvf_vdpa_get_features() and ifcvf_vdpa_get_config_size()
can work properly based on the device ID.
Signed-off-by: Zhu Lingshan
---
drivers/vdpa/ifcvf/ifcvf_base.h | 1 +
drivers/vdpa/ifcvf/ifcvf_main.c | 22 ++
2 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h
b/drivers/vdpa/ifcvf/ifcvf_base.h
index b2eeb16b9c2c..1c04cd256fa7 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.h
+++ b/drivers/vdpa/ifcvf/ifcvf_base.h
@@ -84,6 +84,7 @@ struct ifcvf_hw {
u32 notify_off_multiplier;
u64 req_features;
u64 hw_features;
+ u32 dev_type;
struct virtio_pci_common_cfg __iomem *common_cfg;
void __iomem *net_cfg;
struct vring_info vring[IFCVF_MAX_QUEUE_PAIRS * 2];
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
b/drivers/vdpa/ifcvf/ifcvf_main.c
index 44d7586019da..99b0a6b4c227 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -323,19 +323,9 @@ static u32 ifcvf_vdpa_get_generation(struct
vdpa_device *vdpa_dev)
static u32 ifcvf_vdpa_get_device_id(struct vdpa_device *vdpa_dev)
{
- struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev);
- struct pci_dev *pdev = adapter->pdev;
- u32 ret = -ENODEV;
-
- if (pdev->device < 0x1000 || pdev->device > 0x107f)
- return ret;
-
- if (pdev->device < 0x1040)
- ret = pdev->subsystem_device;
- else
- ret = pdev->device -0x1040;
+ struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
- return ret;
+ return vf->dev_type;
}
static u32 ifcvf_vdpa_get_vendor_id(struct vdpa_device *vdpa_dev)
@@ -466,6 +456,14 @@ static int ifcvf_probe(struct pci_dev *pdev,
const struct pci_device_id *id)
pci_set_drvdata(pdev, adapter);
vf = >vf;
+ if (pdev->device < 0x1000 || pdev->device > 0x107f)
+ return -EOPNOTSUPP;
+
+ if (pdev->device < 0x1040)
+ vf->dev_type = pdev->subsystem_device;
+ else
+ vf->dev_type = pdev->device - 0x1040;
So a question here, is the device a transtional device or modern one?
If it's a transitonal one, can it swtich endianess automatically or
not?
Thanks
Hi Jason,
This driver should drive both modern and transitional devices as we
discussed before.
If it's a transitional one, it will act as a modern device by
default, legacy mode is a fail-over path.
Note that legacy driver use native endian, support legacy driver
requires the device to know native endian which I'm not sure your
device can do that.
Thanks
Yes, legacy requires guest native endianess, I think we don't need to
worry about this because our transitional device should work in modern
mode by
default(legacy mode is the failover path we will never reach,
get_features will fail if no ACCESS_PLATFORM), we don't support legacy
device in vDPA.
Thanks
For vDPA, it has to support VIRTIO_1 and ACCESS_PLATFORM, so it must
in modern mode.
I think we don't need to worry about endianess for legacy mode.
Thanks
Zhu Lingshan
+
vf->base = pcim_iomap_table(pdev);
adapter->pdev = pdev;
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/3] vDPA/ifcvf: enable Intel C5000X-PL virtio-block for vDPA
在 2021/4/15 下午1:55, Zhu Lingshan 写道:
On 4/15/2021 11:34 AM, Jason Wang wrote:
在 2021/4/14 下午5:18, Zhu Lingshan 写道:
This commit enabled Intel FPGA SmartNIC C5000X-PL virtio-block
for vDPA.
Signed-off-by: Zhu Lingshan
---
drivers/vdpa/ifcvf/ifcvf_base.h | 17 -
drivers/vdpa/ifcvf/ifcvf_main.c | 10 +-
2 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h
b/drivers/vdpa/ifcvf/ifcvf_base.h
index 1c04cd256fa7..8b403522bf06 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.h
+++ b/drivers/vdpa/ifcvf/ifcvf_base.h
@@ -15,6 +15,7 @@
#include
#include
#include
+#include
#include
#include
@@ -28,7 +29,12 @@
#define C5000X_PL_SUBSYS_VENDOR_ID 0x8086
#define C5000X_PL_SUBSYS_DEVICE_ID 0x0001
-#define IFCVF_SUPPORTED_FEATURES \
+#define C5000X_PL_BLK_VENDOR_ID 0x1AF4
+#define C5000X_PL_BLK_DEVICE_ID 0x1001
+#define C5000X_PL_BLK_SUBSYS_VENDOR_ID 0x8086
+#define C5000X_PL_BLK_SUBSYS_DEVICE_ID 0x0002
+
+#define IFCVF_NET_SUPPORTED_FEATURES \
((1ULL << VIRTIO_NET_F_MAC) | \
(1ULL << VIRTIO_F_ANY_LAYOUT) | \
(1ULL << VIRTIO_F_VERSION_1) | \
@@ -37,6 +43,15 @@
(1ULL << VIRTIO_F_ACCESS_PLATFORM) | \
(1ULL << VIRTIO_NET_F_MRG_RXBUF))
+#define IFCVF_BLK_SUPPORTED_FEATURES \
+ ((1ULL << VIRTIO_BLK_F_SIZE_MAX) | \
+ (1ULL << VIRTIO_BLK_F_SEG_MAX) | \
+ (1ULL << VIRTIO_BLK_F_BLK_SIZE) | \
+ (1ULL << VIRTIO_BLK_F_TOPOLOGY) | \
+ (1ULL << VIRTIO_BLK_F_MQ) | \
+ (1ULL << VIRTIO_F_VERSION_1) | \
+ (1ULL << VIRTIO_F_ACCESS_PLATFORM))
I think we've discussed this sometime in the past but what's the
reason for such whitelist consider there's already a get_features()
implemention?
E.g Any reason to block VIRTIO_BLK_F_WRITE_ZEROS or
VIRTIO_F_RING_PACKED?
Thanks
The reason is some feature bits are supported in the device but not
supported by the driver, e.g, for virtio-net, mq & cq implementation
is not ready in the driver.
I understand the case of virtio-net but I wonder why we need this for
block where we don't vq cvq.
Thanks
Thanks!
+
/* Only one queue pair for now. */
#define IFCVF_MAX_QUEUE_PAIRS 1
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
b/drivers/vdpa/ifcvf/ifcvf_main.c
index 99b0a6b4c227..9b6a38b798fa 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -171,7 +171,11 @@ static u64 ifcvf_vdpa_get_features(struct
vdpa_device *vdpa_dev)
struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
u64 features;
- features = ifcvf_get_features(vf) & IFCVF_SUPPORTED_FEATURES;
+ if (vf->dev_type == VIRTIO_ID_NET)
+ features = ifcvf_get_features(vf) &
IFCVF_NET_SUPPORTED_FEATURES;
+
+ if (vf->dev_type == VIRTIO_ID_BLOCK)
+ features = ifcvf_get_features(vf) &
IFCVF_BLK_SUPPORTED_FEATURES;
return features;
}
@@ -509,6 +513,10 @@ static struct pci_device_id ifcvf_pci_ids[] = {
C5000X_PL_DEVICE_ID,
C5000X_PL_SUBSYS_VENDOR_ID,
C5000X_PL_SUBSYS_DEVICE_ID) },
+ { PCI_DEVICE_SUB(C5000X_PL_BLK_VENDOR_ID,
+ C5000X_PL_BLK_DEVICE_ID,
+ C5000X_PL_BLK_SUBSYS_VENDOR_ID,
+ C5000X_PL_BLK_SUBSYS_DEVICE_ID) },
{ 0 },
};
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/3] vDPA/ifcvf: deduce VIRTIO device ID when probe
在 2021/4/15 下午1:52, Zhu Lingshan 写道:
On 4/15/2021 11:30 AM, Jason Wang wrote:
在 2021/4/14 下午5:18, Zhu Lingshan 写道:
This commit deduces VIRTIO device ID as device type when probe,
then ifcvf_vdpa_get_device_id() can simply return the ID.
ifcvf_vdpa_get_features() and ifcvf_vdpa_get_config_size()
can work properly based on the device ID.
Signed-off-by: Zhu Lingshan
---
drivers/vdpa/ifcvf/ifcvf_base.h | 1 +
drivers/vdpa/ifcvf/ifcvf_main.c | 22 ++
2 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h
b/drivers/vdpa/ifcvf/ifcvf_base.h
index b2eeb16b9c2c..1c04cd256fa7 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.h
+++ b/drivers/vdpa/ifcvf/ifcvf_base.h
@@ -84,6 +84,7 @@ struct ifcvf_hw {
u32 notify_off_multiplier;
u64 req_features;
u64 hw_features;
+ u32 dev_type;
struct virtio_pci_common_cfg __iomem *common_cfg;
void __iomem *net_cfg;
struct vring_info vring[IFCVF_MAX_QUEUE_PAIRS * 2];
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
b/drivers/vdpa/ifcvf/ifcvf_main.c
index 44d7586019da..99b0a6b4c227 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -323,19 +323,9 @@ static u32 ifcvf_vdpa_get_generation(struct
vdpa_device *vdpa_dev)
static u32 ifcvf_vdpa_get_device_id(struct vdpa_device *vdpa_dev)
{
- struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev);
- struct pci_dev *pdev = adapter->pdev;
- u32 ret = -ENODEV;
-
- if (pdev->device < 0x1000 || pdev->device > 0x107f)
- return ret;
-
- if (pdev->device < 0x1040)
- ret = pdev->subsystem_device;
- else
- ret = pdev->device -0x1040;
+ struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
- return ret;
+ return vf->dev_type;
}
static u32 ifcvf_vdpa_get_vendor_id(struct vdpa_device *vdpa_dev)
@@ -466,6 +456,14 @@ static int ifcvf_probe(struct pci_dev *pdev,
const struct pci_device_id *id)
pci_set_drvdata(pdev, adapter);
vf = >vf;
+ if (pdev->device < 0x1000 || pdev->device > 0x107f)
+ return -EOPNOTSUPP;
+
+ if (pdev->device < 0x1040)
+ vf->dev_type = pdev->subsystem_device;
+ else
+ vf->dev_type = pdev->device - 0x1040;
So a question here, is the device a transtional device or modern one?
If it's a transitonal one, can it swtich endianess automatically or not?
Thanks
Hi Jason,
This driver should drive both modern and transitional devices as we
discussed before.
If it's a transitional one, it will act as a modern device by default,
legacy mode is a fail-over path.
Note that legacy driver use native endian, support legacy driver
requires the device to know native endian which I'm not sure your device
can do that.
Thanks
For vDPA, it has to support VIRTIO_1 and ACCESS_PLATFORM, so it must
in modern mode.
I think we don't need to worry about endianess for legacy mode.
Thanks
Zhu Lingshan
+
vf->base = pcim_iomap_table(pdev);
adapter->pdev = pdev;
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
On 2021/4/14 11:52, Viresh Kumar wrote: Is i2c/for-next the right tree to merge it ? It should be. Thanks Viresh. Hi Wolfram, Do you have any comments for this patch ? Your opinion will be important to improve this patch since you are the maintainer of I2C. Thanks, Jie ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
On 2021/4/15 11:51, Jason Wang wrote:
+ for (i = 0; i < nr; i++) {
+ /* Detach the ith request from the vq */
+ req = virtqueue_get_buf(vq, );
+
+ /*
+ * Condition (req && req == [i]) should always meet since
+ * we have total nr requests in the vq.
So this assumes the requests are completed in order. Is this mandated
in the spec?
This is a mandatory device requirements in spec.
+ */
+ if (!failed && (WARN_ON(!(req && req == [i])) ||
+ (req->in_hdr.status != VIRTIO_I2C_MSG_OK)))
+ failed = true;
+
+ i2c_put_dma_safe_msg_buf(reqs[i].buf, [i], !failed);
+ if (!failed)
+ ++j;
+ }
+
+ return (timeout ? -ETIMEDOUT : j);
Checking timeout is fragile, what happens if the request are completed
after wait_for_completion() but before virtio_i2c_complete_reqs()?
We have discussed this before in v6.
https://lists.linuxfoundation.org/pipermail/virtualization/2021-March/053093.html.
If timeout happens, we don't need to care about the requests whether
really completed by "HW" or not.
Just return error and let the i2c core to decide whether to resend. And
currently, the timeout is statically configured in driver.
We may extend a device timeout value configuration in spec for driver to
use if there is actual need in the future.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
