[PATCH] CHANGELOG.md: Make PV shim smaller by factoring out HVM-specific shadow code

[PATCH] CHANGELOG.md: Make PV shim smaller by factoring out HVM-specific shadow code

[George Dunlap]

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
Missed one from my list when creating the other series

CC: Ian Jackson <ian.jackson@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Roger Pau Monne <roger.pau@citrix.com>
---
 CHANGELOG.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 15a22d6bde..49832ae017 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -18,6 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
  - x86_emulate: Expanded testing for several instruction classes
  - CI loop: Add Alpine Linux, Ubuntu Focal targets; drop CentOS 6
  - CI loop: Add dom0less aarch64 smoke test
+ - Factored out HVM-specific shadow code, allowing PV shim to be slimmer
 
 ## Removed / support downgraded
 
-- 
2.30.2

Re: [PATCH] CHANGELOG.md: Make PV shim smaller by factoring out HVM-specific shadow code

[Jan Beulich]

On 24.03.2021 18:26, George Dunlap wrote:
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> ---
> Missed one from my list when creating the other series
> 
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Roger Pau Monne <roger.pau@citrix.com>
> ---
>  CHANGELOG.md | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/CHANGELOG.md b/CHANGELOG.md
> index 15a22d6bde..49832ae017 100644
> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -18,6 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
>   - x86_emulate: Expanded testing for several instruction classes
>   - CI loop: Add Alpine Linux, Ubuntu Focal targets; drop CentOS 6
>   - CI loop: Add dom0less aarch64 smoke test
> + - Factored out HVM-specific shadow code, allowing PV shim to be slimmer

But shadow code doesn't get included by default in shim-exclusive
builds (and others are unlikely to disable HVM).

Also, just to mention it - some of the patches in the direction
of !HVM builds getting smaller are still pending. They've been
acked (for the most part at least), but couldn't be posted in
time (sitting behind the PV guest accessors changes, which were
waiting for a decision security-wise).

Jan

Re: [PATCH] CHANGELOG.md: Make PV shim smaller by factoring out HVM-specific shadow code

[George Dunlap]

> On Mar 25, 2021, at 7:08 AM, Jan Beulich <JBeulich@suse.com> wrote:
> 
> On 24.03.2021 18:26, George Dunlap wrote:
>> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
>> ---
>> Missed one from my list when creating the other series
>> 
>> CC: Ian Jackson <ian.jackson@citrix.com>
>> CC: Jan Beulich <jbeulich@suse.com>
>> CC: Roger Pau Monne <roger.pau@citrix.com>
>> ---
>> CHANGELOG.md | 1 +
>> 1 file changed, 1 insertion(+)
>> 
>> diff --git a/CHANGELOG.md b/CHANGELOG.md
>> index 15a22d6bde..49832ae017 100644
>> --- a/CHANGELOG.md
>> +++ b/CHANGELOG.md
>> @@ -18,6 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
>>  - x86_emulate: Expanded testing for several instruction classes
>>  - CI loop: Add Alpine Linux, Ubuntu Focal targets; drop CentOS 6
>>  - CI loop: Add dom0less aarch64 smoke test
>> + - Factored out HVM-specific shadow code, allowing PV shim to be slimmer
> 
> But shadow code doesn't get included by default in shim-exclusive
> builds (and others are unlikely to disable HVM).

Can you propose some better text please?

 -George

Re: [PATCH] CHANGELOG.md: Make PV shim smaller by factoring out HVM-specific shadow code

[Jan Beulich]

On 29.03.2021 18:12, George Dunlap wrote:
>> On Mar 25, 2021, at 7:08 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> On 24.03.2021 18:26, George Dunlap wrote:
>>> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
>>> ---
>>> Missed one from my list when creating the other series
>>>
>>> CC: Ian Jackson <ian.jackson@citrix.com>
>>> CC: Jan Beulich <jbeulich@suse.com>
>>> CC: Roger Pau Monne <roger.pau@citrix.com>
>>> ---
>>> CHANGELOG.md | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/CHANGELOG.md b/CHANGELOG.md
>>> index 15a22d6bde..49832ae017 100644
>>> --- a/CHANGELOG.md
>>> +++ b/CHANGELOG.md
>>> @@ -18,6 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
>>>  - x86_emulate: Expanded testing for several instruction classes
>>>  - CI loop: Add Alpine Linux, Ubuntu Focal targets; drop CentOS 6
>>>  - CI loop: Add dom0less aarch64 smoke test
>>> + - Factored out HVM-specific shadow code, allowing PV shim to be slimmer
>>
>> But shadow code doesn't get included by default in shim-exclusive
>> builds (and others are unlikely to disable HVM).
> 
> Can you propose some better text please?

Does this need mentioning here in the first place?

Jan

Re: [PATCH] CHANGELOG.md: Make PV shim smaller by factoring out HVM-specific shadow code

[Andrew Cooper]

On 29/03/2021 17:23, Jan Beulich wrote:
> On 29.03.2021 18:12, George Dunlap wrote:
>>> On Mar 25, 2021, at 7:08 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>> On 24.03.2021 18:26, George Dunlap wrote:
>>>> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
>>>> ---
>>>> Missed one from my list when creating the other series
>>>>
>>>> CC: Ian Jackson <ian.jackson@citrix.com>
>>>> CC: Jan Beulich <jbeulich@suse.com>
>>>> CC: Roger Pau Monne <roger.pau@citrix.com>
>>>> ---
>>>> CHANGELOG.md | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/CHANGELOG.md b/CHANGELOG.md
>>>> index 15a22d6bde..49832ae017 100644
>>>> --- a/CHANGELOG.md
>>>> +++ b/CHANGELOG.md
>>>> @@ -18,6 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
>>>>  - x86_emulate: Expanded testing for several instruction classes
>>>>  - CI loop: Add Alpine Linux, Ubuntu Focal targets; drop CentOS 6
>>>>  - CI loop: Add dom0less aarch64 smoke test
>>>> + - Factored out HVM-specific shadow code, allowing PV shim to be slimmer
>>> But shadow code doesn't get included by default in shim-exclusive
>>> builds (and others are unlikely to disable HVM).
>> Can you propose some better text please?
> Does this need mentioning here in the first place?

I would recommend not.

We've been doing incremental improvements for the shim for several
releases now, and in this case, we're literally talking a few kb of
code.  As we already align to 2M boundaries for superpage reasons, there
almost certainly isn't actually a reduction in runtime size.

~Andrew

Re: [PATCH] CHANGELOG.md: Make PV shim smaller by factoring out HVM-specific shadow code

[George Dunlap]

> On Mar 29, 2021, at 6:26 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> 
> On 29/03/2021 17:23, Jan Beulich wrote:
>> On 29.03.2021 18:12, George Dunlap wrote:
>>>> On Mar 25, 2021, at 7:08 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 24.03.2021 18:26, George Dunlap wrote:
>>>>> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
>>>>> ---
>>>>> Missed one from my list when creating the other series
>>>>> 
>>>>> CC: Ian Jackson <ian.jackson@citrix.com>
>>>>> CC: Jan Beulich <jbeulich@suse.com>
>>>>> CC: Roger Pau Monne <roger.pau@citrix.com>
>>>>> ---
>>>>> CHANGELOG.md | 1 +
>>>>> 1 file changed, 1 insertion(+)
>>>>> 
>>>>> diff --git a/CHANGELOG.md b/CHANGELOG.md
>>>>> index 15a22d6bde..49832ae017 100644
>>>>> --- a/CHANGELOG.md
>>>>> +++ b/CHANGELOG.md
>>>>> @@ -18,6 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
>>>>> - x86_emulate: Expanded testing for several instruction classes
>>>>> - CI loop: Add Alpine Linux, Ubuntu Focal targets; drop CentOS 6
>>>>> - CI loop: Add dom0less aarch64 smoke test
>>>>> + - Factored out HVM-specific shadow code, allowing PV shim to be slimmer
>>>> But shadow code doesn't get included by default in shim-exclusive
>>>> builds (and others are unlikely to disable HVM).
>>> Can you propose some better text please?
>> Does this need mentioning here in the first place?
> 
> I would recommend not.
> 
> We've been doing incremental improvements for the shim for several
> releases now, and in this case, we're literally talking a few kb of
> code.  As we already align to 2M boundaries for superpage reasons, there
> almost certainly isn't actually a reduction in runtime size.

I don’t understand why the two of you are downplaying your work so much.  Yes, these are all only incremental improvements; but they are improvements; and the cumulative effect of loads of incremental improvements can be significant.  Communicating to people just what the nature of all these incremental improvements are is important.

I mean, look at the release notes for Go 1.15 [1].  It includes things like this:

"JSEscape now consistently uses Unicode escapes (\u00XX), which are compatible with JSON."

"go test -v now groups output by test name, rather than printing the test name on each line."

Those sound far more trivial than “Even more shadow code has been moved to an HVM-specific file”.

If the approach is going to be “SUPER IMPORTANT SPECIAL STUFF ONLY", I’d recommend removing CHANGELOG.md.  Having an official list that says, “Well, really, we only did 2 things this release” is going to be actively harmful.  

 -George

[1] https://golang.org/doc/go1.15

Re: [PATCH] CHANGELOG.md: Make PV shim smaller by factoring out HVM-specific shadow code

[Jan Beulich]

On 30.03.2021 11:56, George Dunlap wrote:
> 
> 
>> On Mar 29, 2021, at 6:26 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>
>> On 29/03/2021 17:23, Jan Beulich wrote:
>>> On 29.03.2021 18:12, George Dunlap wrote:
>>>>> On Mar 25, 2021, at 7:08 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 24.03.2021 18:26, George Dunlap wrote:
>>>>>> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
>>>>>> ---
>>>>>> Missed one from my list when creating the other series
>>>>>>
>>>>>> CC: Ian Jackson <ian.jackson@citrix.com>
>>>>>> CC: Jan Beulich <jbeulich@suse.com>
>>>>>> CC: Roger Pau Monne <roger.pau@citrix.com>
>>>>>> ---
>>>>>> CHANGELOG.md | 1 +
>>>>>> 1 file changed, 1 insertion(+)
>>>>>>
>>>>>> diff --git a/CHANGELOG.md b/CHANGELOG.md
>>>>>> index 15a22d6bde..49832ae017 100644
>>>>>> --- a/CHANGELOG.md
>>>>>> +++ b/CHANGELOG.md
>>>>>> @@ -18,6 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
>>>>>> - x86_emulate: Expanded testing for several instruction classes
>>>>>> - CI loop: Add Alpine Linux, Ubuntu Focal targets; drop CentOS 6
>>>>>> - CI loop: Add dom0less aarch64 smoke test
>>>>>> + - Factored out HVM-specific shadow code, allowing PV shim to be slimmer
>>>>> But shadow code doesn't get included by default in shim-exclusive
>>>>> builds (and others are unlikely to disable HVM).
>>>> Can you propose some better text please?
>>> Does this need mentioning here in the first place?
>>
>> I would recommend not.
>>
>> We've been doing incremental improvements for the shim for several
>> releases now, and in this case, we're literally talking a few kb of
>> code.  As we already align to 2M boundaries for superpage reasons, there
>> almost certainly isn't actually a reduction in runtime size.
> 
> I don’t understand why the two of you are downplaying your work so much.  Yes, these are all only incremental improvements; but they are improvements; and the cumulative effect of loads of incremental improvements can be significant.  Communicating to people just what the nature of all these incremental improvements are is important.
> 
> I mean, look at the release notes for Go 1.15 [1].  It includes things like this:
> 
> "JSEscape now consistently uses Unicode escapes (\u00XX), which are compatible with JSON."
> 
> "go test -v now groups output by test name, rather than printing the test name on each line."
> 
> Those sound far more trivial than “Even more shadow code has been moved to an HVM-specific file”.
> 
> If the approach is going to be “SUPER IMPORTANT SPECIAL STUFF ONLY", I’d recommend removing CHANGELOG.md.  Having an official list that says, “Well, really, we only did 2 things this release” is going to be actively harmful.  

I don't think it needs to be "super important" only, but it ought to
be at least user visible / user recognizable imo.

Jan

Re: [PATCH] CHANGELOG.md: Make PV shim smaller by factoring out HVM-specific shadow code

[Ian Jackson]

George Dunlap writes ("Re: [PATCH] CHANGELOG.md: Make PV shim smaller by factoring out HVM-specific shadow code"):
> I don’t understand why the two of you are downplaying your work so much.  Yes, these are all only incremental improvements; but they are improvements; and the cumulative effect of loads of incremental improvements can be significant.  Communicating to people just what the nature of all these incremental improvements are is important.

I agree with George here.

There ae a number of reasons why behind-the-scenes work with little
(intentional) user-visible impact are useful to note in the
CHANGELOG.md.  With my Release Manager hat on I would like to see, for
example,

>> + - Factored out HVM-specific shadow code, allowing PV shim to be slimmer

something about htis work in the CHANGELOG.md.

IDK precisely, and I don't think George does either, what a good and
accurate statement is.  But I guess we will go with the text above if
we don't get something better.

George, were there other changelog items that were subject to a
a similar question ?  I don't find them in my email with a quick look
but I suspect I have missed one or two ?


Thanks,
Ian,

Re: [PATCH] CHANGELOG.md: Make PV shim smaller by factoring out HVM-specific shadow code

[Jan Beulich]

On 31.03.2021 15:52, Ian Jackson wrote:
> George Dunlap writes ("Re: [PATCH] CHANGELOG.md: Make PV shim smaller by factoring out HVM-specific shadow code"):
>> I don’t understand why the two of you are downplaying your work so much.  Yes, these are all only incremental improvements; but they are improvements; and the cumulative effect of loads of incremental improvements can be significant.  Communicating to people just what the nature of all these incremental improvements are is important.
> 
> I agree with George here.
> 
> There ae a number of reasons why behind-the-scenes work with little
> (intentional) user-visible impact are useful to note in the
> CHANGELOG.md.  With my Release Manager hat on I would like to see, for
> example,
> 
>>> + - Factored out HVM-specific shadow code, allowing PV shim to be slimmer
> 
> something about htis work in the CHANGELOG.md.
> 
> IDK precisely, and I don't think George does either, what a good and
> accurate statement is.  But I guess we will go with the text above if
> we don't get something better.

At the very least the part after the comma ought to be deleted. As
said in an earlier reply, at least the shim default config disables
shadow code anyway, so the factoring out has no effect there.

Jan

Re: [PATCH] CHANGELOG.md: Make PV shim smaller by factoring out HVM-specific shadow code

[George Dunlap]

> On Mar 31, 2021, at 2:54 PM, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 31.03.2021 15:52, Ian Jackson wrote:
>> George Dunlap writes ("Re: [PATCH] CHANGELOG.md: Make PV shim smaller by factoring out HVM-specific shadow code"):
>>> I don’t understand why the two of you are downplaying your work so much. Yes, these are all only incremental improvements; but they are improvements; and the cumulative effect of loads of incremental improvements can be significant.  Communicating to people just what the nature of all these incremental improvements are is important.
>> 
>> I agree with George here.
>> 
>> There ae a number of reasons why behind-the-scenes work with little
>> (intentional) user-visible impact are useful to note in the
>> CHANGELOG.md.  With my Release Manager hat on I would like to see, for
>> example,
>> 
>>>> + - Factored out HVM-specific shadow code, allowing PV shim to be slimmer
>> 
>> something about htis work in the CHANGELOG.md.
>> 
>> IDK precisely, and I don't think George does either, what a good and
>> accurate statement is.  But I guess we will go with the text above if
>> we don't get something better.
> 
> At the very least the part after the comma ought to be deleted. As
> said in an earlier reply, at least the shim default config disables
> shadow code anyway, so the factoring out has no effect there.

Thanks.  So when you wrote the series, what was your motivation?  Did you have a particular technical outcome in mind?  Or did it just bother you that there was HVM-only code in a PV-only build? :-)

 -George

Re: [PATCH] CHANGELOG.md: Make PV shim smaller by factoring out HVM-specific shadow code

[Jan Beulich]

On 31.03.2021 16:00, George Dunlap wrote:
> 
> 
>> On Mar 31, 2021, at 2:54 PM, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 31.03.2021 15:52, Ian Jackson wrote:
>>> George Dunlap writes ("Re: [PATCH] CHANGELOG.md: Make PV shim smaller by factoring out HVM-specific shadow code"):
>>>> I don’t understand why the two of you are downplaying your work so much. Yes, these are all only incremental improvements; but they are improvements; and the cumulative effect of loads of incremental improvements can be significant.  Communicating to people just what the nature of all these incremental improvements are is important.
>>>
>>> I agree with George here.
>>>
>>> There ae a number of reasons why behind-the-scenes work with little
>>> (intentional) user-visible impact are useful to note in the
>>> CHANGELOG.md.  With my Release Manager hat on I would like to see, for
>>> example,
>>>
>>>>> + - Factored out HVM-specific shadow code, allowing PV shim to be slimmer
>>>
>>> something about htis work in the CHANGELOG.md.
>>>
>>> IDK precisely, and I don't think George does either, what a good and
>>> accurate statement is.  But I guess we will go with the text above if
>>> we don't get something better.
>>
>> At the very least the part after the comma ought to be deleted. As
>> said in an earlier reply, at least the shim default config disables
>> shadow code anyway, so the factoring out has no effect there.
> 
> Thanks.  So when you wrote the series, what was your motivation?  Did you have a particular technical outcome in mind?  Or did it just bother you that there was HVM-only code in a PV-only build? :-)

What bothers me are more the implications - it being rather hard in
many cases, and in particular in shadow code, to be able to tell what
paths are involved in the handling of what kind(s) of guests. This
has made more time consuming investigation of (suspected) misbehavior
in more than one case.

Jan

Re: [PATCH] CHANGELOG.md: Make PV shim smaller by factoring out HVM-specific shadow code

[George Dunlap]

> On Mar 31, 2021, at 3:06 PM, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 31.03.2021 16:00, George Dunlap wrote:
>> 
>> 
>>> On Mar 31, 2021, at 2:54 PM, Jan Beulich <jbeulich@suse.com> wrote:
>>> 
>>> On 31.03.2021 15:52, Ian Jackson wrote:
>>>> George Dunlap writes ("Re: [PATCH] CHANGELOG.md: Make PV shim smaller by factoring out HVM-specific shadow code"):
>>>>> I don’t understand why the two of you are downplaying your work so much. Yes, these are all only incremental improvements; but they are improvements; and the cumulative effect of loads of incremental improvements can be significant.  Communicating to people just what the nature of all these incremental improvements are is important.
>>>> 
>>>> I agree with George here.
>>>> 
>>>> There ae a number of reasons why behind-the-scenes work with little
>>>> (intentional) user-visible impact are useful to note in the
>>>> CHANGELOG.md.  With my Release Manager hat on I would like to see, for
>>>> example,
>>>> 
>>>>>> + - Factored out HVM-specific shadow code, allowing PV shim to be slimmer
>>>> 
>>>> something about htis work in the CHANGELOG.md.
>>>> 
>>>> IDK precisely, and I don't think George does either, what a good and
>>>> accurate statement is.  But I guess we will go with the text above if
>>>> we don't get something better.
>>> 
>>> At the very least the part after the comma ought to be deleted. As
>>> said in an earlier reply, at least the shim default config disables
>>> shadow code anyway, so the factoring out has no effect there.
>> 
>> Thanks.  So when you wrote the series, what was your motivation?  Did you have a particular technical outcome in mind?  Or did it just bother you that there was HVM-only code in a PV-only build? :-)
> 
> What bothers me are more the implications - it being rather hard in
> many cases, and in particular in shadow code, to be able to tell what
> paths are involved in the handling of what kind(s) of guests. This
> has made more time consuming investigation of (suspected) misbehavior
> in more than one case.

OK, so how about:

- Factored out HVM-specific shadow code, improving code clarity and reducing the size of PV-only hypervisor builds

 -George

Re: [PATCH] CHANGELOG.md: Make PV shim smaller by factoring out HVM-specific shadow code

[Jan Beulich]

On 31.03.2021 16:30, George Dunlap wrote:
> 
> 
>> On Mar 31, 2021, at 3:06 PM, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 31.03.2021 16:00, George Dunlap wrote:
>>>
>>>
>>>> On Mar 31, 2021, at 2:54 PM, Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 31.03.2021 15:52, Ian Jackson wrote:
>>>>> George Dunlap writes ("Re: [PATCH] CHANGELOG.md: Make PV shim smaller by factoring out HVM-specific shadow code"):
>>>>>> I don’t understand why the two of you are downplaying your work so much. Yes, these are all only incremental improvements; but they are improvements; and the cumulative effect of loads of incremental improvements can be significant.  Communicating to people just what the nature of all these incremental improvements are is important.
>>>>>
>>>>> I agree with George here.
>>>>>
>>>>> There ae a number of reasons why behind-the-scenes work with little
>>>>> (intentional) user-visible impact are useful to note in the
>>>>> CHANGELOG.md.  With my Release Manager hat on I would like to see, for
>>>>> example,
>>>>>
>>>>>>> + - Factored out HVM-specific shadow code, allowing PV shim to be slimmer
>>>>>
>>>>> something about htis work in the CHANGELOG.md.
>>>>>
>>>>> IDK precisely, and I don't think George does either, what a good and
>>>>> accurate statement is.  But I guess we will go with the text above if
>>>>> we don't get something better.
>>>>
>>>> At the very least the part after the comma ought to be deleted. As
>>>> said in an earlier reply, at least the shim default config disables
>>>> shadow code anyway, so the factoring out has no effect there.
>>>
>>> Thanks.  So when you wrote the series, what was your motivation?  Did you have a particular technical outcome in mind?  Or did it just bother you that there was HVM-only code in a PV-only build? :-)
>>
>> What bothers me are more the implications - it being rather hard in
>> many cases, and in particular in shadow code, to be able to tell what
>> paths are involved in the handling of what kind(s) of guests. This
>> has made more time consuming investigation of (suspected) misbehavior
>> in more than one case.
> 
> OK, so how about:
> 
> - Factored out HVM-specific shadow code, improving code clarity and reducing the size of PV-only hypervisor builds

This sounds okay to me.

Thanks, Jan