status grub2 port of grub-legasy map command

status grub2 port of grub-legasy map command

[John Stanley]

Hi all,
I was wondering what the current status of a grub2 port of the grub-0.97 
"map" and "rootnoverify" commands is?  I have found some work done to 
this end in the "drivemap.patch" work, but I find nothing more recent 
than drivemap.patch.8 dated around Aug 2008.

I have taken drivemap.patch.8 and made a few updates so that it builds 
and works, at least partially, in grub2-r2101. I build "homebrew" linux 
systems for my laptops (thinkpads) and desktops and on several of them I 
have a 2nd hd installed in the cd-bay with Windows XP. For dual booting, 
I've been using grub legasy w/o problems using:

title   Windows XP
    rootnoverify (hd1,0)
    map (hd1) (hd0)
    map (hd0) (hd1)
    chainloader +1

For grub2-r2101, I use:

### BEGIN /etc/grub.d/40_custom ###
# This file is an example on how to add custom entries
menuentry "Microsoft Windows XP" {
       drivemap (hd0) (hd1)
       set root=(hd1,1)
       chainloader --force +1
}

When I use drivemap as above, on booting Windows I get the message:

unaligned pointer 0x76697264
Aborted. Press any key to exit

I then simply hit "enter," and Windows begins to boot. So, the mapping 
works, I do have dual-booting with Windows not on the (bios) first 
drive,  but mm.c complains about an unaligned pointer.

Could anyone give me any pointers/direction on what might be happening 
here? Could it be that the "norootverify"-functionality of grub-legasy 
is lacking here? Or, perhaps, that the "--force" option is not being 
honored ?

Any help/suggestions/related-info would be greatly appreciated.

Thus far, I have not been able to get a Windows boot using the "-s" 
switch of drivemap , e.g., "drivemap -s (hd0) (hd1)" as this results in 
a grub Abort, followed by a reboot.

John

Re: status grub2 port of grub-legasy map command

[Felix Zielcke]

Am Montag, den 13.04.2009, 21:03 -0400 schrieb John Stanley:
> Hi all,
> I was wondering what the current status of a grub2 port of the grub-0.97 
> "map" and "rootnoverify" commands is?  I have found some work done to 
> this end in the "drivemap.patch" work, but I find nothing more recent 
> than drivemap.patch.8 dated around Aug 2008.

The current status of it are exactly what you found out.
I don't know if that'll ever change.


> Could anyone give me any pointers/direction on what might be happening 
> here? Could it be that the "norootverify"-functionality of grub-legasy 
> is lacking here? Or, perhaps, that the "--force" option is not being 
> honored ?

rootnoverify isn't needed anymore, because root is now just a variable
and not anymore a command which tried to verify it. So basically
rootnoverify is default now.
chainloader --force just skips the check for 0xaa55, normally it
shouldn't be needed with a valid windows bootsector.
-- 
Felix Zielcke

Re: status grub2 port of grub-legasy map command

[John Stanley]

Thanks Felix,

Hurm.. Well, if anyone is interested, I have just made a couple of 
additional updates to the drivemap.path.8 code,
and now with r2104 the "unaligned pointer" issue is gone, and it is 
working great on my systems. I can post the patch if you or anyone else 
is interested.
John


Felix Zielcke wrote:
> Am Montag, den 13.04.2009, 21:03 -0400 schrieb John Stanley:
>   
>> Hi all,
>> I was wondering what the current status of a grub2 port of the grub-0.97 
>> "map" and "rootnoverify" commands is?  I have found some work done to 
>> this end in the "drivemap.patch" work, but I find nothing more recent 
>> than drivemap.patch.8 dated around Aug 2008.
>>     
>
> The current status of it are exactly what you found out.
> I don't know if that'll ever change.
>
>
>   
>> Could anyone give me any pointers/direction on what might be happening 
>> here? Could it be that the "norootverify"-functionality of grub-legasy 
>> is lacking here? Or, perhaps, that the "--force" option is not being 
>> honored ?
>>     
>
> rootnoverify isn't needed anymore, because root is now just a variable
> and not anymore a command which tried to verify it. So basically
> rootnoverify is default now.
> chainloader --force just skips the check for 0xaa55, normally it
> shouldn't be needed with a valid windows bootsector.
>   

Re: status grub2 port of grub-legasy map command

[phcoder]

I haven't yet looked in depth in drivemap patch but it has some 
problems. It uses preboot hook interface for which I proposed an update 
in my recent patch "preboot hooks". Also it doesn't update memorymap 
correctly. For this it should use my "mmap services" interface
John Stanley wrote:
> Thanks Felix,
> 
> Hurm.. Well, if anyone is interested, I have just made a couple of 
> additional updates to the drivemap.path.8 code,
> and now with r2104 the "unaligned pointer" issue is gone, and it is 
> working great on my systems. I can post the patch if you or anyone else 
> is interested.
> John
> 
> 
> Felix Zielcke wrote:
>> Am Montag, den 13.04.2009, 21:03 -0400 schrieb John Stanley:
>>  
>>> Hi all,
>>> I was wondering what the current status of a grub2 port of the 
>>> grub-0.97 "map" and "rootnoverify" commands is?  I have found some 
>>> work done to this end in the "drivemap.patch" work, but I find 
>>> nothing more recent than drivemap.patch.8 dated around Aug 2008.
>>>     
>>
>> The current status of it are exactly what you found out.
>> I don't know if that'll ever change.
>>
>>
>>  
>>> Could anyone give me any pointers/direction on what might be 
>>> happening here? Could it be that the "norootverify"-functionality of 
>>> grub-legasy is lacking here? Or, perhaps, that the "--force" option 
>>> is not being honored ?
>>>     
>>
>> rootnoverify isn't needed anymore, because root is now just a variable
>> and not anymore a command which tried to verify it. So basically
>> rootnoverify is default now.
>> chainloader --force just skips the check for 0xaa55, normally it
>> shouldn't be needed with a valid windows bootsector.
>>   
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel


-- 

Regards
Vladimir 'phcoder' Serbinenko

Re: status grub2 port of grub-legasy map command

[John Stanley]

I have your preboot hooks patch incorporated into grub-r2106 along with 
a suitably updated version of drivemap, which remains flawlessly working 
with  2nd-harddisk Windows XP boots (grub being installed on the 1st 
harddisk along with Linux). Had to make a few minor changes to the 
preboot hooks patch to get it to build in r2106.

I have also incorporated  your mmap services patch as well (again with  
minor mods to build in r2106). My question at this point, is how best to 
incorporate mmap services into drivemap. I see that in 
mmap/i386/pc/mmap.c there is some sort of support for int12 and int15 
services. Should I incorporate the drivemap int13 handler here ? Looks 
relatively straightforward -- just insert the asm handler into 
mmap/i386/pc/mmap_helper.S and update mmap/i386/pc/mmap.c -- (except for 
how I place the mapped drives table), or, should I use the mmap.c code 
as a template for the drivemap int13 handler plus mapped drives table ?

thanks for any help/suggestions,
John

phcoder wrote:
> I haven't yet looked in depth in drivemap patch but it has some 
> problems. It uses preboot hook interface for which I proposed an 
> update in my recent patch "preboot hooks". Also it doesn't update 
> memorymap correctly. For this it should use my "mmap services" interface
> John Stanley wrote:
>> Thanks Felix,
>>
>> Hurm.. Well, if anyone is interested, I have just made a couple of 
>> additional updates to the drivemap.path.8 code,
>> and now with r2104 the "unaligned pointer" issue is gone, and it is 
>> working great on my systems. I can post the patch if you or anyone 
>> else is interested.
>> John
>>
>>
>> Felix Zielcke wrote:
>>> Am Montag, den 13.04.2009, 21:03 -0400 schrieb John Stanley:
>>>  
>>>> Hi all,
>>>> I was wondering what the current status of a grub2 port of the 
>>>> grub-0.97 "map" and "rootnoverify" commands is?  I have found some 
>>>> work done to this end in the "drivemap.patch" work, but I find 
>>>> nothing more recent than drivemap.patch.8 dated around Aug 2008.
>>>>     
>>>
>>> The current status of it are exactly what you found out.
>>> I don't know if that'll ever change.
>>>
>>>
>>>  
>>>> Could anyone give me any pointers/direction on what might be 
>>>> happening here? Could it be that the "norootverify"-functionality 
>>>> of grub-legasy is lacking here? Or, perhaps, that the "--force" 
>>>> option is not being honored ?
>>>>     
>>>
>>> rootnoverify isn't needed anymore, because root is now just a variable
>>> and not anymore a command which tried to verify it. So basically
>>> rootnoverify is default now.
>>> chainloader --force just skips the check for 0xaa55, normally it
>>> shouldn't be needed with a valid windows bootsector.
>>>   
>>
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>

Re: status grub2 port of grub-legasy map command

[phcoder]

If you want your code to be incorporated you need to sign the copyright 
assignment.
John Stanley wrote:
> I have also incorporated  your mmap services patch as well (again with  
> minor mods to build in r2106). My question at this point, is how best to 
> incorporate mmap services into drivemap. I see that in 
> mmap/i386/pc/mmap.c there is some sort of support for int12 and int15 
> services. Should I incorporate the drivemap int13 handler here ? Looks 
> relatively straightforward -- just insert the asm handler into 
> mmap/i386/pc/mmap_helper.S and update mmap/i386/pc/mmap.c -- (except for 
> how I place the mapped drives table), or, should I use the mmap.c code 
> as a template for the drivemap int13 handler plus mapped drives table ?
Don't do it that way. It merges 2 unrelated modules. At some point 
drivermap does following
*bpaMemInKb -= ...;
But this isn't correct because mmap interrupts still list the memory 
used by drivemap as available. Use grub_mmap_register instead of it.
> 
> thanks for any help/suggestions,
> John
> 
> phcoder wrote:
>> I haven't yet looked in depth in drivemap patch but it has some 
>> problems. It uses preboot hook interface for which I proposed an 
>> update in my recent patch "preboot hooks". Also it doesn't update 
>> memorymap correctly. For this it should use my "mmap services" interface
>> John Stanley wrote:
>>> Thanks Felix,
>>>
>>> Hurm.. Well, if anyone is interested, I have just made a couple of 
>>> additional updates to the drivemap.path.8 code,
>>> and now with r2104 the "unaligned pointer" issue is gone, and it is 
>>> working great on my systems. I can post the patch if you or anyone 
>>> else is interested.
>>> John
>>>
>>>
>>> Felix Zielcke wrote:
>>>> Am Montag, den 13.04.2009, 21:03 -0400 schrieb John Stanley:
>>>>  
>>>>> Hi all,
>>>>> I was wondering what the current status of a grub2 port of the 
>>>>> grub-0.97 "map" and "rootnoverify" commands is?  I have found some 
>>>>> work done to this end in the "drivemap.patch" work, but I find 
>>>>> nothing more recent than drivemap.patch.8 dated around Aug 2008.
>>>>>     
>>>>
>>>> The current status of it are exactly what you found out.
>>>> I don't know if that'll ever change.
>>>>
>>>>
>>>>  
>>>>> Could anyone give me any pointers/direction on what might be 
>>>>> happening here? Could it be that the "norootverify"-functionality 
>>>>> of grub-legasy is lacking here? Or, perhaps, that the "--force" 
>>>>> option is not being honored ?
>>>>>     
>>>>
>>>> rootnoverify isn't needed anymore, because root is now just a variable
>>>> and not anymore a command which tried to verify it. So basically
>>>> rootnoverify is default now.
>>>> chainloader --force just skips the check for 0xaa55, normally it
>>>> shouldn't be needed with a valid windows bootsector.
>>>>   
>>>
>>>
>>> _______________________________________________
>>> Grub-devel mailing list
>>> Grub-devel@gnu.org
>>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>
>>
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel


-- 

Regards
Vladimir 'phcoder' Serbinenko

Re: status grub2 port of grub-legasy map command

[John Stanley]

I'd be happy to sign a copywrite statement, no problem -- how do I go 
about it ?

Is this what you're referring to:

        /* BDA offset 0x13 contains the top of conventional memory, in 
kiB.  */
      grub_uint16_t *bpa_freekb = (grub_uint16_t*)0x00000413;
      .
      .
      *bpa_freekb -= payload_sizekb;

where payload_sizekb is the size of the drivemap int13 handler + its 
mapped drive table ?


phcoder wrote:
> If you want your code to be incorporated you need to sign the 
> copyright assignment.
> John Stanley wrote:
>> I have also incorporated  your mmap services patch as well (again 
>> with  minor mods to build in r2106). My question at this point, is 
>> how best to incorporate mmap services into drivemap. I see that in 
>> mmap/i386/pc/mmap.c there is some sort of support for int12 and int15 
>> services. Should I incorporate the drivemap int13 handler here ? 
>> Looks relatively straightforward -- just insert the asm handler into 
>> mmap/i386/pc/mmap_helper.S and update mmap/i386/pc/mmap.c -- (except 
>> for how I place the mapped drives table), or, should I use the mmap.c 
>> code as a template for the drivemap int13 handler plus mapped drives 
>> table ?
> Don't do it that way. It merges 2 unrelated modules. At some point 
> drivermap does following
> *bpaMemInKb -= ...;
> But this isn't correct because mmap interrupts still list the memory 
> used by drivemap as available. Use grub_mmap_register instead of it.
>>
>> thanks for any help/suggestions,
>> John
>>
>> phcoder wrote:
>>> I haven't yet looked in depth in drivemap patch but it has some 
>>> problems. It uses preboot hook interface for which I proposed an 
>>> update in my recent patch "preboot hooks". Also it doesn't update 
>>> memorymap correctly. For this it should use my "mmap services" 
>>> interface
>>> John Stanley wrote:
>>>> Thanks Felix,
>>>>
>>>> Hurm.. Well, if anyone is interested, I have just made a couple of 
>>>> additional updates to the drivemap.path.8 code,
>>>> and now with r2104 the "unaligned pointer" issue is gone, and it is 
>>>> working great on my systems. I can post the patch if you or anyone 
>>>> else is interested.
>>>> John
>>>>
>>>>
>>>> Felix Zielcke wrote:
>>>>> Am Montag, den 13.04.2009, 21:03 -0400 schrieb John Stanley:
>>>>>  
>>>>>> Hi all,
>>>>>> I was wondering what the current status of a grub2 port of the 
>>>>>> grub-0.97 "map" and "rootnoverify" commands is?  I have found 
>>>>>> some work done to this end in the "drivemap.patch" work, but I 
>>>>>> find nothing more recent than drivemap.patch.8 dated around Aug 
>>>>>> 2008.
>>>>>>     
>>>>>
>>>>> The current status of it are exactly what you found out.
>>>>> I don't know if that'll ever change.
>>>>>
>>>>>
>>>>>  
>>>>>> Could anyone give me any pointers/direction on what might be 
>>>>>> happening here? Could it be that the "norootverify"-functionality 
>>>>>> of grub-legasy is lacking here? Or, perhaps, that the "--force" 
>>>>>> option is not being honored ?
>>>>>>     
>>>>>
>>>>> rootnoverify isn't needed anymore, because root is now just a 
>>>>> variable
>>>>> and not anymore a command which tried to verify it. So basically
>>>>> rootnoverify is default now.
>>>>> chainloader --force just skips the check for 0xaa55, normally it
>>>>> shouldn't be needed with a valid windows bootsector.
>>>>>   
>>>>
>>>>
>>>> _______________________________________________
>>>> Grub-devel mailing list
>>>> Grub-devel@gnu.org
>>>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>>
>>>
>>
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>

Re: status grub2 port of grub-legasy map command

[phcoder]

Yes it is. Also it's better to use grub_mmap_iterate instead of basing 
the location on 0x413 value. How to do it look at mmap/i386/pc/mmap.c
John Stanley wrote:
> I'd be happy to sign a copywrite statement, no problem -- how do I go 
> about it ?
> 
> Is this what you're referring to:
> 
>        /* BDA offset 0x13 contains the top of conventional memory, in 
> kiB.  */
>      grub_uint16_t *bpa_freekb = (grub_uint16_t*)0x00000413;
>      .
>      .
>      *bpa_freekb -= payload_sizekb;
> 
> where payload_sizekb is the size of the drivemap int13 handler + its 
> mapped drive table ?
> 
> 
> phcoder wrote:
>> If you want your code to be incorporated you need to sign the 
>> copyright assignment.
>> John Stanley wrote:
>>> I have also incorporated  your mmap services patch as well (again 
>>> with  minor mods to build in r2106). My question at this point, is 
>>> how best to incorporate mmap services into drivemap. I see that in 
>>> mmap/i386/pc/mmap.c there is some sort of support for int12 and int15 
>>> services. Should I incorporate the drivemap int13 handler here ? 
>>> Looks relatively straightforward -- just insert the asm handler into 
>>> mmap/i386/pc/mmap_helper.S and update mmap/i386/pc/mmap.c -- (except 
>>> for how I place the mapped drives table), or, should I use the mmap.c 
>>> code as a template for the drivemap int13 handler plus mapped drives 
>>> table ?
>> Don't do it that way. It merges 2 unrelated modules. At some point 
>> drivermap does following
>> *bpaMemInKb -= ...;
>> But this isn't correct because mmap interrupts still list the memory 
>> used by drivemap as available. Use grub_mmap_register instead of it.
>>>
>>> thanks for any help/suggestions,
>>> John
>>>
>>> phcoder wrote:
>>>> I haven't yet looked in depth in drivemap patch but it has some 
>>>> problems. It uses preboot hook interface for which I proposed an 
>>>> update in my recent patch "preboot hooks". Also it doesn't update 
>>>> memorymap correctly. For this it should use my "mmap services" 
>>>> interface
>>>> John Stanley wrote:
>>>>> Thanks Felix,
>>>>>
>>>>> Hurm.. Well, if anyone is interested, I have just made a couple of 
>>>>> additional updates to the drivemap.path.8 code,
>>>>> and now with r2104 the "unaligned pointer" issue is gone, and it is 
>>>>> working great on my systems. I can post the patch if you or anyone 
>>>>> else is interested.
>>>>> John
>>>>>
>>>>>
>>>>> Felix Zielcke wrote:
>>>>>> Am Montag, den 13.04.2009, 21:03 -0400 schrieb John Stanley:
>>>>>>  
>>>>>>> Hi all,
>>>>>>> I was wondering what the current status of a grub2 port of the 
>>>>>>> grub-0.97 "map" and "rootnoverify" commands is?  I have found 
>>>>>>> some work done to this end in the "drivemap.patch" work, but I 
>>>>>>> find nothing more recent than drivemap.patch.8 dated around Aug 
>>>>>>> 2008.
>>>>>>>     
>>>>>>
>>>>>> The current status of it are exactly what you found out.
>>>>>> I don't know if that'll ever change.
>>>>>>
>>>>>>
>>>>>>  
>>>>>>> Could anyone give me any pointers/direction on what might be 
>>>>>>> happening here? Could it be that the "norootverify"-functionality 
>>>>>>> of grub-legasy is lacking here? Or, perhaps, that the "--force" 
>>>>>>> option is not being honored ?
>>>>>>>     
>>>>>>
>>>>>> rootnoverify isn't needed anymore, because root is now just a 
>>>>>> variable
>>>>>> and not anymore a command which tried to verify it. So basically
>>>>>> rootnoverify is default now.
>>>>>> chainloader --force just skips the check for 0xaa55, normally it
>>>>>> shouldn't be needed with a valid windows bootsector.
>>>>>>   
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> Grub-devel mailing list
>>>>> Grub-devel@gnu.org
>>>>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>>>
>>>>
>>>
>>>
>>> _______________________________________________
>>> Grub-devel mailing list
>>> Grub-devel@gnu.org
>>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>
>>
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel


-- 

Regards
Vladimir 'phcoder' Serbinenko

Re: status grub2 port of grub-legasy map command

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 1151 bytes --]

Hello there. I am the original author of the drivemap command. I was
told by a friend that it was being discussed again on the GRUB list, so
I came back and examined the two new enabling functionalities, preboot
hooks and mmap. I've adapted drivemap to use them both, and I'm
polishing some rough edges. I might send a new version of the patch this
weekend. Should people, however, prefer John's version, I'd be glad to
assist.

El mié, 15-04-2009 a las 11:34 +0200, phcoder escribió:
> Yes it is. Also it's better to use grub_mmap_iterate instead of basing 
> the location on 0x413 value. How to do it look at mmap/i386/pc/mmap.c
The preboot hooks patch works beautifully. However, mmap causes some
sort of glitch that makes FreeDOS (one of my three drivemap tests,
together with ReactOS and XP) triple-fault. I've been trying to debug it
with GDB-over-QEMU, but it is a real PITA. I haven't found any sort of
bug in the mmap code, so I'm quite dumbfounded right now. Nevertheless,
I'm proceeding to the XP test, which would likely be the most common use
case for drivemap.

-- 
-- Lazy, Oblivious, Rational Disaster -- Habbit

[-- Attachment #2: Esto es una parte de mensaje firmado digitalmente --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: status grub2 port of grub-legasy map command

[Vladimir Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1147 bytes --]

Thank you for the feedback on my patch.  Also I'll add a function to
allocation and register map for hook. Also

> The preboot hooks patch works beautifully.

Don't forget to make cleanup function in case loader fails

> However, mmap causes some
> sort of glitch that makes FreeDOS (one of my three drivemap tests,
> together with ReactOS and XP) triple-fault. I've been trying to debug it
> with GDB-over-QEMU, but it is a real PITA. I haven't found any sort of
> bug in the mmap code, so I'm quite dumbfounded right now.

I'll look again into it and try to find the bug.
Also for convenience I'll add a function
grub_mmap_malloc_and_register (size, align, type, flags)
One of the flags will instruct it to allocate memory on lowmem. This way
this chunk of code can be shared for everything that needs to register a
hook

> Nevertheless,
> I'm proceeding to the XP test, which would likely be the most common use
> case for drivemap.
>
> --
> -- Lazy, Oblivious, Rational Disaster -- Habbit
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>

[-- Attachment #2: Type: text/html, Size: 1877 bytes --]

Re: status grub2 port of grub-legasy map command

[John Stanley]

Hi Javier and all,
I could probably have a version of the drivemap completed this weekend 
as well, but since I've basically simply adapted your drivempa.patch.8 
to r2106, with little change, I'd be more comfortable going with your 
new patch since you know the code much better.

In addition, in the last several days,  have started leaning away from 
it altogether -- because of the use  of an int13 intercept TSR. When I 
started looking at the drivemap patch about a week ago, I assumed it 
simply did some sort of bios (internal) table swapping only. I'm not 
really comfortable with the int13 handler redirect. I worry about 
(possibly rare) problems downstream while running Windows. On the other 
hand, I know it "does work," as I and many others have been using map 
with grub-legasy for several years now. Nevertheless, I'm now a bit 
apprehensive.

For me, for the time-being, as my laptops are all Thinkpads, which have 
bios supplied "boot device table"  functionality (via f12 on poweron), I 
can live without drivemap for those occasions when I need to come down 
from the hilltop to work on Windows.

What I'd like to do with drivemap, is to do boot device swapping by 
changing in the bios which device is to be treated as the "first" boot 
device, which is the second, etc. I know this is doable in principle, 
but it may not be practical, or wise-- I'm not sure.  It would also have 
to be done in a way which does not "pull the rug out on grub" insofar as 
boot device enumeration is concerned.
 

Javier Martín wrote:
> Hello there. I am the original author of the drivemap command. I was
> told by a friend that it was being discussed again on the GRUB list, so
> I came back and examined the two new enabling functionalities, preboot
> hooks and mmap. I've adapted drivemap to use them both, and I'm
> polishing some rough edges. I might send a new version of the patch this
> weekend. Should people, however, prefer John's version, I'd be glad to
> assist.
>
> El mié, 15-04-2009 a las 11:34 +0200, phcoder escribió:
>   
>> Yes it is. Also it's better to use grub_mmap_iterate instead of basing 
>> the location on 0x413 value. How to do it look at mmap/i386/pc/mmap.c
>>     
> The preboot hooks patch works beautifully. However, mmap causes some
> sort of glitch that makes FreeDOS (one of my three drivemap tests,
> together with ReactOS and XP) triple-fault. I've been trying to debug it
> with GDB-over-QEMU, but it is a real PITA. I haven't found any sort of
> bug in the mmap code, so I'm quite dumbfounded right now. Nevertheless,
> I'm proceeding to the XP test, which would likely be the most common use
> case for drivemap.
>
>   
> ------------------------------------------------------------------------
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>   

Re: status grub2 port of grub-legasy map command

[Vladimir Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 3746 bytes --]

On Sat, Apr 18, 2009 at 1:12 AM, John Stanley <jpsinthemix@verizon.net>wrote:

> Hi Javier and all,
> I could probably have a version of the drivemap completed this weekend as
> well, but since I've basically simply adapted your drivempa.patch.8 to
> r2106, with little change, I'd be more comfortable going with your new patch
> since you know the code much better.
>
> In addition, in the last several days,  have started leaning away from it
> altogether -- because of the use  of an int13 intercept TSR. When I started
> looking at the drivemap patch about a week ago, I assumed it simply did some
> sort of bios (internal) table swapping only. I'm not really comfort able
> with the int13 handler redirect. I worry about (possibly rare) problems
> downstream while running Windows. On the other hand, I know it "does work,"
> as I and many others have been using map with grub-legasy for several years
> now. Nevertheless, I'm now a bit apprehensive.
>
Modifying internal bios tables isn't practicable because they may be
anywhere and in any format. Windows follows the memory map supplied by
int12/int15/bda which says that the ranges used by hook are unusable. Also
even if windows overwrites int13 handle it doesn't matter at all because
windows switches to its own disk drivers after initial start

>
> For me, for the time-being, as my laptops are all Thinkpads, which have
> bios supplied "boot device table"  functionality (via f12 on poweron), I can
> live without drivemap for those occasions when I need to come down from the
> hilltop to work on Windows.
>
> What I'd like to do with drivemap, is to do boot device swapping by
> changing in the bios which device is to be treated as the "first" boot
> device, which is the second, etc. I know this is doable in principle, but it
> may not be practical, or wise-- I'm not sure.

There is no public API to do so. So you need to reverse engineer BIOS which
is impractical to do for every possible mobo.

>  It would also have to be done in a way which does not "pull the rug out on
> grub" insofar as boot device enumeration is concerned.
>
>
> Javier Martín wrote:
>
>> Hello there. I am the original author of the drivemap command. I was
>> told by a friend that it was being discussed again on the GRUB list, so
>> I came back and examined the two new enabling functionalities, preboot
>> hooks and mmap. I've adapted drivemap to use them both, and I'm
>> polishing some rough edges. I might send a new version of the patch this
>> weekend. Should people, however, prefer John's version, I'd be glad to
>> assist.
>>
>> El mié, 15-04-2009 a las 11:34 +0200, phcoder escribió:
>>
>>
>>> Yes it is. Also it's better to use grub_mmap_iterate instead of basing
>>> the location on 0x413 value. How to do it look at mmap/i386/pc/mmap.c
>>>
>>>
>> The preboot hooks patch works beautifully. However, mmap causes some
>> sort of glitch that makes FreeDOS (one of my three drivemap tests,
>> together with ReactOS and XP) triple-fault. I've been trying to debug it
>> with GDB-over-QEMU, but it is a real PITA. I haven't found any sort of
>> bug in the mmap code, so I'm quite dumbfounded right now. Nevertheless,
>> I'm proceeding to the XP test, which would likely be the most common use
>> case for drivemap.
>>
>>  ------------------------------------------------------------------------
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>
>>
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 5102 bytes --]

Re: status grub2 port of grub-legasy map command

[John Stanley]

Ok, that sounds good... I feel better about it then. I didn't mean 
directly modifying bios data, but through doc'd bios ints of course. It 
been a long, long time since I've looked at (or dealt with) bios code, 
so I was was simply wondering if boot device naming  could be modified  
in  a  documented way nowadays-- after all, the functionality has been 
available in the bios setup in pcs for at least 10 years or so.

Vladimir Serbinenko wrote:
>
>
> On Sat, Apr 18, 2009 at 1:12 AM, John Stanley <jpsinthemix@verizon.net 
> <mailto:jpsinthemix@verizon.net>> wrote:
>
>     Hi Javier and all,
>     I could probably have a version of the drivemap completed this
>     weekend as well, but since I've basically simply adapted your
>     drivempa.patch.8 to r2106, with little change, I'd be more
>     comfortable going with your new patch since you know the code much
>     better.
>
>     In addition, in the last several days,  have started leaning away
>     from it altogether -- because of the use  of an int13 intercept
>     TSR. When I started looking at the drivemap patch about a week
>     ago, I assumed it simply did some sort of bios (internal) table
>     swapping only. I'm not really comfort able with the int13 handler
>     redirect. I worry about (possibly rare) problems downstream while
>     running Windows. On the other hand, I know it "does work," as I
>     and many others have been using map with grub-legasy for several
>     years now. Nevertheless, I'm now a bit apprehensive.
>
> Modifying internal bios tables isn't practicable because they may be 
> anywhere and in any format. Windows follows the memory map supplied by 
> int12/int15/bda which says that the ranges used by hook are unusable. 
> Also even if windows overwrites int13 handle it doesn't matter at all 
> because windows switches to its own disk drivers after initial start
>
>
>     For me, for the time-being, as my laptops are all Thinkpads, which
>     have bios supplied "boot device table"  functionality (via f12 on
>     poweron), I can live without drivemap for those occasions when I
>     need to come down from the hilltop to work on Windows.
>
>     What I'd like to do with drivemap, is to do boot device swapping
>     by changing in the bios which device is to be treated as the
>     "first" boot device, which is the second, etc. I know this is
>     doable in principle, but it may not be practical, or wise-- I'm
>     not sure. 
>
> There is no public API to do so. So you need to reverse engineer BIOS 
> which is impractical to do for every possible mobo.
>
>      It would also have to be done in a way which does not "pull the
>     rug out on grub" insofar as boot device enumeration is concerned.
>
>
>     Javier Martín wrote:
>
>         Hello there. I am the original author of the drivemap command.
>         I was
>         told by a friend that it was being discussed again on the GRUB
>         list, so
>         I came back and examined the two new enabling functionalities,
>         preboot
>         hooks and mmap. I've adapted drivemap to use them both, and I'm
>         polishing some rough edges. I might send a new version of the
>         patch this
>         weekend. Should people, however, prefer John's version, I'd be
>         glad to
>         assist.
>
>         El mié, 15-04-2009 a las 11:34 +0200, phcoder escribió:
>          
>
>             Yes it is. Also it's better to use grub_mmap_iterate
>             instead of basing the location on 0x413 value. How to do
>             it look at mmap/i386/pc/mmap.c
>                
>
>         The preboot hooks patch works beautifully. However, mmap
>         causes some
>         sort of glitch that makes FreeDOS (one of my three drivemap tests,
>         together with ReactOS and XP) triple-fault. I've been trying
>         to debug it
>         with GDB-over-QEMU, but it is a real PITA. I haven't found any
>         sort of
>         bug in the mmap code, so I'm quite dumbfounded right now.
>         Nevertheless,
>         I'm proceeding to the XP test, which would likely be the most
>         common use
>         case for drivemap.
>
>          ------------------------------------------------------------------------
>
>
>
>         _______________________________________________
>         Grub-devel mailing list
>         Grub-devel@gnu.org <mailto:Grub-devel@gnu.org>
>         http://lists.gnu.org/mailman/listinfo/grub-devel
>          
>
>
>
>     _______________________________________________
>     Grub-devel mailing list
>     Grub-devel@gnu.org <mailto:Grub-devel@gnu.org>
>     http://lists.gnu.org/mailman/listinfo/grub-devel
>
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>   

Re: status grub2 port of grub-legasy map command

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 3743 bytes --]

El vie, 17-04-2009 a las 20:01 -0400, John Stanley escribió:
> Ok, that sounds good... I feel better about it then. I didn't mean 
> directly modifying bios data, but through doc'd bios ints of course. It 
> been a long, long time since I've looked at (or dealt with) bios code, 
> so I was was simply wondering if boot device naming  could be modified  
> in  a  documented way nowadays-- after all, the functionality has been 
> available in the bios setup in pcs for at least 10 years or so.

Indeed. However, this is not necessary as most modern OSes only use the
BIOS routines for the initial startup, switching sooner than later to
their own device drivers. The problem with some of them, among them MS
Windows, is that the loader stage fails to either notice the "boot drive
number" info passed in %dl by the BIOS (or, in this case, by GRUB)
and/or propagate it to latter stages of their multi-stage pre-boot
process. As an example, FreeDOS ignores the parameter and fails to
locate KERNEL.SYS, while ReactOS's freeldr uses it and is able to load
its config file, but then fails to boot the (drive-#-hardcoded) paths in
that same file.

This sorry state of things motivates most BIOSes to actually "swap" your
"designated boot device" with the actual first HD, so that systems that
assume they were installed on hd0 will work. Drivemap basically
replicates this functionality using the "canonical" real-mode interrupt
handler chaining pattern: it intercepts calls to the BIOS drive access
interrupt and calls the previous handler after performing its role. This
is not a "dirty hack" (well, it _is_ by current standards), and in fact
it was the way TSRs worked in DOS. Here's what happens when the Windows
loader (or FreeDOS, etc.) tries to access the drives:

ntldr --(int13h: r,0x80)--> drivemap_int13h --(lcall: f,r,0x81)-->BIOS

Drivemap (and other TSRs) actually _simulate_ an interrupt call by
pushing the flags on the stack, then doing a far call to the address of
the handler they replaced (the address that occupied the related slot in
the IVT when they were installed). This was, for example, how resident
anti-virus programs were implemented in DOS, and I suppose that other
programs like DoubleSpace and SmartDrive used it too.

For modern OSes, once the initial stage has loaded the kernel and the
relevant modules, it usually fires up its own disk drivers that reflect
the "true" (as defined by the disk controller) order of the HDs, so
neither the BIOS nor drivemap affect that. Moreover, many OSes currently
select their root device based on its UUID, label, etc, so there is no
problem switching the BIOS routines because they won't be used again
after the initial loader is finished. The exception is, of course,
FreeDOS, where the drives stay swapped for the whole session because it
has no disk drivers other than those provided by BIOS.

I hope this has reassured you that neither mmap nor drivemap are doing
any "dark things" nor "1980s DOS/BIOS black magic". The only BIOS data
table modified (by mmap) should be the "available free memory" at the
BDA, which is a completely documented procedure and quite about the only
way of telling a DOS "do not use this memory".

Uhh... btw, I think I found the problem with mmap and FreeDOS: both the
mmap int handler and the drivemap int handler reserve their memory as
high as possible under 1M. Is FreeDOS aware of this? I mean, we can tell
it not to touch memory under 640K through the BDA and INT12, but how do
we tell it not to touch the 640K-1M area? I will check if this is the
cause of the disaster... tomorrow - it's 4AM and I'm sleepy. Goodbye!

-- 
-- Lazy, Oblivious, Rational Disaster -- Habbit

[-- Attachment #2: Esto es una parte de mensaje firmado digitalmente --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: status grub2 port of grub-legasy map command

[Vladimir Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 348 bytes --]

>
> we tell it not to touch the 640K-1M area?
>
On most machines this range is in ROM and VGA buffer so FreeDOS doesn't
touch it anyway

>
> --
> -- Lazy, Oblivious, Rational Disaster -- Habbit
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>

[-- Attachment #2: Type: text/html, Size: 884 bytes --]

Re: status grub2 port of grub-legasy map command

[Javier Martín]

[-- Attachment #1.1: Type: text/plain, Size: 831 bytes --]

I am glad to inform that, with the new version of the mmap patch,
drivemap now boots all my hd1 installs of:
- Windows XP (Pro x64)
- ReactOS
- FreeDOS

I would suggest, however, that the return type of
grub_mmap_malign_and_register be changed to void* from char*, just like
the return type from malloc, because it's the meaningful data type to
indicate a pointer to generic memory _and_ it automatically casts to any
pointer type the caller uses (which is the reason it's used in malloc).

I've also added an "undo" function for install_int13_handler, as
partially required by the preboot hook interface, but I really have no
idea how to test it, so... is this implementation sensible? It just
restores the old int13 handler and frees the allocated memory.

-- 
-- Lazy, Oblivious, Recurrent Disaster -- Habbit

[-- Attachment #1.2: drivemap.9.patch --]
[-- Type: text/x-patch, Size: 23643 bytes --]

Index: commands/i386/pc/drivemap.c
===================================================================
--- commands/i386/pc/drivemap.c	(revisión: 0)
+++ commands/i386/pc/drivemap.c	(revisión: 0)
@@ -0,0 +1,454 @@
+/* drivemap.c - command to manage the BIOS drive mappings.  */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2008, 2009  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/machine/drivemap.h>
+#include <grub/extcmd.h>
+#include <grub/dl.h>
+#include <grub/mm.h>
+#include <grub/misc.h>
+#include <grub/disk.h>
+#include <grub/machine/biosdisk.h>
+#include <grub/loader.h>
+#include <grub/machine/memory.h>
+
+
+#define MODNAME "drivemap"
+
+static grub_extcmd_t cmd_reghandle;
+
+/* Remember to update enum opt_idxs accordingly.  */
+static const struct grub_arg_option options[] = {
+  {"list", 'l', 0, "show the current mappings", 0, 0},
+  {"reset", 'r', 0, "reset all mappings to the default values", 0, 0},
+  {"swap", 's', 0, "perform both direct and reverse mappings", 0, 0},
+  {0, 0, 0, 0, 0, 0}
+};
+
+/* Remember to update options[] accordingly.  */
+enum opt_idxs {
+  OPTIDX_LIST = 0,
+  OPTIDX_RESET,
+  OPTIDX_SWAP,
+};
+
+typedef struct drivemap_node
+{
+  grub_uint8_t newdrive;
+  grub_uint8_t redirto;
+  struct drivemap_node *next;
+} drivemap_node_t;
+
+static drivemap_node_t *map_head;
+static void* insthandler_hook;
+static int handlermem_hnd;
+static grub_err_t install_int13_handler (int noret __attribute__((unused)));
+static grub_err_t uninstall_int13_handler (void);
+
+/* Puts the specified mapping into the table, replacing an existing mapping
+   for newdrive or adding a new one if required.  */
+static grub_err_t
+drivemap_set (grub_uint8_t newdrive, grub_uint8_t redirto)
+{
+  drivemap_node_t *mapping = 0;
+  drivemap_node_t *search = map_head;
+  while (search)
+    {
+      if (search->newdrive == newdrive)
+        {
+          mapping = search;
+          break;
+        }
+      search = search->next;
+    }
+
+  
+  /* Check for pre-existing mappings to modify before creating a new one.  */
+  if (mapping)
+    mapping->redirto = redirto;
+  else 
+    {
+      mapping = grub_malloc (sizeof (drivemap_node_t));
+      if (!mapping)
+        return grub_error (GRUB_ERR_OUT_OF_MEMORY,
+                           "cannot allocate map entry, not enough memory");
+      mapping->newdrive = newdrive;
+      mapping->redirto = redirto;
+      mapping->next = map_head;
+      map_head = mapping;
+    }
+  return GRUB_ERR_NONE;
+}
+
+/* Removes the mapping for newdrive from the table.  If there is no mapping,
+   then this function behaves like a no-op on the map.  */
+static void
+drivemap_remove (grub_uint8_t newdrive)
+{
+  drivemap_node_t *mapping = 0;
+  drivemap_node_t *search = map_head;
+  drivemap_node_t *previous = 0;
+
+  while (search)
+    {
+      if (search->newdrive == newdrive)
+        {
+          mapping = search;
+          break;
+        }
+      previous = search;
+      search = search->next;
+    }
+
+  if (mapping)
+    {
+      if (previous)
+        previous->next = mapping->next;
+      else /* Entry was head of list.  */
+        map_head = mapping->next;
+      grub_free (mapping);
+    }
+}
+
+/* Given a device name, resolves its BIOS disk number and stores it in the
+   passed location, which should only be trusted if ERR_NONE is returned.  */
+static grub_err_t
+parse_biosdisk (const char *name, grub_uint8_t *disknum)
+{
+  grub_disk_t disk;
+  if (!name || *name == 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name empty");
+  /* Skip the first ( in (hd0) - disk_open wants just the name.  */
+  if (*name == '(')
+    name++;
+  
+  disk = grub_disk_open (name);
+  if (!disk)
+    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "unknown device \"%s\"", name);
+  else
+    {
+      const enum grub_disk_dev_id id = disk->dev->id;
+      /* The following assignment is only sound if the device is indeed a
+         biosdisk.  The caller must check the return value.  */
+      if (disknum)
+        *disknum = disk->id;
+      grub_disk_close (disk);
+      if (id == GRUB_DISK_DEVICE_BIOSDISK_ID)
+        return GRUB_ERR_NONE;
+      else
+        return grub_error (GRUB_ERR_BAD_DEVICE, "%s is not a BIOS disk", name);
+    }
+}
+
+/* Given a BIOS disk number, returns its GRUB device name if it exists.  
+   If the call succeeds, the resulting device string must be freed.  
+   For nonexisting BIOS disk numbers, this function returns
+   GRUB_ERR_UNKNOWN_DEVICE.  */
+static grub_err_t
+revparse_biosdisk(const grub_uint8_t dnum, const char **output)
+{
+  int found = 0;
+  auto int find (const char *name);
+  int find (const char *name)
+  {
+    const grub_disk_t disk = grub_disk_open (name);
+    if (!disk)
+      return 0;
+    else
+      {
+          
+        if (disk->id == dnum && disk->dev->id == GRUB_DISK_DEVICE_BIOSDISK_ID)
+          {
+            found = 1;
+            if (output)
+              *output = grub_strdup (name);
+          }
+        grub_disk_close (disk);
+        return found;
+      }
+  }
+
+  grub_disk_dev_iterate (find);
+  if (found)
+    return GRUB_ERR_NONE;
+  else
+    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "BIOS disk %02x not found", dnum);
+}
+
+/* Given a GRUB-like device name and a convenient location, stores the
+   related BIOS disk number.  Accepts devices like \((f|h)dN\), with
+   0 <= N < 128.  */
+static grub_err_t
+tryparse_diskstring (const char *str, grub_uint8_t *output)
+{
+  if (!str || *str == 0)
+    goto fail;
+  /* Skip opening paren in order to allow both (hd0) and hd0.  */
+  if (*str == '(')
+    str++;
+  if ((str[0] == 'f' || str[0] == 'h') && str[1] == 'd')
+    {
+      grub_uint8_t bios_num = (str[0] == 'h')? 0x80 : 0x00;
+      grub_errno = GRUB_ERR_NONE;
+      unsigned long drivenum = grub_strtoul (str + 2, 0, 0);
+      if (grub_errno != GRUB_ERR_NONE || drivenum > 127)
+        {
+          /* N not a number or out of range.  */
+          goto fail;
+        }
+      else
+        {
+          bios_num |= drivenum;
+          if (output)
+            *output = bios_num;
+          return GRUB_ERR_NONE;
+        }
+    }
+  else
+    goto fail;
+
+fail:
+  return grub_error (GRUB_ERR_BAD_ARGUMENT, "device format \"%s\" "
+                     "invalid: must be (f|h)dN, with 0 <= N < 128", str);
+}
+
+static grub_err_t
+grub_cmd_drivemap (struct grub_extcmd *cmd, int argc, char **args)
+{
+  if (cmd->state[OPTIDX_LIST].set)
+    {
+      /* Show: list mappings.  */
+      if (!map_head)
+        grub_printf ("No drives have been remapped");
+      else
+        {
+          grub_printf ("Showing only remapped drives.\n");
+          grub_printf ("BIOS disk #num ----> GRUB device\n");
+          drivemap_node_t *curnode = map_head;
+          while (curnode)
+            {
+              const char *dname = 0;
+              grub_err_t err = revparse_biosdisk (curnode->redirto, &dname);
+              if (err != GRUB_ERR_NONE)
+                return grub_error (err, "invalid mapping: non-existent disk"
+                                        "or not managed by the BIOS");
+              grub_printf ("%cD #%-3u (0x%02x)       %s\n",
+                          (curnode->newdrive & 0x80) ? 'H' : 'F',
+                          curnode->newdrive & 0x7F, curnode->newdrive,
+                          dname);
+              curnode = curnode->next;
+              grub_free ((char*)dname);
+            }
+        }
+    }
+  else if (cmd->state[OPTIDX_RESET].set)
+    {
+      /* Reset: just delete all mappings, freeing their memory.  */
+      drivemap_node_t *curnode = map_head;
+      drivemap_node_t *prevnode = 0;
+      while (curnode)
+        {
+          prevnode = curnode;
+          curnode = curnode->next;
+          grub_free (prevnode);
+        }
+      map_head = 0;
+    }
+  else
+    {
+      /* Neither flag: put mapping.  */
+      grub_uint8_t mapfrom = 0;
+      grub_uint8_t mapto = 0xFF;
+      grub_err_t err;
+      
+      if (argc != 2)
+        return grub_error (GRUB_ERR_BAD_ARGUMENT, "two arguments required");
+
+      err = parse_biosdisk (args[0], &mapfrom);
+      if (err != GRUB_ERR_NONE)
+        return err;
+      
+      /* When swapping we require both devices to be BIOS disks, but when
+         performing direct mappings we only require the 2nd argument to look
+         like a BIOS disk in order to resolve it into a BIOS disk number.  */
+      if (cmd->state[OPTIDX_SWAP].set)
+        err = parse_biosdisk (args[1], &mapto);
+      else
+        err = tryparse_diskstring (args[1], &mapto);
+      if (err != GRUB_ERR_NONE)
+        return err;
+      
+      if (mapto == mapfrom)
+        {
+          /* Reset to default.  */
+          grub_dprintf (MODNAME, "Removing the mapping for %s (%02x)", args[0], mapfrom);
+          drivemap_remove (mapfrom);
+        }
+      else
+        {
+          /* Set the mapping for the disk (overwrites any existing mapping).  */
+          grub_dprintf (MODNAME, "%s %s (%02x) = %s (%02x)\n",
+                        cmd->state[OPTIDX_SWAP].set ? "Swapping" : "Mapping",
+                        args[1], mapto, args[0], mapfrom);
+          err = drivemap_set (mapto, mapfrom);
+          /* If -s, perform the reverse mapping too (only if the first was OK).  */
+          if (cmd->state[OPTIDX_SWAP].set && err == GRUB_ERR_NONE)
+            err = drivemap_set (mapfrom, mapto);
+          return err;
+        }
+    }
+
+  return GRUB_ERR_NONE;
+}
+
+typedef struct __attribute__ ((packed)) int13map_node
+{
+  grub_uint8_t disknum;
+  grub_uint8_t mapto;
+} int13map_node_t;
+
+#define INT13H_OFFSET(x) ( ((grub_uint8_t*)(x)) - ((grub_uint8_t*)&grub_drivemap_int13_handler_base) )
+#define INT13H_REBASE(x) ( (void*) (handler_base + (x)) )
+#define INT13H_TONEWADDR(x) INT13H_REBASE( INT13H_OFFSET( x ) )
+
+/* Int13h handler installer - reserves conventional memory for the handler,
+   copies it over and sets the IVT entry for int13h.  
+   This code rests on the assumption that GRUB does not activate any kind
+   of memory mapping apart from identity paging, since it accesses
+   realmode structures by their absolute addresses, like the IVT at 0;
+   and transforms a pmode pointer into a rmode seg:off far ptr.  */
+static grub_err_t
+install_int13_handler (int noret __attribute__((unused)))
+{
+  grub_size_t entries = 0;
+  drivemap_node_t *curentry = map_head;
+  
+  /* Count entries to prepare a contiguous map block.  */
+  while (curentry)
+    {
+      entries++;
+      curentry = curentry->next;
+    }
+  if (entries == 0)
+    {
+      /* No need to install the int13h handler.  */
+      grub_dprintf (MODNAME, "No drives marked as remapped, installation "
+                    "of an int13h handler is not required.");
+      return GRUB_ERR_NONE;
+    }
+  else
+    {
+      /* Real mode IVT slot (seg:off far pointer) for interrupt 0x13.  */
+      grub_uint32_t *ivtslot = UINT_TO_PTR(0x0000004c);
+      /* Size of the full int13 handler "bundle", including code and map.  */
+      grub_uint64_t total_size;
+      /* Base address of the space reserved for the handler bundle.  */
+      grub_uint8_t *handler_base = 0;
+      /* Address of the map within the deployed bundle.  */
+      int13map_node_t *handler_map;
+      /* Real mode IVT entry (seg:off far pointer) for the new handler.  */
+      grub_uint32_t ivtentry;
+
+      grub_dprintf (MODNAME, "Installing int13h handler...\n");
+      
+      /* Save the pointer to the old handler.  */
+      grub_drivemap_int13_oldhandler = *ivtslot;
+      grub_dprintf (MODNAME, "Old int13 handler at %04x:%04x\n",
+                    (grub_drivemap_int13_oldhandler >> 16) & 0x0ffff,
+                    grub_drivemap_int13_oldhandler & 0x0ffff);
+
+      /* Find a rmode-segment-aligned zone in conventional memory big
+         enough to hold the handler and its data.  */
+      total_size = grub_drivemap_int13_size
+                   + (entries + 1) * sizeof(int13map_node_t);
+      grub_dprintf (MODNAME, "Payload is %llu bytes long\n", total_size);
+      handler_base = (grub_uint8_t*) grub_mmap_malign_and_register (16, total_size,
+                              &handlermem_hnd, GRUB_MACHINE_MEMORY_RESERVED,
+                              GRUB_MMAP_MALLOC_LOW);
+      if (!handler_base)
+        return grub_error (GRUB_ERR_OUT_OF_MEMORY, "Could not reserve "
+                                        "memory for the int13h handler");
+
+      /* Copy int13h handler bundle to reserved area.  */
+      grub_dprintf (MODNAME, "Reserved memory at %p, copying handler...\n", handler_base);
+      grub_memcpy (handler_base, &grub_drivemap_int13_handler_base,
+                   grub_drivemap_int13_size);
+
+      /* Copy the mappings to the reserved area.  */
+      curentry = map_head;
+      grub_size_t i;
+      handler_map = INT13H_TONEWADDR (&grub_drivemap_int13_mapstart);
+      grub_dprintf (MODNAME, "Target map at %p, copying mappings...\n", handler_map);
+      for (i = 0; i < entries && curentry; ++i, curentry = curentry->next)
+        {
+          handler_map[i].disknum = curentry->newdrive;
+          handler_map[i].mapto = curentry->redirto;
+          grub_dprintf (MODNAME, "\t#%d: 0x%02x <- 0x%02x\n", i,
+                        handler_map[i].disknum, handler_map[i].mapto);
+        }
+      /* Signal end-of-map.  */
+      handler_map[i].disknum = 0;
+      handler_map[i].mapto = 0;
+      grub_dprintf (MODNAME, "\t#%d: 0x00 <- 0x00 (end)\n", i);
+
+      /* Install our function as the int13h handler in the IVT.  */
+      ivtentry = ((grub_uint32_t)handler_base) << 12; /* Segment address.  */
+      ivtentry |= (grub_uint16_t) INT13H_OFFSET(&grub_drivemap_int13_handler);
+      grub_dprintf (MODNAME, "New int13 handler IVT pointer: %04x:%04x\n",
+                    (ivtentry >> 16) & 0x0ffff, ivtentry & 0x0ffff);
+      *ivtslot = ivtentry;
+      
+      return GRUB_ERR_NONE;
+    }
+}
+
+static grub_err_t
+uninstall_int13_handler()
+{
+  grub_uint32_t *ivtslot = UINT_TO_PTR(0x0000004c);
+  
+  if (0 != grub_drivemap_int13_oldhandler)
+    return GRUB_ERR_NONE;
+  
+  *ivtslot = grub_drivemap_int13_oldhandler;
+  grub_mmap_free_and_unregister (handlermem_hnd);
+  grub_drivemap_int13_oldhandler = 0;
+  
+  return GRUB_ERR_NONE;
+}
+
+GRUB_MOD_INIT (drivemap)
+{
+  (void) mod;			/* Stop warning.  */
+  cmd_reghandle = grub_register_extcmd (MODNAME, grub_cmd_drivemap,
+                      GRUB_COMMAND_FLAG_BOTH,
+                      MODNAME " -l | -r | [-s] grubdev biosdisk",
+                      "Manage the BIOS drive mappings", options);
+  insthandler_hook = grub_loader_register_preboot_hook (
+                         &install_int13_handler,
+                         &uninstall_int13_handler,
+                         GRUB_LOADER_PREBOOT_HOOK_PRIO_NORMAL);
+}
+
+GRUB_MOD_FINI (drivemap)
+{
+  grub_loader_unregister_preboot_hook (insthandler_hook);
+  insthandler_hook = 0;
+  grub_unregister_extcmd (cmd_reghandle);
+}
+
Index: commands/i386/pc/drivemap_int13h.S
===================================================================
--- commands/i386/pc/drivemap_int13h.S	(revisión: 0)
+++ commands/i386/pc/drivemap_int13h.S	(revisión: 0)
@@ -0,0 +1,109 @@
+/* drivemap_int13h.S - interrupt handler for the BIOS drive remapper */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2008, 2009  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+
+/*
+ * Note: These functions defined in this file may be called from C.
+ *       Be careful of that you must not modify some registers. Quote
+ *       from gcc-2.95.2/gcc/config/i386/i386.h:
+
+   1 for registers not available across function calls.
+   These must include the FIXED_REGISTERS and also any
+   registers that can be used without being saved.
+   The latter must include the registers where values are returned
+   and the register where structure-value addresses are passed.
+   Aside from that, you can include as many other registers as you like.
+
+  ax,dx,cx,bx,si,di,bp,sp,st,st1,st2,st3,st4,st5,st6,st7,arg
+{  1, 1, 1, 0, 0, 0, 0, 1, 1,  1,  1,  1,  1,  1,  1,  1,  1 }
+ */
+
+/*
+ * Note: GRUB is compiled with the options -mrtd and -mregparm=3.
+ *       So the first three arguments are passed in %eax, %edx, and %ecx,
+ *       respectively, and if a function has a fixed number of arguments
+ *       and the number if greater than three, the function must return
+ *       with "ret $N" where N is ((the number of arguments) - 3) * 4.
+ */
+
+#include <grub/symbol.h>
+
+#define GRUB_DRIVEMAP_INT13H_OFFSET(x) ((x) - grub_drivemap_int13_handler_base)
+
+/* Copy starts here.  When deployed, this label must be segment-aligned.  */
+VARIABLE(grub_drivemap_int13_handler_base)
+
+/* Far pointer to the old handler.  Stored as a CS:IP in the style of real-mode
+   IVT entries (thus PI:SC in mem).  */
+VARIABLE(grub_drivemap_int13_oldhandler)
+  .word 0x0, 0x0
+
+/* Drivemap module bundle - INT 13h handler - BIOS HD map.  */
+/* We need to use relative addressing, and with CS to top it all, since we
+   must make as few changes to the registers as possible.  IP-relative
+   addressing like on amd64 would make life way easier here.  */
+.code16
+FUNCTION(grub_drivemap_int13_handler)
+  push %bp
+  mov %sp, %bp
+
+  /* Map the drive number (always in DL?).  */
+  push %ax
+  push %bx
+  push %si
+  mov $GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_mapstart), %bx
+  xor %si, %si
+1:movw %cs:(%bx,%si), %ax
+  cmp %ah, %al
+  jz 3f /* DRV=DST => map end - drive not remapped, leave DL as-is.  */
+  cmp %dl, %al
+  jz 2f /* Found - drive remapped, modify DL.  */
+  add $2, %si
+  jmp 1b /* Not found, but more remaining, loop.  */
+2:mov %ah, %dl
+3:pop %si
+  pop %bx
+  pop %ax
+  
+  push %bp
+  /* Simulate interrupt call: push flags and do a far call in order to set
+     the stack the way the old handler expects it so that its iret works.  */
+  push 6(%bp)
+  movw (%bp), %bp  /* Restore the caller BP (is this needed and/or sensible?).  */
+  lcall *%cs:GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_oldhandler)
+  pop %bp /* The pushed flags were removed by iret.  */
+  /* Set the saved flags to what the int13h handler returned.  */
+  push %ax
+  pushf
+  pop %ax
+  movw %ax, 6(%bp)
+  pop %ax
+
+4:mov %bp, %sp
+  pop %bp
+  iret
+/* This label MUST be at the end of the copied block, since the installer code
+   reserves additional space for mappings at runtime and copies them over it.  */
+.align 2
+VARIABLE(grub_drivemap_int13_mapstart)
+/* Copy stops here.  */
+.code32
+VARIABLE(grub_drivemap_int13_size)
+  .word GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_size)
+
Index: include/grub/i386/pc/drivemap.h
===================================================================
--- include/grub/i386/pc/drivemap.h	(revisión: 0)
+++ include/grub/i386/pc/drivemap.h	(revisión: 0)
@@ -0,0 +1,42 @@
+/* drivemap.h - command to manage the BIOS drive mappings.  */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2008  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef GRUB_DRIVEMAP_MACHINE_HEADER
+#define GRUB_DRIVEMAP_MACHINE_HEADER	1
+
+#include <grub/types.h>
+
+/* Realmode far ptr (2 * 16b) to the previous INT13h handler.  */
+extern grub_uint32_t grub_drivemap_int13_oldhandler;
+/* Size of the INT13h handler bundle (code+data) to be deployed.  */
+extern grub_uint16_t grub_drivemap_int13_size;
+
+/* This type is used for imported assembly labels, takes no storage and is only
+   used to take the symbol address with &label.  Do NOT put void* here.  */
+typedef void grub_symbol_t;
+
+/* Start of the handler bundle.  */
+extern grub_symbol_t grub_drivemap_int13_handler_base;
+/* Start of the drive mappings area (space reserved at runtime).  */
+extern grub_symbol_t grub_drivemap_int13_mapstart;
+/* The assembly function to replace the old INT13h handler. It should not be
+   called because it does not follow any C callspecs and returns with IRET.  */
+extern grub_symbol_t grub_drivemap_int13_handler;
+
+#endif /* ! GRUB_DRIVEMAP_MACHINE_HEADER */
Index: conf/i386-pc.rmk
===================================================================
--- conf/i386-pc.rmk	(revisión: 2163)
+++ conf/i386-pc.rmk	(copia de trabajo)
@@ -185,8 +185,16 @@
 	aout.mod bsd.mod pxe.mod pxecmd.mod datetime.mod date.mod \
 	datehook.mod lsmmap.mod ata_pthru.mod hdparm.mod \
 	usb.mod uhci.mod ohci.mod usbtest.mod usbms.mod usb_keyboard.mod \
-	efiemu.mod mmap.mod acpi.mod
+	efiemu.mod mmap.mod acpi.mod drivemap.mod
 
+# For drivemap.mod.
+drivemap_mod_HEADERS = machine/drivemap.h
+drivemap_mod_SOURCES = commands/i386/pc/drivemap.c \
+                       commands/i386/pc/drivemap_int13h.S
+drivemap_mod_ASFLAGS = $(COMMON_ASFLAGS)
+drivemap_mod_CFLAGS = $(COMMON_CFLAGS)
+drivemap_mod_LDFLAGS = $(COMMON_LDFLAGS)
+
 # For efiemu.mod.
 efiemu_mod_SOURCES = efiemu/main.c efiemu/i386/loadcore32.c \
 		     efiemu/i386/loadcore64.c efiemu/i386/pc/cfgtables.c \

[-- Attachment #2: Esto es una parte de mensaje firmado digitalmente --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: status grub2 port of grub-legasy map command

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1424 bytes --]

Hello. Now I just quickly answer you questions. Will review the patch a bit
later (probably still today)

2009/5/3 Javier Martín <lordhabbit@gmail.com>

> I am glad to inform that, with the new version of the mmap patch,
> drivemap now boots all my hd1 installs of:
> - Windows XP (Pro x64)
> - ReactOS
> - FreeDOS
>
> I would suggest, however, that the return type of
> grub_mmap_malign_and_register be changed to void* from char*, just like
> the return type from malloc, because it's the meaningful data type to
> indicate a pointer to generic memory _and_ it automatically casts to any
> pointer type the caller uses (which is the reason it's used in malloc).
>
Ok with me. Oppositions from anyone else?

>
> I've also added an "undo" function for install_int13_handler, as
> partially required by the preboot hook interface, but I really have no
> idea how to test it, so... is this implementation sensible? It just
> restores the old int13 handler and frees the allocated memory.
>
It's what it's supposed to do. You can make a fake loader which will just
return and check that (hd0) and (hd1) aren't swapped in grub2

>
> --
> -- Lazy, Oblivious, Recurrent Disaster -- Habbit
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>


-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #2: Type: text/html, Size: 2269 bytes --]

Re: status grub2 port of grub-legasy map command

[Pavel Roskin]

On Sun, 2009-05-03 at 11:17 +0200, Vladimir 'phcoder' Serbinenko wrote:

>         I would suggest, however, that the return type of
>         grub_mmap_malign_and_register be changed to void* from char*,
>         just like
>         the return type from malloc, because it's the meaningful data
>         type to
>         indicate a pointer to generic memory _and_ it automatically
>         casts to any
>         pointer type the caller uses (which is the reason it's used in
>         malloc).
> Ok with me. Oppositions from anyone else?

Committed.

Javier, please remove the unneeded cast.

-- 
Regards,
Pavel Roskin

Re: status grub2 port of grub-legasy map command

[Pavel Roskin]

[-- Attachment #1: Type: text/plain, Size: 1817 bytes --]

On Sun, 2009-05-03 at 02:02 +0200, Javier Martín wrote:
> I am glad to inform that, with the new version of the mmap patch,
> drivemap now boots all my hd1 installs of:
> - Windows XP (Pro x64)
> - ReactOS
> - FreeDOS

I confirm that 32-bit Windows XP is working too.

> I would suggest, however, that the return type of
> grub_mmap_malign_and_register be changed to void* from char*, just like
> the return type from malloc, because it's the meaningful data type to
> indicate a pointer to generic memory _and_ it automatically casts to any
> pointer type the caller uses (which is the reason it's used in malloc).

Done.

A few comments regarding the code.

The patch adds many trailing spaces.  I suggest that you run GNU indent
on drivemap.c.  It will take care of most of the trailing spaces.
Comments will still need to be fixed.

Assembler files use different formatting in GRUB.  Also, it's better to
use meaningful names for the labels.  Label 4 is unused.

Some comments are excessive or unnecessary.  "These functions defined in
this file may be called from C" - irrelevant, we have no such functions.
Complaints that the processor is not in 64-bit mode are also useless.  I
don't understand what "bundle" means in the comments.

Why do we have duplication between grub_drivemap_int13_mapstart and
grub_drivemap_int13_size?

What is "(void) mod;"?  It doesn't prevent any warnings for me.

grub_symbol_t is already used in kern/dl.c with a different meaning.
Why not just use void?

Please use "void" in the argument list if the function takes no
arguments, as in uninstall_int13_handler().

"two arguments required" may be misleading.  In some cases, only one
argument is required, such as "-l".  Let's make drivemap without
arguments show the map.

Improved patch is attached.

-- 
Regards,
Pavel Roskin

[-- Attachment #2: 01-drivemap.patch --]
[-- Type: text/x-patch, Size: 20258 bytes --]

diff --git a/commands/i386/pc/drivemap.c b/commands/i386/pc/drivemap.c
new file mode 100644
index 0000000..e56b5f0
--- /dev/null
+++ b/commands/i386/pc/drivemap.c
@@ -0,0 +1,464 @@
+/* drivemap.c - command to manage the BIOS drive mappings.  */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2008, 2009  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/machine/drivemap.h>
+#include <grub/extcmd.h>
+#include <grub/dl.h>
+#include <grub/mm.h>
+#include <grub/misc.h>
+#include <grub/disk.h>
+#include <grub/machine/biosdisk.h>
+#include <grub/loader.h>
+#include <grub/machine/memory.h>
+
+
+#define MODNAME "drivemap"
+
+static grub_extcmd_t cmd_reghandle;
+
+/* Remember to update enum opt_idxs accordingly.  */
+static const struct grub_arg_option options[] = {
+  {"list", 'l', 0, "show the current mappings", 0, 0},
+  {"reset", 'r', 0, "reset all mappings to the default values", 0, 0},
+  {"swap", 's', 0, "perform both direct and reverse mappings", 0, 0},
+  {0, 0, 0, 0, 0, 0}
+};
+
+/* Remember to update options[] accordingly.  */
+enum opt_idxs
+{
+  OPTIDX_LIST = 0,
+  OPTIDX_RESET,
+  OPTIDX_SWAP,
+};
+
+typedef struct drivemap_node
+{
+  grub_uint8_t newdrive;
+  grub_uint8_t redirto;
+  struct drivemap_node *next;
+} drivemap_node_t;
+
+static drivemap_node_t *map_head;
+static void *insthandler_hook;
+static int handlermem_hnd;
+static grub_err_t install_int13_handler (int noret __attribute__ ((unused)));
+static grub_err_t uninstall_int13_handler (void);
+
+/* Puts the specified mapping into the table, replacing an existing mapping
+   for newdrive or adding a new one if required.  */
+static grub_err_t
+drivemap_set (grub_uint8_t newdrive, grub_uint8_t redirto)
+{
+  drivemap_node_t *mapping = 0;
+  drivemap_node_t *search = map_head;
+  while (search)
+    {
+      if (search->newdrive == newdrive)
+	{
+	  mapping = search;
+	  break;
+	}
+      search = search->next;
+    }
+
+
+  /* Check for pre-existing mappings to modify before creating a new one.  */
+  if (mapping)
+    mapping->redirto = redirto;
+  else
+    {
+      mapping = grub_malloc (sizeof (drivemap_node_t));
+      if (!mapping)
+	return grub_error (GRUB_ERR_OUT_OF_MEMORY,
+			   "cannot allocate map entry, not enough memory");
+      mapping->newdrive = newdrive;
+      mapping->redirto = redirto;
+      mapping->next = map_head;
+      map_head = mapping;
+    }
+  return GRUB_ERR_NONE;
+}
+
+/* Removes the mapping for newdrive from the table.  If there is no mapping,
+   then this function behaves like a no-op on the map.  */
+static void
+drivemap_remove (grub_uint8_t newdrive)
+{
+  drivemap_node_t *mapping = 0;
+  drivemap_node_t *search = map_head;
+  drivemap_node_t *previous = 0;
+
+  while (search)
+    {
+      if (search->newdrive == newdrive)
+	{
+	  mapping = search;
+	  break;
+	}
+      previous = search;
+      search = search->next;
+    }
+
+  if (mapping)
+    {
+      if (previous)
+	previous->next = mapping->next;
+      else			/* Entry was head of list.  */
+	map_head = mapping->next;
+      grub_free (mapping);
+    }
+}
+
+/* Given a device name, resolves its BIOS disk number and stores it in the
+   passed location, which should only be trusted if ERR_NONE is returned.  */
+static grub_err_t
+parse_biosdisk (const char *name, grub_uint8_t * disknum)
+{
+  grub_disk_t disk;
+  if (!name || *name == 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name empty");
+  /* Skip the first ( in (hd0) - disk_open wants just the name.  */
+  if (*name == '(')
+    name++;
+
+  disk = grub_disk_open (name);
+  if (!disk)
+    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "unknown device \"%s\"",
+		       name);
+  else
+    {
+      const enum grub_disk_dev_id id = disk->dev->id;
+      /* The following assignment is only sound if the device is indeed a
+         biosdisk.  The caller must check the return value.  */
+      if (disknum)
+	*disknum = disk->id;
+      grub_disk_close (disk);
+      if (id == GRUB_DISK_DEVICE_BIOSDISK_ID)
+	return GRUB_ERR_NONE;
+      else
+	return grub_error (GRUB_ERR_BAD_DEVICE, "%s is not a BIOS disk",
+			   name);
+    }
+}
+
+/* Given a BIOS disk number, returns its GRUB device name if it exists.
+   If the call succeeds, the resulting device string must be freed.
+   For nonexisting BIOS disk numbers, this function returns
+   GRUB_ERR_UNKNOWN_DEVICE.  */
+static grub_err_t
+revparse_biosdisk (const grub_uint8_t dnum, const char **output)
+{
+  int found = 0;
+  auto int find (const char *name);
+  int find (const char *name)
+  {
+    const grub_disk_t disk = grub_disk_open (name);
+    if (!disk)
+      return 0;
+    else
+      {
+
+	if (disk->id == dnum && disk->dev->id == GRUB_DISK_DEVICE_BIOSDISK_ID)
+	  {
+	    found = 1;
+	    if (output)
+	      *output = grub_strdup (name);
+	  }
+	grub_disk_close (disk);
+	return found;
+      }
+  }
+
+  grub_disk_dev_iterate (find);
+  if (found)
+    return GRUB_ERR_NONE;
+  else
+    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "BIOS disk %02x not found",
+		       dnum);
+}
+
+/* Given a GRUB-like device name and a convenient location, stores the
+   related BIOS disk number.  Accepts devices like \((f|h)dN\), with
+   0 <= N < 128.  */
+static grub_err_t
+tryparse_diskstring (const char *str, grub_uint8_t * output)
+{
+  if (!str || *str == 0)
+    goto fail;
+  /* Skip opening paren in order to allow both (hd0) and hd0.  */
+  if (*str == '(')
+    str++;
+  if ((str[0] == 'f' || str[0] == 'h') && str[1] == 'd')
+    {
+      grub_uint8_t bios_num = (str[0] == 'h') ? 0x80 : 0x00;
+      grub_errno = GRUB_ERR_NONE;
+      unsigned long drivenum = grub_strtoul (str + 2, 0, 0);
+      if (grub_errno != GRUB_ERR_NONE || drivenum > 127)
+	{
+	  /* N not a number or out of range.  */
+	  goto fail;
+	}
+      else
+	{
+	  bios_num |= drivenum;
+	  if (output)
+	    *output = bios_num;
+	  return GRUB_ERR_NONE;
+	}
+    }
+  else
+    goto fail;
+
+fail:
+  return grub_error (GRUB_ERR_BAD_ARGUMENT, "device format \"%s\" "
+		     "invalid: must be (f|h)dN, with 0 <= N < 128", str);
+}
+
+static grub_err_t
+grub_cmd_drivemap (struct grub_extcmd *cmd, int argc, char **args)
+{
+  if (cmd->state[OPTIDX_LIST].set || argc == 0)
+    {
+      /* Show: list mappings.  */
+      if (!map_head)
+	grub_printf ("No drives have been remapped");
+      else
+	{
+	  grub_printf ("Showing only remapped drives.\n");
+	  grub_printf ("BIOS disk #num ----> GRUB device\n");
+	  drivemap_node_t *curnode = map_head;
+	  while (curnode)
+	    {
+	      const char *dname = 0;
+	      grub_err_t err = revparse_biosdisk (curnode->redirto, &dname);
+	      if (err != GRUB_ERR_NONE)
+		return grub_error (err, "invalid mapping: non-existent disk"
+				   "or not managed by the BIOS");
+	      grub_printf ("%cD #%-3u (0x%02x)       %s\n",
+			   (curnode->newdrive & 0x80) ? 'H' : 'F',
+			   curnode->newdrive & 0x7F, curnode->newdrive,
+			   dname);
+	      curnode = curnode->next;
+	      grub_free ((char *) dname);
+	    }
+	}
+    }
+  else if (cmd->state[OPTIDX_RESET].set)
+    {
+      /* Reset: just delete all mappings, freeing their memory.  */
+      drivemap_node_t *curnode = map_head;
+      drivemap_node_t *prevnode = 0;
+      while (curnode)
+	{
+	  prevnode = curnode;
+	  curnode = curnode->next;
+	  grub_free (prevnode);
+	}
+      map_head = 0;
+    }
+  else
+    {
+      /* Neither flag: put mapping.  */
+      grub_uint8_t mapfrom = 0;
+      grub_uint8_t mapto = 0xFF;
+      grub_err_t err;
+
+      if (argc != 2)
+	return grub_error (GRUB_ERR_BAD_ARGUMENT, "two arguments required");
+
+      err = parse_biosdisk (args[0], &mapfrom);
+      if (err != GRUB_ERR_NONE)
+	return err;
+
+      /* When swapping we require both devices to be BIOS disks, but when
+         performing direct mappings we only require the 2nd argument to look
+         like a BIOS disk in order to resolve it into a BIOS disk number.  */
+      if (cmd->state[OPTIDX_SWAP].set)
+	err = parse_biosdisk (args[1], &mapto);
+      else
+	err = tryparse_diskstring (args[1], &mapto);
+      if (err != GRUB_ERR_NONE)
+	return err;
+
+      if (mapto == mapfrom)
+	{
+	  /* Reset to default.  */
+	  grub_dprintf (MODNAME, "Removing the mapping for %s (%02x)",
+			args[0], mapfrom);
+	  drivemap_remove (mapfrom);
+	}
+      else
+	{
+	  /* Set the mapping for the disk (overwrites any existing mapping).  */
+	  grub_dprintf (MODNAME, "%s %s (%02x) = %s (%02x)\n",
+			cmd->state[OPTIDX_SWAP].set ? "Swapping" : "Mapping",
+			args[1], mapto, args[0], mapfrom);
+	  err = drivemap_set (mapto, mapfrom);
+	  /* If -s, perform the reverse mapping too (only if the first was OK).  */
+	  if (cmd->state[OPTIDX_SWAP].set && err == GRUB_ERR_NONE)
+	    err = drivemap_set (mapfrom, mapto);
+	  return err;
+	}
+    }
+
+  return GRUB_ERR_NONE;
+}
+
+typedef struct __attribute__ ((packed)) int13map_node
+{
+  grub_uint8_t disknum;
+  grub_uint8_t mapto;
+} int13map_node_t;
+
+#define INT13H_OFFSET(x) ( ((grub_uint8_t*)(x)) - ((grub_uint8_t*)&grub_drivemap_int13_handler_base) )
+#define INT13H_REBASE(x) ( (void*) (handler_base + (x)) )
+#define INT13H_TONEWADDR(x) INT13H_REBASE( INT13H_OFFSET( x ) )
+
+/* Int13h handler installer - reserves conventional memory for the handler,
+   copies it over and sets the IVT entry for int13h.
+   This code rests on the assumption that GRUB does not activate any kind
+   of memory mapping apart from identity paging, since it accesses
+   realmode structures by their absolute addresses, like the IVT at 0;
+   and transforms a pmode pointer into a rmode seg:off far ptr.  */
+static grub_err_t
+install_int13_handler (int noret __attribute__ ((unused)))
+{
+  grub_size_t entries = 0;
+  drivemap_node_t *curentry = map_head;
+
+  /* Count entries to prepare a contiguous map block.  */
+  while (curentry)
+    {
+      entries++;
+      curentry = curentry->next;
+    }
+  if (entries == 0)
+    {
+      /* No need to install the int13h handler.  */
+      grub_dprintf (MODNAME, "No drives marked as remapped, installation "
+		    "of an int13h handler is not required.");
+      return GRUB_ERR_NONE;
+    }
+  else
+    {
+      /* Real mode IVT slot (seg:off far pointer) for interrupt 0x13.  */
+      grub_uint32_t *ivtslot = UINT_TO_PTR (0x0000004c);
+      /* Size of the full int13 handler "bundle", including code and map.  */
+      grub_uint64_t total_size;
+      /* Base address of the space reserved for the handler bundle.  */
+      grub_uint8_t *handler_base = 0;
+      /* Address of the map within the deployed bundle.  */
+      int13map_node_t *handler_map;
+      /* Real mode IVT entry (seg:off far pointer) for the new handler.  */
+      grub_uint32_t ivtentry;
+
+      grub_dprintf (MODNAME, "Installing int13h handler...\n");
+
+      /* Save the pointer to the old handler.  */
+      grub_drivemap_int13_oldhandler = *ivtslot;
+      grub_dprintf (MODNAME, "Old int13 handler at %04x:%04x\n",
+		    (grub_drivemap_int13_oldhandler >> 16) & 0x0ffff,
+		    grub_drivemap_int13_oldhandler & 0x0ffff);
+
+      /* Find a rmode-segment-aligned zone in conventional memory big
+         enough to hold the handler and its data.  */
+      total_size = INT13H_OFFSET(&grub_drivemap_int13_mapstart)
+	+ (entries + 1) * sizeof (int13map_node_t);
+      grub_dprintf (MODNAME, "Payload is %llu bytes long\n", total_size);
+      handler_base =
+	grub_mmap_malign_and_register (16, total_size,
+				       &handlermem_hnd,
+				       GRUB_MACHINE_MEMORY_RESERVED,
+				       GRUB_MMAP_MALLOC_LOW);
+      if (!handler_base)
+	return grub_error (GRUB_ERR_OUT_OF_MEMORY, "Could not reserve "
+			   "memory for the int13h handler");
+
+      /* Copy int13h handler bundle to reserved area.  */
+      grub_dprintf (MODNAME, "Reserved memory at %p, copying handler...\n",
+		    handler_base);
+      grub_memcpy (handler_base, &grub_drivemap_int13_handler_base,
+		   INT13H_OFFSET(&grub_drivemap_int13_mapstart));
+
+      /* Copy the mappings to the reserved area.  */
+      curentry = map_head;
+      grub_size_t i;
+      handler_map = INT13H_TONEWADDR (&grub_drivemap_int13_mapstart);
+      grub_dprintf (MODNAME, "Target map at %p, copying mappings...\n",
+		    handler_map);
+      for (i = 0; i < entries && curentry; ++i, curentry = curentry->next)
+	{
+	  handler_map[i].disknum = curentry->newdrive;
+	  handler_map[i].mapto = curentry->redirto;
+	  grub_dprintf (MODNAME, "\t#%d: 0x%02x <- 0x%02x\n", i,
+			handler_map[i].disknum, handler_map[i].mapto);
+	}
+      /* Signal end-of-map.  */
+      handler_map[i].disknum = 0;
+      handler_map[i].mapto = 0;
+      grub_dprintf (MODNAME, "\t#%d: 0x00 <- 0x00 (end)\n", i);
+
+      /* Install our function as the int13h handler in the IVT.  */
+      ivtentry = ((grub_uint32_t) handler_base) << 12;	/* Segment address.  */
+      ivtentry |=
+	(grub_uint16_t) INT13H_OFFSET (&grub_drivemap_int13_handler);
+      grub_dprintf (MODNAME, "New int13 handler IVT pointer: %04x:%04x\n",
+		    (ivtentry >> 16) & 0x0ffff, ivtentry & 0x0ffff);
+      *ivtslot = ivtentry;
+
+      return GRUB_ERR_NONE;
+    }
+}
+
+static grub_err_t
+uninstall_int13_handler (void)
+{
+  grub_uint32_t *ivtslot = UINT_TO_PTR (0x0000004c);
+
+  if (!grub_drivemap_int13_oldhandler)
+    return GRUB_ERR_NONE;
+
+  *ivtslot = grub_drivemap_int13_oldhandler;
+  grub_mmap_free_and_unregister (handlermem_hnd);
+  grub_drivemap_int13_oldhandler = 0;
+
+  return GRUB_ERR_NONE;
+}
+
+GRUB_MOD_INIT (drivemap)
+{
+  cmd_reghandle = grub_register_extcmd (MODNAME, grub_cmd_drivemap,
+					GRUB_COMMAND_FLAG_BOTH,
+					MODNAME
+					" -l | -r | [-s] grubdev biosdisk",
+					"Manage the BIOS drive mappings",
+					options);
+  insthandler_hook =
+    grub_loader_register_preboot_hook (&install_int13_handler,
+				       &uninstall_int13_handler,
+				       GRUB_LOADER_PREBOOT_HOOK_PRIO_NORMAL);
+}
+
+GRUB_MOD_FINI (drivemap)
+{
+  grub_loader_unregister_preboot_hook (insthandler_hook);
+  insthandler_hook = 0;
+  grub_unregister_extcmd (cmd_reghandle);
+}
diff --git a/commands/i386/pc/drivemap_int13h.S b/commands/i386/pc/drivemap_int13h.S
new file mode 100644
index 0000000..66fa05c
--- /dev/null
+++ b/commands/i386/pc/drivemap_int13h.S
@@ -0,0 +1,84 @@
+/* drivemap_int13h.S - interrupt handler for the BIOS drive remapper */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2008, 2009  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/symbol.h>
+
+#define GRUB_DRIVEMAP_INT13H_OFFSET(x) ((x) - grub_drivemap_int13_handler_base)
+
+/* Copy starts here.  When deployed, this label must be segment-aligned.  */
+VARIABLE(grub_drivemap_int13_handler_base)
+
+/* Far pointer to the old handler.  Stored as a CS:IP in the style of real-mode
+   IVT entries (thus PI:SC in mem).  */
+VARIABLE(grub_drivemap_int13_oldhandler)
+	.word 0x0, 0x0
+
+/* Drivemap module bundle - INT 13h handler - BIOS HD map.  */
+/* We need to use relative addressing, and with CS to top it all, since we
+   must make as few changes to the registers as possible.  */
+.code16
+FUNCTION(grub_drivemap_int13_handler)
+	push	%bp
+	mov	%sp, %bp
+
+	/* Map the drive number (always in DL?).  */
+	push	%ax
+	push	%bx
+	push	%si
+	mov	$GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_mapstart), %bx
+	xor	%si, %si
+
+more_remaining:
+	movw	%cs:(%bx,%si), %ax
+	cmp	%ah, %al
+	jz	not_found /* DRV=DST => map end - drive not remapped, keep DL.  */
+	cmp	%dl, %al
+	jz	found /* Found - drive remapped, modify DL.  */
+	add	$2, %si
+	jmp	more_remaining /* Not found, but more remaining, loop.  */
+
+found:
+	mov	%ah, %dl
+
+not_found:
+	pop	%si
+	pop	%bx
+	pop	%ax
+
+	push	%bp
+	/* Simulate interrupt call: push flags and do a far call in order to set
+	   the stack the way the old handler expects it so that its iret works.  */
+	push	6(%bp)
+	movw	(%bp), %bp  /* Restore the caller BP (is this needed and/or sensible?).  */
+	lcall	*%cs:GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_oldhandler)
+	pop	%bp /* The pushed flags were removed by iret.  */
+	/* Set the saved flags to what the int13h handler returned.  */
+	push	%ax
+	pushf
+	pop	%ax
+	movw	%ax, 6(%bp)
+	pop	%ax
+	mov	%bp, %sp
+	pop	%bp
+	iret
+
+/* This label MUST be at the end of the copied block, since the installer code
+   reserves additional space for mappings at runtime and copies them over it.  */
+.align 2
+VARIABLE(grub_drivemap_int13_mapstart)
diff --git a/conf/i386-pc.rmk b/conf/i386-pc.rmk
index 6c9100f..ed60d53 100644
--- a/conf/i386-pc.rmk
+++ b/conf/i386-pc.rmk
@@ -185,7 +185,15 @@ pkglib_MODULES = biosdisk.mod chain.mod \
 	aout.mod bsd.mod pxe.mod pxecmd.mod datetime.mod date.mod \
 	datehook.mod lsmmap.mod ata_pthru.mod hdparm.mod \
 	usb.mod uhci.mod ohci.mod usbtest.mod usbms.mod usb_keyboard.mod \
-	efiemu.mod mmap.mod acpi.mod
+	efiemu.mod mmap.mod acpi.mod drivemap.mod
+
+# For drivemap.mod.
+drivemap_mod_HEADERS = machine/drivemap.h
+drivemap_mod_SOURCES = commands/i386/pc/drivemap.c \
+                       commands/i386/pc/drivemap_int13h.S
+drivemap_mod_ASFLAGS = $(COMMON_ASFLAGS)
+drivemap_mod_CFLAGS = $(COMMON_CFLAGS)
+drivemap_mod_LDFLAGS = $(COMMON_LDFLAGS)
 
 # For efiemu.mod.
 efiemu_mod_SOURCES = efiemu/main.c efiemu/i386/loadcore32.c \
diff --git a/include/grub/i386/pc/drivemap.h b/include/grub/i386/pc/drivemap.h
new file mode 100644
index 0000000..f26f4d6
--- /dev/null
+++ b/include/grub/i386/pc/drivemap.h
@@ -0,0 +1,38 @@
+/* drivemap.h - command to manage the BIOS drive mappings.  */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2008  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef GRUB_DRIVEMAP_MACHINE_HEADER
+#define GRUB_DRIVEMAP_MACHINE_HEADER	1
+
+#include <grub/types.h>
+
+/* Realmode far ptr (2 * 16b) to the previous INT13h handler.  */
+extern grub_uint32_t grub_drivemap_int13_oldhandler;
+
+/* Start of the handler bundle.  */
+extern void grub_drivemap_int13_handler_base;
+
+/* Start of the drive mappings area (space reserved at runtime).  */
+extern void grub_drivemap_int13_mapstart;
+
+/* The assembly function to replace the old INT13h handler. It should not be
+   called because it does not follow any C callspecs and returns with IRET.  */
+extern void grub_drivemap_int13_handler;
+
+#endif /* ! GRUB_DRIVEMAP_MACHINE_HEADER */

Re: status grub2 port of grub-legasy map command

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 3605 bytes --]

El dom, 03-05-2009 a las 16:59 -0400, Pavel Roskin escribió:
> On Sun, 2009-05-03 at 02:02 +0200, Javier Martín wrote:
> > I am glad to inform that, with the new version of the mmap patch,
> > drivemap now boots all my hd1 installs of:
> > - Windows XP (Pro x64)
> > - ReactOS
> > - FreeDOS
> 
> I confirm that 32-bit Windows XP is working too.
> 
> > I would suggest, however, that the return type of
> > grub_mmap_malign_and_register be changed to void* from char*, just like
> > the return type from malloc, because it's the meaningful data type to
> > indicate a pointer to generic memory _and_ it automatically casts to any
> > pointer type the caller uses (which is the reason it's used in malloc).
> 
> Done.
> 
> A few comments regarding the code.
> 
> The patch adds many trailing spaces.  I suggest that you run GNU indent
> on drivemap.c.  It will take care of most of the trailing spaces.
> Comments will still need to be fixed.
> 
> Assembler files use different formatting in GRUB.  Also, it's better to
> use meaningful names for the labels.  Label 4 is unused.
Where can I find those assembly formatting conventions? From what I see
in your version of the patch, the gist is that asm instructions start at
the 8th column (not after a \t). This carries an unpleasant
FORTRAN-esque smell that I would rather avoid.

> 
> Some comments are excessive or unnecessary.  "These functions defined in
> this file may be called from C" - irrelevant, we have no such functions.
> Complaints that the processor is not in 64-bit mode are also useless.  I
> don't understand what "bundle" means in the comments.
Sorry, I copied the initial comment from another .S file in GRUB2 as a
kind-of-boilerplate. In this context, "bundle" means the whole "int13
handler" package that is deployed to the reserved memory address,
consisting of the old IVT pointer, the actual int13h handler code and
the entry map.

> 
> Why do we have duplication between grub_drivemap_int13_mapstart and
> grub_drivemap_int13_size?
It was initially added so that install_int13_handler would make no
assumptions about the structure of the bundle it was copying. However,
given that it already assumes that the map space is the last thing in
the bundle, your change is fine.

> 
> What is "(void) mod;"?  It doesn't prevent any warnings for me.
Once again, boilerplate code copied from hello.c long ago. I suspect
this stopped being required when the command framework was revamped.

> 
> grub_symbol_t is already used in kern/dl.c with a different meaning.
> Why not just use void?
The reason to avoid using a plain "void" is that it might be a slightly
confusing sight, so a future coder might have an idea-of-the-moment and
change it to a "meaningful type" like void*. The presence of an explicit
type with a big comment is supposed to at least make people think twice
before changing it.

> 
> Please use "void" in the argument list if the function takes no
> arguments, as in uninstall_int13_handler().
> 
> "two arguments required" may be misleading.  In some cases, only one
> argument is required, such as "-l".  Let's make drivemap without
> arguments show the map.
Good idea.

> 
> Improved patch is attached.
Thanks. I will read it thoroughly tomorrow when I'm back from uni. I
think that drivemap.h could be scrapped, its contents incorporated into
drivemap.c so as to reduce clutter and bitrot potential, and would
reduce the impact of the declaration of grub_symbol_t even more.

-- 
-- Lazy, Oblivious, Recurrent Disaster -- Habbit

[-- Attachment #2: Esto es una parte de mensaje firmado digitalmente --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: status grub2 port of grub-legasy map command

[Pavel Roskin]

On Mon, 2009-05-04 at 01:37 +0200, Javier Martín wrote:

> > The patch adds many trailing spaces.  I suggest that you run GNU indent
> > on drivemap.c.  It will take care of most of the trailing spaces.
> > Comments will still need to be fixed.
> > 
> > Assembler files use different formatting in GRUB.  Also, it's better to
> > use meaningful names for the labels.  Label 4 is unused.
> Where can I find those assembly formatting conventions? From what I see
> in your version of the patch, the gist is that asm instructions start at
> the 8th column (not after a \t). This carries an unpleasant
> FORTRAN-esque smell that I would rather avoid.

I don't know if it's documented anywhere, but it's sufficient to looks
at other *.S files in GRUB or another GNU project, such as GCC.

> > Some comments are excessive or unnecessary.  "These functions defined in
> > this file may be called from C" - irrelevant, we have no such functions.
> > Complaints that the processor is not in 64-bit mode are also useless.  I
> > don't understand what "bundle" means in the comments.
> Sorry, I copied the initial comment from another .S file in GRUB2 as a
> kind-of-boilerplate. In this context, "bundle" means the whole "int13
> handler" package that is deployed to the reserved memory address,
> consisting of the old IVT pointer, the actual int13h handler code and
> the entry map.

It would be great if you change the comments to avoid the word "bundle"
unless it's explained.

> > What is "(void) mod;"?  It doesn't prevent any warnings for me.
> Once again, boilerplate code copied from hello.c long ago. I suspect
> this stopped being required when the command framework was revamped.

Correct.  We are using gcc attributes to suppress warnings in
GRUB_MOD_INIT.  I've committed a patch that removes all that stuff.

> > grub_symbol_t is already used in kern/dl.c with a different meaning.
> > Why not just use void?
> The reason to avoid using a plain "void" is that it might be a slightly
> confusing sight, so a future coder might have an idea-of-the-moment and
> change it to a "meaningful type" like void*. The presence of an explicit
> type with a big comment is supposed to at least make people think twice
> before changing it.

You can leave the comment but use void.  I don't think anyone (of the
reasonable people allowed to touch GRUB code) would change the type if
the code is working.

> > Improved patch is attached.
> Thanks. I will read it thoroughly tomorrow when I'm back from uni. I
> think that drivemap.h could be scrapped, its contents incorporated into
> drivemap.c so as to reduce clutter and bitrot potential, and would
> reduce the impact of the declaration of grub_symbol_t even more.

That's a good idea.  Still, I would prefer that we don't use
grub_symbol_t where void is be sufficient.

-- 
Regards,
Pavel Roskin

Re: status grub2 port of grub-legasy map command

[Javier Martín]

[-- Attachment #1.1: Type: text/plain, Size: 339 bytes --]

Here is a new version of the patch. As you suggested, "grub_symbol_t"
was replaced with "void". Also, drivemap.h no longer exists, its little
content integrated into drivemap.c. Last but not least, I've mostly
adopted your version of the assembly file (indenting, labels, etc.)

-- 
-- Lazy, Oblivious, Recurrent Disaster -- Habbit

[-- Attachment #1.2: drivemap.10.patch --]
[-- Type: text/x-patch, Size: 20156 bytes --]

Index: conf/i386-pc.rmk
===================================================================
--- conf/i386-pc.rmk	(revision 2195)
+++ conf/i386-pc.rmk	(working copy)
@@ -185,8 +185,15 @@
 	aout.mod bsd.mod pxe.mod pxecmd.mod datetime.mod date.mod \
 	datehook.mod lsmmap.mod ata_pthru.mod hdparm.mod \
 	usb.mod uhci.mod ohci.mod usbtest.mod usbms.mod usb_keyboard.mod \
-	efiemu.mod mmap.mod acpi.mod
+	efiemu.mod mmap.mod acpi.mod drivemap.mod
 
+# For drivemap.mod.
+drivemap_mod_SOURCES = commands/i386/pc/drivemap.c \
+                       commands/i386/pc/drivemap_int13h.S
+drivemap_mod_ASFLAGS = $(COMMON_ASFLAGS)
+drivemap_mod_CFLAGS = $(COMMON_CFLAGS)
+drivemap_mod_LDFLAGS = $(COMMON_LDFLAGS)
+
 # For efiemu.mod.
 efiemu_mod_SOURCES = efiemu/main.c efiemu/i386/loadcore32.c \
 		     efiemu/i386/loadcore64.c efiemu/i386/pc/cfgtables.c \
Index: commands/i386/pc/drivemap.c
===================================================================
--- commands/i386/pc/drivemap.c	(revision 0)
+++ commands/i386/pc/drivemap.c	(revision 0)
@@ -0,0 +1,475 @@
+/* drivemap.c - command to manage the BIOS drive mappings.  */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2008, 2009  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/extcmd.h>
+#include <grub/dl.h>
+#include <grub/mm.h>
+#include <grub/misc.h>
+#include <grub/disk.h>
+#include <grub/machine/biosdisk.h>
+#include <grub/loader.h>
+#include <grub/machine/memory.h>
+
+
+#define MODNAME "drivemap"
+
+static grub_extcmd_t cmd_reghandle;
+
+/* Remember to update enum opt_idxs accordingly.  */
+static const struct grub_arg_option options[] = {
+  {"list", 'l', 0, "show the current mappings", 0, 0},
+  {"reset", 'r', 0, "reset all mappings to the default values", 0, 0},
+  {"swap", 's', 0, "perform both direct and reverse mappings", 0, 0},
+  {0, 0, 0, 0, 0, 0}
+};
+
+/* Remember to update options[] accordingly.  */
+enum opt_idxs
+{
+  OPTIDX_LIST = 0,
+  OPTIDX_RESET,
+  OPTIDX_SWAP,
+};
+
+/* Realmode far ptr (2 * 16b) to the previous INT13h handler.  */
+extern grub_uint32_t grub_drivemap_int13_oldhandler;
+
+/* The type "void" is used for imported assembly labels, takes no storage and
+   serves just to take the address with &label.  Do NOT put void* here.  */
+/* Start of the handler bundle.  */
+extern void grub_drivemap_int13_handler_base;
+/* Start of the drive mappings area (space reserved at runtime).  */
+extern void grub_drivemap_int13_mapstart;
+/* The assembly function to replace the old INT13h handler. It should not be
+   called because it does not follow any C callspecs and returns with IRET.  */
+extern void grub_drivemap_int13_handler;
+
+typedef struct drivemap_node
+{
+  grub_uint8_t newdrive;
+  grub_uint8_t redirto;
+  struct drivemap_node *next;
+} drivemap_node_t;
+
+static drivemap_node_t *map_head;
+static void *insthandler_hook;
+static int handlermem_hnd;
+static grub_err_t install_int13_handler (int noret __attribute__ ((unused)));
+static grub_err_t uninstall_int13_handler (void);
+
+/* Puts the specified mapping into the table, replacing an existing mapping
+   for newdrive or adding a new one if required.  */
+static grub_err_t
+drivemap_set (grub_uint8_t newdrive, grub_uint8_t redirto)
+{
+  drivemap_node_t *mapping = 0;
+  drivemap_node_t *search = map_head;
+  while (search)
+    {
+      if (search->newdrive == newdrive)
+	{
+	  mapping = search;
+	  break;
+	}
+      search = search->next;
+    }
+
+
+  /* Check for pre-existing mappings to modify before creating a new one.  */
+  if (mapping)
+    mapping->redirto = redirto;
+  else
+    {
+      mapping = grub_malloc (sizeof (drivemap_node_t));
+      if (!mapping)
+	return grub_error (GRUB_ERR_OUT_OF_MEMORY,
+			   "cannot allocate map entry, not enough memory");
+      mapping->newdrive = newdrive;
+      mapping->redirto = redirto;
+      mapping->next = map_head;
+      map_head = mapping;
+    }
+  return GRUB_ERR_NONE;
+}
+
+/* Removes the mapping for newdrive from the table.  If there is no mapping,
+   then this function behaves like a no-op on the map.  */
+static void
+drivemap_remove (grub_uint8_t newdrive)
+{
+  drivemap_node_t *mapping = 0;
+  drivemap_node_t *search = map_head;
+  drivemap_node_t *previous = 0;
+
+  while (search)
+    {
+      if (search->newdrive == newdrive)
+	{
+	  mapping = search;
+	  break;
+	}
+      previous = search;
+      search = search->next;
+    }
+
+  if (mapping)
+    {
+      if (previous)
+	previous->next = mapping->next;
+      else			/* Entry was head of list.  */
+	map_head = mapping->next;
+      grub_free (mapping);
+    }
+}
+
+/* Given a device name, resolves its BIOS disk number and stores it in the
+   passed location, which should only be trusted if ERR_NONE is returned.  */
+static grub_err_t
+parse_biosdisk (const char *name, grub_uint8_t * disknum)
+{
+  grub_disk_t disk;
+  if (!name || *name == 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name empty");
+  /* Skip the first ( in (hd0) - disk_open wants just the name.  */
+  if (*name == '(')
+    name++;
+
+  disk = grub_disk_open (name);
+  if (!disk)
+    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "unknown device \"%s\"",
+		       name);
+  else
+    {
+      const enum grub_disk_dev_id id = disk->dev->id;
+      /* The following assignment is only sound if the device is indeed a
+         biosdisk.  The caller must check the return value.  */
+      if (disknum)
+	*disknum = disk->id;
+      grub_disk_close (disk);
+      if (id == GRUB_DISK_DEVICE_BIOSDISK_ID)
+	return GRUB_ERR_NONE;
+      else
+	return grub_error (GRUB_ERR_BAD_DEVICE, "%s is not a BIOS disk",
+			   name);
+    }
+}
+
+/* Given a BIOS disk number, returns its GRUB device name if it exists.  
+   If the call succeeds, the resulting device string must be freed.  
+   For nonexisting BIOS disk numbers, this function returns
+   GRUB_ERR_UNKNOWN_DEVICE.  */
+static grub_err_t
+revparse_biosdisk (const grub_uint8_t dnum, const char **output)
+{
+  int found = 0;
+  auto int find (const char *name);
+  int find (const char *name)
+  {
+    const grub_disk_t disk = grub_disk_open (name);
+    if (!disk)
+      return 0;
+    else
+      {
+
+	if (disk->id == dnum && disk->dev->id == GRUB_DISK_DEVICE_BIOSDISK_ID)
+	  {
+	    found = 1;
+	    if (output)
+	      *output = grub_strdup (name);
+	  }
+	grub_disk_close (disk);
+	return found;
+      }
+  }
+
+  grub_disk_dev_iterate (find);
+  if (found)
+    return GRUB_ERR_NONE;
+  else
+    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "BIOS disk %02x not found",
+		       dnum);
+}
+
+/* Given a GRUB-like device name and a convenient location, stores the
+   related BIOS disk number.  Accepts devices like \((f|h)dN\), with
+   0 <= N < 128.  */
+static grub_err_t
+tryparse_diskstring (const char *str, grub_uint8_t * output)
+{
+  if (!str || *str == 0)
+    goto fail;
+  /* Skip opening paren in order to allow both (hd0) and hd0.  */
+  if (*str == '(')
+    str++;
+  if ((str[0] == 'f' || str[0] == 'h') && str[1] == 'd')
+    {
+      grub_uint8_t bios_num = (str[0] == 'h') ? 0x80 : 0x00;
+      grub_errno = GRUB_ERR_NONE;
+      unsigned long drivenum = grub_strtoul (str + 2, 0, 0);
+      if (grub_errno != GRUB_ERR_NONE || drivenum > 127)
+	{
+	  /* N not a number or out of range.  */
+	  goto fail;
+	}
+      else
+	{
+	  bios_num |= drivenum;
+	  if (output)
+	    *output = bios_num;
+	  return GRUB_ERR_NONE;
+	}
+    }
+  else
+    goto fail;
+
+fail:
+  return grub_error (GRUB_ERR_BAD_ARGUMENT, "device format \"%s\" "
+		     "invalid: must be (f|h)dN, with 0 <= N < 128", str);
+}
+
+static grub_err_t
+grub_cmd_drivemap (struct grub_extcmd *cmd, int argc, char **args)
+{
+  if (cmd->state[OPTIDX_LIST].set || 0 == argc)
+    {
+      /* Show: list mappings.  */
+      if (!map_head)
+	grub_printf ("No drives have been remapped");
+      else
+	{
+	  grub_printf ("Showing only remapped drives.\n");
+	  grub_printf ("BIOS disk #num ----> GRUB device\n");
+	  drivemap_node_t *curnode = map_head;
+	  while (curnode)
+	    {
+	      const char *dname = 0;
+	      grub_err_t err = revparse_biosdisk (curnode->redirto, &dname);
+	      if (err != GRUB_ERR_NONE)
+		return grub_error (err, "invalid mapping: non-existent disk"
+				   "or not managed by the BIOS");
+	      grub_printf ("%cD #%-3u (0x%02x)       %s\n",
+			   (curnode->newdrive & 0x80) ? 'H' : 'F',
+			   curnode->newdrive & 0x7F, curnode->newdrive,
+			   dname);
+	      curnode = curnode->next;
+	      grub_free ((char *) dname);
+	    }
+	}
+    }
+  else if (cmd->state[OPTIDX_RESET].set)
+    {
+      /* Reset: just delete all mappings, freeing their memory.  */
+      drivemap_node_t *curnode = map_head;
+      drivemap_node_t *prevnode = 0;
+      while (curnode)
+	{
+	  prevnode = curnode;
+	  curnode = curnode->next;
+	  grub_free (prevnode);
+	}
+      map_head = 0;
+    }
+  else
+    {
+      /* Neither flag: put mapping.  */
+      grub_uint8_t mapfrom = 0;
+      grub_uint8_t mapto = 0xFF;
+      grub_err_t err;
+
+      if (argc != 2)
+	return grub_error (GRUB_ERR_BAD_ARGUMENT, "two arguments required");
+
+      err = parse_biosdisk (args[0], &mapfrom);
+      if (err != GRUB_ERR_NONE)
+	return err;
+
+      /* When swapping we require both devices to be BIOS disks, but when
+         performing direct mappings we only require the 2nd argument to look
+         like a BIOS disk in order to resolve it into a BIOS disk number.  */
+      if (cmd->state[OPTIDX_SWAP].set)
+	err = parse_biosdisk (args[1], &mapto);
+      else
+	err = tryparse_diskstring (args[1], &mapto);
+      if (err != GRUB_ERR_NONE)
+	return err;
+
+      if (mapto == mapfrom)
+	{
+	  /* Reset to default.  */
+	  grub_dprintf (MODNAME, "Removing the mapping for %s (%02x)",
+			args[0], mapfrom);
+	  drivemap_remove (mapfrom);
+	}
+      else
+	{
+	  /* Set the mapping for the disk (overwrites any existing mapping).  */
+	  grub_dprintf (MODNAME, "%s %s (%02x) = %s (%02x)\n",
+			cmd->state[OPTIDX_SWAP].set ? "Swapping" : "Mapping",
+			args[1], mapto, args[0], mapfrom);
+	  err = drivemap_set (mapto, mapfrom);
+	  /* If -s, perform the reverse mapping too (only if the first was OK).  */
+	  if (cmd->state[OPTIDX_SWAP].set && err == GRUB_ERR_NONE)
+	    err = drivemap_set (mapfrom, mapto);
+	  return err;
+	}
+    }
+
+  return GRUB_ERR_NONE;
+}
+
+typedef struct __attribute__ ((packed)) int13map_node
+{
+  grub_uint8_t disknum;
+  grub_uint8_t mapto;
+} int13map_node_t;
+
+#define INT13H_OFFSET(x) ( ((grub_uint8_t*)(x)) - ((grub_uint8_t*)&grub_drivemap_int13_handler_base) )
+#define INT13H_REBASE(x) ( (void*) (handler_base + (x)) )
+#define INT13H_TONEWADDR(x) INT13H_REBASE( INT13H_OFFSET( x ) )
+
+/* Int13h handler installer - reserves conventional memory for the handler,
+   copies it over and sets the IVT entry for int13h.  
+   This code rests on the assumption that GRUB does not activate any kind
+   of memory mapping apart from identity paging, since it accesses
+   realmode structures by their absolute addresses, like the IVT at 0;
+   and transforms a pmode pointer into a rmode seg:off far ptr.  */
+static grub_err_t
+install_int13_handler (int noret __attribute__ ((unused)))
+{
+  grub_size_t entries = 0;
+  drivemap_node_t *curentry = map_head;
+
+  /* Count entries to prepare a contiguous map block.  */
+  while (curentry)
+    {
+      entries++;
+      curentry = curentry->next;
+    }
+  if (entries == 0)
+    {
+      /* No need to install the int13h handler.  */
+      grub_dprintf (MODNAME, "No drives marked as remapped, installation "
+		    "of an int13h handler is not required.");
+      return GRUB_ERR_NONE;
+    }
+  else
+    {
+      /* Real mode IVT slot (seg:off far pointer) for interrupt 0x13.  */
+      grub_uint32_t *ivtslot = UINT_TO_PTR (0x0000004c);
+      /* Size of the full int13 handler "bundle", including code and map.  */
+      grub_uint64_t total_size;
+      /* Base address of the space reserved for the handler bundle.  */
+      grub_uint8_t *handler_base = 0;
+      /* Address of the map within the deployed bundle.  */
+      int13map_node_t *handler_map;
+      /* Real mode IVT entry (seg:off far pointer) for the new handler.  */
+      grub_uint32_t ivtentry;
+
+      grub_dprintf (MODNAME, "Installing int13h handler...\n");
+
+      /* Save the pointer to the old handler.  */
+      grub_drivemap_int13_oldhandler = *ivtslot;
+      grub_dprintf (MODNAME, "Old int13 handler at %04x:%04x\n",
+		    (grub_drivemap_int13_oldhandler >> 16) & 0x0ffff,
+		    grub_drivemap_int13_oldhandler & 0x0ffff);
+
+      /* Find a rmode-segment-aligned zone in conventional memory big
+         enough to hold the handler and its data.  */
+      total_size = INT13H_OFFSET(&grub_drivemap_int13_mapstart)
+	+ (entries + 1) * sizeof (int13map_node_t);
+      grub_dprintf (MODNAME, "Payload is %llu bytes long\n", total_size);
+      handler_base = grub_mmap_malign_and_register (16, total_size,
+						    &handlermem_hnd,
+						    GRUB_MACHINE_MEMORY_RESERVED,
+						    GRUB_MMAP_MALLOC_LOW);
+      if (!handler_base)
+	return grub_error (GRUB_ERR_OUT_OF_MEMORY, "Could not reserve "
+			   "memory for the int13h handler");
+
+      /* Copy int13h handler bundle to reserved area.  */
+      grub_dprintf (MODNAME, "Reserved memory at %p, copying handler...\n",
+		    handler_base);
+      grub_memcpy (handler_base, &grub_drivemap_int13_handler_base,
+		   INT13H_OFFSET(&grub_drivemap_int13_mapstart));
+
+      /* Copy the mappings to the reserved area.  */
+      curentry = map_head;
+      grub_size_t i;
+      handler_map = INT13H_TONEWADDR (&grub_drivemap_int13_mapstart);
+      grub_dprintf (MODNAME, "Target map at %p, copying mappings...\n",
+		    handler_map);
+      for (i = 0; i < entries && curentry; ++i, curentry = curentry->next)
+	{
+	  handler_map[i].disknum = curentry->newdrive;
+	  handler_map[i].mapto = curentry->redirto;
+	  grub_dprintf (MODNAME, "\t#%d: 0x%02x <- 0x%02x\n", i,
+			handler_map[i].disknum, handler_map[i].mapto);
+	}
+      /* Signal end-of-map.  */
+      handler_map[i].disknum = 0;
+      handler_map[i].mapto = 0;
+      grub_dprintf (MODNAME, "\t#%d: 0x00 <- 0x00 (end)\n", i);
+
+      /* Install our function as the int13h handler in the IVT.  */
+      ivtentry = ((grub_uint32_t) handler_base) << 12;	/* Segment address.  */
+      ivtentry |=
+	(grub_uint16_t) INT13H_OFFSET (&grub_drivemap_int13_handler);
+      grub_dprintf (MODNAME, "New int13 handler IVT pointer: %04x:%04x\n",
+		    (ivtentry >> 16) & 0x0ffff, ivtentry & 0x0ffff);
+      *ivtslot = ivtentry;
+
+      return GRUB_ERR_NONE;
+    }
+}
+
+static grub_err_t
+uninstall_int13_handler (void)
+{
+  grub_uint32_t *ivtslot = UINT_TO_PTR (0x0000004c);
+
+  if (0 != grub_drivemap_int13_oldhandler)
+    return GRUB_ERR_NONE;
+
+  *ivtslot = grub_drivemap_int13_oldhandler;
+  grub_mmap_free_and_unregister (handlermem_hnd);
+  grub_drivemap_int13_oldhandler = 0;
+
+  return GRUB_ERR_NONE;
+}
+
+GRUB_MOD_INIT (drivemap)
+{
+  cmd_reghandle = grub_register_extcmd (MODNAME, grub_cmd_drivemap,
+					GRUB_COMMAND_FLAG_BOTH,
+					MODNAME
+					" -l | -r | [-s] grubdev biosdisk",
+					"Manage the BIOS drive mappings",
+					options);
+  insthandler_hook =
+    grub_loader_register_preboot_hook (&install_int13_handler,
+				       &uninstall_int13_handler,
+				       GRUB_LOADER_PREBOOT_HOOK_PRIO_NORMAL);
+}
+
+GRUB_MOD_FINI (drivemap)
+{
+  grub_loader_unregister_preboot_hook (insthandler_hook);
+  insthandler_hook = 0;
+  grub_unregister_extcmd (cmd_reghandle);
+}
Index: commands/i386/pc/drivemap_int13h.S
===================================================================
--- commands/i386/pc/drivemap_int13h.S	(revision 0)
+++ commands/i386/pc/drivemap_int13h.S	(revision 0)
@@ -0,0 +1,85 @@
+/* drivemap_int13h.S - interrupt handler for the BIOS drive remapper */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2008, 2009  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/symbol.h>
+
+#define GRUB_DRIVEMAP_INT13H_OFFSET(x) ((x) - grub_drivemap_int13_handler_base)
+
+.code16
+
+/* Copy starts here.  When deployed, this label must be segment-aligned.  */
+VARIABLE(grub_drivemap_int13_handler_base)
+
+/* Far pointer to the old handler.  Stored as a CS:IP in the style of real-mode
+   IVT entries (thus PI:SC in mem).  */
+VARIABLE(grub_drivemap_int13_oldhandler)
+       .word 0x0, 0x0
+
+/* Actual int13 handler.  We use relative addressing with CS in order to be as
+   unintrusive as possible with registers and the stack.  */
+FUNCTION(grub_drivemap_int13_handler)
+       push    %bp
+       mov     %sp, %bp
+
+       /* Map the drive number (always in DL?).  */
+       push    %ax
+       push    %bx
+       push    %si
+       mov     $GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_mapstart), %bx
+       xor     %si, %si
+
+more_remaining:
+       movw    %cs:(%bx,%si), %ax
+       cmp     %ah, %al
+       jz      not_found /* DRV=DST => map end - drive not remapped, keep DL.  */
+       cmp     %dl, %al
+       jz      found /* Found - drive remapped, modify DL.  */
+       add     $2, %si
+       jmp     more_remaining /* Not found, but more remaining, loop.  */
+
+found:
+       mov     %ah, %dl
+
+not_found:
+       pop     %si
+       pop     %bx
+       pop     %ax
+
+       push    %bp
+       /* Simulate interrupt call: push flags and do a far call in order to set
+          the stack the way the old handler expects it so that its iret works.  */
+       push    6(%bp)
+       movw    (%bp), %bp  /* Restore the caller BP (is this needed and/or sensible?).  */
+       lcall   *%cs:GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_oldhandler)
+       pop     %bp /* The pushed flags were removed by iret.  */
+       /* Set the saved flags to what the int13h handler returned.  */
+       push    %ax
+       pushf
+       pop     %ax
+       movw    %ax, 6(%bp)
+       pop     %ax
+       
+       mov     %bp, %sp
+       pop     %bp
+       iret
+
+/* This label MUST be at the end of the copied block, since the installer code
+   reserves additional space for mappings at runtime and copies them over it.  */
+.align 2
+VARIABLE(grub_drivemap_int13_mapstart)

[-- Attachment #2: Esto es una parte de mensaje firmado digitalmente --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: status grub2 port of grub-legasy map command

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 4620 bytes --]

+/* Realmode far ptr (2 * 16b) to the previous INT13h handler.  */
+extern grub_uint32_t grub_drivemap_int13_oldhandler;
I prefer it to be 2 variables because of the way they interract so nobody
would do something like (char *) grub_drivemap_int13_oldhandler;
+/* The type "void" is used for imported assembly labels, takes no storage
and
+   serves just to take the address with &label.  Do NOT put void* here.  */

+/* Start of the handler bundle.  */
+extern void grub_drivemap_int13_handler_base;
The common practice is to use declarations like
extern char grub_drivemap_int13_handler_base[];
it makes pointer arithmetics easy
+typedef struct drivemap_node
+{
+  grub_uint8_t newdrive;
+  grub_uint8_t redirto;
+  struct drivemap_node *next;
+} drivemap_node_t;
Here you could reuse Bean's list functions
+      if (!mapping)
Put a space after !. I know GCS can be PITA but grub has chosen to follow
it. Not my decision
+      else            /* Entry was head of list.  */
+    map_head = mapping->next;
put the comment on a separate line
+  if (!name || *name == 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name empty");
This check is unnecessary since your function is static and calling function
already ensured that name isn't empty
+  /* Skip the first ( in (hd0) - disk_open wants just the name.  */
+  if (*name == '(')
+    name++;
AFAIK you need to remove ')' as well
It's not necessary to try to open the disk actually because for some reason
biosdisk module may be not loaded (e.g. grub may use ata) and loading it may
cause nasty effects (e.g. conflict between ata and biosdisk)
Same applies for revparse_biosdisk. And if user wants to map to unexistant
disk it's ok. So just use tryparse_diskstring
+  if ((str[0] == 'f' || str[0] == 'h') && str[1] == 'd')
change this to something like
if (! ((str[0] == 'f' || str[0] == 'h') && str[1] == 'd'))
   return ...
It makes code nicer by avoiding unnecessary long else
+      grub_errno = GRUB_ERR_NONE;
No need to set grub_errno explicitely unless you want to ignore an error
+      unsigned long drivenum = grub_strtoul (str + 2, 0, 0);
+      if (grub_errno != GRUB_ERR_NONE || drivenum > 127)
I think it's enough to just check the range
+    {
+      /* N not a number or out of range.  */
+      goto fail;
Since at fail you have just a return, just put a return here with
informative error message
+    }
+      else
else is unnecessary, just continue with the code
+  if (cmd->state[OPTIDX_LIST].set || 0 == argc)
argc == 0 sounds better. Also moving list function to a separate function is
a good idea
+        return grub_error (err, "invalid mapping: non-existent disk"
+                   "or not managed by the BIOS");
If you change error message change error number too. Use either
return err;
or
return grub_error (GRUB_ERR_..., message);
+      map_head = 0;
+    }
+  else
Use return rather then else
+      return GRUB_ERR_NONE;
+    }
+  else
no need for "else"
+      for (i = 0; i < entries && curentry; ++i, curentry = curentry->next)
You don't need to test for both conditions since theirequivalencyis ensured
by preceding code
+  if (0 != grub_drivemap_int13_oldhandler)
Better swap the parts
+                    MODNAME
+                    " -l | -r | [-s] grubdev biosdisk",
Here MODNAME doesn't save any space and makes code less readable. Just write
the command name explicitely
+       push    %bp
+       mov     %sp, %bp
You don't need to change bp. Just use sp. In this case it makes code easier
+       lcall
*%cs:GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_oldhandler)
+       pop     %bp /* The pushed flags were removed by iret.  */
Use rather:
pushf
lcall   *%cs:GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_oldhandler)
Also remember to restore original %dl after the call
I'm looking forward for your improved patch
Thank you for your effort
Could you also prepare a changelog entry for it?
2009/5/6 Javier Martín <lordhabbit@gmail.com>

> Here is a new version of the patch. As you suggested, "grub_symbol_t"
> was replaced with "void". Also, drivemap.h no longer exists, its little
> content integrated into drivemap.c. Last but not least, I've mostly
> adopted your version of the assembly file (indenting, labels, etc.)
>
> --
> -- Lazy, Oblivious, Recurrent Disaster -- Habbit
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>


-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #2: Type: text/html, Size: 5551 bytes --]

Re: status grub2 port of grub-legasy map command

[Javier Martín]

[-- Attachment #1.1: Type: text/plain, Size: 10112 bytes --]

El sáb, 09-05-2009 a las 11:17 +0200, Vladimir 'phcoder' Serbinenko
escribió:
> +/* Realmode far ptr (2 * 16b) to the previous INT13h handler.  */ 
> +extern grub_uint32_t grub_drivemap_int13_oldhandler;
> I prefer it to be 2 variables because of the way they interract so
> nobody would do something like (char *)
> grub_drivemap_int13_oldhandler;
Two variables? I don't understand. You want the CS and IP parts of the
far pointer separated? Why would anyone try to use it like you said? The
comment explicits that it is a realmode pointer, and as such it is not
usable as a linear pmode pointer.

> +/* The type "void" is used for imported assembly labels, takes no
> storage and 
> +   serves just to take the address with &label.  Do NOT put void*
> here.  */ 
> +/* Start of the handler bundle.  */ 
> +extern void grub_drivemap_int13_handler_base;
> The common practice is to use declarations like
> extern char grub_drivemap_int13_handler_base[];
> it makes pointer arithmetics easy
That variable is used only twice in the code: one in a call to memcpy
and another one inside the macro INT13_OFFSET. It would still be so even
if it were changed to a char array, with the added risk that a char
array can be modified with almost nothing to gain (as casts would still
be needed). In fact, I'm declaring all the labels as "const void" so
no-one tries to overwrite the "master" data instead of the deployed
data.

> +typedef struct drivemap_node 
> +{ 
> +  grub_uint8_t newdrive; 
> +  grub_uint8_t redirto; 
> +  struct drivemap_node *next; 
> +} drivemap_node_t; 
> Here you could reuse Bean's list functions
After examining the API, I think such a change would be too invasive for
a "mature" patch right now. However, once the patch is in and drivemap
is working for everyone, I can work on modifying it to use
<grub/list.h>. The biggest hurdle I see is that there is no way to
automatically maintain "key" uniqueness like the current methods do, so
an iteration over the list would be required.

As a side note/rant, <grub/list.h> desperately needs documentation
comments for other developers to be able to actually use it without
wondering if they're going into the darkness. In particular I need to
know what grub_list_insert does without delving into "list.c" (for
example: "if all tests fail, will the item be inserted last or not at
all?"). This rant also applies to other grub include files.

> +      if (!mapping)
> Put a space after !. I know GCS can be PITA but grub has chosen to
> follow it. Not my decision
Oops... I see it has happened in every (! something) test in the file.
I've checked and definitely `indent' did it... I thought that its
default invocation was supposed to adhere to the GCS by default (I'm
using GNU indent 2.2.10).

> +      else            /* Entry was head of list.  */ 
> +    map_head = mapping->next; 
> put the comment on a separate line 
I removed the comment.

> +  if (!name || *name == 0) 
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name empty");
> This check is unnecessary since your function is static and calling
> function already ensured that name isn't empty
I've checked, and actually the main function only ensures that the
pointer is valid, not that it is nonempty (try for example `drivemap
"" (hd1)'). However, trying to grub_disk_open the empty string will fail
and the error will be caught anyway, so the check has been removed.

> +  /* Skip the first ( in (hd0) - disk_open wants just the name.  */ 
> +  if (*name == '(') 
> +    name++;
> AFAIK you need to remove ')' as well
No you don't, it seems that grub_disk_open chokes on the GRUB device
name opening paren but ignores the closing paren kinda like strtoul
stops parsing on the first non-digit. As there is no API documentation I
can't tell whether this is a precondition checking glitch or an intended
effect, but it sure makes the caller logic way simpler.

> It's not necessary to try to open the disk actually because for some
> reason biosdisk module may be not loaded (e.g. grub may use ata) and
> loading it may cause nasty effects (e.g. conflict between ata and
> biosdisk)
> Same applies for revparse_biosdisk. And if user wants to map to
> unexistant disk it's ok. So just use tryparse_diskstring
Hmm... this is a profound design issue: you are arguing that drivemap
should not care about actual BIOS disks, but just their drive numbers,
and let you map 0x86 to 0x80 even if you have no (hd6). I do not agree,
since the main use for GRUB is a bootloader, not a test platform (which
would be the only reason to perform that kind of mappings which are sure
to cause havoc). Thus, by default drivemap only lets you perform
mappings that will work - any stress tester is free to modify the code.

Regarding opening the disk, the biosdisk interface does not assure in
any way (and again there is almost no API documentation) that hd0 will
be 0x80, though it's the sensible thing to expect. However, expecting
sensible things from an unstable API prone to redesigns (like the
command API, for example) usually leads to nasty surprises later on, so
unless 1) the biosdisk interface so specifies and 2) hdN will _always_
refer to biosdisk and not another possible handler, I think the check is
a Good Thing (tm).

> +  if ((str[0] == 'f' || str[0] == 'h') && str[1] == 'd') 
> change this to something like
> if (! ((str[0] == 'f' || str[0] == 'h') && str[1] == 'd'))
>    return ...
> It makes code nicer by avoiding unnecessary long else
I think the "else" you were talking about was one line long, but it was
not needed anyways and so I've removed it.

> +      grub_errno = GRUB_ERR_NONE; 
> No need to set grub_errno explicitely unless you want to ignore an
> error
I may have got it wrong, but I think that functions like strtoul only
_set_ errno if there _has_ been an error, but leave it unchanged if
there hasn't. As I want to check if the conversion failed, I need to
have the variable to check in a known pre-state.

> +      unsigned long drivenum = grub_strtoul (str + 2, 0, 0); 
> +      if (grub_errno != GRUB_ERR_NONE || drivenum > 127) 
> I think it's enough to just check the range
See above.

> +    { 
> +      /* N not a number or out of range.  */ 
> +      goto fail; 
> Since at fail you have just a return, just put a return here with
> informative error message
The function was rearranged so only the first goto is required now.

> +    } 
> +      else 
> else is unnecessary, just continue with the code
I fear you don't give me enough context to find this one. If it was
inside tryparse_diskstring, it has been removed.

> +  if (cmd->state[OPTIDX_LIST].set || 0 == argc) 
> argc == 0 sounds better. Also moving list function to a separate
> function is a good idea
Ok, changed. I'd just like to remark that if someone slipped and changed
the line to "argc = 0" it would pass unnoticed by the compiler, while "0
= argc" would create a compile-time error. Regarding slicing the listing
functionality, I don't see the advantages.

> +        return grub_error (err, "invalid mapping: non-existent disk" 
> +                   "or not managed by the BIOS");
> If you change error message change error number too. Use either
> return err;
> or 
> return grub_error (GRUB_ERR_..., message);
Changed to "return err;"

> +      map_head = 0; 
> +    } 
> +  else
> Use return rather then else
> +      return GRUB_ERR_NONE; 
> +    } 
> +  else 
> no need for "else"
Function reworked so as to add "return"s and remove "else"s.

> +      for (i = 0; i < entries && curentry; ++i, curentry =
> curentry->next)
> You don't need to test for both conditions since theirequivalencyis
> ensured by preceding code
True. Changed.

> +  if (0 != grub_drivemap_int13_oldhandler)
> Better swap the parts
Changed. See my reasoning above, though.

> +                    MODNAME 
> +                    " -l | -r | [-s] grubdev biosdisk", 
> Here MODNAME doesn't save any space and makes code less readable. Just
> write the command name explicitely 
> +       push    %bp 
> +       mov     %sp, %bp 
> You don't need to change bp. Just use sp. In this case it makes code
> easier
That would make it more difficult to address passed parameters like the
flags at [BP+6] because their location would depend on the current
status of SP (which might change depending on code above the relevant
lines). I think that trying to shave off those 4 or so bytes would make
the code unnecessarily more complicated, which is usually a no-no when
assembly is involved. Besides, this prolog sequence and the
corresponding epilog before returning is practically a standard in x86.

> +       lcall   *%
> cs:GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_oldhandler) 
> +       pop     %bp /* The pushed flags were removed by iret.  */ 
> Use rather:
> pushf
> lcall   *%
> cs:GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_oldhandler) 
Why would we want to save our current flags? We want the old int13
handler to receive the flags it would have received if the drivemap
int13 handler were not present, that is, the original caller flags which
were stored above CS:IP in the stack when the "int $0x13" call was
issued. The comment means that "iret" pops not just CS:IP but also the
flags, so we can proceed directly to popping %bp.

> Also remember to restore original %dl after the call
I think there is no need to do so, because BIOS calls modify %dl anyway.

> I'm looking forward for your improved patch
Here it goes.

> Thank you for your effort
Thank _you_ for reviewing it, so many things slip past me (including the
convoluted GCS ._.)

> Could you also prepare a changelog entry for it?
Hmm... I don't really know how to write it, given that both files are
new and the patch does not modify any other GRUB function. Am I supposed
to also declare the modifications in the rmk files?

-- 
-- Lazy, Oblivious, Recurrent Disaster -- Habbit

[-- Attachment #1.2: drivemap.11.patch --]
[-- Type: text/x-patch, Size: 19709 bytes --]

Index: conf/i386-pc.rmk
===================================================================
--- conf/i386-pc.rmk	(revision 2201)
+++ conf/i386-pc.rmk	(working copy)
@@ -185,8 +185,15 @@
 	aout.mod bsd.mod pxe.mod pxecmd.mod datetime.mod date.mod \
 	datehook.mod lsmmap.mod ata_pthru.mod hdparm.mod \
 	usb.mod uhci.mod ohci.mod usbtest.mod usbms.mod usb_keyboard.mod \
-	efiemu.mod mmap.mod acpi.mod
+	efiemu.mod mmap.mod acpi.mod drivemap.mod
 
+# For drivemap.mod.
+drivemap_mod_SOURCES = commands/i386/pc/drivemap.c \
+                       commands/i386/pc/drivemap_int13h.S
+drivemap_mod_ASFLAGS = $(COMMON_ASFLAGS)
+drivemap_mod_CFLAGS = $(COMMON_CFLAGS)
+drivemap_mod_LDFLAGS = $(COMMON_LDFLAGS)
+
 # For efiemu.mod.
 efiemu_mod_SOURCES = efiemu/main.c efiemu/i386/loadcore32.c \
 		     efiemu/i386/loadcore64.c efiemu/i386/pc/cfgtables.c \
Index: commands/i386/pc/drivemap.c
===================================================================
--- commands/i386/pc/drivemap.c	(revision 0)
+++ commands/i386/pc/drivemap.c	(revision 0)
@@ -0,0 +1,447 @@
+/* drivemap.c - command to manage the BIOS drive mappings.  */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2008, 2009  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/extcmd.h>
+#include <grub/dl.h>
+#include <grub/mm.h>
+#include <grub/misc.h>
+#include <grub/disk.h>
+#include <grub/machine/biosdisk.h>
+#include <grub/loader.h>
+#include <grub/machine/memory.h>
+
+
+#define MODNAME "drivemap"
+
+static grub_extcmd_t cmd_reghandle;
+
+/* Remember to update enum opt_idxs accordingly.  */
+static const struct grub_arg_option options[] = {
+  {"list", 'l', 0, "show the current mappings", 0, 0},
+  {"reset", 'r', 0, "reset all mappings to the default values", 0, 0},
+  {"swap", 's', 0, "perform both direct and reverse mappings", 0, 0},
+  {0, 0, 0, 0, 0, 0}
+};
+
+/* Remember to update options[] accordingly.  */
+enum opt_idxs
+{
+  OPTIDX_LIST = 0,
+  OPTIDX_RESET,
+  OPTIDX_SWAP,
+};
+
+/* Realmode far ptr (2 * 16b) to the previous INT13h handler.  */
+extern grub_uint32_t grub_drivemap_int13_oldhandler;
+
+/* The type "void" is used for imported assembly labels, takes no storage and
+   serves just to take the address with &label.  Do NOT put void* here.  */
+/* Start of the handler bundle.  */
+extern const void grub_drivemap_int13_handler_base;
+/* Start of the drive mappings area (space reserved at runtime).  */
+extern const void grub_drivemap_int13_mapstart;
+/* The assembly function to replace the old INT13h handler. It should not be
+   called because it does not follow any C callspecs and returns with IRET.  */
+extern const void grub_drivemap_int13_handler;
+
+typedef struct drivemap_node
+{
+  struct drivemap_node *next;
+  grub_uint8_t newdrive;
+  grub_uint8_t redirto;
+} drivemap_node_t;
+
+static drivemap_node_t *map_head;
+static void *insthandler_hook;
+static int handlermem_hnd;
+static grub_err_t install_int13_handler (int noret __attribute__ ((unused)));
+static grub_err_t uninstall_int13_handler (void);
+
+/* Puts the specified mapping into the table, replacing an existing mapping
+   for newdrive or adding a new one if required.  */
+static grub_err_t
+drivemap_set (grub_uint8_t newdrive, grub_uint8_t redirto)
+{
+  drivemap_node_t *mapping = 0;
+  drivemap_node_t *search = map_head;
+  while (search)
+    {
+      if (search->newdrive == newdrive)
+	{
+	  mapping = search;
+	  break;
+	}
+      search = search->next;
+    }
+
+  /* Check for pre-existing mappings to modify before creating a new one.  */
+  if (mapping)
+    mapping->redirto = redirto;
+  else
+    {
+      mapping = grub_malloc (sizeof (drivemap_node_t));
+      if (! mapping)
+	return grub_error (GRUB_ERR_OUT_OF_MEMORY,
+			   "cannot allocate map entry, not enough memory");
+      mapping->newdrive = newdrive;
+      mapping->redirto = redirto;
+      mapping->next = map_head;
+      map_head = mapping;
+    }
+  return GRUB_ERR_NONE;
+}
+
+/* Removes the mapping for newdrive from the table.  If there is no mapping,
+   then this function behaves like a no-op on the map.  */
+static void
+drivemap_remove (grub_uint8_t newdrive)
+{
+  drivemap_node_t *mapping = 0;
+  drivemap_node_t *search = map_head;
+  drivemap_node_t *previous = 0;
+
+  while (search)
+    {
+      if (search->newdrive == newdrive)
+	{
+	  mapping = search;
+	  break;
+	}
+      previous = search;
+      search = search->next;
+    }
+
+  if (mapping)
+    {
+      if (previous)
+	previous->next = mapping->next;
+      else
+	map_head = mapping->next;
+      grub_free (mapping);
+    }
+}
+
+/* Given a device name, resolves its BIOS disk number and stores it in the
+   passed location, which should only be trusted if ERR_NONE is returned.  */
+static grub_err_t
+parse_biosdisk (const char *name, grub_uint8_t * disknum)
+{
+  grub_disk_t disk;
+  /* Skip the first ( in (hd0) - disk_open wants just the name.  */
+  if (*name == '(')
+    name++;
+
+  disk = grub_disk_open (name);
+  if (! disk)
+    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "unknown device \"%s\"", name);
+  const enum grub_disk_dev_id id = disk->dev->id;
+  /* The following assignment is only sound if the device is indeed a
+     biosdisk.  The caller must check the return value.  */
+  if (disknum)
+    *disknum = disk->id;
+  grub_disk_close (disk);
+  if (id != GRUB_DISK_DEVICE_BIOSDISK_ID)
+    return grub_error (GRUB_ERR_BAD_DEVICE, "%s is not a BIOS disk", name);
+  return GRUB_ERR_NONE;
+}
+
+/* Given a BIOS disk number, returns its GRUB device name if it exists.  
+   If the call succeeds, the resulting device string must be freed.  
+   For nonexisting BIOS disk numbers, this function returns
+   GRUB_ERR_UNKNOWN_DEVICE.  */
+static grub_err_t
+revparse_biosdisk (const grub_uint8_t dnum, const char **output)
+{
+  int found = 0;
+  auto int find (const char *name);
+  int find (const char *name)
+  {
+    const grub_disk_t disk = grub_disk_open (name);
+    if (! disk)
+      return 0;
+    if (disk->id == dnum && disk->dev->id == GRUB_DISK_DEVICE_BIOSDISK_ID)
+      {
+	found = 1;
+	if (output)
+	  *output = grub_strdup (name);
+      }
+    grub_disk_close (disk);
+    return found;
+  }
+
+  grub_disk_dev_iterate (find);
+  if (found)
+    return GRUB_ERR_NONE;
+  return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "BIOS disk %02x not found", dnum);
+}
+
+/* Given a GRUB-like device name and a convenient location, stores the
+   related BIOS disk number.  Accepts devices like \((f|h)dN\), with
+   0 <= N < 128.  */
+static grub_err_t
+tryparse_diskstring (const char *str, grub_uint8_t * output)
+{
+  if (! str || *str == 0)
+    goto fail;
+  /* Skip opening paren in order to allow both (hd0) and hd0.  */
+  if (*str == '(')
+    str++;
+  if ((str[0] == 'f' || str[0] == 'h') && str[1] == 'd')
+    {
+      grub_uint8_t bios_num = (str[0] == 'h') ? 0x80 : 0x00;
+      grub_errno = GRUB_ERR_NONE;
+      unsigned long drivenum = grub_strtoul (str + 2, 0, 0);
+      if (grub_errno == GRUB_ERR_NONE && drivenum < 128)
+	{
+	  bios_num |= drivenum;
+	  if (output)
+	    *output = bios_num;
+	  return GRUB_ERR_NONE;
+	}
+    }
+fail:
+  return grub_error (GRUB_ERR_BAD_ARGUMENT, "device format \"%s\" "
+		     "invalid: must be (f|h)dN, with 0 <= N < 128", str);
+}
+
+static grub_err_t
+grub_cmd_drivemap (struct grub_extcmd *cmd, int argc, char **args)
+{
+  if (cmd->state[OPTIDX_LIST].set || argc == 0)
+    {
+      /* Show: list mappings.  */
+      if (! map_head)
+        {
+	  grub_printf ("No drives have been remapped");
+	  return GRUB_ERR_NONE;
+	}
+      grub_printf ("Showing only remapped drives.\n");
+      grub_printf ("BIOS disk #num ----> GRUB device\n");
+      drivemap_node_t *curnode = map_head;
+      while (curnode)
+	{
+	  const char *dname = 0;
+	  grub_err_t err = revparse_biosdisk (curnode->redirto, &dname);
+	  if (err != GRUB_ERR_NONE)
+	    return err;
+	  grub_printf ("%cD #%-3u (0x%02x)       %s\n",
+		       (curnode->newdrive & 0x80) ? 'H' : 'F',
+		       curnode->newdrive & 0x7F, curnode->newdrive,
+		       dname);
+	  curnode = curnode->next;
+	  grub_free ((char *) dname);
+	}
+      return GRUB_ERR_NONE;
+    }
+  else if (cmd->state[OPTIDX_RESET].set)
+    {
+      /* Reset: just delete all mappings, freeing their memory.  */
+      drivemap_node_t *curnode = map_head;
+      drivemap_node_t *prevnode = 0;
+      while (curnode)
+	{
+	  prevnode = curnode;
+	  curnode = curnode->next;
+	  grub_free (prevnode);
+	}
+      map_head = 0;
+      return GRUB_ERR_NONE;
+    }
+    
+  /* Neither flag: put mapping.  */
+  grub_uint8_t mapfrom = 0;
+  grub_uint8_t mapto = 0xFF;
+  grub_err_t err;
+
+  if (argc != 2)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "two arguments required");
+
+  err = parse_biosdisk (args[0], &mapfrom);
+  if (err != GRUB_ERR_NONE)
+    return err;
+
+  /* When swapping we require both devices to be BIOS disks, but when
+     performing direct mappings we only require the 2nd argument to look
+     like a BIOS disk in order to resolve it into a BIOS disk number.  */
+  if (cmd->state[OPTIDX_SWAP].set)
+    err = parse_biosdisk (args[1], &mapto);
+  else
+    err = tryparse_diskstring (args[1], &mapto);
+  if (err != GRUB_ERR_NONE)
+    return err;
+
+  if (mapto == mapfrom)
+    {
+      /* Reset to default.  */
+      grub_dprintf (MODNAME, "Removing the mapping for %s (%02x)",
+		    args[0], mapfrom);
+      drivemap_remove (mapfrom);
+      return GRUB_ERR_NONE;
+    }
+  /* Set the mapping for the disk (overwrites any existing mapping).  */
+  grub_dprintf (MODNAME, "%s %s (%02x) = %s (%02x)\n",
+		cmd->state[OPTIDX_SWAP].set ? "Swapping" : "Mapping",
+		args[1], mapto, args[0], mapfrom);
+  err = drivemap_set (mapto, mapfrom);
+  /* If -s, perform the reverse mapping too (only if the first was OK).  */
+  if (cmd->state[OPTIDX_SWAP].set && err == GRUB_ERR_NONE)
+    err = drivemap_set (mapfrom, mapto);
+  return err;
+}
+
+typedef struct __attribute__ ((packed)) int13map_node
+{
+  grub_uint8_t disknum;
+  grub_uint8_t mapto;
+} int13map_node_t;
+
+#define INT13H_OFFSET(x) ( ((grub_uint8_t*)(x)) - ((grub_uint8_t*)&grub_drivemap_int13_handler_base) )
+#define INT13H_REBASE(x) ( (void*) (handler_base + (x)) )
+#define INT13H_TONEWADDR(x) INT13H_REBASE( INT13H_OFFSET( x ) )
+
+/* Int13h handler installer - reserves conventional memory for the handler,
+   copies it over and sets the IVT entry for int13h.  
+   This code rests on the assumption that GRUB does not activate any kind
+   of memory mapping apart from identity paging, since it accesses
+   realmode structures by their absolute addresses, like the IVT at 0;
+   and transforms a pmode pointer into a rmode seg:off far ptr.  */
+static grub_err_t
+install_int13_handler (int noret __attribute__ ((unused)))
+{
+  grub_size_t entries = 0;
+  drivemap_node_t *curentry = map_head;
+
+  /* Count entries to prepare a contiguous map block.  */
+  while (curentry)
+    {
+      entries++;
+      curentry = curentry->next;
+    }
+  if (entries == 0)
+    {
+      /* No need to install the int13h handler.  */
+      grub_dprintf (MODNAME, "No drives marked as remapped, installation "
+		    "of an int13h handler is not required.");
+      return GRUB_ERR_NONE;
+    }
+  else
+    {
+      /* Real mode IVT slot (seg:off far pointer) for interrupt 0x13.  */
+      grub_uint32_t *ivtslot = UINT_TO_PTR (0x0000004c);
+      /* Size of the full int13 handler "bundle", including code and map.  */
+      grub_uint64_t total_size;
+      /* Base address of the space reserved for the handler bundle.  */
+      grub_uint8_t *handler_base = 0;
+      /* Address of the map within the deployed bundle.  */
+      int13map_node_t *handler_map;
+      /* Real mode IVT entry (seg:off far pointer) for the new handler.  */
+      grub_uint32_t ivtentry;
+
+      grub_dprintf (MODNAME, "Installing int13h handler...\n");
+
+      /* Save the pointer to the old handler.  */
+      grub_drivemap_int13_oldhandler = *ivtslot;
+      grub_dprintf (MODNAME, "Old int13 handler at %04x:%04x\n",
+		    (grub_drivemap_int13_oldhandler >> 16) & 0x0ffff,
+		    grub_drivemap_int13_oldhandler & 0x0ffff);
+
+      /* Find a rmode-segment-aligned zone in conventional memory big
+         enough to hold the handler and its data.  */
+      total_size = INT13H_OFFSET(&grub_drivemap_int13_mapstart)
+	+ (entries + 1) * sizeof (int13map_node_t);
+      grub_dprintf (MODNAME, "Payload is %llu bytes long\n", total_size);
+      handler_base = grub_mmap_malign_and_register (16, total_size,
+						    &handlermem_hnd,
+						    GRUB_MACHINE_MEMORY_RESERVED,
+						    GRUB_MMAP_MALLOC_LOW);
+      if (! handler_base)
+	return grub_error (GRUB_ERR_OUT_OF_MEMORY, "Could not reserve "
+			   "memory for the int13h handler");
+
+      /* Copy int13h handler bundle to reserved area.  */
+      grub_dprintf (MODNAME, "Reserved memory at %p, copying handler...\n",
+		    handler_base);
+      grub_memcpy (handler_base, &grub_drivemap_int13_handler_base,
+		   INT13H_OFFSET (&grub_drivemap_int13_mapstart));
+
+      /* Copy the mappings to the reserved area.  */
+      curentry = map_head;
+      grub_size_t i;
+      handler_map = INT13H_TONEWADDR (&grub_drivemap_int13_mapstart);
+      grub_dprintf (MODNAME, "Target map at %p, copying mappings...\n",
+		    handler_map);
+      for (i = 0; i < entries; ++i, curentry = curentry->next)
+	{
+	  handler_map[i].disknum = curentry->newdrive;
+	  handler_map[i].mapto = curentry->redirto;
+	  grub_dprintf (MODNAME, "\t#%d: 0x%02x <- 0x%02x\n", i,
+			handler_map[i].disknum, handler_map[i].mapto);
+	}
+      /* Signal end-of-map.  */
+      handler_map[i].disknum = 0;
+      handler_map[i].mapto = 0;
+      grub_dprintf (MODNAME, "\t#%d: 0x00 <- 0x00 (end)\n", i);
+
+      /* Install our function as the int13h handler in the IVT.  */
+      ivtentry = ((grub_uint32_t) handler_base) << 12;	/* Segment address.  */
+      ivtentry |=
+	(grub_uint16_t) INT13H_OFFSET (&grub_drivemap_int13_handler);
+      grub_dprintf (MODNAME, "New int13 handler IVT pointer: %04x:%04x\n",
+		    (ivtentry >> 16) & 0x0ffff, ivtentry & 0x0ffff);
+      *ivtslot = ivtentry;
+
+      return GRUB_ERR_NONE;
+    }
+}
+
+static grub_err_t
+uninstall_int13_handler (void)
+{
+  grub_uint32_t *ivtslot = UINT_TO_PTR (0x0000004c);
+
+  if (grub_drivemap_int13_oldhandler != 0)
+    return GRUB_ERR_NONE;
+
+  *ivtslot = grub_drivemap_int13_oldhandler;
+  grub_mmap_free_and_unregister (handlermem_hnd);
+  grub_drivemap_int13_oldhandler = 0;
+
+  return GRUB_ERR_NONE;
+}
+
+GRUB_MOD_INIT (drivemap)
+{
+  cmd_reghandle = grub_register_extcmd (MODNAME, grub_cmd_drivemap,
+					GRUB_COMMAND_FLAG_BOTH,
+					MODNAME
+					" -l | -r | [-s] grubdev biosdisk",
+					"Manage the BIOS drive mappings",
+					options);
+  insthandler_hook =
+    grub_loader_register_preboot_hook (&install_int13_handler,
+				       &uninstall_int13_handler,
+				       GRUB_LOADER_PREBOOT_HOOK_PRIO_NORMAL);
+}
+
+GRUB_MOD_FINI (drivemap)
+{
+  grub_loader_unregister_preboot_hook (insthandler_hook);
+  insthandler_hook = 0;
+  grub_unregister_extcmd (cmd_reghandle);
+}
Index: commands/i386/pc/drivemap_int13h.S
===================================================================
--- commands/i386/pc/drivemap_int13h.S	(revision 0)
+++ commands/i386/pc/drivemap_int13h.S	(revision 0)
@@ -0,0 +1,85 @@
+/* drivemap_int13h.S - interrupt handler for the BIOS drive remapper */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2008, 2009  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/symbol.h>
+
+#define GRUB_DRIVEMAP_INT13H_OFFSET(x) ((x) - grub_drivemap_int13_handler_base)
+
+.code16
+
+/* Copy starts here.  When deployed, this label must be segment-aligned.  */
+VARIABLE(grub_drivemap_int13_handler_base)
+
+/* Far pointer to the old handler.  Stored as a CS:IP in the style of real-mode
+   IVT entries (thus PI:SC in mem).  */
+VARIABLE(grub_drivemap_int13_oldhandler)
+       .word 0x0, 0x0
+
+/* Actual int13 handler.  We use relative addressing with CS in order to be as
+   unintrusive as possible with registers and the stack.  */
+FUNCTION(grub_drivemap_int13_handler)
+       push    %bp
+       mov     %sp, %bp
+
+       /* Map the drive number (always in DL?).  */
+       push    %ax
+       push    %bx
+       push    %si
+       mov     $GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_mapstart), %bx
+       xor     %si, %si
+
+more_remaining:
+       movw    %cs:(%bx,%si), %ax
+       cmp     %ah, %al
+       jz      not_found /* DRV=DST => map end - drive not remapped, keep DL.  */
+       cmp     %dl, %al
+       jz      found /* Found - drive remapped, modify DL.  */
+       add     $2, %si
+       jmp     more_remaining /* Not found, but more remaining, loop.  */
+
+found:
+       mov     %ah, %dl
+
+not_found:
+       pop     %si
+       pop     %bx
+       pop     %ax
+
+       push    %bp
+       /* Simulate interrupt call: push flags and do a far call in order to set
+          the stack the way the old handler expects it so that its iret works.  */
+       push    6(%bp)
+       movw    (%bp), %bp  /* Restore the caller BP (is this needed and/or sensible?).  */
+       lcall   *%cs:GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_oldhandler)
+       pop     %bp /* The pushed flags were removed by iret.  */
+       /* Set the saved flags to what the int13h handler returned.  */
+       push    %ax
+       pushf
+       pop     %ax
+       movw    %ax, 6(%bp)
+       pop     %ax
+       
+       mov     %bp, %sp
+       pop     %bp
+       iret
+
+/* This label MUST be at the end of the copied block, since the installer code
+   reserves additional space for mappings at runtime and copies them over it.  */
+.align 2
+VARIABLE(grub_drivemap_int13_mapstart)

[-- Attachment #2: Esto es una parte de mensaje firmado digitalmente --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: status grub2 port of grub-legasy map command

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 9439 bytes --]

Hello

2009/5/9 Javier Martín <lordhabbit@gmail.com>

> El sáb, 09-05-2009 a las 11:17 +0200, Vladimir 'phcoder' Serbinenko
> escribió:
> > +/* Realmode far ptr (2 * 16b) to the previous INT13h handler.  */
> > +extern grub_uint32_t grub_drivemap_int13_oldhandler;
> > I prefer it to be 2 variables because of the way they interract so
> > nobody would do something like (char *)
> > grub_drivemap_int13_oldhandler;
> Two variables? I don't understand. You want the CS and IP parts of the
> far pointer separated?

Yes. Especially that in .S you declared it as two words. But it's not an
absolute must

> Why would anyone try to use it like you said? The
> comment explicits that it is a realmode pointer, and as such it is not
> usable as a linear pmode pointer.
>
> > +/* The type "void" is used for imported assembly labels, takes no
> > storage and
> > +   serves just to take the address with &label.  Do NOT put void*
> > here.  */
> > +/* Start of the handler bundle.  */
> > +extern void grub_drivemap_int13_handler_base;
> > The common practice is to use declarations like
> > extern char grub_drivemap_int13_handler_base[];
> > it makes pointer arithmetics easy
> That variable is used only twice in the code: one in a call to memcpy
> and another one inside the macro INT13_OFFSET. It would still be so even
> if it were changed to a char array, with the added risk that a char
> array can be modified with almost nothing to gain (as casts would still
> be needed).

The casts are needed if you declare it as char foo[];
If someone modifies an array he does it on purpose

> In fact, I'm declaring all the labels as "const void" so
> no-one tries to overwrite the "master" data instead of the deployed
> data.
>
> > +typedef struct drivemap_node
> > +{
> > +  grub_uint8_t newdrive;
> > +  grub_uint8_t redirto;
> > +  struct drivemap_node *next;
> > +} drivemap_node_t;
> > Here you could reuse Bean's list functions
> After examining the API, I think such a change would be too invasive for
> a "mature" patch right now. However, once the patch is in and drivemap
> is working for everyone, I can work on modifying it to use
> <grub/list.h>. The biggest hurdle I see is that there is no way to
> automatically maintain "key" uniqueness like the current methods do, so
> an iteration over the list would be required.
>
> As a side note/rant, <grub/list.h> desperately needs documentation
> comments for other developers to be able to actually use it without
> wondering if they're going into the darkness. In particular I need to
> know what grub_list_insert does without delving into "list.c" (for
> example: "if all tests fail, will the item be inserted last or not at
> all?"). This rant also applies to other grub include files.
>
>
> > It's not necessary to try to open the disk actually because for some
> > reason biosdisk module may be not loaded (e.g. grub may use ata) and
> > loading it may cause nasty effects (e.g. conflict between ata and
> > biosdisk)
> > Same applies for revparse_biosdisk. And if user wants to map to
> > unexistant disk it's ok. So just use tryparse_diskstring
> Hmm... this is a profound design issue: you are arguing that drivemap
> should not care about actual BIOS disks, but just their drive numbers,
> and let you map 0x86 to 0x80 even if you have no (hd6). I do not agree,
> since the main use for GRUB is a bootloader, not a test platform (which
> would be the only reason to perform that kind of mappings which are sure
> to cause havoc). Thus, by default drivemap only lets you perform
> mappings that will work - any stress tester is free to modify the code.
>


> Regarding opening the disk, the biosdisk interface does not assure in
> any way (and again there is almost no API documentation) that hd0 will
> be 0x80, though it's the sensible thing to expect. However, expecting
> sensible things from an unstable API prone to redesigns (like the
> command API, for example)

Mapping something to non-existant drive makes the drive non-existant. I
think it's fair
It's almost guaranteed that numbering of disks won't change


> usually leads to nasty surprises later on, so
> unless 1) the biosdisk interface so specifies and 2) hdN will _always_
> refer to biosdisk and not another possible handler, I think the check is
> a Good Thing (tm).

It's not if it may hang when you use ata.mod

>
>
> > +  if ((str[0] == 'f' || str[0] == 'h') && str[1] == 'd')
> > change this to something like
> > if (! ((str[0] == 'f' || str[0] == 'h') && str[1] == 'd'))
> >    return ...
> > It makes code nicer by avoiding unnecessary long else
> I think the "else" you were talking about was one line long, but it was
> not needed anyways and so I've removed it.
>


> > +      grub_errno = GRUB_ERR_NONE;
> > No need to set grub_errno explicitely unless you want to ignore an
> > error
> I may have got it wrong, but I think that functions like strtoul only
> _set_ errno if there _has_ been an error, but leave it unchanged if
> there hasn't. As I want to check if the conversion failed, I need to
> have the variable to check in a known pre-state.
>
If grub_errno is set at the begining of the function then either scripting
or your code has a bug

>
> > +      unsigned long drivenum = grub_strtoul (str + 2, 0, 0);
> > +      if (grub_errno != GRUB_ERR_NONE || drivenum > 127)
> > I think it's enough to just check the range
> See above.
>
> > +    {
> > +      /* N not a number or out of range.  */
> > +      goto fail;
> > Since at fail you have just a return, just put a return here with
> > informative error message
> The function was rearranged so only the first goto is required now.
>
+fail:
+  return grub_error (GRUB_ERR_BAD_ARGUMENT, "device format \"%s\" "
+             "invalid: must be (f|h)dN, with 0 <= N < 128", str);
I don't see a clear benefit of using goto here. Perhaps providing a separate
informative messages for 2 fail cases is a better option


>
> > +    }
> > +      else
> > else is unnecessary, just continue with the code
> I fear you don't give me enough context to find this one. If it was
> inside tryparse_diskstring, it has been removed.
>
> > +  if (cmd->state[OPTIDX_LIST].set || 0 == argc)
> > argc == 0 sounds better. Also moving list function to a separate
> > function is a good idea
> Ok, changed. I'd just like to remark that if someone slipped and changed
> the line to "argc = 0" it would pass unnoticed by the compiler, while "0
> = argc" would create a compile-time error.

It's what warnings are for

> Regarding slicing the listing
> functionality, I don't see the advantages.
>
It makes code more readable and avoids long else clause

> > +       push    %bp
> > +       mov     %sp, %bp
> > You don't need to change bp. Just use sp. In this case it makes code
> > easier
> That would make it more difficult to address passed parameters like the
> flags at [BP+6] because their location would depend on the current
> status of SP (which might change depending on code above the relevant
> lines).

I checked and can say that removing bp code makes the rest actually easier.
If you don't see how tell me I'll modify it

> I think that trying to shave off those 4 or so bytes would make
> the code unnecessarily more complicated, which is usually a no-no when
> assembly is involved. Besides, this prolog sequence and the
> corresponding epilog before returning is practically a standard in x86.
>
> > +       lcall   *%
> > cs:GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_oldhandler)
> > +       pop     %bp /* The pushed flags were removed by iret.  */
> > Use rather:
> > pushf
> > lcall   *%
> > cs:GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_oldhandler)
> Why would we want to save our current flags? We want the old int13
> handler to receive the flags it would have received if the drivemap
> int13 handler were not present, that is, the original caller flags which
> were stored above CS:IP in the stack when the "int $0x13" call was
> issued. The comment means that "iret" pops not just CS:IP but also the
> flags, so we can proceed directly to popping %bp.
>

> > Also remember to restore original %dl after the call
> I think there is no need to do so, because BIOS calls modify %dl anyway.
>
Then you can remove %bp and just make a long jump. This way int 13 handler
recieves the original stack intact

>
>
> > Could you also prepare a changelog entry for it?
> Hmm... I don't really know how to write it, given that both files are
> new and the patch does not modify any other GRUB function. Am I supposed
> to also declare the modifications in the rmk files?

(written in mailer so identation is wrong)
<your contact line>

   <short description>
* conf/i386-pc.rmk (pkglib_MODULES): add drivemap.mod
(drivemap_mod_SOURCES): new variable
(drivemap_mod_CFLAGS): likewise
(drivemap_mod_LDFLAGS): likewise
(drivemap_mod_ASFLAGS): likewise

* commands/i386/pc/drivemap.c: new file
* commands/i386/pc/drivemap_int13h.S: likewise

>
> --
> -- Lazy, Oblivious, Recurrent Disaster -- Habbit
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>


-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #2: Type: text/html, Size: 13305 bytes --]

Re: status grub2 port of grub-legasy map command

[Javier Martín]

[-- Attachment #1.1: Type: text/plain, Size: 13622 bytes --]

El sáb, 09-05-2009 a las 16:04 +0200, Vladimir 'phcoder' Serbinenko
escribió:
> Hello
> 
> 2009/5/9 Javier Martín <lordhabbit@gmail.com>
>         El sáb, 09-05-2009 a las 11:17 +0200, Vladimir 'phcoder'
>         Serbinenko
>         escribió:
>         > +/* Realmode far ptr (2 * 16b) to the previous INT13h
>         handler.  */
>         > +extern grub_uint32_t grub_drivemap_int13_oldhandler;
>         > I prefer it to be 2 variables because of the way they
>         interract so
>         > nobody would do something like (char *)
>         > grub_drivemap_int13_oldhandler;
>         
>         Two variables? I don't understand. You want the CS and IP
>         parts of the
>         far pointer separated? 
> Yes. Especially that in .S you declared it as two words. But it's not
> an absolute must 
I don't see the advantage, particularly taking into account that the
current code is very happy to treat it as a black box that is taken from
the IVT and not touch it. However, if you insist, I'll change it.

> 
>         Why would anyone try to use it like you said? The
>         comment explicits that it is a realmode pointer, and as such
>         it is not
>         usable as a linear pmode pointer.
>         
>         > +/* The type "void" is used for imported assembly labels,
>         takes no
>         > storage and
>         > +   serves just to take the address with &label.  Do NOT put
>         void*
>         > here.  */
>         > +/* Start of the handler bundle.  */
>         > +extern void grub_drivemap_int13_handler_base;
>         > The common practice is to use declarations like
>         > extern char grub_drivemap_int13_handler_base[];
>         > it makes pointer arithmetics easy
>         
>         That variable is used only twice in the code: one in a call to
>         memcpy
>         and another one inside the macro INT13_OFFSET. It would still
>         be so even
>         if it were changed to a char array, with the added risk that a
>         char
>         array can be modified with almost nothing to gain (as casts
>         would still
>         be needed). 
> The casts are needed if you declare it as char foo[];
> If someone modifies an array he does it on purpose 
I guess you were trying to say that the casts are _not_ needed if I
declared it as such. What I'm trying to say is that in its only _direct_
use (in memcpy), its "const void" signature is perfectly fine for the
callee. All other uses are hidden within a nasty macro "INT13_OFFSET",
which would still be used with your proposed change. Thus, nothing is
gained, but a way to modify the int13h handler code on memory opens,
without requiring a cast. Thus, I think such a change would be for the
worse.

>         > It's not necessary to try to open the disk actually because
>         for some
>         > reason biosdisk module may be not loaded (e.g. grub may use
>         ata) and
>         > loading it may cause nasty effects (e.g. conflict between
>         ata and
>         > biosdisk)
>         > Same applies for revparse_biosdisk. And if user wants to map
>         to
>         > unexistant disk it's ok. So just use tryparse_diskstring
>         
>         Hmm... this is a profound design issue: you are arguing that
>         drivemap
>         should not care about actual BIOS disks, but just their drive
>         numbers,
>         and let you map 0x86 to 0x80 even if you have no (hd6). I do
>         not agree,
>         since the main use for GRUB is a bootloader, not a test
>         platform (which
>         would be the only reason to perform that kind of mappings
>         which are sure
>         to cause havoc). Thus, by default drivemap only lets you
>         perform
>         mappings that will work - any stress tester is free to modify
>         the code.
> 
>         
>         Regarding opening the disk, the biosdisk interface does not
>         assure in
>         any way (and again there is almost no API documentation) that
>         hd0 will
>         be 0x80, though it's the sensible thing to expect. However,
>         expecting
>         sensible things from an unstable API prone to redesigns (like
>         the
>         command API, for example)
> Mapping something to non-existant drive makes the drive non-existant.
>  I think it's fair
> It's almost guaranteed that numbering of disks won't change 
AfaIr, most operating systems only implement very basic support for BIOS
disks (because they switch to their own drivers as soon as possible).
Many, among them Linux iIrc, just probe them sequentially and stop when
they find the first non-existant drive, while others probe hd0 thru hd7
and ignore the rest of them. In the first kind of OSes, mapping hd0=hd6
when you have no hd6 will make hd1 disappear along with hd0, which may
be very confusing to the user. Also, making a BIOS disk disappear will
not banish it from being detected by the OS normal drivers, so the
utility of this is pretty much restricted to DOS-like systems.

> 
>  
>         usually leads to nasty surprises later on, so
>         unless 1) the biosdisk interface so specifies and 2) hdN will
>         _always_
>         refer to biosdisk and not another possible handler, I think
>         the check is
>         a Good Thing (tm).
> It's not if it may hang when you use ata.mod 
Well, I don't like to put things like this, but then biosdisk and ata
should be fixed to avoid these kind of bugs.

> 
>         
>         
>         > +  if ((str[0] == 'f' || str[0] == 'h') && str[1] == 'd')
>         > change this to something like
>         > if (! ((str[0] == 'f' || str[0] == 'h') && str[1] == 'd'))
>         >    return ...
>         > It makes code nicer by avoiding unnecessary long else
>         
>         I think the "else" you were talking about was one line long,
>         but it was
>         not needed anyways and so I've removed it.
> 
>         
>         > +      grub_errno = GRUB_ERR_NONE;
>         > No need to set grub_errno explicitely unless you want to
>         ignore an
>         > error
>         
>         I may have got it wrong, but I think that functions like
>         strtoul only
>         _set_ errno if there _has_ been an error, but leave it
>         unchanged if
>         there hasn't. As I want to check if the conversion failed, I
>         need to
>         have the variable to check in a known pre-state.
> If grub_errno is set at the begining of the function then either
> scripting or your code has a bug
So... I can assume that errno=GRUB_ERR_NONE at the start of my function,
right? Ok then, I'll remove that extra line. The check, however, has to
remain, since a failed call to strtoul can return either ULONG_MAX (on
an overflow) or 0 (if the conversion could not be performed), and 0 is
one of our acceptable values.

> 
>         
>         > +      unsigned long drivenum = grub_strtoul (str + 2, 0,
>         0);
>         > +      if (grub_errno != GRUB_ERR_NONE || drivenum > 127)
>         > I think it's enough to just check the range
>         
>         See above.
>         
>         > +    {
>         > +      /* N not a number or out of range.  */
>         > +      goto fail;
>         > Since at fail you have just a return, just put a return here
>         with
>         > informative error message
>         
>         The function was rearranged so only the first goto is required
>         now.
> +fail: 
> +  return grub_error (GRUB_ERR_BAD_ARGUMENT, "device format \"%s\" " 
> +             "invalid: must be (f|h)dN, with 0 <= N < 128", str);
> I don't see a clear benefit of using goto here. Perhaps providing a
> separate informative messages for 2 fail cases is a better option 
After a more thorough analysis, the initial check is not necessary
because the caller never passes a null pointer and the second check will
fail with the empty string. Removed initial "if", the goto and the
label.

> 
>  
>         
>         > +    }
>         > +      else
>         > else is unnecessary, just continue with the code
>         
>         I fear you don't give me enough context to find this one. If
>         it was
>         inside tryparse_diskstring, it has been removed.
>         
>         > +  if (cmd->state[OPTIDX_LIST].set || 0 == argc)
>         > argc == 0 sounds better. Also moving list function to a
>         separate
>         > function is a good idea
>         
>         Ok, changed. I'd just like to remark that if someone slipped
>         and changed
>         the line to "argc = 0" it would pass unnoticed by the
>         compiler, while "0
>         = argc" would create a compile-time error. 
> It's what warnings are for 
This kind of construct:
	if (argc = 0)
is allowed by the standard, even if it seems nonsensical, to allow for
things like:
	if (! (hFile = fopen("my_file.txt")) )
		exit(2);
Thus most compilers will give no warning even with their maximum warning
settings. On the other hand, this:
	if (0 = argc)
is guaranteed to cause a compile-time error. However, the code has
already been changed to what you requested.

> 
>         Regarding slicing the listing
>         functionality, I don't see the advantages.
> It makes code more readable and avoids long else clause 
The else clause was removed.

> 
>         > +       push    %bp
>         > +       mov     %sp, %bp
>         > You don't need to change bp. Just use sp. In this case it
>         makes code
>         > easier
>         
>         That would make it more difficult to address passed parameters
>         like the
>         flags at [BP+6] because their location would depend on the
>         current
>         status of SP (which might change depending on code above the
>         relevant
>         lines). 
> I checked and can say that removing bp code makes the rest actually
> easier. If you don't see how tell me I'll modify it 
>         I think that trying to shave off those 4 or so bytes would
>         make
>         the code unnecessarily more complicated, which is usually a
>         no-no when
>         assembly is involved. Besides, this prolog sequence and the
>         corresponding epilog before returning is practically a
>         standard in x86.
>         
>         > +       lcall   *%
>         >
>         cs:GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_oldhandler)
>         > +       pop     %bp /* The pushed flags were removed by
>         iret.  */
>         > Use rather:
>         > pushf
>         > lcall   *%
>         >
>         cs:GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_oldhandler)
>         
>         Why would we want to save our current flags? We want the old
>         int13
>         handler to receive the flags it would have received if the
>         drivemap
>         int13 handler were not present, that is, the original caller
>         flags which
>         were stored above CS:IP in the stack when the "int $0x13" call
>         was
>         issued. The comment means that "iret" pops not just CS:IP but
>         also the
>         flags, so we can proceed directly to popping %bp.
>         
>         
>         > Also remember to restore original %dl after the call
>         
>         I think there is no need to do so, because BIOS calls modify %
>         dl anyway.
>         
> Then you can remove %bp and just make a long jump. This way int 13
> handler recieves the original stack intact
So what you want is to make control flow return to the caller directly
and seamlessly... Let's see if you like the new version of the handler
better. The con here is that we lost its "normal-function" structure, so
any modification to check anything after the BIOS handler returns would
require its complete revamping into a form... just like what I had been
using. I can't say that I don't like the elegance of your idea, but in
non performance-critical assembly code I usually prefer standard forms
to structural beauty.

> 
>         
>         
>         > Could you also prepare a changelog entry for it?
>         
>         Hmm... I don't really know how to write it, given that both
>         files are
>         new and the patch does not modify any other GRUB function. Am
>         I supposed
>         to also declare the modifications in the rmk files?
> (written in mailer so identation is wrong) 
> 
> <your contact line>
> 
>    <short description>
> * conf/i386-pc.rmk (pkglib_MODULES): add drivemap.mod
> (drivemap_mod_SOURCES): new variable
> (drivemap_mod_CFLAGS): likewise
> (drivemap_mod_LDFLAGS): likewise
> (drivemap_mod_ASFLAGS): likewise
> 
> * commands/i386/pc/drivemap.c: new file
> * commands/i386/pc/drivemap_int13h.S: likewise

Ok then, I'm copying your template:

2009-05-09 Javier Martin <lordhabbit@gmail.com>

	New module drivemap: allows remapping of BIOS disks
	
	* conf/i386-pc.rmk (pkglib_MODULES): add drivemap.mod
	(drivemap_mod_SOURCES): new variable
	(drivemap_mod_CFLAGS): likewise
	(drivemap_mod_LDFLAGS): likewise
	(drivemap_mod_ASFLAGS): likewise
	* commands/i386/pc/drivemap.c: new file
	* commands/i386/pc/drivemap_int13h.S: likewise

OK, I have a good feeling about this version of the patch. Most
importantly, it still works!!

Cheers!

-- 
-- Lazy, Oblivious, Recurrent Disaster -- Habbit

[-- Attachment #1.2: drivemap.12.patch --]
[-- Type: text/x-patch, Size: 19094 bytes --]

Index: conf/i386-pc.rmk
===================================================================
--- conf/i386-pc.rmk	(revision 2201)
+++ conf/i386-pc.rmk	(working copy)
@@ -185,8 +185,15 @@
 	aout.mod bsd.mod pxe.mod pxecmd.mod datetime.mod date.mod \
 	datehook.mod lsmmap.mod ata_pthru.mod hdparm.mod \
 	usb.mod uhci.mod ohci.mod usbtest.mod usbms.mod usb_keyboard.mod \
-	efiemu.mod mmap.mod acpi.mod
+	efiemu.mod mmap.mod acpi.mod drivemap.mod
 
+# For drivemap.mod.
+drivemap_mod_SOURCES = commands/i386/pc/drivemap.c \
+                       commands/i386/pc/drivemap_int13h.S
+drivemap_mod_ASFLAGS = $(COMMON_ASFLAGS)
+drivemap_mod_CFLAGS = $(COMMON_CFLAGS)
+drivemap_mod_LDFLAGS = $(COMMON_LDFLAGS)
+
 # For efiemu.mod.
 efiemu_mod_SOURCES = efiemu/main.c efiemu/i386/loadcore32.c \
 		     efiemu/i386/loadcore64.c efiemu/i386/pc/cfgtables.c \
Index: commands/i386/pc/drivemap.c
===================================================================
--- commands/i386/pc/drivemap.c	(revision 0)
+++ commands/i386/pc/drivemap.c	(revision 0)
@@ -0,0 +1,443 @@
+/* drivemap.c - command to manage the BIOS drive mappings.  */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2008, 2009  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/extcmd.h>
+#include <grub/dl.h>
+#include <grub/mm.h>
+#include <grub/misc.h>
+#include <grub/disk.h>
+#include <grub/machine/biosdisk.h>
+#include <grub/loader.h>
+#include <grub/machine/memory.h>
+
+
+#define MODNAME "drivemap"
+
+static grub_extcmd_t cmd_reghandle;
+
+/* Remember to update enum opt_idxs accordingly.  */
+static const struct grub_arg_option options[] = {
+  {"list", 'l', 0, "show the current mappings", 0, 0},
+  {"reset", 'r', 0, "reset all mappings to the default values", 0, 0},
+  {"swap", 's', 0, "perform both direct and reverse mappings", 0, 0},
+  {0, 0, 0, 0, 0, 0}
+};
+
+/* Remember to update options[] accordingly.  */
+enum opt_idxs
+{
+  OPTIDX_LIST = 0,
+  OPTIDX_RESET,
+  OPTIDX_SWAP,
+};
+
+/* Realmode far ptr (2 * 16b) to the previous INT13h handler.  */
+extern grub_uint32_t grub_drivemap_int13_oldhandler;
+
+/* The type "void" is used for imported assembly labels, takes no storage and
+   serves just to take the address with &label.  Do NOT put void* here.  */
+/* Start of the handler bundle.  */
+extern const void grub_drivemap_int13_handler_base;
+/* Start of the drive mappings area (space reserved at runtime).  */
+extern const void grub_drivemap_int13_mapstart;
+/* The assembly function to replace the old INT13h handler. It should not be
+   called because it does not follow any C callspecs and returns with IRET.  */
+extern const void grub_drivemap_int13_handler;
+
+typedef struct drivemap_node
+{
+  struct drivemap_node *next;
+  grub_uint8_t newdrive;
+  grub_uint8_t redirto;
+} drivemap_node_t;
+
+static drivemap_node_t *map_head;
+static void *insthandler_hook;
+static int handlermem_hnd;
+static grub_err_t install_int13_handler (int noret __attribute__ ((unused)));
+static grub_err_t uninstall_int13_handler (void);
+
+/* Puts the specified mapping into the table, replacing an existing mapping
+   for newdrive or adding a new one if required.  */
+static grub_err_t
+drivemap_set (grub_uint8_t newdrive, grub_uint8_t redirto)
+{
+  drivemap_node_t *mapping = 0;
+  drivemap_node_t *search = map_head;
+  while (search)
+    {
+      if (search->newdrive == newdrive)
+	{
+	  mapping = search;
+	  break;
+	}
+      search = search->next;
+    }
+
+  /* Check for pre-existing mappings to modify before creating a new one.  */
+  if (mapping)
+    mapping->redirto = redirto;
+  else
+    {
+      mapping = grub_malloc (sizeof (drivemap_node_t));
+      if (! mapping)
+	return grub_error (GRUB_ERR_OUT_OF_MEMORY,
+			   "cannot allocate map entry, not enough memory");
+      mapping->newdrive = newdrive;
+      mapping->redirto = redirto;
+      mapping->next = map_head;
+      map_head = mapping;
+    }
+  return GRUB_ERR_NONE;
+}
+
+/* Removes the mapping for newdrive from the table.  If there is no mapping,
+   then this function behaves like a no-op on the map.  */
+static void
+drivemap_remove (grub_uint8_t newdrive)
+{
+  drivemap_node_t *mapping = 0;
+  drivemap_node_t *search = map_head;
+  drivemap_node_t *previous = 0;
+
+  while (search)
+    {
+      if (search->newdrive == newdrive)
+	{
+	  mapping = search;
+	  break;
+	}
+      previous = search;
+      search = search->next;
+    }
+
+  if (mapping)
+    {
+      if (previous)
+	previous->next = mapping->next;
+      else
+	map_head = mapping->next;
+      grub_free (mapping);
+    }
+}
+
+/* Given a device name, resolves its BIOS disk number and stores it in the
+   passed location, which should only be trusted if ERR_NONE is returned.  */
+static grub_err_t
+parse_biosdisk (const char *name, grub_uint8_t * disknum)
+{
+  grub_disk_t disk;
+  /* Skip the first ( in (hd0) - disk_open wants just the name.  */
+  if (*name == '(')
+    name++;
+
+  disk = grub_disk_open (name);
+  if (! disk)
+    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "unknown device \"%s\"", name);
+  const enum grub_disk_dev_id id = disk->dev->id;
+  /* The following assignment is only sound if the device is indeed a
+     biosdisk.  The caller must check the return value.  */
+  if (disknum)
+    *disknum = disk->id;
+  grub_disk_close (disk);
+  if (id != GRUB_DISK_DEVICE_BIOSDISK_ID)
+    return grub_error (GRUB_ERR_BAD_DEVICE, "%s is not a BIOS disk", name);
+  return GRUB_ERR_NONE;
+}
+
+/* Given a BIOS disk number, returns its GRUB device name if it exists.  
+   If the call succeeds, the resulting device string must be freed.  
+   For nonexisting BIOS disk numbers, this function returns
+   GRUB_ERR_UNKNOWN_DEVICE.  */
+static grub_err_t
+revparse_biosdisk (const grub_uint8_t dnum, const char **output)
+{
+  int found = 0;
+  auto int find (const char *name);
+  int find (const char *name)
+  {
+    const grub_disk_t disk = grub_disk_open (name);
+    if (! disk)
+      return 0;
+    if (disk->id == dnum && disk->dev->id == GRUB_DISK_DEVICE_BIOSDISK_ID)
+      {
+	found = 1;
+	if (output)
+	  *output = grub_strdup (name);
+      }
+    grub_disk_close (disk);
+    return found;
+  }
+
+  grub_disk_dev_iterate (find);
+  if (found)
+    return GRUB_ERR_NONE;
+  return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "BIOS disk %02x not found", dnum);
+}
+
+/* Given a GRUB-like device name and a convenient location, stores the
+   related BIOS disk number.  Accepts devices like \((f|h)dN\), with
+   0 <= N < 128.  */
+static grub_err_t
+tryparse_diskstring (const char *str, grub_uint8_t * output)
+{
+  /* Skip opening paren in order to allow both (hd0) and hd0.  */
+  if (*str == '(')
+    str++;
+  if ((str[0] == 'f' || str[0] == 'h') && str[1] == 'd')
+    {
+      grub_uint8_t bios_num = (str[0] == 'h') ? 0x80 : 0x00;
+      unsigned long drivenum = grub_strtoul (str + 2, 0, 0);
+      if (grub_errno == GRUB_ERR_NONE && drivenum < 128)
+	{
+	  bios_num |= drivenum;
+	  if (output)
+	    *output = bios_num;
+	  return GRUB_ERR_NONE;
+	}
+    }
+  return grub_error (GRUB_ERR_BAD_ARGUMENT, "device format \"%s\" "
+		     "invalid: must be (f|h)dN, with 0 <= N < 128", str);
+}
+
+static grub_err_t
+grub_cmd_drivemap (struct grub_extcmd *cmd, int argc, char **args)
+{
+  if (cmd->state[OPTIDX_LIST].set || argc == 0)
+    {
+      /* Show: list mappings.  */
+      if (! map_head)
+        {
+	  grub_printf ("No drives have been remapped");
+	  return GRUB_ERR_NONE;
+	}
+      grub_printf ("Showing only remapped drives.\n");
+      grub_printf ("BIOS disk #num ----> GRUB device\n");
+      drivemap_node_t *curnode = map_head;
+      while (curnode)
+	{
+	  const char *dname = 0;
+	  grub_err_t err = revparse_biosdisk (curnode->redirto, &dname);
+	  if (err != GRUB_ERR_NONE)
+	    return err;
+	  grub_printf ("%cD #%-3u (0x%02x)       %s\n",
+		       (curnode->newdrive & 0x80) ? 'H' : 'F',
+		       curnode->newdrive & 0x7F, curnode->newdrive,
+		       dname);
+	  curnode = curnode->next;
+	  grub_free ((char *) dname);
+	}
+      return GRUB_ERR_NONE;
+    }
+  else if (cmd->state[OPTIDX_RESET].set)
+    {
+      /* Reset: just delete all mappings, freeing their memory.  */
+      drivemap_node_t *curnode = map_head;
+      drivemap_node_t *prevnode = 0;
+      while (curnode)
+	{
+	  prevnode = curnode;
+	  curnode = curnode->next;
+	  grub_free (prevnode);
+	}
+      map_head = 0;
+      return GRUB_ERR_NONE;
+    }
+    
+  /* Neither flag: put mapping.  */
+  grub_uint8_t mapfrom = 0;
+  grub_uint8_t mapto = 0xFF;
+  grub_err_t err;
+
+  if (argc != 2)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "two arguments required");
+
+  err = parse_biosdisk (args[0], &mapfrom);
+  if (err != GRUB_ERR_NONE)
+    return err;
+
+  /* When swapping we require both devices to be BIOS disks, but when
+     performing direct mappings we only require the 2nd argument to look
+     like a BIOS disk in order to resolve it into a BIOS disk number.  */
+  if (cmd->state[OPTIDX_SWAP].set)
+    err = parse_biosdisk (args[1], &mapto);
+  else
+    err = tryparse_diskstring (args[1], &mapto);
+  if (err != GRUB_ERR_NONE)
+    return err;
+
+  if (mapto == mapfrom)
+    {
+      /* Reset to default.  */
+      grub_dprintf (MODNAME, "Removing the mapping for %s (%02x)",
+		    args[0], mapfrom);
+      drivemap_remove (mapfrom);
+      return GRUB_ERR_NONE;
+    }
+  /* Set the mapping for the disk (overwrites any existing mapping).  */
+  grub_dprintf (MODNAME, "%s %s (%02x) = %s (%02x)\n",
+		cmd->state[OPTIDX_SWAP].set ? "Swapping" : "Mapping",
+		args[1], mapto, args[0], mapfrom);
+  err = drivemap_set (mapto, mapfrom);
+  /* If -s, perform the reverse mapping too (only if the first was OK).  */
+  if (cmd->state[OPTIDX_SWAP].set && err == GRUB_ERR_NONE)
+    err = drivemap_set (mapfrom, mapto);
+  return err;
+}
+
+typedef struct __attribute__ ((packed)) int13map_node
+{
+  grub_uint8_t disknum;
+  grub_uint8_t mapto;
+} int13map_node_t;
+
+#define INT13H_OFFSET(x) ( ((grub_uint8_t*)(x)) - ((grub_uint8_t*)&grub_drivemap_int13_handler_base) )
+#define INT13H_REBASE(x) ( (void*) (handler_base + (x)) )
+#define INT13H_TONEWADDR(x) INT13H_REBASE( INT13H_OFFSET( x ) )
+
+/* Int13h handler installer - reserves conventional memory for the handler,
+   copies it over and sets the IVT entry for int13h.  
+   This code rests on the assumption that GRUB does not activate any kind
+   of memory mapping apart from identity paging, since it accesses
+   realmode structures by their absolute addresses, like the IVT at 0;
+   and transforms a pmode pointer into a rmode seg:off far ptr.  */
+static grub_err_t
+install_int13_handler (int noret __attribute__ ((unused)))
+{
+  grub_size_t entries = 0;
+  drivemap_node_t *curentry = map_head;
+
+  /* Count entries to prepare a contiguous map block.  */
+  while (curentry)
+    {
+      entries++;
+      curentry = curentry->next;
+    }
+  if (entries == 0)
+    {
+      /* No need to install the int13h handler.  */
+      grub_dprintf (MODNAME, "No drives marked as remapped, installation "
+		    "of an int13h handler is not required.");
+      return GRUB_ERR_NONE;
+    }
+  else
+    {
+      /* Real mode IVT slot (seg:off far pointer) for interrupt 0x13.  */
+      grub_uint32_t *ivtslot = UINT_TO_PTR (0x0000004c);
+      /* Size of the full int13 handler "bundle", including code and map.  */
+      grub_uint64_t total_size;
+      /* Base address of the space reserved for the handler bundle.  */
+      grub_uint8_t *handler_base = 0;
+      /* Address of the map within the deployed bundle.  */
+      int13map_node_t *handler_map;
+      /* Real mode IVT entry (seg:off far pointer) for the new handler.  */
+      grub_uint32_t ivtentry;
+
+      grub_dprintf (MODNAME, "Installing int13h handler...\n");
+
+      /* Save the pointer to the old handler.  */
+      grub_drivemap_int13_oldhandler = *ivtslot;
+      grub_dprintf (MODNAME, "Old int13 handler at %04x:%04x\n",
+		    (grub_drivemap_int13_oldhandler >> 16) & 0x0ffff,
+		    grub_drivemap_int13_oldhandler & 0x0ffff);
+
+      /* Find a rmode-segment-aligned zone in conventional memory big
+         enough to hold the handler and its data.  */
+      total_size = INT13H_OFFSET(&grub_drivemap_int13_mapstart)
+	+ (entries + 1) * sizeof (int13map_node_t);
+      grub_dprintf (MODNAME, "Payload is %llu bytes long\n", total_size);
+      handler_base = grub_mmap_malign_and_register (16, total_size,
+						    &handlermem_hnd,
+						    GRUB_MACHINE_MEMORY_RESERVED,
+						    GRUB_MMAP_MALLOC_LOW);
+      if (! handler_base)
+	return grub_error (GRUB_ERR_OUT_OF_MEMORY, "Could not reserve "
+			   "memory for the int13h handler");
+
+      /* Copy int13h handler bundle to reserved area.  */
+      grub_dprintf (MODNAME, "Reserved memory at %p, copying handler...\n",
+		    handler_base);
+      grub_memcpy (handler_base, &grub_drivemap_int13_handler_base,
+		   INT13H_OFFSET (&grub_drivemap_int13_mapstart));
+
+      /* Copy the mappings to the reserved area.  */
+      curentry = map_head;
+      grub_size_t i;
+      handler_map = INT13H_TONEWADDR (&grub_drivemap_int13_mapstart);
+      grub_dprintf (MODNAME, "Target map at %p, copying mappings...\n",
+		    handler_map);
+      for (i = 0; i < entries; ++i, curentry = curentry->next)
+	{
+	  handler_map[i].disknum = curentry->newdrive;
+	  handler_map[i].mapto = curentry->redirto;
+	  grub_dprintf (MODNAME, "\t#%d: 0x%02x <- 0x%02x\n", i,
+			handler_map[i].disknum, handler_map[i].mapto);
+	}
+      /* Signal end-of-map.  */
+      handler_map[i].disknum = 0;
+      handler_map[i].mapto = 0;
+      grub_dprintf (MODNAME, "\t#%d: 0x00 <- 0x00 (end)\n", i);
+
+      /* Install our function as the int13h handler in the IVT.  */
+      ivtentry = ((grub_uint32_t) handler_base) << 12;	/* Segment address.  */
+      ivtentry |=
+	(grub_uint16_t) INT13H_OFFSET (&grub_drivemap_int13_handler);
+      grub_dprintf (MODNAME, "New int13 handler IVT pointer: %04x:%04x\n",
+		    (ivtentry >> 16) & 0x0ffff, ivtentry & 0x0ffff);
+      *ivtslot = ivtentry;
+
+      return GRUB_ERR_NONE;
+    }
+}
+
+static grub_err_t
+uninstall_int13_handler (void)
+{
+  grub_uint32_t *ivtslot = UINT_TO_PTR (0x0000004c);
+
+  if (grub_drivemap_int13_oldhandler != 0)
+    return GRUB_ERR_NONE;
+
+  *ivtslot = grub_drivemap_int13_oldhandler;
+  grub_mmap_free_and_unregister (handlermem_hnd);
+  grub_drivemap_int13_oldhandler = 0;
+
+  return GRUB_ERR_NONE;
+}
+
+GRUB_MOD_INIT (drivemap)
+{
+  cmd_reghandle = grub_register_extcmd (MODNAME, grub_cmd_drivemap,
+					GRUB_COMMAND_FLAG_BOTH,
+					MODNAME
+					" -l | -r | [-s] grubdev biosdisk",
+					"Manage the BIOS drive mappings",
+					options);
+  insthandler_hook =
+    grub_loader_register_preboot_hook (&install_int13_handler,
+				       &uninstall_int13_handler,
+				       GRUB_LOADER_PREBOOT_HOOK_PRIO_NORMAL);
+}
+
+GRUB_MOD_FINI (drivemap)
+{
+  grub_loader_unregister_preboot_hook (insthandler_hook);
+  insthandler_hook = 0;
+  grub_unregister_extcmd (cmd_reghandle);
+}
Index: commands/i386/pc/drivemap_int13h.S
===================================================================
--- commands/i386/pc/drivemap_int13h.S	(revision 0)
+++ commands/i386/pc/drivemap_int13h.S	(revision 0)
@@ -0,0 +1,70 @@
+/* drivemap_int13h.S - interrupt handler for the BIOS drive remapper */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2008, 2009  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/symbol.h>
+
+#define INT13H_OFFSET(x) ((x) - grub_drivemap_int13_handler_base)
+
+.code16
+
+/* Copy starts here.  When deployed, this label must be segment-aligned.  */
+VARIABLE(grub_drivemap_int13_handler_base)
+
+/* Far pointer to the old handler.  Stored as a CS:IP in the style of real-mode
+   IVT entries (thus PI:SC in mem).  */
+VARIABLE(grub_drivemap_int13_oldhandler)
+       .word 0x0, 0x0
+
+/* Actual int13 handler.  We use relative addressing with CS in order to be as
+   unintrusive as possible with registers and the stack.  */
+FUNCTION(grub_drivemap_int13_handler)
+       /* Map the drive number (always in DL).  */
+       push	%ax
+       push	%bx
+       push	%si
+       movw	$INT13H_OFFSET(grub_drivemap_int13_mapstart), %bx
+       xorw	%si, %si
+
+more_remaining:
+       movw	%cs:(%bx,%si), %ax
+       cmpb	%ah, %al
+       jz	not_found /* DRV=DST => map end - drive not remapped, keep DL.  */
+       cmpb	%dl, %al
+       jz	found /* Found - drive remapped, modify DL.  */
+       addw	$2, %si
+       jmp	more_remaining /* Not found, but more remaining, loop.  */
+
+found:
+       movb	%ah, %dl
+
+not_found:
+       pop	%si
+       pop	%bx
+       pop	%ax
+       
+       /* Upon arrival to this point the stack must be exactly like at entry.  
+          This long jump will transfer the caller's stack to the old INT13
+          handler, thus making it return directly to the original caller.  */
+       ljmp	*%cs:INT13H_OFFSET(grub_drivemap_int13_oldhandler)
+
+
+/* This label MUST be at the end of the copied block, since the installer code
+   reserves additional space for mappings at runtime and copies them over it.  */
+.align 2
+VARIABLE(grub_drivemap_int13_mapstart)

[-- Attachment #2: Esto es una parte de mensaje firmado digitalmente --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: status grub2 port of grub-legasy map command

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 2171 bytes --]

Hello. Sorry for the delay, I was kept busy by making grub2 compile with
apples compiler. It already works just needs some testing and cleanup and
then I'll post a patch

> I don't see the advantage, particularly taking into account that the
> current code is very happy to treat it as a black box that is taken from
> the IVT and not touch it. However, if you insist, I'll change it.
>
No, it was more a suggestion

> I guess you were trying to say that the casts are _not_ needed if I
> declared it as such. What I'm trying to say is that in its only _direct_
> use (in memcpy), its "const void" signature is perfectly fine for the
> callee. All other uses are hidden within a nasty macro "INT13_OFFSET",
> which would still be used with your proposed change. Thus, nothing is
> gained, but a way to modify the int13h handler code on memory opens,
> without requiring a cast. Thus, I think such a change would be for the
> worse.
>
I see your reasoning. I personally prefer char[] and considering grub2 works
with different compilers I prefer to avoid constructs which might be absent
in some C dialects. I think we need a third opinion on this

> AfaIr, most operating systems only implement very basic support for BIOS
> disks (because they switch to their own drivers as soon as possible).
> Many, among them Linux iIrc, just probe them sequentially and stop when
> they find the first non-existant drive, while others probe hd0 thru hd7
> and ignore the rest of them. In the first kind of OSes, mapping hd0=hd6
> when you have no hd6 will make hd1 disappear along with hd0, which may
> be very confusing to the user. Also, making a BIOS disk disappear will
> not banish it from being detected by the OS normal drivers, so the
> utility of this is pretty much restricted to DOS-like systems.
>
Of course it's not much additional benefit however I don't see any real
reason not to let the user do it

>
> Well, I don't like to put things like this, but then biosdisk and ata
> should be fixed to avoid these kind of bugs.
>
It's more difficult then this. The bugs can be in BIOS. Again I think we
need a third opinion

>
>


-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #2: Type: text/html, Size: 3288 bytes --]

Re: status grub2 port of grub-legasy map command

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 3260 bytes --]

El dom, 10-05-2009 a las 13:47 +0200, Vladimir 'phcoder' Serbinenko
escribió:

>         I guess you were trying to say that the casts are _not_ needed
>         if I
>         declared it as such. What I'm trying to say is that in its
>         only _direct_
>         use (in memcpy), its "const void" signature is perfectly fine
>         for the
>         callee. All other uses are hidden within a nasty macro
>         "INT13_OFFSET",
>         which would still be used with your proposed change. Thus,
>         nothing is
>         gained, but a way to modify the int13h handler code on memory
>         opens,
>         without requiring a cast. Thus, I think such a change would be
>         for the
>         worse.
>         
>         
> I see your reasoning. I personally prefer char[] and considering grub2
> works with different compilers I prefer to avoid constructs which
> might be absent in some C dialects. I think we need a third opinion on
> this
According to the last C standard, section 6.2.5.19, "The void type
comprises an empty set of values; it is an incomplete type that cannot
be completed." Thus, declaring a variable as "extern void" is perfectly
legal C, just like this program is perfectly legal:

	extern struct s b;

	int main(int argc, char** argv) {
		return 0;
	}

Notice that the declaration of "struct s" is absent, and so it is an
incomplete type, but still the declaration of a variable as "extern
struct s" is legal (using "b", however, would not, since the compiler
does not know the fields of "struct s"). Thus, declaring an extern
variable as the incomplete type "void" is legal too, and the only
operation that can be performed on it is taking its address with &. We
can try to get third opinions if you want, though.

> 
>         AfaIr, most operating systems only implement very basic
>         support for BIOS
>         disks (because they switch to their own drivers as soon as
>         possible).
>         Many, among them Linux iIrc, just probe them sequentially and
>         stop when
>         they find the first non-existant drive, while others probe hd0
>         thru hd7
>         and ignore the rest of them. In the first kind of OSes,
>         mapping hd0=hd6
>         when you have no hd6 will make hd1 disappear along with hd0,
>         which may
>         be very confusing to the user. Also, making a BIOS disk
>         disappear will
>         not banish it from being detected by the OS normal drivers, so
>         the
>         utility of this is pretty much restricted to DOS-like systems.
>         
> Of course it's not much additional benefit however I don't see any
> real reason not to let the user do it 
For me, the possibility of creating an inconsistent situation in which
hd0 is "erased" through drivemap and hd1 disappears along with it is
enough. However, I think we could settle on making it a switch of the
drivemap command (like "--force" or SLT). Nevertheless, this would
change the meaning of the drivemap arguments from "the BIOS drive that
is represented in GRUB by hdN" to "the Nth BIOS hard drive", which might
be confusing.


-- 
-- Lazy, Oblivious, Recurrent Disaster -- Habbit

[-- Attachment #2: Esto es una parte de mensaje firmado digitalmente --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: status grub2 port of grub-legasy map command

[Pavel Roskin]

On Sat, 2009-05-09 at 17:42 +0200, Javier Martín wrote:

> OK, I have a good feeling about this version of the patch. Most
> importantly, it still works!!

I have committed your patch after a cleanup.  My changes were following:

grub_drivemap_int13_handler_base and grub_drivemap_int13_handler have
been merged.

grub_drivemap_int13_oldhandler points directly to the ljmp argument
(other GRUB sources do it too, jump look for 0xea).

%si is not used anymore.

Tabs are used consistently in drivemap_int13h.S

Many variables have been renamed.

Constants have been brought to the top level and marked as such.

Logic in uninstall_int13_handler() has been fixed.

Some messages have been clarified.  In particularly, biosdisk is called
osdisk now, as it's what the OS sees.  I was thinking of "payload" or
"loadee" as more generic terms, but it can be confusing to the users.

My check for drivemap with no arguments was wrong, fixed now.

Added missing line ends in all outputs.

Removed INT13H_REBASE and INT13H_TONEWADDR, as they were used only once.

Comments have been improved.

-- 
Regards,
Pavel Roskin

Re: status grub2 port of grub-legasy map command

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 2036 bytes --]

Hello, I had two clear oppositions which weren't resolved. I don't
believe that merge patches screwing up the pendin oppositions is a
good practice. The opposition about declaration is based on another
handlers how it is used accross grub. Opposition about calling
biosdisk is technically relevant. Or perhaps I should start committing
anything without discussion?
Here is a fix patch. Could I commit it even if there will be oppositions?

On Thu, May 14, 2009 at 3:51 AM, Pavel Roskin <proski@gnu.org> wrote:
> On Sat, 2009-05-09 at 17:42 +0200, Javier Martín wrote:
>
>> OK, I have a good feeling about this version of the patch. Most
>> importantly, it still works!!
>
> I have committed your patch after a cleanup.  My changes were following:
>
> grub_drivemap_int13_handler_base and grub_drivemap_int13_handler have
> been merged.
>
> grub_drivemap_int13_oldhandler points directly to the ljmp argument
> (other GRUB sources do it too, jump look for 0xea).
>
> %si is not used anymore.
>
> Tabs are used consistently in drivemap_int13h.S
>
> Many variables have been renamed.
>
> Constants have been brought to the top level and marked as such.
>
> Logic in uninstall_int13_handler() has been fixed.
>
Which logic fix?
Other than variable rename it seems to be identical to Javier Martín's patch
> Some messages have been clarified.  In particularly, biosdisk is called
> osdisk now, as it's what the OS sees.
>  I was thinking of "payload" or
> "loadee" as more generic terms, but it can be confusing to the users.
>
> My check for drivemap with no arguments was wrong, fixed now.
>
> Added missing line ends in all outputs.
>
> Removed INT13H_REBASE and INT13H_TONEWADDR, as they were used only once.
>
> Comments have been improved.
>
> --
> Regards,
> Pavel Roskin
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #2: mapfix.diff --]
[-- Type: text/x-patch, Size: 3903 bytes --]

Index: commands/i386/pc/drivemap.c
===================================================================
--- commands/i386/pc/drivemap.c	(revision 2213)
+++ commands/i386/pc/drivemap.c	(working copy)
@@ -55,10 +55,10 @@
 
 /* The assembly function to replace the old INT13h handler. It does not follow
    any C callspecs and returns with IRET.  */
-extern const void grub_drivemap_handler;
+extern const char grub_drivemap_handler[];
 
 /* Start of the drive mappings area (space reserved at runtime).  */
-extern const void grub_drivemap_mapstart;
+extern const char grub_drivemap_mapstart[];
 
 typedef struct drivemap_node
 {
@@ -74,7 +74,7 @@
 } int13map_node_t;
 
 #define INT13H_OFFSET(x) \
-	(((grub_uint8_t *)(x)) - ((grub_uint8_t *)&grub_drivemap_handler))
+	((x) - (grub_drivemap_handler))
 
 static drivemap_node_t *map_head;
 static void *drivemap_hook;
@@ -144,31 +144,6 @@
     }
 }
 
-/* Given a device name, resolves its BIOS disk number and stores it in the
-   passed location, which should only be trusted if ERR_NONE is returned.  */
-static grub_err_t
-parse_biosdisk (const char *name, grub_uint8_t *disknum)
-{
-  grub_disk_t disk;
-  /* Skip the first ( in (hd0) - disk_open wants just the name.  */
-  if (*name == '(')
-    name++;
-
-  disk = grub_disk_open (name);
-  if (! disk)
-    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "unknown device \"%s\"",
-		       name);
-  const enum grub_disk_dev_id id = disk->dev->id;
-  /* The following assignment is only sound if the device is indeed a
-     biosdisk.  The caller must check the return value.  */
-  if (disknum)
-    *disknum = disk->id;
-  grub_disk_close (disk);
-  if (id != GRUB_DISK_DEVICE_BIOSDISK_ID)
-    return grub_error (GRUB_ERR_BAD_DEVICE, "%s is not a BIOS disk", name);
-  return GRUB_ERR_NONE;
-}
-
 /* Given a BIOS disk number, returns its GRUB device name if it exists.
    If the call succeeds, the resulting device string must be freed.
    For nonexisting BIOS disk numbers, this function returns
@@ -287,17 +262,14 @@
   if (argc != 2)
     return grub_error (GRUB_ERR_BAD_ARGUMENT, "two arguments required");
 
-  err = parse_biosdisk (args[0], &mapfrom);
+  err = tryparse_diskstring (args[0], &mapfrom);
   if (err != GRUB_ERR_NONE)
     return err;
 
   /* When swapping we require both devices to be BIOS disks, but when
      performing direct mappings we only require the 2nd argument to look
      like a BIOS disk in order to resolve it into a BIOS disk number.  */
-  if (cmd->state[OPTIDX_SWAP].set)
-    err = parse_biosdisk (args[1], &mapto);
-  else
-    err = tryparse_diskstring (args[1], &mapto);
+  err = tryparse_diskstring (args[1], &mapto);
   if (err != GRUB_ERR_NONE)
     return err;
 
@@ -364,7 +336,7 @@
 
   /* Find a rmode-segment-aligned zone in conventional memory big
      enough to hold the handler and its data.  */
-  total_size = INT13H_OFFSET (&grub_drivemap_mapstart)
+  total_size = INT13H_OFFSET (grub_drivemap_mapstart)
     + (entries + 1) * sizeof (int13map_node_t);
   grub_dprintf (MODNAME, "Payload is %u bytes long\n", total_size);
   handler_base = grub_mmap_malign_and_register (16, total_size,
@@ -378,13 +350,13 @@
   /* Copy int13h handler bundle to reserved area.  */
   grub_dprintf (MODNAME, "Reserved memory at %p, copying handler\n",
 		handler_base);
-  grub_memcpy (handler_base, &grub_drivemap_handler,
-	       INT13H_OFFSET (&grub_drivemap_mapstart));
+  grub_memcpy (handler_base, grub_drivemap_handler,
+	       INT13H_OFFSET (grub_drivemap_mapstart));
 
   /* Copy the mappings to the reserved area.  */
   curentry = map_head;
   handler_map = (int13map_node_t *) (handler_base +
-				     INT13H_OFFSET (&grub_drivemap_mapstart));
+				     INT13H_OFFSET (grub_drivemap_mapstart));
   grub_dprintf (MODNAME, "Target map at %p, copying mappings\n", handler_map);
   for (i = 0; i < entries; ++i, curentry = curentry->next)
     {

Re: status grub2 port of grub-legasy map command

[Pavel Roskin]

On Thu, 2009-05-14 at 08:49 +0200, Vladimir 'phcoder' Serbinenko wrote:
> Hello, I had two clear oppositions which weren't resolved. I don't
> believe that merge patches screwing up the pendin oppositions is a
> good practice. The opposition about declaration is based on another
> handlers how it is used accross grub. Opposition about calling
> biosdisk is technically relevant. Or perhaps I should start committing
> anything without discussion?
> Here is a fix patch. Could I commit it even if there will be oppositions?

I'm sorry, I didn't realize you were opposed to the patch.  I assumed
that you just wanted to make some improvements.  Nobody was against the
drivemap command in principle.  The discussion was about minor details.

I'm fine with the change from "const void" to "const char", but we need
to remove a preceding comment about void labels.

As for the parse_biosdisk() change, I'd like to see an explanation.  I
understand you want to allow remapping invalid devices, and I'm fine
with that.  But then list_mappings() should be changed accordingly and
revparse_biosdisk() may need to be eliminated.

> > Logic in uninstall_int13_handler() has been fixed.
> >
> Which logic fix?
> Other than variable rename it seems to be identical to Javier Martín's patch

No, it was:

  if (grub_drivemap_int13_oldhandler != 0)
    return GRUB_ERR_NONE;

The code was refusing to uninstall the handler if it was installed.

I checked the logic by patching the chainloader code to return instead
of giving control to the bootsector.

-- 
Regards,
Pavel Roskin

Re: status grub2 port of grub-legasy map command

[Vladimir 'phcoder' Serbinenko]

On Thu, May 14, 2009 at 4:03 PM, Pavel Roskin <proski@gnu.org> wrote:
> On Thu, 2009-05-14 at 08:49 +0200, Vladimir 'phcoder' Serbinenko wrote:
>> Hello, I had two clear oppositions which weren't resolved. I don't
>> believe that merge patches screwing up the pendin oppositions is a
>> good practice. The opposition about declaration is based on another
>> handlers how it is used accross grub. Opposition about calling
>> biosdisk is technically relevant. Or perhaps I should start committing
>> anything without discussion?
>> Here is a fix patch. Could I commit it even if there will be oppositions?
>
> I'm sorry, I didn't realize you were opposed to the patch.  I assumed
> that you just wanted to make some improvements.  Nobody was against the
> drivemap command in principle.  The discussion was about minor details.
Ok. Sorry for my harsh reaction. Now I'm under a lot of stress.
>
> I'm fine with the change from "const void" to "const char", but we need
> to remove a preceding comment about void labels.
It's not that I'm opposed to void in principle. Just using the same
constructions to do the same things in different files makes code
easier to learn and port
>
> As for the parse_biosdisk() change, I'd like to see an explanation.
The explanation is that if user uses ata or usbms code and code calls
biosdisk, BIOS may issue a command which may conflict with ata/usbms.
Unfortunately it's not a scenario we're able to circumvent (BIOS is
headache) so I prefer to err on a safe side
> I
> understand you want to allow remapping invalid devices, and I'm fine
> with that.  But then list_mappings() should be changed accordingly and
> revparse_biosdisk() may need to be eliminated.
>
Ok. Will have a look at it this weekend
>> > Logic in uninstall_int13_handler() has been fixed.
>> >
>> Which logic fix?
>> Other than variable rename it seems to be identical to Javier Martín's patch
>
> No, it was:
>
>  if (grub_drivemap_int13_oldhandler != 0)
>    return GRUB_ERR_NONE;
>
> The code was refusing to uninstall the handler if it was installed.
>
> I checked the logic by patching the chainloader code to return instead
> of giving control to the bootsector.
>
Ok, didn't notice it. Thanks
> --
> Regards,
> Pavel Roskin
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Re: status grub2 port of grub-legasy map command

[Pavel Roskin]

On Thu, 2009-05-14 at 17:01 +0200, Vladimir 'phcoder' Serbinenko wrote:
> On Thu, May 14, 2009 at 4:03 PM, Pavel Roskin <proski@gnu.org> wrote:
> > I'm sorry, I didn't realize you were opposed to the patch.  I assumed
> > that you just wanted to make some improvements.  Nobody was against the
> > drivemap command in principle.  The discussion was about minor details.
> Ok. Sorry for my harsh reaction. Now I'm under a lot of stress.

No problem.

> > I'm fine with the change from "const void" to "const char", but we need
> > to remove a preceding comment about void labels.
> It's not that I'm opposed to void in principle. Just using the same
> constructions to do the same things in different files makes code
> easier to learn and port

I wonder if we can go the other way and use void for all labels without
storage.  Indeed, it's too easy to misuse a char variable by forgetting
the ampersand before it.

> > As for the parse_biosdisk() change, I'd like to see an explanation.
> The explanation is that if user uses ata or usbms code and code calls
> biosdisk, BIOS may issue a command which may conflict with ata/usbms.
> Unfortunately it's not a scenario we're able to circumvent (BIOS is
> headache) so I prefer to err on a safe side

I agree that we should avoid touching the hardware.  Besides, after
loading ata we may not see some drives that BIOS can see.

Validation of user input is good, but only if it's implemented
correctly.

Another approach may be to use biosdisk calls only if biosdisk is
active.  Otherwise, trust the user and turn off validation.

-- 
Regards,
Pavel Roskin

Re: status grub2 port of grub-legasy map command

[Vladimir 'phcoder' Serbinenko]

>> > I'm fine with the change from "const void" to "const char", but we need
>> > to remove a preceding comment about void labels.
>> It's not that I'm opposed to void in principle. Just using the same
>> constructions to do the same things in different files makes code
>> easier to learn and port
>
> I wonder if we can go the other way and use void for all labels without
> storage.  Indeed, it's too easy to misuse a char variable by forgetting
> the ampersand before it.
If you define it as char [] (as done everywhere in grub2), not char,
you need no ampersand.
>
>> > As for the parse_biosdisk() change, I'd like to see an explanation.
>> The explanation is that if user uses ata or usbms code and code calls
>> biosdisk, BIOS may issue a command which may conflict with ata/usbms.
>> Unfortunately it's not a scenario we're able to circumvent (BIOS is
>> headache) so I prefer to err on a safe side
>
> I agree that we should avoid touching the hardware.  Besides, after
> loading ata we may not see some drives that BIOS can see.
>
> Validation of user input is good, but only if it's implemented
> correctly.
>
> Another approach may be to use biosdisk calls only if biosdisk is
> active.  Otherwise, trust the user and turn off validation.
>
Not really feasible since with our design "active" driver isn't well defined.
> --
> Regards,
> Pavel Roskin
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Re: status grub2 port of grub-legasy map command

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 1336 bytes --]

El jue, 14-05-2009 a las 20:38 +0200, Vladimir 'phcoder' Serbinenko
escribió:
> >> > I'm fine with the change from "const void" to "const char", but we need
> >> > to remove a preceding comment about void labels.
> >> It's not that I'm opposed to void in principle. Just using the same
> >> constructions to do the same things in different files makes code
> >> easier to learn and port
> >
> > I wonder if we can go the other way and use void for all labels without
> > storage.  Indeed, it's too easy to misuse a char variable by forgetting
> > the ampersand before it.
> If you define it as char [] (as done everywhere in grub2), not char,
> you need no ampersand.
Indeed, but I think you are creating a tempest in a teapot. Your
proposal only "saves" the ampersand in the code, while using "void" (or
better, some grub_asmlabel_t typedef'ed to "void" with a comment
explaining why) requires the ampersand - which is not illogic, since you
are taking the address of the label - but protects against accidental
modifications or the surprise factor of "why is mymod_interrupthandler a
char array?".

PS "for the audience": Sorry for being away from the list the days the
patch was finally committed. I'm a bit tied up right now with finals
preparation.

-- 
-- Lazy, Oblivious, Recurrent Disaster -- Habbit

[-- Attachment #2: Esto es una parte de mensaje firmado digitalmente --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: status grub2 port of grub-legasy map command

[Vladimir 'phcoder' Serbinenko]

>> > I'm fine with the change from "const void" to "const char", but we need
>> > to remove a preceding comment about void labels.
>> It's not that I'm opposed to void in principle. Just using the same
>> constructions to do the same things in different files makes code
>> easier to learn and port
>
> I wonder if we can go the other way and use void for all labels without
> storage.  Indeed, it's too easy to misuse a char variable by forgetting
> the ampersand before it.
Not  char []
>
>> > As for the parse_biosdisk() change, I'd like to see an explanation.
>> The explanation is that if user uses ata or usbms code and code calls
>> biosdisk, BIOS may issue a command which may conflict with ata/usbms.
>> Unfortunately it's not a scenario we're able to circumvent (BIOS is
>> headache) so I prefer to err on a safe side
>
> I agree that we should avoid touching the hardware.  Besides, after
> loading ata we may not see some drives that BIOS can see.
>
> Validation of user input is good, but only if it's implemented
> correctly.
>
> Another approach may be to use biosdisk calls only if biosdisk is
> active.  Otherwise, trust the user and turn off validation.
>
I've checked and seen that ata disables biosdisk. This mean that we
need to disable this validation. As for calling biosdisk only if it's
active: "active" isn't well-defined with grub2 and it will add
unnecessary complexity
> --
> Regards,
> Pavel Roskin
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Re: status grub2 port of grub-legasy map command

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 2532 bytes --]

El sáb, 30-05-2009 a las 17:28 +0200, Vladimir 'phcoder' Serbinenko
escribió:
> >> > I'm fine with the change from "const void" to "const char", but we need
> >> > to remove a preceding comment about void labels.
> >> It's not that I'm opposed to void in principle. Just using the same
> >> constructions to do the same things in different files makes code
> >> easier to learn and port
> >
> > I wonder if we can go the other way and use void for all labels without
> > storage.  Indeed, it's too easy to misuse a char variable by forgetting
> > the ampersand before it.
> Not  char []
What if someone sees the code and thinks it's truly a char array? I
insist: you are stirring a tempest in a teapot. Even without taking into
account that "const void label" is more "elegant" and logical for a
simple label than using a char array, the only argument you give is
consistency with the rest of GRUB code. This is an important plus for a
decision, but consistency should never be an argument in and of itself.

Furthermore, a distinct C type for labels like "const void", or even
better, "grub_asm_label_t" would help a programmer identify them all
with a simple "find in files" command, while char arrays may abound and
be tricky to tell apart.
> >
> >> > As for the parse_biosdisk() change, I'd like to see an explanation.
> >> The explanation is that if user uses ata or usbms code and code calls
> >> biosdisk, BIOS may issue a command which may conflict with ata/usbms.
> >> Unfortunately it's not a scenario we're able to circumvent (BIOS is
> >> headache) so I prefer to err on a safe side
> >
> > I agree that we should avoid touching the hardware.  Besides, after
> > loading ata we may not see some drives that BIOS can see.
> >
> > Validation of user input is good, but only if it's implemented
> > correctly.
> >
> > Another approach may be to use biosdisk calls only if biosdisk is
> > active.  Otherwise, trust the user and turn off validation.
> >
> I've checked and seen that ata disables biosdisk. This mean that we
> need to disable this validation. As for calling biosdisk only if it's
> active: "active" isn't well-defined with grub2 and it will add
> unnecessary complexity
Put it that way, I agree with your change. To the scrapper with
parse_biosdisk then! I think the new behaviour should be mentioned in
the drivemap --help command, though I'm too tangled up with finals to be
of any utility right now.

-- 
-- Lazy, Oblivious, Recurrent Disaster -- Habbit

[-- Attachment #2: Esto es una parte de mensaje firmado digitalmente --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: status grub2 port of grub-legasy map command

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1998 bytes --]

2009/5/31 Javier Martín <lordhabbit@gmail.com>:
> El sáb, 30-05-2009 a las 17:28 +0200, Vladimir 'phcoder' Serbinenko
> escribió:
>> >> > I'm fine with the change from "const void" to "const char", but we need
>> >> > to remove a preceding comment about void labels.
>> >> It's not that I'm opposed to void in principle. Just using the same
>> >> constructions to do the same things in different files makes code
>> >> easier to learn and port
>> >
>> > I wonder if we can go the other way and use void for all labels without
>> > storage.  Indeed, it's too easy to misuse a char variable by forgetting
>> > the ampersand before it.
>> Not  char []
> What if someone sees the code and thinks it's truly a char array? I
> insist: you are stirring a tempest in a teapot.
You're right. It turns into discussion about apple and oranges. Let's
forget this part
> Put it that way, I agree with your change. To the scrapper with
> parse_biosdisk then!
Sorry, I didn't mean to upset you. Here is a patch to do it
> I think the new behaviour
It's not a new behaviour on valid inputs since biosdisk does exactly
the same transformation.
> should be mentioned in
> the drivemap --help command, though I'm too tangled up with finals to be
> of any utility right now.
Good luck with your exams

Doing some tests I discovered that freedos MBR (but not the
bootsector) relies on %dl being preserved across int 0x13. In the
patch you can see additional hassle to preserve %dx.
Another problem I discovered is that %dl (chainloader) and bootdrive
(multiboot) passed to payload is out of sync.
Does anyone has a suggestion how to do it cleanly. The problems are:
-chainloader/multiboot shouldn't depend on biosdisk if possible
-%dl should be set accordingly to mapping
-%dl should be correct even if grub2 uses its own drivers (now it's
set to nonsense)
I have some ideas on this subject but would like to hear what others think.

-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #2: biosdisk.diff --]
[-- Type: text/x-patch, Size: 4894 bytes --]

diff --git a/commands/i386/pc/drivemap.c b/commands/i386/pc/drivemap.c
index 98f4378..898fb51 100644
--- a/commands/i386/pc/drivemap.c
+++ b/commands/i386/pc/drivemap.c
@@ -143,62 +143,6 @@ drivemap_remove (grub_uint8_t newdrive)
     }
 }
 
-/* Given a device name, resolves its BIOS disk number and stores it in the
-   passed location, which should only be trusted if ERR_NONE is returned.  */
-static grub_err_t
-parse_biosdisk (const char *name, grub_uint8_t *disknum)
-{
-  grub_disk_t disk;
-  /* Skip the first ( in (hd0) - disk_open wants just the name.  */
-  if (*name == '(')
-    name++;
-
-  disk = grub_disk_open (name);
-  if (! disk)
-    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "unknown device \"%s\"",
-		       name);
-  const enum grub_disk_dev_id id = disk->dev->id;
-  /* The following assignment is only sound if the device is indeed a
-     biosdisk.  The caller must check the return value.  */
-  if (disknum)
-    *disknum = disk->id;
-  grub_disk_close (disk);
-  if (id != GRUB_DISK_DEVICE_BIOSDISK_ID)
-    return grub_error (GRUB_ERR_BAD_DEVICE, "%s is not a BIOS disk", name);
-  return GRUB_ERR_NONE;
-}
-
-/* Given a BIOS disk number, returns its GRUB device name if it exists.
-   If the call succeeds, the resulting device string must be freed.
-   For nonexisting BIOS disk numbers, this function returns
-   GRUB_ERR_UNKNOWN_DEVICE.  */
-static grub_err_t
-revparse_biosdisk (const grub_uint8_t dnum, const char **output)
-{
-  int found = 0;
-  auto int find (const char *name);
-  int find (const char *name)
-  {
-    const grub_disk_t disk = grub_disk_open (name);
-    if (! disk)
-      return 0;
-    if (disk->id == dnum && disk->dev->id == GRUB_DISK_DEVICE_BIOSDISK_ID)
-      {
-	found = 1;
-	if (output)
-	  *output = grub_strdup (name);
-      }
-    grub_disk_close (disk);
-    return found;
-  }
-
-  grub_disk_dev_iterate (find);
-  if (found)
-    return GRUB_ERR_NONE;
-  return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "BIOS disk %02x not found",
-		     dnum);
-}
-
 /* Given a GRUB-like device name and a convenient location, stores the
    related BIOS disk number.  Accepts devices like \((f|h)dN\), with
    0 <= N < 128.  */
@@ -238,15 +182,13 @@ list_mappings (void)
   drivemap_node_t *curnode = map_head;
   while (curnode)
     {
-      const char *dname = 0;
-      grub_err_t err = revparse_biosdisk (curnode->redirto, &dname);
-      if (err != GRUB_ERR_NONE)
-	return err;
-      grub_printf ("%cD #%-3u (0x%02x)       %s\n",
+      grub_printf ("%cD #%-3u (0x%02x)       %cd%d\n",
 		   (curnode->newdrive & 0x80) ? 'H' : 'F',
-		   curnode->newdrive & 0x7F, curnode->newdrive, dname);
+		   curnode->newdrive & 0x7F, curnode->newdrive, 
+		   (curnode->redirto & 0x80) ? 'h' : 'f',
+		   curnode->redirto & 0x7F
+		   );
       curnode = curnode->next;
-      grub_free ((char *) dname);
     }
   return GRUB_ERR_NONE;
 }
@@ -286,17 +228,11 @@ grub_cmd_drivemap (struct grub_extcmd *cmd, int argc, char **args)
   if (argc != 2)
     return grub_error (GRUB_ERR_BAD_ARGUMENT, "two arguments required");
 
-  err = parse_biosdisk (args[0], &mapfrom);
+  err = tryparse_diskstring (args[0], &mapfrom);
   if (err != GRUB_ERR_NONE)
     return err;
 
-  /* When swapping we require both devices to be BIOS disks, but when
-     performing direct mappings we only require the 2nd argument to look
-     like a BIOS disk in order to resolve it into a BIOS disk number.  */
-  if (cmd->state[OPTIDX_SWAP].set)
-    err = parse_biosdisk (args[1], &mapto);
-  else
-    err = tryparse_diskstring (args[1], &mapto);
+  err = tryparse_diskstring (args[1], &mapto);
   if (err != GRUB_ERR_NONE)
     return err;
 
diff --git a/commands/i386/pc/drivemap_int13h.S b/commands/i386/pc/drivemap_int13h.S
index 23b7302..328459a 100644
--- a/commands/i386/pc/drivemap_int13h.S
+++ b/commands/i386/pc/drivemap_int13h.S
@@ -27,6 +27,11 @@
 
 /* The replacement int13 handler.   Preserve all registers.  */
 FUNCTION(grub_drivemap_handler)
+	/* Save %dx for future restore. */
+	push 	%dx
+	/* Push flags. Used to simulate interrupt with original flags. */
+	pushf
+
 	/* Map the drive number (always in DL).  */
 	push	%ax
 	push	%bx
@@ -46,16 +51,19 @@ not_found:
 	pop	%bx
 	pop	%ax
 
-	/* Upon arrival to this point the stack must be exactly like at entry.
-	   This long jump will transfer the caller's stack to the old INT13
-	   handler, thus making it return directly to the original caller.  */
-	.byte	0xea
+	/* lcall  */
+	.byte	0x9a
 
 /* Far pointer to the old handler.  Stored as a CS:IP in the style of real-mode
    IVT entries (thus PI:SC in mem).  */
 VARIABLE(grub_drivemap_oldhandler)
 	.word 0x0, 0x0
 
+	/* Restore %dx. */
+	pop %dx
+	iret
+	
+
 /* This label MUST be at the end of the copied block, since the installer code
    reserves additional space for mappings at runtime and copies them over it.  */
 .align 2

Re: status grub2 port of grub-legasy map command

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 2447 bytes --]

El dom, 31-05-2009 a las 13:36 +0200, Vladimir 'phcoder' Serbinenko
escribió: 
> 2009/5/31 Javier Martín <lordhabbit@gmail.com>:
> > El sáb, 30-05-2009 a las 17:28 +0200, Vladimir 'phcoder' Serbinenko
> > escribió:
> > Put it that way, I agree with your change. To the scrapper with
> > parse_biosdisk then!
> Sorry, I didn't mean to upset you. Here is a patch to do it
You did not upset me at all! I'm sorry if I seemed rude or hostile. I
truly meant to give you a go-ahead at scrapping that code.

> Good luck with your exams
Thank you very much, I'll need it.

> 
> Doing some tests I discovered that freedos MBR (but not the
> bootsector) relies on %dl being preserved across int 0x13. In the
> patch you can see additional hassle to preserve %dx.
Do not do this. Some BIOS functions (like ah=08h) return data in dl.
Clients should not expect data in registers to be preserved across
interrupt calls. I don't know if there is something like a 8086/PC-BIOS
ABI document that we can find to confirm the non-preservation of dl, but
the FreeDOS MBR should be fixed then.

> Another problem I discovered is that %dl (chainloader) and bootdrive
> (multiboot) passed to payload is out of sync.
> Does anyone has a suggestion how to do it cleanly. The problems are:
> -chainloader/multiboot shouldn't depend on biosdisk if possible
> -%dl should be set accordingly to mapping
> -%dl should be correct even if grub2 uses its own drivers (now it's
> set to nonsense)
> I have some ideas on this subject but would like to hear what others think.
I managed to get the right %dl passed with the following procedure:
let's say I want to boot FreeDOS on hd1, but I will drivemap it so that
it will become hd0. Instead of doing this:
root=(hd1,1)
drivemap -s (hd0) (hd1)
chainloader +1
I do the following:
root=(hd0)
drivemap -s (hd0) (hd1)
chainloader (hd1,1)+1
This gets the right number into %dl (I have not checked multiboot), but
I acknowledge that it's nothing more than a hack. I cannot see a way for
drivemap to access that data and modify it according to the mappings
without breaking modularity with multiboot and chainloader (and what
about other loaders?).

Regarding grub2 using its own drivers, we have no reliable way of
conclusively saying that (ata2)==(hd1), so we'd better say nothing at
all and keep dl=ffh as we do currently. 

-- 
-- Lazy, Oblivious, Recurrent Disaster -- Habbit

[-- Attachment #2: Esto es una parte de mensaje firmado digitalmente --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: status grub2 port of grub-legasy map command

[Vladimir 'phcoder' Serbinenko]

2009/5/31 Javier Martín <lordhabbit@gmail.com>:
> El dom, 31-05-2009 a las 13:36 +0200, Vladimir 'phcoder' Serbinenko
> escribió:
>> 2009/5/31 Javier Martín <lordhabbit@gmail.com>:
>> > El sáb, 30-05-2009 a las 17:28 +0200, Vladimir 'phcoder' Serbinenko
>> > escribió:
>> > Put it that way, I agree with your change. To the scrapper with
>> > parse_biosdisk then!
>> Sorry, I didn't mean to upset you. Here is a patch to do it
> You did not upset me at all! I'm sorry if I seemed rude or hostile. I
> truly meant to give you a go-ahead at scrapping that code.
Ok, thanks
>>
>> Doing some tests I discovered that freedos MBR (but not the
>> bootsector) relies on %dl being preserved across int 0x13. In the
>> patch you can see additional hassle to preserve %dx.
> Do not do this. Some BIOS functions (like ah=08h) return data in dl.
> Clients should not expect data in registers to be preserved across
> interrupt calls. I don't know if there is something like a 8086/PC-BIOS
> ABI document that we can find to confirm the non-preservation of dl, but
> the FreeDOS MBR should be fixed then.
Thank you for pointing ah=8 function
AFAIK there is no normative reference. The source which was mainly
used for long years is helppc and it states following:
	- registers DS, BX, CX and DX are preserved
(http://heim.ifi.uio.no/~stanisls/helppc/int_13.html)
But now this reference is terribly outdated.
SeaBIOS preserves %dl too
According to wikipedia ah=8 is the only function which returns in %dl.
I will write an updated version which restores %dx only if ah!=8
> I managed to get the right %dl passed with the following procedure:
> let's say I want to boot FreeDOS on hd1, but I will drivemap it so that
> it will become hd0. Instead of doing this:
> root=(hd1,1)
> drivemap -s (hd0) (hd1)
> chainloader +1
> I do the following:
> root=(hd0)
> drivemap -s (hd0) (hd1)
> chainloader (hd1,1)+1
> This gets the right number into %dl (I have not checked multiboot), but
> I acknowledge that it's nothing more than a hack.
And it sets es:di (pointer to partition table entry) to incorrect value
> I cannot see a way for
> drivemap to access that data and modify it according to the mappings
> without breaking modularity with multiboot and chainloader
I think of something like having a function pointer in kernel
int (*grub_get_biosdisk) (grub_device_t dev) = grub_get_biosdisk_basic;
static int grub_get_biosdisk_basic (grub_device_t dev)
{
    if (! dev)
      return 0xff;
    if (! dev->disk)
      return 0xff;
    if (! dev->disk->dev)
      return 0xff;
    if (dev->disk->dev->id != GRUB_DISK_DEVICE_BIOSDISK_ID)
      return 0xff;
    return (int) dev->disk->id;
}
Which can be easily overridden by drivemap
But I don't really like this approach
> (and what
> about other loaders?).
AFAIR only multiboot and chainloader pass bios drive number to
payload. Other payloads are designed to use their own drivers
(multiboot is designed for both possibilities).
>
> Regarding grub2 using its own drivers, we have no reliable way of
> conclusively saying that (ata2)==(hd1), so we'd better say nothing at
> all and keep dl=ffh as we do currently.
I thought of introducing a variable "biosdrive" and doing something like
root=ata1
biosdrive=0x81
chainloader +1
boot
Setting %dl to 0xff if drive can't be determined reliably and isn't
supplied by user is a good possibility. I'll do so.
>
> --
> -- Lazy, Oblivious, Recurrent Disaster -- Habbit
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Re: status grub2 port of grub-legasy map command

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 4668 bytes --]

El dom, 31-05-2009 a las 18:00 +0200, Vladimir 'phcoder' Serbinenko
escribió:
> 2009/5/31 Javier Martín <lordhabbit@gmail.com>:
> > El dom, 31-05-2009 a las 13:36 +0200, Vladimir 'phcoder' Serbinenko
> > escribió:
> >> Doing some tests I discovered that freedos MBR (but not the
> >> bootsector) relies on %dl being preserved across int 0x13. In the
> >> patch you can see additional hassle to preserve %dx.
> > Do not do this. Some BIOS functions (like ah=08h) return data in dl.
> > Clients should not expect data in registers to be preserved across
> > interrupt calls. I don't know if there is something like a 8086/PC-BIOS
> > ABI document that we can find to confirm the non-preservation of dl, but
> > the FreeDOS MBR should be fixed then.
> Thank you for pointing ah=8 function
> AFAIK there is no normative reference. The source which was mainly
> used for long years is helppc and it states following:
> 	- registers DS, BX, CX and DX are preserved
> (http://heim.ifi.uio.no/~stanisls/helppc/int_13.html)
> But now this reference is terribly outdated.
> SeaBIOS preserves %dl too
> According to wikipedia ah=8 is the only function which returns in %dl.
> I will write an updated version which restores %dx only if ah!=8
Do not waste your time doing so. There are many other functions that
return data in DX, some of them specific to certain systems or TSRs -
check Ralf Brown's Interrupt List [0] for a possibly very incomplete
reference. The fuss about DS, BS, CX and DX being preserved is quite
false, as even Wikipedia shows at [[INT 13]]: just check ah=41h, which
returns in both BX and CX.

The only semi-rational action we can take is to reverse the mapping
_only_ if %dl still contains the mapped data we passed into the int13
handler, but that leaves the question - how do we know that some 0x81 is
actually the 0x81 we put there in substitution of the 0x80 the payload
called with, and not the true result of the call?

> > I managed to get the right %dl passed with the following procedure:
> > let's say I want to boot FreeDOS on hd1, but I will drivemap it so that
> > it will become hd0. Instead of doing this:
> > root=(hd1,1)
> > drivemap -s (hd0) (hd1)
> > chainloader +1
> > I do the following:
> > root=(hd0)
> > drivemap -s (hd0) (hd1)
> > chainloader (hd1,1)+1
> > This gets the right number into %dl (I have not checked multiboot), but
> > I acknowledge that it's nothing more than a hack.
> And it sets es:di (pointer to partition table entry) to incorrect value
I have yet to find a bootsector that uses such information. IIrc, there
were so many buggy BIOSes out there that it was unreliable anyways: MBRs
just check their own memory and bootsectors forget about it entirely.
Nevertheless, this issue deserves a deeper look.

> > I cannot see a way for
> > drivemap to access that data and modify it according to the mappings
> > without breaking modularity with multiboot and chainloader
> I think of something like having a function pointer in kernel
> int (*grub_get_biosdisk) (grub_device_t dev) = grub_get_biosdisk_basic;
> static int grub_get_biosdisk_basic (grub_device_t dev)
> {
>     if (! dev)
>       return 0xff;
>     if (! dev->disk)
>       return 0xff;
>     if (! dev->disk->dev)
>       return 0xff;
>     if (dev->disk->dev->id != GRUB_DISK_DEVICE_BIOSDISK_ID)
>       return 0xff;
>     return (int) dev->disk->id;
> }
> Which can be easily overridden by drivemap
> But I don't really like this approach
Neither do I, but it might be the only non-100%-hackish approach. I
think this part should be addressed by a wider audience (coughs at the
passive listeners in the list).

> > Regarding grub2 using its own drivers, we have no reliable way of
> > conclusively saying that (ata2)==(hd1), so we'd better say nothing at
> > all and keep dl=ffh as we do currently.
> I thought of introducing a variable "biosdrive" and doing something like
> root=ata1
> biosdrive=0x81
> chainloader +1
> boot
> Setting %dl to 0xff if drive can't be determined reliably and isn't
> supplied by user is a good possibility. I'll do so.
Er... AfaIk that's what chainloader and multiboot do when they cannot
determine the boot disk. Regarding that "biosdrive", it would add more
complexity to what already is a non-simple system. I'd say that either
this mapping gets done automagically or the current hack is better,
since it is so obviously a hack that if we cannot solve it future
developers will still have an itch to get rid of it.


[0] http://www.ctyme.com/intr/int-13.htm

-- 
-- Lazy, Oblivious, Recurrent Disaster -- Habbit

[-- Attachment #2: Esto es una parte de mensaje firmado digitalmente --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: status grub2 port of grub-legasy map command

[Vladimir 'phcoder' Serbinenko]

> Do not waste your time doing so. There are many other functions that
> return data in DX, some of them specific to certain systems or TSRs -
> check Ralf Brown's Interrupt List [0] for a possibly very incomplete
> reference. The fuss about DS, BS, CX and DX being preserved is quite
> false,
It's meant "preserved unless otherwise stated"
> as even Wikipedia shows at [[INT 13]]: just check ah=41h, which
> returns in both BX and CX.
>
> The only semi-rational action we can take is to reverse the mapping
> _only_ if %dl still contains the mapped data we passed into the int13
> handler, but that leaves the question - how do we know that some 0x81 is
> actually the 0x81 we put there in substitution of the 0x80 the payload
> called with, and not the true result of the call?
>
Or we can maintain a list of interrupts under which we restore %dx. It
would contain only well documented BIOS interrupts and not TSR ones.
This approach will work with normal interrupts and has high chance of
working with TSR. But I don't think TSR compatibility is important.
> I have yet to find a bootsector that uses such information.
AFAIR CHS FAT32 FreeDOS sector does
> IIrc, there
> were so many buggy BIOSes out there that it was unreliable anyways: MBRs
> just check their own memory and bootsectors forget about it entirely.
> Nevertheless, this issue deserves a deeper look.
>> > Regarding grub2 using its own drivers, we have no reliable way of
>> > conclusively saying that (ata2)==(hd1), so we'd better say nothing at
>> > all and keep dl=ffh as we do currently.
>> I thought of introducing a variable "biosdrive" and doing something like
>> root=ata1
>> biosdrive=0x81
>> chainloader +1
>> boot
>> Setting %dl to 0xff if drive can't be determined reliably and isn't
>> supplied by user is a good possibility. I'll do so.
> Er... AfaIk that's what chainloader and multiboot do when they cannot
> determine the boot disk.
Chainloader just fills %dl with garbage
> Regarding that "biosdrive", it would add more
> complexity to what already is a non-simple system. I'd say that either
> this mapping gets done automagically
biosdrive is necessary only when authomatic routines fail (no or wrong result)
>
> [0] http://www.ctyme.com/intr/int-13.htm
>
Thanks for a useful reference
> --
> -- Lazy, Oblivious, Recurrent Disaster -- Habbit
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Re: status grub2 port of grub-legasy map command

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 1512 bytes --]

El dom, 31-05-2009 a las 20:05 +0200, Vladimir 'phcoder' Serbinenko
escribió:
> > The only semi-rational action we can take is to reverse the mapping
> > _only_ if %dl still contains the mapped data we passed into the int13
> > handler, but that leaves the question - how do we know that some 0x81 is
> > actually the 0x81 we put there in substitution of the 0x80 the payload
> > called with, and not the true result of the call?
> >
> Or we can maintain a list of interrupts under which we restore %dx. It
> would contain only well documented BIOS interrupts and not TSR ones.
> This approach will work with normal interrupts and has high chance of
> working with TSR. But I don't think TSR compatibility is important.
> > I have yet to find a bootsector that uses such information.
> AFAIR CHS FAT32 FreeDOS sector does
No, it doesn't: check it out. It overwrites es and di quite soon indeed:
  real_start:	cld
		cli
 		sub	ax, ax
 		mov	ds, ax
 		mov     bp, 0x7c00
 
		mov	ax, 0x1FE0
 		mov	es, ax
 		mov	si, bp
 		mov	di, bp
Same applies to the other two FreeDOS bootsectors.

> > Regarding that "biosdrive", it would add more
> > complexity to what already is a non-simple system. I'd say that either
> > this mapping gets done automagically
> biosdrive is necessary only when authomatic routines fail (no or wrong result)
The point is writing the automatic routine you described so that
biosdrive is not required.

-- 
-- Lazy, Oblivious, Recurrent Disaster -- Habbit

[-- Attachment #2: Esto es una parte de mensaje firmado digitalmente --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: status grub2 port of grub-legasy map command

[Vladimir 'phcoder' Serbinenko]

>> AFAIR CHS FAT32 FreeDOS sector does
> No, it doesn't: check it out. It overwrites es and di quite soon indeed:
>  real_start:   cld
>                cli
>                sub     ax, ax
>                mov     ds, ax
>                mov     bp, 0x7c00
>
>                mov     ax, 0x1FE0
>                mov     es, ax
>                mov     si, bp
>                mov     di, bp
> Same applies to the other two FreeDOS bootsectors.
Ok. I was wrong.
>
>> > Regarding that "biosdrive", it would add more
>> > complexity to what already is a non-simple system. I'd say that either
>> > this mapping gets done automagically
>> biosdrive is necessary only when authomatic routines fail (no or wrong result)
> The point is writing the automatic routine you described so that
> biosdrive is not required.
>
If this was reliable then yes but as it will never be user must be
able to override it
> --
> -- Lazy, Oblivious, Recurrent Disaster -- Habbit
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Re: status grub2 port of grub-legasy map command

[Christian Franke]

Vladimir 'phcoder' Serbinenko wrote:
>>
>> Do not do this. Some BIOS functions (like ah=08h) return data in dl.
>> Clients should not expect data in registers to be preserved across
>> interrupt calls. I don't know if there is something like a 8086/PC-BIOS
>> ABI document that we can find to confirm the non-preservation of dl, but
>> the FreeDOS MBR should be fixed then.
>>     
> Thank you for pointing ah=8 function
> AFAIK there is no normative reference. The source which was mainly
> used for long years is helppc and it states following:
> 	- registers DS, BX, CX and DX are preserved
> (http://heim.ifi.uio.no/~stanisls/helppc/int_13.html)
> But now this reference is terribly outdated.
> SeaBIOS preserves %dl too
>   

T13 EDD provides a probably more up to date documentation of int13.

There was no requirement to preserve registers is in EDD (2000) and 
EDD-2 (2002). It appeared in EDD-3 (2004) and is still in first EDD-4 
draft (2009):

"The values in all registers that are not explicitly defined in the 
following sections shall be preserved at the completion of each function 
call"
 From Section 8 of: BIOS Enhanced Disk Drive Services - 3 (T13/1572D 
Revision 3)

Fortunately, T13 docs are (unlike T10 and SATA-IO) still publicly available:
http://www.t13.org/Documents/MinutesDefault.aspx?DocumentType=4&DocumentStage=2

-- 
Regards,
Christian Franke

Re: status grub2 port of grub-legasy map command

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 2172 bytes --]

El dom, 31-05-2009 a las 21:35 +0200, Christian Franke escribió:
> Vladimir 'phcoder' Serbinenko wrote:
> >>
> >> Do not do this. Some BIOS functions (like ah=08h) return data in dl.
> >> Clients should not expect data in registers to be preserved across
> >> interrupt calls. I don't know if there is something like a 8086/PC-BIOS
> >> ABI document that we can find to confirm the non-preservation of dl, but
> >> the FreeDOS MBR should be fixed then.
> >>     
> > Thank you for pointing ah=8 function
> > AFAIK there is no normative reference. The source which was mainly
> > used for long years is helppc and it states following:
> > 	- registers DS, BX, CX and DX are preserved
> > (http://heim.ifi.uio.no/~stanisls/helppc/int_13.html)
> > But now this reference is terribly outdated.
> > SeaBIOS preserves %dl too
> >   
> 
> T13 EDD provides a probably more up to date documentation of int13.
> 
> There was no requirement to preserve registers is in EDD (2000) and 
> EDD-2 (2002). It appeared in EDD-3 (2004) and is still in first EDD-4 
> draft (2009):
> 
> "The values in all registers that are not explicitly defined in the 
> following sections shall be preserved at the completion of each function 
> call"
>  From Section 8 of: BIOS Enhanced Disk Drive Services - 3 (T13/1572D 
> Revision 3)
> 
> Fortunately, T13 docs are (unlike T10 and SATA-IO) still publicly available:
> http://www.t13.org/Documents/MinutesDefault.aspx?DocumentType=4&DocumentStage=2
> 

Hmm... from those docs, and accepting that we ignore TSRs, we need to
save %ah and %dl at handler entry, then check the saved %ah at exit,
like the old handler from GRUB Legacy did - by the way, when writing the
new handler, I asked what that code did and noone was able to tell me ¬¬

The only functions in the standard that return in %dl are 08h and 15h,
so the check should be simple. If we want to be even more extensible, we
could have a 32-byte bitmap, one bit per %ah function, and restore %dl
depending on the value of the particular bit. However, I think that
would be going too far.

-- 
-- Lazy, Oblivious, Recurrent Disaster -- Habbit

[-- Attachment #2: Esto es una parte de mensaje firmado digitalmente --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: status grub2 port of grub-legasy map command

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 985 bytes --]

> Hmm... from those docs, and accepting that we ignore TSRs, we need to
> save %ah and %dl at handler entry, then check the saved %ah at exit,
> like the old handler from GRUB Legacy did - by the way, when writing the
> new handler, I asked what that code did and noone was able to tell me ¬¬
I've the check before calling original handler. Let's hope it makes
code more readable.
>
> The only functions in the standard that return in %dl are 08h and 15h,
> so the check should be simple. If we want to be even more extensible, we
> could have a 32-byte bitmap, one bit per %ah function, and restore %dl
> depending on the value of the particular bit. However, I think that
> would be going too far.
>
> --
> -- Lazy, Oblivious, Recurrent Disaster -- Habbit
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #2: drivemap.diff --]
[-- Type: text/x-diff, Size: 5178 bytes --]

diff --git a/commands/i386/pc/drivemap.c b/commands/i386/pc/drivemap.c
index 98f4378..898fb51 100644
--- a/commands/i386/pc/drivemap.c
+++ b/commands/i386/pc/drivemap.c
@@ -143,62 +143,6 @@ drivemap_remove (grub_uint8_t newdrive)
     }
 }
 
-/* Given a device name, resolves its BIOS disk number and stores it in the
-   passed location, which should only be trusted if ERR_NONE is returned.  */
-static grub_err_t
-parse_biosdisk (const char *name, grub_uint8_t *disknum)
-{
-  grub_disk_t disk;
-  /* Skip the first ( in (hd0) - disk_open wants just the name.  */
-  if (*name == '(')
-    name++;
-
-  disk = grub_disk_open (name);
-  if (! disk)
-    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "unknown device \"%s\"",
-		       name);
-  const enum grub_disk_dev_id id = disk->dev->id;
-  /* The following assignment is only sound if the device is indeed a
-     biosdisk.  The caller must check the return value.  */
-  if (disknum)
-    *disknum = disk->id;
-  grub_disk_close (disk);
-  if (id != GRUB_DISK_DEVICE_BIOSDISK_ID)
-    return grub_error (GRUB_ERR_BAD_DEVICE, "%s is not a BIOS disk", name);
-  return GRUB_ERR_NONE;
-}
-
-/* Given a BIOS disk number, returns its GRUB device name if it exists.
-   If the call succeeds, the resulting device string must be freed.
-   For nonexisting BIOS disk numbers, this function returns
-   GRUB_ERR_UNKNOWN_DEVICE.  */
-static grub_err_t
-revparse_biosdisk (const grub_uint8_t dnum, const char **output)
-{
-  int found = 0;
-  auto int find (const char *name);
-  int find (const char *name)
-  {
-    const grub_disk_t disk = grub_disk_open (name);
-    if (! disk)
-      return 0;
-    if (disk->id == dnum && disk->dev->id == GRUB_DISK_DEVICE_BIOSDISK_ID)
-      {
-	found = 1;
-	if (output)
-	  *output = grub_strdup (name);
-      }
-    grub_disk_close (disk);
-    return found;
-  }
-
-  grub_disk_dev_iterate (find);
-  if (found)
-    return GRUB_ERR_NONE;
-  return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "BIOS disk %02x not found",
-		     dnum);
-}
-
 /* Given a GRUB-like device name and a convenient location, stores the
    related BIOS disk number.  Accepts devices like \((f|h)dN\), with
    0 <= N < 128.  */
@@ -238,15 +182,13 @@ list_mappings (void)
   drivemap_node_t *curnode = map_head;
   while (curnode)
     {
-      const char *dname = 0;
-      grub_err_t err = revparse_biosdisk (curnode->redirto, &dname);
-      if (err != GRUB_ERR_NONE)
-	return err;
-      grub_printf ("%cD #%-3u (0x%02x)       %s\n",
+      grub_printf ("%cD #%-3u (0x%02x)       %cd%d\n",
 		   (curnode->newdrive & 0x80) ? 'H' : 'F',
-		   curnode->newdrive & 0x7F, curnode->newdrive, dname);
+		   curnode->newdrive & 0x7F, curnode->newdrive, 
+		   (curnode->redirto & 0x80) ? 'h' : 'f',
+		   curnode->redirto & 0x7F
+		   );
       curnode = curnode->next;
-      grub_free ((char *) dname);
     }
   return GRUB_ERR_NONE;
 }
@@ -286,17 +228,11 @@ grub_cmd_drivemap (struct grub_extcmd *cmd, int argc, char **args)
   if (argc != 2)
     return grub_error (GRUB_ERR_BAD_ARGUMENT, "two arguments required");
 
-  err = parse_biosdisk (args[0], &mapfrom);
+  err = tryparse_diskstring (args[0], &mapfrom);
   if (err != GRUB_ERR_NONE)
     return err;
 
-  /* When swapping we require both devices to be BIOS disks, but when
-     performing direct mappings we only require the 2nd argument to look
-     like a BIOS disk in order to resolve it into a BIOS disk number.  */
-  if (cmd->state[OPTIDX_SWAP].set)
-    err = parse_biosdisk (args[1], &mapto);
-  else
-    err = tryparse_diskstring (args[1], &mapto);
+  err = tryparse_diskstring (args[1], &mapto);
   if (err != GRUB_ERR_NONE)
     return err;
 
diff --git a/commands/i386/pc/drivemap_int13h.S b/commands/i386/pc/drivemap_int13h.S
index 23b7302..11a5fe1 100644
--- a/commands/i386/pc/drivemap_int13h.S
+++ b/commands/i386/pc/drivemap_int13h.S
@@ -27,6 +27,11 @@
 
 /* The replacement int13 handler.   Preserve all registers.  */
 FUNCTION(grub_drivemap_handler)
+	/* Save %dx for future restore. */
+	push 	%dx
+	/* Push flags. Used to simulate interrupt with original flags. */
+	pushf
+
 	/* Map the drive number (always in DL).  */
 	push	%ax
 	push	%bx
@@ -46,10 +51,53 @@ not_found:
 	pop	%bx
 	pop	%ax
 
-	/* Upon arrival to this point the stack must be exactly like at entry.
-	   This long jump will transfer the caller's stack to the old INT13
-	   handler, thus making it return directly to the original caller.  */
-	.byte	0xea
+	cmpb 	$0x8, %ah
+	jz	norestore
+	cmpb 	$0x15, %ah
+	jz	norestore
+
+	/* Restore flags.  */
+	popf
+	pushf
+
+	lcall *%cs:INT13H_OFFSET (EXT_C (grub_drivemap_oldhandler))
+
+	push 	%bp
+	mov 	%sp, %bp
+	
+	pushf
+	pop 	%dx
+	mov 	%dx, 8(%bp)
+
+	pop	%bp
+	
+	/* Restore %dx.  */
+	pop 	%dx
+	iret
+
+norestore:
+
+	/* Restore flags.  */
+	popf
+	pushf
+
+	lcall *%cs:INT13H_OFFSET (EXT_C (grub_drivemap_oldhandler))
+
+	push 	%bp
+	mov 	%sp, %bp
+	
+	/* Save %dx.  */
+	mov 	%dx, 2(%bp)
+	
+	pushf
+	pop 	%dx
+	mov 	%dx, 8(%bp)
+
+	pop	%bp
+
+	/* Restore %dx.  */
+	pop 	%dx
+	iret
 
 /* Far pointer to the old handler.  Stored as a CS:IP in the style of real-mode
    IVT entries (thus PI:SC in mem).  */

Re: status grub2 port of grub-legasy map command

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 743 bytes --]

El lun, 01-06-2009 a las 11:53 +0200, Vladimir 'phcoder' Serbinenko
escribió:
> > Hmm... from those docs, and accepting that we ignore TSRs, we need to
> > save %ah and %dl at handler entry, then check the saved %ah at exit,
> > like the old handler from GRUB Legacy did - by the way, when writing the
> > new handler, I asked what that code did and noone was able to tell me ¬¬
> I've the check before calling original handler. Let's hope it makes
> code more readable.
Personally, I don't think the readability gain is so big to offset the
code size increase, but this is just a single opinion. I think the other
part of the patch should be committed right now, though.

-- 
-- Lazy, Oblivious, Recurrent Disaster -- Habbit

[-- Attachment #2: Esto es una parte de mensaje firmado digitalmente --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: status grub2 port of grub-legasy map command

[Vladimir 'phcoder' Serbinenko]

2009/6/1 Javier Martín <lordhabbit@gmail.com>:
> El lun, 01-06-2009 a las 11:53 +0200, Vladimir 'phcoder' Serbinenko
> escribió:
>> > Hmm... from those docs, and accepting that we ignore TSRs, we need to
>> > save %ah and %dl at handler entry, then check the saved %ah at exit,
>> > like the old handler from GRUB Legacy did - by the way, when writing the
>> > new handler, I asked what that code did and noone was able to tell me ¬¬
>> I've the check before calling original handler. Let's hope it makes
>> code more readable.
> Personally, I don't think the readability gain is so big to offset the
> code size increase, but this is just a single opinion.
I personally don't really care how to do it. But for economy of effort
I prefer to take what I already have :) But if you insist I can change
it
> I think the other
> part of the patch should be committed right now, though.
This might be a problem ;) (we still have no repository online:( )
>
> --
> -- Lazy, Oblivious, Recurrent Disaster -- Habbit
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Re: status grub2 port of grub-legasy map command

[Vladimir 'phcoder' Serbinenko]

>> I think the other
>> part of the patch should be committed right now, though.
> This might be a problem ;) (we still have no repository online:( )
Commited



-- 
Regards
Vladimir 'phcoder' Serbinenko

Re: status grub2 port of grub-legasy map command

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1705 bytes --]

On Mon, Jun 1, 2009 at 11:45 PM, Vladimir 'phcoder'
Serbinenko<phcoder@gmail.com> wrote:
> 2009/6/1 Javier Martín <lordhabbit@gmail.com>:
>> El lun, 01-06-2009 a las 11:53 +0200, Vladimir 'phcoder' Serbinenko
>> escribió:
>>> > Hmm... from those docs, and accepting that we ignore TSRs, we need to
>>> > save %ah and %dl at handler entry, then check the saved %ah at exit,
>>> > like the old handler from GRUB Legacy did - by the way, when writing the
>>> > new handler, I asked what that code did and noone was able to tell me ¬¬
>>> I've the check before calling original handler. Let's hope it makes
>>> code more readable.
>> Personally, I don't think the readability gain is so big to offset the
>> code size increase, but this is just a single opinion.
> I personally don't really care how to do it. But for economy of effort
> I prefer to take what I already have :) But if you insist I can change
> it
I found a way to make it smaller. (a trick with jump). See the attached patch.
I also solved the %dl problem (at least partially). I've put biosnum
routine to boot.mod because 3 of 5 loaders need it on pc so I think
boot.mod is better then a separate module. In the same time it doesn't
go to kernel. This move forces to put boot.mod to platform-specific
files but this inflexibility of build system is a subject for another
patch
>> --
>> -- Lazy, Oblivious, Recurrent Disaster -- Habbit
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>
>>
>
>
>
> --
> Regards
> Vladimir 'phcoder' Serbinenko
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #2: drivemap.diff --]
[-- Type: text/x-patch, Size: 15531 bytes --]

diff --git a/commands/i386/pc/drivemap.c b/commands/i386/pc/drivemap.c
index 898fb51..97a1936 100644
--- a/commands/i386/pc/drivemap.c
+++ b/commands/i386/pc/drivemap.c
@@ -23,8 +23,9 @@
 #include <grub/misc.h>
 #include <grub/disk.h>
 #include <grub/loader.h>
+#include <grub/env.h>
 #include <grub/machine/memory.h>
-
+#include <grub/machine/biosnum.h>
 
 
 /* Real mode IVT slot (seg:off far pointer) for interrupt 0x13.  */
@@ -356,10 +357,49 @@ uninstall_int13_handler (void)
   return GRUB_ERR_NONE;
 }
 
+static int
+grub_get_root_biosnumber_drivemap (void)
+{
+  char *biosnum;
+  int ret = -1;
+  grub_device_t dev;
+
+  biosnum = grub_env_get ("biosnum");
+
+  if (biosnum)
+    return grub_strtoul (biosnum, 0, 0);
+
+  dev = grub_device_open (0);
+  if (dev && dev->disk && dev->disk->dev 
+      && grub_strcmp (dev->disk->dev->name, "biosdisk") == 0)
+    {
+      drivemap_node_t *curnode = map_head;
+      ret = (int) dev->disk->id;
+      while (curnode)
+	{
+	  if (curnode->redirto == ret)
+	    {
+	      ret = curnode->newdrive;
+	      break;
+	    }
+	  curnode = curnode->next;
+	}
+
+    }
+
+  if (dev)
+    grub_device_close (dev);
+
+  return ret;
+}
+
 static grub_extcmd_t cmd;
+static int (*grub_get_root_biosnumber_saved) (void);
 
 GRUB_MOD_INIT (drivemap)
 {
+  grub_get_root_biosnumber_saved = grub_get_root_biosnumber;
+  grub_get_root_biosnumber = grub_get_root_biosnumber_drivemap;
   cmd = grub_register_extcmd ("drivemap", grub_cmd_drivemap,
 					GRUB_COMMAND_FLAG_BOTH,
 					"drivemap"
@@ -374,6 +414,7 @@ GRUB_MOD_INIT (drivemap)
 
 GRUB_MOD_FINI (drivemap)
 {
+  grub_get_root_biosnumber = grub_get_root_biosnumber_saved;
   grub_loader_unregister_preboot_hook (drivemap_hook);
   drivemap_hook = 0;
   grub_unregister_extcmd (cmd);
diff --git a/commands/i386/pc/drivemap_int13h.S b/commands/i386/pc/drivemap_int13h.S
index 7a3e8e7..ba31a9c 100644
--- a/commands/i386/pc/drivemap_int13h.S
+++ b/commands/i386/pc/drivemap_int13h.S
@@ -27,6 +27,11 @@
 
 /* The replacement int13 handler.   Preserve all registers.  */
 FUNCTION(grub_drivemap_handler)
+	/* Save %dx for future restore. */
+	push 	%dx
+	/* Push flags. Used to simulate interrupt with original flags. */
+	pushf
+
 	/* Map the drive number (always in DL).  */
 	push	%ax
 	push	%bx
@@ -51,11 +56,48 @@ not_found:
 	pop	%bx
 	pop	%ax
 
-	/* Upon arrival to this point the stack must be exactly like at entry.
-	   This long jump will transfer the caller's stack to the old INT13
-	   handler, thus making it return directly to the original caller.  */
-	.byte	0xea
+	cmpb 	$0x8, %ah
+	jz	norestore
+	cmpb 	$0x15, %ah
+	jz	norestore
+
+	/* Restore flags.  */
+	popf
+	pushf
+
+	lcall *%cs:INT13H_OFFSET (EXT_C (grub_drivemap_oldhandler))
+	
+	push 	%bp
+	mov 	%sp, %bp
+
+postamble:	
+	
+	pushf
+	pop 	%dx
+	mov 	%dx, 8(%bp)
+
+	pop	%bp
+	
+	/* Restore %dx.  */
+	pop 	%dx
+	iret
+
+norestore:
+
+	/* Restore flags.  */
+	popf
+	pushf
+
+	lcall *%cs:INT13H_OFFSET (EXT_C (grub_drivemap_oldhandler))
+
+	push 	%bp
+	mov 	%sp, %bp
+	
+	/* Save %dx.  */
+	mov 	%dx, 2(%bp)
 
+	jmp postamble
+	
 /* Far pointer to the old handler.  Stored as a CS:IP in the style of real-mode
    IVT entries (thus PI:SC in mem).  */
 VARIABLE(grub_drivemap_oldhandler)
diff --git a/conf/common.rmk b/conf/common.rmk
index 3e59c3a..b328bc4 100644
--- a/conf/common.rmk
+++ b/conf/common.rmk
@@ -353,19 +353,13 @@ pkglib_MODULES += minicmd.mod extcmd.mod hello.mod handler.mod	\
 	loopback.mod fs_uuid.mod configfile.mod echo.mod	\
 	terminfo.mod test.mod blocklist.mod hexdump.mod		\
 	read.mod sleep.mod loadenv.mod crc.mod parttool.mod	\
-	pcpart.mod memrw.mod boot.mod normal.mod sh.mod lua.mod	\
-	gptsync.mod
+	pcpart.mod memrw.mod normal.mod sh.mod lua.mod gptsync.mod
 
 # For gptsync.mod.
 gptsync_mod_SOURCES = commands/gptsync.c
 gptsync_mod_CFLAGS = $(COMMON_CFLAGS)
 gptsync_mod_LDFLAGS = $(COMMON_LDFLAGS)
 
-# For boot.mod.
-boot_mod_SOURCES = commands/boot.c
-boot_mod_CFLAGS = $(COMMON_CFLAGS)
-boot_mod_LDFLAGS = $(COMMON_LDFLAGS)
-
 # For minicmd.mod.
 minicmd_mod_SOURCES = commands/minicmd.c
 minicmd_mod_CFLAGS = $(COMMON_CFLAGS)
diff --git a/conf/i386-coreboot.rmk b/conf/i386-coreboot.rmk
index 2f768c0..483f279 100644
--- a/conf/i386-coreboot.rmk
+++ b/conf/i386-coreboot.rmk
@@ -109,6 +109,12 @@ pkglib_MODULES = linux.mod multiboot.mod 		\
 	halt.mod datetime.mod date.mod datehook.mod	\
 	lsmmap.mod mmap.mod
 
+# For boot.mod.
+pkglib_MODULES += boot.mod 
+boot_mod_SOURCES = commands/boot.c
+boot_mod_CFLAGS = $(COMMON_CFLAGS)
+boot_mod_LDFLAGS = $(COMMON_LDFLAGS)
+
 # For mmap.mod.
 mmap_mod_SOURCES = mmap/mmap.c mmap/i386/uppermem.c mmap/i386/mmap.c
 mmap_mod_CFLAGS = $(COMMON_CFLAGS)
diff --git a/conf/i386-efi.rmk b/conf/i386-efi.rmk
index 91e271d..d146733 100644
--- a/conf/i386-efi.rmk
+++ b/conf/i386-efi.rmk
@@ -117,6 +117,12 @@ symlist.c: $(addprefix include/grub/,$(kernel_mod_HEADERS)) config.h gensymlist.
 kernel_syms.lst: $(addprefix include/grub/,$(kernel_mod_HEADERS)) config.h genkernsyms.sh
 	/bin/sh genkernsyms.sh $(filter %.h,$^) > $@ || (rm -f $@; exit 1)
 
+# For boot.mod.
+pkglib_MODULES += boot.mod 
+boot_mod_SOURCES = commands/boot.c
+boot_mod_CFLAGS = $(COMMON_CFLAGS)
+boot_mod_LDFLAGS = $(COMMON_LDFLAGS)
+
 # For acpi.mod.
 acpi_mod_SOURCES = commands/acpi.c commands/efi/acpi.c
 acpi_mod_CFLAGS = $(COMMON_CFLAGS)
diff --git a/conf/i386-ieee1275.rmk b/conf/i386-ieee1275.rmk
index 961e616..6e91b42 100644
--- a/conf/i386-ieee1275.rmk
+++ b/conf/i386-ieee1275.rmk
@@ -108,6 +108,12 @@ pkglib_MODULES = halt.mod reboot.mod suspend.mod		\
 	nand.mod memdisk.mod pci.mod lspci.mod datetime.mod	\
 	date.mod datehook.mod lsmmap.mod mmap.mod
 
+# For boot.mod.
+pkglib_MODULES += boot.mod 
+boot_mod_SOURCES = commands/boot.c
+boot_mod_CFLAGS = $(COMMON_CFLAGS)
+boot_mod_LDFLAGS = $(COMMON_LDFLAGS)
+
 # For mmap.mod.
 mmap_mod_SOURCES = mmap/mmap.c mmap/i386/uppermem.c mmap/i386/mmap.c
 mmap_mod_CFLAGS = $(COMMON_CFLAGS)
diff --git a/conf/i386-pc.rmk b/conf/i386-pc.rmk
index fe67a8f..bebb571 100644
--- a/conf/i386-pc.rmk
+++ b/conf/i386-pc.rmk
@@ -189,6 +189,12 @@ pkglib_MODULES = biosdisk.mod chain.mod \
 	usb.mod uhci.mod ohci.mod usbtest.mod usbms.mod usb_keyboard.mod \
 	efiemu.mod mmap.mod acpi.mod drivemap.mod
 
+# For boot.mod.
+pkglib_MODULES += boot.mod 
+boot_mod_SOURCES = commands/boot.c lib/i386/pc/biosnum.c
+boot_mod_CFLAGS = $(COMMON_CFLAGS)
+boot_mod_LDFLAGS = $(COMMON_LDFLAGS)
+
 # For drivemap.mod.
 drivemap_mod_SOURCES = commands/i386/pc/drivemap.c \
                        commands/i386/pc/drivemap_int13h.S
@@ -445,3 +451,7 @@ endif
 
 include $(srcdir)/conf/i386.mk
 include $(srcdir)/conf/common.mk
+
+ifeq (0,1)
+pkglib_MODULES += boot.mod
+endif
diff --git a/conf/powerpc-ieee1275.rmk b/conf/powerpc-ieee1275.rmk
index 8b5be2b..a15f5b2 100644
--- a/conf/powerpc-ieee1275.rmk
+++ b/conf/powerpc-ieee1275.rmk
@@ -119,6 +119,12 @@ pkglib_MODULES = halt.mod \
 	memdisk.mod \
 	lsmmap.mod
 
+# For boot.mod.
+pkglib_MODULES += boot.mod 
+boot_mod_SOURCES = commands/boot.c lib/i386/pc/biosnum.c
+boot_mod_CFLAGS = $(COMMON_CFLAGS)
+boot_mod_LDFLAGS = $(COMMON_LDFLAGS)
+
 # For linux.mod.
 linux_mod_SOURCES = loader/powerpc/ieee1275/linux.c
 linux_mod_CFLAGS = $(COMMON_CFLAGS)
diff --git a/conf/sparc64-ieee1275.rmk b/conf/sparc64-ieee1275.rmk
index d16501b..f84b281 100644
--- a/conf/sparc64-ieee1275.rmk
+++ b/conf/sparc64-ieee1275.rmk
@@ -149,6 +149,12 @@ pkglib_MODULES = halt.mod \
 	memdisk.mod \
 	lsmmap.mod
 
+# For boot.mod.
+pkglib_MODULES += boot.mod 
+boot_mod_SOURCES = commands/boot.c lib/i386/pc/biosnum.c
+boot_mod_CFLAGS = $(COMMON_CFLAGS)
+boot_mod_LDFLAGS = $(COMMON_LDFLAGS)
+
 # For linux.mod.
 linux_mod_SOURCES = loader/sparc64/ieee1275/linux.c
 linux_mod_CFLAGS = $(COMMON_CFLAGS)
diff --git a/conf/x86_64-efi.rmk b/conf/x86_64-efi.rmk
index 175f8cc..82d0005 100644
--- a/conf/x86_64-efi.rmk
+++ b/conf/x86_64-efi.rmk
@@ -115,6 +115,12 @@ symlist.c: $(addprefix include/grub/,$(kernel_mod_HEADERS)) config.h gensymlist.
 kernel_syms.lst: $(addprefix include/grub/,$(kernel_mod_HEADERS)) config.h genkernsyms.sh
 	/bin/sh genkernsyms.sh $(filter %.h,$^) > $@ || (rm -f $@; exit 1)
 
+# For boot.mod.
+pkglib_MODULES += boot.mod 
+boot_mod_SOURCES = commands/boot.c lib/i386/pc/biosnum.c
+boot_mod_CFLAGS = $(COMMON_CFLAGS)
+boot_mod_LDFLAGS = $(COMMON_LDFLAGS)
+
 # For acpi.mod.
 acpi_mod_SOURCES = commands/acpi.c commands/efi/acpi.c
 acpi_mod_CFLAGS = $(COMMON_CFLAGS)
diff --git a/include/grub/i386/pc/biosnum.h b/include/grub/i386/pc/biosnum.h
new file mode 100644
index 0000000..29c8ecc
--- /dev/null
+++ b/include/grub/i386/pc/biosnum.h
@@ -0,0 +1,6 @@
+#ifndef GRUB_BIOSNUM_MACHINE_HEADER
+#define GRUB_BIOSNUM_MACHINE_HEADER	1
+
+extern int (*grub_get_root_biosnumber) (void);
+
+#endif
diff --git a/lib/i386/pc/biosnum.c b/lib/i386/pc/biosnum.c
new file mode 100644
index 0000000..15274e6
--- /dev/null
+++ b/lib/i386/pc/biosnum.c
@@ -0,0 +1,46 @@
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2009  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/env.h>
+#include <grub/misc.h>
+#include <grub/disk.h>
+
+static int
+grub_get_root_biosnumber_default (void)
+{
+  char *biosnum;
+  int ret = -1;
+  grub_device_t dev;
+
+  biosnum = grub_env_get ("biosnum");
+
+  if (biosnum)
+    return grub_strtoul (biosnum, 0, 0);
+
+  dev = grub_device_open (0);
+  if (dev && dev->disk && dev->disk->dev 
+      && grub_strcmp (dev->disk->dev->name, "biosdisk") == 0)
+    ret = (int) dev->disk->id;
+
+  if (dev)
+    grub_device_close (dev);
+
+  return ret;
+}
+
+int (*grub_get_root_biosnumber) (void) = grub_get_root_biosnumber_default;
diff --git a/loader/i386/bsd.c b/loader/i386/bsd.c
index ab3a635..45f6571 100644
--- a/loader/i386/bsd.c
+++ b/loader/i386/bsd.c
@@ -33,6 +33,12 @@
 #include <grub/gzio.h>
 #include <grub/aout.h>
 #include <grub/command.h>
+#ifdef GRUB_MACHINE_PCBIOS
+#include <grub/machine/biosnum.h>
+#include <grub/disk.h>
+#include <grub/device.h>
+#include <grub/partition.h>
+#endif
 
 #define ALIGN_DWORD(a)	ALIGN_UP (a, 4)
 #define ALIGN_QWORD(a)	ALIGN_UP (a, 8)
@@ -81,23 +87,22 @@ grub_bsd_get_device (grub_uint32_t * biosdev,
 		     grub_uint32_t * slice, grub_uint32_t * part)
 {
   char *p;
-
-  *biosdev = *unit = *slice = *part = 0;
-  p = grub_env_get ("root");
-  if ((p) && ((p[0] == 'h') || (p[0] == 'f')) && (p[1] == 'd') &&
-      (p[2] >= '0') && (p[2] <= '9'))
+  grub_device_t dev; 
+
+  *biosdev = grub_get_root_biosnumber () & 0xff;
+  *unit = (*biosdev & 0x7f);
+  *slice = 0xff;
+  *part = 0xff;
+  dev = grub_device_open (0);
+  if (dev && dev->disk && dev->disk->partition)
     {
-      if (p[0] == 'h')
-	*biosdev = 0x80;
-
-      *unit = grub_strtoul (p + 2, &p, 0);
-      *biosdev += *unit;
 
-      if ((p) && (p[0] == ','))
+      p = dev->disk->partition->partmap->get_name (dev->disk->partition);
+      if (p)
 	{
-	  if ((p[1] >= '0') && (p[1] <= '9'))
+	  if ((p[0] >= '0') && (p[0] <= '9'))
 	    {
-	      *slice = grub_strtoul (p + 1, &p, 0);
+	      *slice = grub_strtoul (p, &p, 0);
 
 	      if ((p) && (p[0] == ','))
 		p++;
@@ -107,6 +112,8 @@ grub_bsd_get_device (grub_uint32_t * biosdev,
 	    *part = p[0] - 'a';
 	}
     }
+  if (dev)
+    grub_device_close (dev);
 }
 
 static grub_err_t
diff --git a/loader/i386/multiboot.c b/loader/i386/multiboot.c
index a59085c..06128d7 100644
--- a/loader/i386/multiboot.c
+++ b/loader/i386/multiboot.c
@@ -42,6 +42,12 @@
 #include <grub/misc.h>
 #include <grub/gzio.h>
 #include <grub/env.h>
+#ifdef GRUB_MACHINE_PCBIOS
+#include <grub/machine/biosnum.h>
+#include <grub/disk.h>
+#include <grub/device.h>
+#include <grub/partition.h>
+#endif
 
 extern grub_dl_t my_mod;
 static struct grub_multiboot_info *mbi, *mbi_dest;
@@ -148,46 +154,42 @@ grub_multiboot_load_elf (grub_file_t file, void *buffer)
 static int
 grub_multiboot_get_bootdev (grub_uint32_t *bootdev)
 {
+#ifdef GRUB_MACHINE_PCBIOS
   char *p;
+  grub_uint32_t biosdev, slice = ~0, part = ~0;
+  grub_device_t dev;
 
-  p = grub_env_get ("root");
-  if ((p) && ((p[0] == 'h') || (p[0] == 'f')) && (p[1] == 'd') &&
-      (p[2] >= '0') && (p[2] <= '9'))
-    {
-      grub_uint32_t bd;
+  biosdev = grub_get_root_biosnumber ();
 
-      bd = (p[0] == 'h') ? 0x80 : 0;
-      bd += grub_strtoul (p + 2, &p, 0);
-      bd <<= 24;
+  dev = grub_device_open (0);
+  if (dev && dev->disk && dev->disk->partition)
+    {
 
-      if ((p) && (p[0] == ','))
+      p = dev->disk->partition->partmap->get_name (dev->disk->partition);
+      if (p)
 	{
-	  if ((p[1] >= '0') && (p[1] <= '9'))
+	  if ((p[0] >= '0') && (p[0] <= '9'))
 	    {
-
-	      bd += ((grub_strtoul (p + 1, &p, 0) - 1) & 0xFF) << 16;
+	      slice = grub_strtoul (p, &p, 0);
 
 	      if ((p) && (p[0] == ','))
 		p++;
 	    }
-          else
-            bd += 0xFF0000;
 
 	  if ((p[0] >= 'a') && (p[0] <= 'z'))
-            bd += (p[0] - 'a') << 8;
-          else
-            bd += 0xFF00;
+	    part = p[0] - 'a';
 	}
-      else
-        bd += 0xFFFF00;
-
-      bd += 0xFF;
-
-      *bootdev = bd;
-      return 1;
     }
-
+  if (dev)
+    grub_device_close (dev);
+
+  *bootdev = ((biosdev & 0xff) << 24) | ((slice & 0xff) << 16) 
+    | ((part & 0xff) << 16) | 0xff;
+  return (biosdev != ~0UL);
+#else
+  *bootdev = 0xffffffff;
   return 0;
+#endif
 }
 
 void
diff --git a/loader/i386/pc/chainloader.c b/loader/i386/pc/chainloader.c
index 304886b..fe7e89a 100644
--- a/loader/i386/pc/chainloader.c
+++ b/loader/i386/pc/chainloader.c
@@ -31,6 +31,7 @@
 #include <grub/machine/memory.h>
 #include <grub/dl.h>
 #include <grub/command.h>
+#include <grub/machine/biosnum.h>
 
 static grub_dl_t my_mod;
 static int boot_drive;
@@ -89,29 +90,18 @@ grub_chainloader_cmd (const char *filename, grub_chainloader_flags_t flags)
   grub_file_close (file);
 
   /* Obtain the partition table from the root device.  */
+  drive = grub_get_root_biosnumber ();
   dev = grub_device_open (0);
-  if (dev)
+  if (dev && dev->disk && dev->disk->partition)
     {
-      grub_disk_t disk = dev->disk;
-      
-      if (disk)
-	{
-	  grub_partition_t p = disk->partition;
-	  
-	  /* In i386-pc, the id is equal to the BIOS drive number.  */
-	  drive = (int) disk->id;
-
-	  if (p)
-	    {
-	      grub_disk_read (disk, p->offset, 446, 64,
-			      (void *) GRUB_MEMORY_MACHINE_PART_TABLE_ADDR);
-	      part_addr = (void *) (GRUB_MEMORY_MACHINE_PART_TABLE_ADDR
-				    + (p->index << 4));
-	    }
-	}
-
-      grub_device_close (dev);
+      grub_disk_read (dev->disk, dev->disk->partition->offset, 446, 64,
+		      (void *) GRUB_MEMORY_MACHINE_PART_TABLE_ADDR);
+      part_addr = (void *) (GRUB_MEMORY_MACHINE_PART_TABLE_ADDR
+			    + (dev->disk->partition->index << 4));
     }
+
+  if (dev)
+    grub_device_close (dev);
   
   /* Ignore errors. Perhaps it's not fatal.  */
   grub_errno = GRUB_ERR_NONE;

Re: status grub2 port of grub-legasy map command

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 456 bytes --]

I think you should move this patch to a new thread, first because most
people in the list seem to no longer watch this one, and second because
it touches many files apart from drivemap itself. That said, I still
don't see the situation in which such an override would be used.

Oh, and I would rather see disk->dev->id == GRUB_DISK_DEVICE_BIOSDISK_ID
than a strcmp with a magic string literal.
-- 
-- Lazy, Oblivious, Recurrent Disaster -- Habbit

[-- Attachment #2: Esto es una parte de mensaje firmado digitalmente --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: status grub2 port of grub-legasy map command

[Vladimir 'phcoder' Serbinenko]

On Thu, May 14, 2009 at 3:51 AM, Pavel Roskin <proski@gnu.org> wrote:
> On Sat, 2009-05-09 at 17:42 +0200, Javier Martín wrote:
>
>> OK, I have a good feeling about this version of the patch. Most
>> importantly, it still works!!
>
> I have committed your patch after a cleanup.  My changes were following:
>
> grub_drivemap_int13_handler_base and grub_drivemap_int13_handler have
> been merged.
>
> grub_drivemap_int13_oldhandler points directly to the ljmp argument
> (other GRUB sources do it too, jump look for 0xea).
>
> %si is not used anymore.
>
> Tabs are used consistently in drivemap_int13h.S
>
> Many variables have been renamed.
>
> Constants have been brought to the top level and marked as such.
>
> Logic in uninstall_int13_handler() has been fixed.
>
> Some messages have been clarified.  In particularly, biosdisk is called
> osdisk now, as it's what the OS sees.  I was thinking of "payload" or
> "loadee" as more generic terms, but it can be confusing to the users.
osdisk isn't less confusing than biosdisk. I already can see the users
doing things like
drivemap ata1 C:
and asking why it fails. mapfrom and mapto are much better
>
> My check for drivemap with no arguments was wrong, fixed now.
>
> Added missing line ends in all outputs.
>
> Removed INT13H_REBASE and INT13H_TONEWADDR, as they were used only once.
>
> Comments have been improved.
>
> --
> Regards,
> Pavel Roskin
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Re: status grub2 port of grub-legasy map command

[Pavel Roskin]

On Thu, 2009-05-14 at 08:53 +0200, Vladimir 'phcoder' Serbinenko wrote:

> > Some messages have been clarified.  In particularly, biosdisk is called
> > osdisk now, as it's what the OS sees.  I was thinking of "payload" or
> > "loadee" as more generic terms, but it can be confusing to the users.
> osdisk isn't less confusing than biosdisk. I already can see the users
> doing things like
> drivemap ata1 C:
> and asking why it fails. mapfrom and mapto are much better

Unfortunately, "to" and "from" depend on the point of view.  From the
GRUB standpoint, "drivemap hd1 hd0" remaps "from hd1 to hd0", so that
our hd1 becomes hd0 for the OS.  From the OS standpoint, the same
command remaps "from hd0 to hd1", since the calls to hd0 (or C: or hda)
are routed to hd1.

I think maybe "real disk" and "emulated disk" would be clear, but I
don't feel it would be a big improvement over "GRUB disk" and "OS disk".

-- 
Regards,
Pavel Roskin