Patchwork [2/3] hd-geometry.c/s390: Disable geometry translation

login
register
mail settings
Submitter Christian Borntraeger
Date Nov. 13, 2012, 8:50 a.m.
Message ID <1352796612-49081-3-git-send-email-borntraeger@de.ibm.com>
Download mbox | patch
Permalink /patch/198565/
State New
Headers show

Comments

Christian Borntraeger - Nov. 13, 2012, 8:50 a.m.
From: Einar Lueck <elelueck@linux.vnet.ibm.com>

This patch disables the translation of geometry information read from
disks on s390. On s390 such translations lead to wrong geometries being
advertized to the guest because there is no entity doing these kinds
of translation on this architecture.

Signed-off-by Einar Lueck <elelueck@linux.vnet.ibm.com>
Signed-off-by Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/hd-geometry.c |    5 +++++
 1 file changed, 5 insertions(+)
Blue Swirl - Nov. 17, 2012, 4:07 p.m.
On Tue, Nov 13, 2012 at 8:50 AM, Christian Borntraeger
<borntraeger@de.ibm.com> wrote:
> From: Einar Lueck <elelueck@linux.vnet.ibm.com>
>
> This patch disables the translation of geometry information read from
> disks on s390. On s390 such translations lead to wrong geometries being
> advertized to the guest because there is no entity doing these kinds
> of translation on this architecture.
>
> Signed-off-by Einar Lueck <elelueck@linux.vnet.ibm.com>
> Signed-off-by Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  hw/hd-geometry.c |    5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/hw/hd-geometry.c b/hw/hd-geometry.c
> index 4cf040d..db1dc81 100644
> --- a/hw/hd-geometry.c
> +++ b/hw/hd-geometry.c
> @@ -144,6 +144,10 @@ static int guess_disk_msdosgeo(BlockDriverState *bs,
>                  continue;
>              }
>
> +#ifdef __s390__

No, this would make all system emulators (e.g. x86) on s390 host to
use this translation, not just s390. I think you want to use #ifdef
CONFIG_S390X instead.

> +            /* on s390 there is no BIOS doing any kind of translation */
> +            translation = BIOS_ATA_TRANSLATION_NONE;
> +#else
>              if (heads > 16) {
>                  /* LCHS guess with heads > 16 means that a BIOS LBA
>                     translation was active, so a standard physical disk
> @@ -158,6 +162,7 @@ static int guess_disk_msdosgeo(BlockDriverState *bs,
>                     the logical geometry */
>                  translation = BIOS_ATA_TRANSLATION_NONE;
>              }
> +#endif
>              *pheads = heads;
>              *psectors = sectors;
>              *pcylinders = cylinders;
> --
> 1.7.10.1
>
>
Paolo Bonzini - Nov. 18, 2012, 4:10 p.m.
Il 17/11/2012 17:07, Blue Swirl ha scritto:
> On Tue, Nov 13, 2012 at 8:50 AM, Christian Borntraeger
> <borntraeger@de.ibm.com> wrote:
>> From: Einar Lueck <elelueck@linux.vnet.ibm.com>
>>
>> This patch disables the translation of geometry information read from
>> disks on s390. On s390 such translations lead to wrong geometries being
>> advertized to the guest because there is no entity doing these kinds
>> of translation on this architecture.
>>
>> Signed-off-by Einar Lueck <elelueck@linux.vnet.ibm.com>
>> Signed-off-by Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  hw/hd-geometry.c |    5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/hw/hd-geometry.c b/hw/hd-geometry.c
>> index 4cf040d..db1dc81 100644
>> --- a/hw/hd-geometry.c
>> +++ b/hw/hd-geometry.c
>> @@ -144,6 +144,10 @@ static int guess_disk_msdosgeo(BlockDriverState *bs,
>>                  continue;
>>              }
>>
>> +#ifdef __s390__
> 
> No, this would make all system emulators (e.g. x86) on s390 host to
> use this translation, not just s390. I think you want to use #ifdef
> CONFIG_S390X instead.

This symbol is not available because this file is compiled just once.
Which non-x86 targets actually care about translation stuff?

Probably it would be best to split the MS-DOS stuff in a separate file,
and use the stub mechanism (no weak symbols, alas :)) to provide a
default implementation that just returns BIOS_ATA_TRANSLATION_NONE.

>> +            /* on s390 there is no BIOS doing any kind of translation */
>> +            translation = BIOS_ATA_TRANSLATION_NONE;
>> +#else
>>              if (heads > 16) {
>>                  /* LCHS guess with heads > 16 means that a BIOS LBA
>>                     translation was active, so a standard physical disk
>> @@ -158,6 +162,7 @@ static int guess_disk_msdosgeo(BlockDriverState *bs,
>>                     the logical geometry */
>>                  translation = BIOS_ATA_TRANSLATION_NONE;
>>              }
>> +#endif
>>              *pheads = heads;
>>              *psectors = sectors;
>>              *pcylinders = cylinders;
>> --
>> 1.7.10.1
>>
>>
> 
>
Blue Swirl - Nov. 18, 2012, 7:14 p.m.
On Sun, Nov 18, 2012 at 4:10 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 17/11/2012 17:07, Blue Swirl ha scritto:
>> On Tue, Nov 13, 2012 at 8:50 AM, Christian Borntraeger
>> <borntraeger@de.ibm.com> wrote:
>>> From: Einar Lueck <elelueck@linux.vnet.ibm.com>
>>>
>>> This patch disables the translation of geometry information read from
>>> disks on s390. On s390 such translations lead to wrong geometries being
>>> advertized to the guest because there is no entity doing these kinds
>>> of translation on this architecture.
>>>
>>> Signed-off-by Einar Lueck <elelueck@linux.vnet.ibm.com>
>>> Signed-off-by Christian Borntraeger <borntraeger@de.ibm.com>
>>> ---
>>>  hw/hd-geometry.c |    5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/hw/hd-geometry.c b/hw/hd-geometry.c
>>> index 4cf040d..db1dc81 100644
>>> --- a/hw/hd-geometry.c
>>> +++ b/hw/hd-geometry.c
>>> @@ -144,6 +144,10 @@ static int guess_disk_msdosgeo(BlockDriverState *bs,
>>>                  continue;
>>>              }
>>>
>>> +#ifdef __s390__
>>
>> No, this would make all system emulators (e.g. x86) on s390 host to
>> use this translation, not just s390. I think you want to use #ifdef
>> CONFIG_S390X instead.
>
> This symbol is not available because this file is compiled just once.
> Which non-x86 targets actually care about translation stuff?

I meant TARGET_S390X, sorry. I have no idea, but hosts and targets
should not be confused.

>
> Probably it would be best to split the MS-DOS stuff in a separate file,
> and use the stub mechanism (no weak symbols, alas :)) to provide a
> default implementation that just returns BIOS_ATA_TRANSLATION_NONE.

A global variable, initialized by target specific code would work too.

>
>>> +            /* on s390 there is no BIOS doing any kind of translation */
>>> +            translation = BIOS_ATA_TRANSLATION_NONE;
>>> +#else
>>>              if (heads > 16) {
>>>                  /* LCHS guess with heads > 16 means that a BIOS LBA
>>>                     translation was active, so a standard physical disk
>>> @@ -158,6 +162,7 @@ static int guess_disk_msdosgeo(BlockDriverState *bs,
>>>                     the logical geometry */
>>>                  translation = BIOS_ATA_TRANSLATION_NONE;
>>>              }
>>> +#endif
>>>              *pheads = heads;
>>>              *psectors = sectors;
>>>              *pcylinders = cylinders;
>>> --
>>> 1.7.10.1
>>>
>>>
>>
>>
>
Christian Borntraeger - Nov. 19, 2012, 10:59 a.m.
On 18/11/12 20:14, Blue Swirl wrote:
> On Sun, Nov 18, 2012 at 4:10 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 17/11/2012 17:07, Blue Swirl ha scritto:
>>> On Tue, Nov 13, 2012 at 8:50 AM, Christian Borntraeger
>>> <borntraeger@de.ibm.com> wrote:
>>>> From: Einar Lueck <elelueck@linux.vnet.ibm.com>
>>>>
>>>> This patch disables the translation of geometry information read from
>>>> disks on s390. On s390 such translations lead to wrong geometries being
>>>> advertized to the guest because there is no entity doing these kinds
>>>> of translation on this architecture.
>>>>
>>>> Signed-off-by Einar Lueck <elelueck@linux.vnet.ibm.com>
>>>> Signed-off-by Christian Borntraeger <borntraeger@de.ibm.com>
>>>> ---
>>>>  hw/hd-geometry.c |    5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/hw/hd-geometry.c b/hw/hd-geometry.c
>>>> index 4cf040d..db1dc81 100644
>>>> --- a/hw/hd-geometry.c
>>>> +++ b/hw/hd-geometry.c
>>>> @@ -144,6 +144,10 @@ static int guess_disk_msdosgeo(BlockDriverState *bs,
>>>>                  continue;
>>>>              }
>>>>
>>>> +#ifdef __s390__
>>>
>>> No, this would make all system emulators (e.g. x86) on s390 host to
>>> use this translation, not just s390. I think you want to use #ifdef
>>> CONFIG_S390X instead.
>>
>> This symbol is not available because this file is compiled just once.
>> Which non-x86 targets actually care about translation stuff?
> 
> I meant TARGET_S390X, sorry. I have no idea, but hosts and targets
> should not be confused.

Would a modified patch with TARGET_S390X be acceptable?
Alexander Graf - Nov. 19, 2012, 11:20 a.m.
On 19.11.2012, at 11:59, Christian Borntraeger wrote:

> On 18/11/12 20:14, Blue Swirl wrote:
>> On Sun, Nov 18, 2012 at 4:10 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> Il 17/11/2012 17:07, Blue Swirl ha scritto:
>>>> On Tue, Nov 13, 2012 at 8:50 AM, Christian Borntraeger
>>>> <borntraeger@de.ibm.com> wrote:
>>>>> From: Einar Lueck <elelueck@linux.vnet.ibm.com>
>>>>> 
>>>>> This patch disables the translation of geometry information read from
>>>>> disks on s390. On s390 such translations lead to wrong geometries being
>>>>> advertized to the guest because there is no entity doing these kinds
>>>>> of translation on this architecture.
>>>>> 
>>>>> Signed-off-by Einar Lueck <elelueck@linux.vnet.ibm.com>
>>>>> Signed-off-by Christian Borntraeger <borntraeger@de.ibm.com>
>>>>> ---
>>>>> hw/hd-geometry.c |    5 +++++
>>>>> 1 file changed, 5 insertions(+)
>>>>> 
>>>>> diff --git a/hw/hd-geometry.c b/hw/hd-geometry.c
>>>>> index 4cf040d..db1dc81 100644
>>>>> --- a/hw/hd-geometry.c
>>>>> +++ b/hw/hd-geometry.c
>>>>> @@ -144,6 +144,10 @@ static int guess_disk_msdosgeo(BlockDriverState *bs,
>>>>>                 continue;
>>>>>             }
>>>>> 
>>>>> +#ifdef __s390__
>>>> 
>>>> No, this would make all system emulators (e.g. x86) on s390 host to
>>>> use this translation, not just s390. I think you want to use #ifdef
>>>> CONFIG_S390X instead.
>>> 
>>> This symbol is not available because this file is compiled just once.
>>> Which non-x86 targets actually care about translation stuff?
>> 
>> I meant TARGET_S390X, sorry. I have no idea, but hosts and targets
>> should not be confused.
> 
> Would a modified patch with TARGET_S390X be acceptable?

If TARGET_S390X is defined in that file (read: it's compiled individually for every target), yes. Otherwise please make this a runtime check and set the runtime checked variable in code that is target specific.


Alex

Patch

diff --git a/hw/hd-geometry.c b/hw/hd-geometry.c
index 4cf040d..db1dc81 100644
--- a/hw/hd-geometry.c
+++ b/hw/hd-geometry.c
@@ -144,6 +144,10 @@  static int guess_disk_msdosgeo(BlockDriverState *bs,
                 continue;
             }
 
+#ifdef __s390__
+            /* on s390 there is no BIOS doing any kind of translation */
+            translation = BIOS_ATA_TRANSLATION_NONE;
+#else
             if (heads > 16) {
                 /* LCHS guess with heads > 16 means that a BIOS LBA
                    translation was active, so a standard physical disk
@@ -158,6 +162,7 @@  static int guess_disk_msdosgeo(BlockDriverState *bs,
                    the logical geometry */
                 translation = BIOS_ATA_TRANSLATION_NONE;
             }
+#endif
             *pheads = heads;
             *psectors = sectors;
             *pcylinders = cylinders;