Re: [Xen-devel] [PATCH v2 3/4] x86/HVM: implement memory read caching

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 12 September 2018 09:38
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; xen-devel
> <xen-devel@xxxxxxxxxxxxxxxxxxxx>
> Subject: RE: [PATCH v2 3/4] x86/HVM: implement memory read caching
> 
> >>> On 11.09.18 at 18:20, <Paul.Durrant@xxxxxxxxxx> wrote:
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: 11 September 2018 14:15
> >>
> >> @@ -2664,9 +2685,35 @@ void hvm_dump_emulation_state(const char
> >>             hvmemul_ctxt->insn_buf);
> >>  }
> >>
> >> +struct hvmemul_cache *hvmemul_cache_init(unsigned int nents)
> >> +{
> >> +    struct hvmemul_cache *cache = xmalloc_bytes(offsetof(struct
> >> hvmemul_cache,
> >> +                                                         ents[nents]));
> >> +
> >> +    if ( cache )
> >> +    {
> >> +        cache->num_ents = 0;
> >> +        cache->max_ents = nents;
> >> +    }
> >> +
> >> +    return cache;
> >> +}
> >> +
> >>  bool hvmemul_read_cache(const struct hvmemul_cache *cache,
> paddr_t
> >> gpa,
> >>                          unsigned int level, void *buffer, unsigned int 
> >> size)
> >>  {
> >> +    unsigned int i;
> >> +
> >
> > Here you could return false if cache is NULL...
> 
> This one could perhaps be considered, but ...
> 
> >> @@ -2674,6 +2721,35 @@ void hvmemul_write_cache(struct hvmemul_
> >>                           unsigned int level, const void *buffer,
> >>                           unsigned int size)
> >>  {
> >> +    unsigned int i;
> >> +
> >
> > ...and here just bail out. Thus making both functions safe to call with a
> > NULL cache.
> 
> ... I'm pretty much opposed to this: The term "cache" might be slightly
> confusing here, but I lack a better idea for a name. Its presence is
> required for correctness. After all the series is not a performance
> improvement, but a plain bug fix (generalizing what we were able to
> special case for the actually observed problem in commit 91afb8139f
> ["x86/HVM: suppress I/O completion for port output"]). And with
> that I'd rather leave the read side as is as well.
> 

Ok. I have no strong objection to the code structure as it stands so you can 
add my R-b to this and patch #2.

  Paul

> Jan
> 


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