Re: [Xen-devel] [PATCH v6 4/4] x86/ioreq server: Reset outstanding p2m_ioreq_server entries when an ioreq server unmaps.
On 9/23/2016 2:06 AM, George Dunlap wrote: On Tue, Sep 20, 2016 at 3:57 AM, Yu Zhang wrote: Well, for the logic of p2m type recalculation, similarities between p2m_ioreq_server and other changeable types exceeds their differences. As to the special cases, how about we use a macro, i.e. p2m_is_ioreq? That'd be better than the open coded check, but would still result in (taking the above example) p2m_is_changeable(ept_entry->sa_p2mt) && !p2m_is_ioreq(ept_entry->sa_p2mt) ) ? What I'd prefer is a predicate that can be applied here on its own, without involving && or ||. OK. I can think of 2 scenarios that p2m_ioreq_server needs special handling: 1> In ept_get_entry()/recal_type(), the p2m types are supposed to return as it is, instead of changing to p2m_log_dirty. So we can use a macro or a inline function like p2m_check_changeable(), which combines the p2m_is_changeable(ept_entry->sa_p2mt) && !p2m_is_ioreq(ept_entry->sa_p2mt) ) together. 2> In resolve_misconfig()/do_recalc(), the entry_count gets decremented. We do not need this new inline function, because they are in a separate if() statement. Is this OK? :) Sounds reasonable. But please give George and others a chance to voice their opinions before you go that route. Jan Hi George, Any comments on this series? :) Well regarding the question you and Jan have been discussing, of what to call / how to do the checks for changeable-but-not-ioreq, I don't really care very much. :-) I'm sorry it's taking so long to look at this series -- the code you're trying to modify is already a bit of a tangled mess, and I think this patch has a ways to go before it's ready. I do think this series is important, so I'll be coming back to it first thing Monday. Regarding the pausing added in this patch -- you listed two reasons in the first patch for the domain pausing. The first was detecting read-modify-write and acting appropriately; I think that can be done with the patch that I sent you. The second was the deadlock due to grabbing locks in a different order. I'm afraid having such a thing lying around, even if you've avoided it for now by pausing the domain, is another major trap that we should try to avoid laying for future developers if we can at all help it. The normal thing to do here is to simply have a locking discipline -- in this case, I don't think it would be too difficult to work out a locking order that would avoid the deadlock in a more robust way than pausing and unpausing the domain. With those two handled, we shouldn't need to pause the domain any more. Thank you, George. Hope we can find a more elegant approach. :-) B.R. Yu Thanks for your work on this -- I'll get back to patch 4/4 next week. Peace, -George ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 4/4] x86/ioreq server: Reset outstanding p2m_ioreq_server entries when an ioreq server unmaps.
On Tue, Sep 20, 2016 at 3:57 AM, Yu Zhang wrote: > Well, for the logic of p2m type recalculation, similarities between > p2m_ioreq_server > and other changeable types exceeds their differences. As to the special > cases, how > about we use a macro, i.e. p2m_is_ioreq? That'd be better than the open coded check, but would still result in (taking the above example) p2m_is_changeable(ept_entry->sa_p2mt) && !p2m_is_ioreq(ept_entry->sa_p2mt) ) ? What I'd prefer is a predicate that can be applied here on its own, without involving && or ||. >>> OK. I can think of 2 scenarios that p2m_ioreq_server needs special >>> handling: >>> >>> 1> In ept_get_entry()/recal_type(), the p2m types are supposed to return >>> as it is, instead of >>> changing to p2m_log_dirty. So we can use a macro or a inline function >>> like p2m_check_changeable(), >>> which combines the >>> >>>p2m_is_changeable(ept_entry->sa_p2mt) && >>>!p2m_is_ioreq(ept_entry->sa_p2mt) ) >>> >>> together. >>> >>> 2> In resolve_misconfig()/do_recalc(), the entry_count gets decremented. >>> We >>> do not need this new inline function, because they are in a separate >>> if() statement. >>> >>> Is this OK? :) >> >> Sounds reasonable. But please give George and others a chance to >> voice their opinions before you go that route. >> >> Jan > > > Hi George, > > Any comments on this series? :) Well regarding the question you and Jan have been discussing, of what to call / how to do the checks for changeable-but-not-ioreq, I don't really care very much. :-) I'm sorry it's taking so long to look at this series -- the code you're trying to modify is already a bit of a tangled mess, and I think this patch has a ways to go before it's ready. I do think this series is important, so I'll be coming back to it first thing Monday. Regarding the pausing added in this patch -- you listed two reasons in the first patch for the domain pausing. The first was detecting read-modify-write and acting appropriately; I think that can be done with the patch that I sent you. The second was the deadlock due to grabbing locks in a different order. I'm afraid having such a thing lying around, even if you've avoided it for now by pausing the domain, is another major trap that we should try to avoid laying for future developers if we can at all help it. The normal thing to do here is to simply have a locking discipline -- in this case, I don't think it would be too difficult to work out a locking order that would avoid the deadlock in a more robust way than pausing and unpausing the domain. With those two handled, we shouldn't need to pause the domain any more. Thanks for your work on this -- I'll get back to patch 4/4 next week. Peace, -George ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 4/4] x86/ioreq server: Reset outstanding p2m_ioreq_server entries when an ioreq server unmaps.
On 9/9/2016 6:09 PM, Jan Beulich wrote:
On 09.09.16 at 11:56, wrote:
On 9/9/2016 5:44 PM, Jan Beulich wrote:
On 09.09.16 at 11:24, wrote:
On 9/9/2016 4:20 PM, Jan Beulich wrote:
On 09.09.16 at 09:24, wrote:
On 9/9/2016 1:26 PM, Yu Zhang wrote:
On 02.09.16 at 12:47, wrote:
@@ -965,7 +968,8 @@ static mfn_t ept_get_entry(struct p2m_domain *p2m,
if ( is_epte_valid(ept_entry) )
{
if ( (recalc || ept_entry->recalc) &&
- p2m_is_changeable(ept_entry->sa_p2mt) )
+ p2m_is_changeable(ept_entry->sa_p2mt) &&
+ (ept_entry->sa_p2mt != p2m_ioreq_server) )
*t = p2m_is_logdirty_range(p2m, gfn, gfn) ?
p2m_ram_logdirty
: p2m_ram_rw;
else
Considering this (and at least one more similar adjustment further
down), is it really appropriate to include p2m_ioreq_server in the
set of "changeable" types?
Well, I agree p2m_ioreq_server do have different behaviors than the
p2m_log_dirty, but removing p2m_ioreq_server from changeable type
would need more specific code for the p2m_ioreq_server in routines like
resolve_misconfig(), do_recalc() and p2m_change_entry_type_global() etc.
I've tried this approach and abandoned later. :)
I can see that the other option might require more adjustments, in
which case I guess this variant would be fine if you created another
helper (well named) inline function instead of open coding this in
several places. Of course such dissimilar handling in the various
places p2m_is_changeable() gets used right now will additionally
require good reasoning - after all that predicate exists to express
the similarities between different code paths.
Thanks Jan.
Well, for the logic of p2m type recalculation, similarities between
p2m_ioreq_server
and other changeable types exceeds their differences. As to the special
cases, how
about we use a macro, i.e. p2m_is_ioreq?
That'd be better than the open coded check, but would still result
in (taking the above example)
p2m_is_changeable(ept_entry->sa_p2mt) &&
!p2m_is_ioreq(ept_entry->sa_p2mt) )
? What I'd prefer is a predicate that can be applied here on its own,
without involving && or ||.
OK. I can think of 2 scenarios that p2m_ioreq_server needs special handling:
1> In ept_get_entry()/recal_type(), the p2m types are supposed to return
as it is, instead of
changing to p2m_log_dirty. So we can use a macro or a inline function
like p2m_check_changeable(),
which combines the
p2m_is_changeable(ept_entry->sa_p2mt) &&
!p2m_is_ioreq(ept_entry->sa_p2mt) )
together.
2> In resolve_misconfig()/do_recalc(), the entry_count gets decremented. We
do not need this new inline function, because they are in a separate
if() statement.
Is this OK? :)
Sounds reasonable. But please give George and others a chance to
voice their opinions before you go that route.
Jan
Hi George,
Any comments on this series? :)
Yu
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 4/4] x86/ioreq server: Reset outstanding p2m_ioreq_server entries when an ioreq server unmaps.
On 9/9/2016 6:09 PM, Jan Beulich wrote:
On 09.09.16 at 11:56, wrote:
On 9/9/2016 5:44 PM, Jan Beulich wrote:
On 09.09.16 at 11:24, wrote:
On 9/9/2016 4:20 PM, Jan Beulich wrote:
On 09.09.16 at 09:24, wrote:
On 9/9/2016 1:26 PM, Yu Zhang wrote:
On 02.09.16 at 12:47, wrote:
@@ -965,7 +968,8 @@ static mfn_t ept_get_entry(struct p2m_domain *p2m,
if ( is_epte_valid(ept_entry) )
{
if ( (recalc || ept_entry->recalc) &&
- p2m_is_changeable(ept_entry->sa_p2mt) )
+ p2m_is_changeable(ept_entry->sa_p2mt) &&
+ (ept_entry->sa_p2mt != p2m_ioreq_server) )
*t = p2m_is_logdirty_range(p2m, gfn, gfn) ?
p2m_ram_logdirty
: p2m_ram_rw;
else
Considering this (and at least one more similar adjustment further
down), is it really appropriate to include p2m_ioreq_server in the
set of "changeable" types?
Well, I agree p2m_ioreq_server do have different behaviors than the
p2m_log_dirty, but removing p2m_ioreq_server from changeable type
would need more specific code for the p2m_ioreq_server in routines like
resolve_misconfig(), do_recalc() and p2m_change_entry_type_global() etc.
I've tried this approach and abandoned later. :)
I can see that the other option might require more adjustments, in
which case I guess this variant would be fine if you created another
helper (well named) inline function instead of open coding this in
several places. Of course such dissimilar handling in the various
places p2m_is_changeable() gets used right now will additionally
require good reasoning - after all that predicate exists to express
the similarities between different code paths.
Thanks Jan.
Well, for the logic of p2m type recalculation, similarities between
p2m_ioreq_server
and other changeable types exceeds their differences. As to the special
cases, how
about we use a macro, i.e. p2m_is_ioreq?
That'd be better than the open coded check, but would still result
in (taking the above example)
p2m_is_changeable(ept_entry->sa_p2mt) &&
!p2m_is_ioreq(ept_entry->sa_p2mt) )
? What I'd prefer is a predicate that can be applied here on its own,
without involving && or ||.
OK. I can think of 2 scenarios that p2m_ioreq_server needs special handling:
1> In ept_get_entry()/recal_type(), the p2m types are supposed to return
as it is, instead of
changing to p2m_log_dirty. So we can use a macro or a inline function
like p2m_check_changeable(),
which combines the
p2m_is_changeable(ept_entry->sa_p2mt) &&
!p2m_is_ioreq(ept_entry->sa_p2mt) )
together.
2> In resolve_misconfig()/do_recalc(), the entry_count gets decremented. We
do not need this new inline function, because they are in a separate
if() statement.
Is this OK? :)
Sounds reasonable. But please give George and others a chance to
voice their opinions before you go that route.
Sure, thanks!
Yu
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 4/4] x86/ioreq server: Reset outstanding p2m_ioreq_server entries when an ioreq server unmaps.
>>> On 09.09.16 at 11:56, wrote:
>
> On 9/9/2016 5:44 PM, Jan Beulich wrote:
> On 09.09.16 at 11:24, wrote:
>>> On 9/9/2016 4:20 PM, Jan Beulich wrote:
>>> On 09.09.16 at 09:24, wrote:
> On 9/9/2016 1:26 PM, Yu Zhang wrote:
> On 02.09.16 at 12:47, wrote:
>>> @@ -965,7 +968,8 @@ static mfn_t ept_get_entry(struct p2m_domain *p2m,
>>>if ( is_epte_valid(ept_entry) )
>>>{
>>>if ( (recalc || ept_entry->recalc) &&
>>> - p2m_is_changeable(ept_entry->sa_p2mt) )
>>> + p2m_is_changeable(ept_entry->sa_p2mt) &&
>>> + (ept_entry->sa_p2mt != p2m_ioreq_server) )
>>>*t = p2m_is_logdirty_range(p2m, gfn, gfn) ?
>> p2m_ram_logdirty
>>> : p2m_ram_rw;
>>>else
>> Considering this (and at least one more similar adjustment further
>> down), is it really appropriate to include p2m_ioreq_server in the
>> set of "changeable" types?
> Well, I agree p2m_ioreq_server do have different behaviors than the
> p2m_log_dirty, but removing p2m_ioreq_server from changeable type
> would need more specific code for the p2m_ioreq_server in routines like
> resolve_misconfig(), do_recalc() and p2m_change_entry_type_global() etc.
> I've tried this approach and abandoned later. :)
I can see that the other option might require more adjustments, in
which case I guess this variant would be fine if you created another
helper (well named) inline function instead of open coding this in
several places. Of course such dissimilar handling in the various
places p2m_is_changeable() gets used right now will additionally
require good reasoning - after all that predicate exists to express
the similarities between different code paths.
>>> Thanks Jan.
>>> Well, for the logic of p2m type recalculation, similarities between
>>> p2m_ioreq_server
>>> and other changeable types exceeds their differences. As to the special
>>> cases, how
>>> about we use a macro, i.e. p2m_is_ioreq?
>> That'd be better than the open coded check, but would still result
>> in (taking the above example)
>>
>> p2m_is_changeable(ept_entry->sa_p2mt) &&
>> !p2m_is_ioreq(ept_entry->sa_p2mt) )
>>
>> ? What I'd prefer is a predicate that can be applied here on its own,
>> without involving && or ||.
>>
>
> OK. I can think of 2 scenarios that p2m_ioreq_server needs special handling:
>
> 1> In ept_get_entry()/recal_type(), the p2m types are supposed to return
> as it is, instead of
> changing to p2m_log_dirty. So we can use a macro or a inline function
> like p2m_check_changeable(),
> which combines the
>
> p2m_is_changeable(ept_entry->sa_p2mt) &&
> !p2m_is_ioreq(ept_entry->sa_p2mt) )
>
> together.
>
> 2> In resolve_misconfig()/do_recalc(), the entry_count gets decremented. We
> do not need this new inline function, because they are in a separate
> if() statement.
>
> Is this OK? :)
Sounds reasonable. But please give George and others a chance to
voice their opinions before you go that route.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 4/4] x86/ioreq server: Reset outstanding p2m_ioreq_server entries when an ioreq server unmaps.
On 9/9/2016 5:44 PM, Jan Beulich wrote:
On 09.09.16 at 11:24, wrote:
On 9/9/2016 4:20 PM, Jan Beulich wrote:
On 09.09.16 at 09:24, wrote:
On 9/9/2016 1:26 PM, Yu Zhang wrote:
On 02.09.16 at 12:47, wrote:
@@ -965,7 +968,8 @@ static mfn_t ept_get_entry(struct p2m_domain *p2m,
if ( is_epte_valid(ept_entry) )
{
if ( (recalc || ept_entry->recalc) &&
- p2m_is_changeable(ept_entry->sa_p2mt) )
+ p2m_is_changeable(ept_entry->sa_p2mt) &&
+ (ept_entry->sa_p2mt != p2m_ioreq_server) )
*t = p2m_is_logdirty_range(p2m, gfn, gfn) ?
p2m_ram_logdirty
: p2m_ram_rw;
else
Considering this (and at least one more similar adjustment further
down), is it really appropriate to include p2m_ioreq_server in the
set of "changeable" types?
Well, I agree p2m_ioreq_server do have different behaviors than the
p2m_log_dirty, but removing p2m_ioreq_server from changeable type
would need more specific code for the p2m_ioreq_server in routines like
resolve_misconfig(), do_recalc() and p2m_change_entry_type_global() etc.
I've tried this approach and abandoned later. :)
I can see that the other option might require more adjustments, in
which case I guess this variant would be fine if you created another
helper (well named) inline function instead of open coding this in
several places. Of course such dissimilar handling in the various
places p2m_is_changeable() gets used right now will additionally
require good reasoning - after all that predicate exists to express
the similarities between different code paths.
Thanks Jan.
Well, for the logic of p2m type recalculation, similarities between
p2m_ioreq_server
and other changeable types exceeds their differences. As to the special
cases, how
about we use a macro, i.e. p2m_is_ioreq?
That'd be better than the open coded check, but would still result
in (taking the above example)
p2m_is_changeable(ept_entry->sa_p2mt) &&
!p2m_is_ioreq(ept_entry->sa_p2mt) )
? What I'd prefer is a predicate that can be applied here on its own,
without involving && or ||.
OK. I can think of 2 scenarios that p2m_ioreq_server needs special handling:
1> In ept_get_entry()/recal_type(), the p2m types are supposed to return
as it is, instead of
changing to p2m_log_dirty. So we can use a macro or a inline function
like p2m_check_changeable(),
which combines the
p2m_is_changeable(ept_entry->sa_p2mt) &&
!p2m_is_ioreq(ept_entry->sa_p2mt) )
together.
2> In resolve_misconfig()/do_recalc(), the entry_count gets decremented. We
do not need this new inline function, because they are in a separate
if() statement.
Is this OK? :)
Thanks
Yu
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 4/4] x86/ioreq server: Reset outstanding p2m_ioreq_server entries when an ioreq server unmaps.
>>> On 09.09.16 at 11:24, wrote:
>
> On 9/9/2016 4:20 PM, Jan Beulich wrote:
> On 09.09.16 at 09:24, wrote:
>>> On 9/9/2016 1:26 PM, Yu Zhang wrote:
>>> On 02.09.16 at 12:47, wrote:
> @@ -965,7 +968,8 @@ static mfn_t ept_get_entry(struct p2m_domain *p2m,
> if ( is_epte_valid(ept_entry) )
> {
> if ( (recalc || ept_entry->recalc) &&
> - p2m_is_changeable(ept_entry->sa_p2mt) )
> + p2m_is_changeable(ept_entry->sa_p2mt) &&
> + (ept_entry->sa_p2mt != p2m_ioreq_server) )
> *t = p2m_is_logdirty_range(p2m, gfn, gfn) ?
p2m_ram_logdirty
> : p2m_ram_rw;
> else
Considering this (and at least one more similar adjustment further
down), is it really appropriate to include p2m_ioreq_server in the
set of "changeable" types?
>>> Well, I agree p2m_ioreq_server do have different behaviors than the
>>> p2m_log_dirty, but removing p2m_ioreq_server from changeable type
>>> would need more specific code for the p2m_ioreq_server in routines like
>>> resolve_misconfig(), do_recalc() and p2m_change_entry_type_global() etc.
>>> I've tried this approach and abandoned later. :)
>> I can see that the other option might require more adjustments, in
>> which case I guess this variant would be fine if you created another
>> helper (well named) inline function instead of open coding this in
>> several places. Of course such dissimilar handling in the various
>> places p2m_is_changeable() gets used right now will additionally
>> require good reasoning - after all that predicate exists to express
>> the similarities between different code paths.
>
> Thanks Jan.
> Well, for the logic of p2m type recalculation, similarities between
> p2m_ioreq_server
> and other changeable types exceeds their differences. As to the special
> cases, how
> about we use a macro, i.e. p2m_is_ioreq?
That'd be better than the open coded check, but would still result
in (taking the above example)
p2m_is_changeable(ept_entry->sa_p2mt) &&
!p2m_is_ioreq(ept_entry->sa_p2mt) )
? What I'd prefer is a predicate that can be applied here on its own,
without involving && or ||.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 4/4] x86/ioreq server: Reset outstanding p2m_ioreq_server entries when an ioreq server unmaps.
On 9/9/2016 4:20 PM, Jan Beulich wrote:
On 09.09.16 at 09:24, wrote:
On 9/9/2016 1:26 PM, Yu Zhang wrote:
On 02.09.16 at 12:47, wrote:
@@ -965,7 +968,8 @@ static mfn_t ept_get_entry(struct p2m_domain *p2m,
if ( is_epte_valid(ept_entry) )
{
if ( (recalc || ept_entry->recalc) &&
- p2m_is_changeable(ept_entry->sa_p2mt) )
+ p2m_is_changeable(ept_entry->sa_p2mt) &&
+ (ept_entry->sa_p2mt != p2m_ioreq_server) )
*t = p2m_is_logdirty_range(p2m, gfn, gfn) ?
p2m_ram_logdirty
: p2m_ram_rw;
else
Considering this (and at least one more similar adjustment further
down), is it really appropriate to include p2m_ioreq_server in the
set of "changeable" types?
Well, I agree p2m_ioreq_server do have different behaviors than the
p2m_log_dirty, but removing p2m_ioreq_server from changeable type
would need more specific code for the p2m_ioreq_server in routines like
resolve_misconfig(), do_recalc() and p2m_change_entry_type_global() etc.
I've tried this approach and abandoned later. :)
I can see that the other option might require more adjustments, in
which case I guess this variant would be fine if you created another
helper (well named) inline function instead of open coding this in
several places. Of course such dissimilar handling in the various
places p2m_is_changeable() gets used right now will additionally
require good reasoning - after all that predicate exists to express
the similarities between different code paths.
Thanks Jan.
Well, for the logic of p2m type recalculation, similarities between
p2m_ioreq_server
and other changeable types exceeds their differences. As to the special
cases, how
about we use a macro, i.e. p2m_is_ioreq?
Jan
Yu
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 4/4] x86/ioreq server: Reset outstanding p2m_ioreq_server entries when an ioreq server unmaps.
>>> On 09.09.16 at 09:24, wrote:
> On 9/9/2016 1:26 PM, Yu Zhang wrote:
>> >>> On 02.09.16 at 12:47, wrote:
>> > @@ -965,7 +968,8 @@ static mfn_t ept_get_entry(struct p2m_domain *p2m,
>> > if ( is_epte_valid(ept_entry) )
>> > {
>> > if ( (recalc || ept_entry->recalc) &&
>> > - p2m_is_changeable(ept_entry->sa_p2mt) )
>> > + p2m_is_changeable(ept_entry->sa_p2mt) &&
>> > + (ept_entry->sa_p2mt != p2m_ioreq_server) )
>> > *t = p2m_is_logdirty_range(p2m, gfn, gfn) ?
>> p2m_ram_logdirty
>> >: p2m_ram_rw;
>> > else
>>
>> Considering this (and at least one more similar adjustment further
>> down), is it really appropriate to include p2m_ioreq_server in the
>> set of "changeable" types?
>
> Well, I agree p2m_ioreq_server do have different behaviors than the
> p2m_log_dirty, but removing p2m_ioreq_server from changeable type
> would need more specific code for the p2m_ioreq_server in routines like
> resolve_misconfig(), do_recalc() and p2m_change_entry_type_global() etc.
> I've tried this approach and abandoned later. :)
I can see that the other option might require more adjustments, in
which case I guess this variant would be fine if you created another
helper (well named) inline function instead of open coding this in
several places. Of course such dissimilar handling in the various
places p2m_is_changeable() gets used right now will additionally
require good reasoning - after all that predicate exists to express
the similarities between different code paths.
>> > +while ( gfn <= end )
>> > +{
>> > +get_gfn_query_unlocked(d, gfn, &t);
>> > +
>> > +if ( t == ot )
>> > +p2m_change_type_one(d, gfn, t, nt);
>> > +
>> > +gfn++;
>> > +}
>> > +
>> > +p2m_unlock(p2m);
>> > +}
>>
>> And then I wonder why p2m_change_type_range() can't be used
>> instead of adding this new function.
>>
>
> Well, like p2m_change_entry_type_global() , p2m_change_type_range() also
> does the change asynchronously. And here this routine is introduced
> to synchronously reset the p2m type.
Ah, of course. Silly me.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 4/4] x86/ioreq server: Reset outstanding p2m_ioreq_server entries when an ioreq server unmaps.
On 9/9/2016 1:26 PM, Yu Zhang wrote:
>>> On 02.09.16 at 12:47, wrote:
> @@ -5551,7 +5553,35 @@ static int hvmop_map_mem_type_to_ioreq_server(
> if ( rc != 0 )
> goto out;
>
> -rc = hvm_map_mem_type_to_ioreq_server(d, op.id, op.type,
op.flags);
> +if ( gfn == 0 )
> +rc = hvm_map_mem_type_to_ioreq_server(d, op.id, op.type,
op.flags);
> +
> +/*
> + * Iterate p2m table when an ioreq server unmaps from
p2m_ioreq_server,
> + * and reset the remaining p2m_ioreq_server entries back to
p2m_ram_rw.
> + */
> +if ( op.flags == 0 && rc == 0 )
> +{
> +struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +
> +while ( read_atomic(&p2m->ioreq.entry_count) &&
> +gfn <= p2m->max_mapped_pfn )
> +{
> +unsigned long gfn_end = gfn + HVMOP_op_mask;
> +
> +p2m_finish_type_change(d, gfn, gfn_end,
> + p2m_ioreq_server, p2m_ram_rw);
> +
> +/* Check for continuation if it's not the last
iteration. */
> +if ( ++gfn_end <= p2m->max_mapped_pfn &&
> +hypercall_preempt_check() )
> +{
> +rc = -ERESTART;
> +*iter = gfn_end;
> +goto out;
> +}
> +}
"gfn" doesn't get updated, so if no preemption happens here, the
same GFN range will get acted on a second time (and so on, until
eventually preemption becomes necessary).
Oh, right. Thanks for pointing out. :)
Also when you don't really need to use "goto", please try to avoid
it - here you could easily use just "break" instead.
OK.
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -545,6 +545,9 @@ static int resolve_misconfig(struct p2m_domain
*p2m,
> unsigned long gfn)
> e.ipat = ipat;
> if ( e.recalc && p2m_is_changeable(e.sa_p2mt) )
> {
> + if ( e.sa_p2mt == p2m_ioreq_server )
> + p2m->ioreq.entry_count--;
ISTR that George had asked you to put this accounting elsewhere.
Yes. You have good memory. : )
George's suggestion is to put inside atomic_write_ept_entry(), which is
a quite core routine,
and is only for ept. And my suggestion is to put inside the
p2m_type_change_one() and routine
resolve_misconfig()/do_recalc() as well, so that we can support both
Intel EPT and AMD NPT -
like the p2m_change_entry_type_global().
I had given a more detailed explanation in a reply on Aug 30 in the v5
thread. :)
And then I'd really like you to add assertions making sure this
count doesn't underflow.
OK.
> @@ -965,7 +968,8 @@ static mfn_t ept_get_entry(struct p2m_domain *p2m,
> if ( is_epte_valid(ept_entry) )
> {
> if ( (recalc || ept_entry->recalc) &&
> - p2m_is_changeable(ept_entry->sa_p2mt) )
> + p2m_is_changeable(ept_entry->sa_p2mt) &&
> + (ept_entry->sa_p2mt != p2m_ioreq_server) )
> *t = p2m_is_logdirty_range(p2m, gfn, gfn) ?
p2m_ram_logdirty
>: p2m_ram_rw;
> else
Considering this (and at least one more similar adjustment further
down), is it really appropriate to include p2m_ioreq_server in the
set of "changeable" types?
Well, I agree p2m_ioreq_server do have different behaviors than the
p2m_log_dirty, but removing p2m_ioreq_server from changeable type
would need more specific code for the p2m_ioreq_server in routines like
resolve_misconfig(), do_recalc() and p2m_change_entry_type_global() etc.
I've tried this approach and abandoned later. :)
> +void p2m_finish_type_change(struct domain *d,
> + unsigned long start, unsigned long end,
> + p2m_type_t ot, p2m_type_t nt)
> +{
> +struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +p2m_type_t t;
> +unsigned long gfn = start;
> +
> +ASSERT(start <= end);
> +ASSERT(ot != nt);
> +ASSERT(p2m_is_changeable(ot) && p2m_is_changeable(nt));
> +
> +p2m_lock(p2m);
> +
> +end = (end > p2m->max_mapped_pfn) ? p2m->max_mapped_pfn : end;
min() or an if() without else.
Got it. Thanks
> +while ( gfn <= end )
> +{
> +get_gfn_query_unlocked(d, gfn, &t);
> +
> +if ( t == ot )
> +p2m_change_type_one(d, gfn, t, nt);
> +
> +gfn++;
> +}
> +
> +p2m_unlock(p2m);
> +}
And then I wonder why p2m_change_type_range() can't be used
instead of adding this new function.
Well, like p2m_change_entry_type_global() , p2m_change_type_range() also
does the change asynchronously. And here this routine is introduced
to synchronously reset the p2m type.
Jan
Thanks
Yu
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 4/4] x86/ioreq server: Reset outstanding p2m_ioreq_server entries when an ioreq server unmaps.
>>> On 02.09.16 at 12:47, wrote:
> @@ -5551,7 +5553,35 @@ static int hvmop_map_mem_type_to_ioreq_server(
> if ( rc != 0 )
> goto out;
>
> -rc = hvm_map_mem_type_to_ioreq_server(d, op.id, op.type, op.flags);
> +if ( gfn == 0 )
> +rc = hvm_map_mem_type_to_ioreq_server(d, op.id, op.type, op.flags);
> +
> +/*
> + * Iterate p2m table when an ioreq server unmaps from p2m_ioreq_server,
> + * and reset the remaining p2m_ioreq_server entries back to p2m_ram_rw.
> + */
> +if ( op.flags == 0 && rc == 0 )
> +{
> +struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +
> +while ( read_atomic(&p2m->ioreq.entry_count) &&
> +gfn <= p2m->max_mapped_pfn )
> +{
> +unsigned long gfn_end = gfn + HVMOP_op_mask;
> +
> +p2m_finish_type_change(d, gfn, gfn_end,
> + p2m_ioreq_server, p2m_ram_rw);
> +
> +/* Check for continuation if it's not the last iteration. */
> +if ( ++gfn_end <= p2m->max_mapped_pfn &&
> +hypercall_preempt_check() )
> +{
> +rc = -ERESTART;
> +*iter = gfn_end;
> +goto out;
> +}
> +}
"gfn" doesn't get updated, so if no preemption happens here, the
same GFN range will get acted on a second time (and so on, until
eventually preemption becomes necessary).
Also when you don't really need to use "goto", please try to avoid
it - here you could easily use just "break" instead.
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -545,6 +545,9 @@ static int resolve_misconfig(struct p2m_domain *p2m,
> unsigned long gfn)
> e.ipat = ipat;
> if ( e.recalc && p2m_is_changeable(e.sa_p2mt) )
> {
> + if ( e.sa_p2mt == p2m_ioreq_server )
> + p2m->ioreq.entry_count--;
ISTR that George had asked you to put this accounting elsewhere.
And then I'd really like you to add assertions making sure this
count doesn't underflow.
> @@ -965,7 +968,8 @@ static mfn_t ept_get_entry(struct p2m_domain *p2m,
> if ( is_epte_valid(ept_entry) )
> {
> if ( (recalc || ept_entry->recalc) &&
> - p2m_is_changeable(ept_entry->sa_p2mt) )
> + p2m_is_changeable(ept_entry->sa_p2mt) &&
> + (ept_entry->sa_p2mt != p2m_ioreq_server) )
> *t = p2m_is_logdirty_range(p2m, gfn, gfn) ? p2m_ram_logdirty
>: p2m_ram_rw;
> else
Considering this (and at least one more similar adjustment further
down), is it really appropriate to include p2m_ioreq_server in the
set of "changeable" types?
> +void p2m_finish_type_change(struct domain *d,
> + unsigned long start, unsigned long end,
> + p2m_type_t ot, p2m_type_t nt)
> +{
> +struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +p2m_type_t t;
> +unsigned long gfn = start;
> +
> +ASSERT(start <= end);
> +ASSERT(ot != nt);
> +ASSERT(p2m_is_changeable(ot) && p2m_is_changeable(nt));
> +
> +p2m_lock(p2m);
> +
> +end = (end > p2m->max_mapped_pfn) ? p2m->max_mapped_pfn : end;
min() or an if() without else.
> +while ( gfn <= end )
> +{
> +get_gfn_query_unlocked(d, gfn, &t);
> +
> +if ( t == ot )
> +p2m_change_type_one(d, gfn, t, nt);
> +
> +gfn++;
> +}
> +
> +p2m_unlock(p2m);
> +}
And then I wonder why p2m_change_type_range() can't be used
instead of adding this new function.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
