Re: [Xen-devel] [PATCH v4 1/2] xen: Switch parameter in get_page_from_gfn to use typesafe gfn

[Julien Grall <Julien.Grall@xxxxxxx>]

Hi,

On 29/08/2019 17:41, Jan Beulich wrote:
> On 19.08.2019 16:26, Julien Grall wrote:
>> No functional change intended.
>>
>> Only reasonable clean-ups are done in this patch. The rest will use _gfn
>> for the time being.
>>
>> The code in get_page_from_gfn is slightly reworked to also handle a bad
>> behavior because it is not safe to use mfn_to_page(...) because
>> mfn_valid(...) succeeds.
> 
> I guess the 2nd "because" is meant to be "before", but even then I
> don't think I can easily agree: mfn_to_page() is just calculations,
> no dereference.

Regardless the s/because/before/. Do you disagree on the wording or the 
change? If the former, I just paraphrased what Andrew wrote in the 
previous version:

"This unfortunately propagates some bad behaviour, because it is not 
safe to use mfn_to_page(mfn); before mfn_valid(mfn) succeeds.  (In 
practice it works because mfn_to_page() is just pointer arithmetic.)"

This is x86 code, so please agree with Andrew on the approach/wording.

> 
>> --- a/xen/arch/x86/hvm/domain.c
>> +++ b/xen/arch/x86/hvm/domain.c
>> @@ -296,8 +296,10 @@ int arch_set_info_hvm_guest(struct vcpu *v, const 
>> vcpu_hvm_context_t *ctx)
>>       if ( hvm_paging_enabled(v) && !paging_mode_hap(v->domain) )
>>       {
>>           /* Shadow-mode CR3 change. Check PDBR and update refcounts. */
>> -        struct page_info *page = get_page_from_gfn(v->domain,
>> -                                 v->arch.hvm.guest_cr[3] >> PAGE_SHIFT,
>> +        struct page_info *page;
>> +
>> +        page = get_page_from_gfn(v->domain,
>> +                                 gaddr_to_gfn(v->arch.hvm.guest_cr[3]),
>>                                    NULL, P2M_ALLOC);
> 
> Iirc I've said so before: I consider use of gaddr_to_gfn() here more
> misleading than a plain shift by PAGE_SHIFT. Neither is really correct,
> but in no event does CR3 as a whole hold an address. (Same elsewhere
> then, and sadly in quite a few places.)

Well, this code has not changed since v3 and you acked it... I only 
dropped the ack here because the previous version was sent 9 months ago. 
I also can't see such comment made on any version of this series.

Anyway, I am more than happy to switch to _gfn((v->arch.hvm.guest_cr[3] 
 >> PAGE_SHIFT)) if you prefer it.

> 
>> --- a/xen/common/event_fifo.c
>> +++ b/xen/common/event_fifo.c
>> @@ -361,7 +361,7 @@ static const struct evtchn_port_ops evtchn_port_ops_fifo 
>> =
>>       .print_state   = evtchn_fifo_print_state,
>>   };
>>   
>> -static int map_guest_page(struct domain *d, uint64_t gfn, void **virt)
>> +static int map_guest_page(struct domain *d, gfn_t gfn, void **virt)
>>   {
>>       struct page_info *p;
>>   
>> @@ -422,7 +422,7 @@ static int setup_control_block(struct vcpu *v)
>>       return 0;
>>   }
>>   
>> -static int map_control_block(struct vcpu *v, uint64_t gfn, uint32_t offset)
>> +static int map_control_block(struct vcpu *v, gfn_t gfn, uint32_t offset)
>>   {
>>       void *virt;
>>       unsigned int i;
> 
> Just as a remark (no action expected) - this makes a truncation issue
> pretty apparent: On 32-bit platforms the upper 32 bits of a passed in
> GFN get ignored. Correct (imo) behavior would be to reject the upper
> bits being non-zero.

This is not only here but on pretty much all the hypercalls taking a gfn 
(on Arm it is 64-bit regardless the bitness).

I agree this is not nice, but I am afraid this is likely another can of 
worm that I am not to open it yet.

Cheers,

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