Patch 1 fixes a really bad bug of mine, and while at it I thought I would carry out the other recently noticed work item here right away. 1: adjust special domain creation (and call it earlier on x86) 2: dom_cow is needed for mem-sharing only Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Split out this mostly arch-independent code into a common-code helper function. (This does away with Arm's arch_init_memory() altogether.) On x86 this needs to happen before acpi_boot_init(): Commit 9fa94e1058 ("x86/ACPI: also parse AMD IOMMU tables early") only appeared to work fine - it's really broken, and doesn't crash (on non-EFI AMD systems) only because of there being a mapping of linear address 0 during early boot. On EFI there is: Early fatal page fault at e008:ffff82d08024d58e (cr2=0000000000000220, ec=0000) ----[ Xen-4.13-unstable x86_64 debug=y Not tainted ]---- CPU: 0 RIP: e008:[<ffff82d08024d58e>] pci.c#_pci_hide_device+0x17/0x3a RFLAGS: 0000000000010046 CONTEXT: hypervisor rax: 0000000000000000 rbx: 0000000000006000 rcx: 0000000000000000 rdx: ffff83104f2ee9b0 rsi: ffff82e0209e5d48 rdi: ffff83104f2ee9a0 rbp: ffff82d08081fce0 rsp: ffff82d08081fcb8 r8: 0000000000000000 r9: 8000000000000000 r10: 0180000000000000 r11: 7fffffffffffffff r12: ffff83104f2ee9a0 r13: 0000000000000002 r14: ffff83104f2ee4b0 r15: 0000000000000064 cr0: 0000000080050033 cr4: 00000000000000a0 cr3: 000000009f614000 cr2: 0000000000000220 fsb: 0000000000000000 gsb: 0000000000000000 gss: 0000000000000000 ds: 0000 es: 0000 fs: 0000 gs: 0000 ss: 0000 cs: e008 Xen code around <ffff82d08024d58e> (pci.c#_pci_hide_device+0x17/0x3a): 48 89 47 38 48 8d 57 10 <48> 8b 88 20 02 00 00 48 89 51 08 48 89 4f 10 48 Xen stack trace from rsp=ffff82d08081fcb8: [...] Xen call trace: [<ffff82d08024d58e>] pci.c#_pci_hide_device+0x17/0x3a [ [< >] pci_ro_device+...] [<ffff82d080617fe1>] amd_iommu_detect_one_acpi+0x161/0x249 [<ffff82d0806186ac>] iommu_acpi.c#detect_iommu_acpi+0xb5/0xe7 [<ffff82d08061cde0>] acpi_table_parse+0x61/0x90 [<ffff82d080619e7d>] amd_iommu_detect_acpi+0x17/0x19 [<ffff82d08061790b>] acpi_ivrs_init+0x20/0x5b [<ffff82d08062e838>] acpi_boot_init+0x301/0x30f [<ffff82d080628b10>] __start_xen+0x1daf/0x28a2 Pagetable walk from 0000000000000220: L4[0x000] = 000000009f44f063 ffffffffffffffff L3[0x000] = 000000009f44b063 ffffffffffffffff L2[0x000] = 0000000000000000 ffffffffffffffff **************************************** Panic on CPU 0: FATAL TRAP: vector = 14 (page fault) [error_code=0000] , IN INTERRUPT CONTEXT **************************************** Of course the bug would nevertheless have lead to post-boot crashes as soon as the list would actually get traversed. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -42,8 +42,6 @@ #include <xen/libfdt/libfdt.h> #include <asm/setup.h> -struct domain *dom_xen, *dom_io, *dom_cow; - /* Override macros from asm/page.h to make them work with mfn_t */ #undef virt_to_mfn #define virt_to_mfn(va) _mfn(__virt_to_mfn(va)) @@ -513,32 +511,6 @@ void flush_page_to_ram(unsigned long mfn invalidate_icache(); } -void __init arch_init_memory(void) -{ - /* - * Initialise our DOMID_XEN domain. - * Any Xen-heap pages that we will allow to be mapped will have - * their domain field set to dom_xen. - */ - dom_xen = domain_create(DOMID_XEN, NULL, false); - BUG_ON(IS_ERR(dom_xen)); - - /* - * Initialise our DOMID_IO domain. - * This domain owns I/O pages that are within the range of the page_info - * array. Mappings occur at the priv of the caller. - */ - dom_io = domain_create(DOMID_IO, NULL, false); - BUG_ON(IS_ERR(dom_io)); - - /* - * Initialise our COW domain. - * This domain owns sharable pages. - */ - dom_cow = domain_create(DOMID_COW, NULL, false); - BUG_ON(IS_ERR(dom_cow)); -} - static inline lpae_t pte_of_xenaddr(vaddr_t va) { paddr_t ma = va + phys_offset; --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -846,7 +846,7 @@ void __init start_xen(unsigned long boot rcu_init(); - arch_init_memory(); + setup_special_domains(); local_irq_enable(); local_abort_enable(); --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -160,9 +160,6 @@ l1_pgentry_t __section(".bss.page_aligne paddr_t __read_mostly mem_hotplug; -/* Private domain structs for DOMID_XEN and DOMID_IO. */ -struct domain *dom_xen, *dom_io, *dom_cow; - /* Frame table size in pages. */ unsigned long max_page; unsigned long total_pages; @@ -283,32 +280,6 @@ void __init arch_init_memory(void) _PAGE_DIRTY | _PAGE_AVAIL | _PAGE_AVAIL_HIGH | _PAGE_NX); /* - * Initialise our DOMID_XEN domain. - * Any Xen-heap pages that we will allow to be mapped will have - * their domain field set to dom_xen. - * Hidden PCI devices will also be associated with this domain - * (but be [partly] controlled by Dom0 nevertheless). - */ - dom_xen = domain_create(DOMID_XEN, NULL, false); - BUG_ON(IS_ERR(dom_xen)); - INIT_LIST_HEAD(&dom_xen->arch.pdev_list); - - /* - * Initialise our DOMID_IO domain. - * This domain owns I/O pages that are within the range of the page_info - * array. Mappings occur at the priv of the caller. - */ - dom_io = domain_create(DOMID_IO, NULL, false); - BUG_ON(IS_ERR(dom_io)); - - /* - * Initialise our COW domain. - * This domain owns sharable pages. - */ - dom_cow = domain_create(DOMID_COW, NULL, false); - BUG_ON(IS_ERR(dom_cow)); - - /* * First 1MB of RAM is historically marked as I/O. * Note that apart from IO Xen also uses the low 1MB to store the AP boot * trampoline and boot information metadata. Due to this always special --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -1533,6 +1533,8 @@ void __init noreturn __start_xen(unsigne mmio_ro_ranges = rangeset_new(NULL, "r/o mmio ranges", RANGESETF_prettyprint_hex); + setup_special_domains(); + acpi_boot_init(); if ( smp_found_config ) --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -71,6 +71,11 @@ domid_t hardware_domid __read_mostly; integer_param("hardware_dom", hardware_domid); #endif +/* Private domain structs for DOMID_XEN, DOMID_IO, etc. */ +struct domain *__read_mostly dom_xen; +struct domain *__read_mostly dom_io; +struct domain *__read_mostly dom_cow; + struct vcpu *idle_vcpu[NR_CPUS] __read_mostly; vcpu_info_t dummy_vcpu_info; @@ -516,6 +521,36 @@ struct domain *domain_create(domid_t dom return ERR_PTR(err); } +void __init setup_special_domains(void) +{ + /* + * Initialise our DOMID_XEN domain. + * Any Xen-heap pages that we will allow to be mapped will have + * their domain field set to dom_xen. + * Hidden PCI devices will also be associated with this domain + * (but be [partly] controlled by Dom0 nevertheless). + */ + dom_xen = domain_create(DOMID_XEN, NULL, false); + BUG_ON(IS_ERR(dom_xen)); +#ifdef CONFIG_HAS_PCI + INIT_LIST_HEAD(&dom_xen->arch.pdev_list); +#endif + + /* + * Initialise our DOMID_IO domain. + * This domain owns I/O pages that are within the range of the page_info + * array. Mappings occur at the priv of the caller. + */ + dom_io = domain_create(DOMID_IO, NULL, false); + BUG_ON(IS_ERR(dom_io)); + + /* + * Initialise our COW domain. + * This domain owns sharable pages. + */ + dom_cow = domain_create(DOMID_COW, NULL, false); + BUG_ON(IS_ERR(dom_cow)); +} void domain_update_node_affinity(struct domain *d) { --- a/xen/include/asm-arm/mm.h +++ b/xen/include/asm-arm/mm.h @@ -334,8 +334,6 @@ long arch_memory_op(int op, XEN_GUEST_HA unsigned long domain_get_maximum_gpfn(struct domain *d); -extern struct domain *dom_xen, *dom_io, *dom_cow; - #define memguard_guard_stack(_p) ((void)0) #define memguard_guard_range(_p,_l) ((void)0) #define memguard_unguard_range(_p,_l) ((void)0) --- a/xen/include/asm-arm/setup.h +++ b/xen/include/asm-arm/setup.h @@ -77,8 +77,6 @@ extern struct bootinfo bootinfo; extern domid_t max_init_domid; -void arch_init_memory(void); - void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len); size_t estimate_efi_size(int mem_nr_banks); --- a/xen/include/asm-x86/mm.h +++ b/xen/include/asm-x86/mm.h @@ -595,8 +595,6 @@ unsigned int domain_clamp_alloc_bitsize( unsigned long domain_get_maximum_gpfn(struct domain *d); -extern struct domain *dom_xen, *dom_io, *dom_cow; /* for vmcoreinfo */ - /* Definition of an mm lock: spinlock with extra fields for debugging */ typedef struct mm_lock { spinlock_t lock; --- a/xen/include/xen/domain.h +++ b/xen/include/xen/domain.h @@ -5,6 +5,7 @@ #include <xen/types.h> #include <public/xen.h> + #include <asm/domain.h> #include <asm/numa.h> @@ -22,6 +23,8 @@ struct vcpu *alloc_dom0_vcpu0(struct dom int vcpu_reset(struct vcpu *); int vcpu_up(struct vcpu *v); +void setup_special_domains(void); + struct xen_domctl_getdomaininfo; void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info); void arch_get_domain_info(const struct domain *d, --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -642,6 +642,9 @@ static inline void filtered_flush_tlb_ma } } +/* Private domain structs for DOMID_XEN, DOMID_IO, etc. */ +extern struct domain *dom_xen, *dom_io, *dom_cow; + enum XENSHARE_flags { SHARE_rw, SHARE_ro, _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Hi Jan, The Arm code looks good to me. I have few comments regarding the common changes. On 5/31/19 10:35 AM, Jan Beulich wrote: > --- a/xen/common/domain.c > +++ b/xen/common/domain.c > @@ -71,6 +71,11 @@ domid_t hardware_domid __read_mostly; > integer_param("hardware_dom", hardware_domid); > #endif > > +/* Private domain structs for DOMID_XEN, DOMID_IO, etc. */ > +struct domain *__read_mostly dom_xen; > +struct domain *__read_mostly dom_io; > +struct domain *__read_mostly dom_cow; The __read_mostly makes sense here, however please mention it in the commit message. [...] > --- a/xen/include/asm-x86/mm.h > +++ b/xen/include/asm-x86/mm.h > @@ -595,8 +595,6 @@ unsigned int domain_clamp_alloc_bitsize( > > unsigned long domain_get_maximum_gpfn(struct domain *d); > > -extern struct domain *dom_xen, *dom_io, *dom_cow; /* for vmcoreinfo */ > - > /* Definition of an mm lock: spinlock with extra fields for debugging */ > typedef struct mm_lock { > spinlock_t lock; > --- a/xen/include/xen/domain.h > +++ b/xen/include/xen/domain.h > @@ -5,6 +5,7 @@ > #include <xen/types.h> > > #include <public/xen.h> > + Without an explanation in the commit message, this looks like a spurious change. > #include <asm/domain.h> > #include <asm/numa.h> Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
>>> On 31.05.19 at 12:03, <julien.grall@arm.com> wrote: > On 5/31/19 10:35 AM, Jan Beulich wrote: >> --- a/xen/include/xen/domain.h >> +++ b/xen/include/xen/domain.h >> @@ -5,6 +5,7 @@ >> #include <xen/types.h> >> >> #include <public/xen.h> >> + > > Without an explanation in the commit message, this looks like a spurious > change. Oh, it indeed is - it's a leftover from where I had first tried to place the declarations. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On 31/05/2019 02:35, Jan Beulich wrote: > @@ -516,6 +521,36 @@ struct domain *domain_create(domid_t dom > return ERR_PTR(err); > } > > +void __init setup_special_domains(void) > +{ > + /* > + * Initialise our DOMID_XEN domain. > + * Any Xen-heap pages that we will allow to be mapped will have > + * their domain field set to dom_xen. > + * Hidden PCI devices will also be associated with this domain > + * (but be [partly] controlled by Dom0 nevertheless). > + */ > + dom_xen = domain_create(DOMID_XEN, NULL, false); > + BUG_ON(IS_ERR(dom_xen)); I know this is copying code like-for-like, but this error handling is terrible in practice. Even just: if ( IS_ERR(dom_xen) ) panic("Failed to create dom_xen: %d\n", PTR_ERR(dom_xen)); would be an improvement. > +#ifdef CONFIG_HAS_PCI > + INIT_LIST_HEAD(&dom_xen->arch.pdev_list); > +#endif The position of this identifies that we've got obviously got bugs (perhaps latent) elsewhere, seeing as other special domains don't get working constructs such as list_empty(). In the code which currently exists, I can't spot it ever being touched for ARM, but it is constructed for all non-special x86 guests at the top of arch_domain_create(). I think the best option, given the #ifdef here, is to reposition the pdev fields into struct domain, rather than arch_domain, and have this code fragment near the top of domain_create() where special domains will all be covered. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
>>> On 31.05.19 at 17:32, <andrew.cooper3@citrix.com> wrote: > On 31/05/2019 02:35, Jan Beulich wrote: >> @@ -516,6 +521,36 @@ struct domain *domain_create(domid_t dom >> return ERR_PTR(err); >> } >> >> +void __init setup_special_domains(void) >> +{ >> + /* >> + * Initialise our DOMID_XEN domain. >> + * Any Xen-heap pages that we will allow to be mapped will have >> + * their domain field set to dom_xen. >> + * Hidden PCI devices will also be associated with this domain >> + * (but be [partly] controlled by Dom0 nevertheless). >> + */ >> + dom_xen = domain_create(DOMID_XEN, NULL, false); >> + BUG_ON(IS_ERR(dom_xen)); > > I know this is copying code like-for-like, but this error handling is > terrible in practice. > > Even just: > > if ( IS_ERR(dom_xen) ) > panic("Failed to create dom_xen: %d\n", PTR_ERR(dom_xen)); > > would be an improvement. I'll be happy to do this; I didn't just because it doesn't really belong here. >> +#ifdef CONFIG_HAS_PCI >> + INIT_LIST_HEAD(&dom_xen->arch.pdev_list); >> +#endif > > The position of this identifies that we've got obviously got bugs > (perhaps latent) elsewhere, seeing as other special domains don't get > working constructs such as list_empty(). > > In the code which currently exists, I can't spot it ever being touched > for ARM, but it is constructed for all non-special x86 guests at the top > of arch_domain_create(). > > I think the best option, given the #ifdef here, is to reposition the > pdev fields into struct domain, rather than arch_domain, and have this > code fragment near the top of domain_create() where special domains will > all be covered. Except that if I do this, then not by special casing special domains. "Normal" domains want this too - the initialization could then be dropped (moved there) from x86-specific code. But this will want to be a separate patch then. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On 31/05/2019 08:59, Jan Beulich wrote: > >>> +#ifdef CONFIG_HAS_PCI >>> + INIT_LIST_HEAD(&dom_xen->arch.pdev_list); >>> +#endif >> The position of this identifies that we've got obviously got bugs >> (perhaps latent) elsewhere, seeing as other special domains don't get >> working constructs such as list_empty(). >> >> In the code which currently exists, I can't spot it ever being touched >> for ARM, but it is constructed for all non-special x86 guests at the top >> of arch_domain_create(). >> >> I think the best option, given the #ifdef here, is to reposition the >> pdev fields into struct domain, rather than arch_domain, and have this >> code fragment near the top of domain_create() where special domains will >> all be covered. > Except that if I do this, then not by special casing special domains. What special casing? There is a block of code near the start of domain_create() which is run for all domain, including special ones. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
>>> On 31.05.19 at 18:02, <andrew.cooper3@citrix.com> wrote: > On 31/05/2019 08:59, Jan Beulich wrote: >> >>>> +#ifdef CONFIG_HAS_PCI >>>> + INIT_LIST_HEAD(&dom_xen->arch.pdev_list); >>>> +#endif >>> The position of this identifies that we've got obviously got bugs >>> (perhaps latent) elsewhere, seeing as other special domains don't get >>> working constructs such as list_empty(). >>> >>> In the code which currently exists, I can't spot it ever being touched >>> for ARM, but it is constructed for all non-special x86 guests at the top >>> of arch_domain_create(). >>> >>> I think the best option, given the #ifdef here, is to reposition the >>> pdev fields into struct domain, rather than arch_domain, and have this >>> code fragment near the top of domain_create() where special domains will >>> all be covered. >> Except that if I do this, then not by special casing special domains. > > What special casing? There is a block of code near the start of > domain_create() which is run for all domain, including special ones. Oh, perhaps a misunderstanding on my part then, as you were saying "where special domains will all be covered", without also mentioning non-special ones. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
A couple of adjustments are needed to code checking for dom_cow, but since there are pretty few it is probably better to adjust those than to set up and keep around a never used domain. Take the opportunity and tighten a BUG_ON() in emul-priv-op.c:read_cr(). (Arguably this perhaps shouldn't be a BUG_ON() in the first place.) Signed-off-by: Jan Beulich <jbeulich@suse.com> --- While for now this avoids creating the domain on Arm only, Tamas'es patch switching to CONFIG_MEM_SHARING will make x86 leverage this too. --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -967,8 +967,8 @@ get_page_from_l1e( return flip; } - if ( unlikely( (real_pg_owner != pg_owner) && - (real_pg_owner != dom_cow) ) ) + if ( unlikely((real_pg_owner != pg_owner) && + (!dom_cow || (real_pg_owner != dom_cow))) ) { /* * Let privileged domains transfer the right to map their target --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -568,7 +568,8 @@ struct page_info *p2m_get_page_from_gfn( } else if ( !get_page(page, p2m->domain) && /* Page could be shared */ - (!p2m_is_shared(*t) || !get_page(page, dom_cow)) ) + (!dom_cow || !p2m_is_shared(*t) || + !get_page(page, dom_cow)) ) page = NULL; } p2m_read_unlock(p2m); @@ -941,7 +942,8 @@ guest_physmap_add_entry(struct domain *d /* Then, look for m->p mappings for this range and deal with them */ for ( i = 0; i < (1UL << page_order); i++ ) { - if ( page_get_owner(mfn_to_page(mfn_add(mfn, i))) == dom_cow ) + if ( dom_cow && + page_get_owner(mfn_to_page(mfn_add(mfn, i))) == dom_cow ) { /* This is no way to add a shared page to your physmap! */ gdprintk(XENLOG_ERR, "Adding shared mfn %lx directly to dom%d physmap not allowed.\n", --- a/xen/arch/x86/pv/emul-priv-op.c +++ b/xen/arch/x86/pv/emul-priv-op.c @@ -723,8 +723,8 @@ static int read_cr(unsigned int reg, uns unmap_domain_page(pl4e); *val = compat_pfn_to_cr3(mfn_to_gmfn(currd, mfn_x(mfn))); } - /* PTs should not be shared */ - BUG_ON(page_get_owner(mfn_to_page(mfn)) == dom_cow); + /* PTs should be owned by their domains */ + BUG_ON(page_get_owner(mfn_to_page(mfn)) != currd); return X86EMUL_OKAY; } } --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -74,7 +74,9 @@ integer_param("hardware_dom", hardware_d /* Private domain structs for DOMID_XEN, DOMID_IO, etc. */ struct domain *__read_mostly dom_xen; struct domain *__read_mostly dom_io; +#ifdef CONFIG_HAS_MEM_SHARING struct domain *__read_mostly dom_cow; +#endif struct vcpu *idle_vcpu[NR_CPUS] __read_mostly; @@ -544,12 +546,14 @@ void __init setup_special_domains(void) dom_io = domain_create(DOMID_IO, NULL, false); BUG_ON(IS_ERR(dom_io)); +#ifdef CONFIG_HAS_MEM_SHARING /* * Initialise our COW domain. * This domain owns sharable pages. */ dom_cow = domain_create(DOMID_COW, NULL, false); BUG_ON(IS_ERR(dom_cow)); +#endif } void domain_update_node_affinity(struct domain *d) --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -1095,7 +1095,7 @@ map_grant_ref( host_map_created = true; } } - else if ( owner == rd || owner == dom_cow ) + else if ( owner == rd || (dom_cow && owner == dom_cow) ) { if ( (op->flags & GNTMAP_device_map) && !(op->flags & GNTMAP_readonly) ) { --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -644,6 +644,9 @@ static inline void filtered_flush_tlb_ma /* Private domain structs for DOMID_XEN, DOMID_IO, etc. */ extern struct domain *dom_xen, *dom_io, *dom_cow; +#ifndef CONFIG_HAS_MEM_SHARING +# define dom_cow NULL +#endif enum XENSHARE_flags { SHARE_rw, _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Hi Jan, On 5/31/19 10:35 AM, Jan Beulich wrote: > A couple of adjustments are needed to code checking for dom_cow, but > since there are pretty few it is probably better to adjust those than > to set up and keep around a never used domain. > > Take the opportunity and tighten a BUG_ON() in emul-priv-op.c:read_cr(). > (Arguably this perhaps shouldn't be a BUG_ON() in the first place.) That's arguably a patch on its own so you can explain why it is tighten. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > While for now this avoids creating the domain on Arm only, Tamas'es > patch switching to CONFIG_MEM_SHARING will make x86 leverage this too. > > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -967,8 +967,8 @@ get_page_from_l1e( > return flip; > } > > - if ( unlikely( (real_pg_owner != pg_owner) && > - (real_pg_owner != dom_cow) ) ) > + if ( unlikely((real_pg_owner != pg_owner) && > + (!dom_cow || (real_pg_owner != dom_cow))) ) > { > /* > * Let privileged domains transfer the right to map their target > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -568,7 +568,8 @@ struct page_info *p2m_get_page_from_gfn( > } > else if ( !get_page(page, p2m->domain) && > /* Page could be shared */ > - (!p2m_is_shared(*t) || !get_page(page, dom_cow)) ) > + (!dom_cow || !p2m_is_shared(*t) || > + !get_page(page, dom_cow)) ) > page = NULL; > } > p2m_read_unlock(p2m); > @@ -941,7 +942,8 @@ guest_physmap_add_entry(struct domain *d > /* Then, look for m->p mappings for this range and deal with them */ > for ( i = 0; i < (1UL << page_order); i++ ) > { > - if ( page_get_owner(mfn_to_page(mfn_add(mfn, i))) == dom_cow ) > + if ( dom_cow && > + page_get_owner(mfn_to_page(mfn_add(mfn, i))) == dom_cow ) > { > /* This is no way to add a shared page to your physmap! */ > gdprintk(XENLOG_ERR, "Adding shared mfn %lx directly to dom%d physmap not allowed.\n", > --- a/xen/arch/x86/pv/emul-priv-op.c > +++ b/xen/arch/x86/pv/emul-priv-op.c > @@ -723,8 +723,8 @@ static int read_cr(unsigned int reg, uns > unmap_domain_page(pl4e); > *val = compat_pfn_to_cr3(mfn_to_gmfn(currd, mfn_x(mfn))); > } > - /* PTs should not be shared */ > - BUG_ON(page_get_owner(mfn_to_page(mfn)) == dom_cow); > + /* PTs should be owned by their domains */ > + BUG_ON(page_get_owner(mfn_to_page(mfn)) != currd); > return X86EMUL_OKAY; > } > } > --- a/xen/common/domain.c > +++ b/xen/common/domain.c > @@ -74,7 +74,9 @@ integer_param("hardware_dom", hardware_d > /* Private domain structs for DOMID_XEN, DOMID_IO, etc. */ > struct domain *__read_mostly dom_xen; > struct domain *__read_mostly dom_io; > +#ifdef CONFIG_HAS_MEM_SHARING > struct domain *__read_mostly dom_cow; > +#endif > > struct vcpu *idle_vcpu[NR_CPUS] __read_mostly; > > @@ -544,12 +546,14 @@ void __init setup_special_domains(void) > dom_io = domain_create(DOMID_IO, NULL, false); > BUG_ON(IS_ERR(dom_io)); > > +#ifdef CONFIG_HAS_MEM_SHARING > /* > * Initialise our COW domain. > * This domain owns sharable pages. > */ > dom_cow = domain_create(DOMID_COW, NULL, false); > BUG_ON(IS_ERR(dom_cow)); > +#endif > } > > void domain_update_node_affinity(struct domain *d) > --- a/xen/common/grant_table.c > +++ b/xen/common/grant_table.c > @@ -1095,7 +1095,7 @@ map_grant_ref( > host_map_created = true; > } > } > - else if ( owner == rd || owner == dom_cow ) > + else if ( owner == rd || (dom_cow && owner == dom_cow) ) > { > if ( (op->flags & GNTMAP_device_map) && !(op->flags & GNTMAP_readonly) ) > { > --- a/xen/include/xen/mm.h > +++ b/xen/include/xen/mm.h > @@ -644,6 +644,9 @@ static inline void filtered_flush_tlb_ma > > /* Private domain structs for DOMID_XEN, DOMID_IO, etc. */ > extern struct domain *dom_xen, *dom_io, *dom_cow; Don't you want to protect dom_cow with "#ifdef CONFIG_HAS_MEM_SHARING"? > +#ifndef CONFIG_HAS_MEM_SHARING > +# define dom_cow NULL > +#endif > > enum XENSHARE_flags { > SHARE_rw, > > > -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
>>> On 31.05.19 at 11:42, <julien.grall@arm.com> wrote: > On 5/31/19 10:35 AM, Jan Beulich wrote: >> --- a/xen/include/xen/mm.h >> +++ b/xen/include/xen/mm.h >> @@ -644,6 +644,9 @@ static inline void filtered_flush_tlb_ma >> >> /* Private domain structs for DOMID_XEN, DOMID_IO, etc. */ >> extern struct domain *dom_xen, *dom_io, *dom_cow; > > Don't you want to protect dom_cow with "#ifdef CONFIG_HAS_MEM_SHARING"? There's no need to with ... >> +#ifndef CONFIG_HAS_MEM_SHARING >> +# define dom_cow NULL >> +#endif ... this, and this way there's less clutter overall. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Hi, On 5/31/19 10:49 AM, Jan Beulich wrote: >>>> On 31.05.19 at 11:42, <julien.grall@arm.com> wrote: >> On 5/31/19 10:35 AM, Jan Beulich wrote: >>> --- a/xen/include/xen/mm.h >>> +++ b/xen/include/xen/mm.h >>> @@ -644,6 +644,9 @@ static inline void filtered_flush_tlb_ma >>> >>> /* Private domain structs for DOMID_XEN, DOMID_IO, etc. */ >>> extern struct domain *dom_xen, *dom_io, *dom_cow; >> >> Don't you want to protect dom_cow with "#ifdef CONFIG_HAS_MEM_SHARING"? > > There's no need to with ... > >>> +#ifndef CONFIG_HAS_MEM_SHARING >>> +# define dom_cow NULL >>> +#endif > > ... this, and this way there's less clutter overall. I am all for avoiding cluttering but not at the expense of making the code less intuitive. In this case, I would prefer the decleration dom_cow to be guarded. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
>>> On 31.05.19 at 11:52, <julien.grall@arm.com> wrote: > On 5/31/19 10:49 AM, Jan Beulich wrote: >>>>> On 31.05.19 at 11:42, <julien.grall@arm.com> wrote: >>> On 5/31/19 10:35 AM, Jan Beulich wrote: >>>> --- a/xen/include/xen/mm.h >>>> +++ b/xen/include/xen/mm.h >>>> @@ -644,6 +644,9 @@ static inline void filtered_flush_tlb_ma >>>> >>>> /* Private domain structs for DOMID_XEN, DOMID_IO, etc. */ >>>> extern struct domain *dom_xen, *dom_io, *dom_cow; >>> >>> Don't you want to protect dom_cow with "#ifdef CONFIG_HAS_MEM_SHARING"? >> >> There's no need to with ... >> >>>> +#ifndef CONFIG_HAS_MEM_SHARING >>>> +# define dom_cow NULL >>>> +#endif >> >> ... this, and this way there's less clutter overall. > > I am all for avoiding cluttering but not at the expense of making the > code less intuitive. In this case, I would prefer the decleration > dom_cow to be guarded. While it would be easy enough to do, I fail to see how the chosen construct is not "intuitive". In fact I don't think this would be the first instance of a #define overriding a prior declaration. Doing so utilizes one of the very basic C preprocessor principles. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Hi Jan, On 5/31/19 11:03 AM, Jan Beulich wrote: >>>> On 31.05.19 at 11:52, <julien.grall@arm.com> wrote: >> On 5/31/19 10:49 AM, Jan Beulich wrote: >>>>>> On 31.05.19 at 11:42, <julien.grall@arm.com> wrote: >>>> On 5/31/19 10:35 AM, Jan Beulich wrote: >>>>> --- a/xen/include/xen/mm.h >>>>> +++ b/xen/include/xen/mm.h >>>>> @@ -644,6 +644,9 @@ static inline void filtered_flush_tlb_ma >>>>> >>>>> /* Private domain structs for DOMID_XEN, DOMID_IO, etc. */ >>>>> extern struct domain *dom_xen, *dom_io, *dom_cow; >>>> >>>> Don't you want to protect dom_cow with "#ifdef CONFIG_HAS_MEM_SHARING"? >>> >>> There's no need to with ... >>> >>>>> +#ifndef CONFIG_HAS_MEM_SHARING >>>>> +# define dom_cow NULL >>>>> +#endif >>> >>> ... this, and this way there's less clutter overall. >> >> I am all for avoiding cluttering but not at the expense of making the >> code less intuitive. In this case, I would prefer the decleration >> dom_cow to be guarded. > > While it would be easy enough to do, I fail to see how the chosen > construct is not "intuitive". Even if I know the pre-preprocessor will do the right thing here, you could spare the developper to trip over this code and wonder why it is first defined and then override with NULL. > In fact I don't think this would be the > first instance of a #define overriding a prior declaration. Doing so > utilizes one of the very basic C preprocessor principles. You are the first one usually to say that some choices in Xen were not correct and therefore no more instance should be added. This is one case where the resulting code is counter-intuitive and ugly. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
>>> On 31.05.19 at 12:10, <julien.grall@arm.com> wrote: > On 5/31/19 11:03 AM, Jan Beulich wrote: >>>>> On 31.05.19 at 11:52, <julien.grall@arm.com> wrote: >>> On 5/31/19 10:49 AM, Jan Beulich wrote: >>>>>>> On 31.05.19 at 11:42, <julien.grall@arm.com> wrote: >>>>> On 5/31/19 10:35 AM, Jan Beulich wrote: >>>>>> --- a/xen/include/xen/mm.h >>>>>> +++ b/xen/include/xen/mm.h >>>>>> @@ -644,6 +644,9 @@ static inline void filtered_flush_tlb_ma >>>>>> >>>>>> /* Private domain structs for DOMID_XEN, DOMID_IO, etc. */ >>>>>> extern struct domain *dom_xen, *dom_io, *dom_cow; >>>>> >>>>> Don't you want to protect dom_cow with "#ifdef CONFIG_HAS_MEM_SHARING"? >>>> >>>> There's no need to with ... >>>> >>>>>> +#ifndef CONFIG_HAS_MEM_SHARING >>>>>> +# define dom_cow NULL >>>>>> +#endif >>>> >>>> ... this, and this way there's less clutter overall. >>> >>> I am all for avoiding cluttering but not at the expense of making the >>> code less intuitive. In this case, I would prefer the decleration >>> dom_cow to be guarded. >> >> While it would be easy enough to do, I fail to see how the chosen >> construct is not "intuitive". > > Even if I know the pre-preprocessor will do the right thing here, you > could spare the developper to trip over this code and wonder why it is > first defined and then override with NULL. To be honest, an unconditional declaration with a conditional override doesn't leave much to wonder about. I'll wait to see what other maintainers think before deciding either way. >> In fact I don't think this would be the >> first instance of a #define overriding a prior declaration. Doing so >> utilizes one of the very basic C preprocessor principles. > > You are the first one usually to say that some choices in Xen were not > correct and therefore no more instance should be added. Oh, did my earlier reply sound as if I'm not happy about those mentioned instances? I certainly didn't mean it to be that way - some of them I'm liable to have introduced myself, and I continue to approve of them being there. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Hi Jan, On 5/31/19 11:31 AM, Jan Beulich wrote: >>>> On 31.05.19 at 12:10, <julien.grall@arm.com> wrote: >> On 5/31/19 11:03 AM, Jan Beulich wrote: >>>>>> On 31.05.19 at 11:52, <julien.grall@arm.com> wrote: >>>> On 5/31/19 10:49 AM, Jan Beulich wrote: >>>>>>>> On 31.05.19 at 11:42, <julien.grall@arm.com> wrote: >>>>>> On 5/31/19 10:35 AM, Jan Beulich wrote: >>>>>>> --- a/xen/include/xen/mm.h >>>>>>> +++ b/xen/include/xen/mm.h >>>>>>> @@ -644,6 +644,9 @@ static inline void filtered_flush_tlb_ma >>>>>>> >>>>>>> /* Private domain structs for DOMID_XEN, DOMID_IO, etc. */ >>>>>>> extern struct domain *dom_xen, *dom_io, *dom_cow; >>>>>> >>>>>> Don't you want to protect dom_cow with "#ifdef CONFIG_HAS_MEM_SHARING"? >>>>> >>>>> There's no need to with ... >>>>> >>>>>>> +#ifndef CONFIG_HAS_MEM_SHARING >>>>>>> +# define dom_cow NULL >>>>>>> +#endif >>>>> >>>>> ... this, and this way there's less clutter overall. >>>> >>>> I am all for avoiding cluttering but not at the expense of making the >>>> code less intuitive. In this case, I would prefer the decleration >>>> dom_cow to be guarded. >>> >>> While it would be easy enough to do, I fail to see how the chosen >>> construct is not "intuitive". >> >> Even if I know the pre-preprocessor will do the right thing here, you >> could spare the developper to trip over this code and wonder why it is >> first defined and then override with NULL. > > To be honest, an unconditional declaration with a conditional > override doesn't leave much to wonder about. I'll wait to see > what other maintainers think before deciding either way. > >>> In fact I don't think this would be the >>> first instance of a #define overriding a prior declaration. Doing so >>> utilizes one of the very basic C preprocessor principles. >> >> You are the first one usually to say that some choices in Xen were not >> correct and therefore no more instance should be added. > > Oh, did my earlier reply sound as if I'm not happy about those > mentioned instances? No it was a more generic statement on the stance "It already exists in Xen so it is fine to spread them a bit more". Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
>>> On 31.05.19 at 12:34, <julien.grall@arm.com> wrote: > No it was a more generic statement on the stance "It already exists in > Xen so it is fine to spread them a bit more". Oh, I see. Of course I'm making remarks when what's in the tree is bad (as per e.g. coding style, or if not mentioned there than in my personal opinion). As a result I take note of you thinking this being bad practice, and the two of us disagreeing. I'm certainly willing to adjust non-obvious code to a more obvious shape in various cases, but I think there needs to be a limit as to what language features we decide should not be used in the code base. Overriding declarations (and in some cases even definitions) by macros is a useful thing for general readability in certain cases in my opinion, and while it's not making much of difference here I'd still prefer if I was allowed to get away with this, unless a majority supports your view. IOW - your change request is, as per my own perspective, making the code less easy to read, even if not by much. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Hi Jan, On 31/05/2019 11:46, Jan Beulich wrote: >>>> On 31.05.19 at 12:34, <julien.grall@arm.com> wrote: >> No it was a more generic statement on the stance "It already exists in >> Xen so it is fine to spread them a bit more". > > Oh, I see. Of course I'm making remarks when what's in the tree is > bad (as per e.g. coding style, or if not mentioned there than in my > personal opinion). As a result I take note of you thinking this being > bad practice, and the two of us disagreeing. I'm certainly willing to > adjust non-obvious code to a more obvious shape in various cases, > but I think there needs to be a limit as to what language features > we decide should not be used in the code base. Overriding > declarations (and in some cases even definitions) by macros is a > useful thing for general readability in certain cases in my opinion, > and while it's not making much of difference here I'd still prefer if > I was allowed to get away with this, unless a majority supports > your view. IOW - your change request is, as per my own > perspective, making the code less easy to read, even if not by > much. Let will wait the opinion from the others here. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On Fri, 31 May 2019, Julien Grall wrote: > Hi Jan, > > On 31/05/2019 11:46, Jan Beulich wrote: > > > > > On 31.05.19 at 12:34, <julien.grall@arm.com> wrote: > > > No it was a more generic statement on the stance "It already exists in > > > Xen so it is fine to spread them a bit more". > > > > Oh, I see. Of course I'm making remarks when what's in the tree is > > bad (as per e.g. coding style, or if not mentioned there than in my > > personal opinion). As a result I take note of you thinking this being > > bad practice, and the two of us disagreeing. I'm certainly willing to > > adjust non-obvious code to a more obvious shape in various cases, > > but I think there needs to be a limit as to what language features > > we decide should not be used in the code base. Overriding > > declarations (and in some cases even definitions) by macros is a > > useful thing for general readability in certain cases in my opinion, > > and while it's not making much of difference here I'd still prefer if > > I was allowed to get away with this, unless a majority supports > > your view. IOW - your change request is, as per my own > > perspective, making the code less easy to read, even if not by > > much. > > Let will wait the opinion from the others here. My preference is what Andrew suggested: #ifdef CONFIG_HAS_MEM_SHARING extern struct domain *dom_cow; #else define dom_cow NULL #endif and I find Jan's original version harder to read. However, for code style related things, I prefer to "suggest" to other maintainers one way or the other, rather than "request" for a change. If something bother us enough to do something about it, the way to go is to send a patch to CODING_STYLE so that we are all on the same page. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Hi, On 5/31/19 6:27 PM, Stefano Stabellini wrote: > On Fri, 31 May 2019, Julien Grall wrote: >> Hi Jan, >> >> On 31/05/2019 11:46, Jan Beulich wrote: >>>>>> On 31.05.19 at 12:34, <julien.grall@arm.com> wrote: >>>> No it was a more generic statement on the stance "It already exists in >>>> Xen so it is fine to spread them a bit more". >>> >>> Oh, I see. Of course I'm making remarks when what's in the tree is >>> bad (as per e.g. coding style, or if not mentioned there than in my >>> personal opinion). As a result I take note of you thinking this being >>> bad practice, and the two of us disagreeing. I'm certainly willing to >>> adjust non-obvious code to a more obvious shape in various cases, >>> but I think there needs to be a limit as to what language features >>> we decide should not be used in the code base. Overriding >>> declarations (and in some cases even definitions) by macros is a >>> useful thing for general readability in certain cases in my opinion, >>> and while it's not making much of difference here I'd still prefer if >>> I was allowed to get away with this, unless a majority supports >>> your view. IOW - your change request is, as per my own >>> perspective, making the code less easy to read, even if not by >>> much. >> >> Let will wait the opinion from the others here. > > My preference is what Andrew suggested: > > #ifdef CONFIG_HAS_MEM_SHARING > extern struct domain *dom_cow; > #else > define dom_cow NULL > #endif > > and I find Jan's original version harder to read. However, for code > style related things, I prefer to "suggest" to other maintainers one way > or the other, rather than "request" for a change. Note that I wrote "I would prefer" in my e-mail and the agreement was to wait on other view. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On Fri, May 31, 2019 at 3:35 AM Jan Beulich <JBeulich@suse.com> wrote: > > A couple of adjustments are needed to code checking for dom_cow, but > since there are pretty few it is probably better to adjust those than > to set up and keep around a never used domain. > > Take the opportunity and tighten a BUG_ON() in emul-priv-op.c:read_cr(). > (Arguably this perhaps shouldn't be a BUG_ON() in the first place.) > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > While for now this avoids creating the domain on Arm only, Tamas'es > patch switching to CONFIG_MEM_SHARING will make x86 leverage this too. Hi Jan, perhaps it would be better to have this patch be applied after my patch? You already acked that one and it could be applied separately from the series that I've sent it as part of. Thanks, Tamas _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
>>> On 02.06.19 at 02:40, <tamas@tklengyel.com> wrote: > On Fri, May 31, 2019 at 3:35 AM Jan Beulich <JBeulich@suse.com> wrote: >> >> A couple of adjustments are needed to code checking for dom_cow, but >> since there are pretty few it is probably better to adjust those than >> to set up and keep around a never used domain. >> >> Take the opportunity and tighten a BUG_ON() in emul-priv-op.c:read_cr(). >> (Arguably this perhaps shouldn't be a BUG_ON() in the first place.) >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> While for now this avoids creating the domain on Arm only, Tamas'es >> patch switching to CONFIG_MEM_SHARING will make x86 leverage this too. > > perhaps it would be better to have this patch be applied after my > patch? You already acked that one and it could be applied separately > from the series that I've sent it as part of. Oh, I didn't realize it is entirely independent of the earlier patches. It would help to point such out in the cover letter, requiring there to be one in the first place. Yet even then - it's lacking an XSM maintainer ack afaics (and it looks as if you didn't even Cc Daniel), and I'd prefer a patch like this to also have an ack from George, even if judging by the files touched this may not be strictly required. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On Mon, Jun 3, 2019 at 2:25 AM Jan Beulich <JBeulich@suse.com> wrote: > > >>> On 02.06.19 at 02:40, <tamas@tklengyel.com> wrote: > > On Fri, May 31, 2019 at 3:35 AM Jan Beulich <JBeulich@suse.com> wrote: > >> > >> A couple of adjustments are needed to code checking for dom_cow, but > >> since there are pretty few it is probably better to adjust those than > >> to set up and keep around a never used domain. > >> > >> Take the opportunity and tighten a BUG_ON() in emul-priv-op.c:read_cr(). > >> (Arguably this perhaps shouldn't be a BUG_ON() in the first place.) > >> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >> --- > >> While for now this avoids creating the domain on Arm only, Tamas'es > >> patch switching to CONFIG_MEM_SHARING will make x86 leverage this too. > > > > perhaps it would be better to have this patch be applied after my > > patch? You already acked that one and it could be applied separately > > from the series that I've sent it as part of. > > Oh, I didn't realize it is entirely independent of the earlier patches. > It would help to point such out in the cover letter, requiring there > to be one in the first place. Right, I should have added a cover letter. That "series" is more like an assorted collection of fixes rather then a series in a strict sense. Tamas _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
>>> On 03.06.19 at 21:57, <tamas@tklengyel.com> wrote: > On Mon, Jun 3, 2019 at 2:25 AM Jan Beulich <JBeulich@suse.com> wrote: >> >> >>> On 02.06.19 at 02:40, <tamas@tklengyel.com> wrote: >> > On Fri, May 31, 2019 at 3:35 AM Jan Beulich <JBeulich@suse.com> wrote: >> >> >> >> A couple of adjustments are needed to code checking for dom_cow, but >> >> since there are pretty few it is probably better to adjust those than >> >> to set up and keep around a never used domain. >> >> >> >> Take the opportunity and tighten a BUG_ON() in emul-priv-op.c:read_cr(). >> >> (Arguably this perhaps shouldn't be a BUG_ON() in the first place.) >> >> >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> --- >> >> While for now this avoids creating the domain on Arm only, Tamas'es >> >> patch switching to CONFIG_MEM_SHARING will make x86 leverage this too. >> > >> > perhaps it would be better to have this patch be applied after my >> > patch? You already acked that one and it could be applied separately >> > from the series that I've sent it as part of. >> >> Oh, I didn't realize it is entirely independent of the earlier patches. >> It would help to point such out in the cover letter, requiring there >> to be one in the first place. > > Right, I should have added a cover letter. That "series" is more like > an assorted collection of fixes rather then a series in a strict > sense. In which case an option would have been to send four standalone patches. (I'm definitely no consistent with this myself - there are cases where I collect things into a series, but there are also cases where I send multiple individual patches.) Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On 31/05/2019 02:35, Jan Beulich wrote: > A couple of adjustments are needed to code checking for dom_cow, but > since there are pretty few it is probably better to adjust those than > to set up and keep around a never used domain. > > Take the opportunity and tighten a BUG_ON() in emul-priv-op.c:read_cr(). > (Arguably this perhaps shouldn't be a BUG_ON() in the first place.) Yes - it should be ASSERT_UNREACHABLE()/domain_crash() I'm not fussed if this done as part of this patch, or split out separately. It almost doesn't seem worth splitting out. > --- a/xen/include/xen/mm.h > +++ b/xen/include/xen/mm.h > @@ -644,6 +644,9 @@ static inline void filtered_flush_tlb_ma > > /* Private domain structs for DOMID_XEN, DOMID_IO, etc. */ > extern struct domain *dom_xen, *dom_io, *dom_cow; > +#ifndef CONFIG_HAS_MEM_SHARING > +# define dom_cow NULL > +#endif > > enum XENSHARE_flags { > SHARE_rw, > > > What is wrong with #ifdef CONFIG_HAS_MEM_SHARING extern struct domain *dom_cow; #else # define dom_cow NULL #endif which is how we usually express things like this? Sure, its a tiny bit longer to write, but it is easier to follow. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
>>> On 31.05.19 at 19:13, <andrew.cooper3@citrix.com> wrote: > On 31/05/2019 02:35, Jan Beulich wrote: >> A couple of adjustments are needed to code checking for dom_cow, but >> since there are pretty few it is probably better to adjust those than >> to set up and keep around a never used domain. >> >> Take the opportunity and tighten a BUG_ON() in emul-priv-op.c:read_cr(). >> (Arguably this perhaps shouldn't be a BUG_ON() in the first place.) > > Yes - it should be ASSERT_UNREACHABLE()/domain_crash() > > I'm not fussed if this done as part of this patch, or split out > separately. It almost doesn't seem worth splitting out. Well, to do both changes at the same time, I'll really split this out into a prereq patch. >> --- a/xen/include/xen/mm.h >> +++ b/xen/include/xen/mm.h >> @@ -644,6 +644,9 @@ static inline void filtered_flush_tlb_ma >> >> /* Private domain structs for DOMID_XEN, DOMID_IO, etc. */ >> extern struct domain *dom_xen, *dom_io, *dom_cow; >> +#ifndef CONFIG_HAS_MEM_SHARING >> +# define dom_cow NULL >> +#endif >> >> enum XENSHARE_flags { >> SHARE_rw, > > What is wrong with > > #ifdef CONFIG_HAS_MEM_SHARING > extern struct domain *dom_cow; > #else > # define dom_cow NULL > #endif > > which is how we usually express things like this? Sure, its a tiny bit > longer to write, but it is easier to follow. Well, since you're the second one to ask, I'll switch, despite not agreeing with this. Yet again some use of the C language that apparently needs to be listed in ./CODING_STYLE as unwanted / forbidden. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
© 2016 - 2024 Red Hat, Inc.