xen/x86: p2m: Don't initialize slot 0 of the P2M
diff mbox series

Message ID 20200203165812.21089-1-julien@xen.org
State New
Headers show
Series
  • xen/x86: p2m: Don't initialize slot 0 of the P2M
Related show

Commit Message

Julien Grall Feb. 3, 2020, 4:58 p.m. UTC
From: Julien Grall <jgrall@amazon.com>

It is not entirely clear why the slot 0 of each p2m should be populated
with empty page-tables. The commit introducing it 759af8e3800 "[HVM]
Fix 64-bit HVM domain creation." does not contain meaningful
explanation except that it was necessary for shadow.

As we don't seem to have a good explanation why this is there, drop the
code completely.

This was tested by successfully booting a HVM with shadow enabled.

Signed-off-by: Julien Grall <jgrall@amazon.com>

---

I don't know for sure if this is going to break a setup. I have tried
HVM guest with hap={0, 1} without any trouble. I am happy to try more
setup if you have any in mind.

If this break a setup, then please describe the setup and I will send a
documentation patch instead.
---
 xen/arch/x86/mm/p2m.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

Comments

George Dunlap Feb. 3, 2020, 5:10 p.m. UTC | #1
On 2/3/20 4:58 PM, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> It is not entirely clear why the slot 0 of each p2m should be populated
> with empty page-tables. The commit introducing it 759af8e3800 "[HVM]
> Fix 64-bit HVM domain creation." does not contain meaningful
> explanation except that it was necessary for shadow.

Tim, any ideas here?

> As we don't seem to have a good explanation why this is there, drop the
> code completely.
> 
> This was tested by successfully booting a HVM with shadow enabled.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> ---
> 
> I don't know for sure if this is going to break a setup. I have tried
> HVM guest with hap={0, 1} without any trouble. I am happy to try more
> setup if you have any in mind.
> 
> If this break a setup, then please describe the setup and I will send a
> documentation patch instead.

This is a somewhat risky strategy.  Other than code clean-up, is there
any advantage to removing this code at the moment?

 -George
Julien Grall Feb. 3, 2020, 5:22 p.m. UTC | #2
Hi,

On 03/02/2020 17:10, George Dunlap wrote:
> On 2/3/20 4:58 PM, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> It is not entirely clear why the slot 0 of each p2m should be populated
>> with empty page-tables. The commit introducing it 759af8e3800 "[HVM]
>> Fix 64-bit HVM domain creation." does not contain meaningful
>> explanation except that it was necessary for shadow.
> 
> Tim, any ideas here?
> 
>> As we don't seem to have a good explanation why this is there, drop the
>> code completely.
>>
>> This was tested by successfully booting a HVM with shadow enabled.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>
>> ---
>>
>> I don't know for sure if this is going to break a setup. I have tried
>> HVM guest with hap={0, 1} without any trouble. I am happy to try more
>> setup if you have any in mind.
>>
>> If this break a setup, then please describe the setup and I will send a
>> documentation patch instead.
> 
> This is a somewhat risky strategy.  Other than code clean-up, is there
> any advantage to removing this code at the moment?

If Tim doesn't have an explanation, then we have two solutions:
    1) Checkin the code and see if that breaks
    2) Keep code we have no clue why it is there

I understand that the former is risky, but the latter is not very ideal 
either because if we can't explain the reason now, then it is unlikely 
that we would in the future.

Regarding the advantage of removing it, I am looking at liveupdate and 
how to keep the P2M around. I am trying to limit the number of "if 
(liveupdate)" within the code. So any cleanup would be beneficial.

Cheers,
George Dunlap Feb. 3, 2020, 5:37 p.m. UTC | #3
On 2/3/20 5:22 PM, Julien Grall wrote:
> Hi,
> 
> On 03/02/2020 17:10, George Dunlap wrote:
>> On 2/3/20 4:58 PM, Julien Grall wrote:
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>> It is not entirely clear why the slot 0 of each p2m should be populated
>>> with empty page-tables. The commit introducing it 759af8e3800 "[HVM]
>>> Fix 64-bit HVM domain creation." does not contain meaningful
>>> explanation except that it was necessary for shadow.
>>
>> Tim, any ideas here?
>>
>>> As we don't seem to have a good explanation why this is there, drop the
>>> code completely.
>>>
>>> This was tested by successfully booting a HVM with shadow enabled.
>>>
>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>>
>>> ---
>>>
>>> I don't know for sure if this is going to break a setup. I have tried
>>> HVM guest with hap={0, 1} without any trouble. I am happy to try more
>>> setup if you have any in mind.
>>>
>>> If this break a setup, then please describe the setup and I will send a
>>> documentation patch instead.
>>
>> This is a somewhat risky strategy.  Other than code clean-up, is there
>> any advantage to removing this code at the moment?
> 
> If Tim doesn't have an explanation, then we have two solutions:
>    1) Checkin the code and see if that breaks
>    2) Keep code we have no clue why it is there

It is probably early enough in the dev cycle to do this.

Also, it's not clear to me what kind of bug the code you're deleting
would fix.  If you read a not-present entry, you should get INVALID_MFN
anyway.  Unless you were calling p2m_get_entry_query(), which I'm pretty
sure hadn't been introduced at this point.

> I understand that the former is risky, but the latter is not very ideal
> either because if we can't explain the reason now, then it is unlikely
> that we would in the future.
> 
> Regarding the advantage of removing it, I am looking at liveupdate and
> how to keep the P2M around. I am trying to limit the number of "if
> (liveupdate)" within the code. So any cleanup would be beneficial.

OK, thanks.

 -George
Julien Grall Feb. 3, 2020, 6:31 p.m. UTC | #4
On 03/02/2020 17:37, George Dunlap wrote:
> On 2/3/20 5:22 PM, Julien Grall wrote:
>> Hi,
>>
>> On 03/02/2020 17:10, George Dunlap wrote:
>>> On 2/3/20 4:58 PM, Julien Grall wrote:
>>>> From: Julien Grall <jgrall@amazon.com>
>>>>
>>>> It is not entirely clear why the slot 0 of each p2m should be populated
>>>> with empty page-tables. The commit introducing it 759af8e3800 "[HVM]
>>>> Fix 64-bit HVM domain creation." does not contain meaningful
>>>> explanation except that it was necessary for shadow.
>>>
>>> Tim, any ideas here?
>>>
>>>> As we don't seem to have a good explanation why this is there, drop the
>>>> code completely.
>>>>
>>>> This was tested by successfully booting a HVM with shadow enabled.
>>>>
>>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>>>
>>>> ---
>>>>
>>>> I don't know for sure if this is going to break a setup. I have tried
>>>> HVM guest with hap={0, 1} without any trouble. I am happy to try more
>>>> setup if you have any in mind.
>>>>
>>>> If this break a setup, then please describe the setup and I will send a
>>>> documentation patch instead.
>>>
>>> This is a somewhat risky strategy.  Other than code clean-up, is there
>>> any advantage to removing this code at the moment?
>>
>> If Tim doesn't have an explanation, then we have two solutions:
>>     1) Checkin the code and see if that breaks
>>     2) Keep code we have no clue why it is there
> 
> It is probably early enough in the dev cycle to do this.
 >
> Also, it's not clear to me what kind of bug the code you're deleting
> would fix.  If you read a not-present entry, you should get INVALID_MFN
> anyway.  Unless you were calling p2m_get_entry_query(), which I'm pretty
> sure hadn't been introduced at this point.

I can't find this function you mention in staging. Was it removed recently?

The code is allocating all page-tables for _gfn(0). I would not expect 
the common code to care whether a table is allocated or not. So this 
would suggest that an internal implementation (of the shadow?) is 
relying on this.

However, I can't find anything obvious suggesting that is necessary. If 
there was anything, I would expect to happen during domain creation, as 
neither Xen nor a guest could rely on this (there are way to make those 
pages disappear with the MEMORY op hypercall).

Cheers,
Tim Deegan Feb. 6, 2020, 7:48 a.m. UTC | #5
At 18:31 +0000 on 03 Feb (1580754711), Julien Grall wrote:
> On 03/02/2020 17:37, George Dunlap wrote:
> > On 2/3/20 5:22 PM, Julien Grall wrote:
> >> On 03/02/2020 17:10, George Dunlap wrote:
> >>> On 2/3/20 4:58 PM, Julien Grall wrote:
> >>>> From: Julien Grall <jgrall@amazon.com>
> >>>>
> >>>> It is not entirely clear why the slot 0 of each p2m should be populated
> >>>> with empty page-tables. The commit introducing it 759af8e3800 "[HVM]
> >>>> Fix 64-bit HVM domain creation." does not contain meaningful
> >>>> explanation except that it was necessary for shadow.
> >>>
> >>> Tim, any ideas here?

Afraid not, sorry.  I can't think what would rely on the tables being
allocated for slot 0 in particular.  Maybe there's something later
that adds other entries in the bottom 2MB and can't handle a table
allocation failure?

> > Also, it's not clear to me what kind of bug the code you're deleting
> > would fix.  If you read a not-present entry, you should get INVALID_MFN
> > anyway.  Unless you were calling p2m_get_entry_query(), which I'm pretty
> > sure hadn't been introduced at this point.
> 
> I can't find this function you mention in staging. Was it removed recently?
> 
> The code is allocating all page-tables for _gfn(0). I would not expect 
> the common code to care whether a table is allocated or not. So this 
> would suggest that an internal implementation (of the shadow?) is 
> relying on this.
> 
> However, I can't find anything obvious suggesting that is necessary. If 
> there was anything, I would expect to happen during domain creation, as 
> neither Xen nor a guest could rely on this (there are way to make those 
> pages disappear with the MEMORY op hypercall).

That may not have been true at the time (and so whatever it was that
neede this may have been fixed when it became true?)

Cheers,

Tim.
George Dunlap Feb. 6, 2020, 12:08 p.m. UTC | #6
On 2/3/20 4:58 PM, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> It is not entirely clear why the slot 0 of each p2m should be populated
> with empty page-tables. The commit introducing it 759af8e3800 "[HVM]
> Fix 64-bit HVM domain creation." does not contain meaningful
> explanation except that it was necessary for shadow.
> 
> As we don't seem to have a good explanation why this is there, drop the
> code completely.
> 
> This was tested by successfully booting a HVM with shadow enabled.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Since nobody knows why it's here, and it doesn't look like it should
have any effect:

Acked-by: George Dunlap <george.dunlap@citrix.com>
Julien Grall Feb. 17, 2020, 10:04 p.m. UTC | #7
Hi George,

On 06/02/2020 12:08, George Dunlap wrote:
> On 2/3/20 4:58 PM, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> It is not entirely clear why the slot 0 of each p2m should be populated
>> with empty page-tables. The commit introducing it 759af8e3800 "[HVM]
>> Fix 64-bit HVM domain creation." does not contain meaningful
>> explanation except that it was necessary for shadow.
>>
>> As we don't seem to have a good explanation why this is there, drop the
>> code completely.
>>
>> This was tested by successfully booting a HVM with shadow enabled.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> Since nobody knows why it's here, and it doesn't look like it should
> have any effect:
> 
> Acked-by: George Dunlap <george.dunlap@citrix.com>

Thank you! I have now committed the patch.

Cheers,

Patch
diff mbox series

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 49cc138362..961bed0635 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -688,7 +688,6 @@  int p2m_alloc_table(struct p2m_domain *p2m)
 {
     mfn_t top_mfn;
     struct domain *d = p2m->domain;
-    int rc = 0;
 
     p2m_lock(p2m);
 
@@ -721,19 +720,8 @@  int p2m_alloc_table(struct p2m_domain *p2m)
     if ( hap_enabled(d) )
         iommu_share_p2m_table(d);
 
-    P2M_PRINTK("populating p2m table\n");
-
-    /* Initialise physmap tables for slot zero. Other code assumes this. */
-    p2m->defer_nested_flush = 1;
-    rc = p2m_set_entry(p2m, _gfn(0), INVALID_MFN, PAGE_ORDER_4K,
-                       p2m_invalid, p2m->default_access);
-    p2m->defer_nested_flush = 0;
     p2m_unlock(p2m);
-    if ( !rc )
-        P2M_PRINTK("p2m table initialised for slot zero\n");
-    else
-        P2M_PRINTK("failed to initialise p2m table for slot zero (%d)\n", rc);
-    return rc;
+    return 0;
 }
 
 /*