Re: [Xen-devel] [PATCH v5 2/4] x86/HVM: implement memory read caching for insn emulation

["Durrant, Paul" <pdurrant@xxxxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 03 March 2020 15:24
> To: Durrant, Paul <pdurrant@xxxxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Kevin Tian <kevin.tian@xxxxxxxxx>; Wei 
> Liu <wl@xxxxxxx>; Paul
> Durrant <paul@xxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; Jun 
> Nakajima
> <jun.nakajima@xxxxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Subject: RE: [EXTERNAL][PATCH v5 2/4] x86/HVM: implement memory read caching 
> for insn emulation
> 
> CAUTION: This email originated from outside of the organization. Do not click 
> links or open
> attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On 03.03.2020 16:16, Durrant, Paul wrote:
> >> From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of Jan 
> >> Beulich
> >> Sent: 03 March 2020 10:17
> >>
> >> --- a/xen/arch/x86/hvm/emulate.c
> >> +++ b/xen/arch/x86/hvm/emulate.c
> >> @@ -28,6 +28,19 @@
> >>  #include <asm/iocap.h>
> >>  #include <asm/vm_event.h>
> >>
> >> +struct hvmemul_cache
> >> +{
> >> +    /* The cache is disabled as long as num_ents > max_ents. */
> >> +    unsigned int num_ents;
> >> +    unsigned int max_ents;
> >> +    struct {
> >> +        paddr_t gpa:PADDR_BITS;
> >> +        unsigned int :BITS_PER_LONG - PADDR_BITS - 8;
> >
> > Is clang ok with unnamed fields?
> 
> Clang 5 at least is, and iirc this is a standard C feature.
> 

Google didn't give me a conclusive answer which was I asked. I've not found 
anything that says it's a gcc-ism though.

> >> +void hvmemul_cache_restore(struct vcpu *v, unsigned int token)
> >> +{
> >> +    struct hvmemul_cache *cache = v->arch.hvm.hvm_io.cache;
> >> +
> >> +    ASSERT(cache->num_ents > cache->max_ents);
> >> +    cache->num_ents = token;
> >
> > ASSERT(token <= cache->max_ents) here?
> 
> Hmm, not sure. Definitely not exactly as you say, as disabling
> in already disabled state would return max_ents + 1, and hence
> this value could also be fed back here.

Ok, I can see keeping that idempotent is useful.

> But even beyond that I
> don't see a need to restrict the value range here - anything
> larger than max_ents will simply result in the cache being
> disabled.
> 

Fair enough.

Reviewed-by: Paul Durrant <pdurrant@xxxxxxxx>

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