Message ID | 536F9707.4060807@linaro.org |
---|---|
State | Not Applicable, archived |
Headers | show |
On 05/11/2014 10:28 AM, Julien Grall wrote: > the guest very soon. With this > solution there is a 1GB hole between the 2 banks. Your function will therefore > stop to work. > > Furthermore, Xen should not assume that the layout of the guest will always start > at GUEST_RAM_BASE. > > I think you can use max_mapped_pfn and lowest_mapped_pfn here. You may need to > modify a bit the signification of it in the p2m code or introduce a new field. These two values don't work for the purpose of dirty tracking. What we need are two fields to track _physical_ memory. lowest_mapped_pfn tracks all memory types, which doesn't apply here. Will new fields to track physical ram space acceptable for you? -Wei
On 05/12/2014 03:00 PM, Wei Huang wrote: > On 05/11/2014 10:28 AM, Julien Grall wrote: >> the guest very soon. With this >> solution there is a 1GB hole between the 2 banks. Your function will >> therefore >> stop to work. >> >> Furthermore, Xen should not assume that the layout of the guest will >> always start >> at GUEST_RAM_BASE. >> >> I think you can use max_mapped_pfn and lowest_mapped_pfn here. You may >> need to >> modify a bit the signification of it in the p2m code or introduce a >> new field. > These two values don't work for the purpose of dirty tracking. What we > need are two fields to track _physical_ memory. lowest_mapped_pfn tracks > all memory types, which doesn't apply here. > > Will new fields to track physical ram space acceptable for you? I'm fine with new fields to track physical RAM space. Don't forget it may have non-RAM hole in this range. Regards,
On Sun, 2014-05-11 at 16:28 +0100, Julien Grall wrote: > [..] > > > +/* Return start and end addr of guest RAM. Note this function only reports > > + * regular RAM. It does not cover other areas such as foreign mapped > > + * pages or MMIO space. */ > > +void domain_get_ram_range(struct domain *d, paddr_t *start, paddr_t *end) > > +{ > > + if ( start ) > > + *start = GUEST_RAM_BASE; > > + > > + if ( end ) > > + *end = GUEST_RAM_BASE + ((paddr_t) d->max_pages << PAGE_SHIFT); > > +} > > As said on V1 this solution won't work. > > Ian plans to add multiple banks support for the guest very soon. With this > solution there is a 1GB hole between the 2 banks. Your function will therefore > stop to work. > > Furthermore, Xen should not assume that the layout of the guest will always start > at GUEST_RAM_BASE. Actually, for the time being that is fine if it is internal to the hypervisor, although it might be storing up pain for later. > > + for ( i2 = second_index; i2 < LPAE_ENTRIES; ++i2 ) > > + { > > + lpae_walk_t second_pte = second[i2].walk; > > + > > + if ( !second_pte.valid || !second_pte.table ) > > + goto out; > > With Ian's multiple bank support, the RAM region (as returned by domain_get_range) > can contain a hole. Rather than leaving the loop, you should continue. Even without that patch you'd need to be careful of ballooned out regions. > > @@ -341,6 +343,27 @@ static inline void put_page_and_type(struct page_info *page) > > put_page(page); > > } > > > > +enum mg { mg_clear, mg_ro, mg_rw, mg_rx }; > > Please describe this enum. Also mg is too generic. This is just code motion. > > @@ -41,6 +42,7 @@ typedef enum { > > p2m_invalid = 0, /* Nothing mapped here */ > > p2m_ram_rw, /* Normal read/write guest RAM */ > > p2m_ram_ro, /* Read-only; writes are silently dropped */ > > + p2m_ram_logdirty, /* Read-only: special mode for log dirty */ > > You should at the new type at the end of the enum. Why? Keeping p2m_ram_* together doesn't seem wrong to me. Ian.
On Mon, 2014-05-12 at 15:11 +0100, Julien Grall wrote: > On 05/12/2014 03:00 PM, Wei Huang wrote: > > On 05/11/2014 10:28 AM, Julien Grall wrote: > >> the guest very soon. With this > >> solution there is a 1GB hole between the 2 banks. Your function will > >> therefore > >> stop to work. > >> > >> Furthermore, Xen should not assume that the layout of the guest will > >> always start > >> at GUEST_RAM_BASE. > >> > >> I think you can use max_mapped_pfn and lowest_mapped_pfn here. You may > >> need to > >> modify a bit the signification of it in the p2m code or introduce a > >> new field. > > These two values don't work for the purpose of dirty tracking. What we > > need are two fields to track _physical_ memory. lowest_mapped_pfn tracks > > all memory types, which doesn't apply here. > > > > Will new fields to track physical ram space acceptable for you? > > I'm fine with new fields to track physical RAM space. Don't forget it > may have non-RAM hole in this range. FWIW x86 appears to use a rangeset for this purpose, and keeps it up to date when modifying the p2m. Doesn't seem like a bad plan to me... Ian.
On 05/14/2014 12:57 PM, Ian Campbell wrote: >>> @@ -341,6 +343,27 @@ static inline void put_page_and_type(struct page_info *page) >>> put_page(page); >>> } >>> >>> +enum mg { mg_clear, mg_ro, mg_rw, mg_rx }; >> >> Please describe this enum. Also mg is too generic. > > This is just code motion. This enum has been moved in a header which is included everywhere. Keeping the name "mg" without any description is confusion. Developper can misuse this enum. >>> @@ -41,6 +42,7 @@ typedef enum { >>> p2m_invalid = 0, /* Nothing mapped here */ >>> p2m_ram_rw, /* Normal read/write guest RAM */ >>> p2m_ram_ro, /* Read-only; writes are silently dropped */ >>> + p2m_ram_logdirty, /* Read-only: special mode for log dirty */ >> >> You should at the new type at the end of the enum. > > Why? Keeping p2m_ram_* together doesn't seem wrong to me. My mistake, I though we store the P2M during the migration. Regards,
On Wed, 2014-05-14 at 13:20 +0100, Julien Grall wrote: > On 05/14/2014 12:57 PM, Ian Campbell wrote: > >>> @@ -341,6 +343,27 @@ static inline void put_page_and_type(struct page_info *page) > >>> put_page(page); > >>> } > >>> > >>> +enum mg { mg_clear, mg_ro, mg_rw, mg_rx }; > >> > >> Please describe this enum. Also mg is too generic. > > > > This is just code motion. > > This enum has been moved in a header which is included everywhere. > > Keeping the name "mg" without any description is confusion. Developper > can misuse this enum. Actually, I think the use of enum mg where it was used here was wrong and should have been p2m_type_t, so no need to move this either. Ian.
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 603c097..61450cf 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -72,6 +72,21 @@ void p2m_restore_state(struct vcpu *n) isb(); } +void flush_tlb_domain(struct domain *d) +{ + /* Update the VTTBR if necessary with the domain d. In this case, + * it's only necessary to flush TLBs on every CPUs with the current VMID + * (our domain). + */ + if ( d != current->domain ) + p2m_load_VTTBR(d); + + flush_tlb(); + + if ( d != current->domain ) + p2m_load_VTTBR(current->domain); +} + static int p2m_first_level_index(paddr_t addr) { /* @@ -450,19 +465,7 @@ static int apply_p2m_changes(struct domain *d, } if ( flush ) - { - /* Update the VTTBR if necessary with the domain where mappings - * are created. In this case it's only necessary to flush TLBs - * on every CPUs with the current VMID (our domain). - */ - if ( d != current->domain ) - p2m_load_VTTBR(d); - - flush_tlb(); - - if ( d != current->domain ) - p2m_load_VTTBR(current->domain); - } + flush_tlb_domain(d); if ( op == ALLOCATE || op == INSERT ) { @@ -550,14 +553,10 @@ int p2m_alloc_table(struct domain *d) d->arch.vttbr = page_to_maddr(p2m->first_level) | ((uint64_t)p2m->vmid&0xff)<<48; - p2m_load_VTTBR(d); - /* Make sure that all TLBs corresponding to the new VMID are flushed * before using it */ - flush_tlb(); - - p2m_load_VTTBR(current->domain); + flush_tlb_domain(d); spin_unlock(&p2m->lock); diff --git a/xen/include/asm-arm/flushtlb.h b/xen/include/asm-arm/flushtlb.h index 329fbb4..5722c67 100644 --- a/xen/include/asm-arm/flushtlb.h +++ b/xen/include/asm-arm/flushtlb.h @@ -25,6 +25,9 @@ do { \ /* Flush specified CPUs' TLBs */ void flush_tlb_mask(const cpumask_t *mask); +/* Flush CPU's TLBs for the speficied domain */ +void flush_tlb_domain(struct domain *d); + #endif /* __ASM_ARM_FLUSHTLB_H__ */ /* * Local variables: