Re: [PATCH V5] xen/gnttab: Store frame GFN in struct page_info on Arm

[Oleksandr Tyshchenko <Oleksandr_Tyshchenko@xxxxxxxx>]

On 07.02.22 19:15, Julien Grall wrote:
> Hi Oleksandr,


Hi Julien


>
>
> On 05/01/2022 23:11, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>>
>> Rework Arm implementation to store grant table frame GFN
>> in struct page_info directly instead of keeping it in
>> standalone status/shared arrays. This patch is based on
>> the assumption that grant table page is the xenheap page.
>
> I would write "grant table pages are xenheap pages" or "a grant table 
> page is a xenheap page".


ok, will do


>
> [...]
>
>> diff --git a/xen/arch/arm/include/asm/grant_table.h 
>> b/xen/arch/arm/include/asm/grant_table.h
>> index d31a4d6..d6fda31 100644
>> --- a/xen/arch/arm/include/asm/grant_table.h
>> +++ b/xen/arch/arm/include/asm/grant_table.h
>> @@ -11,11 +11,6 @@
>>   #define INITIAL_NR_GRANT_FRAMES 1U
>>   #define GNTTAB_MAX_VERSION 1
>>   -struct grant_table_arch {
>> -    gfn_t *shared_gfn;
>> -    gfn_t *status_gfn;
>> -};
>> -
>>   static inline void gnttab_clear_flags(struct domain *d,
>>                                         unsigned int mask, uint16_t 
>> *addr)
>>   {
>> @@ -46,41 +41,12 @@ int replace_grant_host_mapping(unsigned long 
>> gpaddr, mfn_t mfn,
>>   #define gnttab_dom0_frames() \
>>       min_t(unsigned int, opt_max_grant_frames, PFN_DOWN(_etext - 
>> _stext))
>>   -#define gnttab_init_arch(gt) \
>> -({ \
>> -    unsigned int ngf_ = 
>> (gt)->max_grant_frames;                          \
>> -    unsigned int nsf_ = 
>> grant_to_status_frames(ngf_);                    \
>> - \
>> -    (gt)->arch.shared_gfn = xmalloc_array(gfn_t, 
>> ngf_);                  \
>> -    (gt)->arch.status_gfn = xmalloc_array(gfn_t, 
>> nsf_);                  \
>> -    if ( (gt)->arch.shared_gfn && (gt)->arch.status_gfn 
>> )                \
>> - { \
>> -        while ( ngf_-- 
>> )                                                 \
>> -            (gt)->arch.shared_gfn[ngf_] = 
>> INVALID_GFN;                   \
>> -        while ( nsf_-- 
>> )                                                 \
>> -            (gt)->arch.status_gfn[nsf_] = 
>> INVALID_GFN;                   \
>> - } \
>> - else \
>> - gnttab_destroy_arch(gt); \
>> -    (gt)->arch.shared_gfn ? 0 : 
>> -ENOMEM;                                 \
>> -})
>> -
>> -#define gnttab_destroy_arch(gt) \
>> -    do { \
>> - XFREE((gt)->arch.shared_gfn); \
>> - XFREE((gt)->arch.status_gfn); \
>> -    } while ( 0 )
>> -
>>   #define gnttab_set_frame_gfn(gt, st, idx, gfn, 
>> mfn)                      \
>> ({ \
>> -        int rc_ = 
>> 0;                                                     \
>>           gfn_t ogfn = gnttab_get_frame_gfn(gt, st, 
>> idx);                  \
>> -        if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) 
>> ||           \
>> -             (rc_ = guest_physmap_remove_page((gt)->domain, ogfn, 
>> mfn,   \
>> -                                              0)) == 0 
>> )                 \
>> -            ((st) ? 
>> (gt)->arch.status_gfn                                \
>> -                  : (gt)->arch.shared_gfn)[idx] = 
>> (gfn);                 \
>> - rc_; \
>> +        (!gfn_eq(ogfn, INVALID_GFN) && !gfn_eq(ogfn, 
>> gfn))               \
>> +         ? guest_physmap_remove_page((gt)->domain, ogfn, mfn, 
>> 0)         \
>> +         : 
>> 0;                                                            \
>
> Given that we are implementing something similar to an M2P, I was 
> expecting the implementation to be pretty much the same as the x86 
> helper.
>
> Would you be able to outline why it is different?

Being honest, I didn't think about it so far.  But, I agree with the 
question.

It feels to me that Arm variant can now behave as x86 one (as 
xenmem_add_to_physmap_one() now checks for the prior mapping), I mean to 
use INVALID_GFN as an indication to remove a page.

What do you think?


>
>
>>       })
>>     #define gnttab_get_frame_gfn(gt, st, idx) 
>> ({                             \
>> @@ -88,11 +54,21 @@ int replace_grant_host_mapping(unsigned long 
>> gpaddr, mfn_t mfn,
>>           : gnttab_shared_gfn(NULL, gt, 
>> idx);                              \
>>   })
>>   -#define gnttab_shared_gfn(d, t, 
>> i)                                       \
>> -    (((i) >= nr_grant_frames(t)) ? INVALID_GFN : 
>> (t)->arch.shared_gfn[i])
>> +#define gnttab_shared_page(t, i) 
>> ({                                      \
>> + virt_to_page((t)->shared_raw[i]); \
>> +})
>
> This can be simplified to:
>
> #define gnttab_shared_page(t, i) virt_to_page((t)->shared_raw[i])


agree, will do


>
>> +
>> +#define gnttab_status_page(t, i) 
>> ({                                      \
>> + virt_to_page((t)->status[i]); \
>> +})
>
> Same here.

ok


>
>>   -#define gnttab_status_gfn(d, t, 
>> i)                                       \
>> -    (((i) >= nr_status_frames(t)) ? INVALID_GFN : 
>> (t)->arch.status_gfn[i])
>> +#define gnttab_shared_gfn(d, t, i) 
>> ({                                    \
>> +    page_get_xenheap_gfn(gnttab_shared_page(t, 
>> i));                      \
>> +})
>
> Same here

ok


>
>> +
>> +#define gnttab_status_gfn(d, t, i) 
>> ({                                    \
>> +    page_get_xenheap_gfn(gnttab_status_page(t, 
>> i));                      \
>> +})
>
> Same here.

ok


>
>>     #define gnttab_need_iommu_mapping(d)                    \
>>       (is_domain_direct_mapped(d) && is_iommu_enabled(d))
>> diff --git a/xen/arch/arm/include/asm/mm.h 
>> b/xen/arch/arm/include/asm/mm.h
>> index 424aaf2..b99044c 100644
>> --- a/xen/arch/arm/include/asm/mm.h
>> +++ b/xen/arch/arm/include/asm/mm.h
>> @@ -98,9 +98,22 @@ 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)
>> + /* 2-bit count of uses of this frame as its current type. */
>> +#define PGT_count_mask    PG_mask(3, 3)
>> +
>> +/*
>> + * Stored in bits [28:0] or [60:0] GFN if page is xenheap page.
>
> This comment would be easier to understand if you add resp. (arm32) 
> and (arm64) after each range.

agree, will do


>
>
>> + */
>> +#define PGT_gfn_width     PG_shift(3)
>> +#define PGT_gfn_mask      ((1UL<<PGT_gfn_width)-1)
>> +
>> +#define PGT_INVALID_XENHEAP_GFN   _gfn(PGT_gfn_mask)
>> +
>> +/*
>> + * An arch-specific initialization pattern is needed for the 
>> type_info field
>> + * as it's GFN portion can contain the valid GFN if page is xenheap 
>> page.
>> + */
>> +#define PGT_TYPE_INFO_INIT_PATTERN gfn_x(PGT_INVALID_XENHEAP_GFN)
>>      /* Cleared when the owning guest 'frees' this page. */
>>   #define _PGC_allocated    PG_shift(1)
>> @@ -358,6 +371,25 @@ void clear_and_clean_page(struct page_info *page);
>>     unsigned int arch_get_dma_bitsize(void);
>>   +static inline gfn_t page_get_xenheap_gfn(const struct page_info *p)
>> +{
>> +    gfn_t gfn_ = _gfn(p->u.inuse.type_info & PGT_gfn_mask);
>> +
>> +    ASSERT(is_xen_heap_page(p));
>> +
>> +    return gfn_eq(gfn_, PGT_INVALID_XENHEAP_GFN) ? INVALID_GFN : gfn_;
>> +}
>> +
>> +static inline void page_set_xenheap_gfn(struct page_info *p, gfn_t gfn)
>> +{
>> +    gfn_t gfn_ = gfn_eq(gfn, INVALID_GFN) ? PGT_INVALID_XENHEAP_GFN 
>> : gfn;
>> +
>> +    ASSERT(is_xen_heap_page(p));
>> +
>> +    p->u.inuse.type_info &= ~PGT_gfn_mask;
>> +    p->u.inuse.type_info |= gfn_x(gfn_);
>> +}
>
> This is not going to be atomic. So can you outline which locking 
> mechanism should be used to protect access (set/get) to the GFN?


I think, the P2M lock.


>
>
> I will do another review of the patch once I know what we locking 
> should protect the accesses.
>
> Cheers,
>
-- 
Regards,

Oleksandr Tyshchenko