| Message ID | 1352796612-49081-3-git-send-email-borntraeger@de.ibm.com |
|---|---|
| State | New |
| Headers | show |
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 > >
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 >> >> > >
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 >>> >>> >> >> >
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?
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
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;
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(+)