[Xen-devel] [PATCH for 4.13] x86/shim: don't reserve 640k - 1M region in E820

[Xen-devel] [PATCH for 4.13] x86/shim: don't reserve 640k - 1M region in E820

[Sergey Dyasli]

Converting a guest from PV to PV-in-PVH makes the guest to have 384k
less memory, which may confuse guest's balloon driver. This happens
because Xen unconditionally reserves 640k - 1M region in E820 despite
the fact that it's really a usable RAM in PVH boot mode.

Fix this by skipping the region type change for pv-shim mode.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
The condition can be relaxed to be just !pvh_boot if there are no
plans to support VGA MMIO / ROMs for PVH guests.

CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: "Roger Pau Monné" <roger.pau@citrix.com>
CC: Juergen Gross <jgross@suse.com>
---
 xen/arch/x86/e820.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
index 8e8a2c4e1b..54c2ae70c4 100644
--- a/xen/arch/x86/e820.c
+++ b/xen/arch/x86/e820.c
@@ -318,9 +318,10 @@ static int __init copy_e820_map(struct e820entry * biosmap, unsigned int nr_map)
 
         /*
          * Some BIOSes claim RAM in the 640k - 1M region.
-         * Not right. Fix it up.
+         * Not right. Fix it up, except for pv-shim case.
          */
-        if (type == E820_RAM) {
+        if ( !(pv_shim && pvh_boot) && type == E820_RAM )
+        {
             if (start < 0x100000ULL && end > 0xA0000ULL) {
                 if (start < 0xA0000ULL)
                     add_memory_region(start, 0xA0000ULL-start, type);
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for 4.13] x86/shim: don't reserve 640k - 1M region in E820

[Jan Beulich]

On 28.10.2019 09:56, Sergey Dyasli wrote:
> Converting a guest from PV to PV-in-PVH makes the guest to have 384k
> less memory, which may confuse guest's balloon driver. This happens
> because Xen unconditionally reserves 640k - 1M region in E820 despite
> the fact that it's really a usable RAM in PVH boot mode.

This is a PVH property according to your description and according
to my understanding. Why would you then ...

> Fix this by skipping the region type change for pv-shim mode.

... alter behavior for shim mode only, rather than when booted in
PVH mode in general?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for 4.13] x86/shim: don't reserve 640k - 1M region in E820

[Sergey Dyasli]

On 28/10/2019 09:06, Jan Beulich wrote:
> On 28.10.2019 09:56, Sergey Dyasli wrote:
>> Converting a guest from PV to PV-in-PVH makes the guest to have 384k
>> less memory, which may confuse guest's balloon driver. This happens
>> because Xen unconditionally reserves 640k - 1M region in E820 despite
>> the fact that it's really a usable RAM in PVH boot mode.
> 
> This is a PVH property according to your description and according
> to my understanding. Why would you then ...
> 
>> Fix this by skipping the region type change for pv-shim mode.
> 
> ... alter behavior for shim mode only, rather than when booted in
> PVH mode in general?

This is just me being cautious.

My original email does have this text after ---
"The condition can be relaxed to be just !pvh_boot if there are no
plans to support VGA MMIO / ROMs for PVH guests."

I wanted to check with the community about this first.

Thanks,
Sergey

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for 4.13] x86/shim: don't reserve 640k - 1M region in E820

[Jan Beulich]

On 28.10.2019 12:08, Sergey Dyasli wrote:
> On 28/10/2019 09:06, Jan Beulich wrote:
>> On 28.10.2019 09:56, Sergey Dyasli wrote:
>>> Converting a guest from PV to PV-in-PVH makes the guest to have 384k
>>> less memory, which may confuse guest's balloon driver. This happens
>>> because Xen unconditionally reserves 640k - 1M region in E820 despite
>>> the fact that it's really a usable RAM in PVH boot mode.
>>
>> This is a PVH property according to your description and according
>> to my understanding. Why would you then ...
>>
>>> Fix this by skipping the region type change for pv-shim mode.
>>
>> ... alter behavior for shim mode only, rather than when booted in
>> PVH mode in general?
> 
> This is just me being cautious.
> 
> My original email does have this text after ---
> "The condition can be relaxed to be just !pvh_boot if there are no
> plans to support VGA MMIO / ROMs for PVH guests."

I doubt emulated ones are intended to be supported, but of
course a graphics card could be passed through. Iirc passthrough
is a pending work item still anyway for PVH, so dealing with the
case right now may not even be necessary.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for 4.13] x86/shim: don't reserve 640k - 1M region in E820

[Andrew Cooper]

On 28/10/2019 11:11, Jan Beulich wrote:
> On 28.10.2019 12:08, Sergey Dyasli wrote:
>> On 28/10/2019 09:06, Jan Beulich wrote:
>>> On 28.10.2019 09:56, Sergey Dyasli wrote:
>>>> Converting a guest from PV to PV-in-PVH makes the guest to have 384k
>>>> less memory, which may confuse guest's balloon driver. This happens
>>>> because Xen unconditionally reserves 640k - 1M region in E820 despite
>>>> the fact that it's really a usable RAM in PVH boot mode.
>>> This is a PVH property according to your description and according
>>> to my understanding. Why would you then ...
>>>
>>>> Fix this by skipping the region type change for pv-shim mode.
>>> ... alter behavior for shim mode only, rather than when booted in
>>> PVH mode in general?
>> This is just me being cautious.
>>
>> My original email does have this text after ---
>> "The condition can be relaxed to be just !pvh_boot if there are no
>> plans to support VGA MMIO / ROMs for PVH guests."
> I doubt emulated ones are intended to be supported, but of
> course a graphics card could be passed through. Iirc passthrough
> is a pending work item still anyway for PVH, so dealing with the
> case right now may not even be necessary.

The bug is Xen blindly presuming that a missing hole "must be a firmware
bug".

I can believe that there may have been systems decades ago which got
this wrong, but tbh I doubt it applies to 64bit-capable systems,
considering how standardised things were by then.

I'd just delete this whole piece of logic and call it done.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for 4.13] x86/shim: don't reserve 640k - 1M region in E820

[Jan Beulich]

On 28.10.2019 12:15, Andrew Cooper wrote:
> On 28/10/2019 11:11, Jan Beulich wrote:
>> On 28.10.2019 12:08, Sergey Dyasli wrote:
>>> On 28/10/2019 09:06, Jan Beulich wrote:
>>>> On 28.10.2019 09:56, Sergey Dyasli wrote:
>>>>> Converting a guest from PV to PV-in-PVH makes the guest to have 384k
>>>>> less memory, which may confuse guest's balloon driver. This happens
>>>>> because Xen unconditionally reserves 640k - 1M region in E820 despite
>>>>> the fact that it's really a usable RAM in PVH boot mode.
>>>> This is a PVH property according to your description and according
>>>> to my understanding. Why would you then ...
>>>>
>>>>> Fix this by skipping the region type change for pv-shim mode.
>>>> ... alter behavior for shim mode only, rather than when booted in
>>>> PVH mode in general?
>>> This is just me being cautious.
>>>
>>> My original email does have this text after ---
>>> "The condition can be relaxed to be just !pvh_boot if there are no
>>> plans to support VGA MMIO / ROMs for PVH guests."
>> I doubt emulated ones are intended to be supported, but of
>> course a graphics card could be passed through. Iirc passthrough
>> is a pending work item still anyway for PVH, so dealing with the
>> case right now may not even be necessary.
> 
> The bug is Xen blindly presuming that a missing hole "must be a firmware
> bug".
> 
> I can believe that there may have been systems decades ago which got
> this wrong, but tbh I doubt it applies to 64bit-capable systems,
> considering how standardised things were by then.
> 
> I'd just delete this whole piece of logic and call it done.

Well, I could see use being less aggressive here, but firmware (if
there is firmware, i.e. everything except PVH) claiming there to
be RAM immediately below the 1M boundary is a pretty good sign of
something being wrong. There _ought_ to be ROM at that address.
Otoh there were even ways in older chipsets to make RAM appear at
address ranges not used by option ROMs, so the logic we currently
have goes too far potentially even on bare metal.

So while I'm all for relaxing, I don't think I can support
outright deletion.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for 4.13] x86/shim: don't reserve 640k - 1M region in E820

[Andrew Cooper]

On 28/10/2019 11:30, Jan Beulich wrote:
> On 28.10.2019 12:15, Andrew Cooper wrote:
>> On 28/10/2019 11:11, Jan Beulich wrote:
>>> On 28.10.2019 12:08, Sergey Dyasli wrote:
>>>> On 28/10/2019 09:06, Jan Beulich wrote:
>>>>> On 28.10.2019 09:56, Sergey Dyasli wrote:
>>>>>> Converting a guest from PV to PV-in-PVH makes the guest to have 384k
>>>>>> less memory, which may confuse guest's balloon driver. This happens
>>>>>> because Xen unconditionally reserves 640k - 1M region in E820 despite
>>>>>> the fact that it's really a usable RAM in PVH boot mode.
>>>>> This is a PVH property according to your description and according
>>>>> to my understanding. Why would you then ...
>>>>>
>>>>>> Fix this by skipping the region type change for pv-shim mode.
>>>>> ... alter behavior for shim mode only, rather than when booted in
>>>>> PVH mode in general?
>>>> This is just me being cautious.
>>>>
>>>> My original email does have this text after ---
>>>> "The condition can be relaxed to be just !pvh_boot if there are no
>>>> plans to support VGA MMIO / ROMs for PVH guests."
>>> I doubt emulated ones are intended to be supported, but of
>>> course a graphics card could be passed through. Iirc passthrough
>>> is a pending work item still anyway for PVH, so dealing with the
>>> case right now may not even be necessary.
>> The bug is Xen blindly presuming that a missing hole "must be a firmware
>> bug".
>>
>> I can believe that there may have been systems decades ago which got
>> this wrong, but tbh I doubt it applies to 64bit-capable systems,
>> considering how standardised things were by then.
>>
>> I'd just delete this whole piece of logic and call it done.
> Well, I could see use being less aggressive here, but firmware (if
> there is firmware, i.e. everything except PVH) claiming there to
> be RAM immediately below the 1M boundary is a pretty good sign of
> something being wrong. There _ought_ to be ROM at that address.
> Otoh there were even ways in older chipsets to make RAM appear at
> address ranges not used by option ROMs, so the logic we currently
> have goes too far potentially even on bare metal.
>
> So while I'm all for relaxing, I don't think I can support
> outright deletion.

For now, how about cpu_has_hypervisor ?

Whatever the virtual environment, we should trust the provided memory map.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for 4.13] x86/shim: don't reserve 640k - 1M region in E820

[Jan Beulich]

On 28.10.2019 12:33, Andrew Cooper wrote:
> On 28/10/2019 11:30, Jan Beulich wrote:
>> On 28.10.2019 12:15, Andrew Cooper wrote:
>>> On 28/10/2019 11:11, Jan Beulich wrote:
>>>> On 28.10.2019 12:08, Sergey Dyasli wrote:
>>>>> On 28/10/2019 09:06, Jan Beulich wrote:
>>>>>> On 28.10.2019 09:56, Sergey Dyasli wrote:
>>>>>>> Converting a guest from PV to PV-in-PVH makes the guest to have 384k
>>>>>>> less memory, which may confuse guest's balloon driver. This happens
>>>>>>> because Xen unconditionally reserves 640k - 1M region in E820 despite
>>>>>>> the fact that it's really a usable RAM in PVH boot mode.
>>>>>> This is a PVH property according to your description and according
>>>>>> to my understanding. Why would you then ...
>>>>>>
>>>>>>> Fix this by skipping the region type change for pv-shim mode.
>>>>>> ... alter behavior for shim mode only, rather than when booted in
>>>>>> PVH mode in general?
>>>>> This is just me being cautious.
>>>>>
>>>>> My original email does have this text after ---
>>>>> "The condition can be relaxed to be just !pvh_boot if there are no
>>>>> plans to support VGA MMIO / ROMs for PVH guests."
>>>> I doubt emulated ones are intended to be supported, but of
>>>> course a graphics card could be passed through. Iirc passthrough
>>>> is a pending work item still anyway for PVH, so dealing with the
>>>> case right now may not even be necessary.
>>> The bug is Xen blindly presuming that a missing hole "must be a firmware
>>> bug".
>>>
>>> I can believe that there may have been systems decades ago which got
>>> this wrong, but tbh I doubt it applies to 64bit-capable systems,
>>> considering how standardised things were by then.
>>>
>>> I'd just delete this whole piece of logic and call it done.
>> Well, I could see use being less aggressive here, but firmware (if
>> there is firmware, i.e. everything except PVH) claiming there to
>> be RAM immediately below the 1M boundary is a pretty good sign of
>> something being wrong. There _ought_ to be ROM at that address.
>> Otoh there were even ways in older chipsets to make RAM appear at
>> address ranges not used by option ROMs, so the logic we currently
>> have goes too far potentially even on bare metal.
>>
>> So while I'm all for relaxing, I don't think I can support
>> outright deletion.
> 
> For now, how about cpu_has_hypervisor ?
> 
> Whatever the virtual environment, we should trust the provided memory map.

Hmm, yes, this sounds reasonable.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for 4.13] x86/shim: don't reserve 640k - 1M region in E820

[Sergey Dyasli]

On 28/10/2019 11:33, Andrew Cooper wrote:
> On 28/10/2019 11:30, Jan Beulich wrote:
>> On 28.10.2019 12:15, Andrew Cooper wrote:
>>> On 28/10/2019 11:11, Jan Beulich wrote:
>>>> On 28.10.2019 12:08, Sergey Dyasli wrote:
>>>>> On 28/10/2019 09:06, Jan Beulich wrote:
>>>>>> On 28.10.2019 09:56, Sergey Dyasli wrote:
>>>>>>> Converting a guest from PV to PV-in-PVH makes the guest to have 384k
>>>>>>> less memory, which may confuse guest's balloon driver. This happens
>>>>>>> because Xen unconditionally reserves 640k - 1M region in E820 despite
>>>>>>> the fact that it's really a usable RAM in PVH boot mode.
>>>>>> This is a PVH property according to your description and according
>>>>>> to my understanding. Why would you then ...
>>>>>>
>>>>>>> Fix this by skipping the region type change for pv-shim mode.
>>>>>> ... alter behavior for shim mode only, rather than when booted in
>>>>>> PVH mode in general?
>>>>> This is just me being cautious.
>>>>>
>>>>> My original email does have this text after ---
>>>>> "The condition can be relaxed to be just !pvh_boot if there are no
>>>>> plans to support VGA MMIO / ROMs for PVH guests."
>>>> I doubt emulated ones are intended to be supported, but of
>>>> course a graphics card could be passed through. Iirc passthrough
>>>> is a pending work item still anyway for PVH, so dealing with the
>>>> case right now may not even be necessary.
>>> The bug is Xen blindly presuming that a missing hole "must be a firmware
>>> bug".
>>>
>>> I can believe that there may have been systems decades ago which got
>>> this wrong, but tbh I doubt it applies to 64bit-capable systems,
>>> considering how standardised things were by then.
>>>
>>> I'd just delete this whole piece of logic and call it done.
>> Well, I could see use being less aggressive here, but firmware (if
>> there is firmware, i.e. everything except PVH) claiming there to
>> be RAM immediately below the 1M boundary is a pretty good sign of
>> something being wrong. There _ought_ to be ROM at that address.
>> Otoh there were even ways in older chipsets to make RAM appear at
>> address ranges not used by option ROMs, so the logic we currently
>> have goes too far potentially even on bare metal.
>>
>> So while I'm all for relaxing, I don't think I can support
>> outright deletion.
> 
> For now, how about cpu_has_hypervisor ?
> 
> Whatever the virtual environment, we should trust the provided memory map.

Unfortunately, this plan has failed: init_e820() is called way before
early_cpu_init() and testing cpu_has_hypervisor is meaningless there.

I guess I'll go for !pvh_boot check for now.

--
Thanks,
Sergey

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for 4.13] x86/shim: don't reserve 640k - 1M region in E820

[Andrew Cooper]

On 29/10/2019 10:17, Sergey Dyasli wrote:
> On 28/10/2019 11:33, Andrew Cooper wrote:
>> On 28/10/2019 11:30, Jan Beulich wrote:
>>> On 28.10.2019 12:15, Andrew Cooper wrote:
>>>> On 28/10/2019 11:11, Jan Beulich wrote:
>>>>> On 28.10.2019 12:08, Sergey Dyasli wrote:
>>>>>> On 28/10/2019 09:06, Jan Beulich wrote:
>>>>>>> On 28.10.2019 09:56, Sergey Dyasli wrote:
>>>>>>>> Converting a guest from PV to PV-in-PVH makes the guest to have 384k
>>>>>>>> less memory, which may confuse guest's balloon driver. This happens
>>>>>>>> because Xen unconditionally reserves 640k - 1M region in E820 despite
>>>>>>>> the fact that it's really a usable RAM in PVH boot mode.
>>>>>>> This is a PVH property according to your description and according
>>>>>>> to my understanding. Why would you then ...
>>>>>>>
>>>>>>>> Fix this by skipping the region type change for pv-shim mode.
>>>>>>> ... alter behavior for shim mode only, rather than when booted in
>>>>>>> PVH mode in general?
>>>>>> This is just me being cautious.
>>>>>>
>>>>>> My original email does have this text after ---
>>>>>> "The condition can be relaxed to be just !pvh_boot if there are no
>>>>>> plans to support VGA MMIO / ROMs for PVH guests."
>>>>> I doubt emulated ones are intended to be supported, but of
>>>>> course a graphics card could be passed through. Iirc passthrough
>>>>> is a pending work item still anyway for PVH, so dealing with the
>>>>> case right now may not even be necessary.
>>>> The bug is Xen blindly presuming that a missing hole "must be a firmware
>>>> bug".
>>>>
>>>> I can believe that there may have been systems decades ago which got
>>>> this wrong, but tbh I doubt it applies to 64bit-capable systems,
>>>> considering how standardised things were by then.
>>>>
>>>> I'd just delete this whole piece of logic and call it done.
>>> Well, I could see use being less aggressive here, but firmware (if
>>> there is firmware, i.e. everything except PVH) claiming there to
>>> be RAM immediately below the 1M boundary is a pretty good sign of
>>> something being wrong. There _ought_ to be ROM at that address.
>>> Otoh there were even ways in older chipsets to make RAM appear at
>>> address ranges not used by option ROMs, so the logic we currently
>>> have goes too far potentially even on bare metal.
>>>
>>> So while I'm all for relaxing, I don't think I can support
>>> outright deletion.
>> For now, how about cpu_has_hypervisor ?
>>
>> Whatever the virtual environment, we should trust the provided memory map.
> Unfortunately, this plan has failed: init_e820() is called way before
> early_cpu_init() and testing cpu_has_hypervisor is meaningless there.
>
> I guess I'll go for !pvh_boot check for now.

Opencode it with cpuid_ecx(1) for now, as we've done elsewhere.

I've got a patch in my not-progressing-very-quickly series to remove the
boot time mapping at 0 which cleans up early CPUID handling so this can
turn into cpu_has_hypervisor, but that is definitely post-4.13 material
right now.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for 4.13] x86/shim: don't reserve 640k - 1M region in E820

[Sergey Dyasli]

On 29/10/2019 10:19, Andrew Cooper wrote:
> On 29/10/2019 10:17, Sergey Dyasli wrote:
>> On 28/10/2019 11:33, Andrew Cooper wrote:
>>> For now, how about cpu_has_hypervisor ?
>>>
>>> Whatever the virtual environment, we should trust the provided memory map.
>> Unfortunately, this plan has failed: init_e820() is called way before
>> early_cpu_init() and testing cpu_has_hypervisor is meaningless there.
>>
>> I guess I'll go for !pvh_boot check for now.
> 
> Opencode it with cpuid_ecx(1) for now, as we've done elsewhere.
> 
> I've got a patch in my not-progressing-very-quickly series to remove the
> boot time mapping at 0 which cleans up early CPUID handling so this can
> turn into cpu_has_hypervisor, but that is definitely post-4.13 material
> right now.

Looking closely at early_cpu_init(), I don't see anything there that
would prevent placing it near the top of __start_xen(). It works for
pv-shim, but I need to also do some bare metal testing with this change.

Do you know of anything that would prevent it?

--
Thanks,
Sergey

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for 4.13] x86/shim: don't reserve 640k - 1M region in E820

[Andrew Cooper]

On 29/10/2019 10:40, Sergey Dyasli wrote:
> On 29/10/2019 10:19, Andrew Cooper wrote:
>> On 29/10/2019 10:17, Sergey Dyasli wrote:
>>> On 28/10/2019 11:33, Andrew Cooper wrote:
>>>> For now, how about cpu_has_hypervisor ?
>>>>
>>>> Whatever the virtual environment, we should trust the provided memory map.
>>> Unfortunately, this plan has failed: init_e820() is called way before
>>> early_cpu_init() and testing cpu_has_hypervisor is meaningless there.
>>>
>>> I guess I'll go for !pvh_boot check for now.
>> Opencode it with cpuid_ecx(1) for now, as we've done elsewhere.
>>
>> I've got a patch in my not-progressing-very-quickly series to remove the
>> boot time mapping at 0 which cleans up early CPUID handling so this can
>> turn into cpu_has_hypervisor, but that is definitely post-4.13 material
>> right now.
> Looking closely at early_cpu_init(), I don't see anything there that
> would prevent placing it near the top of __start_xen(). It works for
> pv-shim, but I need to also do some bare metal testing with this change.
>
> Do you know of anything that would prevent it?

Yes - microcode loading.  That sequence of things in __start_xen() is
very position sensitive.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel