Hello,

The current calls to memory_type_changed() are wider than strictly
necessary.  Move them inside the iocap handlers and also limit to only
issue them when required.

I would really like to get some feedback on the Arm change, since this
is now a prereq for the actual fix.

Thanks, Roger.

Roger Pau Monne (2):
  arm/vgic: drop const attribute from gic_iomem_deny_access()
  x86/ept: limit calls to memory_type_changed()

 xen/arch/arm/gic-v2.c            |  2 +-
 xen/arch/arm/gic-v3.c            |  2 +-
 xen/arch/arm/gic.c               |  2 +-
 xen/arch/arm/include/asm/gic.h   |  4 ++--
 xen/arch/x86/domctl.c            |  4 ----
 xen/arch/x86/include/asm/iocap.h | 33 +++++++++++++++++++++++----
 xen/arch/x86/mm/p2m-ept.c        |  4 ++++
 xen/common/domctl.c              |  4 ----
 xen/include/xen/iocap.h          | 38 ++++++++++++++++++++++++++++----
 9 files changed, 72 insertions(+), 21 deletions(-)

-- 
2.37.3

On 28.09.2022 16:11, Roger Pau Monne wrote:
> The current calls to memory_type_changed() are wider than strictly
> necessary.  Move them inside the iocap handlers and also limit to only
> issue them when required.
> 
> I would really like to get some feedback on the Arm change, since this
> is now a prereq for the actual fix.
> 
> Thanks, Roger.
> 
> Roger Pau Monne (2):
>   arm/vgic: drop const attribute from gic_iomem_deny_access()
>   x86/ept: limit calls to memory_type_changed()

Are there intentions for having these on 4.17?

Jan

On Thu, Sep 29, 2022 at 12:14:10PM +0200, Jan Beulich wrote:
> On 28.09.2022 16:11, Roger Pau Monne wrote:
> > The current calls to memory_type_changed() are wider than strictly
> > necessary.  Move them inside the iocap handlers and also limit to only
> > issue them when required.
> > 
> > I would really like to get some feedback on the Arm change, since this
> > is now a prereq for the actual fix.
> > 
> > Thanks, Roger.
> > 
> > Roger Pau Monne (2):
> >   arm/vgic: drop const attribute from gic_iomem_deny_access()
> >   x86/ept: limit calls to memory_type_changed()
> 
> Are there intentions for having these on 4.17?

I wasn't sure.  From XenServer PoV it's certainly a bug fix,
otherwise some workloads related to GPU passthrough are simply too
slow to be usable.

I would certainly be fine with it making it's way into 4.17, let me
add Henry:

Cons:
 - Changes the number of issued memory_type_changed(), so there's a
   risk I misplaced some of the conditions and we end up with wrong
   cache types in the guest p2m due to missing memory_type_changed()
   calls.  That however won't affect Xen itself, just the guest.

Pros:
 - Removes unneeded memory_type_changed(), thus making some operations
   faster.  It's effect it's greatly dependent on using a set of
   hypercalls against a domain, which doesn't seem common in upstream.
   It's possible other products based on Xen apart from XenServer will
   also see an speedup as a result.

Thanks, Roger.

Hi Roger and Jan,

> -----Original Message-----
> From: Roger Pau Monné <roger.pau@citrix.com>
> Subject: For 4.17 (was: Re: [PATCH v3 0/2] Move calls to
> memory_type_changed())
> 
> On Thu, Sep 29, 2022 at 12:14:10PM +0200, Jan Beulich wrote:
> > On 28.09.2022 16:11, Roger Pau Monne wrote:
> > > The current calls to memory_type_changed() are wider than strictly
> > > necessary.  Move them inside the iocap handlers and also limit to only
> > > issue them when required.
> > >
> > > I would really like to get some feedback on the Arm change, since this
> > > is now a prereq for the actual fix.
> > >
> > > Thanks, Roger.
> > >
> > > Roger Pau Monne (2):
> > >   arm/vgic: drop const attribute from gic_iomem_deny_access()
> > >   x86/ept: limit calls to memory_type_changed()
> >
> > Are there intentions for having these on 4.17?
> 
> I wasn't sure.  From XenServer PoV it's certainly a bug fix,
> otherwise some workloads related to GPU passthrough are simply too
> slow to be usable.
> 
> I would certainly be fine with it making it's way into 4.17, let me
> add Henry:
> 
> Cons:
>  - Changes the number of issued memory_type_changed(), so there's a
>    risk I misplaced some of the conditions and we end up with wrong
>    cache types in the guest p2m due to missing memory_type_changed()
>    calls.  That however won't affect Xen itself, just the guest.
> 
> Pros:
>  - Removes unneeded memory_type_changed(), thus making some
> operations
>    faster.  It's effect it's greatly dependent on using a set of
>    hypercalls against a domain, which doesn't seem common in upstream.
>    It's possible other products based on Xen apart from XenServer will
>    also see an speedup as a result.

Thanks for the information and the detailed summary!

I think my understanding is the same as what Jan has in 
"x86/NUMA: correct memnode_shift calculation for single node system",
- we are still not in code freeze but in feature freeze, so properly-reviewed
fixes are eligible for the release. For this specific series, (to me) it looks ok
and I will not block the merging of this series if maintainers want to merge
it :))

Kind regards,
Henry

> 
> Thanks, Roger.

On 29.09.2022 13:10, Henry Wang wrote:
>> -----Original Message-----
>> From: Roger Pau Monné <roger.pau@citrix.com>
>>
>> On Thu, Sep 29, 2022 at 12:14:10PM +0200, Jan Beulich wrote:
>>> On 28.09.2022 16:11, Roger Pau Monne wrote:
>>>> The current calls to memory_type_changed() are wider than strictly
>>>> necessary.  Move them inside the iocap handlers and also limit to only
>>>> issue them when required.
>>>>
>>>> I would really like to get some feedback on the Arm change, since this
>>>> is now a prereq for the actual fix.
>>>>
>>>> Thanks, Roger.
>>>>
>>>> Roger Pau Monne (2):
>>>>   arm/vgic: drop const attribute from gic_iomem_deny_access()
>>>>   x86/ept: limit calls to memory_type_changed()
>>>
>>> Are there intentions for having these on 4.17?
>>
>> I wasn't sure.  From XenServer PoV it's certainly a bug fix,
>> otherwise some workloads related to GPU passthrough are simply too
>> slow to be usable.
>>
>> I would certainly be fine with it making it's way into 4.17, let me
>> add Henry:
>>
>> Cons:
>>  - Changes the number of issued memory_type_changed(), so there's a
>>    risk I misplaced some of the conditions and we end up with wrong
>>    cache types in the guest p2m due to missing memory_type_changed()
>>    calls.  That however won't affect Xen itself, just the guest.
>>
>> Pros:
>>  - Removes unneeded memory_type_changed(), thus making some
>> operations
>>    faster.  It's effect it's greatly dependent on using a set of
>>    hypercalls against a domain, which doesn't seem common in upstream.
>>    It's possible other products based on Xen apart from XenServer will
>>    also see an speedup as a result.
> 
> Thanks for the information and the detailed summary!
> 
> I think my understanding is the same as what Jan has in 
> "x86/NUMA: correct memnode_shift calculation for single node system",
> - we are still not in code freeze but in feature freeze, so properly-reviewed
> fixes are eligible for the release. For this specific series, (to me) it looks ok
> and I will not block the merging of this series if maintainers want to merge
> it :))

Thanks. Then, according to my reply to patch 2, the only open thing is
to have at least informal agreement there by (at least) one of the Arm
maintainers. There's no guarantee this will arrive by tomorrow.

Jan