| Message ID | 7e3f69d7-23e8-397d-72b6-8c489d80ea45@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: memcpy() / memset() (non-)ERMS flavors plus fallout | expand |
On 27/04/2021 13:56, Jan Beulich wrote: The grammar in the subject is very awkward. The (not just) like that is weird. If it were me, I'd phrase this as "minor adjustments to command line handling". > Document both options. Add section annotations to both variables holding > the parsed values as well as a few adjacent ones. Adjust the types of > font_height and vga_compat. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> In principle, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>, with one note below. However, is there really any value in these options? I can't see a case where their use will result in a less broken system. > > --- a/docs/misc/xen-command-line.pandoc > +++ b/docs/misc/xen-command-line.pandoc > @@ -2369,9 +2369,21 @@ cache-warming. 1ms (1000) has been measu > ### vesa-map > > `= <integer>` > > +> Default: `0` > + > +Specify, in MiB, the portion of video RAM to actually remap. This will be > +honored only when large enough to cover the space needed for the chosen video > +mode, and only when less than a non-zero value possibly specified through > +'vesa-ram'. "and only when less than a non-zero value possibly specified" is confusing to follow. What I think you mean is that vesa-map will be honoured when it is >= chosen video mode, and <= vesa-ram? ~Andrew > + > ### vesa-ram > > `= <integer>` > > +> Default: `0` > + > +This allows to override the amount of video RAM, in MiB, determined to be > +present. > + > ### vga > > `= ( ask | current | text-80x<rows> | gfx-<width>x<height>x<depth> | mode-<mode> )[,keep]` > > --- a/xen/drivers/video/vesa.c > +++ b/xen/drivers/video/vesa.c > @@ -19,17 +19,17 @@ > > static void lfb_flush(void); > > -static unsigned char *lfb; > -static const struct font_desc *font; > -static bool_t vga_compat; > +static unsigned char *__read_mostly lfb; > +static const struct font_desc *__initdata font; > +static bool __initdata vga_compat; > > -static unsigned int vram_total; > +static unsigned int __initdata vram_total; > integer_param("vesa-ram", vram_total); > > -static unsigned int vram_remap; > +static unsigned int __initdata vram_remap; > integer_param("vesa-map", vram_remap); > > -static int font_height; > +static unsigned int __initdata font_height; > static int __init parse_font_height(const char *s) > { > if ( simple_strtoul(s, &s, 10) == 8 && (*s++ == 'x') ) >
On 27.04.2021 15:49, Andrew Cooper wrote: > On 27/04/2021 13:56, Jan Beulich wrote: > > The grammar in the subject is very awkward. The (not just) like that is > weird. > > If it were me, I'd phrase this as "minor adjustments to command line > handling". Well, the (not just) is intentionally there because there are changes to stuff unrelated to command line option handling as well. >> Document both options. Add section annotations to both variables holding >> the parsed values as well as a few adjacent ones. Adjust the types of >> font_height and vga_compat. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > In principle, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>, with > one note below. Thanks. > However, is there really any value in these options? I can't see a case > where their use will result in a less broken system. Well, if we mis-detect VRAM size, the respective option might indeed help. I'm less certain of the utility of the mapping option, the more that now there's no possible (and implicit) effect on MTRRs anymore. >> --- a/docs/misc/xen-command-line.pandoc >> +++ b/docs/misc/xen-command-line.pandoc >> @@ -2369,9 +2369,21 @@ cache-warming. 1ms (1000) has been measu >> ### vesa-map >> > `= <integer>` >> >> +> Default: `0` >> + >> +Specify, in MiB, the portion of video RAM to actually remap. This will be >> +honored only when large enough to cover the space needed for the chosen video >> +mode, and only when less than a non-zero value possibly specified through >> +'vesa-ram'. > > "and only when less than a non-zero value possibly specified" is > confusing to follow. > > What I think you mean is that vesa-map will be honoured when it is >= > chosen video mode, and <= vesa-ram? Yes. Any suggestion how to improve the wording without using >= and <= ? Jan
On 27.04.2021 16:04, Jan Beulich wrote: > On 27.04.2021 15:49, Andrew Cooper wrote: >> However, is there really any value in these options? I can't see a case >> where their use will result in a less broken system. > > Well, if we mis-detect VRAM size, the respective option might indeed > help. I'm less certain of the utility of the mapping option, the more > that now there's no possible (and implicit) effect on MTRRs anymore. Actually I was wrong with referring to an implied effect on MTRRs - that would come from "vesa-ram=". "vesa-map=" may help if we mis-detected the space we actually need for the mode. However, we'd then be screwed up elsewhere as well, so I guess I'll add a patch removing "vesa-map=" as well. Jan
--- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -2369,9 +2369,21 @@ cache-warming. 1ms (1000) has been measu ### vesa-map > `= <integer>` +> Default: `0` + +Specify, in MiB, the portion of video RAM to actually remap. This will be +honored only when large enough to cover the space needed for the chosen video +mode, and only when less than a non-zero value possibly specified through +'vesa-ram'. + ### vesa-ram > `= <integer>` +> Default: `0` + +This allows to override the amount of video RAM, in MiB, determined to be +present. + ### vga > `= ( ask | current | text-80x<rows> | gfx-<width>x<height>x<depth> | mode-<mode> )[,keep]` --- a/xen/drivers/video/vesa.c +++ b/xen/drivers/video/vesa.c @@ -19,17 +19,17 @@ static void lfb_flush(void); -static unsigned char *lfb; -static const struct font_desc *font; -static bool_t vga_compat; +static unsigned char *__read_mostly lfb; +static const struct font_desc *__initdata font; +static bool __initdata vga_compat; -static unsigned int vram_total; +static unsigned int __initdata vram_total; integer_param("vesa-ram", vram_total); -static unsigned int vram_remap; +static unsigned int __initdata vram_remap; integer_param("vesa-map", vram_remap); -static int font_height; +static unsigned int __initdata font_height; static int __init parse_font_height(const char *s) { if ( simple_strtoul(s, &s, 10) == 8 && (*s++ == 'x') )
Document both options. Add section annotations to both variables holding the parsed values as well as a few adjacent ones. Adjust the types of font_height and vga_compat. Signed-off-by: Jan Beulich <jbeulich@suse.com>