[PATCH V2] Add flag to start info regarding virtual mapped p2m list

[PATCH V2] Add flag to start info regarding virtual mapped p2m list

[Juergen Gross]

In order to indicate the Xen tools capability to support the virtual
mapped linear p2m list instead the 3 level mfn tree add a flag to the
start_info page.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/include/public/xen.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index 3703c39..36c6d62 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -777,6 +777,8 @@ typedef struct start_info start_info_t;
 #define SIF_INITDOMAIN    (1<<1)  /* Is this the initial control domain? */
 #define SIF_MULTIBOOT_MOD (1<<2)  /* Is mod_start a multiboot module? */
 #define SIF_MOD_START_PFN (1<<3)  /* Is mod_start a PFN? */
+#define SIF_VIRT_P2M      (1<<4)  /* Does Xen understand a virt. mapped P->M */
+                                  /* making the 3 level tree obsolete?       */
 #define SIF_PM_MASK       (0xFF<<8) /* reserve 1 byte for xen-pm options */
 
 /*
-- 
2.1.4

Re: [PATCH V2] Add flag to start info regarding virtual mapped p2m list

[Andrew Cooper]

On 03/03/15 09:29, Juergen Gross wrote:
> In order to indicate the Xen tools capability to support the virtual
> mapped linear p2m list instead the 3 level mfn tree add a flag to the
> start_info page.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>

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

Much more simple!

> ---
>  xen/include/public/xen.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> index 3703c39..36c6d62 100644
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -777,6 +777,8 @@ typedef struct start_info start_info_t;
>  #define SIF_INITDOMAIN    (1<<1)  /* Is this the initial control domain? */
>  #define SIF_MULTIBOOT_MOD (1<<2)  /* Is mod_start a multiboot module? */
>  #define SIF_MOD_START_PFN (1<<3)  /* Is mod_start a PFN? */
> +#define SIF_VIRT_P2M      (1<<4)  /* Does Xen understand a virt. mapped P->M */
> +                                  /* making the 3 level tree obsolete?       */
>  #define SIF_PM_MASK       (0xFF<<8) /* reserve 1 byte for xen-pm options */
>  
>  /*

Re: [PATCH V2] Add flag to start info regarding virtual mapped p2m list

[Jan Beulich]

>>> On 03.03.15 at 10:29, <"jgross@suse.com".non-mime.internet> wrote:
> In order to indicate the Xen tools capability to support the virtual
> mapped linear p2m list instead the 3 level mfn tree add a flag to the
> start_info page.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  xen/include/public/xen.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> index 3703c39..36c6d62 100644
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -777,6 +777,8 @@ typedef struct start_info start_info_t;
>  #define SIF_INITDOMAIN    (1<<1)  /* Is this the initial control domain? */
>  #define SIF_MULTIBOOT_MOD (1<<2)  /* Is mod_start a multiboot module? */
>  #define SIF_MOD_START_PFN (1<<3)  /* Is mod_start a PFN? */
> +#define SIF_VIRT_P2M      (1<<4)  /* Does Xen understand a virt. mapped P->M */
> +                                  /* making the 3 level tree obsolete?       */
>  #define SIF_PM_MASK       (0xFF<<8) /* reserve 1 byte for xen-pm options */
>  
>  /*

Is there any reason why this can't be part of the tools patch (series)
actually going to make use of it?

Also I'm not particularly happy with the name, as it suggests to be
a statement about the initial P2M the guest gets handed - yet that
is always virtually mapped. SIF_PERMANENT_VIRT_P2M is getting a
little long I'm afraid, so I'm looking for better suggestions.

Jan

Re: [PATCH V2] Add flag to start info regarding virtual mapped p2m list

[Juergen Gross]

On 03/03/2015 11:27 AM, Jan Beulich wrote:
>>>> On 03.03.15 at 10:29, <"jgross@suse.com".non-mime.internet> wrote:
>> In order to indicate the Xen tools capability to support the virtual
>> mapped linear p2m list instead the 3 level mfn tree add a flag to the
>> start_info page.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   xen/include/public/xen.h | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
>> index 3703c39..36c6d62 100644
>> --- a/xen/include/public/xen.h
>> +++ b/xen/include/public/xen.h
>> @@ -777,6 +777,8 @@ typedef struct start_info start_info_t;
>>   #define SIF_INITDOMAIN    (1<<1)  /* Is this the initial control domain? */
>>   #define SIF_MULTIBOOT_MOD (1<<2)  /* Is mod_start a multiboot module? */
>>   #define SIF_MOD_START_PFN (1<<3)  /* Is mod_start a PFN? */
>> +#define SIF_VIRT_P2M      (1<<4)  /* Does Xen understand a virt. mapped P->M */
>> +                                  /* making the 3 level tree obsolete?       */
>>   #define SIF_PM_MASK       (0xFF<<8) /* reserve 1 byte for xen-pm options */
>>
>>   /*
>
> Is there any reason why this can't be part of the tools patch (series)
> actually going to make use of it?

The main reason is I want to make use of it in the related kernel
series first. And this requires the Xen header implementation.

> Also I'm not particularly happy with the name, as it suggests to be
> a statement about the initial P2M the guest gets handed - yet that
> is always virtually mapped. SIF_PERMANENT_VIRT_P2M is getting a
> little long I'm afraid, so I'm looking for better suggestions.

SIF_VIRT_KERNEL_P2M?
SIF_FLAT_P2M?


Juergen

Re: [PATCH V2] Add flag to start info regarding virtual mapped p2m list

[Jan Beulich]

>>> On 03.03.15 at 11:32, <JGross@suse.com> wrote:
> On 03/03/2015 11:27 AM, Jan Beulich wrote:
>> Also I'm not particularly happy with the name, as it suggests to be
>> a statement about the initial P2M the guest gets handed - yet that
>> is always virtually mapped. SIF_PERMANENT_VIRT_P2M is getting a
>> little long I'm afraid, so I'm looking for better suggestions.
> 
> SIF_VIRT_KERNEL_P2M?
> SIF_FLAT_P2M?

Neither reflects that this is not a statement about the initial P2M.

Jan

Re: [PATCH V2] Add flag to start info regarding virtual mapped p2m list

[Juergen Gross]

On 03/03/2015 11:52 AM, Jan Beulich wrote:
>>>> On 03.03.15 at 11:32, <JGross@suse.com> wrote:
>> On 03/03/2015 11:27 AM, Jan Beulich wrote:
>>> Also I'm not particularly happy with the name, as it suggests to be
>>> a statement about the initial P2M the guest gets handed - yet that
>>> is always virtually mapped. SIF_PERMANENT_VIRT_P2M is getting a
>>> little long I'm afraid, so I'm looking for better suggestions.
>>
>> SIF_VIRT_KERNEL_P2M?
>> SIF_FLAT_P2M?
>
> Neither reflects that this is not a statement about the initial P2M.

What about SIF_VIRT_P2M_4TOOLS?

At least this variant would give a hint regarding the consumer of the
p2m. Anybody wanting to know more should probably read the comment. :-)


Juergen

Re: [PATCH V2] Add flag to start info regarding virtual mapped p2m list

[Jan Beulich]

>>> On 03.03.15 at 12:00, <JGross@suse.com> wrote:
> On 03/03/2015 11:52 AM, Jan Beulich wrote:
>>>>> On 03.03.15 at 11:32, <JGross@suse.com> wrote:
>>> On 03/03/2015 11:27 AM, Jan Beulich wrote:
>>>> Also I'm not particularly happy with the name, as it suggests to be
>>>> a statement about the initial P2M the guest gets handed - yet that
>>>> is always virtually mapped. SIF_PERMANENT_VIRT_P2M is getting a
>>>> little long I'm afraid, so I'm looking for better suggestions.
>>>
>>> SIF_VIRT_KERNEL_P2M?
>>> SIF_FLAT_P2M?
>>
>> Neither reflects that this is not a statement about the initial P2M.
> 
> What about SIF_VIRT_P2M_4TOOLS?

Yes, why not?

Jan

Re: [PATCH V2] Add flag to start info regarding virtual mapped p2m list

[Jan Beulich]

>>> On 03.03.15 at 11:32, <JGross@suse.com> wrote:
> On 03/03/2015 11:27 AM, Jan Beulich wrote:
>>>>> On 03.03.15 at 10:29, <"jgross@suse.com".non-mime.internet> wrote:
>>> In order to indicate the Xen tools capability to support the virtual
>>> mapped linear p2m list instead the 3 level mfn tree add a flag to the
>>> start_info page.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>>   xen/include/public/xen.h | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
>>> index 3703c39..36c6d62 100644
>>> --- a/xen/include/public/xen.h
>>> +++ b/xen/include/public/xen.h
>>> @@ -777,6 +777,8 @@ typedef struct start_info start_info_t;
>>>   #define SIF_INITDOMAIN    (1<<1)  /* Is this the initial control domain? */
>>>   #define SIF_MULTIBOOT_MOD (1<<2)  /* Is mod_start a multiboot module? */
>>>   #define SIF_MOD_START_PFN (1<<3)  /* Is mod_start a PFN? */
>>> +#define SIF_VIRT_P2M      (1<<4)  /* Does Xen understand a virt. mapped P->M 
> */
>>> +                                  /* making the 3 level tree obsolete?      
>  */
>>>   #define SIF_PM_MASK       (0xFF<<8) /* reserve 1 byte for xen-pm options */
>>>
>>>   /*
>>
>> Is there any reason why this can't be part of the tools patch (series)
>> actually going to make use of it?
> 
> The main reason is I want to make use of it in the related kernel
> series first. And this requires the Xen header implementation.

I was about to apply v3, but I'm still unconvinced: How would you
test those kernel side changes without having anything to set the
flag?

Jan

Re: [PATCH V2] Add flag to start info regarding virtual mapped p2m list

[Ian Campbell]

On Wed, 2015-03-04 at 08:58 +0000, Jan Beulich wrote:
> >>> On 03.03.15 at 11:32, <JGross@suse.com> wrote:
> > On 03/03/2015 11:27 AM, Jan Beulich wrote:
> >>>>> On 03.03.15 at 10:29, <"jgross@suse.com".non-mime.internet> wrote:
> >>> In order to indicate the Xen tools capability to support the virtual
> >>> mapped linear p2m list instead the 3 level mfn tree add a flag to the
> >>> start_info page.
> >>>
> >>> Signed-off-by: Juergen Gross <jgross@suse.com>
> >>> ---
> >>>   xen/include/public/xen.h | 2 ++
> >>>   1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> >>> index 3703c39..36c6d62 100644
> >>> --- a/xen/include/public/xen.h
> >>> +++ b/xen/include/public/xen.h
> >>> @@ -777,6 +777,8 @@ typedef struct start_info start_info_t;
> >>>   #define SIF_INITDOMAIN    (1<<1)  /* Is this the initial control domain? */
> >>>   #define SIF_MULTIBOOT_MOD (1<<2)  /* Is mod_start a multiboot module? */
> >>>   #define SIF_MOD_START_PFN (1<<3)  /* Is mod_start a PFN? */
> >>> +#define SIF_VIRT_P2M      (1<<4)  /* Does Xen understand a virt. mapped P->M 
> > */
> >>> +                                  /* making the 3 level tree obsolete?      
> >  */
> >>>   #define SIF_PM_MASK       (0xFF<<8) /* reserve 1 byte for xen-pm options */
> >>>
> >>>   /*
> >>
> >> Is there any reason why this can't be part of the tools patch (series)
> >> actually going to make use of it?
> > 
> > The main reason is I want to make use of it in the related kernel
> > series first. And this requires the Xen header implementation.
> 
> I was about to apply v3, but I'm still unconvinced: How would you
> test those kernel side changes without having anything to set the
> flag?

It does seem odd to be committing to an ABI with no xen.git side
implementation having been posted yet. Normally we ask that things go
into xen.git first before any guests start using them.

Ian.

Re: [PATCH V2] Add flag to start info regarding virtual mapped p2m list

[Jan Beulich]

>>> On 04.03.15 at 10:35, <ian.campbell@citrix.com> wrote:
> On Wed, 2015-03-04 at 08:58 +0000, Jan Beulich wrote:
>> >>> On 03.03.15 at 11:32, <JGross@suse.com> wrote:
>> > On 03/03/2015 11:27 AM, Jan Beulich wrote:
>> >>>>> On 03.03.15 at 10:29, <"jgross@suse.com".non-mime.internet> wrote:
>> >>> In order to indicate the Xen tools capability to support the virtual
>> >>> mapped linear p2m list instead the 3 level mfn tree add a flag to the
>> >>> start_info page.
>> >>>
>> >>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> >>> ---
>> >>>   xen/include/public/xen.h | 2 ++
>> >>>   1 file changed, 2 insertions(+)
>> >>>
>> >>> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
>> >>> index 3703c39..36c6d62 100644
>> >>> --- a/xen/include/public/xen.h
>> >>> +++ b/xen/include/public/xen.h
>> >>> @@ -777,6 +777,8 @@ typedef struct start_info start_info_t;
>> >>>   #define SIF_INITDOMAIN    (1<<1)  /* Is this the initial control domain? */
>> >>>   #define SIF_MULTIBOOT_MOD (1<<2)  /* Is mod_start a multiboot module? */
>> >>>   #define SIF_MOD_START_PFN (1<<3)  /* Is mod_start a PFN? */
>> >>> +#define SIF_VIRT_P2M      (1<<4)  /* Does Xen understand a virt. mapped P->M 
>> > */
>> >>> +                                  /* making the 3 level tree obsolete?     
>  
>> >  */
>> >>>   #define SIF_PM_MASK       (0xFF<<8) /* reserve 1 byte for xen-pm options */
>> >>>
>> >>>   /*
>> >>
>> >> Is there any reason why this can't be part of the tools patch (series)
>> >> actually going to make use of it?
>> > 
>> > The main reason is I want to make use of it in the related kernel
>> > series first. And this requires the Xen header implementation.
>> 
>> I was about to apply v3, but I'm still unconvinced: How would you
>> test those kernel side changes without having anything to set the
>> flag?
> 
> It does seem odd to be committing to an ABI with no xen.git side
> implementation having been posted yet. Normally we ask that things go
> into xen.git first before any guests start using them.

Your reply seems ambiguous to me: If it was sent to Jürgen (with
me Cc-ed) I'd read it as supporting my earlier statement. Since,
however, it was sent to me (with Jürgen Cc-ed), I could also read
it as supporting the public header change alone to go in (even if in
slight collision with the word "implementation" in there). Could you
clarify?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH V2] Add flag to start info regarding virtual mapped p2m list

[Ian Campbell]

On Wed, 2015-03-04 at 09:42 +0000, Jan Beulich wrote:
> >>> On 04.03.15 at 10:35, <ian.campbell@citrix.com> wrote:
> > On Wed, 2015-03-04 at 08:58 +0000, Jan Beulich wrote:
> >> >>> On 03.03.15 at 11:32, <JGross@suse.com> wrote:
> >> > On 03/03/2015 11:27 AM, Jan Beulich wrote:
> >> >>>>> On 03.03.15 at 10:29, <"jgross@suse.com".non-mime.internet> wrote:
> >> >>> In order to indicate the Xen tools capability to support the virtual
> >> >>> mapped linear p2m list instead the 3 level mfn tree add a flag to the
> >> >>> start_info page.
> >> >>>
> >> >>> Signed-off-by: Juergen Gross <jgross@suse.com>
> >> >>> ---
> >> >>>   xen/include/public/xen.h | 2 ++
> >> >>>   1 file changed, 2 insertions(+)
> >> >>>
> >> >>> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> >> >>> index 3703c39..36c6d62 100644
> >> >>> --- a/xen/include/public/xen.h
> >> >>> +++ b/xen/include/public/xen.h
> >> >>> @@ -777,6 +777,8 @@ typedef struct start_info start_info_t;
> >> >>>   #define SIF_INITDOMAIN    (1<<1)  /* Is this the initial control domain? */
> >> >>>   #define SIF_MULTIBOOT_MOD (1<<2)  /* Is mod_start a multiboot module? */
> >> >>>   #define SIF_MOD_START_PFN (1<<3)  /* Is mod_start a PFN? */
> >> >>> +#define SIF_VIRT_P2M      (1<<4)  /* Does Xen understand a virt. mapped P->M 
> >> > */
> >> >>> +                                  /* making the 3 level tree obsolete?     
> >  
> >> >  */
> >> >>>   #define SIF_PM_MASK       (0xFF<<8) /* reserve 1 byte for xen-pm options */
> >> >>>
> >> >>>   /*
> >> >>
> >> >> Is there any reason why this can't be part of the tools patch (series)
> >> >> actually going to make use of it?
> >> > 
> >> > The main reason is I want to make use of it in the related kernel
> >> > series first. And this requires the Xen header implementation.
> >> 
> >> I was about to apply v3, but I'm still unconvinced: How would you
> >> test those kernel side changes without having anything to set the
> >> flag?
> > 
> > It does seem odd to be committing to an ABI with no xen.git side
> > implementation having been posted yet. Normally we ask that things go
> > into xen.git first before any guests start using them.
> 
> Your reply seems ambiguous to me: If it was sent to Jürgen (with
> me Cc-ed) I'd read it as supporting my earlier statement. Since,
> however, it was sent to me (with Jürgen Cc-ed), I could also read
> it as supporting the public header change alone to go in (even if in
> slight collision with the word "implementation" in there). Could you
> clarify?

Sorry, I was in support of you (Jan) here.

Sometime an ABI header change might be all which is needed before guests
start using things (e.g. an io protocol change), but I think in this
case we really need to at least have seen the complete solution (so any
relevant Xen/tools changes) before we commit to an ABI.

It _might_ be sufficient if this patch included some documentation about
what this flag actually means, but best would be to see the actual tools
side (or even a design, sorry if I've missed this somewhere along the
line).

What I don't want to happen is for me to request a change during tools
review only to be told "kernels in the field already do it this way".

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH V2] Add flag to start info regarding virtual mapped p2m list

[Juergen Gross]

On 03/04/2015 11:06 AM, Ian Campbell wrote:
> On Wed, 2015-03-04 at 09:42 +0000, Jan Beulich wrote:
>>>>> On 04.03.15 at 10:35, <ian.campbell@citrix.com> wrote:
>>> On Wed, 2015-03-04 at 08:58 +0000, Jan Beulich wrote:
>>>>>>> On 03.03.15 at 11:32, <JGross@suse.com> wrote:
>>>>> On 03/03/2015 11:27 AM, Jan Beulich wrote:
>>>>>>>>> On 03.03.15 at 10:29, <"jgross@suse.com".non-mime.internet> wrote:
>>>>>>> In order to indicate the Xen tools capability to support the virtual
>>>>>>> mapped linear p2m list instead the 3 level mfn tree add a flag to the
>>>>>>> start_info page.
>>>>>>>
>>>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>>>> ---
>>>>>>>    xen/include/public/xen.h | 2 ++
>>>>>>>    1 file changed, 2 insertions(+)
>>>>>>>
>>>>>>> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
>>>>>>> index 3703c39..36c6d62 100644
>>>>>>> --- a/xen/include/public/xen.h
>>>>>>> +++ b/xen/include/public/xen.h
>>>>>>> @@ -777,6 +777,8 @@ typedef struct start_info start_info_t;
>>>>>>>    #define SIF_INITDOMAIN    (1<<1)  /* Is this the initial control domain? */
>>>>>>>    #define SIF_MULTIBOOT_MOD (1<<2)  /* Is mod_start a multiboot module? */
>>>>>>>    #define SIF_MOD_START_PFN (1<<3)  /* Is mod_start a PFN? */
>>>>>>> +#define SIF_VIRT_P2M      (1<<4)  /* Does Xen understand a virt. mapped P->M
>>>>> */
>>>>>>> +                                  /* making the 3 level tree obsolete?
>>>
>>>>>   */
>>>>>>>    #define SIF_PM_MASK       (0xFF<<8) /* reserve 1 byte for xen-pm options */
>>>>>>>
>>>>>>>    /*
>>>>>>
>>>>>> Is there any reason why this can't be part of the tools patch (series)
>>>>>> actually going to make use of it?
>>>>>
>>>>> The main reason is I want to make use of it in the related kernel
>>>>> series first. And this requires the Xen header implementation.
>>>>
>>>> I was about to apply v3, but I'm still unconvinced: How would you
>>>> test those kernel side changes without having anything to set the
>>>> flag?
>>>
>>> It does seem odd to be committing to an ABI with no xen.git side
>>> implementation having been posted yet. Normally we ask that things go
>>> into xen.git first before any guests start using them.
>>
>> Your reply seems ambiguous to me: If it was sent to Jürgen (with
>> me Cc-ed) I'd read it as supporting my earlier statement. Since,
>> however, it was sent to me (with Jürgen Cc-ed), I could also read
>> it as supporting the public header change alone to go in (even if in
>> slight collision with the word "implementation" in there). Could you
>> clarify?
>
> Sorry, I was in support of you (Jan) here.
>
> Sometime an ABI header change might be all which is needed before guests
> start using things (e.g. an io protocol change), but I think in this
> case we really need to at least have seen the complete solution (so any
> relevant Xen/tools changes) before we commit to an ABI.
>
> It _might_ be sufficient if this patch included some documentation about
> what this flag actually means, but best would be to see the actual tools
> side (or even a design, sorry if I've missed this somewhere along the
> line).
>
> What I don't want to happen is for me to request a change during tools
> review only to be told "kernels in the field already do it this way".

I'd like to do an appropriate change in xl, but I've been told this
would make sense only for migration protocol V2. OTOH I don't want to
wait for an undefined amount of time until this will be posted, so I
sent the ABI change first.

I could, of course, wait with the flag bit until xl is ready and post
another kernel patch then. Unfortunately this would delay Linux support
for automatically be able to run as a pv-domain >500GB further, so I
strongly recommend accepting the interface change now.


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH V2] Add flag to start info regarding virtual mapped p2m list

[Ian Campbell]

On Wed, 2015-03-04 at 11:20 +0100, Juergen Gross wrote:
> On 03/04/2015 11:06 AM, Ian Campbell wrote:
> > On Wed, 2015-03-04 at 09:42 +0000, Jan Beulich wrote:
> >>>>> On 04.03.15 at 10:35, <ian.campbell@citrix.com> wrote:
> >>> On Wed, 2015-03-04 at 08:58 +0000, Jan Beulich wrote:
> >>>>>>> On 03.03.15 at 11:32, <JGross@suse.com> wrote:
> >>>>> On 03/03/2015 11:27 AM, Jan Beulich wrote:
> >>>>>>>>> On 03.03.15 at 10:29, <"jgross@suse.com".non-mime.internet> wrote:
> >>>>>>> In order to indicate the Xen tools capability to support the virtual
> >>>>>>> mapped linear p2m list instead the 3 level mfn tree add a flag to the
> >>>>>>> start_info page.
> >>>>>>>
> >>>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
> >>>>>>> ---
> >>>>>>>    xen/include/public/xen.h | 2 ++
> >>>>>>>    1 file changed, 2 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> >>>>>>> index 3703c39..36c6d62 100644
> >>>>>>> --- a/xen/include/public/xen.h
> >>>>>>> +++ b/xen/include/public/xen.h
> >>>>>>> @@ -777,6 +777,8 @@ typedef struct start_info start_info_t;
> >>>>>>>    #define SIF_INITDOMAIN    (1<<1)  /* Is this the initial control domain? */
> >>>>>>>    #define SIF_MULTIBOOT_MOD (1<<2)  /* Is mod_start a multiboot module? */
> >>>>>>>    #define SIF_MOD_START_PFN (1<<3)  /* Is mod_start a PFN? */
> >>>>>>> +#define SIF_VIRT_P2M      (1<<4)  /* Does Xen understand a virt. mapped P->M
> >>>>> */
> >>>>>>> +                                  /* making the 3 level tree obsolete?
> >>>
> >>>>>   */
> >>>>>>>    #define SIF_PM_MASK       (0xFF<<8) /* reserve 1 byte for xen-pm options */
> >>>>>>>
> >>>>>>>    /*
> >>>>>>
> >>>>>> Is there any reason why this can't be part of the tools patch (series)
> >>>>>> actually going to make use of it?
> >>>>>
> >>>>> The main reason is I want to make use of it in the related kernel
> >>>>> series first. And this requires the Xen header implementation.
> >>>>
> >>>> I was about to apply v3, but I'm still unconvinced: How would you
> >>>> test those kernel side changes without having anything to set the
> >>>> flag?
> >>>
> >>> It does seem odd to be committing to an ABI with no xen.git side
> >>> implementation having been posted yet. Normally we ask that things go
> >>> into xen.git first before any guests start using them.
> >>
> >> Your reply seems ambiguous to me: If it was sent to Jürgen (with
> >> me Cc-ed) I'd read it as supporting my earlier statement. Since,
> >> however, it was sent to me (with Jürgen Cc-ed), I could also read
> >> it as supporting the public header change alone to go in (even if in
> >> slight collision with the word "implementation" in there). Could you
> >> clarify?
> >
> > Sorry, I was in support of you (Jan) here.
> >
> > Sometime an ABI header change might be all which is needed before guests
> > start using things (e.g. an io protocol change), but I think in this
> > case we really need to at least have seen the complete solution (so any
> > relevant Xen/tools changes) before we commit to an ABI.
> >
> > It _might_ be sufficient if this patch included some documentation about
> > what this flag actually means, but best would be to see the actual tools
> > side (or even a design, sorry if I've missed this somewhere along the
> > line).
> >
> > What I don't want to happen is for me to request a change during tools
> > review only to be told "kernels in the field already do it this way".
> 
> I'd like to do an appropriate change in xl, but I've been told this
> would make sense only for migration protocol V2. OTOH I don't want to
> wait for an undefined amount of time until this will be posted, so I
> sent the ABI change first.
> 
> I could, of course, wait with the flag bit until xl is ready and post
> another kernel patch then. Unfortunately this would delay Linux support
> for automatically be able to run as a pv-domain >500GB further, so I
> strongly recommend accepting the interface change now.

Please at least sketch out a design/description of what this flag means
to the guest and/or tools and what eventual tools support you expect
will be needed, and perhaps some ideas regarding what that support might
look like.

Without this your proposed ABI change is just a random bit in a data
structure with no context.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH V2] Add flag to start info regarding virtual mapped p2m list

[Tim Deegan]

At 10:52 +0000 on 04 Mar (1425462776), Ian Campbell wrote:
> On Wed, 2015-03-04 at 11:20 +0100, Juergen Gross wrote:
> > I'd like to do an appropriate change in xl, but I've been told this
> > would make sense only for migration protocol V2. OTOH I don't want to
> > wait for an undefined amount of time until this will be posted, so I
> > sent the ABI change first.
> >
> > I could, of course, wait with the flag bit until xl is ready and post
> > another kernel patch then. Unfortunately this would delay Linux support
> > for automatically be able to run as a pv-domain >500GB further, so I
> > strongly recommend accepting the interface change now.
> 
> Please at least sketch out a design/description of what this flag means
> to the guest and/or tools and what eventual tools support you expect
> will be needed, and perhaps some ideas regarding what that support might
> look like.
> 
> Without this your proposed ABI change is just a random bit in a data
> structure with no context.

If this is just an ordering constraint between kernel and tools work,
maybe we could just define the bit as reserved for now, while the dev work
finishes, and give it a proper name when the toolstack bits go in.

Tim.

Re: [PATCH V2] Add flag to start info regarding virtual mapped p2m list

[Juergen Gross]

On 03/04/2015 12:18 PM, Tim Deegan wrote:
> At 10:52 +0000 on 04 Mar (1425462776), Ian Campbell wrote:
>> On Wed, 2015-03-04 at 11:20 +0100, Juergen Gross wrote:
>>> I'd like to do an appropriate change in xl, but I've been told this
>>> would make sense only for migration protocol V2. OTOH I don't want to
>>> wait for an undefined amount of time until this will be posted, so I
>>> sent the ABI change first.
>>>
>>> I could, of course, wait with the flag bit until xl is ready and post
>>> another kernel patch then. Unfortunately this would delay Linux support
>>> for automatically be able to run as a pv-domain >500GB further, so I
>>> strongly recommend accepting the interface change now.
>>
>> Please at least sketch out a design/description of what this flag means
>> to the guest and/or tools and what eventual tools support you expect
>> will be needed, and perhaps some ideas regarding what that support might
>> look like.
>>
>> Without this your proposed ABI change is just a random bit in a data
>> structure with no context.
>
> If this is just an ordering constraint between kernel and tools work,
> maybe we could just define the bit as reserved for now, while the dev work
> finishes, and give it a proper name when the toolstack bits go in.

I don't think this will be a proper solution. Using this bit in the
kernel would either be okay in case the interface is added later to Xen,
or it would be not okay making it impossible later to use that bit for
other purposes. In both cases we could easily name the bit like I did.
It would either be used like intended, or it would never be set thus
doing no harm.

Juergen

Re: [PATCH V2] Add flag to start info regarding virtual mapped p2m list

[Ian Campbell]

On Wed, 2015-03-04 at 12:22 +0100, Juergen Gross wrote:
> It would either be used like intended,

Which is how? That is what is really missing here.

So far this appears to be a bit which enables some as yet unspecified[0]
behaviour in one particular OS kernel with some as yet undiscussed
potential future impact on toolstack functionality.

Ian.

[0] Apart from an implementation in Linux, which is not a specification.

Re: [PATCH V2] Add flag to start info regarding virtual mapped p2m list

[Juergen Gross]

On 03/04/2015 12:41 PM, Ian Campbell wrote:
> On Wed, 2015-03-04 at 12:22 +0100, Juergen Gross wrote:
>> It would either be used like intended,
>
> Which is how? That is what is really missing here.
>
> So far this appears to be a bit which enables some as yet unspecified[0]
> behaviour in one particular OS kernel with some as yet undiscussed
> potential future impact on toolstack functionality.

Sorry for the late answer, but I managed to move this mail to my archive
before reacting on it. :-(

Xen pv domains are using a domain private p2m list to convert guest pfns
to mfns. This p2m list has to be updated by the Xen tools during domain
restore and migration, as the mfns will most likely change. In order to
locate the p2m list the Xen tools need an interface provided by the
guest. Up to now this interface has been the shared info page where the
guest would store the mfn of the top level page of a 3-level p2m tree.

This p2m tree is fixed in it's layout and due to the limitation of
entries it can hold at each level it is limiting the maximum size of the
p2m list which can be reported to the Xen tools. The maximum memory the
p2m tree can support for 64 bit domains is 512 GB (32 bit domains don't
have a problem, as the p2m tree limit is much higher than the supported
domain size of 64 GB).

In order to be able to support pv domains with more than 512 GB an
additional way to specify the p2m list for the Xen tools has been added:
instead of a tree structure linked via mfns, the virtual address of a
linear p2m list, the cr3 value of the related address space and the size
of the p2m list can be specified by the guest.

Guests implementing this new interface need to know, of course, whether
the Xen tools are capable to use the new interface instead of the old
p2m tree interface. Otherwise a guest using only the new interface with
the virtual mapped linear p2m list on a machine with old Xen tools not
supporting this interface could not be restored or migrated.

The added flag in the start info indicates the Xen tool's capability to
use the new interface enabling the guest to omit the p2m tree and thus
to support more than 512 GB of RAM.


Juergen

Re: [PATCH V2] Add flag to start info regarding virtual mapped p2m list

[Ian Campbell]

On Tue, 2015-03-17 at 06:50 +0100, Juergen Gross wrote:
> On 03/04/2015 12:41 PM, Ian Campbell wrote:
> > On Wed, 2015-03-04 at 12:22 +0100, Juergen Gross wrote:
> >> It would either be used like intended,
> >
> > Which is how? That is what is really missing here.
> >
> > So far this appears to be a bit which enables some as yet unspecified[0]
> > behaviour in one particular OS kernel with some as yet undiscussed
> > potential future impact on toolstack functionality.
> 
> Sorry for the late answer, but I managed to move this mail to my archive
> before reacting on it. :-(

No problem.

The description looks like the sort of thing which ought to go into the
commit log.

Is there not an ABI change somewhere else relating to the exposure of
the cr3+vaddr+size? If so why is it not in this patch?

Ideally whichever file which needs to change in xen/include/public to
expose that change should also come along with documentation for this
new ABI.

If that change has been deferred for some reason then I think it (and
why) should be mentioned in the commit message, you'll also want to
explain why adding the bit now but the ABI change later is safe, i.e.
what the transition plan is.

AIUI this change has broken memory hotplug and has also made it
difficult from an ABI PoV to reinstate that support. I think that needs
to be addressed (i.e. at the ABI design level, not necessary
implemented) before we add a bit exposing this feature.

> Guests implementing this new interface need to know, of course, whether
> the Xen tools are capable to use the new interface instead of the old
> p2m tree interface. Otherwise a guest using only the new interface with
> the virtual mapped linear p2m list on a machine with old Xen tools not
> supporting this interface could not be restored or migrated.

What is the mechanism for the converse, i.e. for the tools to detect if
a given guest is making use of this functionality?

Are tools going to be able to tell at migration time, or is it something
which needs to be detectable at domain build time (and therefore exposed
in the kernel ELF notes)?

Ian.

Re: [PATCH V2] Add flag to start info regarding virtual mapped p2m list

[Juergen Gross]

On 03/18/2015 10:59 AM, Ian Campbell wrote:
> On Tue, 2015-03-17 at 06:50 +0100, Juergen Gross wrote:
>> On 03/04/2015 12:41 PM, Ian Campbell wrote:
>>> On Wed, 2015-03-04 at 12:22 +0100, Juergen Gross wrote:
>>>> It would either be used like intended,
>>>
>>> Which is how? That is what is really missing here.
>>>
>>> So far this appears to be a bit which enables some as yet unspecified[0]
>>> behaviour in one particular OS kernel with some as yet undiscussed
>>> potential future impact on toolstack functionality.
>>
>> Sorry for the late answer, but I managed to move this mail to my archive
>> before reacting on it. :-(
>
> No problem.
>
> The description looks like the sort of thing which ought to go into the
> commit log.

Okay, I can change it.

> Is there not an ABI change somewhere else relating to the exposure of
> the cr3+vaddr+size? If so why is it not in this patch?

Commit 50bd1f0825339dfacde471df7664729216fc46e3

> Ideally whichever file which needs to change in xen/include/public to
> expose that change should also come along with documentation for this
> new ABI.

Included in above commit in form of comments in the modified file.

> If that change has been deferred for some reason then I think it (and
> why) should be mentioned in the commit message, you'll also want to
> explain why adding the bit now but the ABI change later is safe, i.e.
> what the transition plan is.
>
> AIUI this change has broken memory hotplug and has also made it
> difficult from an ABI PoV to reinstate that support. I think that needs
> to be addressed (i.e. at the ABI design level, not necessary
> implemented) before we add a bit exposing this feature.

The interface change didn't brake anything. It was the implementation in
the Linux kernel.

>> Guests implementing this new interface need to know, of course, whether
>> the Xen tools are capable to use the new interface instead of the old
>> p2m tree interface. Otherwise a guest using only the new interface with
>> the virtual mapped linear p2m list on a machine with old Xen tools not
>> supporting this interface could not be restored or migrated.
>
> What is the mechanism for the converse, i.e. for the tools to detect if
> a given guest is making use of this functionality?

See above commit. It's all in the comments.

> Are tools going to be able to tell at migration time, or is it something
> which needs to be detectable at domain build time (and therefore exposed
> in the kernel ELF notes)?

Migration time is fine.


Juergen

Re: [PATCH V2] Add flag to start info regarding virtual mapped p2m list

[Ian Campbell]

On Wed, 2015-03-18 at 11:59 +0100, Juergen Gross wrote:

> > Is there not an ABI change somewhere else relating to the exposure of
> > the cr3+vaddr+size? If so why is it not in this patch?
> 
> Commit 50bd1f0825339dfacde471df7664729216fc46e3
> 
> > Ideally whichever file which needs to change in xen/include/public to
> > expose that change should also come along with documentation for this
> > new ABI.
> 
> Included in above commit in form of comments in the modified file.

Great, please mention in the commit log here that this is building on
that work (would have saved me having to ask).

> > If that change has been deferred for some reason then I think it (and
> > why) should be mentioned in the commit message, you'll also want to
> > explain why adding the bit now but the ABI change later is safe, i.e.
> > what the transition plan is.
> >
> > AIUI this change has broken memory hotplug and has also made it
> > difficult from an ABI PoV to reinstate that support. I think that needs
> > to be addressed (i.e. at the ABI design level, not necessary
> > implemented) before we add a bit exposing this feature.
> 
> The interface change didn't brake anything. It was the implementation in
> the Linux kernel.

AIUI the new interface has made it difficult to for OS kernels to
arrange to be able to grow their P2M. Whether that is an "OS kernel
issues" or an "interface issue" isn't really the point, the fact is that
for whatever reason it is now difficult to arrange.

Is there a plan for how this might be dealt with?

Ian.

Re: [PATCH V2] Add flag to start info regarding virtual mapped p2m list

[David Vrabel]

On 04/03/15 10:20, Juergen Gross wrote:
> 
> I could, of course, wait with the flag bit until xl is ready and post
> another kernel patch then. Unfortunately this would delay Linux support
> for automatically be able to run as a pv-domain >500GB further, so I
> strongly recommend accepting the interface change now.

I suggested before:

"If dom0, enable >512G.
If domU, enable >512G if requested by command line option /or/ toolstack
indicates that it supports the linear p2m."

So this flag is only required to /automatically/ enable >512GB PV
guests.  You still need the manual (command line) override so this flag
isn't needed until the toolstack actually supports the linear p2m, and
the kernel support for this automatic enablement can be added later as well.

David

Re: [PATCH V2] Add flag to start info regarding virtual mapped p2m list

[Juergen Gross]

On 03/04/2015 11:59 AM, David Vrabel wrote:
> On 04/03/15 10:20, Juergen Gross wrote:
>>
>> I could, of course, wait with the flag bit until xl is ready and post
>> another kernel patch then. Unfortunately this would delay Linux support
>> for automatically be able to run as a pv-domain >500GB further, so I
>> strongly recommend accepting the interface change now.
>
> I suggested before:
>
> "If dom0, enable >512G.
> If domU, enable >512G if requested by command line option /or/ toolstack
> indicates that it supports the linear p2m."
>
> So this flag is only required to /automatically/ enable >512GB PV
> guests.  You still need the manual (command line) override so this flag
> isn't needed until the toolstack actually supports the linear p2m, and
> the kernel support for this automatic enablement can be added later as well.

Correct. The question is, whether we want some kernel versions capable
of running as 1TB guest requiring an additional boot parameter to do so.
Avoiding this is easy, but if you don't like it, I can wait with this
patch.


Juergen

Re: [PATCH V2] Add flag to start info regarding virtual mapped p2m list

[David Vrabel]

On 04/03/15 11:09, Juergen Gross wrote:
> On 03/04/2015 11:59 AM, David Vrabel wrote:
>> On 04/03/15 10:20, Juergen Gross wrote:
>>>
>>> I could, of course, wait with the flag bit until xl is ready and post
>>> another kernel patch then. Unfortunately this would delay Linux support
>>> for automatically be able to run as a pv-domain >500GB further, so I
>>> strongly recommend accepting the interface change now.
>>
>> I suggested before:
>>
>> "If dom0, enable >512G.
>> If domU, enable >512G if requested by command line option /or/ toolstack
>> indicates that it supports the linear p2m."
>>
>> So this flag is only required to /automatically/ enable >512GB PV
>> guests.  You still need the manual (command line) override so this flag
>> isn't needed until the toolstack actually supports the linear p2m, and
>> the kernel support for this automatic enablement can be added later as
>> well.
> 
> Correct. The question is, whether we want some kernel versions capable
> of running as 1TB guest requiring an additional boot parameter to do so.
> Avoiding this is easy, but if you don't like it, I can wait with this
> patch.

I think running such large PV domUs is a sufficiently rare use case that
this is fine.

David