diff mbox series

[04/15] s390-bios: Extend find_dev() for non-virtio devices

Message ID 1548768562-20007-5-git-send-email-jjherne@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series s390: vfio-ccw dasd ipl support | expand

Commit Message

Jason J. Herne Jan. 29, 2019, 1:29 p.m. UTC
We need a method for finding the subchannel of a dasd device. Let's
modify find_dev to handle this since it mostly does what we need. Up to
this point find_dev has been specific to only virtio devices.

Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
Acked-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 pc-bios/s390-ccw/main.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

Comments

Cornelia Huck Feb. 4, 2019, 10:33 a.m. UTC | #1
On Tue, 29 Jan 2019 08:29:11 -0500
"Jason J. Herne" <jjherne@linux.ibm.com> wrote:

> We need a method for finding the subchannel of a dasd device. Let's
> modify find_dev to handle this since it mostly does what we need. Up to
> this point find_dev has been specific to only virtio devices.
> 
> Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
> Acked-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  pc-bios/s390-ccw/main.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Thomas Huth Feb. 11, 2019, 4:38 p.m. UTC | #2
On 2019-01-29 14:29, Jason J. Herne wrote:
> We need a method for finding the subchannel of a dasd device. Let's
> modify find_dev to handle this since it mostly does what we need. Up to
> this point find_dev has been specific to only virtio devices.
> 
> Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
> Acked-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  pc-bios/s390-ccw/main.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index 67df421..7e3f65e 100644
> --- a/pc-bios/s390-ccw/main.c
> +++ b/pc-bios/s390-ccw/main.c
> @@ -49,6 +49,12 @@ unsigned int get_loadparm_index(void)
>      return atoui(loadparm_str);
>  }
>  
> +/*
> + * Find the subchannel connected to the given device (dev_no) and fill in the
> + * subchannel information block (schib) with the connected subchannel's info.
> + * NOTE: The global variable blk_schid is updated to contain the subchannel
> + * information.
> + */
>  static bool find_dev(Schib *schib, int dev_no)
>  {
>      int i, r;
> @@ -62,15 +68,15 @@ static bool find_dev(Schib *schib, int dev_no)
>          if (!schib->pmcw.dnv) {
>              continue;
>          }
> -        if (!virtio_is_supported(blk_schid)) {
> -            continue;
> -        }
> +
>          /* Skip net devices since no IPLB is created and therefore no
> -         * no network bootloader has been loaded
> +         * network bootloader has been loaded
>           */
> -        if (virtio_get_device_type() == VIRTIO_ID_NET && dev_no < 0) {
> +        if (virtio_is_supported(blk_schid) &&
> +            virtio_get_device_type() == VIRTIO_ID_NET && dev_no < 0) {
>              continue;
>          }
> +
>          if ((dev_no < 0) || (schib->pmcw.dev == dev_no)) {
>              return true;
>          }
> 

Not sure whether this really works as expected? If dev_no is -1, this
used to return the first supported virtio device. Now it returns the
first device that could be found - but how are we sure that we can boot
from that device?

 Thomas
Jason J. Herne Feb. 13, 2019, 1:59 p.m. UTC | #3
On 2/11/19 11:38 AM, Thomas Huth wrote:
> On 2019-01-29 14:29, Jason J. Herne wrote:
>> We need a method for finding the subchannel of a dasd device. Let's
>> modify find_dev to handle this since it mostly does what we need. Up to
>> this point find_dev has been specific to only virtio devices.
>>
>> Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
>> Acked-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> ---
>>   pc-bios/s390-ccw/main.c | 16 +++++++++++-----
>>   1 file changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
>> index 67df421..7e3f65e 100644
>> --- a/pc-bios/s390-ccw/main.c
>> +++ b/pc-bios/s390-ccw/main.c
>> @@ -49,6 +49,12 @@ unsigned int get_loadparm_index(void)
>>       return atoui(loadparm_str);
>>   }
>>   
>> +/*
>> + * Find the subchannel connected to the given device (dev_no) and fill in the
>> + * subchannel information block (schib) with the connected subchannel's info.
>> + * NOTE: The global variable blk_schid is updated to contain the subchannel
>> + * information.
>> + */
>>   static bool find_dev(Schib *schib, int dev_no)
>>   {
>>       int i, r;
>> @@ -62,15 +68,15 @@ static bool find_dev(Schib *schib, int dev_no)
>>           if (!schib->pmcw.dnv) {
>>               continue;
>>           }
>> -        if (!virtio_is_supported(blk_schid)) {
>> -            continue;
>> -        }
>> +
>>           /* Skip net devices since no IPLB is created and therefore no
>> -         * no network bootloader has been loaded
>> +         * network bootloader has been loaded
>>            */
>> -        if (virtio_get_device_type() == VIRTIO_ID_NET && dev_no < 0) {
>> +        if (virtio_is_supported(blk_schid) &&
>> +            virtio_get_device_type() == VIRTIO_ID_NET && dev_no < 0) {
>>               continue;
>>           }
>> +
>>           if ((dev_no < 0) || (schib->pmcw.dev == dev_no)) {
>>               return true;
>>           }
>>
> 
> Not sure whether this really works as expected? If dev_no is -1, this
> used to return the first supported virtio device. Now it returns the
> first device that could be found - but how are we sure that we can boot
> from that device?
> 

How could we ever be sure we could boot from the first virtio device? The dev_no=1 case 
means we don't know which device to boot from so we're guessing.  The only thing that is 
changing here is that we're allowing non-virtio devices as well. We do this because we now 
support booting from a device type that is not virtio.
Thomas Huth March 4, 2019, 7:23 p.m. UTC | #4
On 13/02/2019 14.59, Jason J. Herne wrote:
> On 2/11/19 11:38 AM, Thomas Huth wrote:
>> On 2019-01-29 14:29, Jason J. Herne wrote:
>>> We need a method for finding the subchannel of a dasd device. Let's
>>> modify find_dev to handle this since it mostly does what we need. Up to
>>> this point find_dev has been specific to only virtio devices.
>>>
>>> Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
>>> Acked-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>> ---
>>>   pc-bios/s390-ccw/main.c | 16 +++++++++++-----
>>>   1 file changed, 11 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
>>> index 67df421..7e3f65e 100644
>>> --- a/pc-bios/s390-ccw/main.c
>>> +++ b/pc-bios/s390-ccw/main.c
>>> @@ -49,6 +49,12 @@ unsigned int get_loadparm_index(void)
>>>       return atoui(loadparm_str);
>>>   }
>>>   +/*
>>> + * Find the subchannel connected to the given device (dev_no) and
>>> fill in the
>>> + * subchannel information block (schib) with the connected
>>> subchannel's info.
>>> + * NOTE: The global variable blk_schid is updated to contain the
>>> subchannel
>>> + * information.
>>> + */
>>>   static bool find_dev(Schib *schib, int dev_no)
>>>   {
>>>       int i, r;
>>> @@ -62,15 +68,15 @@ static bool find_dev(Schib *schib, int dev_no)
>>>           if (!schib->pmcw.dnv) {
>>>               continue;
>>>           }
>>> -        if (!virtio_is_supported(blk_schid)) {
>>> -            continue;
>>> -        }
>>> +
>>>           /* Skip net devices since no IPLB is created and therefore no
>>> -         * no network bootloader has been loaded
>>> +         * network bootloader has been loaded
>>>            */
>>> -        if (virtio_get_device_type() == VIRTIO_ID_NET && dev_no < 0) {
>>> +        if (virtio_is_supported(blk_schid) &&
>>> +            virtio_get_device_type() == VIRTIO_ID_NET && dev_no < 0) {
>>>               continue;
>>>           }
>>> +
>>>           if ((dev_no < 0) || (schib->pmcw.dev == dev_no)) {
>>>               return true;
>>>           }
>>>
>>
>> Not sure whether this really works as expected? If dev_no is -1, this
>> used to return the first supported virtio device. Now it returns the
>> first device that could be found - but how are we sure that we can boot
>> from that device?
> 
> How could we ever be sure we could boot from the first virtio device?
> The dev_no=1 case means we don't know which device to boot from so we're
> guessing.  The only thing that is changing here is that we're allowing
> non-virtio devices as well. We do this because we now support booting
> from a device type that is not virtio.

For example, this command line used to work fine in the past:

s390x-softmmu/qemu-system-s390x -enable-kvm -nographic \
  -device x-terminal3270 -device virtio-blk-ccw,drive=dr1 \
  -drive if=none,id=dr1,format=qcow2,file=guest.qcow2

With this patch applied, the guest is now not bootable anymore. So I'm
sorry, but you break setups here that worked fine in the past. Isn't
there an easy way to determine whether a device is a bootable block
device or not?

 Thomas
diff mbox series

Patch

diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 67df421..7e3f65e 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -49,6 +49,12 @@  unsigned int get_loadparm_index(void)
     return atoui(loadparm_str);
 }
 
+/*
+ * Find the subchannel connected to the given device (dev_no) and fill in the
+ * subchannel information block (schib) with the connected subchannel's info.
+ * NOTE: The global variable blk_schid is updated to contain the subchannel
+ * information.
+ */
 static bool find_dev(Schib *schib, int dev_no)
 {
     int i, r;
@@ -62,15 +68,15 @@  static bool find_dev(Schib *schib, int dev_no)
         if (!schib->pmcw.dnv) {
             continue;
         }
-        if (!virtio_is_supported(blk_schid)) {
-            continue;
-        }
+
         /* Skip net devices since no IPLB is created and therefore no
-         * no network bootloader has been loaded
+         * network bootloader has been loaded
          */
-        if (virtio_get_device_type() == VIRTIO_ID_NET && dev_no < 0) {
+        if (virtio_is_supported(blk_schid) &&
+            virtio_get_device_type() == VIRTIO_ID_NET && dev_no < 0) {
             continue;
         }
+
         if ((dev_no < 0) || (schib->pmcw.dev == dev_no)) {
             return true;
         }