Re: [PATCH for-7.1] applesmc: silence invalid key warning in case default one is used
Daniel P. Berrangé writes: > On Thu, Jul 28, 2022 at 02:40:22PM +0100, Peter Maydell wrote: >> On Thu, 28 Jul 2022 at 14:30, Markus Armbruster wrote: >> > Peter Maydell writes: >> > I applaud the renaissance of roman-style inscriptions, but it's not just >> > words without spaces, it's also capital letters only: >> > >> > ANY64CHARACTERFAKEOSKISENOUGHTOPREVENTINVALIDKEYWARNINGSONSTDERR >> > >> > Seriously, throw in some dashes or spaces. >> >> any-64-char-fake-osk-will-avoid-an-invalid-key-warning-on-stderr > > On the basis that virtualization gives you turtles all the way down... > > -device isa-applesmc,osk= Oh, Egyptian rather than Roman. Works for me!
Re: [PATCH for-7.1] applesmc: silence invalid key warning in case default one is used
On Thu, Jul 28, 2022 at 02:40:22PM +0100, Peter Maydell wrote: > On Thu, 28 Jul 2022 at 14:30, Markus Armbruster wrote: > > Peter Maydell writes: > > I applaud the renaissance of roman-style inscriptions, but it's not just > > words without spaces, it's also capital letters only: > > > > ANY64CHARACTERFAKEOSKISENOUGHTOPREVENTINVALIDKEYWARNINGSONSTDERR > > > > Seriously, throw in some dashes or spaces. > > any-64-char-fake-osk-will-avoid-an-invalid-key-warning-on-stderr On the basis that virtualization gives you turtles all the way down... -device isa-applesmc,osk= With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH for-7.1] applesmc: silence invalid key warning in case default one is used
On Thu, 28 Jul 2022 15:29:58 +0200
Markus Armbruster wrote:
> Peter Maydell writes:
>
> > On Thu, 28 Jul 2022 at 11:23, Daniel P. Berrangé
> > wrote:
> >>
> >> On Thu, Jul 28, 2022 at 11:05:13AM +0100, Peter Maydell wrote:
> >> > On Thu, 28 Jul 2022 at 10:48, Daniel P. Berrangé
> >> > wrote:
> >> > >
> >> > > On Thu, Jul 28, 2022 at 05:35:58AM -0400, Igor Mammedov wrote:
> >> > > > QEMU probably can't carry OSK key[1] for legal reasons so it
> >> > > > can't supply the valid default key. However when tests are run
> >> > > > applesmc will pollute test log with distracting warning,
> >> > > > silence that warning so it won't distract maintainers/CI.
> >> > >
> >> > > What test is causing this problem ?
> >> >
> >> > bios-tables-test -- see here for the relevant bit of the log:
> >> >
> >> > https://lore.kernel.org/qemu-devel/CAFEAcA8u8jm7b+JD_t0qMNMy+WSJPOw=qxqptZpwTp=tkcx...@mail.gmail.com/
> >> >
> >>
> >> The right fix is for bios-tables-tests to pass an explicit 'osk' value
> >> then. As its a test it doesn't have to be a genuine OSK, jsut any old
> >> 64-byte string
> >>
> >> diff --git a/tests/qtest/bios-tables-test.c
> >> b/tests/qtest/bios-tables-test.c
> >> index 359916c228..f6b5adf200 100644
> >> --- a/tests/qtest/bios-tables-test.c
> >> +++ b/tests/qtest/bios-tables-test.c
> >> @@ -1632,7 +1632,7 @@ static void test_acpi_q35_applesmc(void)
> >> .variant = ".applesmc",
> >> };
> >>
> >> -test_acpi_one("-device isa-applesmc", );
> >> +test_acpi_one("-device
> >> isa-applesmc,osk=iamalsonottherealoskimjustheretostopbiostablestestspammingstderr",
> >> );
> >> free_test_data();
> >
> > We should either have a comment saying that this has to be exactly
> > 64 characters and it doesn't matter what they are; or we could use
> > any64characterfakeoskisenoughtopreventinvalidkeywarningsonstderr
> >
> > :-)
>
> I applaud the renaissance of roman-style inscriptions, but it's not just
> words without spaces, it's also capital letters only:
>
> ANY64CHARACTERFAKEOSKISENOUGHTOPREVENTINVALIDKEYWARNINGSONSTDERR
>
> Seriously, throw in some dashes or spaces.
too late, but added a comment above hieroglyphs style string
>
Re: [PATCH for-7.1] applesmc: silence invalid key warning in case default one is used
On Thu, 28 Jul 2022 at 14:30, Markus Armbruster wrote: > Peter Maydell writes: > I applaud the renaissance of roman-style inscriptions, but it's not just > words without spaces, it's also capital letters only: > > ANY64CHARACTERFAKEOSKISENOUGHTOPREVENTINVALIDKEYWARNINGSONSTDERR > > Seriously, throw in some dashes or spaces. any-64-char-fake-osk-will-avoid-an-invalid-key-warning-on-stderr -- PMM
Re: [PATCH for-7.1] applesmc: silence invalid key warning in case default one is used
Peter Maydell writes:
> On Thu, 28 Jul 2022 at 11:23, Daniel P. Berrangé wrote:
>>
>> On Thu, Jul 28, 2022 at 11:05:13AM +0100, Peter Maydell wrote:
>> > On Thu, 28 Jul 2022 at 10:48, Daniel P. Berrangé
>> > wrote:
>> > >
>> > > On Thu, Jul 28, 2022 at 05:35:58AM -0400, Igor Mammedov wrote:
>> > > > QEMU probably can't carry OSK key[1] for legal reasons so it
>> > > > can't supply the valid default key. However when tests are run
>> > > > applesmc will pollute test log with distracting warning,
>> > > > silence that warning so it won't distract maintainers/CI.
>> > >
>> > > What test is causing this problem ?
>> >
>> > bios-tables-test -- see here for the relevant bit of the log:
>> >
>> > https://lore.kernel.org/qemu-devel/CAFEAcA8u8jm7b+JD_t0qMNMy+WSJPOw=qxqptZpwTp=tkcx...@mail.gmail.com/
>>
>> The right fix is for bios-tables-tests to pass an explicit 'osk' value
>> then. As its a test it doesn't have to be a genuine OSK, jsut any old
>> 64-byte string
>>
>> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
>> index 359916c228..f6b5adf200 100644
>> --- a/tests/qtest/bios-tables-test.c
>> +++ b/tests/qtest/bios-tables-test.c
>> @@ -1632,7 +1632,7 @@ static void test_acpi_q35_applesmc(void)
>> .variant = ".applesmc",
>> };
>>
>> -test_acpi_one("-device isa-applesmc", );
>> +test_acpi_one("-device
>> isa-applesmc,osk=iamalsonottherealoskimjustheretostopbiostablestestspammingstderr",
>> );
>> free_test_data();
>
> We should either have a comment saying that this has to be exactly
> 64 characters and it doesn't matter what they are; or we could use
> any64characterfakeoskisenoughtopreventinvalidkeywarningsonstderr
>
> :-)
I applaud the renaissance of roman-style inscriptions, but it's not just
words without spaces, it's also capital letters only:
ANY64CHARACTERFAKEOSKISENOUGHTOPREVENTINVALIDKEYWARNINGSONSTDERR
Seriously, throw in some dashes or spaces.
Re: [PATCH for-7.1] applesmc: silence invalid key warning in case default one is used
On Thu, Jul 28, 2022 at 02:00:37PM +0200, Igor Mammedov wrote:
> On Thu, 28 Jul 2022 11:23:00 +0100
> Daniel P. Berrangé wrote:
>
> > On Thu, Jul 28, 2022 at 11:05:13AM +0100, Peter Maydell wrote:
> > > On Thu, 28 Jul 2022 at 10:48, Daniel P. Berrangé
> > > wrote:
> > > >
> > > > On Thu, Jul 28, 2022 at 05:35:58AM -0400, Igor Mammedov wrote:
> > > > > QEMU probably can't carry OSK key[1] for legal reasons so it
> > > > > can't supply the valid default key. However when tests are run
> > > > > applesmc will pollute test log with distracting warning,
> > > > > silence that warning so it won't distract maintainers/CI.
> > > >
> > > > What test is causing this problem ?
> > >
> > > bios-tables-test -- see here for the relevant bit of the log:
> > >
> > > https://lore.kernel.org/qemu-devel/CAFEAcA8u8jm7b+JD_t0qMNMy+WSJPOw=qxqptZpwTp=tkcx...@mail.gmail.com/
> > >
> >
> > The right fix is for bios-tables-tests to pass an explicit 'osk' value
> > then. As its a test it doesn't have to be a genuine OSK, jsut any old
> > 64-byte string
> >
> > diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> > index 359916c228..f6b5adf200 100644
> > --- a/tests/qtest/bios-tables-test.c
> > +++ b/tests/qtest/bios-tables-test.c
> > @@ -1632,7 +1632,7 @@ static void test_acpi_q35_applesmc(void)
> > .variant = ".applesmc",
> > };
> >
> > -test_acpi_one("-device isa-applesmc", );
> > +test_acpi_one("-device
> > isa-applesmc,osk=iamalsonottherealoskimjustheretostopbiostablestestspammingstderr",
> > );
> > free_test_data();
> > }
>
> that will work, care tho send a formal patch or should I take over?
Could you spin something up, as I'm not in a position to do formal
testing today.
> However we still have bogus default_osk, yes it will cause warning which
> typically nobody will see and end user will still end up with upset guest.
> Right thing would be to require osk explicitly and drop default completely.
> Users who actually run MacOS guest must be providing OSK explicitly already
> so they won't be affected and anyone else using default is broken anyways
> (whether QEMU started directly or through mgmt layer)
There are other patches onlist to make QEMU extract an osk from the
host hardware, which is ok because IIUC macOS allows you to run macOS
as a VM, provided the host is Apple hardware.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH for-7.1] applesmc: silence invalid key warning in case default one is used
On Thu, 28 Jul 2022 11:23:00 +0100
Daniel P. Berrangé wrote:
> On Thu, Jul 28, 2022 at 11:05:13AM +0100, Peter Maydell wrote:
> > On Thu, 28 Jul 2022 at 10:48, Daniel P. Berrangé
> > wrote:
> > >
> > > On Thu, Jul 28, 2022 at 05:35:58AM -0400, Igor Mammedov wrote:
> > > > QEMU probably can't carry OSK key[1] for legal reasons so it
> > > > can't supply the valid default key. However when tests are run
> > > > applesmc will pollute test log with distracting warning,
> > > > silence that warning so it won't distract maintainers/CI.
> > >
> > > What test is causing this problem ?
> >
> > bios-tables-test -- see here for the relevant bit of the log:
> >
> > https://lore.kernel.org/qemu-devel/CAFEAcA8u8jm7b+JD_t0qMNMy+WSJPOw=qxqptZpwTp=tkcx...@mail.gmail.com/
> >
>
> The right fix is for bios-tables-tests to pass an explicit 'osk' value
> then. As its a test it doesn't have to be a genuine OSK, jsut any old
> 64-byte string
>
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index 359916c228..f6b5adf200 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -1632,7 +1632,7 @@ static void test_acpi_q35_applesmc(void)
> .variant = ".applesmc",
> };
>
> -test_acpi_one("-device isa-applesmc", );
> +test_acpi_one("-device
> isa-applesmc,osk=iamalsonottherealoskimjustheretostopbiostablestestspammingstderr",
> );
> free_test_data();
> }
that will work, care tho send a formal patch or should I take over?
However we still have bogus default_osk, yes it will cause warning which
typically nobody will see and end user will still end up with upset guest.
Right thing would be to require osk explicitly and drop default completely.
Users who actually run MacOS guest must be providing OSK explicitly already
so they won't be affected and anyone else using default is broken anyways
(whether QEMU started directly or through mgmt layer)
>
>
> With regards,
> Daniel
Re: [PATCH for-7.1] applesmc: silence invalid key warning in case default one is used
On Thu, 28 Jul 2022 10:48:27 +0100
Daniel P. Berrangé wrote:
> On Thu, Jul 28, 2022 at 05:35:58AM -0400, Igor Mammedov wrote:
> > QEMU probably can't carry OSK key[1] for legal reasons so it
> > can't supply the valid default key. However when tests are run
> > applesmc will pollute test log with distracting warning,
> > silence that warning so it won't distract maintainers/CI.
>
> What test is causing this problem ?
>
> >
> > If user launches MacOS guest without supplying valid or any key
> > they will get upset MacOS (and legality of specifying valid key
> > on CLI is also questionable). This patch favors QEMU/tests vs
> > MacOS guest.
> >
> > A patch from Apple needed to change default to the valid key,
> > until then prefer QEMU maintainers sanity/cleaner test logs
> > vs any inconvenience users running MacOS guest might suffer.
> >
> > 1) After some googling, it seems 'secret' key is the same for
> >all Mac hardware and is widely known but whether it's
> >permited to use that is up to lawyers/Apple to decide.
> >
> > Signed-off-by: Igor Mammedov
> > ---
> > hw/misc/applesmc.c | 8 +---
> > 1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
> > index 5f9c742e50..bce27b0950 100644
> > --- a/hw/misc/applesmc.c
> > +++ b/hw/misc/applesmc.c
> > @@ -84,7 +84,7 @@ enum {
> > #define smc_debug(...) do { } while (0)
> > #endif
> >
> > -static char default_osk[64] = "This is a dummy key. Enter the real key "
> > +static char default_osk[65] = "This is a dummy key. Enter the real key "
> >"using the -osk parameter";
>
> This is a distinct bug fix from the problem described. We should
> take this as a standalone fix no matter what.
thought about it (until I started to write commit message for that)
but it becomes bug only if default_osk is treated as NULL terminated
string, which happens only if following hunk is applied.
Without that, it's treated as 64 bytes. (we can null-terminate it
anyways just to be on a safe side)
> > struct AppleSMCData {
> > @@ -335,10 +335,12 @@ static void applesmc_isa_realize(DeviceState *dev,
> > Error **errp)
> > isa_register_ioport(>parent_obj, >io_err,
> > s->iobase + APPLESMC_ERR_PORT);
> >
> > -if (!s->osk || (strlen(s->osk) != 64)) {
> > -warn_report("Using AppleSMC with invalid key");
> > +if (!s->osk) {
> > s->osk = default_osk;
> > }
> > +if (strlen(s->osk) != 64) {
> > +warn_report("Using AppleSMC with invalid key");
> > +}
>
> This means users who invoke QEMU won't see the warning any more. This
> feels like a dubious change, as it degrades the user experiance for
> sake of tests - IMHO users takes priority over any testing.
>
> With regards,
> Daniel
Re: [PATCH for-7.1] applesmc: silence invalid key warning in case default one is used
On Thu, 28 Jul 2022 at 11:23, Daniel P. Berrangé wrote:
>
> On Thu, Jul 28, 2022 at 11:05:13AM +0100, Peter Maydell wrote:
> > On Thu, 28 Jul 2022 at 10:48, Daniel P. Berrangé
> > wrote:
> > >
> > > On Thu, Jul 28, 2022 at 05:35:58AM -0400, Igor Mammedov wrote:
> > > > QEMU probably can't carry OSK key[1] for legal reasons so it
> > > > can't supply the valid default key. However when tests are run
> > > > applesmc will pollute test log with distracting warning,
> > > > silence that warning so it won't distract maintainers/CI.
> > >
> > > What test is causing this problem ?
> >
> > bios-tables-test -- see here for the relevant bit of the log:
> >
> > https://lore.kernel.org/qemu-devel/CAFEAcA8u8jm7b+JD_t0qMNMy+WSJPOw=qxqptZpwTp=tkcx...@mail.gmail.com/
>
> The right fix is for bios-tables-tests to pass an explicit 'osk' value
> then. As its a test it doesn't have to be a genuine OSK, jsut any old
> 64-byte string
>
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index 359916c228..f6b5adf200 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -1632,7 +1632,7 @@ static void test_acpi_q35_applesmc(void)
> .variant = ".applesmc",
> };
>
> -test_acpi_one("-device isa-applesmc", );
> +test_acpi_one("-device
> isa-applesmc,osk=iamalsonottherealoskimjustheretostopbiostablestestspammingstderr",
> );
> free_test_data();
We should either have a comment saying that this has to be exactly
64 characters and it doesn't matter what they are; or we could use
any64characterfakeoskisenoughtopreventinvalidkeywarningsonstderr
:-)
-- PMM
Re: [PATCH for-7.1] applesmc: silence invalid key warning in case default one is used
On Thu, Jul 28, 2022 at 11:05:13AM +0100, Peter Maydell wrote:
> On Thu, 28 Jul 2022 at 10:48, Daniel P. Berrangé wrote:
> >
> > On Thu, Jul 28, 2022 at 05:35:58AM -0400, Igor Mammedov wrote:
> > > QEMU probably can't carry OSK key[1] for legal reasons so it
> > > can't supply the valid default key. However when tests are run
> > > applesmc will pollute test log with distracting warning,
> > > silence that warning so it won't distract maintainers/CI.
> >
> > What test is causing this problem ?
>
> bios-tables-test -- see here for the relevant bit of the log:
>
> https://lore.kernel.org/qemu-devel/CAFEAcA8u8jm7b+JD_t0qMNMy+WSJPOw=qxqptZpwTp=tkcx...@mail.gmail.com/
The right fix is for bios-tables-tests to pass an explicit 'osk' value
then. As its a test it doesn't have to be a genuine OSK, jsut any old
64-byte string
diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index 359916c228..f6b5adf200 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -1632,7 +1632,7 @@ static void test_acpi_q35_applesmc(void)
.variant = ".applesmc",
};
-test_acpi_one("-device isa-applesmc", );
+test_acpi_one("-device
isa-applesmc,osk=iamalsonottherealoskimjustheretostopbiostablestestspammingstderr",
);
free_test_data();
}
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH for-7.1] applesmc: silence invalid key warning in case default one is used
On Thu, 28 Jul 2022 at 10:48, Daniel P. Berrangé wrote: > > On Thu, Jul 28, 2022 at 05:35:58AM -0400, Igor Mammedov wrote: > > QEMU probably can't carry OSK key[1] for legal reasons so it > > can't supply the valid default key. However when tests are run > > applesmc will pollute test log with distracting warning, > > silence that warning so it won't distract maintainers/CI. > > What test is causing this problem ? bios-tables-test -- see here for the relevant bit of the log: https://lore.kernel.org/qemu-devel/CAFEAcA8u8jm7b+JD_t0qMNMy+WSJPOw=qxqptZpwTp=tkcx...@mail.gmail.com/ -- PMM
Re: [PATCH for-7.1] applesmc: silence invalid key warning in case default one is used
On Thu, Jul 28, 2022 at 05:35:58AM -0400, Igor Mammedov wrote:
> QEMU probably can't carry OSK key[1] for legal reasons so it
> can't supply the valid default key. However when tests are run
> applesmc will pollute test log with distracting warning,
> silence that warning so it won't distract maintainers/CI.
What test is causing this problem ?
>
> If user launches MacOS guest without supplying valid or any key
> they will get upset MacOS (and legality of specifying valid key
> on CLI is also questionable). This patch favors QEMU/tests vs
> MacOS guest.
>
> A patch from Apple needed to change default to the valid key,
> until then prefer QEMU maintainers sanity/cleaner test logs
> vs any inconvenience users running MacOS guest might suffer.
>
> 1) After some googling, it seems 'secret' key is the same for
>all Mac hardware and is widely known but whether it's
>permited to use that is up to lawyers/Apple to decide.
>
> Signed-off-by: Igor Mammedov
> ---
> hw/misc/applesmc.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
> index 5f9c742e50..bce27b0950 100644
> --- a/hw/misc/applesmc.c
> +++ b/hw/misc/applesmc.c
> @@ -84,7 +84,7 @@ enum {
> #define smc_debug(...) do { } while (0)
> #endif
>
> -static char default_osk[64] = "This is a dummy key. Enter the real key "
> +static char default_osk[65] = "This is a dummy key. Enter the real key "
>"using the -osk parameter";
This is a distinct bug fix from the problem described. We should
take this as a standalone fix no matter what.
> struct AppleSMCData {
> @@ -335,10 +335,12 @@ static void applesmc_isa_realize(DeviceState *dev,
> Error **errp)
> isa_register_ioport(>parent_obj, >io_err,
> s->iobase + APPLESMC_ERR_PORT);
>
> -if (!s->osk || (strlen(s->osk) != 64)) {
> -warn_report("Using AppleSMC with invalid key");
> +if (!s->osk) {
> s->osk = default_osk;
> }
> +if (strlen(s->osk) != 64) {
> +warn_report("Using AppleSMC with invalid key");
> +}
This means users who invoke QEMU won't see the warning any more. This
feels like a dubious change, as it degrades the user experiance for
sake of tests - IMHO users takes priority over any testing.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
