| Message ID | 1570110555-24287-2-git-send-email-igor.druzhinin@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | EFI GOP fixes | expand |
On 03.10.2019 15:49, Igor Druzhinin wrote: > - 0 is a possible and allowed value for a color mask accroding to > UEFI Spec 2.6 (11.9) especially for reserved mask Hmm, looking at 2.8 (where it's section 12.9, which in turn is why section titles would be more helpful in such references) I can't see the case being mentioned explicitly. I can accept that ReservedMask might be zero, but then I'd prefer to handle that case in the caller, rather than allowing zero also for the three colors. Jan
On 04/10/2019 11:34, Jan Beulich wrote: > On 03.10.2019 15:49, Igor Druzhinin wrote: >> - 0 is a possible and allowed value for a color mask accroding to >> UEFI Spec 2.6 (11.9) especially for reserved mask > > Hmm, looking at 2.8 (where it's section 12.9, which in turn is why > section titles would be more helpful in such references) I can't > see the case being mentioned explicitly. I can accept that > ReservedMask might be zero, but then I'd prefer to handle that > case in the caller, rather than allowing zero also for the three > colors. "If a bit is set in RedMask, GreenMask, or BlueMask then those bits of the pixel represent the corresponding color." - "If a bit is set..." implies it might not be set. Nothing prevents mask for the colors be 0 as well. Igor
On 04.10.2019 12:54, Igor Druzhinin wrote: > On 04/10/2019 11:34, Jan Beulich wrote: >> On 03.10.2019 15:49, Igor Druzhinin wrote: >>> - 0 is a possible and allowed value for a color mask accroding to >>> UEFI Spec 2.6 (11.9) especially for reserved mask >> >> Hmm, looking at 2.8 (where it's section 12.9, which in turn is why >> section titles would be more helpful in such references) I can't >> see the case being mentioned explicitly. I can accept that >> ReservedMask might be zero, but then I'd prefer to handle that >> case in the caller, rather than allowing zero also for the three >> colors. > > "If a bit is set in RedMask, GreenMask, or BlueMask then those bits of > the pixel represent the corresponding color." - "If a bit is set..." > implies it might not be set. This talks about the function of individual bits. There's nothing said about not bit at all being set for a particular color. > Nothing prevents mask for the colors be 0 as well. I wouldn't read it like this, no. I'm fine imply such for the reserved field, but I'd rather consider it a broken mode if one of the colors has no way of representing at all. In particular "The color intensities must increase as the color values for a each color mask increase with a minimum intensity of all bits in a color mask clear to a maximum intensity of all bits in a color mask set." suggests to me that there can't be zero bits set, or else there'd be no difference between minimum and maximum intensity. Also, while mathematically it makes sense for "all bits" to include the case of zero of them, it doesn't (to me at least) in day-to-day use of the language. Jan
On 04/10/2019 12:14, Jan Beulich wrote: > On 04.10.2019 12:54, Igor Druzhinin wrote: >> On 04/10/2019 11:34, Jan Beulich wrote: >>> On 03.10.2019 15:49, Igor Druzhinin wrote: >>>> - 0 is a possible and allowed value for a color mask accroding to >>>> UEFI Spec 2.6 (11.9) especially for reserved mask >>> >>> Hmm, looking at 2.8 (where it's section 12.9, which in turn is why >>> section titles would be more helpful in such references) I can't >>> see the case being mentioned explicitly. I can accept that >>> ReservedMask might be zero, but then I'd prefer to handle that >>> case in the caller, rather than allowing zero also for the three >>> colors. >> >> "If a bit is set in RedMask, GreenMask, or BlueMask then those bits of >> the pixel represent the corresponding color." - "If a bit is set..." >> implies it might not be set. > > This talks about the function of individual bits. There's nothing said > about not bit at all being set for a particular color. > I know certainly that it's not only me who reads this sentence the same way - firmware developers as well. But if you insist I will restrict this change to reserved mask only. Igor
On 04.10.2019 13:25, Igor Druzhinin wrote: > On 04/10/2019 12:14, Jan Beulich wrote: >> On 04.10.2019 12:54, Igor Druzhinin wrote: >>> On 04/10/2019 11:34, Jan Beulich wrote: >>>> On 03.10.2019 15:49, Igor Druzhinin wrote: >>>>> - 0 is a possible and allowed value for a color mask accroding to >>>>> UEFI Spec 2.6 (11.9) especially for reserved mask >>>> >>>> Hmm, looking at 2.8 (where it's section 12.9, which in turn is why >>>> section titles would be more helpful in such references) I can't >>>> see the case being mentioned explicitly. I can accept that >>>> ReservedMask might be zero, but then I'd prefer to handle that >>>> case in the caller, rather than allowing zero also for the three >>>> colors. >>> >>> "If a bit is set in RedMask, GreenMask, or BlueMask then those bits of >>> the pixel represent the corresponding color." - "If a bit is set..." >>> implies it might not be set. >> >> This talks about the function of individual bits. There's nothing said >> about not bit at all being set for a particular color. >> > > I know certainly that it's not only me who reads this sentence the same > way - firmware developers as well. But if you insist I will restrict > this change to reserved mask only. Please do, unless you can provide a plausible (non-broken) scenario where one of the three color masks could be zero. Jan
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 9a89414..933db88 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -1113,10 +1113,14 @@ static int __init __maybe_unused set_color(u32 mask, int bpp, u8 *pos, u8 *sz) if ( bpp < 0 ) return bpp; if ( !mask ) - return -EINVAL; + { + *pos = 0; + *sz = 0; + return bpp; + } for ( *pos = 0; !(mask & 1); ++*pos ) mask >>= 1; - for ( *sz = 0; mask & 1; ++sz) + for ( *sz = 0; mask & 1; ++*sz) mask >>= 1; if ( mask ) return -EINVAL;
- 0 is a possible and allowed value for a color mask accroding to UEFI Spec 2.6 (11.9) especially for reserved mask - add missing pointer dereference Without these changes non-TrueColor modes won't work which will cause GOP init to fail - observed while trying to boot EFI Xen with Cirrus VGA. Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com> --- xen/common/efi/boot.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)