Problem with IOMEM and domain reboot

Problem with IOMEM and domain reboot

[Oleksandr Andrushchenko]

[-- Attachment #1: Type: text/plain, Size: 1545 bytes --]

Hi, all!

While trying to reboot a domain which has iomem configured
(we are passing through some devices), I found an issue,
that after domain reboot those iomem's are incorrectly re-mapped,
e.g. for the configuration snippet below fe960 -> 0.

Part of the domain config I use:
iomem=[
     "0xfd010,1@0xfd000",
     "fe960,8",
]

During domain creation:
libxl_create.c:210:libxl__domain_build_info_setdefault: iomem gfn fd000 
start fd010
libxl_create.c:210:libxl__domain_build_info_setdefault: iomem gfn 
ffffffffffffffff start fe960

which means that for fe960 initial value was set to LIBXL_INVALID_GFN
and then on domain configuration, 
tools/libxl/libxl_create.c:libxl__domain_build_info_setdefault:

     for (i = 0 ; i < b_info->num_iomem; i++)
         if (b_info->iomem[i].gfn == LIBXL_INVALID_GFN)
             b_info->iomem[i].gfn = b_info->iomem[i].start;

made that GFN for fe960 to be set to the correct value.

But during domain reboot I see that 
tools/xl/xl_vmcontrol.c:reload_domain_config
tries to replicate configuration from the original domain config
being rebooted, but that leads to iomem's GFN to be set to 0 (if configured
in form [IOMEM_START,NUM_PAGES], but for [IOMEM_START,NUM_PAGES[@GFN] it 
is ok):

iomem gfn fd000 start fd010
iomem gfn 0 start fe960

Thus, further domain restart procedure leads to invalid mapping, e.g. 
fe960 -> 0.

I created a patch which allowed me to reboot the domain, but I would love
to hear comments on what would be the proper fix.

Thank you,
Oleksandr


[-- Attachment #2: 0001-HACK-Reset-iomem-s-gfn-to-LIBXL_INVALID_GFN-on-reboo.patch --]
[-- Type: text/x-patch, Size: 1717 bytes --]

>From aa1f20af73a5a3c8f2c904b857a79334d18d41ff Mon Sep 17 00:00:00 2001
From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Date: Wed, 20 Dec 2017 17:51:18 +0200
Subject: [PATCH] [HACK] Reset iomem's gfn to LIBXL_INVALID_GFN on reboot

During domain reboot its configuration is partially reused
to re-create a new domain, but iomem's GFN field for the
iomem is only restored for those memory ranges, which are
configured in form of [IOMEM_START,NUM_PAGES[@GFN], but not for
those in form of [IOMEM_START,NUM_PAGES], e.g. without GFN.
For the latter GFN is reset to 0, but while mapping ranges
to a domain during reboot there is a check that GFN treated
as valid if it is not equal to LIBXL_INVALID_GFN, thus making
Xen to map IOMEM_START to address 0 in the guest's address space.

Workaround it by resseting GFN to LIBXL_INVALID_GFN, so xl
can set proper values for mapping on reboot.

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
---
 tools/libxl/libxl_domain.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/libxl/libxl_domain.c b/tools/libxl/libxl_domain.c
index ef1a0927b00d..2678ad2ad54f 100644
--- a/tools/libxl/libxl_domain.c
+++ b/tools/libxl/libxl_domain.c
@@ -1647,6 +1647,15 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
         }
     }
 
+    /* reset IOMEM's GFN to initial value */
+    {
+        int i;
+
+        for (i = 0; i < d_config->b_info.num_iomem; i++)
+            if (d_config->b_info.iomem[i].gfn == 0)
+                d_config->b_info.iomem[i].gfn = LIBXL_INVALID_GFN;
+    }
+
     /* Devices: disk, nic, vtpm, pcidev etc. */
 
     /* The MERGE macro implements following logic:
-- 
2.7.4


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

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

Re: Problem with IOMEM and domain reboot

[Oleksandr Andrushchenko]

Ian, Wei,

could you please take a look at the below?

Thank you,

Oleksandr


On 12/20/2017 06:27 PM, Oleksandr Andrushchenko wrote:
> Hi, all!
>
> While trying to reboot a domain which has iomem configured
> (we are passing through some devices), I found an issue,
> that after domain reboot those iomem's are incorrectly re-mapped,
> e.g. for the configuration snippet below fe960 -> 0.
>
> Part of the domain config I use:
> iomem=[
>     "0xfd010,1@0xfd000",
>     "fe960,8",
> ]
>
> During domain creation:
> libxl_create.c:210:libxl__domain_build_info_setdefault: iomem gfn 
> fd000 start fd010
> libxl_create.c:210:libxl__domain_build_info_setdefault: iomem gfn 
> ffffffffffffffff start fe960
>
> which means that for fe960 initial value was set to LIBXL_INVALID_GFN
> and then on domain configuration, 
> tools/libxl/libxl_create.c:libxl__domain_build_info_setdefault:
>
>     for (i = 0 ; i < b_info->num_iomem; i++)
>         if (b_info->iomem[i].gfn == LIBXL_INVALID_GFN)
>             b_info->iomem[i].gfn = b_info->iomem[i].start;
>
> made that GFN for fe960 to be set to the correct value.
>
> But during domain reboot I see that 
> tools/xl/xl_vmcontrol.c:reload_domain_config
> tries to replicate configuration from the original domain config
> being rebooted, but that leads to iomem's GFN to be set to 0 (if 
> configured
> in form [IOMEM_START,NUM_PAGES], but for [IOMEM_START,NUM_PAGES[@GFN] 
> it is ok):
>
> iomem gfn fd000 start fd010
> iomem gfn 0 start fe960
>
> Thus, further domain restart procedure leads to invalid mapping, e.g. 
> fe960 -> 0.
>
> I created a patch which allowed me to reboot the domain, but I would love
> to hear comments on what would be the proper fix.
>
> Thank you,
> Oleksandr
>


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

Re: Problem with IOMEM and domain reboot

[Wei Liu]

On Wed, Dec 20, 2017 at 06:27:02PM +0200, Oleksandr Andrushchenko wrote:
> Hi, all!
> 
> While trying to reboot a domain which has iomem configured
> (we are passing through some devices), I found an issue,
> that after domain reboot those iomem's are incorrectly re-mapped,
> e.g. for the configuration snippet below fe960 -> 0.
> 
> Part of the domain config I use:
> iomem=[
>     "0xfd010,1@0xfd000",
>     "fe960,8",
> ]
> 
> During domain creation:
> libxl_create.c:210:libxl__domain_build_info_setdefault: iomem gfn fd000
> start fd010
> libxl_create.c:210:libxl__domain_build_info_setdefault: iomem gfn
> ffffffffffffffff start fe960
> 
> which means that for fe960 initial value was set to LIBXL_INVALID_GFN
> and then on domain configuration,
> tools/libxl/libxl_create.c:libxl__domain_build_info_setdefault:
> 
>     for (i = 0 ; i < b_info->num_iomem; i++)
>         if (b_info->iomem[i].gfn == LIBXL_INVALID_GFN)
>             b_info->iomem[i].gfn = b_info->iomem[i].start;
> 
> made that GFN for fe960 to be set to the correct value.
> 
> But during domain reboot I see that
> tools/xl/xl_vmcontrol.c:reload_domain_config
> tries to replicate configuration from the original domain config
> being rebooted, but that leads to iomem's GFN to be set to 0 (if configured
> in form [IOMEM_START,NUM_PAGES], but for [IOMEM_START,NUM_PAGES[@GFN] it is
> ok):
> 
> iomem gfn fd000 start fd010
> iomem gfn 0 start fe960
> 
> Thus, further domain restart procedure leads to invalid mapping, e.g. fe960
> -> 0.
> 
> I created a patch which allowed me to reboot the domain, but I would love
> to hear comments on what would be the proper fix.
> 
> Thank you,
> Oleksandr
> 

> From aa1f20af73a5a3c8f2c904b857a79334d18d41ff Mon Sep 17 00:00:00 2001
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> Date: Wed, 20 Dec 2017 17:51:18 +0200
> Subject: [PATCH] [HACK] Reset iomem's gfn to LIBXL_INVALID_GFN on reboot
> 
> During domain reboot its configuration is partially reused
> to re-create a new domain, but iomem's GFN field for the
> iomem is only restored for those memory ranges, which are
> configured in form of [IOMEM_START,NUM_PAGES[@GFN], but not for
> those in form of [IOMEM_START,NUM_PAGES], e.g. without GFN.
> For the latter GFN is reset to 0, but while mapping ranges
> to a domain during reboot there is a check that GFN treated
> as valid if it is not equal to LIBXL_INVALID_GFN, thus making
> Xen to map IOMEM_START to address 0 in the guest's address space.
> 
> Workaround it by resseting GFN to LIBXL_INVALID_GFN, so xl
> can set proper values for mapping on reboot.
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> ---
>  tools/libxl/libxl_domain.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/tools/libxl/libxl_domain.c b/tools/libxl/libxl_domain.c
> index ef1a0927b00d..2678ad2ad54f 100644
> --- a/tools/libxl/libxl_domain.c
> +++ b/tools/libxl/libxl_domain.c
> @@ -1647,6 +1647,15 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
>          }
>      }
>  
> +    /* reset IOMEM's GFN to initial value */
> +    {
> +        int i;
> +
> +        for (i = 0; i < d_config->b_info.num_iomem; i++)
> +            if (d_config->b_info.iomem[i].gfn == 0)
> +                d_config->b_info.iomem[i].gfn = LIBXL_INVALID_GFN;
> +    }
> +

I don't think this is necessary. Instead we should tell libxl to save
the generated value into the template. Add an update_config hook for the
iomem type should be better.

Wei.

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

Re: Problem with IOMEM and domain reboot

[Oleksandr Andrushchenko]

Hi, Wei!


On 02/06/2018 02:36 PM, Wei Liu wrote:
> On Wed, Dec 20, 2017 at 06:27:02PM +0200, Oleksandr Andrushchenko wrote:
>> Hi, all!
>>
>> While trying to reboot a domain which has iomem configured
>> (we are passing through some devices), I found an issue,
>> that after domain reboot those iomem's are incorrectly re-mapped,
>> e.g. for the configuration snippet below fe960 -> 0.
>>
>> Part of the domain config I use:
>> iomem=[
>>      "0xfd010,1@0xfd000",
>>      "fe960,8",
>> ]
>>
>> During domain creation:
>> libxl_create.c:210:libxl__domain_build_info_setdefault: iomem gfn fd000
>> start fd010
>> libxl_create.c:210:libxl__domain_build_info_setdefault: iomem gfn
>> ffffffffffffffff start fe960
>>
>> which means that for fe960 initial value was set to LIBXL_INVALID_GFN
>> and then on domain configuration,
>> tools/libxl/libxl_create.c:libxl__domain_build_info_setdefault:
>>
>>      for (i = 0 ; i < b_info->num_iomem; i++)
>>          if (b_info->iomem[i].gfn == LIBXL_INVALID_GFN)
>>              b_info->iomem[i].gfn = b_info->iomem[i].start;
>>
>> made that GFN for fe960 to be set to the correct value.
>>
>> But during domain reboot I see that
>> tools/xl/xl_vmcontrol.c:reload_domain_config
>> tries to replicate configuration from the original domain config
>> being rebooted, but that leads to iomem's GFN to be set to 0 (if configured
>> in form [IOMEM_START,NUM_PAGES], but for [IOMEM_START,NUM_PAGES[@GFN] it is
>> ok):
>>
>> iomem gfn fd000 start fd010
>> iomem gfn 0 start fe960
>>
>> Thus, further domain restart procedure leads to invalid mapping, e.g. fe960
>> -> 0.
>>
>> I created a patch which allowed me to reboot the domain, but I would love
>> to hear comments on what would be the proper fix.
>>
>> Thank you,
>> Oleksandr
>>
>>  From aa1f20af73a5a3c8f2c904b857a79334d18d41ff Mon Sep 17 00:00:00 2001
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>> Date: Wed, 20 Dec 2017 17:51:18 +0200
>> Subject: [PATCH] [HACK] Reset iomem's gfn to LIBXL_INVALID_GFN on reboot
>>
>> During domain reboot its configuration is partially reused
>> to re-create a new domain, but iomem's GFN field for the
>> iomem is only restored for those memory ranges, which are
>> configured in form of [IOMEM_START,NUM_PAGES[@GFN], but not for
>> those in form of [IOMEM_START,NUM_PAGES], e.g. without GFN.
>> For the latter GFN is reset to 0, but while mapping ranges
>> to a domain during reboot there is a check that GFN treated
>> as valid if it is not equal to LIBXL_INVALID_GFN, thus making
>> Xen to map IOMEM_START to address 0 in the guest's address space.
>>
>> Workaround it by resseting GFN to LIBXL_INVALID_GFN, so xl
>> can set proper values for mapping on reboot.
>>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>> ---
>>   tools/libxl/libxl_domain.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/tools/libxl/libxl_domain.c b/tools/libxl/libxl_domain.c
>> index ef1a0927b00d..2678ad2ad54f 100644
>> --- a/tools/libxl/libxl_domain.c
>> +++ b/tools/libxl/libxl_domain.c
>> @@ -1647,6 +1647,15 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
>>           }
>>       }
>>   
>> +    /* reset IOMEM's GFN to initial value */
>> +    {
>> +        int i;
>> +
>> +        for (i = 0; i < d_config->b_info.num_iomem; i++)
>> +            if (d_config->b_info.iomem[i].gfn == 0)
>> +                d_config->b_info.iomem[i].gfn = LIBXL_INVALID_GFN;
>> +    }
>> +
> I don't think this is necessary. Instead we should tell libxl to save
> the generated value into the template. Add an update_config hook for the
> iomem type should be better.
Agree, this is why I tagged the patch as [HACK]
Unfortunately, I have little knowledge of libxl and not sure
how to properly fix it. Can you tell a bit more on what
a proper fix could be?
> Wei.
Thank you,
Oleksandr

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

Re: Problem with IOMEM and domain reboot

[Wei Liu]

On Tue, Feb 06, 2018 at 02:44:56PM +0200, Oleksandr Andrushchenko wrote:
>  From aa1f20af73a5a3c8f2c904b857a79334d18d41ff Mon Sep 17 00:00:00 2001
> > > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> > > Date: Wed, 20 Dec 2017 17:51:18 +0200
> > > Subject: [PATCH] [HACK] Reset iomem's gfn to LIBXL_INVALID_GFN on reboot
> > > 
> > > During domain reboot its configuration is partially reused
> > > to re-create a new domain, but iomem's GFN field for the
> > > iomem is only restored for those memory ranges, which are
> > > configured in form of [IOMEM_START,NUM_PAGES[@GFN], but not for
> > > those in form of [IOMEM_START,NUM_PAGES], e.g. without GFN.
> > > For the latter GFN is reset to 0, but while mapping ranges
> > > to a domain during reboot there is a check that GFN treated
> > > as valid if it is not equal to LIBXL_INVALID_GFN, thus making
> > > Xen to map IOMEM_START to address 0 in the guest's address space.
> > > 
> > > Workaround it by resseting GFN to LIBXL_INVALID_GFN, so xl
> > > can set proper values for mapping on reboot.
> > > 
> > > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> > > ---
> > >   tools/libxl/libxl_domain.c | 9 +++++++++
> > >   1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/tools/libxl/libxl_domain.c b/tools/libxl/libxl_domain.c
> > > index ef1a0927b00d..2678ad2ad54f 100644
> > > --- a/tools/libxl/libxl_domain.c
> > > +++ b/tools/libxl/libxl_domain.c
> > > @@ -1647,6 +1647,15 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
> > >           }
> > >       }
> > > +    /* reset IOMEM's GFN to initial value */
> > > +    {
> > > +        int i;
> > > +
> > > +        for (i = 0; i < d_config->b_info.num_iomem; i++)
> > > +            if (d_config->b_info.iomem[i].gfn == 0)
> > > +                d_config->b_info.iomem[i].gfn = LIBXL_INVALID_GFN;
> > > +    }
> > > +
> > I don't think this is necessary. Instead we should tell libxl to save
> > the generated value into the template. Add an update_config hook for the
> > iomem type should be better.
> Agree, this is why I tagged the patch as [HACK]
> Unfortunately, I have little knowledge of libxl and not sure
> how to properly fix it. Can you tell a bit more on what
> a proper fix could be?

See libxl__update_domain_configuration, which is called after domain
construction is completed. It will call the update_config hook for a
device type to save anything that is generated in the process of domain
creation. One example is in libxl_nic. You can do the same to iomem I
think.

The end result is the generated values you care about are saved into the
template. When the domain is migrated / rebooted libxl will use the
saved values instead.

Strictly speaking your patch of adding the snippet to
libxl_retrieve_domain_configuration isn't wrong, but I would prefer that
function to only contain code to fetch states that can be changed during
domain runtime. The iomem range isn't one of those states AIUI.

Wei.


> > Wei.
> Thank you,
> Oleksandr

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

Re: Problem with IOMEM and domain reboot

[Oleksandr Andrushchenko]

On 02/06/2018 02:52 PM, Wei Liu wrote:
> On Tue, Feb 06, 2018 at 02:44:56PM +0200, Oleksandr Andrushchenko wrote:
>>   From aa1f20af73a5a3c8f2c904b857a79334d18d41ff Mon Sep 17 00:00:00 2001
>>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>>> Date: Wed, 20 Dec 2017 17:51:18 +0200
>>>> Subject: [PATCH] [HACK] Reset iomem's gfn to LIBXL_INVALID_GFN on reboot
>>>>
>>>> During domain reboot its configuration is partially reused
>>>> to re-create a new domain, but iomem's GFN field for the
>>>> iomem is only restored for those memory ranges, which are
>>>> configured in form of [IOMEM_START,NUM_PAGES[@GFN], but not for
>>>> those in form of [IOMEM_START,NUM_PAGES], e.g. without GFN.
>>>> For the latter GFN is reset to 0, but while mapping ranges
>>>> to a domain during reboot there is a check that GFN treated
>>>> as valid if it is not equal to LIBXL_INVALID_GFN, thus making
>>>> Xen to map IOMEM_START to address 0 in the guest's address space.
>>>>
>>>> Workaround it by resseting GFN to LIBXL_INVALID_GFN, so xl
>>>> can set proper values for mapping on reboot.
>>>>
>>>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>>> ---
>>>>    tools/libxl/libxl_domain.c | 9 +++++++++
>>>>    1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/tools/libxl/libxl_domain.c b/tools/libxl/libxl_domain.c
>>>> index ef1a0927b00d..2678ad2ad54f 100644
>>>> --- a/tools/libxl/libxl_domain.c
>>>> +++ b/tools/libxl/libxl_domain.c
>>>> @@ -1647,6 +1647,15 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
>>>>            }
>>>>        }
>>>> +    /* reset IOMEM's GFN to initial value */
>>>> +    {
>>>> +        int i;
>>>> +
>>>> +        for (i = 0; i < d_config->b_info.num_iomem; i++)
>>>> +            if (d_config->b_info.iomem[i].gfn == 0)
>>>> +                d_config->b_info.iomem[i].gfn = LIBXL_INVALID_GFN;
>>>> +    }
>>>> +
>>> I don't think this is necessary. Instead we should tell libxl to save
>>> the generated value into the template. Add an update_config hook for the
>>> iomem type should be better.
>> Agree, this is why I tagged the patch as [HACK]
>> Unfortunately, I have little knowledge of libxl and not sure
>> how to properly fix it. Can you tell a bit more on what
>> a proper fix could be?
> See libxl__update_domain_configuration, which is called after domain
> construction is completed. It will call the update_config hook for a
> device type to save anything that is generated in the process of domain
> creation. One example is in libxl_nic. You can do the same to iomem I
> think.
>
> The end result is the generated values you care about are saved into the
> template. When the domain is migrated / rebooted libxl will use the
> saved values instead.
Thank you, will look at it to make a proper fix
> Strictly speaking your patch of adding the snippet to
> libxl_retrieve_domain_configuration isn't wrong, but I would prefer that
> function to only contain code to fetch states that can be changed during
> domain runtime. The iomem range isn't one of those states AIUI.
>
> Wei.
>
>
>>> Wei.
>> Thank you,
>> Oleksandr


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

Re: Problem with IOMEM and domain reboot

[Oleksandr Grytsov]

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

On Wed, Feb 7, 2018 at 2:14 PM, Oleksandr Andrushchenko <andr2000@gmail.com>
wrote:

> On 02/06/2018 02:52 PM, Wei Liu wrote:
>
>> On Tue, Feb 06, 2018 at 02:44:56PM +0200, Oleksandr Andrushchenko wrote:
>>
>>>   From aa1f20af73a5a3c8f2c904b857a79334d18d41ff Mon Sep 17 00:00:00 2001
>>>
>>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>>>> Date: Wed, 20 Dec 2017 17:51:18 +0200
>>>>> Subject: [PATCH] [HACK] Reset iomem's gfn to LIBXL_INVALID_GFN on
>>>>> reboot
>>>>>
>>>>> During domain reboot its configuration is partially reused
>>>>> to re-create a new domain, but iomem's GFN field for the
>>>>> iomem is only restored for those memory ranges, which are
>>>>> configured in form of [IOMEM_START,NUM_PAGES[@GFN], but not for
>>>>> those in form of [IOMEM_START,NUM_PAGES], e.g. without GFN.
>>>>> For the latter GFN is reset to 0, but while mapping ranges
>>>>> to a domain during reboot there is a check that GFN treated
>>>>> as valid if it is not equal to LIBXL_INVALID_GFN, thus making
>>>>> Xen to map IOMEM_START to address 0 in the guest's address space.
>>>>>
>>>>> Workaround it by resseting GFN to LIBXL_INVALID_GFN, so xl
>>>>> can set proper values for mapping on reboot.
>>>>>
>>>>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.
>>>>> com>
>>>>> ---
>>>>>    tools/libxl/libxl_domain.c | 9 +++++++++
>>>>>    1 file changed, 9 insertions(+)
>>>>>
>>>>> diff --git a/tools/libxl/libxl_domain.c b/tools/libxl/libxl_domain.c
>>>>> index ef1a0927b00d..2678ad2ad54f 100644
>>>>> --- a/tools/libxl/libxl_domain.c
>>>>> +++ b/tools/libxl/libxl_domain.c
>>>>> @@ -1647,6 +1647,15 @@ int libxl_retrieve_domain_configuration(libxl_ctx
>>>>> *ctx, uint32_t domid,
>>>>>            }
>>>>>        }
>>>>> +    /* reset IOMEM's GFN to initial value */
>>>>> +    {
>>>>> +        int i;
>>>>> +
>>>>> +        for (i = 0; i < d_config->b_info.num_iomem; i++)
>>>>> +            if (d_config->b_info.iomem[i].gfn == 0)
>>>>> +                d_config->b_info.iomem[i].gfn = LIBXL_INVALID_GFN;
>>>>> +    }
>>>>> +
>>>>>
>>>> I don't think this is necessary. Instead we should tell libxl to save
>>>> the generated value into the template. Add an update_config hook for the
>>>> iomem type should be better.
>>>>
>>> Agree, this is why I tagged the patch as [HACK]
>>> Unfortunately, I have little knowledge of libxl and not sure
>>> how to properly fix it. Can you tell a bit more on what
>>> a proper fix could be?
>>>
>> See libxl__update_domain_configuration, which is called after domain
>> construction is completed. It will call the update_config hook for a
>> device type to save anything that is generated in the process of domain
>> creation. One example is in libxl_nic. You can do the same to iomem I
>> think.
>>
>> The end result is the generated values you care about are saved into the
>> template. When the domain is migrated / rebooted libxl will use the
>> saved values instead.
>>
> Thank you, will look at it to make a proper fix
>
> Strictly speaking your patch of adding the snippet to
>> libxl_retrieve_domain_configuration isn't wrong, but I would prefer that
>> function to only contain code to fetch states that can be changed during
>> domain runtime. The iomem range isn't one of those states AIUI.
>>
>> Wei.
>>
>>
>> Wei.
>>>>
>>> Thank you,
>>> Oleksandr
>>>
>>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
>


Hi Wei,

The root cause of this problem is that auto generated code doesn't handle
default value when json is parsed. It is related not only IOMEM but
potentially some other structure as well.

Field "gfn" of libxl_iomem_range structure has default value
LIBXL_INVALID_GFN
which is not 0.

libxl_iomem_range = Struct("iomem_range", [
    ...
    ("gfn", uint64, {'init_val': "LIBXL_INVALID_GFN"}),
    ])

The default value is handled correctly when json is generated:

yajl_gen_status libxl_iomem_range_gen_json(yajl_gen hand, libxl_iomem_range
*p)
{
    ...

    if (p->gfn != LIBXL_INVALID_GFN) {
        s = yajl_gen_string(hand, (const unsigned char *)"gfn",
sizeof("gfn")-1);
        if (s != yajl_gen_status_ok)
            goto out;
        s = libxl__uint64_gen_json(hand, p->gfn);
        if (s != yajl_gen_status_ok)
            goto out;
    }

    ...
}

But when json is parsed, this "gfn" field is parsed as any other uint64
value.
As result we have 0 instead of LIBXL_INVALID_GFN.

int libxl__iomem_range_parse_json(libxl__gc *gc, const libxl__json_object
*o, libxl_iomem_range *p)
{
    ...

    {
        const libxl__json_object *saved_gfn = x;
        x = libxl__json_map_get("gfn", o, JSON_INTEGER);
        if (x) {
            rc = libxl__uint64_parse_json(gc, x, &p->gfn);
            if (rc)
                goto out;
        }
        x = saved_gfn;
    }
}

Is it done by design or there is an issue with parse_json?
If it is done by design then the solution proposed by you (update_config
hook)
will solve this problem. But handling default value in parse json looks
more correct.

-- 
Best Regards,
Oleksandr Grytsov.

[-- Attachment #1.2: Type: text/html, Size: 8094 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: Problem with IOMEM and domain reboot

[Wei Liu]

On Mon, Feb 12, 2018 at 07:22:27PM +0200, Oleksandr Grytsov wrote:
> On Wed, Feb 7, 2018 at 2:14 PM, Oleksandr Andrushchenko <andr2000@gmail.com>
> wrote:
> 
> > On 02/06/2018 02:52 PM, Wei Liu wrote:
> >
> >> On Tue, Feb 06, 2018 at 02:44:56PM +0200, Oleksandr Andrushchenko wrote:
> >>
> >>>   From aa1f20af73a5a3c8f2c904b857a79334d18d41ff Mon Sep 17 00:00:00 2001
> >>>
> >>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> >>>>> Date: Wed, 20 Dec 2017 17:51:18 +0200
> >>>>> Subject: [PATCH] [HACK] Reset iomem's gfn to LIBXL_INVALID_GFN on
> >>>>> reboot
> >>>>>
> >>>>> During domain reboot its configuration is partially reused
> >>>>> to re-create a new domain, but iomem's GFN field for the
> >>>>> iomem is only restored for those memory ranges, which are
> >>>>> configured in form of [IOMEM_START,NUM_PAGES[@GFN], but not for
> >>>>> those in form of [IOMEM_START,NUM_PAGES], e.g. without GFN.
> >>>>> For the latter GFN is reset to 0, but while mapping ranges
> >>>>> to a domain during reboot there is a check that GFN treated
> >>>>> as valid if it is not equal to LIBXL_INVALID_GFN, thus making
> >>>>> Xen to map IOMEM_START to address 0 in the guest's address space.
> >>>>>
> >>>>> Workaround it by resseting GFN to LIBXL_INVALID_GFN, so xl
> >>>>> can set proper values for mapping on reboot.
> >>>>>
> >>>>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.
> >>>>> com>
> >>>>> ---
> >>>>>    tools/libxl/libxl_domain.c | 9 +++++++++
> >>>>>    1 file changed, 9 insertions(+)
> >>>>>
> >>>>> diff --git a/tools/libxl/libxl_domain.c b/tools/libxl/libxl_domain.c
> >>>>> index ef1a0927b00d..2678ad2ad54f 100644
> >>>>> --- a/tools/libxl/libxl_domain.c
> >>>>> +++ b/tools/libxl/libxl_domain.c
> >>>>> @@ -1647,6 +1647,15 @@ int libxl_retrieve_domain_configuration(libxl_ctx
> >>>>> *ctx, uint32_t domid,
> >>>>>            }
> >>>>>        }
> >>>>> +    /* reset IOMEM's GFN to initial value */
> >>>>> +    {
> >>>>> +        int i;
> >>>>> +
> >>>>> +        for (i = 0; i < d_config->b_info.num_iomem; i++)
> >>>>> +            if (d_config->b_info.iomem[i].gfn == 0)
> >>>>> +                d_config->b_info.iomem[i].gfn = LIBXL_INVALID_GFN;
> >>>>> +    }
> >>>>> +
> >>>>>
> >>>> I don't think this is necessary. Instead we should tell libxl to save
> >>>> the generated value into the template. Add an update_config hook for the
> >>>> iomem type should be better.
> >>>>
> >>> Agree, this is why I tagged the patch as [HACK]
> >>> Unfortunately, I have little knowledge of libxl and not sure
> >>> how to properly fix it. Can you tell a bit more on what
> >>> a proper fix could be?
> >>>
> >> See libxl__update_domain_configuration, which is called after domain
> >> construction is completed. It will call the update_config hook for a
> >> device type to save anything that is generated in the process of domain
> >> creation. One example is in libxl_nic. You can do the same to iomem I
> >> think.
> >>
> >> The end result is the generated values you care about are saved into the
> >> template. When the domain is migrated / rebooted libxl will use the
> >> saved values instead.
> >>
> > Thank you, will look at it to make a proper fix
> >
> > Strictly speaking your patch of adding the snippet to
> >> libxl_retrieve_domain_configuration isn't wrong, but I would prefer that
> >> function to only contain code to fetch states that can be changed during
> >> domain runtime. The iomem range isn't one of those states AIUI.
> >>
> >> Wei.
> >>
> >>
> >> Wei.
> >>>>
> >>> Thank you,
> >>> Oleksandr
> >>>
> >>
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xenproject.org
> > https://lists.xenproject.org/mailman/listinfo/xen-devel
> >
> 
> 
> Hi Wei,
> 
> The root cause of this problem is that auto generated code doesn't handle
> default value when json is parsed. It is related not only IOMEM but
> potentially some other structure as well.
> 

Oh, that. It most likely affect primitive types.

> Field "gfn" of libxl_iomem_range structure has default value
> LIBXL_INVALID_GFN
> which is not 0.
> 
> libxl_iomem_range = Struct("iomem_range", [
>     ...
>     ("gfn", uint64, {'init_val': "LIBXL_INVALID_GFN"}),
>     ])
> 
> The default value is handled correctly when json is generated:
> 
> yajl_gen_status libxl_iomem_range_gen_json(yajl_gen hand, libxl_iomem_range
> *p)
> {
>     ...
> 
>     if (p->gfn != LIBXL_INVALID_GFN) {
>         s = yajl_gen_string(hand, (const unsigned char *)"gfn",
> sizeof("gfn")-1);
>         if (s != yajl_gen_status_ok)
>             goto out;
>         s = libxl__uint64_gen_json(hand, p->gfn);
>         if (s != yajl_gen_status_ok)
>             goto out;
>     }
> 
>     ...
> }
> 
> But when json is parsed, this "gfn" field is parsed as any other uint64
> value.
> As result we have 0 instead of LIBXL_INVALID_GFN.

Why? The above snippet says no output is generated if the value is
LIBXL_INVALID_GFN.

Hence ...

> 
> int libxl__iomem_range_parse_json(libxl__gc *gc, const libxl__json_object
> *o, libxl_iomem_range *p)
> {
>     ...
> 
>     {
>         const libxl__json_object *saved_gfn = x;
>         x = libxl__json_map_get("gfn", o, JSON_INTEGER);

... This should fail to get a value from the "gfn" key, right?

>         if (x) {
>             rc = libxl__uint64_parse_json(gc, x, &p->gfn);
>             if (rc)
>                 goto out;
>         }
>         x = saved_gfn;
>     }
> }
> 
> Is it done by design or there is an issue with parse_json?
> If it is done by design then the solution proposed by you (update_config
> hook)
> will solve this problem. But handling default value in parse json looks
> more correct.

I need to figure out what is going on before I can answer these
questions. :-)

Wei.

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

Re: Problem with IOMEM and domain reboot

[Andrii Anisov]

Hello Wei,

On 13.02.18 14:24, Wei Liu wrote:
> On Mon, Feb 12, 2018 at 07:22:27PM +0200, Oleksandr Grytsov wrote:
>> Is it done by design or there is an issue with parse_json?
>> If it is done by design then the solution proposed by you (update_config
>> hook)
>> will solve this problem. But handling default value in parse json looks
>> more correct.
> 
> I need to figure out what is going on before I can answer these
> questions. :-)

Sorry for my ignorance, is this issue fixed in coming 4.12?
If not, is there any chance to get it finished for 4.12?

-- 
Sincerely,
Andrii Anisov.

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

Re: Problem with IOMEM and domain reboot

[Wei Liu]

On Wed, Jan 23, 2019 at 06:28:46PM +0200, Andrii Anisov wrote:
> Hello Wei,
> 
> On 13.02.18 14:24, Wei Liu wrote:
> > On Mon, Feb 12, 2018 at 07:22:27PM +0200, Oleksandr Grytsov wrote:
> > > Is it done by design or there is an issue with parse_json?
> > > If it is done by design then the solution proposed by you (update_config
> > > hook)
> > > will solve this problem. But handling default value in parse json looks
> > > more correct.
> > 
> > I need to figure out what is going on before I can answer these
> > questions. :-)
> 
> Sorry for my ignorance, is this issue fixed in coming 4.12?
> If not, is there any chance to get it finished for 4.12?

There were some unanswered questions. I don't think anything has changed
in 4.12.

Wei.

> 
> -- 
> Sincerely,
> Andrii Anisov.

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