diff mbox

Pass the drive's readonly attribute to the guest OS

Message ID 4AD5F3BD.2040402@redhat.com
State New
Headers show

Commit Message

Naphtali Sprei Oct. 14, 2009, 3:52 p.m. UTC
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(-)

Comments

Gerd Hoffmann Oct. 14, 2009, 3:57 p.m. UTC | #1
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
Naphtali Sprei Oct. 14, 2009, 3:59 p.m. UTC | #2
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
>
Gerd Hoffmann Oct. 14, 2009, 4:07 p.m. UTC | #3
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
Naphtali Sprei Oct. 14, 2009, 4:32 p.m. UTC | #4
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.
Naphtali Sprei Oct. 14, 2009, 4:40 p.m. UTC | #5
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
Kevin Wolf Oct. 15, 2009, 9:36 a.m. UTC | #6
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
Gleb Natapov Oct. 15, 2009, 9:43 a.m. UTC | #7
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.
Kevin Wolf Oct. 15, 2009, 9:50 a.m. UTC | #8
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
Gleb Natapov Oct. 15, 2009, 9:54 a.m. UTC | #9
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.
Kevin Wolf Oct. 15, 2009, 9:55 a.m. UTC | #10
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
Gleb Natapov Oct. 15, 2009, 10:01 a.m. UTC | #11
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.
Kevin Wolf Oct. 15, 2009, 10:05 a.m. UTC | #12
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
Gleb Natapov Oct. 15, 2009, 10:11 a.m. UTC | #13
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.
Kevin Wolf Oct. 15, 2009, 10:18 a.m. UTC | #14
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
diff mbox

Patch

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;
 }