From: Oleksandr <olekstysh@gmail.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Oleksandr Tyshchenko" <oleksandr_tyshchenko@epam.com>,
"Stefano Stabellini" <sstabellini@kernel.org>,
"Julien Grall" <julien@xen.org>,
"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
"Andrew Cooper" <andrew.cooper3@citrix.com>,
"George Dunlap" <george.dunlap@citrix.com>,
"Ian Jackson" <iwj@xenproject.org>, "Wei Liu" <wl@xen.org>,
"Roger Pau Monné" <roger.pau@citrix.com>,
xen-devel@lists.xenproject.org
Subject: Re: [RFC PATCH V2] xen/gnttab: Store frame GFN in struct page_info on Arm
Date: Thu, 16 Sep 2021 01:13:48 +0300 [thread overview]
Message-ID: <86cdc577-7085-48cd-c417-85b20afc9bf1@gmail.com> (raw)
In-Reply-To: <b6744333-4d43-ef24-0f9b-b5cd54680660@suse.com>
On 15.09.21 13:06, Jan Beulich wrote:
Hi Jan
> On 14.09.2021 22:44, Oleksandr Tyshchenko wrote:
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -1376,14 +1376,18 @@ unsigned long domain_get_maximum_gpfn(struct domain *d)
>> void share_xen_page_with_guest(struct page_info *page, struct domain *d,
>> enum XENSHARE_flags flags)
>> {
>> + unsigned long type_info;
>> +
>> if ( page_get_owner(page) == d )
>> return;
>>
>> spin_lock(&d->page_alloc_lock);
>>
>> /* The incremented type count pins as writable or read-only. */
>> - page->u.inuse.type_info =
>> - (flags == SHARE_ro ? PGT_none : PGT_writable_page) | 1;
>> + type_info = page->u.inuse.type_info & ~(PGT_type_mask | PGT_count_mask);
>> + page->u.inuse.type_info = type_info |
>> + (flags == SHARE_ro ? PGT_none : PGT_writable_page) |
>> + (1UL << PGT_count_base);
> Just as a note: If this was x86 code, I'd request the redundant
> PGT_count_base to be dropped. Constructs like the above is what we
> have MASK_INSR() for.
I got it, I will look at it.
>
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -2204,7 +2204,10 @@ void *alloc_xenheap_pages(unsigned int order, unsigned int memflags)
>> return NULL;
>>
>> for ( i = 0; i < (1u << order); i++ )
>> + {
>> pg[i].count_info |= PGC_xen_heap;
>> + arch_alloc_xenheap_page(&pg[i]);
>> + }
>>
>> return page_to_virt(pg);
>> }
>> @@ -2222,7 +2225,10 @@ void free_xenheap_pages(void *v, unsigned int order)
>> pg = virt_to_page(v);
>>
>> for ( i = 0; i < (1u << order); i++ )
>> + {
>> pg[i].count_info &= ~PGC_xen_heap;
>> + arch_free_xenheap_page(&pg[i]);
>> + }
>>
>> free_heap_pages(pg, order, true);
>> }
> You look to only be adjusting the !SEPARATE_XENHEAP instances of the
> functions. Isn't 32-bit Arm using SEPARATE_XENHEAP?
Hmm, looks like yes, thank you. At least config.h defines that on Arm32.
I will update the instances.
>
>> --- a/xen/include/asm-arm/mm.h
>> +++ b/xen/include/asm-arm/mm.h
>> @@ -98,9 +98,18 @@ struct page_info
>> #define PGT_writable_page PG_mask(1, 1) /* has writable mappings? */
>> #define PGT_type_mask PG_mask(1, 1) /* Bits 31 or 63. */
>>
>> - /* Count of uses of this frame as its current type. */
>> -#define PGT_count_width PG_shift(2)
>> -#define PGT_count_mask ((1UL<<PGT_count_width)-1)
>> + /* 3-bit count of uses of this frame as its current type. */
>> +#define PGT_count_base PG_shift(4)
>> +#define PGT_count_mask PG_mask(7, 4)
>> +
>> +/*
>> + * Stored in bits [27:0] or [59:0] GFN if page is used for grant table frame.
> I don't know enough Arm details to tell whether this is properly
> one bit more than the maximum number of physical address bits.
> Without the extra bit you wouldn't be able to tell apart a
> guest specified GFN matching the value of PGT_INVALID_FRAME_GFN
> from an entry which was set from INVALID_GFN.
Really good point.
1. On Arm64 the p2m_ipa_bits could (theoretically) be 64-bit which, I
assume, corresponds to the maximum guest physical address (1 << 64) - 1
= 0xFFFFFFFFFFFFFFFF.
To store that GFN we need 52-bit. But, we provide 60-bit field which is
more than enough, I think. Practically, the maximum supported
p2m_ipa_bits is 48-bit, so the maximum supported GFN will occupy 36-bit
only. Everything is ok here.
2. On Arm32 the p2m_ipa_bits is 40-bit which, I assume, corresponds to
the maximum guest physical address (1 << 40) - 1 = 0xFFFFFFFFFF. To
store that GFN we need 28-bit. If I did the calculation correctly, what
we have on Arm32 is that PGT_INVALID_FRAME_GFN == maximum guest physical
address and it looks like we need and extra bit on Arm32. Do you think
we need to borrow one more bit from the count portion to stay on the
safe side?
>
>> + * This only valid for the xenheap pages.
>> + */
>> +#define PGT_gfn_width PG_shift(4)
>> +#define PGT_gfn_mask ((1UL<<PGT_gfn_width)-1)
> Any reason you don't use PG_mask() here? Any open-coding is prone
> to people later making mistakes.
I failed to come up with idea how to do that without #ifdef. As GFN
starts at bit 0 different first parameter would be needed for PG_mask on
32-bit and 64-bit systems.
I wonder whether PGC_count_mask/PGT_count_mask are open-coded on Arm/x86
because of the same reason.
--
Regards,
Oleksandr Tyshchenko
next prev parent reply other threads:[~2021-09-15 22:14 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-14 20:44 [RFC PATCH V2] xen/gnttab: Store frame GFN in struct page_info on Arm Oleksandr Tyshchenko
2021-09-15 10:06 ` Jan Beulich
2021-09-15 22:13 ` Oleksandr [this message]
2021-09-16 6:38 ` Jan Beulich
2021-09-16 13:06 ` Oleksandr
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=86cdc577-7085-48cd-c417-85b20afc9bf1@gmail.com \
--to=olekstysh@gmail.com \
--cc=Volodymyr_Babchuk@epam.com \
--cc=andrew.cooper3@citrix.com \
--cc=george.dunlap@citrix.com \
--cc=iwj@xenproject.org \
--cc=jbeulich@suse.com \
--cc=julien@xen.org \
--cc=oleksandr_tyshchenko@epam.com \
--cc=roger.pau@citrix.com \
--cc=sstabellini@kernel.org \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.