x86: drop a bogus SHARED_M2P() check from Dom0 building code

[PATCH] x86: drop a bogus SHARED_M2P() check from Dom0 building code

Posted by Jan Beulich 2 years, 10 months ago

If anything, a check covering a wider range of invalid M2P entries ought
to be used (e.g. VALID_M2P()). But since everything is fully under Xen's
control at this stage, simply remove the BUG_ON().

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -815,7 +815,6 @@ int __init dom0_construct_pv(struct doma
     page_list_for_each ( page, &d->page_list )
     {
         mfn = mfn_x(page_to_mfn(page));
-        BUG_ON(SHARED_M2P(get_gpfn_from_mfn(mfn)));
         if ( get_gpfn_from_mfn(mfn) >= count )
         {
             BUG_ON(compat);

Ping: [PATCH] x86: drop a bogus SHARED_M2P() check from Dom0 building code

Posted by Jan Beulich 2 years, 9 months ago

On 28.06.2021 13:52, Jan Beulich wrote:
> If anything, a check covering a wider range of invalid M2P entries ought
> to be used (e.g. VALID_M2P()). But since everything is fully under Xen's
> control at this stage, simply remove the BUG_ON().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I didn't expect this to be controversial, so may I please ask for an ack
(or otherwise)?

Thanks, Jan

> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -815,7 +815,6 @@ int __init dom0_construct_pv(struct doma
>      page_list_for_each ( page, &d->page_list )
>      {
>          mfn = mfn_x(page_to_mfn(page));
> -        BUG_ON(SHARED_M2P(get_gpfn_from_mfn(mfn)));
>          if ( get_gpfn_from_mfn(mfn) >= count )
>          {
>              BUG_ON(compat);
>

Ping²: [PATCH] x86: drop a bogus SHARED_M2P() check from Dom0 building code

Posted by Jan Beulich 2 years, 7 months ago

On 06.07.2021 09:37, Jan Beulich wrote:
> On 28.06.2021 13:52, Jan Beulich wrote:
>> If anything, a check covering a wider range of invalid M2P entries ought
>> to be used (e.g. VALID_M2P()). But since everything is fully under Xen's
>> control at this stage, simply remove the BUG_ON().
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> I didn't expect this to be controversial, so may I please ask for an ack
> (or otherwise)?

To be quite honest, I find it very strange that even simple changes like
this one sit un-responded to for months. This isn't the only example ...

Thanks, Jan

>> --- a/xen/arch/x86/pv/dom0_build.c
>> +++ b/xen/arch/x86/pv/dom0_build.c
>> @@ -815,7 +815,6 @@ int __init dom0_construct_pv(struct doma
>>      page_list_for_each ( page, &d->page_list )
>>      {
>>          mfn = mfn_x(page_to_mfn(page));
>> -        BUG_ON(SHARED_M2P(get_gpfn_from_mfn(mfn)));
>>          if ( get_gpfn_from_mfn(mfn) >= count )
>>          {
>>              BUG_ON(compat);
>>
>

Re: [PATCH] x86: drop a bogus SHARED_M2P() check from Dom0 building code

Posted by Roger Pau Monné 2 years, 7 months ago

Could you explicitly mention PV somewhere in the subject?

On Mon, Jun 28, 2021 at 01:52:52PM +0200, Jan Beulich wrote:
> If anything, a check covering a wider range of invalid M2P entries ought
> to be used (e.g. VALID_M2P()). But since everything is fully under Xen's
> control at this stage, simply remove the BUG_ON().

AFAICT adding this BUG_ON was a wholesale change to protect against
shared frames, but I also cannot ssee an scenario where we can get
here with shared frames in the m2p.

> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.