| Message ID | DDAFD321-BBFA-4299-8310-BE0B52B7C601@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 2016-09-20 at 00:28 -0400, G 3 wrote: > + RegEntryID *entry_id; > + OSErr err; > + OSStatus os_status = noErr; > + Boolean is_done; > + void *value; > + RegPropertyValueSize property_size = -1; > + int index, res_set_count; > + char *set_str; > + > + #define PROPERTY_NAME "resolutions" > + #define NODE_PATH "Devices:device-tree:options" > + > + /* init the entry variable */ > + err = RegistryEntryIDInit(entry_id); > + if (err != noErr) { > + lprintf("Error: Failed to init entry variable! > (Error: %d)\n", err); > + return err; > + } > + is_done = false; > + No, you need to allocate the RegistryEntryID on the stack otherwise you are whacking at a random uninitialized pointer. IE: RegistryEntryID entry_id; RegistryEntryIDInit(&entry_id); .../... See if that helps with your OS X problem. Also I don't like the use of pow(), there must be a better way ... Check if there's anything of value to be picked up from DSL, otherwise, put those utilities somewhere in common, other drivers might want them. (What does our lprintf implementation do for example ?) Cheers, Ben.
Also .. your patch was all HTML and email-damaged... On Tue, 2016-09-20 at 19:01 +1000, Benjamin Herrenschmidt wrote: > On Tue, 2016-09-20 at 00:28 -0400, G 3 wrote: > > > + RegEntryID *entry_id; > > + OSErr err; > > + OSStatus os_status = noErr; > > + Boolean is_done; > > + void *value; > > + RegPropertyValueSize property_size = -1; > > + int index, res_set_count; > > + char *set_str; > > + > > + #define PROPERTY_NAME "resolutions" > > + #define NODE_PATH "Devices:device-tree:options" > > + > > + /* init the entry variable */ > > + err = RegistryEntryIDInit(entry_id); > > + if (err != noErr) { > > + lprintf("Error: Failed to init entry variable! > > (Error: %d)\n", err); > > + return err; > > + } > > + is_done = false; > > + > > No, you need to allocate the RegistryEntryID on the stack otherwise > you are whacking at a random uninitialized pointer. IE: > > RegistryEntryID entry_id; > > RegistryEntryIDInit(&entry_id); > .../... > > See if that helps with your OS X problem. Also I don't like the > use of pow(), there must be a better way ... Check if there's > anything > of value to be picked up from DSL, otherwise, put those utilities > somewhere in common, other drivers might want them. > > (What does our lprintf implementation do for example ?) > > Cheers, > Ben.
On Sep 20, 2016, at 5:01 AM, Benjamin Herrenschmidt wrote: > On Tue, 2016-09-20 at 00:28 -0400, G 3 wrote: >> + RegEntryID *entry_id; >> + OSErr err; >> + OSStatus os_status = noErr; >> + Boolean is_done; >> + void *value; >> + RegPropertyValueSize property_size = -1; >> + int index, res_set_count; >> + char *set_str; >> + >> + #define PROPERTY_NAME "resolutions" >> + #define NODE_PATH "Devices:device-tree:options" >> + >> + /* init the entry variable */ >> + err = RegistryEntryIDInit(entry_id); >> + if (err != noErr) { >> + lprintf("Error: Failed to init entry variable! >> (Error: %d)\n", err); >> + return err; >> + } >> + is_done = false; >> + > > No, you need to allocate the RegistryEntryID on the stack otherwise > you are whacking at a random uninitialized pointer. IE: > > RegistryEntryID entry_id; > > RegistryEntryIDInit(&entry_id); > .../... > See if that helps with your OS X problem. Good catch. I missed that one. It didn't fix the problem. I think the options node isn't accessible for some reason in Mac OS X. > Also I don't like the > use of pow(), there must be a better way ... What did you have in mind? Do you want atoi() rewritten to exclude it? > Check if there's anything > of value to be picked up from DSL, otherwise, put those utilities > somewhere in common, other drivers might want them. What is DSL? Did you want me to put the pow() function in the MacDriverUtils.c file? > > (What does our lprintf implementation do for example ?) I think it prints to some virtual device. I'm not sure. I haven't figured out how to use it yet. If you could provide some directions I think I might be able to make it work.
On 09/19/2016 11:28 PM, G 3 wrote: > Add the ability to add resolutions from the command-line. This patch > works by > looking for a property called 'resolutions' in the options node of > OpenBIOS. > If it is found all the resolutions are parsed and loaded. > > +/* strlen() implementation */ > +static int strlen(const char *str) Is it worth renaming this to make it obvious that it is your (non-optimal) replacement, intentionally because you don't want to use the libc version? By the way, strlen() normally returns size_t; redefining a standard function to return a different type is just asking for problems. > +{ > + int count = 0; > + > + for( ; *str != '\0'; str++) { > + count++; > + } > + > + return count; > +} > + > + > +/* convert ascii string to number */ > +static int atoi(const char *str) > +{ > + int result = 0, multiplier; > + > + multiplier = strlen(str) - 1; > + for ( ; *str != '\0'; str++) { > + result += (*str - '0') * pow(10, multiplier); > + multiplier--; > + if (multiplier < 0) /* Something might be wrong if returning > here */ > + return result; > + } EWWWW. If you're going to (poorly) reimplement a standard function, at the very minimum you should NOT be doing it with a dirt-slow algorithm. atoi() already has undefined behavior on integer overflow; use that to your advantage. pow() (and to a lesser extent strlen()) should NEVER be part of atoi(). Try something more like: static int my_atoi(const char *str) { int result = 0; while (*str) { result = result * 10 + *str - '0'; str++; } } There you go. Much nicer, shorter, and drags in fewer dependencies (and I renamed it because I do NOT detect leading whitespace or '-', which is a requirement of the libc atoi()). > + > +/* An interesting way of emulating memory allocation. */ > +static char *malloc(int size) Again, if you're going to override a standard function, either change the name (to make it obvious what you are doing), or at a bare minimum match the standard signature (void *malloc(size_t)). And overriding malloc() without also overriding free(), realloc(), calloc(), and who knows what else, is likely a recipe for disaster if a later programmer sees your use of malloc and tries to add in other uses of memory management, without realizing your override is NOT the same as the libc version. So my recommendation: PLEASE rename this to something that is NOT 'malloc()', to make it OBVIOUS that you are NOT allocating heap memory. Meanwhile, I seriously doubt you really want to be reimplementing malloc(); you are likely headed to disaster (if you can't even override atoi() in a sane manner, I dread what will happen when your driver runs out of malloc() space).
On Sep 20, 2016, at 9:37 AM, Eric Blake wrote: > On 09/19/2016 11:28 PM, G 3 wrote: >> Add the ability to add resolutions from the command-line. This patch >> works by >> looking for a property called 'resolutions' in the options node of >> OpenBIOS. >> If it is found all the resolutions are parsed and loaded. >> > >> +/* strlen() implementation */ >> +static int strlen(const char *str) > > Is it worth renaming this to make it obvious that it is your > (non-optimal) replacement, intentionally because you don't want to use > the libc version? I originally thought about adding a prefix to all my functions. Ben what do you think? I'm ok with either way. > > By the way, strlen() normally returns size_t; redefining a standard > function to return a different type is just asking for problems. Changing int to size_t isn't a problem. Will do it in my next patch. > >> +{ >> + int count = 0; >> + >> + for( ; *str != '\0'; str++) { >> + count++; >> + } >> + >> + return count; >> +} >> + >> + > >> +/* convert ascii string to number */ >> +static int atoi(const char *str) >> +{ >> + int result = 0, multiplier; >> + >> + multiplier = strlen(str) - 1; >> + for ( ; *str != '\0'; str++) { >> + result += (*str - '0') * pow(10, multiplier); >> + multiplier--; >> + if (multiplier < 0) /* Something might be wrong if >> returning >> here */ >> + return result; >> + } > > EWWWW. If you're going to (poorly) reimplement a standard > function, at > the very minimum you should NOT be doing it with a dirt-slow > algorithm. > atoi() already has undefined behavior on integer overflow; use that to > your advantage. pow() (and to a lesser extent strlen()) should > NEVER be > part of atoi(). Try something more like: > > static int my_atoi(const char *str) > { > int result = 0; > while (*str) { > result = result * 10 + *str - '0'; > str++; > } > } > > There you go. Much nicer, shorter, and drags in fewer dependencies > (and > I renamed it because I do NOT detect leading whitespace or '-', > which is > a requirement of the libc atoi()). Ok. > >> + >> +/* An interesting way of emulating memory allocation. */ >> +static char *malloc(int size) > > Again, if you're going to override a standard function, either change > the name (to make it obvious what you are doing), or at a bare minimum > match the standard signature (void *malloc(size_t)). And overriding > malloc() without also overriding free(), realloc(), calloc(), and who > knows what else, is likely a recipe for disaster if a later programmer > sees your use of malloc and tries to add in other uses of memory > management, without realizing your override is NOT the same as the > libc > version. So my recommendation: PLEASE rename this to something > that is > NOT 'malloc()', to make it OBVIOUS that you are NOT allocating heap > memory. That's fine with me. > Meanwhile, I seriously doubt you really want to be reimplementing > malloc(); you are likely headed to disaster (if you can't even > override > atoi() in a sane manner, I dread what will happen when your driver > runs > out of malloc() space). The malloc() function used in this driver is used to allocate a very small amount of space at most. We are realistically talking under 2k. Running out of space is highly unlikely. I'm sure the user could do something evil to try to crash the driver, but it they succeed, it would be their own fault. Under normal use 2k of memory is all that is needed Thank you for reviewing my patch.
On Tue, 2016-09-20 at 09:51 -0400, G 3 wrote: > > > Is it worth renaming this to make it obvious that it is your > > (non-optimal) replacement, intentionally because you don't want to > > use > > the libc version? > > I originally thought about adding a prefix to all my functions. Ben > what do you think? > I'm ok with either way. DriverServicesLibrary has: char *CStrCopy(char *dst, const char *src); char *CStrNCopy (char *dst, const char *src, UInt32 max); char *CStrCat (char *dst, const char *src); char *CStrNCat (char *dst, const char *src, UInt32 max); short CStrCmp (const char *str1, const char *str2); short CStrNCmp (const char *str1, const char *str2, UInt32 max); UInt32 CStrLen (const char *src); Cheers, Ben.
On Tue, 2016-09-20 at 09:51 -0400, G 3 wrote: > > The malloc() function used in this driver is used to allocate a > very > small amount of space at most. We are realistically talking under > 2k. > Running out of space is highly unlikely. I'm sure the user could do > something evil to try to crash the driver, but it they succeed, it > would be their own fault. Under normal use 2k of memory is all that > is needed Why don't you use PoolAllocateResident from the DSL ?
diff --git a/QemuVGADriver/src/QemuVga.c b/QemuVGADriver/src/QemuVga.c index 4584242..cba65ba 100644 --- a/QemuVGADriver/src/QemuVga.c +++ b/QemuVGADriver/src/QemuVga.c @@ -18,7 +18,19 @@ static struct vMode vModes[] = { { 1600, 1200 }, { 1920, 1080 }, { 1920, 1200 }, - { 0,0 } + { 0,0 }, + { 0,0 }, + { 0,0 }, + { 0,0 }, + { 0,0 }, + { 0,0 }, + { 0,0 }, + { 0,0 }, + { 0,0 }, + { 0,0 }, + { 0,0 }, + { 0,0 }, + { 0,0 }, }; static void VgaWriteB(UInt16 port, UInt8 val) @@ -148,11 +160,280 @@ static InterruptMemberNumber PCIInterruptHandler(InterruptSetMember ISTmember, #endif +/* Returns the number of resolutions */ +static int get_number_of_resolutions() +{ + int size_of_array, num_of_resolutions, index; + + num_of_resolutions = 0; + size_of_array = sizeof(vModes) / sizeof(struct vMode); + + for(index = 0; index < size_of_array; index++) + { + if (vModes[index].width != 0) { + num_of_resolutions++; + } + } + + return num_of_resolutions; +} + + +/* strlen() implementation */ +static int strlen(const char *str) +{ + int count = 0; + + for( ; *str != '\0'; str++) { + count++; + } + + return count; +} + + +/* output = base^power */ +static int pow(int base, int power) +{ + int i, output; + output = 1; + for (i = 0; i < power; i++) { + output = output * base; + } + return output; +} + + +/* convert ascii string to number */ +static int atoi(const char *str) +{ + int result = 0, multiplier; + + multiplier = strlen(str) - 1; + for ( ; *str != '\0'; str++) { + result += (*str - '0') * pow(10, multiplier); + multiplier--; + if (multiplier < 0) /* Something might be wrong if returning here */ + return result; + } + + return result; +} + + +/* An interesting way of emulating memory allocation. */ +static char *malloc(int size) +{ + const int max_buffer_size = 2000; + static char buffer[max_buffer_size]; + static int free_mem_pointer = 0; + static Boolean first_alloc = true; + + free_mem_pointer += size; + if (free_mem_pointer >= max_buffer_size) { + return NULL; + } + + /* For getting the very beginning of the buffer */ + if (first_alloc == true) { + first_alloc = false; + return buffer; + } + + return (buffer + free_mem_pointer); +} + + +/* Get the resolution set at the specified index. */ +static char *get_resolution_set(const char *resolution_set_str, int set_number) +{ + const int max_buf_size = 100; + char c, *buffer; + int index, comma_count; + + buffer = (char *) malloc(max_buf_size); + comma_count = 0; + index = 0; + set_number++; /* Makes things easier to understand */ + + c = *(resolution_set_str++); + while (c != '\0') { + buffer[index++] = c; + c = *(resolution_set_str++); + if (c == ',') { + comma_count++; + if (comma_count == set_number || index >= max_buf_size) { + buffer[index] = '\0'; + return buffer; + } + + else { + /* reset the buffer */ + index = 0; + c = *(resolution_set_str++); + } + } + } + + buffer[index] = '\0'; + + return buffer; +} + + +/* Get the number of resolution sets - a set is a width by height pair */ +static int get_resolution_set_count(const char *resolution_sets_str) +{ + char c; + int count; + + /* Count the number of commas */ + count = 0; + c = *(resolution_sets_str++); + while (c != '\0') { + if (c == ',') { + count++; + } + c = *(resolution_sets_str++); + } + + return count + 1; +} + + +/* + * Returns the width value of a resolution set + * Example: + * input: 16000x9000 + * output: 16000 + */ +static int get_width(const char *resolution_str) +{ + int index; + char c; + const int max_buf_size = 25; + char buffer[max_buf_size]; + c = *(resolution_str++); + index = 0; + while (c != 'x' && index < max_buf_size) { + buffer[index++] = c; + c = *(resolution_str++); + } + + /* Terminate string */ + buffer[index] = '\0'; + + return atoi(buffer); +} + + +/* + * Returns the height value of a resolution set + * Example + * input: 16000x9000 + * output: 9000 + */ +static int get_height(const char *resolution_str) +{ + int index; + char c; + const int max_buf_size = 25; + char buffer[max_buf_size]; + + /* skip to character after x */ + while (*resolution_str != 'x') { + resolution_str++; + } + resolution_str++; + + /* Add digits of the height to the buffer */ + index = 0; + c = *(resolution_str++); + while (c != '\0') { + buffer[index++] = c; + c = *(resolution_str++); + if(index >= max_buf_size) { + break; + } + } + + /* Terminate string */ + buffer[index] = '\0'; + + return atoi(buffer); +} + + +/* Looks in the /options node for the value of the resolutions property */ +static int add_user_resolutions(void) +{ + RegEntryID *entry_id; + OSErr err; + OSStatus os_status = noErr; + Boolean is_done; + void *value; + RegPropertyValueSize property_size = -1; + int index, res_set_count; + char *set_str; + + #define PROPERTY_NAME "resolutions" + #define NODE_PATH "Devices:device-tree:options" + + /* init the entry variable */ + err = RegistryEntryIDInit(entry_id); + if (err != noErr) { + lprintf("Error: Failed to init entry variable! (Error: %d)\n", err); + return err; + } + is_done = false; + + /* Get the entry ID value */ + err = RegistryCStrEntryLookup(NULL /* start root */, NODE_PATH, entry_id); + if (err != noErr) { + lprintf("RegistryCStrEntryLookup() failure (Error: %d)\n", err); + return err; + } + + /* Get the size of the property */ + os_status = RegistryPropertyGetSize(entry_id, PROPERTY_NAME, &property_size); + if (os_status != noErr) { + lprintf("Error: Failed to get property size! (Error: %d)\n", os_status); + return os_status; + } + + /* allocate memory to the value variable */ + value = (void *) malloc(property_size); + + /* Get the value of the property */ + err = RegistryPropertyGet(entry_id, PROPERTY_NAME, value, &property_size); + if (err != noErr) { + lprintf("Error: Failed to find property value %s! (Error: %d)\n", PROPERTY_NAME, err); + return err; + } + + /* Limit the number of resolutions to number of available slots in vMode */ + #define available_slots 20 + res_set_count = get_resolution_set_count(value); + res_set_count = (res_set_count > available_slots ? available_slots : res_set_count); + + /* Add each resolution set */ + for(index = 0; index < res_set_count; index++) { + set_str = get_resolution_set(value, index); + vModes[index].width = get_width(set_str); + vModes[index].height = get_height(set_str); + } + + return 0; +} +
Add the ability to add resolutions from the command-line. This patch works by looking for a property called 'resolutions' in the options node of OpenBIOS. If it is found all the resolutions are parsed and loaded. Example command-line: -prom-env resolutions=512x342,640x480,800x600,1024x600,1200x700,1440x900 Signed-off-by: John Arbuckle <programmingkidx@gmail.com> --- Implemented my own malloc(), strlen(), pow(), and atoi() functions. Removed free() calls. Changed get_set_count() to get_resolution_set_count(). QemuVGADriver/src/QemuVga.c | 285 ++++++++++++++++++++++++++++++++++ +++++++++- 1 file changed, 283 insertions(+), 2 deletions(-) + OSStatus QemuVga_Init(void) { UInt16 id, i; UInt32 mem, width, height, depth; + add_user_resolutions(); + lprintf("First MMIO read...\n"); id = DispiReadW(VBE_DISPI_INDEX_ID); mem = DispiReadW(VBE_DISPI_INDEX_VIDEO_MEMORY_64K); @@ -183,7 +464,7 @@ OSStatus QemuVga_Init(void) i = 0; } GLOBAL.bootMode = i; - GLOBAL.numModes = sizeof(vModes) / sizeof(struct vMode) - 1; + GLOBAL.numModes = get_number_of_resolutions(); QemuVga_SetMode(GLOBAL.bootMode, depth, 0);