On Thu, Apr 28, 2022 at 11:13 PM Stefano Garzarella <sgarzare at redhat.com> wrote:> > The simulator behaves like a ramdisk, so we don't have to do > anything when a VIRTIO_BLK_T_FLUSH request is received, but it > could be useful to test driver behavior. > > Let's expose the VIRTIO_BLK_F_FLUSH feature to inform the driver > that we support the flush command. > > Signed-off-by: Stefano Garzarella <sgarzare at redhat.com> > --- > drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c > index 42d401d43911..a6dd1233797c 100644 > --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c > @@ -25,6 +25,7 @@ > #define DRV_LICENSE "GPL v2" > > #define VDPASIM_BLK_FEATURES (VDPASIM_FEATURES | \ > + (1ULL << VIRTIO_BLK_F_FLUSH) | \ > (1ULL << VIRTIO_BLK_F_SIZE_MAX) | \ > (1ULL << VIRTIO_BLK_F_SEG_MAX) | \ > (1ULL << VIRTIO_BLK_F_BLK_SIZE) | \ > @@ -166,6 +167,17 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim, > pushed += bytes; > break; > > + case VIRTIO_BLK_T_FLUSH: > + if (sector != 0) { > + dev_err(&vdpasim->vdpa.dev, > + "A driver MUST set sector to 0 for a VIRTIO_BLK_T_FLUSH request - sector: 0x%llx\n", > + sector);If this is something that could be triggered by userspace/guest, then we should avoid this. Thanks> + status = VIRTIO_BLK_S_IOERR; > + break; > + } > + > + break; > + > default: > dev_warn(&vdpasim->vdpa.dev, > "Unsupported request type %d\n", type); > -- > 2.35.1 >
Stefano Garzarella
2022-Apr-29 07:14 UTC
[PATCH] vdpa_sim_blk: add support for VIRTIO_BLK_T_FLUSH
On Fri, Apr 29, 2022 at 10:46:40AM +0800, Jason Wang wrote:>On Thu, Apr 28, 2022 at 11:13 PM Stefano Garzarella <sgarzare at redhat.com> wrote: >> >> The simulator behaves like a ramdisk, so we don't have to do >> anything when a VIRTIO_BLK_T_FLUSH request is received, but it >> could be useful to test driver behavior. >> >> Let's expose the VIRTIO_BLK_F_FLUSH feature to inform the driver >> that we support the flush command. >> >> Signed-off-by: Stefano Garzarella <sgarzare at redhat.com> >> --- >> drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c >> index 42d401d43911..a6dd1233797c 100644 >> --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c >> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c >> @@ -25,6 +25,7 @@ >> #define DRV_LICENSE "GPL v2" >> >> #define VDPASIM_BLK_FEATURES (VDPASIM_FEATURES | \ >> + (1ULL << VIRTIO_BLK_F_FLUSH) | \ >> (1ULL << VIRTIO_BLK_F_SIZE_MAX) | \ >> (1ULL << VIRTIO_BLK_F_SEG_MAX) | \ >> (1ULL << VIRTIO_BLK_F_BLK_SIZE) | \ >> @@ -166,6 +167,17 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim, >> pushed += bytes; >> break; >> >> + case VIRTIO_BLK_T_FLUSH: >> + if (sector != 0) { >> + dev_err(&vdpasim->vdpa.dev, >> + "A driver MUST set sector to 0 for a VIRTIO_BLK_T_FLUSH request - sector: 0x%llx\n", >> + sector); > >If this is something that could be triggered by userspace/guest, then >we should avoid this.It can only be triggered by an erratic driver. I was using the simulator to test a virtio-blk driver that I'm writing in userspace and I forgot to set `sector` to zero, so I thought it would be useful. Do you mean to remove the error message? Thanks, Stefano
On Fri, Apr 29, 2022 at 3:14 PM Stefano Garzarella <sgarzare at redhat.com> wrote:> > On Fri, Apr 29, 2022 at 10:46:40AM +0800, Jason Wang wrote: > >On Thu, Apr 28, 2022 at 11:13 PM Stefano Garzarella <sgarzare at redhat.com> wrote: > >> > >> The simulator behaves like a ramdisk, so we don't have to do > >> anything when a VIRTIO_BLK_T_FLUSH request is received, but it > >> could be useful to test driver behavior. > >> > >> Let's expose the VIRTIO_BLK_F_FLUSH feature to inform the driver > >> that we support the flush command. > >> > >> Signed-off-by: Stefano Garzarella <sgarzare at redhat.com> > >> --- > >> drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 12 ++++++++++++ > >> 1 file changed, 12 insertions(+) > >> > >> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c > >> index 42d401d43911..a6dd1233797c 100644 > >> --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c > >> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c > >> @@ -25,6 +25,7 @@ > >> #define DRV_LICENSE "GPL v2" > >> > >> #define VDPASIM_BLK_FEATURES (VDPASIM_FEATURES | \ > >> + (1ULL << VIRTIO_BLK_F_FLUSH) | \ > >> (1ULL << VIRTIO_BLK_F_SIZE_MAX) | \ > >> (1ULL << VIRTIO_BLK_F_SEG_MAX) | \ > >> (1ULL << VIRTIO_BLK_F_BLK_SIZE) | \ > >> @@ -166,6 +167,17 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim, > >> pushed += bytes; > >> break; > >> > >> + case VIRTIO_BLK_T_FLUSH: > >> + if (sector != 0) { > >> + dev_err(&vdpasim->vdpa.dev, > >> + "A driver MUST set sector to 0 for a VIRTIO_BLK_T_FLUSH request - sector: 0x%llx\n", > >> + sector); > > > >If this is something that could be triggered by userspace/guest, then > >we should avoid this. > > It can only be triggered by an erratic driver.Right, so guest can try to DOS the host via this.> > I was using the simulator to test a virtio-blk driver that I'm writing > in userspace and I forgot to set `sector` to zero, so I thought it would > be useful. > > Do you mean to remove the error message?Some like dev_warn_once() might be better here. Thanks> > Thanks, > Stefano >