Re: [Xen-devel] [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table
On 08/08/18 14:15, Paul Durrant wrote:
>>
>> @@ -3906,6 +3943,38 @@ int gnttab_map_frame(struct domain *d,
> unsigned long idx, gfn_t gfn,
>> return rc;
>> }
>>
>> +int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
>> +mfn_t *mfn)
>> +{
>> +struct grant_table *gt = d->grant_table;
>> +int rc;
>> +
>> +grant_write_lock(gt);
>> +
>> +if ( gt->gt_version == 0 )
>> +gt->gt_version = 1;
> Since you've moved this here instead of dropping it, what requirement
> have you found for this to be set (other than the ASSERT() you put in
> gnttab_get_shared_frame_mfn()?
>
The code in patch #2 is executed before the grant table version is set. I
could alternatively have libxl explicitly set the version to 1 before
trying
to seed the table.
>>> But that's not my point. What's wrong with leaving it at zero?
>>>
>> I'm not particularly happy calling gnttab_grow_table() with version left at 0
>> but I can try it and see if it breaks.
> Actually, no. There is nowhere else that leaves it at 0 and I find that I
> can't set the version explicitly from the toolstack as gnttab_set_version
> doesn't take a domid as a parameter. I'll leave the version setting as-is.
Yeah - this looks like the best option for now, and I'll fix the
defaulting-to-1 in due course.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table
> -Original Message-
> From: Xen-devel [mailto:xen-devel-boun...@lists.xenproject.org] On Behalf
> Of Paul Durrant
> Sent: 08 August 2018 12:33
> To: 'Jan Beulich'
> Cc: Stefano Stabellini ; Wei Liu
> ; Andrew Cooper ; Tim
> (Xen.org) ; George Dunlap ; Ian
> Jackson ; xen-devel de...@lists.xenproject.org>
> Subject: Re: [Xen-devel] [PATCH v21 1/2] common: add a new mappable
> resource type: XENMEM_resource_grant_table
>
> > -Original Message-
> > From: Jan Beulich [mailto:jbeul...@suse.com]
> > Sent: 08 August 2018 11:43
> > To: Paul Durrant
> > Cc: Andrew Cooper ; George Dunlap
> > ; Ian Jackson ; Wei
> Liu
> > ; Stefano Stabellini ; xen-
> > devel ; Konrad Rzeszutek Wilk
> > ; Tim (Xen.org)
> > Subject: RE: [PATCH v21 1/2] common: add a new mappable resource type:
> > XENMEM_resource_grant_table
> >
> > >>> On 08.08.18 at 12:38, wrote:
> > >> From: Jan Beulich [mailto:jbeul...@suse.com]
> > >> Sent: 08 August 2018 11:30
> > >>
> > >> >>> On 08.08.18 at 11:00, wrote:
> > >> > @@ -3860,6 +3866,47 @@ int mem_sharing_gref_to_gfn(struct
> > >> grant_table *gt, grant_ref_t ref,
> > >> > }
> > >> > #endif
> > >> >
> > >> > +/* caller must hold write lock */
> > >> > +static int gnttab_get_status_frame_mfn(struct domain *d,
> > >> > + unsigned long idx, mfn_t *mfn)
> > >> > +{
> > >> > +struct grant_table *gt = d->grant_table;
> > >>
> > >> const?
> > >>
> > >
> > > IIRC that didn't work because gnttab_grow_table() modifies the content.
> >
> > But you don't pass gt to the function:
> >
> > >> > +ASSERT(gt->gt_version == 2);
> > >> > +
> > >> > +if ( idx >= nr_status_frames(gt) )
> > >> > +{
> > >> > +unsigned long nr = status_to_grant_frames(idx + 1);
> > >> > +
> > >> > +if ( nr <= gt->max_grant_frames )
> > >> > +gnttab_grow_table(d, nr);
> >
> > ^^^
>
> I know, I was just remembering that the compiler complained when I tried
> this before. My memory could be wrong so I'll try it again.
>
> >
> > >> > @@ -3906,6 +3943,38 @@ int gnttab_map_frame(struct domain *d,
> > >> unsigned long idx, gfn_t gfn,
> > >> > return rc;
> > >> > }
> > >> >
> > >> > +int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
> > >> > +mfn_t *mfn)
> > >> > +{
> > >> > +struct grant_table *gt = d->grant_table;
> > >> > +int rc;
> > >> > +
> > >> > +grant_write_lock(gt);
> > >> > +
> > >> > +if ( gt->gt_version == 0 )
> > >> > +gt->gt_version = 1;
> > >>
> > >> Since you've moved this here instead of dropping it, what requirement
> > >> have you found for this to be set (other than the ASSERT() you put in
> > >> gnttab_get_shared_frame_mfn()?
> > >>
> > >
> > > The code in patch #2 is executed before the grant table version is set. I
> > > could alternatively have libxl explicitly set the version to 1 before
> > > trying
> > > to seed the table.
> >
> > But that's not my point. What's wrong with leaving it at zero?
> >
>
> I'm not particularly happy calling gnttab_grow_table() with version left at 0
> but I can try it and see if it breaks.
Actually, no. There is nowhere else that leaves it at 0 and I find that I can't
set the version explicitly from the toolstack as gnttab_set_version doesn't
take a domid as a parameter. I'll leave the version setting as-is.
Paul
>
> Paul
>
> > Jan
> >
>
>
> ___
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table
> -Original Message-
> From: Andrew Cooper
> Sent: 08 August 2018 12:34
> To: Jan Beulich
> Cc: George Dunlap ; Ian Jackson
> ; Paul Durrant ; Wei Liu
> ; Stefano Stabellini ; xen-
> devel ; Konrad Rzeszutek Wilk
> ; Tim (Xen.org)
> Subject: Re: [PATCH v21 1/2] common: add a new mappable resource type:
> XENMEM_resource_grant_table
>
> On 08/08/18 11:55, Jan Beulich wrote:
> On 08.08.18 at 12:46, wrote:
> >> On 08/08/18 11:43, Jan Beulich wrote:
> >> On 08.08.18 at 12:38, wrote:
> > From: Jan Beulich [mailto:jbeul...@suse.com]
> > Sent: 08 August 2018 11:30
> >
> On 08.08.18 at 11:00, wrote:
> >> +int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
> >> +mfn_t *mfn)
> >> +{
> >> +struct grant_table *gt = d->grant_table;
> >> +int rc;
> >> +
> >> +grant_write_lock(gt);
> >> +
> >> +if ( gt->gt_version == 0 )
> >> +gt->gt_version = 1;
> > Since you've moved this here instead of dropping it, what
> requirement
> > have you found for this to be set (other than the ASSERT() you put in
> > gnttab_get_shared_frame_mfn()?
> >
> The code in patch #2 is executed before the grant table version is set. I
> could alternatively have libxl explicitly set the version to 1 before
> trying
> to seed the table.
> >>> But that's not my point. What's wrong with leaving it at zero?
> >> On a tangent, why does a gnttab version of 0 exist at all? Why don't we
> >> explicitly initialise it to 1 in the hypervisor?
> > Fair question, which unfortunately I can't answer.
> >
> >> We've had plenty of XSAs to do with mishandling of a gnttab version of
> >> 0. Why not fix the problem at its source, and simplify the gnttab code
> >> while we are at it.
> > I don't mind, unless a reason for the seemingly strange behavior can be
> > found.
>
> gt_version only came in with grant table v2, so the toolstack never
> previously set a version. That ended up with cases where dom0 tries to
> map the store/cons grants before the guest driver has chosen a version.
>
> I expect its like this because grant table v2 was a giant pile of poorly
> reviewed hacks, rather than for any better reason.
>
> If noone else gets to it, I'll fold it into my series to mess thoroughly
> with parameter handling in DOMCTL_createdomain.
If that's going to take a while then I can add the explicit version set in
patch #2.
Paul
>
> ~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table
On 08/08/18 11:55, Jan Beulich wrote:
On 08.08.18 at 12:46, wrote:
>> On 08/08/18 11:43, Jan Beulich wrote:
>> On 08.08.18 at 12:38, wrote:
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 08 August 2018 11:30
>
On 08.08.18 at 11:00, wrote:
>> +int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
>> +mfn_t *mfn)
>> +{
>> +struct grant_table *gt = d->grant_table;
>> +int rc;
>> +
>> +grant_write_lock(gt);
>> +
>> +if ( gt->gt_version == 0 )
>> +gt->gt_version = 1;
> Since you've moved this here instead of dropping it, what requirement
> have you found for this to be set (other than the ASSERT() you put in
> gnttab_get_shared_frame_mfn()?
>
The code in patch #2 is executed before the grant table version is set. I
could alternatively have libxl explicitly set the version to 1 before
trying
to seed the table.
>>> But that's not my point. What's wrong with leaving it at zero?
>> On a tangent, why does a gnttab version of 0 exist at all? Why don't we
>> explicitly initialise it to 1 in the hypervisor?
> Fair question, which unfortunately I can't answer.
>
>> We've had plenty of XSAs to do with mishandling of a gnttab version of
>> 0. Why not fix the problem at its source, and simplify the gnttab code
>> while we are at it.
> I don't mind, unless a reason for the seemingly strange behavior can be
> found.
gt_version only came in with grant table v2, so the toolstack never
previously set a version. That ended up with cases where dom0 tries to
map the store/cons grants before the guest driver has chosen a version.
I expect its like this because grant table v2 was a giant pile of poorly
reviewed hacks, rather than for any better reason.
If noone else gets to it, I'll fold it into my series to mess thoroughly
with parameter handling in DOMCTL_createdomain.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table
> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 08 August 2018 11:43
> To: Paul Durrant
> Cc: Andrew Cooper ; George Dunlap
> ; Ian Jackson ; Wei Liu
> ; Stefano Stabellini ; xen-
> devel ; Konrad Rzeszutek Wilk
> ; Tim (Xen.org)
> Subject: RE: [PATCH v21 1/2] common: add a new mappable resource type:
> XENMEM_resource_grant_table
>
> >>> On 08.08.18 at 12:38, wrote:
> >> From: Jan Beulich [mailto:jbeul...@suse.com]
> >> Sent: 08 August 2018 11:30
> >>
> >> >>> On 08.08.18 at 11:00, wrote:
> >> > @@ -3860,6 +3866,47 @@ int mem_sharing_gref_to_gfn(struct
> >> grant_table *gt, grant_ref_t ref,
> >> > }
> >> > #endif
> >> >
> >> > +/* caller must hold write lock */
> >> > +static int gnttab_get_status_frame_mfn(struct domain *d,
> >> > + unsigned long idx, mfn_t *mfn)
> >> > +{
> >> > +struct grant_table *gt = d->grant_table;
> >>
> >> const?
> >>
> >
> > IIRC that didn't work because gnttab_grow_table() modifies the content.
>
> But you don't pass gt to the function:
>
> >> > +ASSERT(gt->gt_version == 2);
> >> > +
> >> > +if ( idx >= nr_status_frames(gt) )
> >> > +{
> >> > +unsigned long nr = status_to_grant_frames(idx + 1);
> >> > +
> >> > +if ( nr <= gt->max_grant_frames )
> >> > +gnttab_grow_table(d, nr);
>
> ^^^
I know, I was just remembering that the compiler complained when I tried this
before. My memory could be wrong so I'll try it again.
>
> >> > @@ -3906,6 +3943,38 @@ int gnttab_map_frame(struct domain *d,
> >> unsigned long idx, gfn_t gfn,
> >> > return rc;
> >> > }
> >> >
> >> > +int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
> >> > +mfn_t *mfn)
> >> > +{
> >> > +struct grant_table *gt = d->grant_table;
> >> > +int rc;
> >> > +
> >> > +grant_write_lock(gt);
> >> > +
> >> > +if ( gt->gt_version == 0 )
> >> > +gt->gt_version = 1;
> >>
> >> Since you've moved this here instead of dropping it, what requirement
> >> have you found for this to be set (other than the ASSERT() you put in
> >> gnttab_get_shared_frame_mfn()?
> >>
> >
> > The code in patch #2 is executed before the grant table version is set. I
> > could alternatively have libxl explicitly set the version to 1 before trying
> > to seed the table.
>
> But that's not my point. What's wrong with leaving it at zero?
>
I'm not particularly happy calling gnttab_grow_table() with version left at 0
but I can try it and see if it breaks.
Paul
> Jan
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table
> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 08 August 2018 11:47
> To: Andrew Cooper ; Paul Durrant
>
> Cc: Wei Liu ; George Dunlap
> ; Ian Jackson ;
> Stefano Stabellini ; xen-devel de...@lists.xenproject.org>; Konrad Rzeszutek Wilk
> ; Tim (Xen.org)
> Subject: Re: [PATCH v21 1/2] common: add a new mappable resource type:
> XENMEM_resource_grant_table
>
> >>> On 08.08.18 at 12:10, wrote:
> > On 08/08/18 10:00, Paul Durrant wrote:
> >> +static int gnttab_get_status_frame_mfn(struct domain *d,
> >> + unsigned long idx, mfn_t *mfn)
> >> +{
> >> +struct grant_table *gt = d->grant_table;
> >> +
> >> +ASSERT(gt->gt_version == 2);
> >> +
> >> +if ( idx >= nr_status_frames(gt) )
> >> +{
> >> +unsigned long nr = status_to_grant_frames(idx + 1);
> >
> > Why the +1 ? Won't that cause a failure if you attempt to map the
> > maximum valid index?
>
> That's the normal index-of-something to count-of-something
> conversion (or else the table may get grown by too little). I've
> instead been considering the badness of overflow here, but
> decided to leave it uncommented as the check further down
> would at least not make this insecure. However, with ...
>
> >> +
> >> +if ( nr <= gt->max_grant_frames )
> >> +gnttab_grow_table(d, nr);
> >
> > You want to capture the return value of grow_table(), which at least
> > distinguishes between ENODEV and ENOMEM.
> >
> >> +
> >> +if ( idx >= nr_status_frames(gt) )
> >> +return -EINVAL;
> >
> > This can probably(?) be asserted if gnttab_grow_table() returns
> > successfully.
>
> ... these two a potential overflow above would then have a
> chance of triggering the assertion you suggest to add.
>
> As to the grow_table() return value check - I'd prefer if the
> patch here didn't alter original behavior. If we want it altered,
> better in a separate patch.
Ok. I'll leave the return value of gnttab_grow_table() unchecked as-is.
>
> Jan
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table
>>> On 08.08.18 at 12:46, wrote:
> On 08/08/18 11:43, Jan Beulich wrote:
> On 08.08.18 at 12:38, wrote:
From: Jan Beulich [mailto:jbeul...@suse.com]
Sent: 08 August 2018 11:30
>>> On 08.08.18 at 11:00, wrote:
> +int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
> +mfn_t *mfn)
> +{
> +struct grant_table *gt = d->grant_table;
> +int rc;
> +
> +grant_write_lock(gt);
> +
> +if ( gt->gt_version == 0 )
> +gt->gt_version = 1;
Since you've moved this here instead of dropping it, what requirement
have you found for this to be set (other than the ASSERT() you put in
gnttab_get_shared_frame_mfn()?
>>> The code in patch #2 is executed before the grant table version is set. I
>>> could alternatively have libxl explicitly set the version to 1 before
>>> trying
>>> to seed the table.
>> But that's not my point. What's wrong with leaving it at zero?
>
> On a tangent, why does a gnttab version of 0 exist at all? Why don't we
> explicitly initialise it to 1 in the hypervisor?
Fair question, which unfortunately I can't answer.
> We've had plenty of XSAs to do with mishandling of a gnttab version of
> 0. Why not fix the problem at its source, and simplify the gnttab code
> while we are at it.
I don't mind, unless a reason for the seemingly strange behavior can be
found.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table
>>> On 08.08.18 at 12:10, wrote:
> On 08/08/18 10:00, Paul Durrant wrote:
>> +static int gnttab_get_status_frame_mfn(struct domain *d,
>> + unsigned long idx, mfn_t *mfn)
>> +{
>> +struct grant_table *gt = d->grant_table;
>> +
>> +ASSERT(gt->gt_version == 2);
>> +
>> +if ( idx >= nr_status_frames(gt) )
>> +{
>> +unsigned long nr = status_to_grant_frames(idx + 1);
>
> Why the +1 ? Won't that cause a failure if you attempt to map the
> maximum valid index?
That's the normal index-of-something to count-of-something
conversion (or else the table may get grown by too little). I've
instead been considering the badness of overflow here, but
decided to leave it uncommented as the check further down
would at least not make this insecure. However, with ...
>> +
>> +if ( nr <= gt->max_grant_frames )
>> +gnttab_grow_table(d, nr);
>
> You want to capture the return value of grow_table(), which at least
> distinguishes between ENODEV and ENOMEM.
>
>> +
>> +if ( idx >= nr_status_frames(gt) )
>> +return -EINVAL;
>
> This can probably(?) be asserted if gnttab_grow_table() returns
> successfully.
... these two a potential overflow above would then have a
chance of triggering the assertion you suggest to add.
As to the grow_table() return value check - I'd prefer if the
patch here didn't alter original behavior. If we want it altered,
better in a separate patch.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table
On 08/08/18 11:43, Jan Beulich wrote:
On 08.08.18 at 12:38, wrote:
>>> From: Jan Beulich [mailto:jbeul...@suse.com]
>>> Sent: 08 August 2018 11:30
>>>
>> On 08.08.18 at 11:00, wrote:
@@ -3860,6 +3866,47 @@ int mem_sharing_gref_to_gfn(struct
>>> grant_table *gt, grant_ref_t ref,
}
#endif
+/* caller must hold write lock */
+static int gnttab_get_status_frame_mfn(struct domain *d,
+ unsigned long idx, mfn_t *mfn)
+{
+struct grant_table *gt = d->grant_table;
>>> const?
>>>
>> IIRC that didn't work because gnttab_grow_table() modifies the content.
> But you don't pass gt to the function:
>
+ASSERT(gt->gt_version == 2);
+
+if ( idx >= nr_status_frames(gt) )
+{
+unsigned long nr = status_to_grant_frames(idx + 1);
+
+if ( nr <= gt->max_grant_frames )
+gnttab_grow_table(d, nr);
> ^^^
>
@@ -3906,6 +3943,38 @@ int gnttab_map_frame(struct domain *d,
>>> unsigned long idx, gfn_t gfn,
return rc;
}
+int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
+mfn_t *mfn)
+{
+struct grant_table *gt = d->grant_table;
+int rc;
+
+grant_write_lock(gt);
+
+if ( gt->gt_version == 0 )
+gt->gt_version = 1;
>>> Since you've moved this here instead of dropping it, what requirement
>>> have you found for this to be set (other than the ASSERT() you put in
>>> gnttab_get_shared_frame_mfn()?
>>>
>> The code in patch #2 is executed before the grant table version is set. I
>> could alternatively have libxl explicitly set the version to 1 before trying
>> to seed the table.
> But that's not my point. What's wrong with leaving it at zero?
On a tangent, why does a gnttab version of 0 exist at all? Why don't we
explicitly initialise it to 1 in the hypervisor?
We've had plenty of XSAs to do with mishandling of a gnttab version of
0. Why not fix the problem at its source, and simplify the gnttab code
while we are at it.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table
On 08/08/18 11:19, Paul Durrant wrote:
>
>>> +static inline unsigned int status_to_grant_frames(unsigned int
>> status_frames)
>>> +{
>>> +return DIV_ROUND_UP(status_frames * GRANT_STATUS_PER_PAGE,
>> GRANT_PER_PAGE);
>>> +}
>>> +
>>> /* Check if the page has been paged out, or needs unsharing.
>>> If rc == GNTST_okay, *page contains the page struct with a ref taken.
>>> Caller must do put_page(*page).
>>> @@ -3860,6 +3866,47 @@ int mem_sharing_gref_to_gfn(struct
>> grant_table *gt, grant_ref_t ref,
>>> }
>>> #endif
>>>
>>> +/* caller must hold write lock */
>>> +static int gnttab_get_status_frame_mfn(struct domain *d,
>>> + unsigned long idx, mfn_t *mfn)
>>> +{
>>> +struct grant_table *gt = d->grant_table;
>>> +
>>> +ASSERT(gt->gt_version == 2);
>>> +
>>> +if ( idx >= nr_status_frames(gt) )
>>> +{
>>> +unsigned long nr = status_to_grant_frames(idx + 1);
>> Why the +1 ? Won't that cause a failure if you attempt to map the
>> maximum valid index?
> That's kind of the idea...
To force a failure when mapping a legitimate index? Whatever the old
code did, this smells like broken boundary case.
A caller is going to want to map frames 0 to N-1, based on some
knowledge of either how many frames the guest has, or what the total
number of expected frames is. nr_status_frames() OTOH, is 1-based,
which is why this looks wrong.
How about a comment along the lines of
/* idx is 0-based, nr_* is 1-based. */
which might help reduce the confusion?
>>> +
>>> +if ( idx >= nr_grant_frames(gt) )
>>> +return -EINVAL;
>>> +
>>> +*mfn = _mfn(virt_to_mfn(gt->shared_raw[idx]));
>>> +return 0;
>>> +}
>>> +
>>> int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
>>> mfn_t *mfn)
>>> {
>>> diff --git a/xen/common/memory.c b/xen/common/memory.c
>>> index e29d596727..427f65a5e1 100644
>>> --- a/xen/common/memory.c
>>> +++ b/xen/common/memory.c
>>> @@ -23,6 +23,7 @@
>>> #include
>>> #include
>>> #include
>>> +#include
>>> #include
>>> #include
>>> #include
>>> @@ -982,6 +983,43 @@ static long xatp_permission_check(struct domain
>> *d, unsigned int space)
>>> return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
>>> }
>>>
>>> +static int acquire_grant_table(struct domain *d, unsigned int id,
>>> + unsigned long frame,
>>> + unsigned int nr_frames,
>>> + xen_pfn_t mfn_list[])
>>> +{
>>> +unsigned int i = nr_frames;
>>> +
>>> +/* Iterate backwards in case table needs to grow */
>>> +while ( i-- != 0 )
>>> +{
>>> +mfn_t mfn = INVALID_MFN;
>>> +int rc;
>>> +
>>> +switch ( id )
>>> +{
>>> +case XENMEM_resource_grant_table_id_shared:
>>> +rc = gnttab_get_shared_frame(d, frame + i, );
>>> +break;
>>> +
>>> +case XENMEM_resource_grant_table_id_status:
>>> +rc = gnttab_get_status_frame(d, frame + i, );
>>> +break;
>>> +
>>> +default:
>>> +rc = -EINVAL;
>>> +break;
>>> +}
>>> +
>>> +if ( rc )
>> Would it perhaps be prudent to have || mfn_eq(mfn, INVALID_MFN)
>> here? I
>> don't think anything good will come of handing INVALID_MFN back to the
>> caller.
> Why? The value of mfn will be overwritten if rc == 0 and will be left as
> INVALID_MFN if rc != 0. I can ASSERT that if you'd like.
Yeah - an ASSERT() would be nice.
>>> +!is_hardware_domain(currd) )
>>> +return -EOPNOTSUPP;
>> EPERM perhaps?
>>
> I debated that. I'm really not sure which one would be best.
Hmm, nor me. Lets leave it like that for now.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table
>>> On 08.08.18 at 12:38, wrote:
>> From: Jan Beulich [mailto:jbeul...@suse.com]
>> Sent: 08 August 2018 11:30
>>
>> >>> On 08.08.18 at 11:00, wrote:
>> > @@ -3860,6 +3866,47 @@ int mem_sharing_gref_to_gfn(struct
>> grant_table *gt, grant_ref_t ref,
>> > }
>> > #endif
>> >
>> > +/* caller must hold write lock */
>> > +static int gnttab_get_status_frame_mfn(struct domain *d,
>> > + unsigned long idx, mfn_t *mfn)
>> > +{
>> > +struct grant_table *gt = d->grant_table;
>>
>> const?
>>
>
> IIRC that didn't work because gnttab_grow_table() modifies the content.
But you don't pass gt to the function:
>> > +ASSERT(gt->gt_version == 2);
>> > +
>> > +if ( idx >= nr_status_frames(gt) )
>> > +{
>> > +unsigned long nr = status_to_grant_frames(idx + 1);
>> > +
>> > +if ( nr <= gt->max_grant_frames )
>> > +gnttab_grow_table(d, nr);
^^^
>> > @@ -3906,6 +3943,38 @@ int gnttab_map_frame(struct domain *d,
>> unsigned long idx, gfn_t gfn,
>> > return rc;
>> > }
>> >
>> > +int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
>> > +mfn_t *mfn)
>> > +{
>> > +struct grant_table *gt = d->grant_table;
>> > +int rc;
>> > +
>> > +grant_write_lock(gt);
>> > +
>> > +if ( gt->gt_version == 0 )
>> > +gt->gt_version = 1;
>>
>> Since you've moved this here instead of dropping it, what requirement
>> have you found for this to be set (other than the ASSERT() you put in
>> gnttab_get_shared_frame_mfn()?
>>
>
> The code in patch #2 is executed before the grant table version is set. I
> could alternatively have libxl explicitly set the version to 1 before trying
> to seed the table.
But that's not my point. What's wrong with leaving it at zero?
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table
> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 08 August 2018 11:30
> To: Paul Durrant
> Cc: Andrew Cooper ; Wei Liu
> ; George Dunlap ; Ian
> Jackson ; Stefano Stabellini
> ; xen-devel ;
> Konrad Rzeszutek Wilk ; Tim (Xen.org)
>
> Subject: Re: [PATCH v21 1/2] common: add a new mappable resource type:
> XENMEM_resource_grant_table
>
> >>> On 08.08.18 at 11:00, wrote:
> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -358,6 +358,12 @@ static inline unsigned int
> grant_to_status_frames(unsigned int grant_frames)
> > return DIV_ROUND_UP(grant_frames * GRANT_PER_PAGE,
> GRANT_STATUS_PER_PAGE);
> > }
> >
> > +/* Number of grant table entries. Caller must hold d's gr. table lock.*/
> > +static inline unsigned int status_to_grant_frames(unsigned int
> status_frames)
> > +{
> > +return DIV_ROUND_UP(status_frames * GRANT_STATUS_PER_PAGE,
> GRANT_PER_PAGE);
> > +}
>
> Seeing no use of the grant table (it's not even passed in) I'm confused
> by the comment you add. I guess you've simply copied
> grant_to_status_frames()'es, which looks similarly wrong.
>
Indeed.
> > @@ -3860,6 +3866,47 @@ int mem_sharing_gref_to_gfn(struct
> grant_table *gt, grant_ref_t ref,
> > }
> > #endif
> >
> > +/* caller must hold write lock */
> > +static int gnttab_get_status_frame_mfn(struct domain *d,
> > + unsigned long idx, mfn_t *mfn)
> > +{
> > +struct grant_table *gt = d->grant_table;
>
> const?
>
IIRC that didn't work because gnttab_grow_table() modifies the content.
> > +ASSERT(gt->gt_version == 2);
> > +
> > +if ( idx >= nr_status_frames(gt) )
> > +{
> > +unsigned long nr = status_to_grant_frames(idx + 1);
> > +
> > +if ( nr <= gt->max_grant_frames )
> > +gnttab_grow_table(d, nr);
> > +
> > +if ( idx >= nr_status_frames(gt) )
> > +return -EINVAL;
> > +}
> > +
> > +*mfn = _mfn(virt_to_mfn(gt->status[idx]));
> > +return 0;
> > +}
> > +
> > +/* caller must hold write lock */
> > +static int gnttab_get_shared_frame_mfn(struct domain *d,
> > + unsigned long idx, mfn_t *mfn)
> > +{
> > +struct grant_table *gt = d->grant_table;
>
> Again?
>
> > @@ -3906,6 +3943,38 @@ int gnttab_map_frame(struct domain *d,
> unsigned long idx, gfn_t gfn,
> > return rc;
> > }
> >
> > +int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
> > +mfn_t *mfn)
> > +{
> > +struct grant_table *gt = d->grant_table;
> > +int rc;
> > +
> > +grant_write_lock(gt);
> > +
> > +if ( gt->gt_version == 0 )
> > +gt->gt_version = 1;
>
> Since you've moved this here instead of dropping it, what requirement
> have you found for this to be set (other than the ASSERT() you put in
> gnttab_get_shared_frame_mfn()?
>
The code in patch #2 is executed before the grant table version is set. I could
alternatively have libxl explicitly set the version to 1 before trying to seed
the table.
> > @@ -1046,6 +1089,16 @@ static int acquire_resource(
> > xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
> > unsigned int i;
> >
> > +/*
> > + * FIXME: until foreign pages inserted into the P2M are properly
> > + *reference counted, it is unsafe to allow mapping of
> > + *non-caller-owned resource pages unless the caller is
> > + *the hardware domain.
> > + */
> > +if (!(xmar.flags & XENMEM_rsrc_acq_caller_owned) &&
> > +!is_hardware_domain(currd) )
>
> Missing blank and as a result improper indentation. Also the U in
> "until" wants to be upper case I think.
>
Ok.
> The cosmetic aspects could of course be taken care of while
> committing, but the version related question needs to be
> clarified first.
>
It's ok, I'm happy to send v22. Just need to know whether you'd prefer explicit
version setting in the toolstack.
Paul
> Jan
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table
>>> On 08.08.18 at 11:00, wrote:
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -358,6 +358,12 @@ static inline unsigned int
> grant_to_status_frames(unsigned int grant_frames)
> return DIV_ROUND_UP(grant_frames * GRANT_PER_PAGE,
> GRANT_STATUS_PER_PAGE);
> }
>
> +/* Number of grant table entries. Caller must hold d's gr. table lock.*/
> +static inline unsigned int status_to_grant_frames(unsigned int status_frames)
> +{
> +return DIV_ROUND_UP(status_frames * GRANT_STATUS_PER_PAGE,
> GRANT_PER_PAGE);
> +}
Seeing no use of the grant table (it's not even passed in) I'm confused
by the comment you add. I guess you've simply copied
grant_to_status_frames()'es, which looks similarly wrong.
> @@ -3860,6 +3866,47 @@ int mem_sharing_gref_to_gfn(struct grant_table *gt,
> grant_ref_t ref,
> }
> #endif
>
> +/* caller must hold write lock */
> +static int gnttab_get_status_frame_mfn(struct domain *d,
> + unsigned long idx, mfn_t *mfn)
> +{
> +struct grant_table *gt = d->grant_table;
const?
> +ASSERT(gt->gt_version == 2);
> +
> +if ( idx >= nr_status_frames(gt) )
> +{
> +unsigned long nr = status_to_grant_frames(idx + 1);
> +
> +if ( nr <= gt->max_grant_frames )
> +gnttab_grow_table(d, nr);
> +
> +if ( idx >= nr_status_frames(gt) )
> +return -EINVAL;
> +}
> +
> +*mfn = _mfn(virt_to_mfn(gt->status[idx]));
> +return 0;
> +}
> +
> +/* caller must hold write lock */
> +static int gnttab_get_shared_frame_mfn(struct domain *d,
> + unsigned long idx, mfn_t *mfn)
> +{
> +struct grant_table *gt = d->grant_table;
Again?
> @@ -3906,6 +3943,38 @@ int gnttab_map_frame(struct domain *d, unsigned long
> idx, gfn_t gfn,
> return rc;
> }
>
> +int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
> +mfn_t *mfn)
> +{
> +struct grant_table *gt = d->grant_table;
> +int rc;
> +
> +grant_write_lock(gt);
> +
> +if ( gt->gt_version == 0 )
> +gt->gt_version = 1;
Since you've moved this here instead of dropping it, what requirement
have you found for this to be set (other than the ASSERT() you put in
gnttab_get_shared_frame_mfn()?
> @@ -1046,6 +1089,16 @@ static int acquire_resource(
> xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
> unsigned int i;
>
> +/*
> + * FIXME: until foreign pages inserted into the P2M are properly
> + *reference counted, it is unsafe to allow mapping of
> + *non-caller-owned resource pages unless the caller is
> + *the hardware domain.
> + */
> +if (!(xmar.flags & XENMEM_rsrc_acq_caller_owned) &&
> +!is_hardware_domain(currd) )
Missing blank and as a result improper indentation. Also the U in
"until" wants to be upper case I think.
The cosmetic aspects could of course be taken care of while
committing, but the version related question needs to be
clarified first.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table
> -Original Message-
> From: Andrew Cooper
> Sent: 08 August 2018 11:11
> To: Paul Durrant ; xen-devel@lists.xenproject.org
> Cc: Jan Beulich ; George Dunlap
> ; Ian Jackson ; Konrad
> Rzeszutek Wilk ; Stefano Stabellini
> ; Tim (Xen.org) ; Wei Liu
>
> Subject: Re: [PATCH v21 1/2] common: add a new mappable resource type:
> XENMEM_resource_grant_table
>
> On 08/08/18 10:00, Paul Durrant wrote:
> > diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> > index d9ec711c73..8e623ea08e 100644
> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -358,6 +358,12 @@ static inline unsigned int
> grant_to_status_frames(unsigned int grant_frames)
> > return DIV_ROUND_UP(grant_frames * GRANT_PER_PAGE,
> GRANT_STATUS_PER_PAGE);
> > }
> >
> > +/* Number of grant table entries. Caller must hold d's gr. table lock.*/
>
> Really? this is some straight arithmetic on the integer parameter.
>
Good point. I'd just adapted the comment from the reverse translation above it
but the comment is indeed bogus.
> > +static inline unsigned int status_to_grant_frames(unsigned int
> status_frames)
> > +{
> > +return DIV_ROUND_UP(status_frames * GRANT_STATUS_PER_PAGE,
> GRANT_PER_PAGE);
> > +}
> > +
> > /* Check if the page has been paged out, or needs unsharing.
> > If rc == GNTST_okay, *page contains the page struct with a ref taken.
> > Caller must do put_page(*page).
> > @@ -3860,6 +3866,47 @@ int mem_sharing_gref_to_gfn(struct
> grant_table *gt, grant_ref_t ref,
> > }
> > #endif
> >
> > +/* caller must hold write lock */
> > +static int gnttab_get_status_frame_mfn(struct domain *d,
> > + unsigned long idx, mfn_t *mfn)
> > +{
> > +struct grant_table *gt = d->grant_table;
> > +
> > +ASSERT(gt->gt_version == 2);
> > +
> > +if ( idx >= nr_status_frames(gt) )
> > +{
> > +unsigned long nr = status_to_grant_frames(idx + 1);
>
> Why the +1 ? Won't that cause a failure if you attempt to map the
> maximum valid index?
That's kind of the idea...
>
> > +
> > +if ( nr <= gt->max_grant_frames )
> > +gnttab_grow_table(d, nr);
>
> You want to capture the return value of grow_table(), which at least
> distinguishes between ENODEV and ENOMEM.
>
> > +
> > +if ( idx >= nr_status_frames(gt) )
> > +return -EINVAL;
>
> This can probably(?) be asserted if gnttab_grow_table() returns
> successfully.
>
... which is why this is an if and not an ASSERT. I'm just following the
analogy of the way the table is grown in gnttab_get_shared_frame_mfn() which is
the way it was done before. If you'd rather I change things to use the return
value from gnttab_grow_table() then I guess I could do that.
> > +}
> > +
> > +*mfn = _mfn(virt_to_mfn(gt->status[idx]));
> > +return 0;
> > +}
> > +
> > +/* caller must hold write lock */
> > +static int gnttab_get_shared_frame_mfn(struct domain *d,
> > + unsigned long idx, mfn_t *mfn)
> > +{
> > +struct grant_table *gt = d->grant_table;
> > +
> > +ASSERT(gt->gt_version != 0);
> > +
> > +if ( (idx >= nr_grant_frames(gt)) && (idx < gt->max_grant_frames) )
> > +gnttab_grow_table(d, idx + 1);
>
> Similarly wrt rc.
>
> > +
> > +if ( idx >= nr_grant_frames(gt) )
> > +return -EINVAL;
> > +
> > +*mfn = _mfn(virt_to_mfn(gt->shared_raw[idx]));
> > +return 0;
> > +}
> > +
> > int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
> > mfn_t *mfn)
> > {
> > diff --git a/xen/common/memory.c b/xen/common/memory.c
> > index e29d596727..427f65a5e1 100644
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -23,6 +23,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -982,6 +983,43 @@ static long xatp_permission_check(struct domain
> *d, unsigned int space)
> > return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
> > }
> >
> > +static int acquire_grant_table(struct domain *d, unsigned int id,
> > + unsigned long frame,
> > + unsigned int nr_frames,
> > + xen_pfn_t mfn_list[])
> > +{
> > +unsigned int i = nr_frames;
> > +
> > +/* Iterate backwards in case table needs to grow */
> > +while ( i-- != 0 )
> > +{
> > +mfn_t mfn = INVALID_MFN;
> > +int rc;
> > +
> > +switch ( id )
> > +{
> > +case XENMEM_resource_grant_table_id_shared:
> > +rc = gnttab_get_shared_frame(d, frame + i, );
> > +break;
> > +
> > +case XENMEM_resource_grant_table_id_status:
> > +rc = gnttab_get_status_frame(d, frame + i, );
> > +break;
> > +
> > +default:
> > +rc = -EINVAL;
> > +break;
> > +}
> > +
> > +if ( rc )
>
> Would it
Re: [Xen-devel] [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table
On 08/08/18 10:00, Paul Durrant wrote:
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index d9ec711c73..8e623ea08e 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -358,6 +358,12 @@ static inline unsigned int
> grant_to_status_frames(unsigned int grant_frames)
> return DIV_ROUND_UP(grant_frames * GRANT_PER_PAGE,
> GRANT_STATUS_PER_PAGE);
> }
>
> +/* Number of grant table entries. Caller must hold d's gr. table lock.*/
Really? this is some straight arithmetic on the integer parameter.
> +static inline unsigned int status_to_grant_frames(unsigned int status_frames)
> +{
> +return DIV_ROUND_UP(status_frames * GRANT_STATUS_PER_PAGE,
> GRANT_PER_PAGE);
> +}
> +
> /* Check if the page has been paged out, or needs unsharing.
> If rc == GNTST_okay, *page contains the page struct with a ref taken.
> Caller must do put_page(*page).
> @@ -3860,6 +3866,47 @@ int mem_sharing_gref_to_gfn(struct grant_table *gt,
> grant_ref_t ref,
> }
> #endif
>
> +/* caller must hold write lock */
> +static int gnttab_get_status_frame_mfn(struct domain *d,
> + unsigned long idx, mfn_t *mfn)
> +{
> +struct grant_table *gt = d->grant_table;
> +
> +ASSERT(gt->gt_version == 2);
> +
> +if ( idx >= nr_status_frames(gt) )
> +{
> +unsigned long nr = status_to_grant_frames(idx + 1);
Why the +1 ? Won't that cause a failure if you attempt to map the
maximum valid index?
> +
> +if ( nr <= gt->max_grant_frames )
> +gnttab_grow_table(d, nr);
You want to capture the return value of grow_table(), which at least
distinguishes between ENODEV and ENOMEM.
> +
> +if ( idx >= nr_status_frames(gt) )
> +return -EINVAL;
This can probably(?) be asserted if gnttab_grow_table() returns
successfully.
> +}
> +
> +*mfn = _mfn(virt_to_mfn(gt->status[idx]));
> +return 0;
> +}
> +
> +/* caller must hold write lock */
> +static int gnttab_get_shared_frame_mfn(struct domain *d,
> + unsigned long idx, mfn_t *mfn)
> +{
> +struct grant_table *gt = d->grant_table;
> +
> +ASSERT(gt->gt_version != 0);
> +
> +if ( (idx >= nr_grant_frames(gt)) && (idx < gt->max_grant_frames) )
> +gnttab_grow_table(d, idx + 1);
Similarly wrt rc.
> +
> +if ( idx >= nr_grant_frames(gt) )
> +return -EINVAL;
> +
> +*mfn = _mfn(virt_to_mfn(gt->shared_raw[idx]));
> +return 0;
> +}
> +
> int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
> mfn_t *mfn)
> {
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index e29d596727..427f65a5e1 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -23,6 +23,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -982,6 +983,43 @@ static long xatp_permission_check(struct domain *d,
> unsigned int space)
> return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
> }
>
> +static int acquire_grant_table(struct domain *d, unsigned int id,
> + unsigned long frame,
> + unsigned int nr_frames,
> + xen_pfn_t mfn_list[])
> +{
> +unsigned int i = nr_frames;
> +
> +/* Iterate backwards in case table needs to grow */
> +while ( i-- != 0 )
> +{
> +mfn_t mfn = INVALID_MFN;
> +int rc;
> +
> +switch ( id )
> +{
> +case XENMEM_resource_grant_table_id_shared:
> +rc = gnttab_get_shared_frame(d, frame + i, );
> +break;
> +
> +case XENMEM_resource_grant_table_id_status:
> +rc = gnttab_get_status_frame(d, frame + i, );
> +break;
> +
> +default:
> +rc = -EINVAL;
> +break;
> +}
> +
> +if ( rc )
Would it perhaps be prudent to have || mfn_eq(mfn, INVALID_MFN) here? I
don't think anything good will come of handing INVALID_MFN back to the
caller.
> +return rc;
> +
> +mfn_list[i] = mfn_x(mfn);
> +}
> +
> +return 0;
> +}
> +
> static int acquire_resource(
> XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg)
> {
> @@ -1046,6 +1089,16 @@ static int acquire_resource(
> xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
> unsigned int i;
>
> +/*
> + * FIXME: until foreign pages inserted into the P2M are properly
> + *reference counted, it is unsafe to allow mapping of
> + *non-caller-owned resource pages unless the caller is
> + *the hardware domain.
> + */
> +if (!(xmar.flags & XENMEM_rsrc_acq_caller_owned) &&
Space.
> +!is_hardware_domain(currd) )
> +return -EOPNOTSUPP;
EPERM perhaps?
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
