[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Julien Grall <Julien.Grall@xxxxxxx>
  • Date: Thu, 29 Aug 2019 17:36:31 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=Do3N60GmlI3jwmi2sy2VksTcup1NiCKsoCiNx2UQGPU=; b=eM8voCWrIJGPvcCI88mGDSns5PO2LIcnWolc3LWyJ3obpE1s2XSTMInYGEuSwurg6WttmTbDQtNCYqJrYWd5lJtdBKJ/8W3atBjbhvWNMmwRrMROS89Xl5VocoetI95lyCUmyEc2H2ZpqMyMdrPJwJv1DaUJVX4iaIMuv8JdaNJJy2g297fZJjx+Xauyjbr/oLOBlOlKFmBJydYU1Fdita+QWIDesINMO4hvpLrk45APqwwUC/pLzd7W9hFZ1A++E1IGs1XZaeOulviAdqY4cJdEw1x30/FGl5jkTnyHtQZhT8yZTG0a+BJ+dpF2rcLg0gJFXe23rZQSpAfFRpUXzw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=d+OYmmqJDmG38Kpl6ZvPhiBNSQdTGBh2R+bFFFs8jTeaJEGOcBH5MW7YUVrMJboGKLKq9AY+p+jV3Bu3sjmP/x2RxQpoPqy5sI+cVYzvsRXEb2vxgxE9Zdvlje8jT0XIrycOIxxIelmjmfryZktLfgdayqhEXx6gW3UBeko6JfxLFRSqITHnmAnJ3rOtwZPpETkCMbLg6W673UA4m520ghDY+YdamHjUP9Jd4F8zU96iOkrqMXbkcHdphvxMNlESOyR5bD1to3epDjrO8vXCgqrR70FCYN5Bxyn1wqTcYndIqAQ1gC/qz6WsnS0l2zUm6mCxNsPDWgWU30JM2aKmmA==
  • Authentication-results: spf=temperror (sender IP is smtp.mailfrom=arm.com; lists.xenproject.org; dkim=pass (signature was verified) header.d=armh.onmicrosoft.com; lists.xenproject.org; dmarc=temperror action=none header.from=arm.com;
  • Authentication-results-original: spf=none (sender IP is ) smtp.mailfrom=Julien.Grall@xxxxxxx;
  • Cc: Kevin Tian <kevin.tian@xxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Jun Nakajima <jun.nakajima@xxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>, George Dunlap <George.Dunlap@xxxxxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Ian Jackson <ian.jackson@xxxxxxxxxxxxx>, Tim Deegan <tim@xxxxxxx>, Paul Durrant <paul.durrant@xxxxxxxxxx>, "suravee.suthikulpanit@xxxxxxx" <suravee.suthikulpanit@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>, nd <nd@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Delivery-date: Thu, 29 Aug 2019 17:37:05 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: True
  • Original-authentication-results: spf=none (sender IP is ) smtp.mailfrom=Julien.Grall@xxxxxxx;
  • Thread-index: AQHVVpozFci0Qdq7TU6cBizMDmPrrKcSUx4AgAAgPgA=
  • Thread-topic: [PATCH v4 1/2] xen: Switch parameter in get_page_from_gfn to use typesafe gfn


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.


Julien Grall
Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.