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

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

[Jan Beulich]

Patch 3 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 (patch 4). Patches 1 and 2 are preparatory.

1: x86/PV: tighten page table ownership check in emul-priv-op.c:read_cr()
2: PCI: move pdev_list field to common structure
3: adjust special domain creation (and call it earlier on x86)
4: 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 v2 1/4] x86/PV: tighten page table ownership check in emul-priv-op.c:read_cr()

[Jan Beulich]

Rather than checking that a page table is _not_ "owned" by the fake COW
domain, check that it's owned by the domain actually wanting to install
it.

Switch away from BUG_ON() at the same time.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Split out from larger patch to make further adjustments.
---
Thinking about it I wonder why we have such a check here and no-where
else. An alternative would seem to be to simply drop the BUG_ON().

--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -706,7 +706,7 @@ static int read_cr(unsigned int reg, uns
 
     case 3: /* Read CR3 */
     {
-        const struct domain *currd = curr->domain;
+        struct domain *currd = curr->domain;
         mfn_t mfn;
 
         if ( !is_pv_32bit_domain(currd) )
@@ -723,8 +723,14 @@ 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 */
+        if ( page_get_owner(mfn_to_page(mfn)) != currd )
+        {
+            ASSERT_UNREACHABLE();
+            domain_crash(currd);
+        }
+
         return X86EMUL_OKAY;
     }
     }





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 1/4] x86/PV: tighten page table ownership check in emul-priv-op.c:read_cr()

[Roger Pau Monné]

On Tue, Jun 04, 2019 at 06:41:29AM -0600, Jan Beulich wrote:
> Rather than checking that a page table is _not_ "owned" by the fake COW
> domain, check that it's owned by the domain actually wanting to install
> it.
> 
> Switch away from BUG_ON() at the same time.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> v2: Split out from larger patch to make further adjustments.
> ---
> Thinking about it I wonder why we have such a check here and no-where
> else. An alternative would seem to be to simply drop the BUG_ON().
> 
> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -706,7 +706,7 @@ static int read_cr(unsigned int reg, uns
>  
>      case 3: /* Read CR3 */
>      {
> -        const struct domain *currd = curr->domain;
> +        struct domain *currd = curr->domain;
>          mfn_t mfn;
>  
>          if ( !is_pv_32bit_domain(currd) )
> @@ -723,8 +723,14 @@ 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 */
> +        if ( page_get_owner(mfn_to_page(mfn)) != currd )
> +        {
> +            ASSERT_UNREACHABLE();
> +            domain_crash(currd);

I wonder whether you could keep currd as const and just use
curr->domain here.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 2/4] PCI: move pdev_list field to common structure

[Jan Beulich]

Its management shouldn't be arch-specific, and in particular there
should be no need for special precautions when creating the special
domains.

At this occasion
- correct parenthesization of for_each_pdev(),
- stop open-coding for_each_pdev() in vPCI code.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -496,7 +496,6 @@ int arch_domain_create(struct domain *d,
     uint32_t emflags;
     int rc;
 
-    INIT_LIST_HEAD(&d->arch.pdev_list);
     INIT_PAGE_LIST_HEAD(&d->arch.relmem_list);
 
     spin_lock_init(&d->arch.e820_lock);
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -291,7 +291,6 @@ void __init arch_init_memory(void)
      */
     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.
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -389,6 +389,10 @@ struct domain *domain_create(domid_t dom
 
     rwlock_init(&d->vnuma_rwlock);
 
+#ifdef CONFIG_HAS_PCI
+    INIT_LIST_HEAD(&d->pdev_list);
+#endif
+
     err = -ENOMEM;
     if ( !zalloc_cpumask_var(&d->dirty_cpumask) )
         goto fail;
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -315,7 +315,7 @@ static int reassign_device(struct domain
 
     if ( devfn == pdev->devfn )
     {
-        list_move(&pdev->domain_list, &target->arch.pdev_list);
+        list_move(&pdev->domain_list, &target->pdev_list);
         pdev->domain = target;
     }
 
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -467,7 +467,7 @@ static void _pci_hide_device(struct pci_
     if ( pdev->domain )
         return;
     pdev->domain = dom_xen;
-    list_add(&pdev->domain_list, &dom_xen->arch.pdev_list);
+    list_add(&pdev->domain_list, &dom_xen->pdev_list);
 }
 
 int __init pci_hide_device(unsigned int seg, unsigned int bus,
@@ -803,7 +803,7 @@ int pci_add_device(u16 seg, u8 bus, u8 d
             goto out;
         }
 
-        list_add(&pdev->domain_list, &hardware_domain->arch.pdev_list);
+        list_add(&pdev->domain_list, &hardware_domain->pdev_list);
     }
     else
         iommu_enable_device(pdev);
@@ -1153,7 +1153,7 @@ static int __hwdom_init _setup_hwdom_pci
             if ( !pdev->domain )
             {
                 pdev->domain = ctxt->d;
-                list_add(&pdev->domain_list, &ctxt->d->arch.pdev_list);
+                list_add(&pdev->domain_list, &ctxt->d->pdev_list);
                 setup_one_hwdom_device(ctxt, pdev);
             }
             else if ( pdev->domain == dom_xen )
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2453,7 +2453,7 @@ static int reassign_device_ownership(
 
     if ( devfn == pdev->devfn )
     {
-        list_move(&pdev->domain_list, &target->arch.pdev_list);
+        list_move(&pdev->domain_list, &target->pdev_list);
         pdev->domain = target;
     }
 
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -267,7 +267,7 @@ static int modify_bars(const struct pci_
      * Check for overlaps with other BARs. Note that only BARs that are
      * currently mapped (enabled) are checked for overlaps.
      */
-    list_for_each_entry(tmp, &pdev->domain->arch.pdev_list, domain_list)
+    for_each_pdev ( pdev->domain, tmp )
     {
         if ( tmp == pdev )
         {
--- a/xen/drivers/vpci/msi.c
+++ b/xen/drivers/vpci/msi.c
@@ -282,7 +282,7 @@ void vpci_dump_msi(void)
 
         printk("vPCI MSI/MSI-X d%d\n", d->domain_id);
 
-        list_for_each_entry ( pdev, &d->arch.pdev_list, domain_list )
+        for_each_pdev ( d, pdev )
         {
             const struct vpci_msi *msi;
             const struct vpci_msix *msix;
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -298,8 +298,6 @@ struct arch_domain
 
     bool_t s3_integrity;
 
-    struct list_head pdev_list;
-
     union {
         struct pv_domain pv;
         struct hvm_domain hvm;
@@ -476,8 +474,6 @@ struct arch_domain
 #define has_pirq(d)        (!!((d)->arch.emulation_flags & X86_EMU_USE_PIRQ))
 #define has_vpci(d)        (!!((d)->arch.emulation_flags & X86_EMU_VPCI))
 
-#define has_arch_pdevs(d)    (!list_empty(&(d)->arch.pdev_list))
-
 #define gdt_ldt_pt_idx(v) \
       ((v)->vcpu_id >> (PAGETABLE_ORDER - GDT_LDT_VCPU_SHIFT))
 #define pv_gdt_ptes(v) \
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -121,7 +121,9 @@ struct pci_dev {
 };
 
 #define for_each_pdev(domain, pdev) \
-    list_for_each_entry(pdev, &(domain->arch.pdev_list), domain_list)
+    list_for_each_entry(pdev, &(domain)->pdev_list, domain_list)
+
+#define has_arch_pdevs(d) (!list_empty(&(d)->pdev_list))
 
 /*
  * The pcidevs_lock protect alldevs_list, and the assignment for the 
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -370,6 +370,10 @@ struct domain
 
     int64_t          time_offset_seconds;
 
+#ifdef CONFIG_HAS_PCI
+    struct list_head pdev_list;
+#endif
+
 #ifdef CONFIG_HAS_PASSTHROUGH
     struct domain_iommu iommu;
 #endif




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/4] PCI: move pdev_list field to common structure

[Julien Grall]

Hi,

On 6/4/19 1:42 PM, Jan Beulich wrote:
> Its management shouldn't be arch-specific, and in particular there
> should be no need for special precautions when creating the special
> domains.
> 
> At this occasion
> - correct parenthesization of for_each_pdev(),
> - stop open-coding for_each_pdev() in vPCI code.

 From an Arm POV, this makes sense. With one comment below.

> @@ -476,8 +474,6 @@ struct arch_domain
>   #define has_pirq(d)        (!!((d)->arch.emulation_flags & X86_EMU_USE_PIRQ))
>   #define has_vpci(d)        (!!((d)->arch.emulation_flags & X86_EMU_VPCI))
>   
> -#define has_arch_pdevs(d)    (!list_empty(&(d)->arch.pdev_list))
> -
>   #define gdt_ldt_pt_idx(v) \
>         ((v)->vcpu_id >> (PAGETABLE_ORDER - GDT_LDT_VCPU_SHIFT))
>   #define pv_gdt_ptes(v) \
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -121,7 +121,9 @@ struct pci_dev {
>   };
>   
>   #define for_each_pdev(domain, pdev) \
> -    list_for_each_entry(pdev, &(domain->arch.pdev_list), domain_list)
> +    list_for_each_entry(pdev, &(domain)->pdev_list, domain_list)
> +
> +#define has_arch_pdevs(d) (!list_empty(&(d)->pdev_list))

This feels a bit strange to keep "arch" in the macro name when the code 
is now generic. How about "domain_has_pdevs"?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/4] PCI: move pdev_list field to common structure

[Jan Beulich]

>>> On 04.06.19 at 14:55, <julien.grall@arm.com> wrote:
> On 6/4/19 1:42 PM, Jan Beulich wrote:
>> --- a/xen/include/xen/pci.h
>> +++ b/xen/include/xen/pci.h
>> @@ -121,7 +121,9 @@ struct pci_dev {
>>   };
>>   
>>   #define for_each_pdev(domain, pdev) \
>> -    list_for_each_entry(pdev, &(domain->arch.pdev_list), domain_list)
>> +    list_for_each_entry(pdev, &(domain)->pdev_list, domain_list)
>> +
>> +#define has_arch_pdevs(d) (!list_empty(&(d)->pdev_list))
> 
> This feels a bit strange to keep "arch" in the macro name when the code 
> is now generic. How about "domain_has_pdevs"?

Indeed I did notice this oddity too, but if such an adjustment is
to be made then imo not in this patch. After all in turn I'd need
to change all use sites.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/4] PCI: move pdev_list field to common structure

[Julien Grall]

Hi Jan,

On 6/4/19 2:03 PM, Jan Beulich wrote:
>>>> On 04.06.19 at 14:55, <julien.grall@arm.com> wrote:
>> On 6/4/19 1:42 PM, Jan Beulich wrote:
>>> --- a/xen/include/xen/pci.h
>>> +++ b/xen/include/xen/pci.h
>>> @@ -121,7 +121,9 @@ struct pci_dev {
>>>    };
>>>    
>>>    #define for_each_pdev(domain, pdev) \
>>> -    list_for_each_entry(pdev, &(domain->arch.pdev_list), domain_list)
>>> +    list_for_each_entry(pdev, &(domain)->pdev_list, domain_list)
>>> +
>>> +#define has_arch_pdevs(d) (!list_empty(&(d)->pdev_list))
>>
>> This feels a bit strange to keep "arch" in the macro name when the code
>> is now generic. How about "domain_has_pdevs"?
> 
> Indeed I did notice this oddity too, but if such an adjustment is
> to be made then imo not in this patch. After all in turn I'd need
> to change all use sites.

It feels to me they are kind of tied together because has_arch_pdevs is 
an accessor of the field.

But I am happy to see this in a separate patches.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/4] PCI: move pdev_list field to common structure

[Jan Beulich]

>>> On 04.06.19 at 15:05, <julien.grall@arm.com> wrote:
> Hi Jan,
> 
> On 6/4/19 2:03 PM, Jan Beulich wrote:
>>>>> On 04.06.19 at 14:55, <julien.grall@arm.com> wrote:
>>> On 6/4/19 1:42 PM, Jan Beulich wrote:
>>>> --- a/xen/include/xen/pci.h
>>>> +++ b/xen/include/xen/pci.h
>>>> @@ -121,7 +121,9 @@ struct pci_dev {
>>>>    };
>>>>    
>>>>    #define for_each_pdev(domain, pdev) \
>>>> -    list_for_each_entry(pdev, &(domain->arch.pdev_list), domain_list)
>>>> +    list_for_each_entry(pdev, &(domain)->pdev_list, domain_list)
>>>> +
>>>> +#define has_arch_pdevs(d) (!list_empty(&(d)->pdev_list))
>>>
>>> This feels a bit strange to keep "arch" in the macro name when the code
>>> is now generic. How about "domain_has_pdevs"?
>> 
>> Indeed I did notice this oddity too, but if such an adjustment is
>> to be made then imo not in this patch. After all in turn I'd need
>> to change all use sites.
> 
> It feels to me they are kind of tied together because has_arch_pdevs is 
> an accessor of the field.

In a way they are. But the name of the macro hasn't become
"wrong" by the change here. That renaming you ask for could
also have been done a year ago, if so wanted.

> But I am happy to see this in a separate patches.

FAOD - I didn't promise anything.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/4] PCI: move pdev_list field to common structure

[Julien Grall]

On 6/4/19 2:14 PM, Jan Beulich wrote:
>>>> On 04.06.19 at 15:05, <julien.grall@arm.com> wrote:
>> Hi Jan,
>>
>> On 6/4/19 2:03 PM, Jan Beulich wrote:
>>>>>> On 04.06.19 at 14:55, <julien.grall@arm.com> wrote:
>>>> On 6/4/19 1:42 PM, Jan Beulich wrote:
>>>>> --- a/xen/include/xen/pci.h
>>>>> +++ b/xen/include/xen/pci.h
>>>>> @@ -121,7 +121,9 @@ struct pci_dev {
>>>>>     };
>>>>>     
>>>>>     #define for_each_pdev(domain, pdev) \
>>>>> -    list_for_each_entry(pdev, &(domain->arch.pdev_list), domain_list)
>>>>> +    list_for_each_entry(pdev, &(domain)->pdev_list, domain_list)
>>>>> +
>>>>> +#define has_arch_pdevs(d) (!list_empty(&(d)->pdev_list))
>>>>
>>>> This feels a bit strange to keep "arch" in the macro name when the code
>>>> is now generic. How about "domain_has_pdevs"?
>>>
>>> Indeed I did notice this oddity too, but if such an adjustment is
>>> to be made then imo not in this patch. After all in turn I'd need
>>> to change all use sites.
>>
>> It feels to me they are kind of tied together because has_arch_pdevs is
>> an accessor of the field.
> 
> In a way they are. But the name of the macro hasn't become
> "wrong" by the change here. That renaming you ask for could
> also have been done a year ago, if so wanted.

Fair enough for non-x86 code:

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/4] PCI: move pdev_list field to common structure

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 1272 bytes --]

On 04/06/2019 13:55, Julien Grall wrote:
>> @@ -476,8 +474,6 @@ struct arch_domain
>>   #define has_pirq(d)        (!!((d)->arch.emulation_flags &
>> X86_EMU_USE_PIRQ))
>>   #define has_vpci(d)        (!!((d)->arch.emulation_flags &
>> X86_EMU_VPCI))
>>   -#define has_arch_pdevs(d)    (!list_empty(&(d)->arch.pdev_list))
>> -
>>   #define gdt_ldt_pt_idx(v) \
>>         ((v)->vcpu_id >> (PAGETABLE_ORDER - GDT_LDT_VCPU_SHIFT))
>>   #define pv_gdt_ptes(v) \
>> --- a/xen/include/xen/pci.h
>> +++ b/xen/include/xen/pci.h
>> @@ -121,7 +121,9 @@ struct pci_dev {
>>   };
>>     #define for_each_pdev(domain, pdev) \
>> -    list_for_each_entry(pdev, &(domain->arch.pdev_list), domain_list)
>> +    list_for_each_entry(pdev, &(domain)->pdev_list, domain_list)
>> +
>> +#define has_arch_pdevs(d) (!list_empty(&(d)->pdev_list))
>
> This feels a bit strange to keep "arch" in the macro name when the
> code is now generic. How about "domain_has_pdevs"?

I agree that keeping arch in the name is a little weird, and this is
definitely the patch to rename it in - there are only 9 instances in the
entire codebase.

For the patch overall, Acked-by: Andrew Cooper
<andrew.cooper3@citrix.com>, however this helper ends up being named.

~Andrew


[-- Attachment #1.2: Type: text/html, Size: 2161 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 3/4] adjust special domain creation (and call it earlier on x86)

[Jan Beulich]

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.

Take the opportunity and
- convert BUG_ON()s being moved to panic(),
- add __read_mostly annotations to the dom_* definitions.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Re-base over pdev_list movement patch. Replace BUG_ON(). Drop stray
blank line insertion.

--- 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,31 +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));
-
-    /*
-     * 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;
@@ -520,6 +525,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);
+    if ( IS_ERR(dom_xen) )
+        panic("Failed to create d[XEN]: %ld\n", PTR_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);
+    if ( IS_ERR(dom_io) )
+        panic("Failed to create d[IO]: %ld\n", PTR_ERR(dom_io));
+
+    /*
+     * Initialise our COW domain.
+     * This domain owns sharable pages.
+     */
+    dom_cow = domain_create(DOMID_COW, NULL, false);
+    if ( IS_ERR(dom_cow) )
+        panic("Failed to create d[COW]: %ld\n", PTR_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
@@ -22,6 +22,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 v2 3/4] adjust special domain creation (and call it earlier on x86)

[Andrew Cooper]

On 04/06/2019 13:43, Jan Beulich wrote:
> 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+...]

What is this in the stack trace?

>     [<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.
>
> Take the opportunity and
> - convert BUG_ON()s being moved to panic(),
> - add __read_mostly annotations to the dom_* definitions.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Sorry for not noticing this before, but s/special/system/ to match up
with the existing terminology in is_system_domain()

> --- 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;
> +

Any chance you can move these higher up, to before the include of
<asm/mm.h>, or you're going to break Julien's M2P series.

With at least the name adjusted, Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 3/4] adjust special domain creation (and call it earlier on x86)

[Jan Beulich]

>>> On 04.06.19 at 15:35, <andrew.cooper3@citrix.com> wrote:
> On 04/06/2019 13:43, Jan Beulich wrote:
>> 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+...]
> 
> What is this in the stack trace?

The entry missing here, to make the whole thing sensible. See
the other patch I did send ("x86/traps: widen condition for
logging top-of-stack").

>>     [<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.
>>
>> Take the opportunity and
>> - convert BUG_ON()s being moved to panic(),
>> - add __read_mostly annotations to the dom_* definitions.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Sorry for not noticing this before, but s/special/system/ to match up
> with the existing terminology in is_system_domain()

Easily done.

>> --- 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;
>> +
> 
> Any chance you can move these higher up, to before the include of
> <asm/mm.h>, or you're going to break Julien's M2P series.

Hmm, I could, albeit I did intentionally place them there. In fact I
had them elsewhere first, until the build broke because of the use
of dom_xen in share_xen_page_with_privileged_guests(). That's
what made me decide to put it where it now is (and I'd prefer to
keep it there for now).

> With at least the name adjusted, Reviewed-by: Andrew Cooper
> <andrew.cooper3@citrix.com>

Thanks.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 3/4] adjust special domain creation (and call it earlier on x86)

[Julien Grall]

Hi Jan,

On 04/06/2019 13:43, Jan Beulich wrote:
> 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.
> 
> Take the opportunity and
> - convert BUG_ON()s being moved to panic(),
> - add __read_mostly annotations to the dom_* definitions.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

For Arm:

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 4/4] dom_cow is needed for mem-sharing only

[Jan Beulich]

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.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Use #if/#else. Split out emul-priv-op.c change.
---
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/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;
 
@@ -547,6 +549,7 @@ void __init setup_special_domains(void)
     if ( IS_ERR(dom_io) )
         panic("Failed to create d[IO]: %ld\n", PTR_ERR(dom_io));
 
+#ifdef CONFIG_HAS_MEM_SHARING
     /*
      * Initialise our COW domain.
      * This domain owns sharable pages.
@@ -554,6 +557,7 @@ void __init setup_special_domains(void)
     dom_cow = domain_create(DOMID_COW, NULL, false);
     if ( IS_ERR(dom_cow) )
         panic("Failed to create d[COW]: %ld\n", PTR_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
@@ -643,7 +643,12 @@ 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;
+extern struct domain *dom_xen, *dom_io;
+#ifdef CONFIG_HAS_MEM_SHARING
+extern struct domain *dom_cow;
+#else
+# 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 v2 4/4] dom_cow is needed for mem-sharing only

[Andrew Cooper]

On 04/06/2019 13:44, 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.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 4/4] dom_cow is needed for mem-sharing only

[Julien Grall]

Hi Jan,

On 04/06/2019 13:44, 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.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

> ---
> v2: Use #if/#else. Split out emul-priv-op.c change.
> ---
> 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/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;
>   
> @@ -547,6 +549,7 @@ void __init setup_special_domains(void)
>       if ( IS_ERR(dom_io) )
>           panic("Failed to create d[IO]: %ld\n", PTR_ERR(dom_io));
>   
> +#ifdef CONFIG_HAS_MEM_SHARING
>       /*
>        * Initialise our COW domain.
>        * This domain owns sharable pages.
> @@ -554,6 +557,7 @@ void __init setup_special_domains(void)
>       dom_cow = domain_create(DOMID_COW, NULL, false);
>       if ( IS_ERR(dom_cow) )
>           panic("Failed to create d[COW]: %ld\n", PTR_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
> @@ -643,7 +643,12 @@ 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;
> +extern struct domain *dom_xen, *dom_io;
> +#ifdef CONFIG_HAS_MEM_SHARING
> +extern struct domain *dom_cow;
> +#else
> +# 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