Re: [Xen-devel] [PATCH V4 4/4] x86/mm: Make use of the default access param from xc_altp2m_create_view

From: Alexandru Stefan ISAILA <aisaila@bitdefender.com>
To: Jan Beulich <jbeulich@suse.com>, Tamas K Lengyel <tamas@tklengyel.com>
Cc: "Petre Ovidiu PIRCALABU" <ppircalabu@bitdefender.com>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Julien Grall" <julien@xen.org>,
	"Razvan COJOCARU" <rcojocaru@bitdefender.com>,
	"Wei Liu" <wl@xen.org>,
	"Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>,
	"George Dunlap" <George.Dunlap@eu.citrix.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Ian Jackson" <ian.jackson@eu.citrix.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH V4 4/4] x86/mm: Make use of the default access param from xc_altp2m_create_view
Date: Wed, 18 Dec 2019 11:53:12 +0000	[thread overview]
Message-ID: <9a3244a5-36c4-07e3-d79c-4a2f8beb2d54@bitdefender.com> (raw)
In-Reply-To: <81907d55-2e36-96f7-95cf-990f062a273c@suse.com>

On 18.12.2019 12:45, Jan Beulich wrote:
> On 17.12.2019 16:12, Alexandru Stefan ISAILA wrote:
>> --- a/xen/arch/x86/mm/mem_access.c
>> +++ b/xen/arch/x86/mm/mem_access.c
>> @@ -314,9 +314,9 @@ static int set_mem_access(struct domain *d, struct p2m_domain *p2m,
>>       return rc;
>>   }
>>   
>> -static bool xenmem_access_to_p2m_access(struct p2m_domain *p2m,
>> -                                        xenmem_access_t xaccess,
>> -                                        p2m_access_t *paccess)
>> +bool xenmem_access_to_p2m_access(struct p2m_domain *p2m,
>> +                                 xenmem_access_t xaccess,
>> +                                 p2m_access_t *paccess)
> 
> Would you mind taking the opportunity and add const to the first
> parameter?

Sure, given that there will be a new version, it will add it.

> 
>> @@ -2601,7 +2610,15 @@ int p2m_init_next_altp2m(struct domain *d, uint16_t *idx)
>>           if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
>>               continue;
>>   
>> -        rc = p2m_activate_altp2m(d, i);
>> +        p2m = d->arch.altp2m_p2m[i];
>> +
>> +        if ( !xenmem_access_to_p2m_access(p2m, hvmmem_default_access, &a) )
>> +        {
>> +            altp2m_list_unlock(d);
>> +            return -EINVAL;
>> +        }
> 
> Can this be pulled out of the locked region, ahead of the loop?
> The p2m getting passed in here (which is why it's in the loop)
> shouldn't have been in use yet, i.e. its ->default_access should
> have a known value. Hence this case could be taken care of
> independently, e.g. by adjusting xenmem_access_to_p2m_access()
> to cope with a NULL p2m coming in (producing whatever the default
> of the default is).
> 

Yes this sounds good. In xenmem_access_to_p2m_access() there can be a 
check like:

if ( !p2m )
     *paccess = p2m_access_rwx;
else
     *paccess = p2m->default_access;

But before I change this maybe Tamas or George have something to add?
And can this stay in the same patch or should it have a prereq one?

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