[Xen-devel] [PATCH 0/2] adjust special domain creation

Jan Beulich posted 2 patches 4 years, 10 months ago
Only 0 patches received!
There is a newer version of this series
[Xen-devel] [PATCH 0/2] adjust special domain creation
Posted by Jan Beulich 4 years, 10 months ago
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
[Xen-devel] [PATCH 1/2] adjust special domain creation (and call it earlier on x86)
Posted by Jan Beulich 4 years, 10 months ago
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
Re: [Xen-devel] [PATCH 1/2] adjust special domain creation (and call it earlier on x86)
Posted by Julien Grall 4 years, 10 months ago
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
Re: [Xen-devel] [PATCH 1/2] adjust special domain creation (and call it earlier on x86)
Posted by Jan Beulich 4 years, 10 months ago
>>> 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
Re: [Xen-devel] [PATCH 1/2] adjust special domain creation (and call it earlier on x86)
Posted by Andrew Cooper 4 years, 10 months ago
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
Re: [Xen-devel] [PATCH 1/2] adjust special domain creation (and call it earlier on x86)
Posted by Jan Beulich 4 years, 10 months ago
>>> 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
Re: [Xen-devel] [PATCH 1/2] adjust special domain creation (and call it earlier on x86)
Posted by Andrew Cooper 4 years, 10 months ago
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
Re: [Xen-devel] [PATCH 1/2] adjust special domain creation (and call it earlier on x86)
Posted by Jan Beulich 4 years, 9 months ago
>>> 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
[Xen-devel] [PATCH 2/2] dom_cow is needed for mem-sharing only
Posted by Jan Beulich 4 years, 10 months ago
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
Re: [Xen-devel] [PATCH 2/2] dom_cow is needed for mem-sharing only
Posted by Julien Grall 4 years, 10 months ago
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
Re: [Xen-devel] [PATCH 2/2] dom_cow is needed for mem-sharing only
Posted by Jan Beulich 4 years, 10 months ago
>>> 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
Re: [Xen-devel] [PATCH 2/2] dom_cow is needed for mem-sharing only
Posted by Julien Grall 4 years, 10 months ago
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
Re: [Xen-devel] [PATCH 2/2] dom_cow is needed for mem-sharing only
Posted by Jan Beulich 4 years, 10 months ago
>>> 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
Re: [Xen-devel] [PATCH 2/2] dom_cow is needed for mem-sharing only
Posted by Julien Grall 4 years, 10 months ago
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
Re: [Xen-devel] [PATCH 2/2] dom_cow is needed for mem-sharing only
Posted by Jan Beulich 4 years, 10 months ago
>>> 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
Re: [Xen-devel] [PATCH 2/2] dom_cow is needed for mem-sharing only
Posted by Julien Grall 4 years, 10 months ago
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
Re: [Xen-devel] [PATCH 2/2] dom_cow is needed for mem-sharing only
Posted by Jan Beulich 4 years, 10 months ago
>>> 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
Re: [Xen-devel] [PATCH 2/2] dom_cow is needed for mem-sharing only
Posted by Julien Grall 4 years, 10 months ago
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
Re: [Xen-devel] [PATCH 2/2] dom_cow is needed for mem-sharing only
Posted by Stefano Stabellini 4 years, 10 months ago
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
Re: [Xen-devel] [PATCH 2/2] dom_cow is needed for mem-sharing only
Posted by Julien Grall 4 years, 10 months ago
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
Re: [Xen-devel] [PATCH 2/2] dom_cow is needed for mem-sharing only
Posted by Tamas K Lengyel 4 years, 10 months ago
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
Re: [Xen-devel] [PATCH 2/2] dom_cow is needed for mem-sharing only
Posted by Jan Beulich 4 years, 9 months ago
>>> 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
Re: [Xen-devel] [PATCH 2/2] dom_cow is needed for mem-sharing only
Posted by Tamas K Lengyel 4 years, 9 months ago
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
Re: [Xen-devel] [PATCH 2/2] dom_cow is needed for mem-sharing only
Posted by Jan Beulich 4 years, 9 months ago
>>> 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
Re: [Xen-devel] [PATCH 2/2] dom_cow is needed for mem-sharing only
Posted by Andrew Cooper 4 years, 10 months ago
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
Re: [Xen-devel] [PATCH 2/2] dom_cow is needed for mem-sharing only
Posted by Jan Beulich 4 years, 9 months ago
>>> 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