[Xen-devel] [PATCH 0/2] x86/setup: Dom0 creation cleanups

[Xen-devel] [PATCH 0/2] x86/setup: Dom0 creation cleanups

[David Woodhouse]

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

As a precursor to live update support, clean up the dom0 creation in
start_xen() a little bit.

This makes it easier for later live update patches to make it
conditional, since in live update we'll already have an existing dom0
being brought over from the previous Xen. But it's a cleanup in its own
right in the meantime.

David Woodhouse (2):
      x86/setup: simplify handling of initrdidx when no initrd present
      x86/setup: lift dom0 creation out into create_dom0() function


xen/arch/x86/setup.c | 175 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 95 insertions(+), 80 deletions(-)

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 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 1/2] x86/setup: simplify handling of initrdidx when no initrd present

[David Woodhouse]

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

From: David Woodhouse <dwmw@amazon.co.uk>

Remove a ternary operator that made my brain hurt.

Replace it with something simpler that makes it somewhat clearer that
the check for initrdidx < mbi->mods_count is because larger values are
what find_first_bit() will return when it doesn't find anything.

Also drop the explicit check for module #0 since that would be the
dom0 kernel and the corresponding bit is always clear in module_map.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Acked-by: Julien Grall <julien@xen.org>
---
 xen/arch/x86/setup.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index c87040c890..2986cf5a3a 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -688,7 +688,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
     char *cmdline, *kextra, *loader;
     unsigned int initrdidx, num_parked = 0;
     multiboot_info_t *mbi;
-    module_t *mod;
+    module_t *mod, *initrd = NULL;
     unsigned long nr_pages, raw_max_page, modules_headroom, module_map[1];
     int i, j, e820_warn = 0, bytes = 0;
     bool acpi_boot_table_init_done = false, relocated = false;
@@ -1798,6 +1798,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
         xen_processor_pmbits |= XEN_PROCESSOR_PM_CX;
 
     initrdidx = find_first_bit(module_map, mbi->mods_count);
+    if ( initrdidx < mbi->mods_count )
+        initrd = mod + initrdidx;
     if ( bitmap_weight(module_map, mbi->mods_count) > 1 )
         printk(XENLOG_WARNING
                "Multiple initrd candidates, picking module #%u\n",
@@ -1822,9 +1824,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
      * We're going to setup domain0 using the module(s) that we stashed safely
      * above our heap. The second module, if present, is an initrd ramdisk.
      */
-    if ( construct_dom0(dom0, mod, modules_headroom,
-                        (initrdidx > 0) && (initrdidx < mbi->mods_count)
-                        ? mod + initrdidx : NULL, cmdline) != 0)
+    if ( construct_dom0(dom0, mod, modules_headroom, initrd, cmdline) != 0 )
         panic("Could not set up DOM0 guest OS\n");
 
     if ( cpu_has_smap )


[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 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 2/2] x86/setup: lift dom0 creation out into create_dom0() function

[David Woodhouse]

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

From: David Woodhouse <dwmw@amazon.co.uk>

The creation of dom0 can be relatively self-contained. Shift it into
a separate function and simplify __start_xen() a little bit.

This is a cleanup in its own right, but will be even more desireable
when live update provides an alternative path through __start_xen()
that doesn't involve creating a new dom0 at all.

Move the calculation of the 'initrd' parameter for create_dom0()
down past the cosmetic printk about NX support, because in the fullness
of time the whole initrd and create_dom0() part will be under the same
"not live update" conditional. And in the meantime it's just neater.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 xen/arch/x86/setup.c | 169 +++++++++++++++++++++++--------------------
 1 file changed, 92 insertions(+), 77 deletions(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 2986cf5a3a..72724ffe6f 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -679,6 +679,92 @@ static unsigned int __init copy_bios_e820(struct e820entry *map, unsigned int li
     return n;
 }
 
+static struct domain * __init create_dom0(const module_t *image,
+                                          unsigned long headroom,
+                                          module_t *initrd, const char *kextra,
+                                          char *loader)
+{
+    struct xen_domctl_createdomain dom0_cfg = {
+        .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0,
+        .max_evtchn_port = -1,
+        .max_grant_frames = -1,
+        .max_maptrack_frames = -1,
+        .max_vcpus = dom0_max_vcpus(),
+    };
+    struct domain *d;
+    char *cmdline;
+
+    if ( opt_dom0_pvh )
+    {
+        dom0_cfg.flags |= (XEN_DOMCTL_CDF_hvm |
+                           ((hvm_hap_supported() && !opt_dom0_shadow) ?
+                            XEN_DOMCTL_CDF_hap : 0));
+
+        dom0_cfg.arch.emulation_flags |=
+            XEN_X86_EMU_LAPIC | XEN_X86_EMU_IOAPIC | XEN_X86_EMU_VPCI;
+    }
+
+    if ( iommu_enabled )
+        dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
+
+    /* Create initial domain 0. */
+    d = domain_create(get_initial_domain_id(), &dom0_cfg, !pv_shim);
+    if ( IS_ERR(d) || (alloc_dom0_vcpu0(d) == NULL) )
+        panic("Error creating domain 0\n");
+
+    /* Grab the DOM0 command line. */
+    cmdline = image->string ? __va(image->string) : NULL;
+    if ( cmdline || kextra )
+    {
+        static char __initdata dom0_cmdline[MAX_GUEST_CMDLINE];
+
+        cmdline = cmdline_cook(cmdline, loader);
+        safe_strcpy(dom0_cmdline, cmdline);
+
+        if ( kextra )
+            /* kextra always includes exactly one leading space. */
+            safe_strcat(dom0_cmdline, kextra);
+
+        /* Append any extra parameters. */
+        if ( skip_ioapic_setup && !strstr(dom0_cmdline, "noapic") )
+            safe_strcat(dom0_cmdline, " noapic");
+        if ( (strlen(acpi_param) == 0) && acpi_disabled )
+        {
+            printk("ACPI is disabled, notifying Domain 0 (acpi=off)\n");
+            safe_strcpy(acpi_param, "off");
+        }
+        if ( (strlen(acpi_param) != 0) && !strstr(dom0_cmdline, "acpi=") )
+        {
+            safe_strcat(dom0_cmdline, " acpi=");
+            safe_strcat(dom0_cmdline, acpi_param);
+        }
+
+        cmdline = dom0_cmdline;
+    }
+
+    /*
+     * Temporarily clear SMAP in CR4 to allow user-accesses in construct_dom0().
+     * This saves a large number of corner cases interactions with
+     * copy_from_user().
+     */
+    if ( cpu_has_smap )
+    {
+        cr4_pv32_mask &= ~X86_CR4_SMAP;
+        write_cr4(read_cr4() & ~X86_CR4_SMAP);
+    }
+
+    if ( construct_dom0(d, image, headroom, initrd, cmdline) != 0 )
+        panic("Could not construct domain 0\n");
+
+    if ( cpu_has_smap )
+    {
+        write_cr4(read_cr4() | X86_CR4_SMAP);
+        cr4_pv32_mask |= X86_CR4_SMAP;
+    }
+
+    return d;
+}
+
 /* How much of the directmap is prebuilt at compile time. */
 #define PREBUILT_MAP_LIMIT (1 << L2_PAGETABLE_SHIFT)
 
@@ -698,12 +784,6 @@ void __init noreturn __start_xen(unsigned long mbi_p)
         .parity    = 'n',
         .stop_bits = 1
     };
-    struct xen_domctl_createdomain dom0_cfg = {
-        .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0,
-        .max_evtchn_port = -1,
-        .max_grant_frames = -1,
-        .max_maptrack_frames = -1,
-    };
     const char *hypervisor_name;
 
     /* Critical region without IDT or TSS.  Any fault is deadly! */
@@ -1745,58 +1825,13 @@ void __init noreturn __start_xen(unsigned long mbi_p)
     init_guest_cpuid();
     init_guest_msr_policy();
 
-    if ( opt_dom0_pvh )
-    {
-        dom0_cfg.flags |= (XEN_DOMCTL_CDF_hvm |
-                           ((hvm_hap_supported() && !opt_dom0_shadow) ?
-                            XEN_DOMCTL_CDF_hap : 0));
-
-        dom0_cfg.arch.emulation_flags |=
-            XEN_X86_EMU_LAPIC | XEN_X86_EMU_IOAPIC | XEN_X86_EMU_VPCI;
-    }
-    dom0_cfg.max_vcpus = dom0_max_vcpus();
-
-    if ( iommu_enabled )
-        dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
-
-    /* Create initial domain 0. */
-    dom0 = domain_create(get_initial_domain_id(), &dom0_cfg, !pv_shim);
-    if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
-        panic("Error creating domain 0\n");
-
-    /* Grab the DOM0 command line. */
-    cmdline = (char *)(mod[0].string ? __va(mod[0].string) : NULL);
-    if ( (cmdline != NULL) || (kextra != NULL) )
-    {
-        static char __initdata dom0_cmdline[MAX_GUEST_CMDLINE];
-
-        cmdline = cmdline_cook(cmdline, loader);
-        safe_strcpy(dom0_cmdline, cmdline);
-
-        if ( kextra != NULL )
-            /* kextra always includes exactly one leading space. */
-            safe_strcat(dom0_cmdline, kextra);
-
-        /* Append any extra parameters. */
-        if ( skip_ioapic_setup && !strstr(dom0_cmdline, "noapic") )
-            safe_strcat(dom0_cmdline, " noapic");
-        if ( (strlen(acpi_param) == 0) && acpi_disabled )
-        {
-            printk("ACPI is disabled, notifying Domain 0 (acpi=off)\n");
-            safe_strcpy(acpi_param, "off");
-        }
-        if ( (strlen(acpi_param) != 0) && !strstr(dom0_cmdline, "acpi=") )
-        {
-            safe_strcat(dom0_cmdline, " acpi=");
-            safe_strcat(dom0_cmdline, acpi_param);
-        }
-
-        cmdline = dom0_cmdline;
-    }
-
     if ( xen_cpuidle )
         xen_processor_pmbits |= XEN_PROCESSOR_PM_CX;
 
+    printk("%sNX (Execute Disable) protection %sactive\n",
+           cpu_has_nx ? XENLOG_INFO : XENLOG_WARNING "Warning: ",
+           cpu_has_nx ? "" : "not ");
+
     initrdidx = find_first_bit(module_map, mbi->mods_count);
     if ( initrdidx < mbi->mods_count )
         initrd = mod + initrdidx;
@@ -1805,34 +1840,14 @@ void __init noreturn __start_xen(unsigned long mbi_p)
                "Multiple initrd candidates, picking module #%u\n",
                initrdidx);
 
-    /*
-     * Temporarily clear SMAP in CR4 to allow user-accesses in construct_dom0().
-     * This saves a large number of corner cases interactions with
-     * copy_from_user().
-     */
-    if ( cpu_has_smap )
-    {
-        cr4_pv32_mask &= ~X86_CR4_SMAP;
-        write_cr4(read_cr4() & ~X86_CR4_SMAP);
-    }
-
-    printk("%sNX (Execute Disable) protection %sactive\n",
-           cpu_has_nx ? XENLOG_INFO : XENLOG_WARNING "Warning: ",
-           cpu_has_nx ? "" : "not ");
-
     /*
      * We're going to setup domain0 using the module(s) that we stashed safely
      * above our heap. The second module, if present, is an initrd ramdisk.
      */
-    if ( construct_dom0(dom0, mod, modules_headroom, initrd, cmdline) != 0 )
+    dom0 = create_dom0(mod, modules_headroom, initrd, kextra, loader);
+    if ( dom0 == NULL )
         panic("Could not set up DOM0 guest OS\n");
 
-    if ( cpu_has_smap )
-    {
-        write_cr4(read_cr4() | X86_CR4_SMAP);
-        cr4_pv32_mask |= X86_CR4_SMAP;
-    }
-
     heap_init_late();
 
     init_trace_bufs();


[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 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

Re: [Xen-devel] [PATCH 1/2] x86/setup: simplify handling of initrdidx when no initrd present

[Jan Beulich]

On 18.03.2020 12:46, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> Remove a ternary operator that made my brain hurt.

My position towards this hasn't changed, just ftr.

> Replace it with something simpler that makes it somewhat clearer that
> the check for initrdidx < mbi->mods_count is because larger values are
> what find_first_bit() will return when it doesn't find anything.
> 
> Also drop the explicit check for module #0 since that would be the
> dom0 kernel and the corresponding bit is always clear in module_map.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> Acked-by: Julien Grall <julien@xen.org>

Strictly speaking this is not a valid tag here, only R-b would be.

Jan

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

Re: [Xen-devel] [PATCH 1/2] x86/setup: simplify handling of initrdidx when no initrd present

[Julien Grall]

Hi,

On 18/03/2020 11:51, Jan Beulich wrote:
> On 18.03.2020 12:46, David Woodhouse wrote:
>> From: David Woodhouse <dwmw@amazon.co.uk>
>>
>> Remove a ternary operator that made my brain hurt.
> 
> My position towards this hasn't changed, just ftr.
> 
>> Replace it with something simpler that makes it somewhat clearer that
>> the check for initrdidx < mbi->mods_count is because larger values are
>> what find_first_bit() will return when it doesn't find anything.
>>
>> Also drop the explicit check for module #0 since that would be the
>> dom0 kernel and the corresponding bit is always clear in module_map.
>>
>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
>> Acked-by: Julien Grall <julien@xen.org>
> 
> Strictly speaking this is not a valid tag here, only R-b would be.

I can't find any rule in our code base preventing a non-maintainer to 
add its "acked-by" tag.

But if you want to play at this game, my tag is technically valid 
because "THE REST" englobes the full Xen codebase (Note the * in the 
MAINTAINERS file). We happen to not be CCed by 
scripts/get_maintainers.pl because *you* were not happy to be spammed... 
So we modified the scripts.

In this particular case, I stand with the acked-by because I am ready to 
take the blame if something goes wrong with the patch. Such meaning is 
is not conveyed with "reviewed-by".

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] x86/setup: simplify handling of initrdidx when no initrd present

[David Woodhouse]

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

On Wed, 2020-03-18 at 12:51 +0100, Jan Beulich wrote:
> On 18.03.2020 12:46, David Woodhouse wrote:
> > From: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > Remove a ternary operator that made my brain hurt.
> 
> My position towards this hasn't changed, just ftr.

Your position was not clearly stated. In
https://lists.xenproject.org/archives/html/xen-devel/2020-02/msg01664.html
you indicated that you preferred for it to remain as-is but you did not
even seem to be disputing that the code is simpler and easier for the
reader to understand after my cleanup.

I was left wondering if your position was merely that you *liked*
making my brain hurt? :)

> > Replace it with something simpler that makes it somewhat clearer that
> > the check for initrdidx < mbi->mods_count is because larger values are
> > what find_first_bit() will return when it doesn't find anything.
> > 
> > Also drop the explicit check for module #0 since that would be the
> > dom0 kernel and the corresponding bit is always clear in module_map.
> > 
> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> > Acked-by: Julien Grall <julien@xen.org>
> 
> Strictly speaking this is not a valid tag here, only R-b would be.


[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 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

Re: [Xen-devel] [PATCH 1/2] x86/setup: simplify handling of initrdidx when no initrd present

[Jan Beulich]

On 18.03.2020 13:12, Julien Grall wrote:
> Hi,
> 
> On 18/03/2020 11:51, Jan Beulich wrote:
>> On 18.03.2020 12:46, David Woodhouse wrote:
>>> From: David Woodhouse <dwmw@amazon.co.uk>
>>>
>>> Remove a ternary operator that made my brain hurt.
>>
>> My position towards this hasn't changed, just ftr.
>>
>>> Replace it with something simpler that makes it somewhat clearer that
>>> the check for initrdidx < mbi->mods_count is because larger values are
>>> what find_first_bit() will return when it doesn't find anything.
>>>
>>> Also drop the explicit check for module #0 since that would be the
>>> dom0 kernel and the corresponding bit is always clear in module_map.
>>>
>>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
>>> Acked-by: Julien Grall <julien@xen.org>
>>
>> Strictly speaking this is not a valid tag here, only R-b would be.
> 
> I can't find any rule in our code base preventing a non-maintainer to add its "acked-by" tag.

I could have said "meaningful" instead of "valid": A patch is not
supposed to go in without a direct maintainer's ack, unless there's
a reason to invoke the nested maintainership rules. That's my
understanding at least.

> But if you want to play at this game, my tag is technically valid
> because "THE REST" englobes the full Xen codebase (Note the * in
> the MAINTAINERS file).

Note the nested maintainership wording in that file, which was added
pretty recently. If that wording isn't clear enough, perhaps we can
further refine it?

Jan

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

Re: [Xen-devel] [PATCH 1/2] x86/setup: simplify handling of initrdidx when no initrd present

[Jan Beulich]

On 18.03.2020 13:33, David Woodhouse wrote:
> On Wed, 2020-03-18 at 12:51 +0100, Jan Beulich wrote:
>> On 18.03.2020 12:46, David Woodhouse wrote:
>>> From: David Woodhouse <dwmw@amazon.co.uk>
>>>
>>> Remove a ternary operator that made my brain hurt.
>>
>> My position towards this hasn't changed, just ftr.
> 
> Your position was not clearly stated. In
> https://lists.xenproject.org/archives/html/xen-devel/2020-02/msg01664.html
> you indicated that you preferred for it to remain as-is but you did not
> even seem to be disputing that the code is simpler and easier for the
> reader to understand after my cleanup.
> 
> I was left wondering if your position was merely that you *liked*
> making my brain hurt? :)

Ehem. I would have thought indicating that you'd need Andrew's
ack was clear enough a sign that I wouldn't want to give mine.
I'm sorry if this wasn't the case.

Jan

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

Re: [Xen-devel] [PATCH 1/2] x86/setup: simplify handling of initrdidx when no initrd present

[Julien Grall]

On 18/03/2020 13:20, Jan Beulich wrote:
> On 18.03.2020 13:12, Julien Grall wrote:
>> Hi,
>>
>> On 18/03/2020 11:51, Jan Beulich wrote:
>>> On 18.03.2020 12:46, David Woodhouse wrote:
>>>> From: David Woodhouse <dwmw@amazon.co.uk>
>>>>
>>>> Remove a ternary operator that made my brain hurt.
>>>
>>> My position towards this hasn't changed, just ftr.
>>>
>>>> Replace it with something simpler that makes it somewhat clearer that
>>>> the check for initrdidx < mbi->mods_count is because larger values are
>>>> what find_first_bit() will return when it doesn't find anything.
>>>>
>>>> Also drop the explicit check for module #0 since that would be the
>>>> dom0 kernel and the corresponding bit is always clear in module_map.
>>>>
>>>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
>>>> Acked-by: Julien Grall <julien@xen.org>
>>>
>>> Strictly speaking this is not a valid tag here, only R-b would be.
>>
>> I can't find any rule in our code base preventing a non-maintainer to add its "acked-by" tag.
> 
> I could have said "meaningful" instead of "valid": A patch is not
> supposed to go in without a direct maintainer's ack, unless there's
> a reason to invoke the nested maintainership rules. That's my
> understanding at least.

I still don't see why you are not happy with my tag here the more I 
don't think David or I ever claimed my acked-by was sufficient for the 
patch to be merged.

With my tag I acknowledged the patch. I could also have ignored it and 
you would have complained that nobody help you reviewing patches...

> 
>> But if you want to play at this game, my tag is technically valid
>> because "THE REST" englobes the full Xen codebase (Note the * in
>> the MAINTAINERS file).
> 
> Note the nested maintainership wording in that file, which was added
> pretty recently. If that wording isn't clear enough, perhaps we can
> further refine it?

The wording is clear enough, but it still doesn't prevent me to add my 
acked-by.

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] x86/setup: simplify handling of initrdidx when no initrd present

[Jan Beulich]

On 18.03.2020 14:29, Julien Grall wrote:
> 
> 
> On 18/03/2020 13:20, Jan Beulich wrote:
>> On 18.03.2020 13:12, Julien Grall wrote:
>>> Hi,
>>>
>>> On 18/03/2020 11:51, Jan Beulich wrote:
>>>> On 18.03.2020 12:46, David Woodhouse wrote:
>>>>> From: David Woodhouse <dwmw@amazon.co.uk>
>>>>>
>>>>> Remove a ternary operator that made my brain hurt.
>>>>
>>>> My position towards this hasn't changed, just ftr.
>>>>
>>>>> Replace it with something simpler that makes it somewhat clearer that
>>>>> the check for initrdidx < mbi->mods_count is because larger values are
>>>>> what find_first_bit() will return when it doesn't find anything.
>>>>>
>>>>> Also drop the explicit check for module #0 since that would be the
>>>>> dom0 kernel and the corresponding bit is always clear in module_map.
>>>>>
>>>>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
>>>>> Acked-by: Julien Grall <julien@xen.org>
>>>>
>>>> Strictly speaking this is not a valid tag here, only R-b would be.
>>>
>>> I can't find any rule in our code base preventing a non-maintainer to add its "acked-by" tag.
>>
>> I could have said "meaningful" instead of "valid": A patch is not
>> supposed to go in without a direct maintainer's ack, unless there's
>> a reason to invoke the nested maintainership rules. That's my
>> understanding at least.
> 
> I still don't see why you are not happy with my tag here
> the more I don't think David or I ever claimed my acked-by
> was sufficient for the patch to be merged.

I didn't say I'm not happy with it. I merely tried to state a
fact, for the avoidance of doubt.

> With my tag I acknowledged the patch. I could also have
> ignored it and you would have complained that nobody help
> you reviewing patches...

An R-b would have achieved the same effect.

Jan

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

Re: [Xen-devel] [PATCH 1/2] x86/setup: simplify handling of initrdidx when no initrd present

[Wei Liu]

On Wed, Mar 18, 2020 at 11:46:06AM +0000, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> Remove a ternary operator that made my brain hurt.
> 
> Replace it with something simpler that makes it somewhat clearer that
> the check for initrdidx < mbi->mods_count is because larger values are
> what find_first_bit() will return when it doesn't find anything.
> 
> Also drop the explicit check for module #0 since that would be the
> dom0 kernel and the corresponding bit is always clear in module_map.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> Acked-by: Julien Grall <julien@xen.org>

Reviewed-by: Wei Liu <wl@xen.org>

I think this is a fine improvement. It is more straightforward to
follow.

Wei.

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

Re: [Xen-devel] [PATCH 2/2] x86/setup: lift dom0 creation out into create_dom0() function

[Jan Beulich]

On 18.03.2020 12:46, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> The creation of dom0 can be relatively self-contained. Shift it into
> a separate function and simplify __start_xen() a little bit.
> 
> This is a cleanup in its own right, but will be even more desireable
> when live update provides an alternative path through __start_xen()
> that doesn't involve creating a new dom0 at all.
> 
> Move the calculation of the 'initrd' parameter for create_dom0()
> down past the cosmetic printk about NX support, because in the fullness
> of time the whole initrd and create_dom0() part will be under the same
> "not live update" conditional. And in the meantime it's just neater.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>

Acked-by: Jan Beulich <jbeulich@suse.com>
with one further small cosmetic aspect taken care of (which
ought to be doable while committing):

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -679,6 +679,92 @@ static unsigned int __init copy_bios_e820(struct e820entry *map, unsigned int li
>      return n;
>  }
>  
> +static struct domain * __init create_dom0(const module_t *image,

We put blanks to the left of stars, but not to the right. (I'm sure
you'd be able to point out examples to the contrary, but that's
what we're at least striving for, from all I can tell.)

Jan

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