Re: [PATCH] vdpasim: Off by one in vdpasim_set_group_asid()

2022-05-23 Thread Jason Wang
On Mon, May 23, 2022 at 4:31 PM Dan Carpenter  wrote:
>
> The > comparison needs to be >= to prevent an out of bounds access
> of the vdpasim->iommu[] array.  The vdpasim->iommu[] is allocated in
> vdpasim_create() and it has vdpasim->dev_attr.nas elements.
>
> Fixes: 87e5afeac247 ("vdpasim: control virtqueue support")
> Signed-off-by: Dan Carpenter 

Acked-by: Jason Wang 

> ---
>  drivers/vdpa/vdpa_sim/vdpa_sim.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c 
> b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> index 50d721072beb..0f2865899647 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> @@ -567,7 +567,7 @@ static int vdpasim_set_group_asid(struct vdpa_device 
> *vdpa, unsigned int group,
> if (group > vdpasim->dev_attr.ngroups)
> return -EINVAL;
>
> -   if (asid > vdpasim->dev_attr.nas)
> +   if (asid >= vdpasim->dev_attr.nas)
> return -EINVAL;
>
> iommu = >iommu[asid];
> --
> 2.35.1
>

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


[PATCH] vdpa: ifcvf: set pci driver data in probe

2022-05-23 Thread Jason Wang
We should set the pci driver data in probe instead of the vdpa device
adding callback. Otherwise if no vDPA device is created we will lose
the pointer to the management device.

Fixes: 6b5df347c6482 ("vDPA/ifcvf: implement management netlink framework for 
ifcvf")
Tested-by: Zheyu Ma 
Signed-off-by: Jason Wang 
---
 drivers/vdpa/ifcvf/ifcvf_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index 750e5f23406d..0a5670729412 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -771,7 +771,6 @@ static int ifcvf_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, 
const char *name,
}
 
ifcvf_mgmt_dev->adapter = adapter;
-   pci_set_drvdata(pdev, ifcvf_mgmt_dev);
 
vf = >vf;
vf->dev_type = get_dev_type(pdev);
@@ -886,6 +885,8 @@ static int ifcvf_probe(struct pci_dev *pdev, const struct 
pci_device_id *id)
goto err;
}
 
+   pci_set_drvdata(pdev, ifcvf_mgmt_dev);
+
return 0;
 
 err:
-- 
2.25.1

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH 1/4] vdpa: Add stop operation

2022-05-23 Thread Jason Wang


在 2022/5/24 08:01, Si-Wei Liu 写道:



On 5/23/2022 4:54 PM, Si-Wei Liu wrote:



On 5/23/2022 12:20 PM, Eugenio Perez Martin wrote:
On Sat, May 21, 2022 at 12:13 PM Si-Wei Liu  
wrote:



On 5/20/2022 10:23 AM, Eugenio Pérez wrote:
This operation is optional: It it's not implemented, backend 
feature bit

will not be exposed.

Signed-off-by: Eugenio Pérez 
---
   include/linux/vdpa.h | 6 ++
   1 file changed, 6 insertions(+)

diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index 15af802d41c4..ddfebc4e1e01 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h
@@ -215,6 +215,11 @@ struct vdpa_map_file {
    * @reset:  Reset device
    *  @vdev: vdpa device
    *  Returns integer: success (0) or 
error (< 0)
+ * @stop:    Stop or resume the device (optional, 
but it must

+ *   be implemented if require device stop)
+ *   @vdev: vdpa device
+ *   @stop: stop (true), not stop (false)
+ *   Returns integer: success (0) or 
error (< 0)
Is this uAPI meant to address all use cases described in the full 
blown

_F_STOP virtio spec proposal, such as:

--%<--

.. the device MUST finish any in flight
operations after the driver writes STOP.  Depending on the device, it
can do it
in many ways as long as the driver can recover its normal operation 
if it

resumes the device without the need of resetting it:

- Drain and wait for the completion of all pending requests until a
    convenient avail descriptor. Ignore any other posterior 
descriptor.
- Return a device-specific failure for these descriptors, so the 
driver

    can choose to retry or to cancel them.
- Mark them as done even if they are not, if the kind of device can
    assume to lose them.
--%<--


Right, this is totally underspecified in this series.

I'll expand on it in the next version, but that text proposed to
virtio-comment was complicated and misleading. I find better to get
the previous version description. Would the next description work?

```
After the return of ioctl, the device MUST finish any pending 
operations like

in flight requests. It must also preserve all the necessary state (the
virtqueue vring base plus the possible device specific states)
Hmmm, "possible device specific states" is a bit vague. Does it 
require the device to save any device internal state that is not 
defined in the virtio spec - such as any failed in-flight requests to 
resubmit upon resume? Or you would lean on SVQ to intercept it in 
depth and save it with some other means? I think network device also 
has internal state such as flow steering state that needs bookkeeping 
as well.
Noted that I understand you may introduce additional feature call 
similar to VHOST_USER_GET_INFLIGHT_FD for (failed) in-flight request, 
but since that's is a get interface, I assume the actual state 
preserving should still take place in this STOP call.




Yes, I think so.



-Siwei



A follow-up question is what is the use of the `stop` argument of 
false, does it require the device to support resume? 



Yes, this is required by the hypervisor e.g for Qemu it supports vm 
stop/resume.



I seem to recall this is something to abandon in favor of device 
reset plus setting queue base/addr after. Or it's just a optional 
feature that may be device specific (if one can do so in simple way).



Rest is more like a workarond consider we don't have a stop API. 
Consider we don't add stop at the beginning, it can only be an optional 
feature.


Thanks




-Siwei


  that is required
for restoring in the future.

In the future, we will provide features similar to 
VHOST_USER_GET_INFLIGHT_FD

so the device can save pending operations.
```

Thanks for pointing it out!






E.g. do I assume correctly all in flight requests are flushed after
return from this uAPI call? Or some of pending requests may be subject
to loss or failure? How does the caller/user specify these various
options (if there are) for device stop?

BTW, it would be nice to add the corresponding support to vdpa_sim_blk
as well to demo the stop handling. To just show it on vdpa-sim-net 
IMHO

is perhaps not so convincing.

-Siwei

    * @get_config_size: Get the size of the configuration space 
includes
    *  fields that are conditional on 
feature bits.

    *  @vdev: vdpa device
@@ -316,6 +321,7 @@ struct vdpa_config_ops {
   u8 (*get_status)(struct vdpa_device *vdev);
   void (*set_status)(struct vdpa_device *vdev, u8 status);
   int (*reset)(struct vdpa_device *vdev);
+ int (*stop)(struct vdpa_device *vdev, bool stop);
   size_t (*get_config_size)(struct vdpa_device *vdev);
   void (*get_config)(struct vdpa_device *vdev, unsigned int 
offset,

  void *buf, unsigned int len);







Re: [PATCH V2 5/7] dt-bindings: Add xen, dev-domid property description for xen-grant DMA ops

2022-05-23 Thread Stefano Stabellini
On Mon, 23 May 2022, Oleksandr wrote:
> > > On Thu, 19 May 2022, Oleksandr wrote:
> > > > > On Wed, May 18, 2022 at 5:06 PM Oleksandr  wrote:
> > > > > > On 18.05.22 17:32, Arnd Bergmann wrote:
> > > > > > > On Sat, May 7, 2022 at 7:19 PM Oleksandr Tyshchenko
> > > > > > >  wrote:
> > > > > > >     This would mean having a device
> > > > > > > node for the grant-table mechanism that can be referred to using
> > > > > > > the
> > > > > > > 'iommus'
> > > > > > > phandle property, with the domid as an additional argument.
> > > > > > I assume, you are speaking about something like the following?
> > > > > > 
> > > > > > 
> > > > > > xen_dummy_iommu {
> > > > > >   compatible = "xen,dummy-iommu";
> > > > > >   #iommu-cells = <1>;
> > > > > > };
> > > > > > 
> > > > > > virtio@3000 {
> > > > > >   compatible = "virtio,mmio";
> > > > > >   reg = <0x3000 0x100>;
> > > > > >   interrupts = <41>;
> > > > > > 
> > > > > >   /* The device is located in Xen domain with ID 1 */
> > > > > >   iommus = <_dummy_iommu 1>;
> > > > > > };
> > > > > Right, that's that's the idea,
> > > > thank you for the confirmation
> > > > 
> > > > 
> > > > 
> > > > >    except I would not call it a 'dummy'.
> > > > >   From the perspective of the DT, this behaves just like an IOMMU,
> > > > > even if the exact mechanism is different from most hardware IOMMU
> > > > > implementations.
> > > > well, agree
> > > > 
> > > > 
> > > > > > > It does not quite fit the model that Linux currently uses for
> > > > > > > iommus,
> > > > > > > as that has an allocator for dma_addr_t space
> > > > > > yes (# 3/7 adds grant-table based allocator)
> > > > > > 
> > > > > > 
> > > > > > > , but it would think it's
> > > > > > > conceptually close enough that it makes sense for the binding.
> > > > > > Interesting idea. I am wondering, do we need an extra actions for
> > > > > > this
> > > > > > to work in Linux guest (dummy IOMMU driver, etc)?
> > > > > It depends on how closely the guest implementation can be made to
> > > > > resemble a normal iommu. If you do allocate dma_addr_t addresses,
> > > > > it may actually be close enough that you can just turn the grant-table
> > > > > code into a normal iommu driver and change nothing else.
> > > > Unfortunately, I failed to find a way how use grant references at the
> > > > iommu_ops level (I mean to fully pretend that we are an IOMMU driver). I
> > > > am
> > > > not too familiar with that, so what is written below might be wrong or
> > > > at
> > > > least not precise.
> > > > 
> > > > The normal IOMMU driver in Linux doesn’t allocate DMA addresses by
> > > > itself, it
> > > > just maps (IOVA-PA) what was requested to be mapped by the upper layer.
> > > > The
> > > > DMA address allocation is done by the upper layer (DMA-IOMMU which is
> > > > the glue
> > > > layer between DMA API and IOMMU API allocates IOVA for PA?). But, all
> > > > what we
> > > > need here is just to allocate our specific grant-table based DMA
> > > > addresses
> > > > (DMA address = grant reference + offset in the page), so let’s say we
> > > > need an
> > > > entity to take a physical address as parameter and return a DMA address
> > > > (what
> > > > actually commit #3/7 is doing), and that’s all. So working at the
> > > > dma_ops
> > > > layer we get exactly what we need, with the minimal changes to guest
> > > > infrastructure. In our case the Xen itself acts as an IOMMU.
> > > > 
> > > > Assuming that we want to reuse the IOMMU infrastructure somehow for our
> > > > needs.
> > > > I think, in that case we will likely need to introduce a new specific
> > > > IOVA
> > > > allocator (alongside with a generic one) to be hooked up by the
> > > > DMA-IOMMU
> > > > layer if we run on top of Xen. But, even having the specific IOVA
> > > > allocator to
> > > > return what we indeed need (DMA address = grant reference + offset in
> > > > the
> > > > page) we will still need the specific minimal required IOMMU driver to
> > > > be
> > > > present in the system anyway in order to track the mappings(?) and do
> > > > nothing
> > > > with them, returning a success (this specific IOMMU driver should have
> > > > all
> > > > mandatory callbacks implemented).
> > > > 
> > > > I completely agree, it would be really nice to reuse generic IOMMU
> > > > bindings
> > > > rather than introducing Xen specific property if what we are trying to
> > > > implement in current patch series fits in the usage of "iommus" in Linux
> > > > more-less. But, if we will have to add more complexity/more components
> > > > to the
> > > > code for the sake of reusing device tree binding, this raises a question
> > > > whether that’s worthwhile.
> > > > 
> > > > Or I really missed something?
> > > I think Arnd was primarily suggesting to reuse the IOMMU Device Tree
> > > bindings, not necessarily the IOMMU drivers framework in Linux (although
> > > that would be an added bonus.)
> > > 
> > > I know from previous discussions with you that 

RE: [PATCH v2 2/3] vdpa: Add a device object for vdpa management device

2022-05-23 Thread Parav Pandit via Virtualization

> From: Jason Wang 
> Sent: Sunday, May 22, 2022 11:41 PM
> To: Parav Pandit 
> Cc: gre...@linuxfoundation.org; Yongji Xie ;
> m...@redhat.com; lingshan@intel.com; Eli Cohen ;
> virtualization@lists.linux-foundation.org
> Subject: Re: [PATCH v2 2/3] vdpa: Add a device object for vdpa management
> device
> 
> On Mon, May 23, 2022 at 10:00 AM Parav Pandit 
> wrote:
> >
> >
> > > From: Jason Wang 
> > > Sent: Wednesday, May 18, 2022 4:32 AM
> > >
> > > 在 2022/5/18 07:03, Parav Pandit 写道:
> > > >>> And regarding vduse_dev_release() and existing empty release
> > > >>> function,
> > > >> they can be dynamically allocated.
> > > >>> This is because they are really the struct device.
> > > >> I do not understand this statement, sorry.
> > > > It was bad sentence, my bad.
> > > >
> > > > What I wanted to say is, probably better explained with real patch
> below.
> > > > In context of [1], struct vduse_mgmtdev empty release function can
> > > > be
> > > avoided by below inline patch [2].
> > > >
> > > > [1]https://www.spinics.net/lists/linux-virtualization/msg56371.htm
> > > > l
> > > >
> > > > [2] patch:
> > >
> > >
> > > Ok, if we go this way (looks more like mdev_parent). I think we need
> > > at least rename the vdpa_mgmt_device, maybe vdpa_parent (like
> mdev_parent)?
> > >
> > It can be.
> > Parent is also a device. So...
> 
> How about "vdpa_parent_data"? Not a native speaker but it's better to
> remove "device" from the name anhow.
> 
_data is diluting the role what this object does as has ops.
And object is more than a parent.
So may be vdpa_mgmt_link as it enables handling vdpa device management and 
linkage to the *device.

I still feel that its overkill.
Mgmt_dev is probably just fine, as its taking care of life cycling multiple 
devices...

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/4] vdpa: Add stop operation

2022-05-23 Thread Si-Wei Liu



On 5/23/2022 4:54 PM, Si-Wei Liu wrote:



On 5/23/2022 12:20 PM, Eugenio Perez Martin wrote:
On Sat, May 21, 2022 at 12:13 PM Si-Wei Liu  
wrote:



On 5/20/2022 10:23 AM, Eugenio Pérez wrote:
This operation is optional: It it's not implemented, backend 
feature bit

will not be exposed.

Signed-off-by: Eugenio Pérez 
---
   include/linux/vdpa.h | 6 ++
   1 file changed, 6 insertions(+)

diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index 15af802d41c4..ddfebc4e1e01 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h
@@ -215,6 +215,11 @@ struct vdpa_map_file {
    * @reset:  Reset device
    *  @vdev: vdpa device
    *  Returns integer: success (0) or 
error (< 0)
+ * @stop:    Stop or resume the device (optional, 
but it must

+ *   be implemented if require device stop)
+ *   @vdev: vdpa device
+ *   @stop: stop (true), not stop (false)
+ *   Returns integer: success (0) or error 
(< 0)

Is this uAPI meant to address all use cases described in the full blown
_F_STOP virtio spec proposal, such as:

--%<--

.. the device MUST finish any in flight
operations after the driver writes STOP.  Depending on the device, it
can do it
in many ways as long as the driver can recover its normal operation 
if it

resumes the device without the need of resetting it:

- Drain and wait for the completion of all pending requests until a
    convenient avail descriptor. Ignore any other posterior descriptor.
- Return a device-specific failure for these descriptors, so the driver
    can choose to retry or to cancel them.
- Mark them as done even if they are not, if the kind of device can
    assume to lose them.
--%<--


Right, this is totally underspecified in this series.

I'll expand on it in the next version, but that text proposed to
virtio-comment was complicated and misleading. I find better to get
the previous version description. Would the next description work?

```
After the return of ioctl, the device MUST finish any pending 
operations like

in flight requests. It must also preserve all the necessary state (the
virtqueue vring base plus the possible device specific states)
Hmmm, "possible device specific states" is a bit vague. Does it 
require the device to save any device internal state that is not 
defined in the virtio spec - such as any failed in-flight requests to 
resubmit upon resume? Or you would lean on SVQ to intercept it in 
depth and save it with some other means? I think network device also 
has internal state such as flow steering state that needs bookkeeping 
as well.
Noted that I understand you may introduce additional feature call 
similar to VHOST_USER_GET_INFLIGHT_FD for (failed) in-flight request, 
but since that's is a get interface, I assume the actual state 
preserving should still take place in this STOP call.


-Siwei



A follow-up question is what is the use of the `stop` argument of 
false, does it require the device to support resume? I seem to recall 
this is something to abandon in favor of device reset plus setting 
queue base/addr after. Or it's just a optional feature that may be 
device specific (if one can do so in simple way).


-Siwei


  that is required
for restoring in the future.

In the future, we will provide features similar to 
VHOST_USER_GET_INFLIGHT_FD

so the device can save pending operations.
```

Thanks for pointing it out!






E.g. do I assume correctly all in flight requests are flushed after
return from this uAPI call? Or some of pending requests may be subject
to loss or failure? How does the caller/user specify these various
options (if there are) for device stop?

BTW, it would be nice to add the corresponding support to vdpa_sim_blk
as well to demo the stop handling. To just show it on vdpa-sim-net IMHO
is perhaps not so convincing.

-Siwei

    * @get_config_size: Get the size of the configuration space 
includes
    *  fields that are conditional on 
feature bits.

    *  @vdev: vdpa device
@@ -316,6 +321,7 @@ struct vdpa_config_ops {
   u8 (*get_status)(struct vdpa_device *vdev);
   void (*set_status)(struct vdpa_device *vdev, u8 status);
   int (*reset)(struct vdpa_device *vdev);
+ int (*stop)(struct vdpa_device *vdev, bool stop);
   size_t (*get_config_size)(struct vdpa_device *vdev);
   void (*get_config)(struct vdpa_device *vdev, unsigned int 
offset,

  void *buf, unsigned int len);




___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/4] vdpa: Add stop operation

2022-05-23 Thread Si-Wei Liu



On 5/23/2022 12:20 PM, Eugenio Perez Martin wrote:

On Sat, May 21, 2022 at 12:13 PM Si-Wei Liu  wrote:



On 5/20/2022 10:23 AM, Eugenio Pérez wrote:

This operation is optional: It it's not implemented, backend feature bit
will not be exposed.

Signed-off-by: Eugenio Pérez 
---
   include/linux/vdpa.h | 6 ++
   1 file changed, 6 insertions(+)

diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index 15af802d41c4..ddfebc4e1e01 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h
@@ -215,6 +215,11 @@ struct vdpa_map_file {
* @reset:  Reset device
*  @vdev: vdpa device
*  Returns integer: success (0) or error (< 0)
+ * @stop:Stop or resume the device (optional, but it must
+ *   be implemented if require device stop)
+ *   @vdev: vdpa device
+ *   @stop: stop (true), not stop (false)
+ *   Returns integer: success (0) or error (< 0)

Is this uAPI meant to address all use cases described in the full blown
_F_STOP virtio spec proposal, such as:

--%<--

.. the device MUST finish any in flight
operations after the driver writes STOP.  Depending on the device, it
can do it
in many ways as long as the driver can recover its normal operation if it
resumes the device without the need of resetting it:

- Drain and wait for the completion of all pending requests until a
convenient avail descriptor. Ignore any other posterior descriptor.
- Return a device-specific failure for these descriptors, so the driver
can choose to retry or to cancel them.
- Mark them as done even if they are not, if the kind of device can
assume to lose them.
--%<--


Right, this is totally underspecified in this series.

I'll expand on it in the next version, but that text proposed to
virtio-comment was complicated and misleading. I find better to get
the previous version description. Would the next description work?

```
After the return of ioctl, the device MUST finish any pending operations like
in flight requests. It must also preserve all the necessary state (the
virtqueue vring base plus the possible device specific states)
Hmmm, "possible device specific states" is a bit vague. Does it require 
the device to save any device internal state that is not defined in the 
virtio spec - such as any failed in-flight requests to resubmit upon 
resume? Or you would lean on SVQ to intercept it in depth and save it 
with some other means? I think network device also has internal state 
such as flow steering state that needs bookkeeping as well.


A follow-up question is what is the use of the `stop` argument of false, 
does it require the device to support resume? I seem to recall this is 
something to abandon in favor of device reset plus setting queue 
base/addr after. Or it's just a optional feature that may be device 
specific (if one can do so in simple way).


-Siwei


  that is required
for restoring in the future.

In the future, we will provide features similar to VHOST_USER_GET_INFLIGHT_FD
so the device can save pending operations.
```

Thanks for pointing it out!






E.g. do I assume correctly all in flight requests are flushed after
return from this uAPI call? Or some of pending requests may be subject
to loss or failure? How does the caller/user specify these various
options (if there are) for device stop?

BTW, it would be nice to add the corresponding support to vdpa_sim_blk
as well to demo the stop handling. To just show it on vdpa-sim-net IMHO
is perhaps not so convincing.

-Siwei


* @get_config_size:Get the size of the configuration space 
includes
*  fields that are conditional on feature bits.
*  @vdev: vdpa device
@@ -316,6 +321,7 @@ struct vdpa_config_ops {
   u8 (*get_status)(struct vdpa_device *vdev);
   void (*set_status)(struct vdpa_device *vdev, u8 status);
   int (*reset)(struct vdpa_device *vdev);
+ int (*stop)(struct vdpa_device *vdev, bool stop);
   size_t (*get_config_size)(struct vdpa_device *vdev);
   void (*get_config)(struct vdpa_device *vdev, unsigned int offset,
  void *buf, unsigned int len);


___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] vhost-vdpa: return -EFAULT on copy_to_user() failure

2022-05-23 Thread Stefano Garzarella

On Mon, May 23, 2022 at 11:33:26AM +0300, Dan Carpenter wrote:

The copy_to_user() function returns the number of bytes remaining to be
copied.  However, we need to return a negative error code, -EFAULT, to
the user.

Fixes: 87f4c217413a ("vhost-vdpa: introduce uAPI to get the number of virtqueue 
groups")
Fixes: e96ef636f154 ("vhost-vdpa: introduce uAPI to get the number of address 
spaces")
Signed-off-by: Dan Carpenter 
---
drivers/vhost/vdpa.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)


Reviewed-by: Stefano Garzarella 



diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 3e86080041fc..935a1d0ddb97 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -609,11 +609,13 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
r = vhost_vdpa_get_vring_num(v, argp);
break;
case VHOST_VDPA_GET_GROUP_NUM:
-   r = copy_to_user(argp, >vdpa->ngroups,
-sizeof(v->vdpa->ngroups));
+   if (copy_to_user(argp, >vdpa->ngroups,
+sizeof(v->vdpa->ngroups)))
+   r = -EFAULT;
break;
case VHOST_VDPA_GET_AS_NUM:
-   r = copy_to_user(argp, >vdpa->nas, sizeof(v->vdpa->nas));
+   if (copy_to_user(argp, >vdpa->nas, sizeof(v->vdpa->nas)))
+   r = -EFAULT;
break;
case VHOST_SET_LOG_BASE:
case VHOST_SET_LOG_FD:
--
2.35.1

___
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] vhost-vdpa: Fix some error handling path in vhost_vdpa_process_iotlb_msg()

2022-05-23 Thread Stefano Garzarella

On Mon, May 23, 2022 at 12:41:03PM +0800, Jason Wang wrote:

On Sun, May 22, 2022 at 9:59 PM Christophe JAILLET
 wrote:


In the error paths introduced by the commit in the Fixes tag, a mutex may
be left locked.
Add the correct goto instead of a direct return.

Fixes: a1468175bb17 ("vhost-vdpa: support ASID based IOTLB API")
Signed-off-by: Christophe JAILLET 
---
WARNING: This patch only fixes the goto vs return mix-up in this function.
However, the 2nd hunk looks really spurious to me. I think that the:
-   return -EINVAL;
+   r = -EINVAL;
+   goto unlock;
should be done only in the 'if (!iotlb)' block.


It should be fine, the error happen if

1) the batched ASID based request is not equal (the first if)
2) there's no IOTLB for this ASID (the second if)

But I agree the code could be tweaked to use two different if instead
of using a or condition here.


Yeah, I think so!

Anyway, this patch LGTM:

Reviewed-by: Stefano Garzarella 

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [mst-vhost:vhost 26/43] drivers/vhost/vdpa.c:1003:3-9: preceding lock on line 991 (fwd)

2022-05-23 Thread Stefano Garzarella

On Fri, May 20, 2022 at 06:37:01PM +0200, Julia Lawall wrote:

Please check whether an unlock is needed before line 1003.


Yep, I think so. Same also for line 1016.

I just saw that there is already a patch posted to solve this problem:
https://lore.kernel.org/netdev/89ef0ae4c26ac3cfa440c71e97e392dcb328ac1b.1653227924.git.christophe.jail...@wanadoo.fr/

Thanks for the report,
Stefano



julia

-- Forwarded message --
Date: Fri, 20 May 2022 17:35:29 +0800
From: kernel test robot 
To: kbu...@lists.01.org
Cc: l...@intel.com, Julia Lawall 
Subject: [mst-vhost:vhost 26/43] drivers/vhost/vdpa.c:1003:3-9: preceding lock
   on line 991

CC: kbuild-...@lists.01.org
BCC: l...@intel.com
CC: k...@vger.kernel.org
CC: virtualization@lists.linux-foundation.org
CC: net...@vger.kernel.org
TO: Gautam Dawar 
CC: "Michael S. Tsirkin" 
CC: Jason Wang 

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost
head:   73211bf1bc3ac0a3c544225e270401c1fe5d395d
commit: a1468175bb17ca5e477147de5d886e7a22d93527 [26/43] vhost-vdpa: support 
ASID based IOTLB API
:: branch date: 10 hours ago
:: commit date: 10 hours ago
config: arc-allmodconfig 
(https://download.01.org/0day-ci/archive/20220520/202205201721.rgqusahl-...@intel.com/config)
compiler: arceb-elf-gcc (GCC) 11.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot 
Reported-by: Julia Lawall 


cocci warnings: (new ones prefixed by >>)

drivers/vhost/vdpa.c:1003:3-9: preceding lock on line 991

  drivers/vhost/vdpa.c:1016:2-8: preceding lock on line 991

vim +1003 drivers/vhost/vdpa.c

4c8cf31885f69e Tiwei Bie2020-03-26   980
0f05062453fb51 Gautam Dawar 2022-03-30   981  static int 
vhost_vdpa_process_iotlb_msg(struct vhost_dev *dev, u32 asid,
4c8cf31885f69e Tiwei Bie2020-03-26   982
struct vhost_iotlb_msg *msg)
4c8cf31885f69e Tiwei Bie2020-03-26   983  {
4c8cf31885f69e Tiwei Bie2020-03-26   984struct vhost_vdpa *v = 
container_of(dev, struct vhost_vdpa, vdev);
25abc060d28213 Jason Wang   2020-08-04   985struct vdpa_device *vdpa = 
v->vdpa;
25abc060d28213 Jason Wang   2020-08-04   986const struct vdpa_config_ops *ops 
= vdpa->config;
a1468175bb17ca Gautam Dawar 2022-03-30   987struct vhost_iotlb *iotlb = 
NULL;
a1468175bb17ca Gautam Dawar 2022-03-30   988struct vhost_vdpa_as *as = NULL;
4c8cf31885f69e Tiwei Bie2020-03-26   989int r = 0;
4c8cf31885f69e Tiwei Bie2020-03-26   990
a9d064524fc3cf Xie Yongji   2021-04-12  @991mutex_lock(>mutex);
a9d064524fc3cf Xie Yongji   2021-04-12   992
4c8cf31885f69e Tiwei Bie2020-03-26   993r = vhost_dev_check_owner(dev);
4c8cf31885f69e Tiwei Bie2020-03-26   994if (r)
a9d064524fc3cf Xie Yongji   2021-04-12   995goto unlock;
4c8cf31885f69e Tiwei Bie2020-03-26   996
a1468175bb17ca Gautam Dawar 2022-03-30   997if (msg->type == 
VHOST_IOTLB_UPDATE ||
a1468175bb17ca Gautam Dawar 2022-03-30   998msg->type == 
VHOST_IOTLB_BATCH_BEGIN) {
a1468175bb17ca Gautam Dawar 2022-03-30   999as = 
vhost_vdpa_find_alloc_as(v, asid);
a1468175bb17ca Gautam Dawar 2022-03-30  1000if (!as) {
a1468175bb17ca Gautam Dawar 2022-03-30  1001dev_err(>dev, 
"can't find and alloc asid %d\n",
a1468175bb17ca Gautam Dawar 2022-03-30  1002asid);
a1468175bb17ca Gautam Dawar 2022-03-30 @1003return -EINVAL;
a1468175bb17ca Gautam Dawar 2022-03-30  1004}
a1468175bb17ca Gautam Dawar 2022-03-30  1005iotlb = >iotlb;
a1468175bb17ca Gautam Dawar 2022-03-30  1006} else
a1468175bb17ca Gautam Dawar 2022-03-30  1007iotlb = 
asid_to_iotlb(v, asid);
a1468175bb17ca Gautam Dawar 2022-03-30  1008
a1468175bb17ca Gautam Dawar 2022-03-30  1009if ((v->in_batch && 
v->batch_asid != asid) || !iotlb) {
a1468175bb17ca Gautam Dawar 2022-03-30  1010  		if (v->in_batch 
&& v->batch_asid != asid) {

a1468175bb17ca Gautam Dawar 2022-03-30  1011dev_info(>dev, 
"batch id %d asid %d\n",
a1468175bb17ca Gautam Dawar 2022-03-30  1012 
v->batch_asid, asid);
a1468175bb17ca Gautam Dawar 2022-03-30  1013}
a1468175bb17ca Gautam Dawar 2022-03-30  1014if (!iotlb)
a1468175bb17ca Gautam Dawar 2022-03-30  1015dev_err(>dev, "no 
iotlb for asid %d\n", asid);
a1468175bb17ca Gautam Dawar 2022-03-30  1016return -EINVAL;
a1468175bb17ca Gautam Dawar 2022-03-30  1017}
a1468175bb17ca Gautam Dawar 2022-03-30  1018
4c8cf31885f69e Tiwei Bie2020-03-26  1019switch (msg->type) {
4c8cf31885f69e Tiwei Bie2020-03-26  1020case VHOST_IOTLB_UPDATE:
3111cb7283065a Gautam Dawar 2022-03-30  1021r = 
vhost_vdpa_process_iotlb_update(v, iotlb, msg);
4c8cf31885f69e Tiwei Bie2020-03-26  1022break;
4c8cf31885f69e Tiwei Bie2020-03-26  1023case 

Re: [PATCH v6 4/9] crypto: add ASN.1 DER decoder

2022-05-23 Thread Daniel P . Berrangé
On Sat, May 14, 2022 at 08:54:59AM +0800, zhenwei pi wrote:
> From: Lei He 
> 
> Add an ANS.1 DER decoder which is used to parse asymmetric
> cipher keys
> 
> Signed-off-by: zhenwei pi 
> Signed-off-by: lei he 
> ---
>  crypto/der.c | 189 +++
>  crypto/der.h |  81 ++
>  crypto/meson.build   |   1 +
>  tests/unit/meson.build   |   1 +
>  tests/unit/test-crypto-der.c | 290 +++
>  5 files changed, 562 insertions(+)
>  create mode 100644 crypto/der.c
>  create mode 100644 crypto/der.h
>  create mode 100644 tests/unit/test-crypto-der.c
> 

> diff --git a/tests/unit/meson.build b/tests/unit/meson.build
> index 264f2bc0c8..a8af85128d 100644
> --- a/tests/unit/meson.build
> +++ b/tests/unit/meson.build
> @@ -47,6 +47,7 @@ tests = {
>'ptimer-test': ['ptimer-test-stubs.c', meson.project_source_root() / 
> 'hw/core/ptimer.c'],
>'test-qapi-util': [],
>'test-smp-parse': [qom, meson.project_source_root() / 
> 'hw/core/machine-smp.c'],
> +  'test-crypto-der': [crypto],
>  }

This needs to be moved to later in this file where the other
test-crypto- rules are, otherwise it fails to build
on a configuration  --without-system --without-tools.


With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH v6 7/9] test/crypto: Add test suite for crypto akcipher

2022-05-23 Thread Daniel P . Berrangé
On Sat, May 14, 2022 at 08:55:02AM +0800, zhenwei pi wrote:
> From: Lei He 
> 
> Add unit test and benchmark test for crypto akcipher.
> 
> Signed-off-by: lei he 
> Signed-off-by: zhenwei pi 
> Reviewed-by: Daniel P. Berrangé 
> ---
>  tests/bench/benchmark-crypto-akcipher.c | 157 ++
>  tests/bench/meson.build |   1 +
>  tests/bench/test_akcipher_keys.inc  | 537 ++
>  tests/unit/meson.build  |   1 +
>  tests/unit/test-crypto-akcipher.c   | 711 
>  5 files changed, 1407 insertions(+)
>  create mode 100644 tests/bench/benchmark-crypto-akcipher.c
>  create mode 100644 tests/bench/test_akcipher_keys.inc
>  create mode 100644 tests/unit/test-crypto-akcipher.c
> 
> diff --git a/tests/bench/benchmark-crypto-akcipher.c 
> b/tests/bench/benchmark-crypto-akcipher.c
> new file mode 100644
> index 00..c6c80c0be1
> --- /dev/null
> +++ b/tests/bench/benchmark-crypto-akcipher.c
> @@ -0,0 +1,157 @@
> +/*
> + * QEMU Crypto akcipher speed benchmark
> + *
> + * Copyright (c) 2022 Bytedance
> + *
> + * Authors:
> + *lei he 
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * (at your option) any later version.  See the COPYING file in the
> + * top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "crypto/init.h"
> +#include "crypto/akcipher.h"
> +#include "standard-headers/linux/virtio_crypto.h"
> +
> +#include "test_akcipher_keys.inc"
> +
> +static bool keep_running;
> +
> +static void alarm_handler(int sig)
> +{
> +keep_running = false;
> +}
> +
> +static QCryptoAkCipher *create_rsa_akcipher(const uint8_t *priv_key,
> +size_t keylen,
> +QCryptoRSAPaddingAlgorithm 
> padding,
> +QCryptoHashAlgorithm hash)
> +{
> +QCryptoAkCipherOptions opt;
> +QCryptoAkCipher *rsa;
> +
> +opt.alg = QCRYPTO_AKCIPHER_ALG_RSA;
> +opt.u.rsa.padding_alg = padding;
> +opt.u.rsa.hash_alg = hash;
> +rsa = qcrypto_akcipher_new(, QCRYPTO_AKCIPHER_KEY_TYPE_PRIVATE,
> +   priv_key, keylen, _abort);
> +return rsa;
> +}
> +
> +static void test_rsa_speed(const uint8_t *priv_key, size_t keylen,
> +   size_t key_size)
> +{
> +#define BYTE 8
> +#define SHA1_DGST_LEN 20
> +#define DURATION_SECONDS 10
> +#define PADDING QCRYPTO_RSA_PADDING_ALG_PKCS1
> +#define HASH QCRYPTO_HASH_ALG_SHA1
> +
> +g_autoptr(QCryptoAkCipher) rsa =
> +create_rsa_akcipher(priv_key, keylen, PADDING, HASH);
> +g_autofree uint8_t *dgst = NULL;
> +g_autofree uint8_t *signature = NULL;
> +size_t count;
> +
> +dgst = g_new0(uint8_t, SHA1_DGST_LEN);
> +memset(dgst, g_test_rand_int(), SHA1_DGST_LEN);
> +signature = g_new0(uint8_t, key_size / BYTE);
> +
> +g_test_message("benchmark rsa%lu (%s-%s) sign in %d seconds", key_size,
> +   QCryptoRSAPaddingAlgorithm_str(PADDING),
> +   QCryptoHashAlgorithm_str(HASH),
> +   DURATION_SECONDS);

Needs to be '%zu' here and several other places in this file for any
parameter which is 'size_t'.

> +alarm(DURATION_SECONDS);
> +g_test_timer_start();
> +for (keep_running = true, count = 0; keep_running; ++count) {
> +g_assert(qcrypto_akcipher_sign(rsa, dgst, SHA1_DGST_LEN,
> +   signature, key_size / BYTE,
> +   _abort) > 0);
> +}
> +g_test_timer_elapsed();
> +g_test_message("rsa%lu (%s-%s) sign %lu times in %.2f seconds,"
> +   " %.2f times/sec ",
> +   key_size,  QCryptoRSAPaddingAlgorithm_str(PADDING),
> +   QCryptoHashAlgorithm_str(HASH),
> +   count, g_test_timer_last(),
> +   (double)count / g_test_timer_last());
> +
> +g_test_message("benchmark rsa%lu (%s-%s) verify in %d seconds", key_size,
> +   QCryptoRSAPaddingAlgorithm_str(PADDING),
> +   QCryptoHashAlgorithm_str(HASH),
> +   DURATION_SECONDS);
> +alarm(DURATION_SECONDS);
> +g_test_timer_start();
> +for (keep_running = true, count = 0; keep_running; ++count) {
> +g_assert(qcrypto_akcipher_verify(rsa, signature, key_size / BYTE,
> + dgst, SHA1_DGST_LEN,
> + _abort) == 0);
> +}
> +g_test_timer_elapsed();
> +g_test_message("rsa%lu (%s-%s) verify %lu times in %.2f seconds,"
> +   " %.2f times/sec ",
> +   key_size, QCryptoRSAPaddingAlgorithm_str(PADDING),
> +   QCryptoHashAlgorithm_str(HASH),
> +   count, g_test_timer_last(),
> +   (double)count / g_test_timer_last());
> +}
> +
> +static void test_rsa_1024_speed(const void *opaque)
> +{
> 

Re: [PATCH v6 5/9] crypto: Implement RSA algorithm by hogweed

2022-05-23 Thread Daniel P . Berrangé
On Sat, May 14, 2022 at 08:55:00AM +0800, zhenwei pi wrote:
> From: Lei He 
> 
> Implement RSA algorithm by hogweed from nettle. Thus QEMU supports
> a 'real' RSA backend to handle request from guest side. It's
> important to test RSA offload case without OS & hardware requirement.
> 
> Signed-off-by: lei he 
> Signed-off-by: zhenwei pi 
> ---
>  crypto/akcipher-nettle.c.inc | 451 +++
>  crypto/akcipher.c|   4 +
>  crypto/meson.build   |   4 +
>  crypto/rsakey-builtin.c.inc  | 200 
>  crypto/rsakey-nettle.c.inc   | 158 
>  crypto/rsakey.c  |  44 
>  crypto/rsakey.h  |  94 
>  meson.build  |  11 +
>  8 files changed, 966 insertions(+)
>  create mode 100644 crypto/akcipher-nettle.c.inc
>  create mode 100644 crypto/rsakey-builtin.c.inc
>  create mode 100644 crypto/rsakey-nettle.c.inc
>  create mode 100644 crypto/rsakey.c
>  create mode 100644 crypto/rsakey.h


> diff --git a/crypto/rsakey.h b/crypto/rsakey.h
> new file mode 100644
> index 00..17bb22333d
> --- /dev/null
> +++ b/crypto/rsakey.h
> @@ -0,0 +1,94 @@
> +
> +#ifndef QCRYPTO_RSAKEY_H
> +#define QCRYPTO_RSAKEY_H
> +
> +#include 

This should be removed -it isn't needed and breaks build on
without-nettle build configurations.

With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH v6 5/9] crypto: Implement RSA algorithm by hogweed

2022-05-23 Thread Daniel P . Berrangé
On Sat, May 14, 2022 at 08:55:00AM +0800, zhenwei pi wrote:
> From: Lei He 
> 
> Implement RSA algorithm by hogweed from nettle. Thus QEMU supports
> a 'real' RSA backend to handle request from guest side. It's
> important to test RSA offload case without OS & hardware requirement.
> 
> Signed-off-by: lei he 
> Signed-off-by: zhenwei pi 
> ---
>  crypto/akcipher-nettle.c.inc | 451 +++
>  crypto/akcipher.c|   4 +
>  crypto/meson.build   |   4 +
>  crypto/rsakey-builtin.c.inc  | 200 
>  crypto/rsakey-nettle.c.inc   | 158 
>  crypto/rsakey.c  |  44 
>  crypto/rsakey.h  |  94 
>  meson.build  |  11 +
>  8 files changed, 966 insertions(+)
>  create mode 100644 crypto/akcipher-nettle.c.inc
>  create mode 100644 crypto/rsakey-builtin.c.inc
>  create mode 100644 crypto/rsakey-nettle.c.inc
>  create mode 100644 crypto/rsakey.c
>  create mode 100644 crypto/rsakey.h
> 
> diff --git a/crypto/akcipher-nettle.c.inc b/crypto/akcipher-nettle.c.inc
> new file mode 100644
> index 00..0796bddcaa
> --- /dev/null
> +++ b/crypto/akcipher-nettle.c.inc

> +static int qcrypto_nettle_rsa_encrypt(QCryptoAkCipher *akcipher,
> +  const void *data, size_t data_len,
> +  void *enc, size_t enc_len,
> +  Error **errp)
> +{
> +
> +QCryptoNettleRSA *rsa = (QCryptoNettleRSA *)akcipher;
> +mpz_t c;
> +int ret = -1;
> +
> +if (data_len > rsa->pub.size) {
> +error_setg(errp, "Plaintext length should be less than key size: 
> %lu",
> +   rsa->pub.size);
> +return ret;
> +}

This needs to include both the good & bad values. I'm going to make
the following changes to error messages:

ie

+error_setg(errp, "Plaintext length %zu is greater than key size: %lu"
+   data_len, rsa->pub.size);
 return ret;
 }


But also the '%lu' needs to change to '%zu' because the rsa->pub.size
parameter is 'size_t'.  %lu doesn't match size_t on 32-bit hosts.

The same issues appear in several other error messages through this
file

With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH v6 6/9] crypto: Implement RSA algorithm by gcrypt

2022-05-23 Thread Daniel P . Berrangé
On Sat, May 14, 2022 at 08:55:01AM +0800, zhenwei pi wrote:
> From: Lei He 
> 
> Added gcryt implementation of RSA algorithm, RSA algorithm
> implemented by gcrypt has a higher priority than nettle because
> it supports raw padding.
> 
> Signed-off-by: zhenwei pi 
> Signed-off-by: lei he 
> ---
>  crypto/akcipher-gcrypt.c.inc | 597 +++
>  crypto/akcipher.c|   4 +-
>  2 files changed, 600 insertions(+), 1 deletion(-)
>  create mode 100644 crypto/akcipher-gcrypt.c.inc
> 
> diff --git a/crypto/akcipher-gcrypt.c.inc b/crypto/akcipher-gcrypt.c.inc
> new file mode 100644
> index 00..6c5daa301e
> --- /dev/null
> +++ b/crypto/akcipher-gcrypt.c.inc
> @@ -0,0 +1,597 @@
> +/*
> + * QEMU Crypto akcipher algorithms
> + *
> + * Copyright (c) 2022 Bytedance
> + * Author: lei he 
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see 
> .
> + *
> + */
> +
> +#include 
> +static QCryptoGcryptRSA *qcrypto_gcrypt_rsa_new(
> +const QCryptoAkCipherOptionsRSA *opt,
> +QCryptoAkCipherKeyType type,
> +const uint8_t *key, size_t keylen,
> +Error **errp)
> +{
> +QCryptoGcryptRSA *rsa = g_new0(QCryptoGcryptRSA, 1);
> +rsa->padding_alg = opt->padding_alg;
> +rsa->hash_alg = opt->hash_alg;
> +rsa->akcipher.driver = _rsa;
> +
> +switch (type) {
> +case QCRYPTO_AKCIPHER_KEY_TYPE_PRIVATE:
> +if (qcrypto_gcrypt_parse_rsa_private_key(rsa, key, keylen, errp) != 
> 0) {
> +error_setg(errp, "Failed to parse rsa private key");

Not need now, since qcrypto_gcrypt_parse_rsa_private_key reports the
real error message.

> +goto error;
> +}
> +break;
> +
> +case QCRYPTO_AKCIPHER_KEY_TYPE_PUBLIC:
> +if (qcrypto_gcrypt_parse_rsa_public_key(rsa, key, keylen, errp) != 
> 0) {
> +error_setg(errp, "Failed to parse rsa public rsa key");

Likewise not needed.

> +goto error;
> +}
> +break;
> +
> +default:
> +error_setg(errp, "Unknown akcipher key type %d", type);
> +goto error;
> +}
> +
> +return rsa;
> +
> +error:
> +qcrypto_gcrypt_rsa_free((QCryptoAkCipher *)rsa);
> +return NULL;
> +}

With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH V5 0/9] rework on the IRQ hardening of virtio

2022-05-23 Thread Halil Pasic
On Wed, 18 May 2022 11:59:42 +0800
Jason Wang  wrote:

> Hi All:

Sorry for being slow on this one. I'm pretty much under water. Will try
to get some regression-testing done till tomorrow end of day.

Regards,
Halil
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


[PATCH] vhost-vdpa: return -EFAULT on copy_to_user() failure

2022-05-23 Thread Dan Carpenter
The copy_to_user() function returns the number of bytes remaining to be
copied.  However, we need to return a negative error code, -EFAULT, to
the user.

Fixes: 87f4c217413a ("vhost-vdpa: introduce uAPI to get the number of virtqueue 
groups")
Fixes: e96ef636f154 ("vhost-vdpa: introduce uAPI to get the number of address 
spaces")
Signed-off-by: Dan Carpenter 
---
 drivers/vhost/vdpa.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 3e86080041fc..935a1d0ddb97 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -609,11 +609,13 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
r = vhost_vdpa_get_vring_num(v, argp);
break;
case VHOST_VDPA_GET_GROUP_NUM:
-   r = copy_to_user(argp, >vdpa->ngroups,
-sizeof(v->vdpa->ngroups));
+   if (copy_to_user(argp, >vdpa->ngroups,
+sizeof(v->vdpa->ngroups)))
+   r = -EFAULT;
break;
case VHOST_VDPA_GET_AS_NUM:
-   r = copy_to_user(argp, >vdpa->nas, sizeof(v->vdpa->nas));
+   if (copy_to_user(argp, >vdpa->nas, sizeof(v->vdpa->nas)))
+   r = -EFAULT;
break;
case VHOST_SET_LOG_BASE:
case VHOST_SET_LOG_FD:
-- 
2.35.1

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


[PATCH] vdpasim: Off by one in vdpasim_set_group_asid()

2022-05-23 Thread Dan Carpenter
The > comparison needs to be >= to prevent an out of bounds access
of the vdpasim->iommu[] array.  The vdpasim->iommu[] is allocated in
vdpasim_create() and it has vdpasim->dev_attr.nas elements.

Fixes: 87e5afeac247 ("vdpasim: control virtqueue support")
Signed-off-by: Dan Carpenter 
---
 drivers/vdpa/vdpa_sim/vdpa_sim.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
index 50d721072beb..0f2865899647 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
@@ -567,7 +567,7 @@ static int vdpasim_set_group_asid(struct vdpa_device *vdpa, 
unsigned int group,
if (group > vdpasim->dev_attr.ngroups)
return -EINVAL;
 
-   if (asid > vdpasim->dev_attr.nas)
+   if (asid >= vdpasim->dev_attr.nas)
return -EINVAL;
 
iommu = >iommu[asid];
-- 
2.35.1

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH 4/4] vdpa_sim: Implement stop vdpa op

2022-05-23 Thread Stefano Garzarella

On Fri, May 20, 2022 at 07:23:25PM +0200, Eugenio Pérez wrote:

Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer
that backend feature and userspace can effectively stop the device.

This is a must before get virtqueue indexes (base) for live migration,
since the device could modify them after userland gets them. There are
individual ways to perform that action for some devices
(VHOST_NET_SET_BACKEND, VHOST_VSOCK_SET_RUNNING, ...) but there was no
way to perform it for any vhost device (and, in particular, vhost-vdpa).

Signed-off-by: Eugenio Pérez 
---
drivers/vdpa/vdpa_sim/vdpa_sim.c | 21 +
drivers/vdpa/vdpa_sim/vdpa_sim.h |  1 +
drivers/vdpa/vdpa_sim/vdpa_sim_net.c |  3 +++
3 files changed, 25 insertions(+)

diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
index 50d721072beb..0515cf314bed 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
@@ -107,6 +107,7 @@ static void vdpasim_do_reset(struct vdpasim *vdpasim)
for (i = 0; i < vdpasim->dev_attr.nas; i++)
vhost_iotlb_reset(>iommu[i]);

+   vdpasim->running = true;
spin_unlock(>iommu_lock);

vdpasim->features = 0;
@@ -505,6 +506,24 @@ static int vdpasim_reset(struct vdpa_device *vdpa)
return 0;
}

+static int vdpasim_stop(struct vdpa_device *vdpa, bool stop)
+{
+   struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
+   int i;
+
+   spin_lock(>lock);
+   vdpasim->running = !stop;
+   if (vdpasim->running) {
+   /* Check for missed buffers */
+   for (i = 0; i < vdpasim->dev_attr.nvqs; ++i)
+   vdpasim_kick_vq(vdpa, i);
+
+   }
+   spin_unlock(>lock);
+
+   return 0;
+}
+
static size_t vdpasim_get_config_size(struct vdpa_device *vdpa)
{
struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
@@ -694,6 +713,7 @@ static const struct vdpa_config_ops vdpasim_config_ops = {
.get_status = vdpasim_get_status,
.set_status = vdpasim_set_status,
.reset  = vdpasim_reset,
+   .stop   = vdpasim_stop,
.get_config_size= vdpasim_get_config_size,
.get_config = vdpasim_get_config,
.set_config = vdpasim_set_config,
@@ -726,6 +746,7 @@ static const struct vdpa_config_ops 
vdpasim_batch_config_ops = {
.get_status = vdpasim_get_status,
.set_status = vdpasim_set_status,
.reset  = vdpasim_reset,
+   .stop   = vdpasim_stop,
.get_config_size= vdpasim_get_config_size,
.get_config = vdpasim_get_config,
.set_config = vdpasim_set_config,
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.h b/drivers/vdpa/vdpa_sim/vdpa_sim.h
index 622782e92239..061986f30911 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.h
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.h
@@ -66,6 +66,7 @@ struct vdpasim {
u32 generation;
u64 features;
u32 groups;
+   bool running;
/* spinlock to synchronize iommu table */
spinlock_t iommu_lock;
};
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c 
b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
index 5125976a4df8..886449e88502 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
@@ -154,6 +154,9 @@ static void vdpasim_net_work(struct work_struct *work)

spin_lock(>lock);

+   if (!vdpasim->running)
+   goto out;
+


It would be nice to do the same for vdpa_sim_blk as well.

Thanks,
Stefano

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization