[Qemu-devel] [PATCH] Pass the drive's readonly attribute to the guest OS

[Qemu-devel] [PATCH] Pass the drive's readonly attribute to the guest OS

[Naphtali Sprei]

Hi,

I've seen my patch (http://repo.or.cz/w/qemu/aliguori-queue.git?a=commit;h=2286b94c7458cd6d72883990b53500194975c2ff)
in the staging tree, but the patch relies on a previous patch (http://lists.gnu.org/archive/html/qemu-devel/2009-10/msg01316.html)
I sent that I cannot find in the staging tree, nor committed.
So here is the missing patch again, against current head.

 Naphtali

Subject: [PATCH] Pass the drive's readonly attribute to the guest OS
 Implemented for virtio-blk and for scsi


Signed-off-by: Naphtali Sprei <nsprei@redhat.com>
---
 hw/scsi-disk.c  |    3 ++-
 hw/virtio-blk.c |    3 +++
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 2a9268a..5da573d 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -633,7 +633,8 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
             memset(p, 0, 4);
             outbuf[1] = 0; /* Default media type.  */
             outbuf[3] = 0; /* Block descriptor length.  */
-            if (bdrv_get_type_hint(s->dinfo->bdrv) == BDRV_TYPE_CDROM) {
+            if (bdrv_get_type_hint(s->dinfo->bdrv) == BDRV_TYPE_CDROM ||
+                bdrv_is_read_only(s->dinfo->bdrv)) {
                 outbuf[2] = 0x80; /* Readonly.  */
             }
             p += 4;
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 2630b99..e6df9f2 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -444,6 +444,9 @@ static uint32_t virtio_blk_get_features(VirtIODevice *vdev)
 #endif
     if (strcmp(s->serial_str, "0"))
         features |= 1 << VIRTIO_BLK_F_IDENTIFY;
+    
+    if (bdrv_is_read_only(s->bs))
+        features |= 1 << VIRTIO_BLK_F_RO;
 
     return features;
 }
-- 
1.6.3.3

[Qemu-devel] [PATCH] Pass the drive's readonly attribute to the guest OS

[Naphtali Sprei]

Hi,
as a preliminary step for adding read only flag for the -drive command, I've added code to pass
the (existing) read only attribute of the drive to the guest OS (if it bother to ask).

I've added for virtio and for scsi.

Verified it already works for floppy.

Searched like mad (in linux sources) for same read only/write protect attribute for ide, but all I found was only for ide-floppy.
I assume it's not supported in ide drives. Please correct me if I'm wrong.

I don't know about the other interface types for the drive command: sd, mtd, floppy, pflash. Where is usb ??
Will be happy to add those, too, if applicable. Please advise.

I'm planning to investigate where qemu should check the read only attribute before exeuting any write command
to drives, would be sent in a different patch.

 Naphtali

Signed-off-by: Naphtali Sprei <nsprei@redhat.com>
---
 hw/scsi-disk.c  |    3 ++-
 hw/virtio-blk.c |    3 +++
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 3940726..e4511fb 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -631,7 +631,8 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
             memset(p, 0, 4);
             outbuf[1] = 0; /* Default media type.  */
             outbuf[3] = 0; /* Block descriptor length.  */
-            if (bdrv_get_type_hint(s->dinfo->bdrv) == BDRV_TYPE_CDROM) {
+            if (bdrv_get_type_hint(s->dinfo->bdrv) == BDRV_TYPE_CDROM ||
+                bdrv_is_read_only(s->dinfo->bdrv)) {
                 outbuf[2] = 0x80; /* Readonly.  */
             }
             p += 4;
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 2630b99..e6df9f2 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -444,6 +444,9 @@ static uint32_t virtio_blk_get_features(VirtIODevice *vdev)
 #endif
     if (strcmp(s->serial_str, "0"))
         features |= 1 << VIRTIO_BLK_F_IDENTIFY;
+    
+    if (bdrv_is_read_only(s->bs))
+        features |= 1 << VIRTIO_BLK_F_RO;
 
     return features;
 }
-- 
1.6.3.3

Re: [Qemu-devel] [PATCH] Pass the drive's readonly attribute to the guest OS

[Gerd Hoffmann]

On 10/14/09 17:52, Naphtali Sprei wrote:
> Hi,
> as a preliminary step for adding read only flag for the -drive command, I've added code to pass
> the (existing) read only attribute of the drive to the guest OS (if it bother to ask).
>
> I've added for virtio and for scsi.

> Where is usb ??

hw/usb-msd.c

should be already covered by scsi though.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH] Pass the drive's readonly attribute to the guest OS

[Naphtali Sprei]

Gerd Hoffmann wrote:
> On 10/14/09 17:52, Naphtali Sprei wrote:
>> Hi,
>> as a preliminary step for adding read only flag for the -drive
>> command, I've added code to pass
>> the (existing) read only attribute of the drive to the guest OS (if it
>> bother to ask).
>>
>> I've added for virtio and for scsi.
> 
>> Where is usb ??
> 
> hw/usb-msd.c

Thanks,
what I meant is I can't find how can it be specified as a drive interface.


> 
> should be already covered by scsi though.
> 
> cheers,
>   Gerd
> 

Re: [Qemu-devel] [PATCH] Pass the drive's readonly attribute to the guest OS

[Gerd Hoffmann]

On 10/14/09 17:59, Naphtali Sprei wrote:
> Gerd Hoffmann wrote:
>> On 10/14/09 17:52, Naphtali Sprei wrote:
>>> Hi,
>>> as a preliminary step for adding read only flag for the -drive
>>> command, I've added code to pass
>>> the (existing) read only attribute of the drive to the guest OS (if it
>>> bother to ask).
>>>
>>> I've added for virtio and for scsi.
>>
>>> Where is usb ??
>>
>> hw/usb-msd.c
>
> Thanks,
> what I meant is I can't find how can it be specified as a drive interface.

You can use either

   -usbdevice disk:/some/image
   (usb_add in monitor)

or

   -drive if=none,id=pendrive,file=/some/image
   -device usb-storage,drive=pendrive
   (drive_add + device_add in monitor)

cheers,
   Gerd

Re: [Qemu-devel] [PATCH] Pass the drive's readonly attribute to the guest OS

[Naphtali Sprei]

Gerd Hoffmann wrote:
> On 10/14/09 17:59, Naphtali Sprei wrote:
>> Gerd Hoffmann wrote:
>>> On 10/14/09 17:52, Naphtali Sprei wrote:
>>>> Hi,
>>>> as a preliminary step for adding read only flag for the -drive
>>>> command, I've added code to pass
>>>> the (existing) read only attribute of the drive to the guest OS (if it
>>>> bother to ask).
>>>>
>>>> I've added for virtio and for scsi.
>>>
>>>> Where is usb ??
>>>
>>> hw/usb-msd.c
>>
>> Thanks,
>> what I meant is I can't find how can it be specified as a drive
>> interface.
> 
> You can use either
> 
>   -usbdevice disk:/some/image
>   (usb_add in monitor)
> 
> or
> 
>   -drive if=none,id=pendrive,file=/some/image
>   -device usb-storage,drive=pendrive
>   (drive_add + device_add in monitor)
> 
> cheers,
>   Gerd
> 
> 
> 

Thanks.
Verified, it "works", meaning readonly attribute passed to guest.

Re: [Qemu-devel] [PATCH] Pass the drive's readonly attribute to the guest OS

[Naphtali Sprei]

Naphtali Sprei wrote:
<snip>
> I'm planning to investigate where qemu should check the read only attribute before exeuting any write command
> to drives, would be sent in a different patch.

revisiting it, if guest OS knows it's a read only device and tries to modify it, anyhow, we don't really care about error reporting,
as long as qemu doesn't crash (or modify the drive).

 Naphtali

Re: [Qemu-devel] [PATCH] Pass the drive's readonly attribute to the guest OS

[Kevin Wolf]

Am 14.10.2009 18:40, schrieb Naphtali Sprei:
> Naphtali Sprei wrote:
> <snip>
>> I'm planning to investigate where qemu should check the read only attribute before exeuting any write command
>> to drives, would be sent in a different patch.
> 
> revisiting it, if guest OS knows it's a read only device and tries to modify it, anyhow, we don't really care about error reporting,
> as long as qemu doesn't crash (or modify the drive).

If the right response to a write on a read-only device is defined in the
specification (and it most probably is), we should still give the right
response, even though the OS is doing something wrong.

Kevin

Re: [Qemu-devel] [PATCH] Pass the drive's readonly attribute to the guest OS

[Gleb Natapov]

On Thu, Oct 15, 2009 at 11:36:59AM +0200, Kevin Wolf wrote:
> Am 14.10.2009 18:40, schrieb Naphtali Sprei:
> > Naphtali Sprei wrote:
> > <snip>
> >> I'm planning to investigate where qemu should check the read only attribute before exeuting any write command
> >> to drives, would be sent in a different patch.
> > 
> > revisiting it, if guest OS knows it's a read only device and tries to modify it, anyhow, we don't really care about error reporting,
> > as long as qemu doesn't crash (or modify the drive).
> 
> If the right response to a write on a read-only device is defined in the
> specification (and it most probably is), we should still give the right
> response, even though the OS is doing something wrong.
> 
And since our response to write error may be pausing a VM we shouldn't
allow this to be triggered by a guest OS.

--
			Gleb.

Re: [Qemu-devel] [PATCH] Pass the drive's readonly attribute to the guest OS

[Kevin Wolf]

Am 15.10.2009 11:43, schrieb Gleb Natapov:
> On Thu, Oct 15, 2009 at 11:36:59AM +0200, Kevin Wolf wrote:
>> Am 14.10.2009 18:40, schrieb Naphtali Sprei:
>>> Naphtali Sprei wrote:
>>> <snip>
>>>> I'm planning to investigate where qemu should check the read only attribute before exeuting any write command
>>>> to drives, would be sent in a different patch.
>>>
>>> revisiting it, if guest OS knows it's a read only device and tries to modify it, anyhow, we don't really care about error reporting,
>>> as long as qemu doesn't crash (or modify the drive).
>>
>> If the right response to a write on a read-only device is defined in the
>> specification (and it most probably is), we should still give the right
>> response, even though the OS is doing something wrong.
>>
> And since our response to write error may be pausing a VM we shouldn't
> allow this to be triggered by a guest OS.

I thought we only pause the VM if we get an host IO error? But if you do
want to stop it for all errors, you shouldn't start suppressing errors
so that it doesn't stop.

Kevin

Re: [Qemu-devel] [PATCH] Pass the drive's readonly attribute to the guest OS

[Gleb Natapov]

On Thu, Oct 15, 2009 at 11:50:39AM +0200, Kevin Wolf wrote:
> Am 15.10.2009 11:43, schrieb Gleb Natapov:
> > On Thu, Oct 15, 2009 at 11:36:59AM +0200, Kevin Wolf wrote:
> >> Am 14.10.2009 18:40, schrieb Naphtali Sprei:
> >>> Naphtali Sprei wrote:
> >>> <snip>
> >>>> I'm planning to investigate where qemu should check the read only attribute before exeuting any write command
> >>>> to drives, would be sent in a different patch.
> >>>
> >>> revisiting it, if guest OS knows it's a read only device and tries to modify it, anyhow, we don't really care about error reporting,
> >>> as long as qemu doesn't crash (or modify the drive).
> >>
> >> If the right response to a write on a read-only device is defined in the
> >> specification (and it most probably is), we should still give the right
> >> response, even though the OS is doing something wrong.
> >>
> > And since our response to write error may be pausing a VM we shouldn't
> > allow this to be triggered by a guest OS.
> 
> I thought we only pause the VM if we get an host IO error? But if you do
> want to stop it for all errors, you shouldn't start suppressing errors
> so that it doesn't stop.
> 
We pause only on host IO errors, but if we open underlying file as
read only (do we?) and try to write into it we will get an IO error
in the host.

--
			Gleb.

Re: [Qemu-devel] [PATCH] Pass the drive's readonly attribute to the guest OS

[Kevin Wolf]

Am 15.10.2009 11:54, schrieb Gleb Natapov:
> On Thu, Oct 15, 2009 at 11:50:39AM +0200, Kevin Wolf wrote:
>> Am 15.10.2009 11:43, schrieb Gleb Natapov:
>>> On Thu, Oct 15, 2009 at 11:36:59AM +0200, Kevin Wolf wrote:
>>>> Am 14.10.2009 18:40, schrieb Naphtali Sprei:
>>>>> Naphtali Sprei wrote:
>>>>> <snip>
>>>>>> I'm planning to investigate where qemu should check the read only attribute before exeuting any write command
>>>>>> to drives, would be sent in a different patch.
>>>>>
>>>>> revisiting it, if guest OS knows it's a read only device and tries to modify it, anyhow, we don't really care about error reporting,
>>>>> as long as qemu doesn't crash (or modify the drive).
>>>>
>>>> If the right response to a write on a read-only device is defined in the
>>>> specification (and it most probably is), we should still give the right
>>>> response, even though the OS is doing something wrong.
>>>>
>>> And since our response to write error may be pausing a VM we shouldn't
>>> allow this to be triggered by a guest OS.
>>
>> I thought we only pause the VM if we get an host IO error? But if you do
>> want to stop it for all errors, you shouldn't start suppressing errors
>> so that it doesn't stop.
>>
> We pause only on host IO errors, but if we open underlying file as
> read only (do we?) and try to write into it we will get an IO error
> in the host.

No, we'll return an error before a write request to the host is even issued.

Kevin

Re: [Qemu-devel] [PATCH] Pass the drive's readonly attribute to the guest OS

[Gleb Natapov]

On Thu, Oct 15, 2009 at 11:55:20AM +0200, Kevin Wolf wrote:
> Am 15.10.2009 11:54, schrieb Gleb Natapov:
> > On Thu, Oct 15, 2009 at 11:50:39AM +0200, Kevin Wolf wrote:
> >> Am 15.10.2009 11:43, schrieb Gleb Natapov:
> >>> On Thu, Oct 15, 2009 at 11:36:59AM +0200, Kevin Wolf wrote:
> >>>> Am 14.10.2009 18:40, schrieb Naphtali Sprei:
> >>>>> Naphtali Sprei wrote:
> >>>>> <snip>
> >>>>>> I'm planning to investigate where qemu should check the read only attribute before exeuting any write command
> >>>>>> to drives, would be sent in a different patch.
> >>>>>
> >>>>> revisiting it, if guest OS knows it's a read only device and tries to modify it, anyhow, we don't really care about error reporting,
> >>>>> as long as qemu doesn't crash (or modify the drive).
> >>>>
> >>>> If the right response to a write on a read-only device is defined in the
> >>>> specification (and it most probably is), we should still give the right
> >>>> response, even though the OS is doing something wrong.
> >>>>
> >>> And since our response to write error may be pausing a VM we shouldn't
> >>> allow this to be triggered by a guest OS.
> >>
> >> I thought we only pause the VM if we get an host IO error? But if you do
> >> want to stop it for all errors, you shouldn't start suppressing errors
> >> so that it doesn't stop.
> >>
> > We pause only on host IO errors, but if we open underlying file as
> > read only (do we?) and try to write into it we will get an IO error
> > in the host.
> 
> No, we'll return an error before a write request to the host is even issued.
> 
Who is "we"? If "we" == "bdrv_write()/dma_bdrv_write()" then it's all the same.
Upper layers don't actually care why block driver failed.

--
			Gleb.

Re: [Qemu-devel] [PATCH] Pass the drive's readonly attribute to the guest OS

[Kevin Wolf]

Am 15.10.2009 12:01, schrieb Gleb Natapov:
> On Thu, Oct 15, 2009 at 11:55:20AM +0200, Kevin Wolf wrote:
>> Am 15.10.2009 11:54, schrieb Gleb Natapov:
>>> On Thu, Oct 15, 2009 at 11:50:39AM +0200, Kevin Wolf wrote:
>>>> Am 15.10.2009 11:43, schrieb Gleb Natapov:
>>>>> On Thu, Oct 15, 2009 at 11:36:59AM +0200, Kevin Wolf wrote:
>>>>>> Am 14.10.2009 18:40, schrieb Naphtali Sprei:
>>>>>>> Naphtali Sprei wrote:
>>>>>>> <snip>
>>>>>>>> I'm planning to investigate where qemu should check the read only attribute before exeuting any write command
>>>>>>>> to drives, would be sent in a different patch.
>>>>>>>
>>>>>>> revisiting it, if guest OS knows it's a read only device and tries to modify it, anyhow, we don't really care about error reporting,
>>>>>>> as long as qemu doesn't crash (or modify the drive).
>>>>>>
>>>>>> If the right response to a write on a read-only device is defined in the
>>>>>> specification (and it most probably is), we should still give the right
>>>>>> response, even though the OS is doing something wrong.
>>>>>>
>>>>> And since our response to write error may be pausing a VM we shouldn't
>>>>> allow this to be triggered by a guest OS.
>>>>
>>>> I thought we only pause the VM if we get an host IO error? But if you do
>>>> want to stop it for all errors, you shouldn't start suppressing errors
>>>> so that it doesn't stop.
>>>>
>>> We pause only on host IO errors, but if we open underlying file as
>>> read only (do we?) and try to write into it we will get an IO error
>>> in the host.
>>
>> No, we'll return an error before a write request to the host is even issued.
>>
> Who is "we"? If "we" == "bdrv_write()/dma_bdrv_write()" then it's all the same.
> Upper layers don't actually care why block driver failed.

Right, "we" is the qemu block layer. If the devices don't use the error
code returned, they better should be fixed, I think?

Kevin

Re: [Qemu-devel] [PATCH] Pass the drive's readonly attribute to the guest OS

[Gleb Natapov]

On Thu, Oct 15, 2009 at 12:05:41PM +0200, Kevin Wolf wrote:
> >>>>>> If the right response to a write on a read-only device is defined in the
> >>>>>> specification (and it most probably is), we should still give the right
> >>>>>> response, even though the OS is doing something wrong.
> >>>>>>
> >>>>> And since our response to write error may be pausing a VM we shouldn't
> >>>>> allow this to be triggered by a guest OS.
> >>>>
> >>>> I thought we only pause the VM if we get an host IO error? But if you do
> >>>> want to stop it for all errors, you shouldn't start suppressing errors
> >>>> so that it doesn't stop.
> >>>>
> >>> We pause only on host IO errors, but if we open underlying file as
> >>> read only (do we?) and try to write into it we will get an IO error
> >>> in the host.
> >>
> >> No, we'll return an error before a write request to the host is even issued.
> >>
> > Who is "we"? If "we" == "bdrv_write()/dma_bdrv_write()" then it's all the same.
> > Upper layers don't actually care why block driver failed.
> 
> Right, "we" is the qemu block layer. If the devices don't use the error
> code returned, they better should be fixed, I think?
> 
Fixed in what way? There is an option to pause VM on any write error. If
block layer returns write error for whatever reason VM will be paused.
If scsi/ide/virtio knows that the media is read only it shouldn't
issue writes to block layer, but handle it like real HW would.

--
			Gleb.

Re: [Qemu-devel] [PATCH] Pass the drive's readonly attribute to the guest OS

[Kevin Wolf]

Am 15.10.2009 12:11, schrieb Gleb Natapov:
> On Thu, Oct 15, 2009 at 12:05:41PM +0200, Kevin Wolf wrote:
>>>>>>>> If the right response to a write on a read-only device is defined in the
>>>>>>>> specification (and it most probably is), we should still give the right
>>>>>>>> response, even though the OS is doing something wrong.
>>>>>>>>
>>>>>>> And since our response to write error may be pausing a VM we shouldn't
>>>>>>> allow this to be triggered by a guest OS.
>>>>>>
>>>>>> I thought we only pause the VM if we get an host IO error? But if you do
>>>>>> want to stop it for all errors, you shouldn't start suppressing errors
>>>>>> so that it doesn't stop.
>>>>>>
>>>>> We pause only on host IO errors, but if we open underlying file as
>>>>> read only (do we?) and try to write into it we will get an IO error
>>>>> in the host.
>>>>
>>>> No, we'll return an error before a write request to the host is even issued.
>>>>
>>> Who is "we"? If "we" == "bdrv_write()/dma_bdrv_write()" then it's all the same.
>>> Upper layers don't actually care why block driver failed.
>>
>> Right, "we" is the qemu block layer. If the devices don't use the error
>> code returned, they better should be fixed, I think?
>>
> Fixed in what way? There is an option to pause VM on any write error. If
> block layer returns write error for whatever reason VM will be paused.
> If scsi/ide/virtio knows that the media is read only it shouldn't
> issue writes to block layer, but handle it like real HW would.

Ok, I agree if you want to do it this way. How it's done I really don't
care too much, but in the end the guest OS should see the right error. I
had the impression that Naphtali and you wanted to silently ignore
writes to a read-only disk, which would be just wrong.

Kevin